[PATCH] spi: AD accelerometer: declare missing of table

[PATCH] spi: AD accelerometer: declare missing of table

[Daniel Gomez]

Add missing <of_device_id> table for SPI driver relying on SPI
device match since compatible is in a DT binding or in a DTS.

Before this patch:
modinfo drivers/iio/accel/adxl372_spi.ko | grep alias

After this patch:
modinfo drivers/iio/accel/adxl372_spi.ko | grep alias
alias:          spi:adxl372
alias:          of:N*T*Cadi,adxl372C*
alias:          of:N*T*Cadi,adxl372

Reported-by: Javier Martinez Canillas <javier@dowhile0.org>
Signed-off-by: Daniel Gomez <dagmcr@gmail.com>
---
 drivers/iio/accel/adxl372_spi.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/iio/accel/adxl372_spi.c b/drivers/iio/accel/adxl372_spi.c
index e14e655..af0624b 100644
--- a/drivers/iio/accel/adxl372_spi.c
+++ b/drivers/iio/accel/adxl372_spi.c
@@ -7,6 +7,8 @@
 
 #include <linux/module.h>
 #include <linux/regmap.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/spi/spi.h>
 
 #include "adxl372.h"
@@ -37,9 +39,16 @@ static const struct spi_device_id adxl372_spi_id[] = {
 };
 MODULE_DEVICE_TABLE(spi, adxl372_spi_id);
 
+static const struct of_device_id adxl372_spi_of_match[] = {
+        { .compatible = "adi,adxl372" },
+        { },
+};
+MODULE_DEVICE_TABLE(of, adxl372_spi_of_match);
+
 static struct spi_driver adxl372_spi_driver = {
 	.driver = {
 		.name = "adxl372_spi",
+		.of_match_table = of_match_ptr(adxl372_spi_of_match),
 	},
 	.probe = adxl372_spi_probe,
 	.id_table = adxl372_spi_id,
-- 
2.7.4

Re: [PATCH] spi: AD accelerometer: declare missing of table

[Alexandru Ardelean]

On Mon, Apr 22, 2019 at 10:20 PM Daniel Gomez <dagmcr@gmail.com> wrote:
>
> Add missing <of_device_id> table for SPI driver relying on SPI
> device match since compatible is in a DT binding or in a DTS.
>

Hey,

The driver is under iio, so commit title should be something like:

iio: accelerometer: adxl372: declare missing of table
or
iio: adxl372: declare missing of table

> Before this patch:
> modinfo drivers/iio/accel/adxl372_spi.ko | grep alias
>
> After this patch:
> modinfo drivers/iio/accel/adxl372_spi.ko | grep alias
> alias:          spi:adxl372
> alias:          of:N*T*Cadi,adxl372C*
> alias:          of:N*T*Cadi,adxl372
>
> Reported-by: Javier Martinez Canillas <javier@dowhile0.org>
> Signed-off-by: Daniel Gomez <dagmcr@gmail.com>
> ---
>  drivers/iio/accel/adxl372_spi.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/iio/accel/adxl372_spi.c b/drivers/iio/accel/adxl372_spi.c
> index e14e655..af0624b 100644
> --- a/drivers/iio/accel/adxl372_spi.c
> +++ b/drivers/iio/accel/adxl372_spi.c
> @@ -7,6 +7,8 @@
>
>  #include <linux/module.h>
>  #include <linux/regmap.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/spi/spi.h>
>
>  #include "adxl372.h"
> @@ -37,9 +39,16 @@ static const struct spi_device_id adxl372_spi_id[] = {
>  };
>  MODULE_DEVICE_TABLE(spi, adxl372_spi_id);
>
> +static const struct of_device_id adxl372_spi_of_match[] = {

The `spi` part of the name is un-needed.
So,
adxl372_spi_of_match  ->  adxl372_of_match
or
adxl372_spi_of_match  ->  adxl372_of_table


> +        { .compatible = "adi,adxl372" },
> +        { },
> +};
> +MODULE_DEVICE_TABLE(of, adxl372_spi_of_match);
> +
>  static struct spi_driver adxl372_spi_driver = {
>         .driver = {
>                 .name = "adxl372_spi",
> +               .of_match_table = of_match_ptr(adxl372_spi_of_match),

I think the the `of_match_ptr()` is un-needed. It messes with some ACPI stuff.

So, something like:
    .of_match_table = adxl372_of_table,
should be fine.

Rest looks good

Thanks
Alex

>         },
>         .probe = adxl372_spi_probe,
>         .id_table = adxl372_spi_id,
> --
> 2.7.4
>

Re: [PATCH] spi: AD accelerometer: declare missing of table

[Daniel G]

On Tue, Apr 23, 2019 at 9:58 AM Alexandru Ardelean
<ardeleanalex@gmail.com> wrote:
>
> On Mon, Apr 22, 2019 at 10:20 PM Daniel Gomez <dagmcr@gmail.com> wrote:
> >
> > Add missing <of_device_id> table for SPI driver relying on SPI
> > device match since compatible is in a DT binding or in a DTS.
> >
>
> Hey,
>
> The driver is under iio, so commit title should be something like:
>
> iio: accelerometer: adxl372: declare missing of table
> or
> iio: adxl372: declare missing of table
Okay! It makes sense so, I'll make the changes and send it back as v2.
I have sent other patches fixing the same issue under iio but with other
maintainers so, I will follow the same approach.
>
> > Before this patch:
> > modinfo drivers/iio/accel/adxl372_spi.ko | grep alias
> >
> > After this patch:
> > modinfo drivers/iio/accel/adxl372_spi.ko | grep alias
> > alias:          spi:adxl372
> > alias:          of:N*T*Cadi,adxl372C*
> > alias:          of:N*T*Cadi,adxl372
> >
> > Reported-by: Javier Martinez Canillas <javier@dowhile0.org>
> > Signed-off-by: Daniel Gomez <dagmcr@gmail.com>
> > ---
> >  drivers/iio/accel/adxl372_spi.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/iio/accel/adxl372_spi.c b/drivers/iio/accel/adxl372_spi.c
> > index e14e655..af0624b 100644
> > --- a/drivers/iio/accel/adxl372_spi.c
> > +++ b/drivers/iio/accel/adxl372_spi.c
> > @@ -7,6 +7,8 @@
> >
> >  #include <linux/module.h>
> >  #include <linux/regmap.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> >  #include <linux/spi/spi.h>
> >
> >  #include "adxl372.h"
> > @@ -37,9 +39,16 @@ static const struct spi_device_id adxl372_spi_id[] = {
> >  };
> >  MODULE_DEVICE_TABLE(spi, adxl372_spi_id);
> >
> > +static const struct of_device_id adxl372_spi_of_match[] = {
>
> The `spi` part of the name is un-needed.
> So,
> adxl372_spi_of_match  ->  adxl372_of_match
> or
> adxl372_spi_of_match  ->  adxl372_of_table
>
>
> > +        { .compatible = "adi,adxl372" },
> > +        { },
> > +};
> > +MODULE_DEVICE_TABLE(of, adxl372_spi_of_match);
> > +
> >  static struct spi_driver adxl372_spi_driver = {
> >         .driver = {
> >                 .name = "adxl372_spi",
> > +               .of_match_table = of_match_ptr(adxl372_spi_of_match),
>
> I think the the `of_match_ptr()` is un-needed. It messes with some ACPI stuff.
>
> So, something like:
>     .of_match_table = adxl372_of_table,
> should be fine.
Okay, I'll remove it as well but I thought this macro was needed to check if DT
support is enabled but only if the driver can be compiled as a module.
So, what's the reason for being unneeded in this case?
Thanks in advance!
>
> Rest looks good
>
> Thanks
> Alex
>
> >         },
> >         .probe = adxl372_spi_probe,
> >         .id_table = adxl372_spi_id,
> > --
> > 2.7.4
> >

Re: [PATCH] spi: AD accelerometer: declare missing of table

[Alexandru Ardelean]

On Tue, Apr 23, 2019 at 10:55 PM Daniel G <dagmcr@gmail.com> wrote:
>
> On Tue, Apr 23, 2019 at 9:58 AM Alexandru Ardelean
> <ardeleanalex@gmail.com> wrote:
> >
> > On Mon, Apr 22, 2019 at 10:20 PM Daniel Gomez <dagmcr@gmail.com> wrote:
> > >
> > > Add missing <of_device_id> table for SPI driver relying on SPI
> > > device match since compatible is in a DT binding or in a DTS.
> > >
> >
> > Hey,
> >
> > The driver is under iio, so commit title should be something like:
> >
> > iio: accelerometer: adxl372: declare missing of table
> > or
> > iio: adxl372: declare missing of table
> Okay! It makes sense so, I'll make the changes and send it back as v2.
> I have sent other patches fixing the same issue under iio but with other
> maintainers so, I will follow the same approach.
> >
> > > Before this patch:
> > > modinfo drivers/iio/accel/adxl372_spi.ko | grep alias
> > >
> > > After this patch:
> > > modinfo drivers/iio/accel/adxl372_spi.ko | grep alias
> > > alias:          spi:adxl372
> > > alias:          of:N*T*Cadi,adxl372C*
> > > alias:          of:N*T*Cadi,adxl372
> > >
> > > Reported-by: Javier Martinez Canillas <javier@dowhile0.org>
> > > Signed-off-by: Daniel Gomez <dagmcr@gmail.com>
> > > ---
> > >  drivers/iio/accel/adxl372_spi.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/drivers/iio/accel/adxl372_spi.c b/drivers/iio/accel/adxl372_spi.c
> > > index e14e655..af0624b 100644
> > > --- a/drivers/iio/accel/adxl372_spi.c
> > > +++ b/drivers/iio/accel/adxl372_spi.c
> > > @@ -7,6 +7,8 @@
> > >
> > >  #include <linux/module.h>
> > >  #include <linux/regmap.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_device.h>
> > >  #include <linux/spi/spi.h>
> > >
> > >  #include "adxl372.h"
> > > @@ -37,9 +39,16 @@ static const struct spi_device_id adxl372_spi_id[] = {
> > >  };
> > >  MODULE_DEVICE_TABLE(spi, adxl372_spi_id);
> > >
> > > +static const struct of_device_id adxl372_spi_of_match[] = {
> >
> > The `spi` part of the name is un-needed.
> > So,
> > adxl372_spi_of_match  ->  adxl372_of_match
> > or
> > adxl372_spi_of_match  ->  adxl372_of_table
> >
> >
> > > +        { .compatible = "adi,adxl372" },
> > > +        { },
> > > +};
> > > +MODULE_DEVICE_TABLE(of, adxl372_spi_of_match);
> > > +
> > >  static struct spi_driver adxl372_spi_driver = {
> > >         .driver = {
> > >                 .name = "adxl372_spi",
> > > +               .of_match_table = of_match_ptr(adxl372_spi_of_match),
> >
> > I think the the `of_match_ptr()` is un-needed. It messes with some ACPI stuff.
> >
> > So, something like:
> >     .of_match_table = adxl372_of_table,
> > should be fine.
> Okay, I'll remove it as well but I thought this macro was needed to check if DT
> support is enabled but only if the driver can be compiled as a module.
> So, what's the reason for being unneeded in this case?

There are 2 main mechanisms for configuring a device/board/computer [in Linux].
One is ACPI (mostly driven by Intel & Microsoft) and the other one is
DT (mostly driven by ARM).
Ideally, a driver should work with both.

I'm a bit vague on how ACPI uses this table/information.
I did look through the ACPI code (drivers/acpi) but a lot of the fine
details are unclear to me.
There seems to be some code in there that can parse of_match_table.

In any case, I do remember however that this is some older comment
from other reviews.
i.e. (if possible) do not use of_match_ptr(), because ACPI won't work

btw, a fun read about ACPI on ARM is:
http://www.secretlab.ca/archives/151

Thanks
Alex

> Thanks in advance!
> >
> > Rest looks good
> >
> > Thanks
> > Alex
> >
> > >         },
> > >         .probe = adxl372_spi_probe,
> > >         .id_table = adxl372_spi_id,
> > > --
> > > 2.7.4
> > >

Re: [PATCH] spi: AD accelerometer: declare missing of table

[Jonathan Cameron]

On Wed, 24 Apr 2019 09:39:20 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Tue, Apr 23, 2019 at 10:55 PM Daniel G <dagmcr@gmail.com> wrote:
> >
> > On Tue, Apr 23, 2019 at 9:58 AM Alexandru Ardelean
> > <ardeleanalex@gmail.com> wrote:  
> > >
> > > On Mon, Apr 22, 2019 at 10:20 PM Daniel Gomez <dagmcr@gmail.com> wrote:  
> > > >
> > > > Add missing <of_device_id> table for SPI driver relying on SPI
> > > > device match since compatible is in a DT binding or in a DTS.
> > > >  
> > >
> > > Hey,
> > >
> > > The driver is under iio, so commit title should be something like:
> > >
> > > iio: accelerometer: adxl372: declare missing of table
> > > or
> > > iio: adxl372: declare missing of table  
> > Okay! It makes sense so, I'll make the changes and send it back as v2.
> > I have sent other patches fixing the same issue under iio but with other
> > maintainers so, I will follow the same approach.  
> > >  
> > > > Before this patch:
> > > > modinfo drivers/iio/accel/adxl372_spi.ko | grep alias
> > > >
> > > > After this patch:
> > > > modinfo drivers/iio/accel/adxl372_spi.ko | grep alias
> > > > alias:          spi:adxl372
> > > > alias:          of:N*T*Cadi,adxl372C*
> > > > alias:          of:N*T*Cadi,adxl372
> > > >
> > > > Reported-by: Javier Martinez Canillas <javier@dowhile0.org>
> > > > Signed-off-by: Daniel Gomez <dagmcr@gmail.com>
> > > > ---
> > > >  drivers/iio/accel/adxl372_spi.c | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/drivers/iio/accel/adxl372_spi.c b/drivers/iio/accel/adxl372_spi.c
> > > > index e14e655..af0624b 100644
> > > > --- a/drivers/iio/accel/adxl372_spi.c
> > > > +++ b/drivers/iio/accel/adxl372_spi.c
> > > > @@ -7,6 +7,8 @@
> > > >
> > > >  #include <linux/module.h>
> > > >  #include <linux/regmap.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/of_device.h>
> > > >  #include <linux/spi/spi.h>
> > > >
> > > >  #include "adxl372.h"
> > > > @@ -37,9 +39,16 @@ static const struct spi_device_id adxl372_spi_id[] = {
> > > >  };
> > > >  MODULE_DEVICE_TABLE(spi, adxl372_spi_id);
> > > >
> > > > +static const struct of_device_id adxl372_spi_of_match[] = {  
> > >
> > > The `spi` part of the name is un-needed.
> > > So,
> > > adxl372_spi_of_match  ->  adxl372_of_match
> > > or
> > > adxl372_spi_of_match  ->  adxl372_of_table
> > >
> > >  
> > > > +        { .compatible = "adi,adxl372" },
> > > > +        { },
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, adxl372_spi_of_match);
> > > > +
> > > >  static struct spi_driver adxl372_spi_driver = {
> > > >         .driver = {
> > > >                 .name = "adxl372_spi",
> > > > +               .of_match_table = of_match_ptr(adxl372_spi_of_match),  
> > >
> > > I think the the `of_match_ptr()` is un-needed. It messes with some ACPI stuff.
> > >
> > > So, something like:
> > >     .of_match_table = adxl372_of_table,
> > > should be fine.  
> > Okay, I'll remove it as well but I thought this macro was needed to check if DT
> > support is enabled but only if the driver can be compiled as a module.
> > So, what's the reason for being unneeded in this case?  
> 
> There are 2 main mechanisms for configuring a device/board/computer [in Linux].
> One is ACPI (mostly driven by Intel & Microsoft) and the other one is
> DT (mostly driven by ARM).
A bit more here for reference.

DT actually came from Power originally I believe.

ACPI is very heavily driven by ARM and ARM licensees as well as the others
these days. I spend far too much time in work groups around that to not
comment ;)

It is just normally used on more 'standard' oriented platforms
such as laptops, desktops and servers.

> Ideally, a driver should work with both.
> 
> I'm a bit vague on how ACPI uses this table/information.
> I did look through the ACPI code (drivers/acpi) but a lot of the fine
> details are unclear to me.
> There seems to be some code in there that can parse of_match_table.

There is indeed.  PRP0001 which is wonderfully a vendor namespace allocated
to the UEFI forum (who 'own' ACPI these days). 
It basically lets you put DT elements in an ACPI DSDT table.
 
> 
> In any case, I do remember however that this is some older comment
> from other reviews.
> i.e. (if possible) do not use of_match_ptr(), because ACPI won't work
> 
> btw, a fun read about ACPI on ARM is:
> http://www.secretlab.ca/archives/151
> 
> Thanks
> Alex
> 
> > Thanks in advance!  
> > >
> > > Rest looks good
> > >
> > > Thanks
> > > Alex
> > >  
> > > >         },
> > > >         .probe = adxl372_spi_probe,
> > > >         .id_table = adxl372_spi_id,
> > > > --
> > > > 2.7.4
> > > >