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

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 14 May 2023 16:32:33 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> 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.

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é
>