diff mbox series

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

Message ID 20230509160852.158101-3-herve.codina@bootlin.com
State New
Headers
Series Add the Renesas X9250 potentiometers IIO support |

Commit Message

Herve Codina May 9, 2023, 4:08 p.m. UTC 
  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  | 223 +++++++++++++++++++++++++++++
 3 files changed, 234 insertions(+)
 create mode 100644 drivers/iio/potentiometer/x9250.c

Comments

Jonathan Cameron May 13, 2023, 6:35 p.m. UTC | #1 
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),
Herve Codina May 14, 2023, 2:32 p.m. UTC | #2 
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é
Jonathan Cameron May 14, 2023, 5:19 p.m. UTC | #3 
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é
>
Herve Codina May 15, 2023, 6:44 a.m. UTC | #4 
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é
> >   
>
Jonathan Cameron May 20, 2023, 4:30 p.m. UTC | #5 
On Mon, 15 May 2023 08:44:16 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> 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é
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é
> > >     
> >
Andy Shevchenko May 28, 2023, 10:35 p.m. UTC | #6 
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 <herve.codina@bootlin.com>
> + */

> +#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);
Andy Shevchenko May 29, 2023, 3:57 p.m. UTC | #7 
Sat, May 20, 2023 at 05:30:57PM +0100, Jonathan Cameron kirjoitti:

...

> Done

Not sure if my comments can be addressed.
Jonathan Cameron June 4, 2023, 12:57 p.m. UTC | #8 
On Mon, 29 May 2023 18:57:45 +0300
andy.shevchenko@gmail.com 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
diff mbox series

Patch

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 <herve.codina@bootlin.com>
+ */
+
+#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 <herve.codina@bootlin.com>");
+MODULE_DESCRIPTION("X9250 ALSA SoC driver");
+MODULE_LICENSE("GPL");