[PATCH v1 1/2] iio: adc: ti-adc128s052: Switch to use spi_get_device_match_data()

[PATCH v1 1/2] iio: adc: ti-adc128s052: Switch to use spi_get_device_match_data()

[Andy Shevchenko]

The spi_get_device_match_data() helps to get driver data from the
firmware node or SPI ID table. Use it instead of open coding.

While at it, switch ID tables to provide an acrual pointers to
the configuration data.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---

Requires aea672d054a2 ("spi: Introduce spi_get_device_match_data()
helper") which is part of upstream as of today.

 drivers/iio/adc/ti-adc128s052.c | 39 +++++++++++++++------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
index b3d5b9b7255b..9dfc625100b6 100644
--- a/drivers/iio/adc/ti-adc128s052.c
+++ b/drivers/iio/adc/ti-adc128s052.c
@@ -139,16 +139,11 @@ static void adc128_disable_regulator(void *reg)
 
 static int adc128_probe(struct spi_device *spi)
 {
+	const struct adc128_configuration *config;
 	struct iio_dev *indio_dev;
-	unsigned int config;
 	struct adc128 *adc;
 	int ret;
 
-	if (dev_fwnode(&spi->dev))
-		config = (unsigned long) device_get_match_data(&spi->dev);
-	else
-		config = spi_get_device_id(spi)->driver_data;
-
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
 	if (!indio_dev)
 		return -ENOMEM;
@@ -160,6 +155,8 @@ static int adc128_probe(struct spi_device *spi)
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &adc128_info;
 
+	config = spi_get_device_match_data(&spi->dev);
+
 	indio_dev->channels = adc128_config[config].channels;
 	indio_dev->num_channels = adc128_config[config].num_channels;
 
@@ -181,32 +178,32 @@ static int adc128_probe(struct spi_device *spi)
 }
 
 static const struct of_device_id adc128_of_match[] = {
-	{ .compatible = "ti,adc128s052", .data = (void*)0L, },
-	{ .compatible = "ti,adc122s021", .data = (void*)1L, },
-	{ .compatible = "ti,adc122s051", .data = (void*)1L, },
-	{ .compatible = "ti,adc122s101", .data = (void*)1L, },
-	{ .compatible = "ti,adc124s021", .data = (void*)2L, },
-	{ .compatible = "ti,adc124s051", .data = (void*)2L, },
-	{ .compatible = "ti,adc124s101", .data = (void*)2L, },
+	{ .compatible = "ti,adc128s052", .data = &adc128_config[0] },
+	{ .compatible = "ti,adc122s021", .data = &adc128_config[1] },
+	{ .compatible = "ti,adc122s051", .data = &adc128_config[1] },
+	{ .compatible = "ti,adc122s101", .data = &adc128_config[1] },
+	{ .compatible = "ti,adc124s021", .data = &adc128_config[2] },
+	{ .compatible = "ti,adc124s051", .data = &adc128_config[2] },
+	{ .compatible = "ti,adc124s101", .data = &adc128_config[2] },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, adc128_of_match);
 
 static const struct spi_device_id adc128_id[] = {
-	{ "adc128s052", 0 },	/* index into adc128_config */
-	{ "adc122s021",	1 },
-	{ "adc122s051",	1 },
-	{ "adc122s101",	1 },
-	{ "adc124s021", 2 },
-	{ "adc124s051", 2 },
-	{ "adc124s101", 2 },
+	{ "adc128s052", (kernel_ulong_t)&adc128_config[0] },
+	{ "adc122s021",	(kernel_ulong_t)&adc128_config[1] },
+	{ "adc122s051",	(kernel_ulong_t)&adc128_config[1] },
+	{ "adc122s101",	(kernel_ulong_t)&adc128_config[1] },
+	{ "adc124s021", (kernel_ulong_t)&adc128_config[2] },
+	{ "adc124s051", (kernel_ulong_t)&adc128_config[2] },
+	{ "adc124s101", (kernel_ulong_t)&adc128_config[2] },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, adc128_id);
 
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id adc128_acpi_match[] = {
-	{ "AANT1280", 2 }, /* ADC124S021 compatible ACPI ID */
+	{ "AANT1280", (kernel_ulong_t)&adc128_config[2] },
 	{ }
 };
 MODULE_DEVICE_TABLE(acpi, adc128_acpi_match);
-- 
2.35.1

[PATCH v1 2/2] iio: adc: ti-adc128s052: Drop anti-pattern of ACPI_PTR() use

[Andy Shevchenko]

ACPI_PTR() is more harmful than helpful. For example, in this case
if CONFIG_ACPI=n, the ID table left unused and code is obfuscated
by ifdeffery.

Drop anti-pattern of ACPI_PTR() use.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/adc/ti-adc128s052.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
index 9dfc625100b6..fc09ee6bb174 100644
--- a/drivers/iio/adc/ti-adc128s052.c
+++ b/drivers/iio/adc/ti-adc128s052.c
@@ -9,7 +9,6 @@
  * https://www.ti.com/lit/ds/symlink/adc124s021.pdf
  */
 
-#include <linux/acpi.h>
 #include <linux/err.h>
 #include <linux/spi/spi.h>
 #include <linux/module.h>
@@ -201,19 +200,17 @@ static const struct spi_device_id adc128_id[] = {
 };
 MODULE_DEVICE_TABLE(spi, adc128_id);
 
-#ifdef CONFIG_ACPI
 static const struct acpi_device_id adc128_acpi_match[] = {
 	{ "AANT1280", (kernel_ulong_t)&adc128_config[2] },
 	{ }
 };
 MODULE_DEVICE_TABLE(acpi, adc128_acpi_match);
-#endif
 
 static struct spi_driver adc128_driver = {
 	.driver = {
 		.name = "adc128s052",
 		.of_match_table = adc128_of_match,
-		.acpi_match_table = ACPI_PTR(adc128_acpi_match),
+		.acpi_match_table = adc128_acpi_match,
 	},
 	.probe = adc128_probe,
 	.id_table = adc128_id,
-- 
2.35.1

Re: [PATCH v1 1/2] iio: adc: ti-adc128s052: Switch to use spi_get_device_match_data()

[Jonathan Cameron]

On Wed, 14 Dec 2022 13:49:43 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> The spi_get_device_match_data() helps to get driver data from the
> firmware node or SPI ID table. Use it instead of open coding.
> 
> While at it, switch ID tables to provide an acrual pointers to
> the configuration data.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> 
> Requires aea672d054a2 ("spi: Introduce spi_get_device_match_data()
> helper") which is part of upstream as of today.

I rebased to get that (will rebase again on rc1).

Applied to the togreg branch of iio.git and pushed out as testing
to keep 0-day busy over my holidays.

Jonathan

> 
>  drivers/iio/adc/ti-adc128s052.c | 39 +++++++++++++++------------------
>  1 file changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> index b3d5b9b7255b..9dfc625100b6 100644
> --- a/drivers/iio/adc/ti-adc128s052.c
> +++ b/drivers/iio/adc/ti-adc128s052.c
> @@ -139,16 +139,11 @@ static void adc128_disable_regulator(void *reg)
>  
>  static int adc128_probe(struct spi_device *spi)
>  {
> +	const struct adc128_configuration *config;
>  	struct iio_dev *indio_dev;
> -	unsigned int config;
>  	struct adc128 *adc;
>  	int ret;
>  
> -	if (dev_fwnode(&spi->dev))
> -		config = (unsigned long) device_get_match_data(&spi->dev);
> -	else
> -		config = spi_get_device_id(spi)->driver_data;
> -
>  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
>  	if (!indio_dev)
>  		return -ENOMEM;
> @@ -160,6 +155,8 @@ static int adc128_probe(struct spi_device *spi)
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->info = &adc128_info;
>  
> +	config = spi_get_device_match_data(&spi->dev);
> +
>  	indio_dev->channels = adc128_config[config].channels;
>  	indio_dev->num_channels = adc128_config[config].num_channels;
>  
> @@ -181,32 +178,32 @@ static int adc128_probe(struct spi_device *spi)
>  }
>  
>  static const struct of_device_id adc128_of_match[] = {
> -	{ .compatible = "ti,adc128s052", .data = (void*)0L, },
> -	{ .compatible = "ti,adc122s021", .data = (void*)1L, },
> -	{ .compatible = "ti,adc122s051", .data = (void*)1L, },
> -	{ .compatible = "ti,adc122s101", .data = (void*)1L, },
> -	{ .compatible = "ti,adc124s021", .data = (void*)2L, },
> -	{ .compatible = "ti,adc124s051", .data = (void*)2L, },
> -	{ .compatible = "ti,adc124s101", .data = (void*)2L, },
> +	{ .compatible = "ti,adc128s052", .data = &adc128_config[0] },
> +	{ .compatible = "ti,adc122s021", .data = &adc128_config[1] },
> +	{ .compatible = "ti,adc122s051", .data = &adc128_config[1] },
> +	{ .compatible = "ti,adc122s101", .data = &adc128_config[1] },
> +	{ .compatible = "ti,adc124s021", .data = &adc128_config[2] },
> +	{ .compatible = "ti,adc124s051", .data = &adc128_config[2] },
> +	{ .compatible = "ti,adc124s101", .data = &adc128_config[2] },
>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, adc128_of_match);
>  
>  static const struct spi_device_id adc128_id[] = {
> -	{ "adc128s052", 0 },	/* index into adc128_config */
> -	{ "adc122s021",	1 },
> -	{ "adc122s051",	1 },
> -	{ "adc122s101",	1 },
> -	{ "adc124s021", 2 },
> -	{ "adc124s051", 2 },
> -	{ "adc124s101", 2 },
> +	{ "adc128s052", (kernel_ulong_t)&adc128_config[0] },
> +	{ "adc122s021",	(kernel_ulong_t)&adc128_config[1] },
> +	{ "adc122s051",	(kernel_ulong_t)&adc128_config[1] },
> +	{ "adc122s101",	(kernel_ulong_t)&adc128_config[1] },
> +	{ "adc124s021", (kernel_ulong_t)&adc128_config[2] },
> +	{ "adc124s051", (kernel_ulong_t)&adc128_config[2] },
> +	{ "adc124s101", (kernel_ulong_t)&adc128_config[2] },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(spi, adc128_id);
>  
>  #ifdef CONFIG_ACPI
>  static const struct acpi_device_id adc128_acpi_match[] = {
> -	{ "AANT1280", 2 }, /* ADC124S021 compatible ACPI ID */
> +	{ "AANT1280", (kernel_ulong_t)&adc128_config[2] },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(acpi, adc128_acpi_match);

Re: [PATCH v1 2/2] iio: adc: ti-adc128s052: Drop anti-pattern of ACPI_PTR() use

[Jonathan Cameron]

On Wed, 14 Dec 2022 13:49:44 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> ACPI_PTR() is more harmful than helpful. For example, in this case
> if CONFIG_ACPI=n, the ID table left unused and code is obfuscated
> by ifdeffery.
> 
> Drop anti-pattern of ACPI_PTR() use.
Longer term I wonder if we should just make ACPI_PTR() work the same
way as pm_ptr() now does so that the visibility is maintained and
we don't need the ifdef magic.

I'm sure there is someone out there somewhere who actually cares about
the minor costs of all these tables for their non ACPI platform and
doing something like that might make everyone happier.

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Applied.
> ---
>  drivers/iio/adc/ti-adc128s052.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> index 9dfc625100b6..fc09ee6bb174 100644
> --- a/drivers/iio/adc/ti-adc128s052.c
> +++ b/drivers/iio/adc/ti-adc128s052.c
> @@ -9,7 +9,6 @@
>   * https://www.ti.com/lit/ds/symlink/adc124s021.pdf
>   */
>  
> -#include <linux/acpi.h>
>  #include <linux/err.h>
>  #include <linux/spi/spi.h>
>  #include <linux/module.h>
> @@ -201,19 +200,17 @@ static const struct spi_device_id adc128_id[] = {
>  };
>  MODULE_DEVICE_TABLE(spi, adc128_id);
>  
> -#ifdef CONFIG_ACPI
>  static const struct acpi_device_id adc128_acpi_match[] = {
>  	{ "AANT1280", (kernel_ulong_t)&adc128_config[2] },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(acpi, adc128_acpi_match);
> -#endif
>  
>  static struct spi_driver adc128_driver = {
>  	.driver = {
>  		.name = "adc128s052",
>  		.of_match_table = adc128_of_match,
> -		.acpi_match_table = ACPI_PTR(adc128_acpi_match),
> +		.acpi_match_table = adc128_acpi_match,
>  	},
>  	.probe = adc128_probe,
>  	.id_table = adc128_id,

Re: [PATCH v1 1/2] iio: adc: ti-adc128s052: Switch to use spi_get_device_match_data()

[Jonathan Cameron]

On Fri, 23 Dec 2022 15:22:42 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Wed, 14 Dec 2022 13:49:43 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> 
> > The spi_get_device_match_data() helps to get driver data from the
> > firmware node or SPI ID table. Use it instead of open coding.
> > 
> > While at it, switch ID tables to provide an acrual pointers to
> > the configuration data.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> > 
> > Requires aea672d054a2 ("spi: Introduce spi_get_device_match_data()
> > helper") which is part of upstream as of today.  
> 
> I rebased to get that (will rebase again on rc1).
> 
> Applied to the togreg branch of iio.git and pushed out as testing
> to keep 0-day busy over my holidays.

I take it back...  Build test failed...

> 
> Jonathan
> 
> > 
> >  drivers/iio/adc/ti-adc128s052.c | 39 +++++++++++++++------------------
> >  1 file changed, 18 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/iio/adc/ti-adc128s052.c b/drivers/iio/adc/ti-adc128s052.c
> > index b3d5b9b7255b..9dfc625100b6 100644
> > --- a/drivers/iio/adc/ti-adc128s052.c
> > +++ b/drivers/iio/adc/ti-adc128s052.c
> > @@ -139,16 +139,11 @@ static void adc128_disable_regulator(void *reg)
> >  
> >  static int adc128_probe(struct spi_device *spi)
> >  {
> > +	const struct adc128_configuration *config;
> >  	struct iio_dev *indio_dev;
> > -	unsigned int config;
> >  	struct adc128 *adc;
> >  	int ret;
> >  
> > -	if (dev_fwnode(&spi->dev))
> > -		config = (unsigned long) device_get_match_data(&spi->dev);
> > -	else
> > -		config = spi_get_device_id(spi)->driver_data;
> > -
> >  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> >  	if (!indio_dev)
> >  		return -ENOMEM;
> > @@ -160,6 +155,8 @@ static int adc128_probe(struct spi_device *spi)
> >  	indio_dev->modes = INDIO_DIRECT_MODE;
> >  	indio_dev->info = &adc128_info;
> >  
> > +	config = spi_get_device_match_data(&spi->dev);
Takes a struct spi_device*

Also, having opened code up to fix this, we have a spi_get_device_id() left
just above that needs dealing with.

And a lot of uses of config below that need fixing.

I'm dropping this series until that's all fixed up.

Jonathan



> > +
> >  	indio_dev->channels = adc128_config[config].channels;
> >  	indio_dev->num_channels = adc128_config[config].num_channels;
> >  
> > @@ -181,32 +178,32 @@ static int adc128_probe(struct spi_device *spi)
> >  }
> >  
> >  static const struct of_device_id adc128_of_match[] = {
> > -	{ .compatible = "ti,adc128s052", .data = (void*)0L, },
> > -	{ .compatible = "ti,adc122s021", .data = (void*)1L, },
> > -	{ .compatible = "ti,adc122s051", .data = (void*)1L, },
> > -	{ .compatible = "ti,adc122s101", .data = (void*)1L, },
> > -	{ .compatible = "ti,adc124s021", .data = (void*)2L, },
> > -	{ .compatible = "ti,adc124s051", .data = (void*)2L, },
> > -	{ .compatible = "ti,adc124s101", .data = (void*)2L, },
> > +	{ .compatible = "ti,adc128s052", .data = &adc128_config[0] },
> > +	{ .compatible = "ti,adc122s021", .data = &adc128_config[1] },
> > +	{ .compatible = "ti,adc122s051", .data = &adc128_config[1] },
> > +	{ .compatible = "ti,adc122s101", .data = &adc128_config[1] },
> > +	{ .compatible = "ti,adc124s021", .data = &adc128_config[2] },
> > +	{ .compatible = "ti,adc124s051", .data = &adc128_config[2] },
> > +	{ .compatible = "ti,adc124s101", .data = &adc128_config[2] },
> >  	{ /* sentinel */ },
> >  };
> >  MODULE_DEVICE_TABLE(of, adc128_of_match);
> >  
> >  static const struct spi_device_id adc128_id[] = {
> > -	{ "adc128s052", 0 },	/* index into adc128_config */
> > -	{ "adc122s021",	1 },
> > -	{ "adc122s051",	1 },
> > -	{ "adc122s101",	1 },
> > -	{ "adc124s021", 2 },
> > -	{ "adc124s051", 2 },
> > -	{ "adc124s101", 2 },
> > +	{ "adc128s052", (kernel_ulong_t)&adc128_config[0] },
> > +	{ "adc122s021",	(kernel_ulong_t)&adc128_config[1] },
> > +	{ "adc122s051",	(kernel_ulong_t)&adc128_config[1] },
> > +	{ "adc122s101",	(kernel_ulong_t)&adc128_config[1] },
> > +	{ "adc124s021", (kernel_ulong_t)&adc128_config[2] },
> > +	{ "adc124s051", (kernel_ulong_t)&adc128_config[2] },
> > +	{ "adc124s101", (kernel_ulong_t)&adc128_config[2] },
> >  	{ }
> >  };
> >  MODULE_DEVICE_TABLE(spi, adc128_id);
> >  
> >  #ifdef CONFIG_ACPI
> >  static const struct acpi_device_id adc128_acpi_match[] = {
> > -	{ "AANT1280", 2 }, /* ADC124S021 compatible ACPI ID */
> > +	{ "AANT1280", (kernel_ulong_t)&adc128_config[2] },
> >  	{ }
> >  };
> >  MODULE_DEVICE_TABLE(acpi, adc128_acpi_match);  
> 

Re: [PATCH v1 1/2] iio: adc: ti-adc128s052: Switch to use spi_get_device_match_data()

[Andy Shevchenko]

On Fri, Dec 23, 2022 at 03:44:50PM +0000, Jonathan Cameron wrote:
> On Fri, 23 Dec 2022 15:22:42 +0000
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Wed, 14 Dec 2022 13:49:43 +0200
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > 
> > > The spi_get_device_match_data() helps to get driver data from the
> > > firmware node or SPI ID table. Use it instead of open coding.
> > > 
> > > While at it, switch ID tables to provide an acrual pointers to
> > > the configuration data.
> > > 
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > > 
> > > Requires aea672d054a2 ("spi: Introduce spi_get_device_match_data()
> > > helper") which is part of upstream as of today.  
> > 
> > I rebased to get that (will rebase again on rc1).
> > 
> > Applied to the togreg branch of iio.git and pushed out as testing
> > to keep 0-day busy over my holidays.
> 
> I take it back...  Build test failed...

As comment on the first patch stays this requires an SPI core patch, which is
now the part of the v6.2-rc1.

Can you reapply it taking the above into consideration?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] iio: adc: ti-adc128s052: Switch to use spi_get_device_match_data()

[Jonathan Cameron]

On Wed, 28 Dec 2022 11:59:53 +0200
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Fri, Dec 23, 2022 at 03:44:50PM +0000, Jonathan Cameron wrote:
> > On Fri, 23 Dec 2022 15:22:42 +0000
> > Jonathan Cameron <jic23@kernel.org> wrote:
> >   
> > > On Wed, 14 Dec 2022 13:49:43 +0200
> > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > >   
> > > > The spi_get_device_match_data() helps to get driver data from the
> > > > firmware node or SPI ID table. Use it instead of open coding.
> > > > 
> > > > While at it, switch ID tables to provide an acrual pointers to
> > > > the configuration data.
> > > > 
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > > ---
> > > > 
> > > > Requires aea672d054a2 ("spi: Introduce spi_get_device_match_data()
> > > > helper") which is part of upstream as of today.    
> > > 
> > > I rebased to get that (will rebase again on rc1).
> > > 
> > > Applied to the togreg branch of iio.git and pushed out as testing
> > > to keep 0-day busy over my holidays.  
> > 
> > I take it back...  Build test failed...  
> 
> As comment on the first patch stays this requires an SPI core patch, which is
> now the part of the v6.2-rc1.
> 
> Can you reapply it taking the above into consideration?
>

I should have been more specific though I do mention rebasing to get the
patch above.. Doesn't build with it.

Signature of spi_get_device_match_data is:
extern const void *
spi_get_device_match_data(const struct spi_device *sdev);

and you are passing it a struct device * which rather implies you didn't
successfully build test this.

Jonathan

Re: [PATCH v1 1/2] iio: adc: ti-adc128s052: Switch to use spi_get_device_match_data()

[Andy Shevchenko]

On Sat, Dec 31, 2022 at 02:45:58PM +0000, Jonathan Cameron wrote:
> On Wed, 28 Dec 2022 11:59:53 +0200
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

...

> I should have been more specific though I do mention rebasing to get the
> patch above.. Doesn't build with it.
> 
> Signature of spi_get_device_match_data is:
> extern const void *
> spi_get_device_match_data(const struct spi_device *sdev);
> 
> and you are passing it a struct device * which rather implies you didn't
> successfully build test this.

Definitely. Thanks for spotting this, I'll investigate what happened on my side
that it wasn't built.

-- 
With Best Regards,
Andy Shevchenko