diff mbox series

[18/23] iio:adc:ti-adc108s102: Drop CONFIG_OF and of_match_ptr protections

Message ID 20200628123654.32830-19-jic23@kernel.org (mailing list archive)
State New, archived
Headers show
Series iio:adc more of_match_ptr and similar removal | expand

Commit Message

Jonathan Cameron June 28, 2020, 12:36 p.m. UTC 
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Although this driver has an explicit ACPI binding it is good to also
allow for PRP0001 use of the of_match_id table.

I'm also trying to clean this (now) anti-pattern out of IIO to avoid
cut and paste into new drivers.

Also add an include of mod_devicetable.h as the driver directly uses
struct of_device_id which is defined in there.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/iio/adc/ti-adc108s102.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Andy Shevchenko June 30, 2020, 7:20 a.m. UTC | #1 
On Sun, Jun 28, 2020 at 3:39 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Although this driver has an explicit ACPI binding it is good to also
> allow for PRP0001 use of the of_match_id table.
>
> I'm also trying to clean this (now) anti-pattern out of IIO to avoid
> cut and paste into new drivers.
>
> Also add an include of mod_devicetable.h as the driver directly uses
> struct of_device_id which is defined in there.
>

I think this is not needed. At least the commit message in this case
is misleading.
We have only one ID and ACPI already has it. So, there is no need for
this patch from that perspective.

> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  drivers/iio/adc/ti-adc108s102.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/adc/ti-adc108s102.c b/drivers/iio/adc/ti-adc108s102.c
> index 9b9b27415c93..183b2245e89b 100644
> --- a/drivers/iio/adc/ti-adc108s102.c
> +++ b/drivers/iio/adc/ti-adc108s102.c
> @@ -20,6 +20,7 @@
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/interrupt.h>
>  #include <linux/module.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/property.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/spi/spi.h>
> @@ -299,13 +300,11 @@ static int adc108s102_remove(struct spi_device *spi)
>         return 0;
>  }
>
> -#ifdef CONFIG_OF
>  static const struct of_device_id adc108s102_of_match[] = {
>         { .compatible = "ti,adc108s102" },
>         { }
>  };
>  MODULE_DEVICE_TABLE(of, adc108s102_of_match);
> -#endif
>
>  #ifdef CONFIG_ACPI
>  static const struct acpi_device_id adc108s102_acpi_ids[] = {
> @@ -324,7 +323,7 @@ MODULE_DEVICE_TABLE(spi, adc108s102_id);
>  static struct spi_driver adc108s102_driver = {
>         .driver = {
>                 .name   = "adc108s102",
> -               .of_match_table = of_match_ptr(adc108s102_of_match),
> +               .of_match_table = adc108s102_of_match,
>                 .acpi_match_table = ACPI_PTR(adc108s102_acpi_ids),
>         },
>         .probe          = adc108s102_probe,
> --
> 2.27.0
>
Jonathan Cameron July 4, 2020, 3:54 p.m. UTC | #2 
On Tue, 30 Jun 2020 10:20:55 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Sun, Jun 28, 2020 at 3:39 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > Although this driver has an explicit ACPI binding it is good to also
> > allow for PRP0001 use of the of_match_id table.
> >
> > I'm also trying to clean this (now) anti-pattern out of IIO to avoid
> > cut and paste into new drivers.
> >
> > Also add an include of mod_devicetable.h as the driver directly uses
> > struct of_device_id which is defined in there.
> >  
> 
> I think this is not needed. At least the commit message in this case
> is misleading.
> We have only one ID and ACPI already has it. So, there is no need for
> this patch from that perspective.

It would be a bit odd if non Intel boards used an Intel ID for a TI
part.  Particularly I as can't immediately find any public docs
of the ID.  Do you know if these are documented publicly anywhere?

I know we don't do this in an ideal fashion as we have an internal
registry but I don't think we publish it anywhere, but then we don't
really expect anyone else to use them. I'm not aware of us having
issued IDs for parts made by anyone other than ourselves.

So in general I'd rather we gave the option for using the dt binding
PRP0001 route in addition to the intel ACPI ID.

Thanks,

Jonathan

> 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Cc: Jan Kiszka <jan.kiszka@siemens.com>
> > ---
> >  drivers/iio/adc/ti-adc108s102.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ti-adc108s102.c b/drivers/iio/adc/ti-adc108s102.c
> > index 9b9b27415c93..183b2245e89b 100644
> > --- a/drivers/iio/adc/ti-adc108s102.c
> > +++ b/drivers/iio/adc/ti-adc108s102.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/iio/trigger_consumer.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> >  #include <linux/property.h>
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/spi/spi.h>
> > @@ -299,13 +300,11 @@ static int adc108s102_remove(struct spi_device *spi)
> >         return 0;
> >  }
> >
> > -#ifdef CONFIG_OF
> >  static const struct of_device_id adc108s102_of_match[] = {
> >         { .compatible = "ti,adc108s102" },
> >         { }
> >  };
> >  MODULE_DEVICE_TABLE(of, adc108s102_of_match);
> > -#endif
> >
> >  #ifdef CONFIG_ACPI
> >  static const struct acpi_device_id adc108s102_acpi_ids[] = {
> > @@ -324,7 +323,7 @@ MODULE_DEVICE_TABLE(spi, adc108s102_id);
> >  static struct spi_driver adc108s102_driver = {
> >         .driver = {
> >                 .name   = "adc108s102",
> > -               .of_match_table = of_match_ptr(adc108s102_of_match),
> > +               .of_match_table = adc108s102_of_match,
> >                 .acpi_match_table = ACPI_PTR(adc108s102_acpi_ids),
> >         },
> >         .probe          = adc108s102_probe,
> > --
> > 2.27.0
> >  
> 
>
Andy Shevchenko July 4, 2020, 6:16 p.m. UTC | #3 
On Sat, Jul 4, 2020 at 6:55 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 30 Jun 2020 10:20:55 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> > On Sun, Jun 28, 2020 at 3:39 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > >
> > > Although this driver has an explicit ACPI binding it is good to also
> > > allow for PRP0001 use of the of_match_id table.
> > >
> > > I'm also trying to clean this (now) anti-pattern out of IIO to avoid
> > > cut and paste into new drivers.
> > >
> > > Also add an include of mod_devicetable.h as the driver directly uses
> > > struct of_device_id which is defined in there.
> > >
> >
> > I think this is not needed. At least the commit message in this case
> > is misleading.
> > We have only one ID and ACPI already has it. So, there is no need for
> > this patch from that perspective.
>
> It would be a bit odd if non Intel boards used an Intel ID for a TI
> part.  Particularly I as can't immediately find any public docs
> of the ID.  Do you know if these are documented publicly anywhere?

Unfortunately it's vendor specific stuff, only vendor prefixes are
documented on uefi.org.
And it's fine to use an already existing ID for the component from the
same company.
The thing is that this is a really discrete component from TI on Intel
Galileo board.

> I know we don't do this in an ideal fashion as we have an internal
> registry but I don't think we publish it anywhere, but then we don't
> really expect anyone else to use them. I'm not aware of us having
> issued IDs for parts made by anyone other than ourselves.
>
> So in general I'd rather we gave the option for using the dt binding
> PRP0001 route in addition to the intel ACPI ID.

I'm not against the clean up in code, but I don't think it is good to
encourage users to use PRP0001 when the ACPI ID is already allocated
(doesn't matter by whom).

So, I would recommend to leave the change, but rephrase the commit
message to avoid PRP0001 mentioning.
diff mbox series

Patch

diff --git a/drivers/iio/adc/ti-adc108s102.c b/drivers/iio/adc/ti-adc108s102.c
index 9b9b27415c93..183b2245e89b 100644
--- a/drivers/iio/adc/ti-adc108s102.c
+++ b/drivers/iio/adc/ti-adc108s102.c
@@ -20,6 +20,7 @@ 
 #include <linux/iio/trigger_consumer.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/mod_devicetable.h>
 #include <linux/property.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
@@ -299,13 +300,11 @@  static int adc108s102_remove(struct spi_device *spi)
 	return 0;
 }
 
-#ifdef CONFIG_OF
 static const struct of_device_id adc108s102_of_match[] = {
 	{ .compatible = "ti,adc108s102" },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, adc108s102_of_match);
-#endif
 
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id adc108s102_acpi_ids[] = {
@@ -324,7 +323,7 @@  MODULE_DEVICE_TABLE(spi, adc108s102_id);
 static struct spi_driver adc108s102_driver = {
 	.driver = {
 		.name   = "adc108s102",
-		.of_match_table = of_match_ptr(adc108s102_of_match),
+		.of_match_table = adc108s102_of_match,
 		.acpi_match_table = ACPI_PTR(adc108s102_acpi_ids),
 	},
 	.probe		= adc108s102_probe,