| Message ID | 3ce2ea87-809e-5999-e920-07ddd5fcc035@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: i801: Series with improvements | expand |
On Fri, Aug 06, 2021 at 11:18:05PM +0200, Heiner Kallweit wrote: > - Use an initializer for struct i2c_board_info info > - Use dmi_match() > - Simplify loop logic I'm wondering if changing this to a DMI match table will give better result. Something like (Sorry I forgot APIs, but plenty of examples are under PDx86: drivers/platform/x86): struct dmi_..._id *id; id = dmi_..._match(); if (!id) { pci_warn(); return; } i2c_new_client_device(...); -- With Best Regards, Andy Shevchenko
On 11.08.2021 17:52, Andy Shevchenko wrote: > On Fri, Aug 06, 2021 at 11:18:05PM +0200, Heiner Kallweit wrote: >> - Use an initializer for struct i2c_board_info info >> - Use dmi_match() >> - Simplify loop logic > > I'm wondering if changing this to a DMI match table will give better result. > > Something like > (Sorry I forgot APIs, but plenty of examples are under PDx86: drivers/platform/x86): > > struct dmi_..._id *id; > > id = dmi_..._match(); > if (!id) { > pci_warn(); > return; > } > > i2c_new_client_device(...); > > We could do something like the following. Whether it's better may be a question of personal taste. I have no strong opinion here and would leave it to Jean. const struct dmi_system_id lis3_id_table[] = { { .driver_data = (void *)0x29, .matches = { DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"), }, }, ... dmi_system_id *id = dmi_first_match(lis3_id_table); if (id) i2c_new_client_device(..., (unsigned int)id->driver_data; else lament()
On Wed, Aug 11, 2021 at 11:05:06PM +0200, Heiner Kallweit wrote: > On 11.08.2021 17:52, Andy Shevchenko wrote: > > On Fri, Aug 06, 2021 at 11:18:05PM +0200, Heiner Kallweit wrote: > >> - Use an initializer for struct i2c_board_info info > >> - Use dmi_match() > >> - Simplify loop logic > > > > I'm wondering if changing this to a DMI match table will give better result. > > > > Something like > > (Sorry I forgot APIs, but plenty of examples are under PDx86: drivers/platform/x86): > > > > struct dmi_..._id *id; > > > > id = dmi_..._match(); > > if (!id) { > > pci_warn(); > > return; > > } > > > > i2c_new_client_device(...); > > > We could do something like the following. Whether it's better may be a > question of personal taste. I have no strong opinion here and would leave > it to Jean. > > const struct dmi_system_id lis3_id_table[] = { > { > .driver_data = (void *)0x29, > .matches = { > DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"), > }, > }, > ... > > dmi_system_id *id = dmi_first_match(lis3_id_table); > if (id) > i2c_new_client_device(..., (unsigned int)id->driver_data; > else > lament() Yep, my point here that this has less indentation of the code, no unneeded for-loop (which will be inside DMI APIs, etc). But yes, I agree that this is rather matter of taste. -- With Best Regards, Andy Shevchenko
Hi Heiner, Andy, On Wed, 11 Aug 2021 23:05:06 +0200, Heiner Kallweit wrote: > On 11.08.2021 17:52, Andy Shevchenko wrote: > > On Fri, Aug 06, 2021 at 11:18:05PM +0200, Heiner Kallweit wrote: > >> - Use an initializer for struct i2c_board_info info > >> - Use dmi_match() > >> - Simplify loop logic > > > > I'm wondering if changing this to a DMI match table will give better result. > > > > Something like > > (Sorry I forgot APIs, but plenty of examples are under PDx86: drivers/platform/x86): > > > > struct dmi_..._id *id; > > > > id = dmi_..._match(); > > if (!id) { > > pci_warn(); > > return; > > } > > > > i2c_new_client_device(...); > > We could do something like the following. Whether it's better may be a > question of personal taste. I have no strong opinion here and would leave > it to Jean. > > const struct dmi_system_id lis3_id_table[] = { > { > .driver_data = (void *)0x29, > .matches = { > DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"), > }, > }, > ... > > dmi_system_id *id = dmi_first_match(lis3_id_table); > if (id) > i2c_new_client_device(..., (unsigned int)id->driver_data; > else > lament() > I gave it an actual try, and must say I'm not convinced. Heiner's patch is -6 lines of source code and +1 byte on the binary (according to scripts/bloat-o-meter - ls -l disagrees, don't ask me why). My suggested alternative (as discussed in the v1 of this patch set, basically Heiner's patch minus the removal of dmi_product_name) is -3 lines of source code and +17 bytes on the binary, but should be faster than Heiner's version. Andy's approach results in an overall increase of the source code by 29 lines and +2582 bytes on the binary. Sure, if you break it down, it's +2624 data and -42 code, so it does "simplify the code", but that's too high a price to pay for a marginal code simplification. It also has the downside of reintroducing a cast from int to pointer and back, something we were trying to get rid of in another patch of this series. This could of course be avoided but this would make the patch even larger. So thanks, but no thanks. Just because an API exists does not mean you have to use it in all cases. I stand on my original position, let's stick to dmi_get_system_info() + strcmp() as the driver did originally. In other words, don't change code that has been working for years when the alternatives bring no clear benefit. Thanks, -- Jean Delvare SUSE L3 Support
> I stand on my original position, let's stick to dmi_get_system_info() + > strcmp() as the driver did originally. In other words, don't change > code that has been working for years when the alternatives bring no > clear benefit. So, I can skip this patch and continue with patch 9?
On Wed, 29 Sep 2021 22:11:20 +0200, Wolfram Sang wrote: > > I stand on my original position, let's stick to dmi_get_system_info() + > > strcmp() as the driver did originally. In other words, don't change > > code that has been working for years when the alternatives bring no > > clear benefit. > > So, I can skip this patch and continue with patch 9? Yes please. There may be a subset of it that can still be applied, we can look into it once the dust has settled. -- Jean Delvare SUSE L3 Support
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 6e9aca81b..88745e3bc 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1247,28 +1247,22 @@ static const struct { static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv) { - struct i2c_board_info info; - const char *dmi_product_name; int i; - dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME); for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) { - if (strcmp(dmi_product_name, - dell_lis3lv02d_devices[i].dmi_product_name) == 0) - break; - } - - if (i == ARRAY_SIZE(dell_lis3lv02d_devices)) { - dev_warn(&priv->pci_dev->dev, - "Accelerometer lis3lv02d is present on SMBus but its" - " address is unknown, skipping registration\n"); - return; + if (dmi_match(DMI_PRODUCT_NAME, dell_lis3lv02d_devices[i].dmi_product_name)) { + struct i2c_board_info info = { + .type = "lis3lv02d", + .addr = dell_lis3lv02d_devices[i].i2c_addr, + }; + + i2c_new_client_device(&priv->adapter, &info); + return; + } } - memset(&info, 0, sizeof(struct i2c_board_info)); - info.addr = dell_lis3lv02d_devices[i].i2c_addr; - strlcpy(info.type, "lis3lv02d", I2C_NAME_SIZE); - i2c_new_client_device(&priv->adapter, &info); + pci_warn(priv->pci_dev, + "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n"); } /* Register optional slaves */
- Use an initializer for struct i2c_board_info info - Use dmi_match() - Simplify loop logic Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- v2: - move definition of struct i2c_board_info info to inner loop - add missing curly braces to outer for loop --- drivers/i2c/busses/i2c-i801.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-)