diff mbox series

[15/23] iio:adc:ti-adc081c: Drop of_match_ptr and change to mod_devicetable.h

Message ID 20200628123654.32830-16-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>

Whilst this driver already supports explicit ACPI bindings we
might as well also allow for PRP0001 based binding.

I'm also keen to remove of_match_ptr from IIO drivers to avoid
this (now) anti-pattern getting coppied into new drivers.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/iio/adc/ti-adc081c.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Andy Shevchenko June 30, 2020, 7:12 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>
>
> Whilst this driver already supports explicit ACPI bindings we
> might as well also allow for PRP0001 based binding.
>
> I'm also keen to remove of_match_ptr from IIO drivers to avoid
> this (now) anti-pattern getting coppied into new drivers.
>

Code LGTM, but see below.

> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/iio/adc/ti-adc081c.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/adc/ti-adc081c.c b/drivers/iio/adc/ti-adc081c.c
> index 82e524b3db88..e00350e6503f 100644
> --- a/drivers/iio/adc/ti-adc081c.c
> +++ b/drivers/iio/adc/ti-adc081c.c
> @@ -18,7 +18,7 @@
>  #include <linux/err.h>
>  #include <linux/i2c.h>
>  #include <linux/module.h>
> -#include <linux/of.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/acpi.h>
>
>  #include <linux/iio/iio.h>
> @@ -230,7 +230,6 @@ static const struct i2c_device_id adc081c_id[] = {
>  };
>  MODULE_DEVICE_TABLE(i2c, adc081c_id);
>
> -#ifdef CONFIG_OF
>  static const struct of_device_id adc081c_of_match[] = {
>         { .compatible = "ti,adc081c" },
>         { .compatible = "ti,adc101c" },
> @@ -238,7 +237,6 @@ static const struct of_device_id adc081c_of_match[] = {
>         { }
>  };
>  MODULE_DEVICE_TABLE(of, adc081c_of_match);
> -#endif
>
>  #ifdef CONFIG_ACPI
>  static const struct acpi_device_id adc081c_acpi_match[] = {

These IDs seem to me artificial (and non-official). Perhaps in a
separate patch remove them?
Or do we have confirmation (in writing) from TI that these are okay?

> @@ -253,7 +251,7 @@ MODULE_DEVICE_TABLE(acpi, adc081c_acpi_match);
>  static struct i2c_driver adc081c_driver = {
>         .driver = {
>                 .name = "adc081c",
> -               .of_match_table = of_match_ptr(adc081c_of_match),
> +               .of_match_table = adc081c_of_match,
>                 .acpi_match_table = ACPI_PTR(adc081c_acpi_match),
>         },
>         .probe = adc081c_probe,
> --
> 2.27.0
>
Jonathan Cameron July 4, 2020, 3:45 p.m. UTC | #2 
On Tue, 30 Jun 2020 10:12:51 +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>
> >
> > Whilst this driver already supports explicit ACPI bindings we
> > might as well also allow for PRP0001 based binding.
> >
> > I'm also keen to remove of_match_ptr from IIO drivers to avoid
> > this (now) anti-pattern getting coppied into new drivers.
> >  
> 
> Code LGTM, but see below.
> 
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  drivers/iio/adc/ti-adc081c.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/iio/adc/ti-adc081c.c b/drivers/iio/adc/ti-adc081c.c
> > index 82e524b3db88..e00350e6503f 100644
> > --- a/drivers/iio/adc/ti-adc081c.c
> > +++ b/drivers/iio/adc/ti-adc081c.c
> > @@ -18,7 +18,7 @@
> >  #include <linux/err.h>
> >  #include <linux/i2c.h>
> >  #include <linux/module.h>
> > -#include <linux/of.h>
> > +#include <linux/mod_devicetable.h>
> >  #include <linux/acpi.h>
> >
> >  #include <linux/iio/iio.h>
> > @@ -230,7 +230,6 @@ static const struct i2c_device_id adc081c_id[] = {
> >  };
> >  MODULE_DEVICE_TABLE(i2c, adc081c_id);
> >
> > -#ifdef CONFIG_OF
> >  static const struct of_device_id adc081c_of_match[] = {
> >         { .compatible = "ti,adc081c" },
> >         { .compatible = "ti,adc101c" },
> > @@ -238,7 +237,6 @@ static const struct of_device_id adc081c_of_match[] = {
> >         { }
> >  };
> >  MODULE_DEVICE_TABLE(of, adc081c_of_match);
> > -#endif
> >
> >  #ifdef CONFIG_ACPI
> >  static const struct acpi_device_id adc081c_acpi_match[] = {  
> 
> These IDs seem to me artificial (and non-official). Perhaps in a
> separate patch remove them?
> Or do we have confirmation (in writing) from TI that these are okay?
+CC Dan O'Donovan,

It seems highly unlikely these are 'official'.

Dan.  You added them, can you give us some background (admittedly
4 years ago so you may not recall!)

Unfortunately I was rather less aware of ACPI than I have become in
the meantime, so let these in without questioning them.

If we have these out there in in the wild, we can still add a note
to make it clear that people should avoid using them in future,
or copying the approach in other drivers.

Jonathan

> 
> > @@ -253,7 +251,7 @@ MODULE_DEVICE_TABLE(acpi, adc081c_acpi_match);
> >  static struct i2c_driver adc081c_driver = {
> >         .driver = {
> >                 .name = "adc081c",
> > -               .of_match_table = of_match_ptr(adc081c_of_match),
> > +               .of_match_table = adc081c_of_match,
> >                 .acpi_match_table = ACPI_PTR(adc081c_acpi_match),
> >         },
> >         .probe = adc081c_probe,
> > --
> > 2.27.0
> >  
> 
>
Jonathan Cameron July 4, 2020, 5:26 p.m. UTC | #3 
On Sat, 4 Jul 2020 16:45:05 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 30 Jun 2020 10:12:51 +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>
> > >
> > > Whilst this driver already supports explicit ACPI bindings we
> > > might as well also allow for PRP0001 based binding.
> > >
> > > I'm also keen to remove of_match_ptr from IIO drivers to avoid
> > > this (now) anti-pattern getting coppied into new drivers.
> > >    
> > 
> > Code LGTM, but see below.
> >   
> > > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > > ---
> > >  drivers/iio/adc/ti-adc081c.c | 6 ++----
> > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/iio/adc/ti-adc081c.c b/drivers/iio/adc/ti-adc081c.c
> > > index 82e524b3db88..e00350e6503f 100644
> > > --- a/drivers/iio/adc/ti-adc081c.c
> > > +++ b/drivers/iio/adc/ti-adc081c.c
> > > @@ -18,7 +18,7 @@
> > >  #include <linux/err.h>
> > >  #include <linux/i2c.h>
> > >  #include <linux/module.h>
> > > -#include <linux/of.h>
> > > +#include <linux/mod_devicetable.h>
> > >  #include <linux/acpi.h>
> > >
> > >  #include <linux/iio/iio.h>
> > > @@ -230,7 +230,6 @@ static const struct i2c_device_id adc081c_id[] = {
> > >  };
> > >  MODULE_DEVICE_TABLE(i2c, adc081c_id);
> > >
> > > -#ifdef CONFIG_OF
> > >  static const struct of_device_id adc081c_of_match[] = {
> > >         { .compatible = "ti,adc081c" },
> > >         { .compatible = "ti,adc101c" },
> > > @@ -238,7 +237,6 @@ static const struct of_device_id adc081c_of_match[] = {
> > >         { }
> > >  };
> > >  MODULE_DEVICE_TABLE(of, adc081c_of_match);
> > > -#endif
> > >
> > >  #ifdef CONFIG_ACPI
> > >  static const struct acpi_device_id adc081c_acpi_match[] = {    
> > 
> > These IDs seem to me artificial (and non-official). Perhaps in a
> > separate patch remove them?
> > Or do we have confirmation (in writing) from TI that these are okay?  
> +CC Dan O'Donovan,
> 
> It seems highly unlikely these are 'official'.
> 
> Dan.  You added them, can you give us some background (admittedly
> 4 years ago so you may not recall!)
> 
> Unfortunately I was rather less aware of ACPI than I have become in
> the meantime, so let these in without questioning them.
> 
> If we have these out there in in the wild, we can still add a note
> to make it clear that people should avoid using them in future,
> or copying the approach in other drivers.
> 
As this is a separate issue (kind of) I've applied this patch and
we can address whether to remove the ACPI bindings separately.

Thanks,

Jonathan

> Jonathan
> 
> >   
> > > @@ -253,7 +251,7 @@ MODULE_DEVICE_TABLE(acpi, adc081c_acpi_match);
> > >  static struct i2c_driver adc081c_driver = {
> > >         .driver = {
> > >                 .name = "adc081c",
> > > -               .of_match_table = of_match_ptr(adc081c_of_match),
> > > +               .of_match_table = adc081c_of_match,
> > >                 .acpi_match_table = ACPI_PTR(adc081c_acpi_match),
> > >         },
> > >         .probe = adc081c_probe,
> > > --
> > > 2.27.0
> > >    
> > 
> >   
>
Andy Shevchenko July 4, 2020, 6:31 p.m. UTC | #4 
On Sat, Jul 4, 2020 at 8:26 PM Jonathan Cameron
<jic23@jic23.retrosnub.co.uk> wrote:
> On Sat, 4 Jul 2020 16:45:05 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> > On Tue, 30 Jun 2020 10:12:51 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > On Sun, Jun 28, 2020 at 3:39 PM Jonathan Cameron <jic23@kernel.org> wrote:

...

> > > >  #ifdef CONFIG_ACPI
> > > >  static const struct acpi_device_id adc081c_acpi_match[] = {
> > >
> > > These IDs seem to me artificial (and non-official). Perhaps in a
> > > separate patch remove them?
> > > Or do we have confirmation (in writing) from TI that these are okay?
> > +CC Dan O'Donovan,
> >
> > It seems highly unlikely these are 'official'.

+1 here.

> > Dan.  You added them, can you give us some background (admittedly
> > 4 years ago so you may not recall!)
> >
> > Unfortunately I was rather less aware of ACPI than I have become in
> > the meantime, so let these in without questioning them.
> >
> > If we have these out there in in the wild, we can still add a note
> > to make it clear that people should avoid using them in future,
> > or copying the approach in other drivers.
> >
> As this is a separate issue (kind of) I've applied this patch and
> we can address whether to remove the ACPI bindings separately.

Agree. Just to mention one more time that this shouldn't be forgotten.
Also we can ask somebody from TI.
diff mbox series

Patch

diff --git a/drivers/iio/adc/ti-adc081c.c b/drivers/iio/adc/ti-adc081c.c
index 82e524b3db88..e00350e6503f 100644
--- a/drivers/iio/adc/ti-adc081c.c
+++ b/drivers/iio/adc/ti-adc081c.c
@@ -18,7 +18,7 @@ 
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
-#include <linux/of.h>
+#include <linux/mod_devicetable.h>
 #include <linux/acpi.h>
 
 #include <linux/iio/iio.h>
@@ -230,7 +230,6 @@  static const struct i2c_device_id adc081c_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, adc081c_id);
 
-#ifdef CONFIG_OF
 static const struct of_device_id adc081c_of_match[] = {
 	{ .compatible = "ti,adc081c" },
 	{ .compatible = "ti,adc101c" },
@@ -238,7 +237,6 @@  static const struct of_device_id adc081c_of_match[] = {
 	{ }
 };
 MODULE_DEVICE_TABLE(of, adc081c_of_match);
-#endif
 
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id adc081c_acpi_match[] = {
@@ -253,7 +251,7 @@  MODULE_DEVICE_TABLE(acpi, adc081c_acpi_match);
 static struct i2c_driver adc081c_driver = {
 	.driver = {
 		.name = "adc081c",
-		.of_match_table = of_match_ptr(adc081c_of_match),
+		.of_match_table = adc081c_of_match,
 		.acpi_match_table = ACPI_PTR(adc081c_acpi_match),
 	},
 	.probe = adc081c_probe,