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

[Herve Codina <herve.codina@bootlin.com>]

Hi Jonathan,

On Sat, 13 May 2023 19:35:25 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue,  9 May 2023 18:08:51 +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>  
> 
> 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