[PATCH v3 0/4] FXLS8967AF and FXLS8974CF support

[PATCH v3 0/4] FXLS8967AF and FXLS8974CF support

[Han Xu]

The patch set add two more NXP accelerometer entries for FXLS8967AF and
FXLS8974CF in driver and update related compatible string for dt bindings.

Haibo Chen (1):
  iio: accel: add fxls8974cf support

Han Xu (3):
  iio: accel: add the new entry in driver for fxls8967af
  dt-bindings: iio: accel: fxls8962af: add new compatible string
  dt-bindings: iio: accel: fxls8974cf: add new compatible string

 .../bindings/iio/accel/nxp,fxls8962af.yaml         |  6 ++++++
 drivers/iio/accel/fxls8962af-core.c                | 14 ++++++++++++++
 drivers/iio/accel/fxls8962af-i2c.c                 |  4 ++++
 drivers/iio/accel/fxls8962af.h                     |  2 ++
 4 files changed, 26 insertions(+)

-- 
2.25.1

[PATCH v3 1/4] iio: accel: add fxls8974cf support

[Han Xu]

From: Haibo Chen <haibo.chen@nxp.com>

FXLS8974CF is similar with FXLS8962AF, the only difference is the
device id change to 0x86.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
Reviewed-by: Clark Wang <xiaoning.wang@nxp.com>
---
 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 0d672b1469e8..98811e4e16bb 100644
--- a/drivers/iio/accel/fxls8962af-core.c
+++ b/drivers/iio/accel/fxls8962af-core.c
@@ -126,6 +126,7 @@
 
 #define FXLS8962AF_DEVICE_ID			0x62
 #define FXLS8964AF_DEVICE_ID			0x84
+#define FXLS8974CF_DEVICE_ID			0x86
 
 /* Raw temp channel offset */
 #define FXLS8962AF_TEMP_CENTER_VAL		25
@@ -764,6 +765,12 @@ static const struct fxls8962af_chip_info fxls_chip_info_table[] = {
 		.channels = fxls8962af_channels,
 		.num_channels = ARRAY_SIZE(fxls8962af_channels),
 	},
+	[fxls8974cf] = {
+		.chip_id = FXLS8974CF_DEVICE_ID,
+		.name = "fxls8974cf",
+		.channels = fxls8962af_channels,
+		.num_channels = ARRAY_SIZE(fxls8962af_channels),
+	},
 };
 
 static const struct iio_info fxls8962af_info = {
diff --git a/drivers/iio/accel/fxls8962af-i2c.c b/drivers/iio/accel/fxls8962af-i2c.c
index 22640eaebac7..17dd56756ff9 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 },
+	{ "fxls8974cf", fxls8974cf },
 	{}
 };
 MODULE_DEVICE_TABLE(i2c, fxls8962af_id);
@@ -37,6 +38,7 @@ MODULE_DEVICE_TABLE(i2c, fxls8962af_id);
 static const struct of_device_id fxls8962af_of_match[] = {
 	{ .compatible = "nxp,fxls8962af" },
 	{ .compatible = "nxp,fxls8964af" },
+	{ .compatible = "nxp,fxls8974cf" },
 	{}
 };
 MODULE_DEVICE_TABLE(of, fxls8962af_of_match);
diff --git a/drivers/iio/accel/fxls8962af.h b/drivers/iio/accel/fxls8962af.h
index 9cbe98c3ba9a..45c7e57412e0 100644
--- a/drivers/iio/accel/fxls8962af.h
+++ b/drivers/iio/accel/fxls8962af.h
@@ -11,6 +11,7 @@ struct device;
 enum {
 	fxls8962af,
 	fxls8964af,
+	fxls8974cf,
 };
 
 int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq);
-- 
2.25.1

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

[Han Xu]

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" },
 	{}
 };
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,
 };
 
-- 
2.25.1

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

[Han Xu]

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" },
 	{}
 };
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,
 };
 
-- 
2.25.1

[PATCH v3 3/4] dt-bindings: iio: accel: fxls8962af: add new compatible string

[Han Xu]

Add new compatible string for the NXP FXLS8967AF accelerometer sensor.

Signed-off-by: Han Xu <han.xu@nxp.com>

---
changes in v3
- Start commit message in capital
- Describe all these chips are compatible
---
 .../devicetree/bindings/iio/accel/nxp,fxls8962af.yaml         | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml b/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
index 65ce8ea14b52..8f07ade21abb 100644
--- a/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
@@ -14,12 +14,16 @@ description: |
   SPI and I2C interface.
     https://www.nxp.com/docs/en/data-sheet/FXLS8962AF.pdf
     https://www.nxp.com/docs/en/data-sheet/FXLS8964AF.pdf
+    https://www.nxp.com/docs/en/data-sheet/FXLS8967AF.pdf
 
 properties:
   compatible:
+    description:
+      These chips are compatible with each other, just have different IDs.
     enum:
       - nxp,fxls8962af
       - nxp,fxls8964af
+      - nxp,fxls8967af
 
   reg:
     maxItems: 1
-- 
2.25.1

[PATCH v3 4/4] dt-bindings: iio: accel: fxls8974cf: add new compatible string

[Han Xu]

Add new compatible string for the NXP FXLS8974CF accelerometer sensor.

Signed-off-by: Han Xu <han.xu@nxp.com>

---
changes in v3:
- Start commit message in capital
---
 Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml b/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
index 8f07ade21abb..34c452e04fe0 100644
--- a/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
@@ -15,6 +15,7 @@ description: |
     https://www.nxp.com/docs/en/data-sheet/FXLS8962AF.pdf
     https://www.nxp.com/docs/en/data-sheet/FXLS8964AF.pdf
     https://www.nxp.com/docs/en/data-sheet/FXLS8967AF.pdf
+    https://www.nxp.com/docs/en/data-sheet/FXLS8974CF.pdf
 
 properties:
   compatible:
@@ -24,6 +25,7 @@ properties:
       - nxp,fxls8962af
       - nxp,fxls8964af
       - nxp,fxls8967af
+      - nxp,fxls8974cf
 
   reg:
     maxItems: 1
-- 
2.25.1

Re: [PATCH v3 0/4] FXLS8967AF and FXLS8974CF support

[Krzysztof Kozlowski]

On 13/12/2022 18:15, Han Xu wrote:
> The patch set add two more NXP accelerometer entries for FXLS8967AF and
> FXLS8974CF in driver and update related compatible string for dt bindings.
> 
> Haibo Chen (1):
>   iio: accel: add fxls8974cf support
> 
> Han Xu (3):
>   iio: accel: add the new entry in driver for fxls8967af
>   dt-bindings: iio: accel: fxls8962af: add new compatible string
>   dt-bindings: iio: accel: fxls8974cf: add new compatible string

Where is the changelog?

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

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

?? This is v1 attached here. There is also "iio: accel: add the new
entry in driver for fxls8967af"? This patchset cannot be applied, when
sent like this.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

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

Re: [PATCH v3 3/4] dt-bindings: iio: accel: fxls8962af: add new compatible string

[Krzysztof Kozlowski]

On 13/12/2022 18:15, Han Xu wrote:
> Add new compatible string for the NXP FXLS8967AF accelerometer sensor.
> 
> Signed-off-by: Han Xu <han.xu@nxp.com>
> 
> ---
> changes in v3
> - Start commit message in capital
> - Describe all these chips are compatible
> ---
>  .../devicetree/bindings/iio/accel/nxp,fxls8962af.yaml         | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml b/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
> index 65ce8ea14b52..8f07ade21abb 100644
> --- a/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
> +++ b/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
> @@ -14,12 +14,16 @@ description: |
>    SPI and I2C interface.
>      https://www.nxp.com/docs/en/data-sheet/FXLS8962AF.pdf
>      https://www.nxp.com/docs/en/data-sheet/FXLS8964AF.pdf
> +    https://www.nxp.com/docs/en/data-sheet/FXLS8967AF.pdf
>  
>  properties:
>    compatible:
> +    description:
> +      These chips are compatible with each other, just have different IDs.

That's not what I meant.

Compatibility is expressed (check DT spec) by following specific
compatible with a more generic one (fallback), so in DTS:

	compatible = "nxp,fxls8967af", "nxp,fxls8964af";

Your driver changes partially suggested that (and driver changes should
be probably skipped in such case), but now I wonder how it is in reality....

Best regards,
Krzysztof

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

[Jonathan Cameron]

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
> 

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

[Krzysztof Kozlowski]

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

Re: [PATCH v3 3/4] dt-bindings: iio: accel: fxls8962af: add new compatible string

[Krzysztof Kozlowski]

On 13/12/2022 18:15, Han Xu wrote:
> Add new compatible string for the NXP FXLS8967AF accelerometer sensor.
> 
> Signed-off-by: Han Xu <han.xu@nxp.com>
> 
> ---
> changes in v3
> - Start commit message in capital
> - Describe all these chips are compatible
> ---
>  .../devicetree/bindings/iio/accel/nxp,fxls8962af.yaml         | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml b/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
> index 65ce8ea14b52..8f07ade21abb 100644
> --- a/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
> +++ b/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
> @@ -14,12 +14,16 @@ description: |
>    SPI and I2C interface.
>      https://www.nxp.com/docs/en/data-sheet/FXLS8962AF.pdf
>      https://www.nxp.com/docs/en/data-sheet/FXLS8964AF.pdf
> +    https://www.nxp.com/docs/en/data-sheet/FXLS8967AF.pdf
>  
>  properties:
>    compatible:
> +    description:
> +      These chips are compatible with each other, just have different IDs.

As pointed in other mail, the chips are not compatible, so drop the comment.

Best regards,
Krzysztof

Re: [PATCH v3 4/4] dt-bindings: iio: accel: fxls8974cf: add new compatible string

[Krzysztof Kozlowski]

On 13/12/2022 18:15, Han Xu wrote:
> Add new compatible string for the NXP FXLS8974CF accelerometer sensor.
> 
> Signed-off-by: Han Xu <han.xu@nxp.com>

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v3 3/4] dt-bindings: iio: accel: fxls8962af: add new compatible string

[Jonathan Cameron]

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

> On 13/12/2022 18:15, Han Xu wrote:
> > Add new compatible string for the NXP FXLS8967AF accelerometer sensor.
> > 
> > Signed-off-by: Han Xu <han.xu@nxp.com>
> > 
> > ---
> > changes in v3
> > - Start commit message in capital
> > - Describe all these chips are compatible
> > ---
> >  .../devicetree/bindings/iio/accel/nxp,fxls8962af.yaml         | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml b/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
> > index 65ce8ea14b52..8f07ade21abb 100644
> > --- a/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
> > +++ b/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
> > @@ -14,12 +14,16 @@ description: |
> >    SPI and I2C interface.
> >      https://www.nxp.com/docs/en/data-sheet/FXLS8962AF.pdf
> >      https://www.nxp.com/docs/en/data-sheet/FXLS8964AF.pdf
> > +    https://www.nxp.com/docs/en/data-sheet/FXLS8967AF.pdf
> >  
> >  properties:
> >    compatible:
> > +    description:
> > +      These chips are compatible with each other, just have different IDs.  
> 
> As pointed in other mail, the chips are not compatible, so drop the comment.

The difference is an ID register.  Given we have had a bunch of cases of board
manufacturers swapping parts that aren't always compatible we have an old
pattern that we are fixing of rejecting unmatched who am I registers.

This driver should be relaxed to just print a message when the value doesn't
match - that was the compromise we reached with still pointing out possible
compatibility problems, whilst assuming the dts is correct.
Even better would be to make it a little more clever so it doesn't bother moaning
about part changes if the driver knows the new ID is compatible.

Once that's done the comment would reflect how we treat it in the driver
(which shouldn't matter to the binding anyway).

I'm not entirely sure why this driver has handling to allow for different channel
sets, but assume there are more parts to be upstreamed where that flexibility will
be useful.

Jonathan



> 
> Best regards,
> Krzysztof
> 

Re: [PATCH v3 1/4] iio: accel: add fxls8974cf support

[Jonathan Cameron]

On Tue, 13 Dec 2022 11:15:32 -0600
Han Xu <han.xu@nxp.com> wrote:

> From: Haibo Chen <haibo.chen@nxp.com>
> 
> FXLS8974CF is similar with FXLS8962AF, the only difference is the
> device id change to 0x86.
> 
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> Reviewed-by: Clark Wang <xiaoning.wang@nxp.com>
As per the discussion on bindings, we should establish
1) Is this driver going to support devices that have different channel descriptions
   If not - rip that support out as it is misleading.
2) Drop the error return if we don't match IDs.  Assume that it is infact yet another
   compatible part and use whatever it was said to be compatible with.
   If they are all compatible, just use the first element for this - things
   get more complex if there are multiple sets of compatible parts.

either way, we should drop the driver_data in fxls8962af_id[] as it's not used
anyway and that will avoid the complexity of the dt binding discussion.

> ---
>  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 0d672b1469e8..98811e4e16bb 100644
> --- a/drivers/iio/accel/fxls8962af-core.c
> +++ b/drivers/iio/accel/fxls8962af-core.c
> @@ -126,6 +126,7 @@
>  
>  #define FXLS8962AF_DEVICE_ID			0x62
>  #define FXLS8964AF_DEVICE_ID			0x84
> +#define FXLS8974CF_DEVICE_ID			0x86
>  
>  /* Raw temp channel offset */
>  #define FXLS8962AF_TEMP_CENTER_VAL		25
> @@ -764,6 +765,12 @@ static const struct fxls8962af_chip_info fxls_chip_info_table[] = {
>  		.channels = fxls8962af_channels,
>  		.num_channels = ARRAY_SIZE(fxls8962af_channels),
>  	},
> +	[fxls8974cf] = {
> +		.chip_id = FXLS8974CF_DEVICE_ID,
> +		.name = "fxls8974cf",
> +		.channels = fxls8962af_channels,
> +		.num_channels = ARRAY_SIZE(fxls8962af_channels),
> +	},
>  };
>  
>  static const struct iio_info fxls8962af_info = {
> diff --git a/drivers/iio/accel/fxls8962af-i2c.c b/drivers/iio/accel/fxls8962af-i2c.c
> index 22640eaebac7..17dd56756ff9 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 },
> +	{ "fxls8974cf", fxls8974cf },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(i2c, fxls8962af_id);
> @@ -37,6 +38,7 @@ MODULE_DEVICE_TABLE(i2c, fxls8962af_id);
>  static const struct of_device_id fxls8962af_of_match[] = {
>  	{ .compatible = "nxp,fxls8962af" },
>  	{ .compatible = "nxp,fxls8964af" },
> +	{ .compatible = "nxp,fxls8974cf" },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, fxls8962af_of_match);
> diff --git a/drivers/iio/accel/fxls8962af.h b/drivers/iio/accel/fxls8962af.h
> index 9cbe98c3ba9a..45c7e57412e0 100644
> --- a/drivers/iio/accel/fxls8962af.h
> +++ b/drivers/iio/accel/fxls8962af.h
> @@ -11,6 +11,7 @@ struct device;
>  enum {
>  	fxls8962af,
>  	fxls8964af,
> +	fxls8974cf,
>  };
>  
>  int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq);

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

[Jonathan Cameron]

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,
>  };
>  

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

[Jonathan Cameron]

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
>