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/[email protected]/
This v4 series updates the binding, introduced the power-supply
regulators and the write-protect gpio, uses spi_write_then_read(),
removes spi_get_device_id(spi)->name, removes spi_set_drvdata() call.
Best regards,
Herve Codina
Changes v3 -> v4
- Patch 1
Remove iio.yaml.
Add 'vcc-supply', 'avp-supply' and 'avn-supply'.
Add 'wp-gpios'
- Patch 2
Get and enable the regulators.
Manage the write-protect gpio.
Use spi_write_then_read().
Remove the unneeded spi_setup() call.
Get name from field added in struct x9250_cfg instead of
spi_get_device_id(spi)->name.
- Patch 3
No changes
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 | 78 ++++++
MAINTAINERS | 7 +
drivers/iio/potentiometer/Kconfig | 10 +
drivers/iio/potentiometer/Makefile | 1 +
drivers/iio/potentiometer/x9250.c | 223 ++++++++++++++++++
5 files changed, 319 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml
create mode 100644 drivers/iio/potentiometer/x9250.c
--
2.40.1
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 <[email protected]>
---
drivers/iio/potentiometer/Kconfig | 10 ++
drivers/iio/potentiometer/Makefile | 1 +
drivers/iio/potentiometer/x9250.c | 223 +++++++++++++++++++++++++++++
3 files changed, 234 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..3d4ca18d1f14
--- /dev/null
+++ b/drivers/iio/potentiometer/x9250.c
@@ -0,0 +1,223 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ *
+ * x9250.c -- Renesas X9250 potentiometers IIO driver
+ *
+ * Copyright 2023 CS GROUP France
+ *
+ * Author: Herve Codina <[email protected]>
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/iio/iio.h>
+#include <linux/limits.h>
+#include <linux/module.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+
+struct x9250_cfg {
+ const char *name;
+ int kohms;
+};
+
+struct x9250 {
+ struct spi_device *spi;
+ const struct x9250_cfg *cfg;
+ struct gpio_desc *wp_gpio;
+};
+
+#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)
+{
+ u8 txbuf[3];
+
+ txbuf[0] = X9250_ID;
+ txbuf[1] = cmd;
+ txbuf[2] = val;
+
+ return spi_write_then_read(x9250->spi, txbuf, ARRAY_SIZE(txbuf), NULL, 0);
+}
+
+static int x9250_read8(struct x9250 *x9250, u8 cmd, u8 *val)
+{
+ u8 txbuf[2];
+
+ txbuf[0] = X9250_ID;
+ txbuf[1] = cmd;
+
+ return spi_write_then_read(x9250->spi, txbuf, ARRAY_SIZE(txbuf), val, 1);
+}
+
+#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;
+ int ret;
+
+ if (mask != IIO_CHAN_INFO_RAW)
+ return -EINVAL;
+
+ if (val > U8_MAX || val < 0)
+ return -EINVAL;
+
+ gpiod_set_value_cansleep(x9250->wp_gpio, 0);
+ ret = x9250_write8(x9250, X9250_CMD_WR_WCR(ch), val);
+ gpiod_set_value_cansleep(x9250->wp_gpio, 1);
+
+ return ret;
+}
+
+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] = { .name = "x9250t", .kohms = 100, },
+ [X9250U] = { .name = "x9250u", .kohms = 50, },
+};
+
+static const char *const x9250_regulator_names[] = {
+ "vcc",
+ "avp",
+ "avn",
+};
+
+static int x9250_probe(struct spi_device *spi)
+{
+ struct iio_dev *indio_dev;
+ struct x9250 *x9250;
+ int ret;
+
+ ret = devm_regulator_bulk_get_enable(&spi->dev, ARRAY_SIZE(x9250_regulator_names),
+ x9250_regulator_names);
+ if (ret)
+ return dev_err_probe(&spi->dev, ret, "Failed to get regulators\n");
+
+ /*
+ * The x9250 needs a 5ms maximum delay after the power-supplies are set
+ * before performing the first write (1ms for the first read).
+ */
+ usleep_range(5000, 6000);
+
+ 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];
+
+ x9250->wp_gpio = devm_gpiod_get_optional(&spi->dev, "wp", GPIOD_OUT_LOW);
+ if (IS_ERR(x9250->wp_gpio))
+ return dev_err_probe(&spi->dev, PTR_ERR(x9250->wp_gpio),
+ "failed to get wp gpio\n");
+
+ indio_dev->info = &x9250_info;
+ indio_dev->channels = x9250_channels;
+ indio_dev->num_channels = ARRAY_SIZE(x9250_channels);
+ indio_dev->name = x9250->cfg->name;
+
+ 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 <[email protected]>");
+MODULE_DESCRIPTION("X9250 ALSA SoC driver");
+MODULE_LICENSE("GPL");
--
2.40.1
The Renesas X9250 is a quad digitally controlled potentiometers.
Signed-off-by: Herve Codina <[email protected]>
---
.../iio/potentiometer/renesas,x9250.yaml | 78 +++++++++++++++++++
1 file changed, 78 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..ab5c09c00ff4
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml
@@ -0,0 +1,78 @@
+# 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 <[email protected]>
+
+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
+
+properties:
+ compatible:
+ enum:
+ - renesas,x9250t
+ - renesas,x9250u
+
+ reg:
+ maxItems: 1
+
+ vcc-supply:
+ description:
+ Regulator for the VCC power supply.
+
+ avp-supply:
+ description:
+ Regulator for the analog V+ power supply.
+
+ avn-supply:
+ description:
+ Regulator for the analog V- power supply.
+
+ '#io-channel-cells':
+ const: 1
+
+ spi-max-frequency:
+ maximum: 2000000
+
+ wp-gpios:
+ maxItems: 1
+ description:
+ GPIO connected to the write-protect pin.
+
+required:
+ - compatible
+ - reg
+ - vcc-supply
+ - avp-supply
+ - avn-supply
+ - '#io-channel-cells'
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ potentiometer@0 {
+ compatible = "renesas,x9250t";
+ reg = <0>;
+ vcc-supply = <&vcc_regulator>;
+ avp-supply = <&avp_regulator>;
+ avn-supply = <&avp_regulator>;
+ wp-gpios = <&gpio 1 GPIO_ACTIVE_LOW>;
+ spi-max-frequency = <2000000>;
+ #io-channel-cells = <1>;
+ };
+ };
--
2.40.1
After contributing the driver, add myself as the maintainer for the
Renesas X9250 IIO driver.
Signed-off-by: Herve Codina <[email protected]>
---
MAINTAINERS | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 7517002b7735..159b3e7ce566 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18046,6 +18046,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 <[email protected]>
+L: [email protected]
+S: Maintained
+F: Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml
+F: drivers/iio/potentiometer/x9250.c
+
RESET CONTROLLER FRAMEWORK
M: Philipp Zabel <[email protected]>
S: Maintained
--
2.40.1
On 09/05/2023 18:08, Herve Codina wrote:
> The Renesas X9250 is a quad digitally controlled potentiometers.
>
> Signed-off-by: Herve Codina <[email protected]>
> ---
> .../iio/potentiometer/renesas,x9250.yaml | 78 +++++++++++++++++++
> 1 file changed, 78 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml
>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On Tue, 9 May 2023 18:08:51 +0200
Herve Codina <[email protected]> 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 <[email protected]>
As I only noticed one trivial thing I made the change whilst applying.
diff --git a/drivers/iio/potentiometer/x9250.c b/drivers/iio/potentiometer/x9250.c
index 3d4ca18d1f14..7e145d7d14f1 100644
--- a/drivers/iio/potentiometer/x9250.c
+++ b/drivers/iio/potentiometer/x9250.c
@@ -176,10 +176,7 @@ static int x9250_probe(struct spi_device *spi)
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];
-
+ x9250->cfg = spi_get_device_match_data(spi);
x9250->wp_gpio = devm_gpiod_get_optional(&spi->dev, "wp", GPIOD_OUT_LOW);
if (IS_ERR(x9250->wp_gpio))
return dev_err_probe(&spi->dev, PTR_ERR(x9250->wp_gpio),
On Tue, 9 May 2023 18:08:49 +0200
Herve Codina <[email protected]> wrote:
> 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/[email protected]/
> This v4 series updates the binding, introduced the power-supply
> regulators and the write-protect gpio, uses spi_write_then_read(),
> removes spi_get_device_id(spi)->name, removes spi_set_drvdata() call.
>
> Best regards,
> Herve Codina
Series applied to the togreg branch of iio.git with one tweak as per reply to patch 2.
Thanks,
Jonathan
>
> Changes v3 -> v4
> - Patch 1
> Remove iio.yaml.
> Add 'vcc-supply', 'avp-supply' and 'avn-supply'.
> Add 'wp-gpios'
>
> - Patch 2
> Get and enable the regulators.
> Manage the write-protect gpio.
> Use spi_write_then_read().
> Remove the unneeded spi_setup() call.
> Get name from field added in struct x9250_cfg instead of
> spi_get_device_id(spi)->name.
>
> - Patch 3
> No changes
>
> 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 | 78 ++++++
> MAINTAINERS | 7 +
> drivers/iio/potentiometer/Kconfig | 10 +
> drivers/iio/potentiometer/Makefile | 1 +
> drivers/iio/potentiometer/x9250.c | 223 ++++++++++++++++++
> 5 files changed, 319 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/potentiometer/renesas,x9250.yaml
> create mode 100644 drivers/iio/potentiometer/x9250.c
>
Hi Jonathan,
On Sat, 13 May 2023 19:35:25 +0100
Jonathan Cameron <[email protected]> wrote:
> On Tue, 9 May 2023 18:08:51 +0200
> Herve Codina <[email protected]> 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 <[email protected]>
>
> As I only noticed one trivial thing I made the change whilst applying.
> diff --git a/drivers/iio/potentiometer/x9250.c b/drivers/iio/potentiometer/x9250.c
> index 3d4ca18d1f14..7e145d7d14f1 100644
> --- a/drivers/iio/potentiometer/x9250.c
> +++ b/drivers/iio/potentiometer/x9250.c
> @@ -176,10 +176,7 @@ static int x9250_probe(struct spi_device *spi)
>
> 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];
> -
> + x9250->cfg = spi_get_device_match_data(spi);
> x9250->wp_gpio = devm_gpiod_get_optional(&spi->dev, "wp", GPIOD_OUT_LOW);
> if (IS_ERR(x9250->wp_gpio))
> return dev_err_probe(&spi->dev, PTR_ERR(x9250->wp_gpio),
>
Are you sure about your modification ?
I am not sure (maybe I am wrong) that
x9250->cfg = spi_get_device_match_data(spi);
is equivalent to
x9250->cfg = &x9250_cfg[spi_get_device_id(spi)->driver_data];
The spi_get_device_id(spi)->driver_data value I used is a simple integer
(X9250T or X9250U) and not the x9250_cfg item.
Maybe the x9250_id_table should be modified to replace X9250T by
&x9250_cfg[X9250T] to have your modification working.
The data defined in the driver are the following:
--- 8< ---
static const struct x9250_cfg x9250_cfg[] = {
[X9250T] = { .name = "x9250t", .kohms = 100, },
[X9250U] = { .name = "x9250u", .kohms = 50, },
};
...
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,
};
--- 8< ---
Best regards,
Hervé
--
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Sun, 14 May 2023 16:32:33 +0200
Herve Codina <[email protected]> wrote:
> Hi Jonathan,
>
> On Sat, 13 May 2023 19:35:25 +0100
> Jonathan Cameron <[email protected]> wrote:
>
> > On Tue, 9 May 2023 18:08:51 +0200
> > Herve Codina <[email protected]> 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 <[email protected]>
> >
> > As I only noticed one trivial thing I made the change whilst applying.
> > diff --git a/drivers/iio/potentiometer/x9250.c b/drivers/iio/potentiometer/x9250.c
> > index 3d4ca18d1f14..7e145d7d14f1 100644
> > --- a/drivers/iio/potentiometer/x9250.c
> > +++ b/drivers/iio/potentiometer/x9250.c
> > @@ -176,10 +176,7 @@ static int x9250_probe(struct spi_device *spi)
> >
> > 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];
> > -
> > + x9250->cfg = spi_get_device_match_data(spi);
> > x9250->wp_gpio = devm_gpiod_get_optional(&spi->dev, "wp", GPIOD_OUT_LOW);
> > if (IS_ERR(x9250->wp_gpio))
> > return dev_err_probe(&spi->dev, PTR_ERR(x9250->wp_gpio),
> >
>
> Are you sure about your modification ?
>
> I am not sure (maybe I am wrong) that
> x9250->cfg = spi_get_device_match_data(spi);
> is equivalent to
> x9250->cfg = &x9250_cfg[spi_get_device_id(spi)->driver_data];
>
> The spi_get_device_id(spi)->driver_data value I used is a simple integer
> (X9250T or X9250U) and not the x9250_cfg item.
> Maybe the x9250_id_table should be modified to replace X9250T by
> &x9250_cfg[X9250T] to have your modification working.
Excellent point. I'm was clearly half asleep. The mod should have included
switching them over to be pointers.
>
> The data defined in the driver are the following:
> --- 8< ---
> static const struct x9250_cfg x9250_cfg[] = {
> [X9250T] = { .name = "x9250t", .kohms = 100, },
> [X9250U] = { .name = "x9250u", .kohms = 50, },
> };
>
> ...
>
> 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 },
So these should be (kernel_ulong_t)&x9250_cfg[X9250T] etc for the data.
I've tweaked it so that is now the case. Oops and thanks for sanity checking.
Sometimes we see what we expect to see rather than what is there.
Tweak on top of original tweak is:
diff --git a/drivers/iio/potentiometer/x9250.c b/drivers/iio/potentiometer/x9250.c
index 7e145d7d14f1..0cc7f72529be 100644
--- a/drivers/iio/potentiometer/x9250.c
+++ b/drivers/iio/potentiometer/x9250.c
@@ -198,8 +198,8 @@ static const struct of_device_id x9250_of_match[] = {
MODULE_DEVICE_TABLE(of, x9250_of_match);
static const struct spi_device_id x9250_id_table[] = {
- { "x9250t", X9250T },
- { "x9250u", X9250U },
+ { "x9250t", (kernel_ulong_t)&x9250_cfg[X9250T] },
+ { "x9250u", (kernel_ulong_t)&x9250_cfg[X9250U] },
{ }
};
Jonathan
> { }
> };
> 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,
> };
> --- 8< ---
>
>
> Best regards,
> Hervé
>
On Sun, 14 May 2023 18:19:12 +0100
Jonathan Cameron <[email protected]> wrote:
> On Sun, 14 May 2023 16:32:33 +0200
> Herve Codina <[email protected]> wrote:
>
> > Hi Jonathan,
> >
> > On Sat, 13 May 2023 19:35:25 +0100
> > Jonathan Cameron <[email protected]> wrote:
> >
> > > On Tue, 9 May 2023 18:08:51 +0200
> > > Herve Codina <[email protected]> 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 <[email protected]>
> > >
> > > As I only noticed one trivial thing I made the change whilst applying.
> > > diff --git a/drivers/iio/potentiometer/x9250.c b/drivers/iio/potentiometer/x9250.c
> > > index 3d4ca18d1f14..7e145d7d14f1 100644
> > > --- a/drivers/iio/potentiometer/x9250.c
> > > +++ b/drivers/iio/potentiometer/x9250.c
> > > @@ -176,10 +176,7 @@ static int x9250_probe(struct spi_device *spi)
> > >
> > > 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];
> > > -
> > > + x9250->cfg = spi_get_device_match_data(spi);
> > > x9250->wp_gpio = devm_gpiod_get_optional(&spi->dev, "wp", GPIOD_OUT_LOW);
> > > if (IS_ERR(x9250->wp_gpio))
> > > return dev_err_probe(&spi->dev, PTR_ERR(x9250->wp_gpio),
> > >
> >
> > Are you sure about your modification ?
> >
> > I am not sure (maybe I am wrong) that
> > x9250->cfg = spi_get_device_match_data(spi);
> > is equivalent to
> > x9250->cfg = &x9250_cfg[spi_get_device_id(spi)->driver_data];
> >
> > The spi_get_device_id(spi)->driver_data value I used is a simple integer
> > (X9250T or X9250U) and not the x9250_cfg item.
> > Maybe the x9250_id_table should be modified to replace X9250T by
> > &x9250_cfg[X9250T] to have your modification working.
>
> Excellent point. I'm was clearly half asleep. The mod should have included
> switching them over to be pointers.
>
> >
> > The data defined in the driver are the following:
> > --- 8< ---
> > static const struct x9250_cfg x9250_cfg[] = {
> > [X9250T] = { .name = "x9250t", .kohms = 100, },
> > [X9250U] = { .name = "x9250u", .kohms = 50, },
> > };
> >
> > ...
> >
> > 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 },
> So these should be (kernel_ulong_t)&x9250_cfg[X9250T] etc for the data.
> I've tweaked it so that is now the case. Oops and thanks for sanity checking.
> Sometimes we see what we expect to see rather than what is there.
>
> Tweak on top of original tweak is:
> diff --git a/drivers/iio/potentiometer/x9250.c b/drivers/iio/potentiometer/x9250.c
> index 7e145d7d14f1..0cc7f72529be 100644
> --- a/drivers/iio/potentiometer/x9250.c
> +++ b/drivers/iio/potentiometer/x9250.c
> @@ -198,8 +198,8 @@ static const struct of_device_id x9250_of_match[] = {
> MODULE_DEVICE_TABLE(of, x9250_of_match);
>
> static const struct spi_device_id x9250_id_table[] = {
> - { "x9250t", X9250T },
> - { "x9250u", X9250U },
> + { "x9250t", (kernel_ulong_t)&x9250_cfg[X9250T] },
> + { "x9250u", (kernel_ulong_t)&x9250_cfg[X9250U] },
> { }
> };
>
>
Pefect, thanks.
Also can you add a last modification (my bad, I should see that before):
static const struct of_device_id x9250_of_match[] = {
- { .compatible = "renesas,x9250t", &x9250_cfg[X9250T]},
- { .compatible = "renesas,x9250u", &x9250_cfg[X9250U]},
+ { .compatible = "renesas,x9250t", .data = &x9250_cfg[X9250T]},
+ { .compatible = "renesas,x9250u", .data = &x9250_cfg[X9250U]},
{ }
};
I think adding '.data = ' would be better and avoid to have some quite tricky
bug in case of struct of_device_id modification.
Regards,
Hervé
> Jonathan
>
> > { }
> > };
> > 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,
> > };
> > --- 8< ---
> >
> >
> > Best regards,
> > Hervé
> >
>
On Mon, 15 May 2023 08:44:16 +0200
Herve Codina <[email protected]> wrote:
> On Sun, 14 May 2023 18:19:12 +0100
> Jonathan Cameron <[email protected]> wrote:
>
> > On Sun, 14 May 2023 16:32:33 +0200
> > Herve Codina <[email protected]> wrote:
> >
> > > Hi Jonathan,
> > >
> > > On Sat, 13 May 2023 19:35:25 +0100
> > > Jonathan Cameron <[email protected]> wrote:
> > >
> > > > On Tue, 9 May 2023 18:08:51 +0200
> > > > Herve Codina <[email protected]> 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 <[email protected]>
> > > >
> > > > As I only noticed one trivial thing I made the change whilst applying.
> > > > diff --git a/drivers/iio/potentiometer/x9250.c b/drivers/iio/potentiometer/x9250.c
> > > > index 3d4ca18d1f14..7e145d7d14f1 100644
> > > > --- a/drivers/iio/potentiometer/x9250.c
> > > > +++ b/drivers/iio/potentiometer/x9250.c
> > > > @@ -176,10 +176,7 @@ static int x9250_probe(struct spi_device *spi)
> > > >
> > > > 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];
> > > > -
> > > > + x9250->cfg = spi_get_device_match_data(spi);
> > > > x9250->wp_gpio = devm_gpiod_get_optional(&spi->dev, "wp", GPIOD_OUT_LOW);
> > > > if (IS_ERR(x9250->wp_gpio))
> > > > return dev_err_probe(&spi->dev, PTR_ERR(x9250->wp_gpio),
> > > >
> > >
> > > Are you sure about your modification ?
> > >
> > > I am not sure (maybe I am wrong) that
> > > x9250->cfg = spi_get_device_match_data(spi);
> > > is equivalent to
> > > x9250->cfg = &x9250_cfg[spi_get_device_id(spi)->driver_data];
> > >
> > > The spi_get_device_id(spi)->driver_data value I used is a simple integer
> > > (X9250T or X9250U) and not the x9250_cfg item.
> > > Maybe the x9250_id_table should be modified to replace X9250T by
> > > &x9250_cfg[X9250T] to have your modification working.
> >
> > Excellent point. I'm was clearly half asleep. The mod should have included
> > switching them over to be pointers.
> >
> > >
> > > The data defined in the driver are the following:
> > > --- 8< ---
> > > static const struct x9250_cfg x9250_cfg[] = {
> > > [X9250T] = { .name = "x9250t", .kohms = 100, },
> > > [X9250U] = { .name = "x9250u", .kohms = 50, },
> > > };
> > >
> > > ...
> > >
> > > 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 },
> > So these should be (kernel_ulong_t)&x9250_cfg[X9250T] etc for the data.
> > I've tweaked it so that is now the case. Oops and thanks for sanity checking.
> > Sometimes we see what we expect to see rather than what is there.
> >
> > Tweak on top of original tweak is:
> > diff --git a/drivers/iio/potentiometer/x9250.c b/drivers/iio/potentiometer/x9250.c
> > index 7e145d7d14f1..0cc7f72529be 100644
> > --- a/drivers/iio/potentiometer/x9250.c
> > +++ b/drivers/iio/potentiometer/x9250.c
> > @@ -198,8 +198,8 @@ static const struct of_device_id x9250_of_match[] = {
> > MODULE_DEVICE_TABLE(of, x9250_of_match);
> >
> > static const struct spi_device_id x9250_id_table[] = {
> > - { "x9250t", X9250T },
> > - { "x9250u", X9250U },
> > + { "x9250t", (kernel_ulong_t)&x9250_cfg[X9250T] },
> > + { "x9250u", (kernel_ulong_t)&x9250_cfg[X9250U] },
> > { }
> > };
> >
> >
>
> Pefect, thanks.
>
> Also can you add a last modification (my bad, I should see that before):
>
> static const struct of_device_id x9250_of_match[] = {
> - { .compatible = "renesas,x9250t", &x9250_cfg[X9250T]},
> - { .compatible = "renesas,x9250u", &x9250_cfg[X9250U]},
> + { .compatible = "renesas,x9250t", .data = &x9250_cfg[X9250T]},
> + { .compatible = "renesas,x9250u", .data = &x9250_cfg[X9250U]},
> { }
> };
>
> I think adding '.data = ' would be better and avoid to have some quite tricky
> bug in case of struct of_device_id modification.
>
> Regards,
> Hervé
Done
>
>
> > Jonathan
> >
> > > { }
> > > };
> > > 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,
> > > };
> > > --- 8< ---
> > >
> > >
> > > Best regards,
> > > Hervé
> > >
> >
Tue, May 09, 2023 at 06:08:51PM +0200, Herve Codina kirjoitti:
> 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.
...
> +/*
> + *
Redundant blank line.
> + * x9250.c -- Renesas X9250 potentiometers IIO driver
Please, no filename in the file itself. It adds an additional burden in case
the module will be renamed in the future.
> + * Copyright 2023 CS GROUP France
> + *
> + * Author: Herve Codina <[email protected]>
> + */
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/limits.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/spi/spi.h>
...
> + return spi_write_then_read(x9250->spi, txbuf, ARRAY_SIZE(txbuf), NULL, 0);
sizeof() suffice.
...
> + return spi_write_then_read(x9250->spi, txbuf, ARRAY_SIZE(txbuf), val, 1);
Ditto.
...
> + 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;
Just make it part of default: case.
...
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + *length = ARRAY_SIZE(range);
> + *vals = range;
> + *type = IIO_VAL_INT;
> + return IIO_AVAIL_RANGE;
> + }
> +
> + return -EINVAL;
Same.
...
> + if (val > U8_MAX || val < 0)
> + return -EINVAL;
ERANGE ?
...
> +
Redundant blank line.
> +module_spi_driver(x9250_spi_driver);
--
With Best Regards,
Andy Shevchenko
Sat, May 20, 2023 at 05:30:57PM +0100, Jonathan Cameron kirjoitti:
...
> Done
Not sure if my comments can be addressed.
--
With Best Regards,
Andy Shevchenko
On Mon, 29 May 2023 18:57:45 +0300
[email protected] wrote:
> Sat, May 20, 2023 at 05:30:57PM +0100, Jonathan Cameron kirjoitti:
>
> ...
>
> > Done
>
> Not sure if my comments can be addressed.
>
Hi Andy,
I've pushed it out as togreg (which is more or less non rebasing - except
when something goes horribly wrong) now so I'd rather handle your suggestions
as a follow up cleanup patch / series.
Thanks,
Jonathan