* [PATCH] spi: AD accelerometer: declare missing of table @ 2019-04-22 19:05 Daniel Gomez 2019-04-23 7:58 ` Alexandru Ardelean 0 siblings, 1 reply; 5+ messages in thread From: Daniel Gomez @ 2019-04-22 19:05 UTC (permalink / raw) To: jic23 Cc: stefan.popa, lars, Michael.Hennerich, knaack.h, pmeerw, linux-iio, dagmcr, javier 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] spi: AD accelerometer: declare missing of table 2019-04-22 19:05 [PATCH] spi: AD accelerometer: declare missing of table Daniel Gomez @ 2019-04-23 7:58 ` Alexandru Ardelean 2019-04-23 19:55 ` Daniel G 0 siblings, 1 reply; 5+ messages in thread From: Alexandru Ardelean @ 2019-04-23 7:58 UTC (permalink / raw) To: Daniel Gomez Cc: Jonathan Cameron, stefan.popa, Lars-Peter Clausen, Hennerich, Michael, knaack.h, Peter Meerwald-Stadler, linux-iio, javier 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] spi: AD accelerometer: declare missing of table 2019-04-23 7:58 ` Alexandru Ardelean @ 2019-04-23 19:55 ` Daniel G 2019-04-24 6:39 ` Alexandru Ardelean 0 siblings, 1 reply; 5+ messages in thread From: Daniel G @ 2019-04-23 19:55 UTC (permalink / raw) To: Alexandru Ardelean Cc: Jonathan Cameron, stefan.popa, Lars-Peter Clausen, Hennerich, Michael, knaack.h, Peter Meerwald-Stadler, linux-iio, Javier Martinez Canillas 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 > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] spi: AD accelerometer: declare missing of table 2019-04-23 19:55 ` Daniel G @ 2019-04-24 6:39 ` Alexandru Ardelean 2019-04-24 12:51 ` Jonathan Cameron 0 siblings, 1 reply; 5+ messages in thread From: Alexandru Ardelean @ 2019-04-24 6:39 UTC (permalink / raw) To: Daniel G Cc: Jonathan Cameron, stefan.popa, Lars-Peter Clausen, Hennerich, Michael, knaack.h, Peter Meerwald-Stadler, linux-iio, Javier Martinez Canillas 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 > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] spi: AD accelerometer: declare missing of table 2019-04-24 6:39 ` Alexandru Ardelean @ 2019-04-24 12:51 ` Jonathan Cameron 0 siblings, 0 replies; 5+ messages in thread From: Jonathan Cameron @ 2019-04-24 12:51 UTC (permalink / raw) To: Alexandru Ardelean Cc: Daniel G, Jonathan Cameron, stefan.popa, Lars-Peter Clausen, Hennerich, Michael, knaack.h, Peter Meerwald-Stadler, linux-iio, Javier Martinez Canillas 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 > > > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-04-24 12:51 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-04-22 19:05 [PATCH] spi: AD accelerometer: declare missing of table Daniel Gomez 2019-04-23 7:58 ` Alexandru Ardelean 2019-04-23 19:55 ` Daniel G 2019-04-24 6:39 ` Alexandru Ardelean 2019-04-24 12:51 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox