diff mbox series

[20/24] staging:iio:cdc:ad7150: Add of_match_table

Message ID 20210207154623.433442-21-jic23@kernel.org (mailing list archive)
State New, archived
Headers show
Series staging:iio:cdc:ad7150: cleanup / fixup / graduate | expand

Commit Message

Jonathan Cameron Feb. 7, 2021, 3:46 p.m. UTC 
From: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Rather than using the fallback path in the i2c subsystem and hoping
for no clashes across vendors, lets put in an explicit table for
matching.

Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
---
 drivers/staging/iio/cdc/ad7150.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Alexandru Ardelean Feb. 8, 2021, 7:11 a.m. UTC | #1 
On Sun, Feb 7, 2021 at 5:52 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Rather than using the fallback path in the i2c subsystem and hoping
> for no clashes across vendors, lets put in an explicit table for
> matching.
>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/staging/iio/cdc/ad7150.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> index 0bc8c7a99883..33c8a78c076f 100644
> --- a/drivers/staging/iio/cdc/ad7150.c
> +++ b/drivers/staging/iio/cdc/ad7150.c
> @@ -12,6 +12,7 @@
>  #include <linux/i2c.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>
> @@ -655,9 +656,16 @@ static const struct i2c_device_id ad7150_id[] = {
>
>  MODULE_DEVICE_TABLE(i2c, ad7150_id);
>
> +static const struct of_device_id ad7150_of_match[] = {
> +       { "adi,ad7150" },
> +       { "adi,ad7151" },
> +       { "adi,ad7156" },

Is this missing some match_driver_data logic?
Something like this:
https://patchwork.kernel.org/project/linux-iio/patch/20210207154623.433442-7-jic23@kernel.org/
?

> +       {}
> +};
>  static struct i2c_driver ad7150_driver = {
>         .driver = {
>                 .name = "ad7150",
> +               .of_match_table = ad7150_of_match,
>         },
>         .probe = ad7150_probe,
>         .id_table = ad7150_id,
> --
> 2.30.0
>
Song Bao Hua (Barry Song) Feb. 8, 2021, 7:50 a.m. UTC | #2 
> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@kernel.org]
> Sent: Monday, February 8, 2021 4:46 AM
> To: linux-iio@vger.kernel.org
> Cc: Lars-Peter Clausen <lars@metafoo.de>; Michael Hennerich
> <Michael.Hennerich@analog.com>; Song Bao Hua (Barry Song)
> <song.bao.hua@hisilicon.com>; robh+dt@kernel.org; Jonathan Cameron
> <jonathan.cameron@huawei.com>
> Subject: [PATCH 20/24] staging:iio:cdc:ad7150: Add of_match_table
> 
> From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> Rather than using the fallback path in the i2c subsystem and hoping
> for no clashes across vendors, lets put in an explicit table for
> matching.
> 
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  drivers/staging/iio/cdc/ad7150.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/staging/iio/cdc/ad7150.c
> b/drivers/staging/iio/cdc/ad7150.c
> index 0bc8c7a99883..33c8a78c076f 100644
> --- a/drivers/staging/iio/cdc/ad7150.c
> +++ b/drivers/staging/iio/cdc/ad7150.c
> @@ -12,6 +12,7 @@
>  #include <linux/i2c.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/mod_devicetable.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
> 
> @@ -655,9 +656,16 @@ static const struct i2c_device_id ad7150_id[] = {
> 
>  MODULE_DEVICE_TABLE(i2c, ad7150_id);
> 
> +static const struct of_device_id ad7150_of_match[] = {
> +	{ "adi,ad7150" },
> +	{ "adi,ad7151" },
> +	{ "adi,ad7156" },
> +	{}
> +};

Does it compile if CONFIG_OF is not enabled?

>  static struct i2c_driver ad7150_driver = {
>  	.driver = {
>  		.name = "ad7150",
> +		.of_match_table = ad7150_of_match,

of_match_ptr(ad7150_of_match)?

Do we need dt-binding doc?


>  	},
>  	.probe = ad7150_probe,
>  	.id_table = ad7150_id,
> --
> 2.30.0

Thanks
Barry
Alexandru Ardelean Feb. 8, 2021, 8:01 a.m. UTC | #3 
On Mon, Feb 8, 2021 at 9:52 AM Song Bao Hua (Barry Song)
<song.bao.hua@hisilicon.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Jonathan Cameron [mailto:jic23@kernel.org]
> > Sent: Monday, February 8, 2021 4:46 AM
> > To: linux-iio@vger.kernel.org
> > Cc: Lars-Peter Clausen <lars@metafoo.de>; Michael Hennerich
> > <Michael.Hennerich@analog.com>; Song Bao Hua (Barry Song)
> > <song.bao.hua@hisilicon.com>; robh+dt@kernel.org; Jonathan Cameron
> > <jonathan.cameron@huawei.com>
> > Subject: [PATCH 20/24] staging:iio:cdc:ad7150: Add of_match_table
> >
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > Rather than using the fallback path in the i2c subsystem and hoping
> > for no clashes across vendors, lets put in an explicit table for
> > matching.
> >
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  drivers/staging/iio/cdc/ad7150.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/staging/iio/cdc/ad7150.c
> > b/drivers/staging/iio/cdc/ad7150.c
> > index 0bc8c7a99883..33c8a78c076f 100644
> > --- a/drivers/staging/iio/cdc/ad7150.c
> > +++ b/drivers/staging/iio/cdc/ad7150.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/i2c.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/slab.h>
> >
> > @@ -655,9 +656,16 @@ static const struct i2c_device_id ad7150_id[] = {
> >
> >  MODULE_DEVICE_TABLE(i2c, ad7150_id);
> >
> > +static const struct of_device_id ad7150_of_match[] = {
> > +     { "adi,ad7150" },
> > +     { "adi,ad7151" },
> > +     { "adi,ad7156" },
> > +     {}
> > +};
>
> Does it compile if CONFIG_OF is not enabled?
>
> >  static struct i2c_driver ad7150_driver = {
> >       .driver = {
> >               .name = "ad7150",
> > +             .of_match_table = ad7150_of_match,
>
> of_match_ptr(ad7150_of_match)?

of_match_ptr() is not recommended anymore because of the ACPI PRP0001
thing/compat with OF;

>
> Do we need dt-binding doc?

Should be here:
https://lore.kernel.org/linux-iio/20210207161820.28abeb33@archlinux/T/#u

>
>
> >       },
> >       .probe = ad7150_probe,
> >       .id_table = ad7150_id,
> > --
> > 2.30.0
>
> Thanks
> Barry
>
Jonathan Cameron Feb. 11, 2021, 3:51 p.m. UTC | #4 
On Mon, 8 Feb 2021 09:11:21 +0200
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Sun, Feb 7, 2021 at 5:52 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > From: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > Rather than using the fallback path in the i2c subsystem and hoping
> > for no clashes across vendors, lets put in an explicit table for
> > matching.
> >
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > ---
> >  drivers/staging/iio/cdc/ad7150.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> > index 0bc8c7a99883..33c8a78c076f 100644
> > --- a/drivers/staging/iio/cdc/ad7150.c
> > +++ b/drivers/staging/iio/cdc/ad7150.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/i2c.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> >  #include <linux/regulator/consumer.h>
> >  #include <linux/slab.h>
> >
> > @@ -655,9 +656,16 @@ static const struct i2c_device_id ad7150_id[] = {
> >
> >  MODULE_DEVICE_TABLE(i2c, ad7150_id);
> >
> > +static const struct of_device_id ad7150_of_match[] = {
> > +       { "adi,ad7150" },
> > +       { "adi,ad7151" },
> > +       { "adi,ad7156" },  
> 
> Is this missing some match_driver_data logic?

Odd though it may seem, we can still use id->driver_data
even though we matched against these ids from the point of view
of probe. 

https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-core-base.c#L528
In particular the call of i2c_match_id(driver->id_table, client)
which is passed (buried in client) via of_modalias_node() the
of_device_id string with vendor prefix stripped. 

We could do firmware specific match data logic, but we'd then need
to fallback to the i2c id->driver_data anyway if it failed.

So as things currently stand I think it is better to avoid duplication
of the data and just keep it in the one table.

The disadvantage is that we have to keep the two ID tables in sync.

Jonathan


> Something like this:
> https://patchwork.kernel.org/project/linux-iio/patch/20210207154623.433442-7-jic23@kernel.org/
> ?
> 
> > +       {}
> > +};
> >  static struct i2c_driver ad7150_driver = {
> >         .driver = {
> >                 .name = "ad7150",
> > +               .of_match_table = ad7150_of_match,
> >         },
> >         .probe = ad7150_probe,
> >         .id_table = ad7150_id,
> > --
> > 2.30.0
> >
diff mbox series

Patch

diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 0bc8c7a99883..33c8a78c076f 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -12,6 +12,7 @@ 
 #include <linux/i2c.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/mod_devicetable.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 
@@ -655,9 +656,16 @@  static const struct i2c_device_id ad7150_id[] = {
 
 MODULE_DEVICE_TABLE(i2c, ad7150_id);
 
+static const struct of_device_id ad7150_of_match[] = {
+	{ "adi,ad7150" },
+	{ "adi,ad7151" },
+	{ "adi,ad7156" },
+	{}
+};
 static struct i2c_driver ad7150_driver = {
 	.driver = {
 		.name = "ad7150",
+		.of_match_table = ad7150_of_match,
 	},
 	.probe = ad7150_probe,
 	.id_table = ad7150_id,