Re: [PATCH v3 2/4] iio: accel: add the new entry in driver for fxls8967af

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 14 Dec 2022 10:54:00 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 14/12/2022 10:32, Jonathan Cameron wrote:
> > On Tue, 13 Dec 2022 19:53:30 +0100
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >   
> >> On 13/12/2022 18:15, Han Xu wrote:  
> >>> Add this new device entry in the driver id table.
> >>>
> >>> Signed-off-by: Han Xu <han.xu@nxp.com>
> >>>
> >>> ---
> >>> changes in v2
> >>> - change chip info orders
> >>> ---
> >>>  drivers/iio/accel/fxls8962af-core.c | 7 +++++++
> >>>  drivers/iio/accel/fxls8962af-i2c.c  | 2 ++
> >>>  drivers/iio/accel/fxls8962af.h      | 1 +
> >>>  3 files changed, 10 insertions(+)
> >>>
> >>> diff --git a/drivers/iio/accel/fxls8962af-core.c b/drivers/iio/accel/fxls8962af-core.c
> >>> index 98811e4e16bb..c3589c3084ee 100644
> >>> --- a/drivers/iio/accel/fxls8962af-core.c
> >>> +++ b/drivers/iio/accel/fxls8962af-core.c
> >>> @@ -127,6 +127,7 @@
> >>>  #define FXLS8962AF_DEVICE_ID			0x62
> >>>  #define FXLS8964AF_DEVICE_ID			0x84
> >>>  #define FXLS8974CF_DEVICE_ID			0x86
> >>> +#define FXLS8967AF_DEVICE_ID			0x87
> >>>  
> >>>  /* Raw temp channel offset */
> >>>  #define FXLS8962AF_TEMP_CENTER_VAL		25
> >>> @@ -765,6 +766,12 @@ static const struct fxls8962af_chip_info fxls_chip_info_table[] = {
> >>>  		.channels = fxls8962af_channels,
> >>>  		.num_channels = ARRAY_SIZE(fxls8962af_channels),
> >>>  	},
> >>> +	[fxls8967af] = {
> >>> +		.chip_id = FXLS8967AF_DEVICE_ID,
> >>> +		.name = "fxls8967af",
> >>> +		.channels = fxls8962af_channels,
> >>> +		.num_channels = ARRAY_SIZE(fxls8962af_channels),
> >>> +	},
> >>>  	[fxls8974cf] = {
> >>>  		.chip_id = FXLS8974CF_DEVICE_ID,
> >>>  		.name = "fxls8974cf",
> >>> diff --git a/drivers/iio/accel/fxls8962af-i2c.c b/drivers/iio/accel/fxls8962af-i2c.c
> >>> index 17dd56756ff9..a8944b255a28 100644
> >>> --- a/drivers/iio/accel/fxls8962af-i2c.c
> >>> +++ b/drivers/iio/accel/fxls8962af-i2c.c
> >>> @@ -30,6 +30,7 @@ static int fxls8962af_probe(struct i2c_client *client)
> >>>  static const struct i2c_device_id fxls8962af_id[] = {
> >>>  	{ "fxls8962af", fxls8962af },
> >>>  	{ "fxls8964af", fxls8964af },
> >>> +	{ "fxls8967af", fxls8967af },
> >>>  	{ "fxls8974cf", fxls8974cf },
> >>>  	{}
> >>>  };
> >>> @@ -38,6 +39,7 @@ MODULE_DEVICE_TABLE(i2c, fxls8962af_id);
> >>>  static const struct of_device_id fxls8962af_of_match[] = {
> >>>  	{ .compatible = "nxp,fxls8962af" },
> >>>  	{ .compatible = "nxp,fxls8964af" },
> >>> +	{ .compatible = "nxp,fxls8967af" },
> >>>  	{ .compatible = "nxp,fxls8974cf" },    
> >>
> >> This is confusing. The I2C ID table has driver data, but OF ID table
> >> hasn't. So are they compatible or not?  
> > 
> > Due to some evilness in i2c that 'works' as long as the two arrays have
> > matching entries.  As a general rule we prefer to have the data in both, check
> > the firmware table first and only then fallback to i2c_device_id data on the
> > basis it is less fragile.
> > 
> > The evilness in i2c is that the search for match data will use the dt compatible
> > stripped of the vendor prefix and string match that against the i2c_device_id table.
> > 
> > Nice to clean this up, but not necessarily in this series (fine if it is though!)  
> 
> OK, so in fact devices are not fully compatible - I got mislead by OF
> table. I'll comment in bindings about it.

I actually took a look at the driver today. It's not using the driver_data anyway.
It does the better option of searching for a match based on the WHO_AM_I anyway.
So best option is a precursor patch dropping the driver_data from the i2c_device_id
table.
 
Oops. I was guilty of making assumptions and didn't previously check. It could have
been used as described, but wasn't.

> 
> Best regards,
> Krzysztof
>