On Tue, Apr 23, 2019 at 10:55 PM Daniel G <dagmcr@xxxxxxxxx> wrote:
>
> On Tue, Apr 23, 2019 at 9:58 AM Alexandru Ardelean
> <ardeleanalex@xxxxxxxxx> wrote:
> >
> > On Mon, Apr 22, 2019 at 10:20 PM Daniel Gomez <dagmcr@xxxxxxxxx> 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@xxxxxxxxxxxx>
> > > Signed-off-by: Daniel Gomez <dagmcr@xxxxxxxxx>
> > > ---
> > > 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
> > >
