[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-i2c
Subject:    Re: [PATCH v2 8/9] i2c: i801: Improve register_dell_lis3lv02d_i2c_device
From:       Jean Delvare <jdelvare () suse ! de>
Date:       2021-08-27 16:21:35
Message-ID: 20210827182135.1d9670c0 () endymion
[Download RAW message or body]

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
[prev in list] [next in list] [prev in thread] [next in thread]