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

[Jonathan Cameron <jonathan.cameron@huawei.com>]

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
> > > >