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

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

On Sun, 14 May 2023 18:19:12 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

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

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