[PATCH v3 0/3] Add the Renesas X9250 potentiometers IIO support

[PATCH v3 0/3] Add the Renesas X9250 potentiometers IIO support

[Herve Codina]

Hi,

The Renesas X9250 integrated four digitally controlled potentiometers.
On each potentiometer, the X9250T has a 100 kOhms total resistance and
the X9250U has a 50 kOhms total resistance.

Compare to the previous iteration
  https://lore.kernel.org/linux-kernel/20230420161052.270366-1-herve.codina@bootlin.com/
This v3 series updates the binding.

Best regards,
Herve Codina

Changes v2 -> v3
  - Patch 1
    Remove the reg property description
    Use 'potentiometer' for the node name in the example.

  - Patch 2 and 3
    No changes

Changes v1 -> v2
  - Patch 1
    No changes

  - Patch 2
    Use a define for the 0x50 value used multiple times.

  - Patch 3
    No changes

Herve Codina (3):
  dt-bindings: iio: potentiometer: Add the Renesas X9250 potentiometers
  iio: potentiometer: Add support for the Renesas X9250 potentiometers
  MAINTAINERS: add the Renesas X9250 driver entry

 .../iio/potentiometer/renesas,x9250.yaml      |  54 ++++
 MAINTAINERS                                   |   7 +
 drivers/iio/potentiometer/Kconfig             |  10 +
 drivers/iio/potentiometer/Makefile            |   1 +
 drivers/iio/potentiometer/x9250.c             | 234 ++++++++++++++++++
 5 files changed, 306 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml
 create mode 100644 drivers/iio/potentiometer/x9250.c

-- 
2.39.2

[PATCH v3 1/3] dt-bindings: iio: potentiometer: Add the Renesas X9250 potentiometers

[Herve Codina]

The Renesas X9250 is a quad digitally controlled potentiometers.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 .../iio/potentiometer/renesas,x9250.yaml      | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml

diff --git a/Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml b/Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml
new file mode 100644
index 000000000000..dfa36b23eb0d
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/potentiometer/renesas,x9250.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas X9250 quad potentiometers
+
+maintainers:
+  - Herve Codina <herve.codina@bootlin.com>
+
+description:
+  The Renesas X9250 integrates four digitally controlled potentiometers.
+  On each potentiometer, the X9250T has a 100 kOhms total resistance and the
+  X9250U has a 50 kOhms total resistance.
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml
+  - $ref: /schemas/iio/iio.yaml
+
+properties:
+  compatible:
+    enum:
+      - renesas,x9250t
+      - renesas,x9250u
+
+  reg:
+    maxItems: 1
+
+  '#io-channel-cells':
+    const: 1
+
+  spi-max-frequency:
+    maximum: 2000000
+
+required:
+  - compatible
+  - reg
+  - '#io-channel-cells'
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        potentiometer@0 {
+            compatible = "renesas,x9250t";
+            reg = <0>;
+            spi-max-frequency = <2000000>;
+            #io-channel-cells = <1>;
+        };
+    };
-- 
2.39.2

Re: [PATCH v3 1/3] dt-bindings: iio: potentiometer: Add the Renesas X9250 potentiometers

[Krzysztof Kozlowski]

On 21/04/2023 10:52, Herve Codina wrote:
> The Renesas X9250 is a quad digitally controlled potentiometers.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  .../iio/potentiometer/renesas,x9250.yaml      | 54 +++++++++++++++++++
>  1 file changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml b/Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml
> new file mode 100644
> index 000000000000..dfa36b23eb0d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml
> @@ -0,0 +1,54 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/potentiometer/renesas,x9250.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas X9250 quad potentiometers
> +
> +maintainers:
> +  - Herve Codina <herve.codina@bootlin.com>
> +
> +description:
> +  The Renesas X9250 integrates four digitally controlled potentiometers.
> +  On each potentiometer, the X9250T has a 100 kOhms total resistance and the
> +  X9250U has a 50 kOhms total resistance.
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml
> +  - $ref: /schemas/iio/iio.yaml

Apologies, I missed it last time - you do not need iio.yaml. It's coming
from core schema and is always selected. You won't find its usage
anywhere in the kernel (git grep iio.yaml)

With iio.yaml dropped:

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

Best regards,
Krzysztof

Re: [PATCH v3 1/3] dt-bindings: iio: potentiometer: Add the Renesas X9250 potentiometers

[Jonathan Cameron]

On Fri, 21 Apr 2023 10:52:43 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> The Renesas X9250 is a quad digitally controlled potentiometers.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>

Hi Herve,

Historically we've been a bit lax in IIO bindings in always making
sure the per supplies are included.  As a result we frequently get
them added later and it just makes things messier than they should
be.

So please add vcc-supply from the start.  V+ and V- are a little trickier.
I was expecting datasheet to say they should be symmetric about 0 but it
doesn't. So they could be two independent supplies.

Also make it required as my current understanding is that we should
do that for supplies that are definitely present even if we could
rely on the fallback to regulator stubs if they aren't supplied.
So add the 3 supplies to required as well.

Less of a requirement, but you might want to also provide an optional 
gpio for the not WP pin on basis someone might wire it up to the host processor.

Beyond the comment Krzystof made on iio.yaml this otherwise looks good to me.

Thanks,

Jonathan



> ---
>  .../iio/potentiometer/renesas,x9250.yaml      | 54 +++++++++++++++++++
>  1 file changed, 54 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml b/Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml
> new file mode 100644
> index 000000000000..dfa36b23eb0d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml
> @@ -0,0 +1,54 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/potentiometer/renesas,x9250.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas X9250 quad potentiometers
> +
> +maintainers:
> +  - Herve Codina <herve.codina@bootlin.com>
> +
> +description:
> +  The Renesas X9250 integrates four digitally controlled potentiometers.
> +  On each potentiometer, the X9250T has a 100 kOhms total resistance and the
> +  X9250U has a 50 kOhms total resistance.
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml
> +  - $ref: /schemas/iio/iio.yaml
> +
> +properties:
> +  compatible:
> +    enum:
> +      - renesas,x9250t
> +      - renesas,x9250u
> +
> +  reg:
> +    maxItems: 1
> +
> +  '#io-channel-cells':
> +    const: 1
> +
> +  spi-max-frequency:
> +    maximum: 2000000
> +
> +required:
> +  - compatible
> +  - reg
> +  - '#io-channel-cells'
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +        potentiometer@0 {
> +            compatible = "renesas,x9250t";
> +            reg = <0>;
> +            spi-max-frequency = <2000000>;
> +            #io-channel-cells = <1>;
> +        };
> +    };

Re: [PATCH v3 1/3] dt-bindings: iio: potentiometer: Add the Renesas X9250 potentiometers

[Herve Codina]

Hi Jonathan, Krzysztof,

On Sat, 22 Apr 2023 17:18:07 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 21 Apr 2023 10:52:43 +0200
> Herve Codina <herve.codina@bootlin.com> wrote:
> 
> > The Renesas X9250 is a quad digitally controlled potentiometers.
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>  
> 
> Hi Herve,
> 
> Historically we've been a bit lax in IIO bindings in always making
> sure the per supplies are included.  As a result we frequently get
> them added later and it just makes things messier than they should
> be.
> 
> So please add vcc-supply from the start.  V+ and V- are a little trickier.
> I was expecting datasheet to say they should be symmetric about 0 but it
> doesn't. So they could be two independent supplies.
> 
> Also make it required as my current understanding is that we should
> do that for supplies that are definitely present even if we could
> rely on the fallback to regulator stubs if they aren't supplied.
> So add the 3 supplies to required as well.

Yes, I will add the following supplies in the next iteration:
 - 'vcc-supply' for VCC
 - 'avp-supply' for the analog V+
 - 'avn-supply' for the analog V-

and add them in the required list of properties.

Are the names correct for these power supplies (avp and avn) ?

> 
> Less of a requirement, but you might want to also provide an optional 
> gpio for the not WP pin on basis someone might wire it up to the host processor.

I will add the 'wp-gpios' property.

> 
> Beyond the comment Krzystof made on iio.yaml this otherwise looks good to me.

And for the Krzystof comment on iio.yaml, as he suggested, I will drop iio.yaml.

Thanks for the review,
Hervé

> 
> 
> 
> 
> > ---
> >  .../iio/potentiometer/renesas,x9250.yaml      | 54 +++++++++++++++++++
> >  1 file changed, 54 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml b/Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml
> > new file mode 100644
> > index 000000000000..dfa36b23eb0d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml
> > @@ -0,0 +1,54 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/potentiometer/renesas,x9250.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Renesas X9250 quad potentiometers
> > +
> > +maintainers:
> > +  - Herve Codina <herve.codina@bootlin.com>
> > +
> > +description:
> > +  The Renesas X9250 integrates four digitally controlled potentiometers.
> > +  On each potentiometer, the X9250T has a 100 kOhms total resistance and the
> > +  X9250U has a 50 kOhms total resistance.
> > +
> > +allOf:
> > +  - $ref: /schemas/spi/spi-peripheral-props.yaml
> > +  - $ref: /schemas/iio/iio.yaml
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - renesas,x9250t
> > +      - renesas,x9250u
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  '#io-channel-cells':
> > +    const: 1
> > +
> > +  spi-max-frequency:
> > +    maximum: 2000000
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - '#io-channel-cells'
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    spi {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +        potentiometer@0 {
> > +            compatible = "renesas,x9250t";
> > +            reg = <0>;
> > +            spi-max-frequency = <2000000>;
> > +            #io-channel-cells = <1>;
> > +        };
> > +    };  
> 

Re: [PATCH v3 1/3] dt-bindings: iio: potentiometer: Add the Renesas X9250 potentiometers

[Jonathan Cameron]

On Mon, 24 Apr 2023 09:03:18 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> Hi Jonathan, Krzysztof,
> 
> On Sat, 22 Apr 2023 17:18:07 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Fri, 21 Apr 2023 10:52:43 +0200
> > Herve Codina <herve.codina@bootlin.com> wrote:
> >   
> > > The Renesas X9250 is a quad digitally controlled potentiometers.
> > > 
> > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>    
> > 
> > Hi Herve,
> > 
> > Historically we've been a bit lax in IIO bindings in always making
> > sure the per supplies are included.  As a result we frequently get
> > them added later and it just makes things messier than they should
> > be.
> > 
> > So please add vcc-supply from the start.  V+ and V- are a little trickier.
> > I was expecting datasheet to say they should be symmetric about 0 but it
> > doesn't. So they could be two independent supplies.
> > 
> > Also make it required as my current understanding is that we should
> > do that for supplies that are definitely present even if we could
> > rely on the fallback to regulator stubs if they aren't supplied.
> > So add the 3 supplies to required as well.  
> 
> Yes, I will add the following supplies in the next iteration:
>  - 'vcc-supply' for VCC
>  - 'avp-supply' for the analog V+
>  - 'avn-supply' for the analog V-
> 
> and add them in the required list of properties.
> 
> Are the names correct for these power supplies (avp and avn) ?

I think so.  I'm not totally sure on how DT maintainers think we should deal
with a two voltage level reference though.  Perhaps add some description to
make it very clear what is going on and we'll see what review comments we get!

Jonathan

[PATCH v3 2/3] iio: potentiometer: Add support for the Renesas X9250 potentiometers

[Herve Codina]

The Renesas X9250 integrates four digitally controlled potentiometers.
On each potentiometer, the X9250T has a 100 kOhms total resistance and
the X9250U has a 50 kOhms total resistance.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/iio/potentiometer/Kconfig  |  10 ++
 drivers/iio/potentiometer/Makefile |   1 +
 drivers/iio/potentiometer/x9250.c  | 234 +++++++++++++++++++++++++++++
 3 files changed, 245 insertions(+)
 create mode 100644 drivers/iio/potentiometer/x9250.c

diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
index 01dd3f858d99..e6a9a3c67845 100644
--- a/drivers/iio/potentiometer/Kconfig
+++ b/drivers/iio/potentiometer/Kconfig
@@ -136,4 +136,14 @@ config TPL0102
 	  To compile this driver as a module, choose M here: the
 	  module will be called tpl0102.
 
+config X9250
+	tristate "Renesas X9250 quad controlled potentiometers"
+	depends on SPI
+	help
+	  Enable support for the Renesas X9250 quad controlled
+	  potentiometers.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called x9250.
+
 endmenu
diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
index 5ebb8e3bbd76..d11fb739176c 100644
--- a/drivers/iio/potentiometer/Makefile
+++ b/drivers/iio/potentiometer/Makefile
@@ -15,3 +15,4 @@ obj-$(CONFIG_MCP4131) += mcp4131.o
 obj-$(CONFIG_MCP4531) += mcp4531.o
 obj-$(CONFIG_MCP41010) += mcp41010.o
 obj-$(CONFIG_TPL0102) += tpl0102.o
+obj-$(CONFIG_X9250) += x9250.o
diff --git a/drivers/iio/potentiometer/x9250.c b/drivers/iio/potentiometer/x9250.c
new file mode 100644
index 000000000000..c6bc959205a3
--- /dev/null
+++ b/drivers/iio/potentiometer/x9250.c
@@ -0,0 +1,234 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ *
+ * x9250.c  --  Renesas X9250 potentiometers IIO driver
+ *
+ * Copyright 2023 CS GROUP France
+ *
+ * Author: Herve Codina <herve.codina@bootlin.com>
+ */
+
+#include <linux/iio/iio.h>
+#include <linux/limits.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+
+struct x9250_cfg {
+	int kohms;
+};
+
+struct x9250 {
+	struct spi_device *spi;
+	const struct x9250_cfg *cfg;
+	struct mutex lock; /* Protect tx and rx buffers */
+	/* Cannot use stack area for SPI (dma-safe memory) */
+	u8 spi_tx_buf[3] __aligned(IIO_DMA_MINALIGN);
+	u8 spi_rx_buf[3] __aligned(IIO_DMA_MINALIGN);
+};
+
+#define X9250_ID		0x50
+#define X9250_CMD_RD_WCR(_p)    (0x90 | (_p))
+#define X9250_CMD_WR_WCR(_p)    (0xa0 | (_p))
+
+static int x9250_write8(struct x9250 *x9250, u8 cmd, u8 val)
+{
+	struct spi_transfer xfer = {
+		.tx_buf = &x9250->spi_tx_buf,
+		.len = 3,
+	};
+	int ret;
+
+	BUILD_BUG_ON(sizeof(x9250->spi_tx_buf) < 3);
+
+	mutex_lock(&x9250->lock);
+
+	x9250->spi_tx_buf[0] = X9250_ID;
+	x9250->spi_tx_buf[1] = cmd;
+	x9250->spi_tx_buf[2] = val;
+
+	ret = spi_sync_transfer(x9250->spi, &xfer, 1);
+
+	mutex_unlock(&x9250->lock);
+
+	return ret;
+}
+
+static int x9250_read8(struct x9250 *x9250, u8 cmd, u8 *val)
+{
+	struct spi_transfer xfer = {
+		.tx_buf = &x9250->spi_tx_buf,
+		.rx_buf = &x9250->spi_rx_buf,
+		.len = 3,
+	};
+	int ret;
+
+	BUILD_BUG_ON(sizeof(x9250->spi_tx_buf) < 3);
+	BUILD_BUG_ON(sizeof(x9250->spi_rx_buf) < 3);
+
+	mutex_lock(&x9250->lock);
+
+	x9250->spi_tx_buf[0] = X9250_ID;
+	x9250->spi_tx_buf[1] = cmd;
+
+	ret = spi_sync_transfer(x9250->spi, &xfer, 1);
+	if (ret)
+		goto end;
+
+	*val = x9250->spi_rx_buf[2];
+	ret = 0;
+end:
+	mutex_unlock(&x9250->lock);
+	return ret;
+}
+
+#define X9250_CHANNEL(ch) {						\
+	.type = IIO_RESISTANCE,						\
+	.indexed = 1,							\
+	.output = 1,							\
+	.channel = (ch),						\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
+	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_RAW),	\
+}
+
+static const struct iio_chan_spec x9250_channels[] = {
+	X9250_CHANNEL(0),
+	X9250_CHANNEL(1),
+	X9250_CHANNEL(2),
+	X9250_CHANNEL(3),
+};
+
+static int x9250_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+			  int *val, int *val2, long mask)
+{
+	struct x9250 *x9250 = iio_priv(indio_dev);
+	int ch = chan->channel;
+	int ret;
+	u8 v;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = x9250_read8(x9250, X9250_CMD_RD_WCR(ch), &v);
+		if (ret)
+			return ret;
+		*val = v;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = 1000 * x9250->cfg->kohms;
+		*val2 = U8_MAX;
+		return IIO_VAL_FRACTIONAL;
+	}
+
+	return -EINVAL;
+}
+
+static int x9250_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+			    const int **vals, int *type, int *length, long mask)
+{
+	static const int range[] = {0, 1, 255}; /* min, step, max */
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		*length = ARRAY_SIZE(range);
+		*vals = range;
+		*type = IIO_VAL_INT;
+		return IIO_AVAIL_RANGE;
+	}
+
+	return -EINVAL;
+}
+
+static int x9250_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
+			   int val, int val2, long mask)
+{
+	struct x9250 *x9250 = iio_priv(indio_dev);
+	int ch = chan->channel;
+
+	if (mask != IIO_CHAN_INFO_RAW)
+		return -EINVAL;
+
+	if (val > U8_MAX || val < 0)
+		return -EINVAL;
+
+	return x9250_write8(x9250, X9250_CMD_WR_WCR(ch), val);
+}
+
+static const struct iio_info x9250_info = {
+	.read_raw = x9250_read_raw,
+	.read_avail = x9250_read_avail,
+	.write_raw = x9250_write_raw,
+};
+
+enum x9250_type {
+	X9250T,
+	X9250U,
+};
+
+static const struct x9250_cfg x9250_cfg[] = {
+	[X9250T] = { .kohms =  100, },
+	[X9250U] = { .kohms =  50, },
+};
+
+static int x9250_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct x9250 *x9250;
+	int ret;
+
+	spi->bits_per_word = 8;
+	ret = spi_setup(spi);
+	if (ret < 0)
+		return ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*x9250));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	x9250 = iio_priv(indio_dev);
+	x9250->spi = spi;
+	x9250->cfg = device_get_match_data(&spi->dev);
+	if (!x9250->cfg)
+		x9250->cfg = &x9250_cfg[spi_get_device_id(spi)->driver_data];
+
+	mutex_init(&x9250->lock);
+
+	indio_dev->info = &x9250_info;
+	indio_dev->channels = x9250_channels;
+	indio_dev->num_channels = ARRAY_SIZE(x9250_channels);
+	indio_dev->name = spi_get_device_id(spi)->name;
+
+	spi_set_drvdata(spi, indio_dev);
+
+	return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct of_device_id x9250_of_match[] = {
+	{ .compatible = "renesas,x9250t", &x9250_cfg[X9250T]},
+	{ .compatible = "renesas,x9250u", &x9250_cfg[X9250U]},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, x9250_of_match);
+
+static const struct spi_device_id x9250_id_table[] = {
+	{ "x9250t", X9250T },
+	{ "x9250u", X9250U },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, x9250_id_table);
+
+static struct spi_driver x9250_spi_driver = {
+	.driver  = {
+		.name   = "x9250",
+		.of_match_table = x9250_of_match,
+	},
+	.id_table = x9250_id_table,
+	.probe  = x9250_probe,
+};
+
+module_spi_driver(x9250_spi_driver);
+
+MODULE_AUTHOR("Herve Codina <herve.codina@bootlin.com>");
+MODULE_DESCRIPTION("X9250 ALSA SoC driver");
+MODULE_LICENSE("GPL");
-- 
2.39.2

Re: [PATCH v3 2/3] iio: potentiometer: Add support for the Renesas X9250 potentiometers

[Jonathan Cameron]

On Fri, 21 Apr 2023 10:52:44 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> The Renesas X9250 integrates four digitally controlled potentiometers.
> On each potentiometer, the X9250T has a 100 kOhms total resistance and
> the X9250U has a 50 kOhms total resistance.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>

Hi Herve,

Nice clean driver.  Comments are mostly about ways to avoid boilerplate
and simplify the code a bit.

Note that the binding comment on regulators should be matched by
appropriate devm_get_regulator_enabled() calls in here to ensure they 
are turned on.

Jonathan

> ---
>  drivers/iio/potentiometer/Kconfig  |  10 ++
>  drivers/iio/potentiometer/Makefile |   1 +
>  drivers/iio/potentiometer/x9250.c  | 234 +++++++++++++++++++++++++++++
>  3 files changed, 245 insertions(+)
>  create mode 100644 drivers/iio/potentiometer/x9250.c
> 
> diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
> index 01dd3f858d99..e6a9a3c67845 100644
> --- a/drivers/iio/potentiometer/Kconfig
> +++ b/drivers/iio/potentiometer/Kconfig
> @@ -136,4 +136,14 @@ config TPL0102
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called tpl0102.
>  
> +config X9250
> +	tristate "Renesas X9250 quad controlled potentiometers"
> +	depends on SPI
> +	help
> +	  Enable support for the Renesas X9250 quad controlled
> +	  potentiometers.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called x9250.
> +
>  endmenu
> diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
> index 5ebb8e3bbd76..d11fb739176c 100644
> --- a/drivers/iio/potentiometer/Makefile
> +++ b/drivers/iio/potentiometer/Makefile
> @@ -15,3 +15,4 @@ obj-$(CONFIG_MCP4131) += mcp4131.o
>  obj-$(CONFIG_MCP4531) += mcp4531.o
>  obj-$(CONFIG_MCP41010) += mcp41010.o
>  obj-$(CONFIG_TPL0102) += tpl0102.o
> +obj-$(CONFIG_X9250) += x9250.o
> diff --git a/drivers/iio/potentiometer/x9250.c b/drivers/iio/potentiometer/x9250.c
> new file mode 100644
> index 000000000000..c6bc959205a3
> --- /dev/null
> +++ b/drivers/iio/potentiometer/x9250.c
> @@ -0,0 +1,234 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + *
> + * x9250.c  --  Renesas X9250 potentiometers IIO driver
> + *
> + * Copyright 2023 CS GROUP France
> + *
> + * Author: Herve Codina <herve.codina@bootlin.com>
> + */
> +
> +#include <linux/iio/iio.h>
> +#include <linux/limits.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/spi/spi.h>
> +
> +struct x9250_cfg {
> +	int kohms;
> +};
> +
> +struct x9250 {
> +	struct spi_device *spi;
> +	const struct x9250_cfg *cfg;
> +	struct mutex lock; /* Protect tx and rx buffers */
> +	/* Cannot use stack area for SPI (dma-safe memory) */
> +	u8 spi_tx_buf[3] __aligned(IIO_DMA_MINALIGN);
> +	u8 spi_rx_buf[3] __aligned(IIO_DMA_MINALIGN);

Technically you don't need to align the second one.  A particular peripheral
is always assumed to not step on it self.  So as long as only that
peripheral is accessing data in a cacheline then it's fine.

Note that below I suggest you just use spi_write_then_read() for all these
cases allowing  you to drop these and the lock.

> +};
> +
> +#define X9250_ID		0x50
> +#define X9250_CMD_RD_WCR(_p)    (0x90 | (_p))
> +#define X9250_CMD_WR_WCR(_p)    (0xa0 | (_p))
> +
> +static int x9250_write8(struct x9250 *x9250, u8 cmd, u8 val)
> +{
> +	struct spi_transfer xfer = {
> +		.tx_buf = &x9250->spi_tx_buf,
> +		.len = 3,
> +	};
> +	int ret;
> +
> +	BUILD_BUG_ON(sizeof(x9250->spi_tx_buf) < 3);

I don't see an advantage in this check as the buffer
is allocated only a few lines above here and will have size
of 3.

If there were lots of different uses of that buffer
then it would be fair enough, but there aren't.
Anyhow, follow advice below and the buffers go away anyway
making this point irrelevant :)

> +
> +	mutex_lock(&x9250->lock);
> +
> +	x9250->spi_tx_buf[0] = X9250_ID;
> +	x9250->spi_tx_buf[1] = cmd;
> +	x9250->spi_tx_buf[2] = val;
> +
> +	ret = spi_sync_transfer(x9250->spi, &xfer, 1);

Given buffer is small, I'd be tempted to use the 'cheat' approach
of spi_write_then_read() with a 0 sized read.  Avoids the need
for ensuring dma safe buffers etc and having to handle the buffer
locking in your driver.

> +
> +	mutex_unlock(&x9250->lock);
> +
> +	return ret;
> +}
> +
> +static int x9250_read8(struct x9250 *x9250, u8 cmd, u8 *val)
> +{
> +	struct spi_transfer xfer = {
> +		.tx_buf = &x9250->spi_tx_buf,
> +		.rx_buf = &x9250->spi_rx_buf,
> +		.len = 3,
> +	};
> +	int ret;
> +
> +	BUILD_BUG_ON(sizeof(x9250->spi_tx_buf) < 3);
> +	BUILD_BUG_ON(sizeof(x9250->spi_rx_buf) < 3);
> +
> +	mutex_lock(&x9250->lock);
> +
> +	x9250->spi_tx_buf[0] = X9250_ID;
> +	x9250->spi_tx_buf[1] = cmd;
> +
> +	ret = spi_sync_transfer(x9250->spi, &xfer, 1);
> +	if (ret)
> +		goto end;
> +
> +	*val = x9250->spi_rx_buf[2];

Cleaner as an spi_write_then_read() as transfer is not actually
duplex.  + then you don't need to bother with your own locks
for the buffer. The spi core can do that for you (and provide
DMA safe buffers as needed).


> +	ret = 0;
> +end:
> +	mutex_unlock(&x9250->lock);
> +	return ret;
> +}
> +
> +#define X9250_CHANNEL(ch) {						\
> +	.type = IIO_RESISTANCE,						\
> +	.indexed = 1,							\
> +	.output = 1,							\
> +	.channel = (ch),						\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
> +	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_RAW),	\
> +}
> +
> +static const struct iio_chan_spec x9250_channels[] = {
> +	X9250_CHANNEL(0),
> +	X9250_CHANNEL(1),
> +	X9250_CHANNEL(2),
> +	X9250_CHANNEL(3),
> +};
> +
> +static int x9250_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +			  int *val, int *val2, long mask)
> +{
> +	struct x9250 *x9250 = iio_priv(indio_dev);
> +	int ch = chan->channel;
> +	int ret;
> +	u8 v;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = x9250_read8(x9250, X9250_CMD_RD_WCR(ch), &v);
> +		if (ret)
> +			return ret;
> +		*val = v;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 1000 * x9250->cfg->kohms;
> +		*val2 = U8_MAX;
> +		return IIO_VAL_FRACTIONAL;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int x9250_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +			    const int **vals, int *type, int *length, long mask)
> +{
> +	static const int range[] = {0, 1, 255}; /* min, step, max */
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		*length = ARRAY_SIZE(range);
> +		*vals = range;
> +		*type = IIO_VAL_INT;
> +		return IIO_AVAIL_RANGE;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int x9250_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +			   int val, int val2, long mask)
> +{
> +	struct x9250 *x9250 = iio_priv(indio_dev);
> +	int ch = chan->channel;
> +
> +	if (mask != IIO_CHAN_INFO_RAW)
> +		return -EINVAL;
> +
> +	if (val > U8_MAX || val < 0)
> +		return -EINVAL;
> +
> +	return x9250_write8(x9250, X9250_CMD_WR_WCR(ch), val);
> +}
> +
> +static const struct iio_info x9250_info = {
> +	.read_raw = x9250_read_raw,
> +	.read_avail = x9250_read_avail,
> +	.write_raw = x9250_write_raw,
> +};
> +
> +enum x9250_type {
> +	X9250T,
> +	X9250U,
> +};
> +
> +static const struct x9250_cfg x9250_cfg[] = {
> +	[X9250T] = { .kohms =  100, },
> +	[X9250U] = { .kohms =  50, },
> +};
> +
> +static int x9250_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct x9250 *x9250;
> +	int ret;
> +
> +	spi->bits_per_word = 8;

This is configuring it to the default value.  You shouldn't need to
call spi_setup() explicitly. It will already have been called by
the spi subsystem as part of adding the device.  If you feel this
has important documentation value, then I don't mind it. However
we rarely do this unless we need to choose a non default value.


> +	ret = spi_setup(spi);
> +	if (ret < 0)
> +		return ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*x9250));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	x9250 = iio_priv(indio_dev);
> +	x9250->spi = spi;
> +	x9250->cfg = device_get_match_data(&spi->dev);
> +	if (!x9250->cfg)
> +		x9250->cfg = &x9250_cfg[spi_get_device_id(spi)->driver_data];
> +
> +	mutex_init(&x9250->lock);
> +
> +	indio_dev->info = &x9250_info;
> +	indio_dev->channels = x9250_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(x9250_channels);
> +	indio_dev->name = spi_get_device_id(spi)->name;

Not relying on that preferred because the two tables may get out
of sync and spi_get_device_id() fail to match as a result.

Put a const char *name  in the x9250 structure and repeat
the names there.  Let the compiler do the magic of fusing the
identical strings if it can.


> +
> +	spi_set_drvdata(spi, indio_dev);

Why?  Nothing seems to use it that I can see.

> +
> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +static const struct of_device_id x9250_of_match[] = {
> +	{ .compatible = "renesas,x9250t", &x9250_cfg[X9250T]},
> +	{ .compatible = "renesas,x9250u", &x9250_cfg[X9250U]},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, x9250_of_match);
> +
> +static const struct spi_device_id x9250_id_table[] = {
> +	{ "x9250t", X9250T },
> +	{ "x9250u", X9250U },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, x9250_id_table);
> +
> +static struct spi_driver x9250_spi_driver = {
> +	.driver  = {
> +		.name   = "x9250",

I'd stick to a single space before the =
It just ends up messy if you attempt to align things with
extra spaces.

> +		.of_match_table = x9250_of_match,
> +	},
> +	.id_table = x9250_id_table,
> +	.probe  = x9250_probe,
> +};
> +
> +module_spi_driver(x9250_spi_driver);
> +
> +MODULE_AUTHOR("Herve Codina <herve.codina@bootlin.com>");
> +MODULE_DESCRIPTION("X9250 ALSA SoC driver");
> +MODULE_LICENSE("GPL");

Re: [PATCH v3 2/3] iio: potentiometer: Add support for the Renesas X9250 potentiometers

[Herve Codina]

Hi Jonathan,

On Sat, 22 Apr 2023 17:34:53 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Fri, 21 Apr 2023 10:52:44 +0200
> Herve Codina <herve.codina@bootlin.com> wrote:
> 
> > The Renesas X9250 integrates four digitally controlled potentiometers.
> > On each potentiometer, the X9250T has a 100 kOhms total resistance and
> > the X9250U has a 50 kOhms total resistance.
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>  
> 
> Hi Herve,
> 
> Nice clean driver.  Comments are mostly about ways to avoid boilerplate
> and simplify the code a bit.
> 
> Note that the binding comment on regulators should be matched by
> appropriate devm_get_regulator_enabled() calls in here to ensure they 
> are turned on.

Yes, I will use devm_get_regulator_enabled().

> 
> Jonathan
> 
> > ---
> >  drivers/iio/potentiometer/Kconfig  |  10 ++
> >  drivers/iio/potentiometer/Makefile |   1 +
> >  drivers/iio/potentiometer/x9250.c  | 234 +++++++++++++++++++++++++++++
> >  3 files changed, 245 insertions(+)
> >  create mode 100644 drivers/iio/potentiometer/x9250.c
> > 
> > diff --git a/drivers/iio/potentiometer/Kconfig b/drivers/iio/potentiometer/Kconfig
> > index 01dd3f858d99..e6a9a3c67845 100644
> > --- a/drivers/iio/potentiometer/Kconfig
> > +++ b/drivers/iio/potentiometer/Kconfig
> > @@ -136,4 +136,14 @@ config TPL0102
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called tpl0102.
> >  
> > +config X9250
> > +	tristate "Renesas X9250 quad controlled potentiometers"
> > +	depends on SPI
> > +	help
> > +	  Enable support for the Renesas X9250 quad controlled
> > +	  potentiometers.
> > +
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called x9250.
> > +
> >  endmenu
> > diff --git a/drivers/iio/potentiometer/Makefile b/drivers/iio/potentiometer/Makefile
> > index 5ebb8e3bbd76..d11fb739176c 100644
> > --- a/drivers/iio/potentiometer/Makefile
> > +++ b/drivers/iio/potentiometer/Makefile
> > @@ -15,3 +15,4 @@ obj-$(CONFIG_MCP4131) += mcp4131.o
> >  obj-$(CONFIG_MCP4531) += mcp4531.o
> >  obj-$(CONFIG_MCP41010) += mcp41010.o
> >  obj-$(CONFIG_TPL0102) += tpl0102.o
> > +obj-$(CONFIG_X9250) += x9250.o
> > diff --git a/drivers/iio/potentiometer/x9250.c b/drivers/iio/potentiometer/x9250.c
> > new file mode 100644
> > index 000000000000..c6bc959205a3
> > --- /dev/null
> > +++ b/drivers/iio/potentiometer/x9250.c
> > @@ -0,0 +1,234 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + *
> > + * x9250.c  --  Renesas X9250 potentiometers IIO driver
> > + *
> > + * Copyright 2023 CS GROUP France
> > + *
> > + * Author: Herve Codina <herve.codina@bootlin.com>
> > + */
> > +
> > +#include <linux/iio/iio.h>
> > +#include <linux/limits.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/spi/spi.h>
> > +
> > +struct x9250_cfg {
> > +	int kohms;
> > +};
> > +
> > +struct x9250 {
> > +	struct spi_device *spi;
> > +	const struct x9250_cfg *cfg;
> > +	struct mutex lock; /* Protect tx and rx buffers */
> > +	/* Cannot use stack area for SPI (dma-safe memory) */
> > +	u8 spi_tx_buf[3] __aligned(IIO_DMA_MINALIGN);
> > +	u8 spi_rx_buf[3] __aligned(IIO_DMA_MINALIGN);  
> 
> Technically you don't need to align the second one.  A particular peripheral
> is always assumed to not step on it self.  So as long as only that
> peripheral is accessing data in a cacheline then it's fine.
> 
> Note that below I suggest you just use spi_write_then_read() for all these
> cases allowing  you to drop these and the lock.

Indeed, I will give a try to spi_write_then_read()

> 
> > +};
> > +
> > +#define X9250_ID		0x50
> > +#define X9250_CMD_RD_WCR(_p)    (0x90 | (_p))
> > +#define X9250_CMD_WR_WCR(_p)    (0xa0 | (_p))
> > +
> > +static int x9250_write8(struct x9250 *x9250, u8 cmd, u8 val)
> > +{
> > +	struct spi_transfer xfer = {
> > +		.tx_buf = &x9250->spi_tx_buf,
> > +		.len = 3,
> > +	};
> > +	int ret;
> > +
> > +	BUILD_BUG_ON(sizeof(x9250->spi_tx_buf) < 3);  
> 
> I don't see an advantage in this check as the buffer
> is allocated only a few lines above here and will have size
> of 3.
> 
> If there were lots of different uses of that buffer
> then it would be fair enough, but there aren't.
> Anyhow, follow advice below and the buffers go away anyway
> making this point irrelevant :)
> 
> > +
> > +	mutex_lock(&x9250->lock);
> > +
> > +	x9250->spi_tx_buf[0] = X9250_ID;
> > +	x9250->spi_tx_buf[1] = cmd;
> > +	x9250->spi_tx_buf[2] = val;
> > +
> > +	ret = spi_sync_transfer(x9250->spi, &xfer, 1);  
> 
> Given buffer is small, I'd be tempted to use the 'cheat' approach
> of spi_write_then_read() with a 0 sized read.  Avoids the need
> for ensuring dma safe buffers etc and having to handle the buffer
> locking in your driver.
> 
> > +
> > +	mutex_unlock(&x9250->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int x9250_read8(struct x9250 *x9250, u8 cmd, u8 *val)
> > +{
> > +	struct spi_transfer xfer = {
> > +		.tx_buf = &x9250->spi_tx_buf,
> > +		.rx_buf = &x9250->spi_rx_buf,
> > +		.len = 3,
> > +	};
> > +	int ret;
> > +
> > +	BUILD_BUG_ON(sizeof(x9250->spi_tx_buf) < 3);
> > +	BUILD_BUG_ON(sizeof(x9250->spi_rx_buf) < 3);
> > +
> > +	mutex_lock(&x9250->lock);
> > +
> > +	x9250->spi_tx_buf[0] = X9250_ID;
> > +	x9250->spi_tx_buf[1] = cmd;
> > +
> > +	ret = spi_sync_transfer(x9250->spi, &xfer, 1);
> > +	if (ret)
> > +		goto end;
> > +
> > +	*val = x9250->spi_rx_buf[2];  
> 
> Cleaner as an spi_write_then_read() as transfer is not actually
> duplex.  + then you don't need to bother with your own locks
> for the buffer. The spi core can do that for you (and provide
> DMA safe buffers as needed).
> 
> 
> > +	ret = 0;
> > +end:
> > +	mutex_unlock(&x9250->lock);
> > +	return ret;
> > +}
> > +
> > +#define X9250_CHANNEL(ch) {						\
> > +	.type = IIO_RESISTANCE,						\
> > +	.indexed = 1,							\
> > +	.output = 1,							\
> > +	.channel = (ch),						\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> > +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
> > +	.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_RAW),	\
> > +}
> > +
> > +static const struct iio_chan_spec x9250_channels[] = {
> > +	X9250_CHANNEL(0),
> > +	X9250_CHANNEL(1),
> > +	X9250_CHANNEL(2),
> > +	X9250_CHANNEL(3),
> > +};
> > +
> > +static int x9250_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> > +			  int *val, int *val2, long mask)
> > +{
> > +	struct x9250 *x9250 = iio_priv(indio_dev);
> > +	int ch = chan->channel;
> > +	int ret;
> > +	u8 v;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = x9250_read8(x9250, X9250_CMD_RD_WCR(ch), &v);
> > +		if (ret)
> > +			return ret;
> > +		*val = v;
> > +		return IIO_VAL_INT;
> > +
> > +	case IIO_CHAN_INFO_SCALE:
> > +		*val = 1000 * x9250->cfg->kohms;
> > +		*val2 = U8_MAX;
> > +		return IIO_VAL_FRACTIONAL;
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int x9250_read_avail(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> > +			    const int **vals, int *type, int *length, long mask)
> > +{
> > +	static const int range[] = {0, 1, 255}; /* min, step, max */
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		*length = ARRAY_SIZE(range);
> > +		*vals = range;
> > +		*type = IIO_VAL_INT;
> > +		return IIO_AVAIL_RANGE;
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int x9250_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> > +			   int val, int val2, long mask)
> > +{
> > +	struct x9250 *x9250 = iio_priv(indio_dev);
> > +	int ch = chan->channel;
> > +
> > +	if (mask != IIO_CHAN_INFO_RAW)
> > +		return -EINVAL;
> > +
> > +	if (val > U8_MAX || val < 0)
> > +		return -EINVAL;
> > +
> > +	return x9250_write8(x9250, X9250_CMD_WR_WCR(ch), val);
> > +}
> > +
> > +static const struct iio_info x9250_info = {
> > +	.read_raw = x9250_read_raw,
> > +	.read_avail = x9250_read_avail,
> > +	.write_raw = x9250_write_raw,
> > +};
> > +
> > +enum x9250_type {
> > +	X9250T,
> > +	X9250U,
> > +};
> > +
> > +static const struct x9250_cfg x9250_cfg[] = {
> > +	[X9250T] = { .kohms =  100, },
> > +	[X9250U] = { .kohms =  50, },
> > +};
> > +
> > +static int x9250_probe(struct spi_device *spi)
> > +{
> > +	struct iio_dev *indio_dev;
> > +	struct x9250 *x9250;
> > +	int ret;
> > +
> > +	spi->bits_per_word = 8;  
> 
> This is configuring it to the default value.  You shouldn't need to
> call spi_setup() explicitly. It will already have been called by
> the spi subsystem as part of adding the device.  If you feel this
> has important documentation value, then I don't mind it. However
> we rarely do this unless we need to choose a non default value.

Agree.
I will remove in next iteration.

> 
> 
> > +	ret = spi_setup(spi);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*x9250));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	x9250 = iio_priv(indio_dev);
> > +	x9250->spi = spi;
> > +	x9250->cfg = device_get_match_data(&spi->dev);
> > +	if (!x9250->cfg)
> > +		x9250->cfg = &x9250_cfg[spi_get_device_id(spi)->driver_data];
> > +
> > +	mutex_init(&x9250->lock);
> > +
> > +	indio_dev->info = &x9250_info;
> > +	indio_dev->channels = x9250_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(x9250_channels);
> > +	indio_dev->name = spi_get_device_id(spi)->name;  
> 
> Not relying on that preferred because the two tables may get out
> of sync and spi_get_device_id() fail to match as a result.
> 
> Put a const char *name  in the x9250 structure and repeat
> the names there.  Let the compiler do the magic of fusing the
> identical strings if it can.

Ok,
I will add a name field in the x9250_cfg structure and use this new field
here.

> 
> 
> > +
> > +	spi_set_drvdata(spi, indio_dev);  
> 
> Why?  Nothing seems to use it that I can see.

Indeed, nothing use the drv data. I will remove in the next iteration.

> 
> > +
> > +	return devm_iio_device_register(&spi->dev, indio_dev);
> > +}
> > +
> > +static const struct of_device_id x9250_of_match[] = {
> > +	{ .compatible = "renesas,x9250t", &x9250_cfg[X9250T]},
> > +	{ .compatible = "renesas,x9250u", &x9250_cfg[X9250U]},
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, x9250_of_match);
> > +
> > +static const struct spi_device_id x9250_id_table[] = {
> > +	{ "x9250t", X9250T },
> > +	{ "x9250u", X9250U },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(spi, x9250_id_table);
> > +
> > +static struct spi_driver x9250_spi_driver = {
> > +	.driver  = {
> > +		.name   = "x9250",  
> 
> I'd stick to a single space before the =
> It just ends up messy if you attempt to align things with
> extra spaces.

Will be fixed.

> 
> > +		.of_match_table = x9250_of_match,
> > +	},
> > +	.id_table = x9250_id_table,
> > +	.probe  = x9250_probe,
> > +};
> > +
> > +module_spi_driver(x9250_spi_driver);
> > +
> > +MODULE_AUTHOR("Herve Codina <herve.codina@bootlin.com>");
> > +MODULE_DESCRIPTION("X9250 ALSA SoC driver");
> > +MODULE_LICENSE("GPL");  
> 

Thanks for the review.

Best regards,
Hervé

[PATCH v3 3/3] MAINTAINERS: add the Renesas X9250 driver entry

[Herve Codina]

After contributing the driver, add myself as the maintainer for the
Renesas X9250 IIO driver.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 8ec7ccba9848..0027af3e14cb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17926,6 +17926,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/clock/renesas,versaclock7.yaml
 F:	drivers/clk/clk-versaclock7.c
 
+RENESAS X9250 DIGITAL POTENTIOMETERS DRIVER
+M:	Herve Codina <herve.codina@bootlin.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml
+F:	drivers/iio/potentiometer/x9250.c
+
 RESET CONTROLLER FRAMEWORK
 M:	Philipp Zabel <p.zabel@pengutronix.de>
 S:	Maintained
-- 
2.39.2