| Message ID | 20221213171536.1880089-3-han.xu@nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | FXLS8967AF and FXLS8974CF support | expand |
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? Best regards, Krzysztof
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!) Jonathan > > Best regards, > Krzysztof >
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. Best regards, Krzysztof
On Tue, 13 Dec 2022 11:15:33 -0600 Han Xu <han.xu@nxp.com> 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), So far all the channels /num_channels in these have pointed to same place. Not a lot of point in having the complexity if it's not used. I'd like to see that dropped unless we know it is going to be used in a patch set you expect to post soon after this one. > + }, > [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 }, As in previous patch, driver_data isn't used anyway so drop it. > { "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" }, > {} > }; > diff --git a/drivers/iio/accel/fxls8962af.h b/drivers/iio/accel/fxls8962af.h > index 45c7e57412e0..ba33eb2b807d 100644 > --- a/drivers/iio/accel/fxls8962af.h > +++ b/drivers/iio/accel/fxls8962af.h > @@ -11,6 +11,7 @@ struct device; > enum { > fxls8962af, > fxls8964af, > + fxls8967af, > fxls8974cf, > }; >
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 >
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" }, {} }; diff --git a/drivers/iio/accel/fxls8962af.h b/drivers/iio/accel/fxls8962af.h index 45c7e57412e0..ba33eb2b807d 100644 --- a/drivers/iio/accel/fxls8962af.h +++ b/drivers/iio/accel/fxls8962af.h @@ -11,6 +11,7 @@ struct device; enum { fxls8962af, fxls8964af, + fxls8967af, fxls8974cf, };
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(+)