| Message ID | 20180918224525.28001-3-tpiepho@impinj.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | device tree spidev solution - driver_override for SPI | expand |
On Wed, Sep 19, 2018 at 12:50 AM Trent Piepho <tpiepho@impinj.com> wrote: > spidev will make a big fuss if a device tree node binds a device by > using "spidev" as the node's compatible property. > > However, the logic for this isn't looking for "spidev" in the > compatible, but rather checking that the device is NOT compatible with > spidev's list of devices. > > This causes a false positive if a device not named "rohm,dh2228fv", etc. > binds to spidev, even if a means other than putting "spidev" in the > device tree was used. E.g., the sysfs driver_override attribute. > > Signed-off-by: Trent Piepho <tpiepho@impinj.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- a/drivers/spi/spidev.c > +++ b/drivers/spi/spidev.c > @@ -724,10 +724,11 @@ static int spidev_probe(struct spi_device *spi) > * compatible string, it is a Linux implementation thing > * rather than a description of the hardware. > */ > - if (spi->dev.of_node && !of_match_device(spidev_dt_ids, &spi->dev)) { > + if (spi->dev.of_node && > + of_device_is_compatible(spi->dev.of_node, "spidev")) { > dev_err(&spi->dev, "buggy DT: spidev listed directly in DT\n"); > WARN_ON(spi->dev.of_node && > - !of_match_device(spidev_dt_ids, &spi->dev)); > + of_device_is_compatible(spi->dev.of_node, "spidev")); To avoid having the same conditional twice, perhaps the dev_err() and WARN_ON() should just be replaced by WARN(1, "%pOF: buggy DT: spidev listed directly in DT\n", spi->dev.of_node); > } Gr{oetje,eeting}s, Geert
On Wed, Sep 19, 2018 at 08:23:09AM +0200, Geert Uytterhoeven wrote: > On Wed, Sep 19, 2018 at 12:50 AM Trent Piepho <tpiepho@impinj.com> wrote: > > + if (spi->dev.of_node && > > + of_device_is_compatible(spi->dev.of_node, "spidev")) { > > dev_err(&spi->dev, "buggy DT: spidev listed directly in DT\n"); > > WARN_ON(spi->dev.of_node && > > - !of_match_device(spidev_dt_ids, &spi->dev)); > > + of_device_is_compatible(spi->dev.of_node, "spidev")); > To avoid having the same conditional twice, perhaps the dev_err() and WARN_ON() > should just be replaced by > WARN(1, "%pOF: buggy DT: spidev listed directly in DT\n", > spi->dev.of_node); Yes, that'd be neater.
On Wed, 2018-09-19 at 10:39 -0700, Mark Brown wrote: > On Wed, Sep 19, 2018 at 08:23:09AM +0200, Geert Uytterhoeven wrote: > > On Wed, Sep 19, 2018 at 12:50 AM Trent Piepho <tpiepho@impinj.com> wrote: > > > + if (spi->dev.of_node && > > > + of_device_is_compatible(spi->dev.of_node, "spidev")) { > > > dev_err(&spi->dev, "buggy DT: spidev listed directly in DT\n"); > > > WARN_ON(spi->dev.of_node && > > > - !of_match_device(spidev_dt_ids, &spi->dev)); > > > + of_device_is_compatible(spi->dev.of_node, "spidev")); > > To avoid having the same conditional twice, perhaps the dev_err() and WARN_ON() > > should just be replaced by > > WARN(1, "%pOF: buggy DT: spidev listed directly in DT\n", > > spi->dev.of_node); > > Yes, that'd be neater. How about: - if (spi->dev.of_node && !of_match_device(spidev_dt_ids, &spi->dev)) { - dev_err(&spi->dev, "buggy DT: spidev listed directly in DT\n"); - WARN_ON(spi->dev.of_node && - !of_match_device(spidev_dt_ids, &spi->dev)); - } + WARN_ON(spi->dev.of_node && + of_device_is_compatible(spi->dev.of_node, "spidev"), + "%pOF: buggy DT: spidev listed directly in DT\n", spi->dev.of_node); x
Hi Trent, On Wed, Sep 19, 2018 at 10:54 PM Trent Piepho <tpiepho@impinj.com> wrote: > On Wed, 2018-09-19 at 10:39 -0700, Mark Brown wrote: > > On Wed, Sep 19, 2018 at 08:23:09AM +0200, Geert Uytterhoeven wrote: > > > On Wed, Sep 19, 2018 at 12:50 AM Trent Piepho <tpiepho@impinj.com> wrote: > > > > + if (spi->dev.of_node && > > > > + of_device_is_compatible(spi->dev.of_node, "spidev")) { > > > > dev_err(&spi->dev, "buggy DT: spidev listed directly in DT\n"); > > > > WARN_ON(spi->dev.of_node && > > > > - !of_match_device(spidev_dt_ids, &spi->dev)); > > > > + of_device_is_compatible(spi->dev.of_node, "spidev")); > > > To avoid having the same conditional twice, perhaps the dev_err() and WARN_ON() > > > should just be replaced by > > > WARN(1, "%pOF: buggy DT: spidev listed directly in DT\n", > > > spi->dev.of_node); > > > > Yes, that'd be neater. > > How about: > > - if (spi->dev.of_node && !of_match_device(spidev_dt_ids, &spi->dev)) { > - dev_err(&spi->dev, "buggy DT: spidev listed directly in DT\n"); > - WARN_ON(spi->dev.of_node && > - !of_match_device(spidev_dt_ids, &spi->dev)); > - } > + WARN_ON(spi->dev.of_node && > + of_device_is_compatible(spi->dev.of_node, "spidev"), > + "%pOF: buggy DT: spidev listed directly in DT\n", spi->dev.of_node); s/WARN_ON/WARN/? The former doesn't take a message, the latter does. Gr{oetje,eeting}s, Geert
On Wed, 2018-09-19 at 23:17 +0200, Geert Uytterhoeven wrote: > Hi Trent, > > On Wed, Sep 19, 2018 at 10:54 PM Trent Piepho <tpiepho@impinj.com> wrote: > > On Wed, 2018-09-19 at 10:39 -0700, Mark Brown wrote: > > > On Wed, Sep 19, 2018 at 08:23:09AM +0200, Geert Uytterhoeven wrote: > > > > On Wed, Sep 19, 2018 at 12:50 AM Trent Piepho <tpiepho@impinj.com> wrote: > > > > > + if (spi->dev.of_node && > > > > > + of_device_is_compatible(spi->dev.of_node, "spidev")) { > > > > > dev_err(&spi->dev, "buggy DT: spidev listed directly in DT\n"); > > > > > WARN_ON(spi->dev.of_node && > > > > > - !of_match_device(spidev_dt_ids, &spi->dev)); > > > > > + of_device_is_compatible(spi->dev.of_node, "spidev")); > > > > > > > > To avoid having the same conditional twice, perhaps the dev_err() and WARN_ON() > > > > should just be replaced by > > > > WARN(1, "%pOF: buggy DT: spidev listed directly in DT\n", > > > > spi->dev.of_node); > > > > > > Yes, that'd be neater. > > > > How about: > > > > - if (spi->dev.of_node && !of_match_device(spidev_dt_ids, &spi->dev)) { > > - dev_err(&spi->dev, "buggy DT: spidev listed directly in DT\n"); > > - WARN_ON(spi->dev.of_node && > > - !of_match_device(spidev_dt_ids, &spi->dev)); > > - } > > + WARN_ON(spi->dev.of_node && > > + of_device_is_compatible(spi->dev.of_node, "spidev"), > > + "%pOF: buggy DT: spidev listed directly in DT\n", spi->dev.of_node); > > s/WARN_ON/WARN/? > > The former doesn't take a message, the latter does. Yes, that's what I meant. If the dev_err is to be removed, no reason to use WARN(1, ) instead of putting the condition in the WARN.
Hi Trent, On Wed, Sep 19, 2018 at 11:34 PM Trent Piepho <tpiepho@impinj.com> wrote: > On Wed, 2018-09-19 at 23:17 +0200, Geert Uytterhoeven wrote: > > On Wed, Sep 19, 2018 at 10:54 PM Trent Piepho <tpiepho@impinj.com> wrote: > > > On Wed, 2018-09-19 at 10:39 -0700, Mark Brown wrote: > > > > On Wed, Sep 19, 2018 at 08:23:09AM +0200, Geert Uytterhoeven wrote: > > > > > On Wed, Sep 19, 2018 at 12:50 AM Trent Piepho <tpiepho@impinj.com> wrote: > > > > > > + if (spi->dev.of_node && > > > > > > + of_device_is_compatible(spi->dev.of_node, "spidev")) { > > > > > > dev_err(&spi->dev, "buggy DT: spidev listed directly in DT\n"); > > > > > > WARN_ON(spi->dev.of_node && > > > > > > - !of_match_device(spidev_dt_ids, &spi->dev)); > > > > > > + of_device_is_compatible(spi->dev.of_node, "spidev")); > > > > > > > > > > To avoid having the same conditional twice, perhaps the dev_err() and WARN_ON() > > > > > should just be replaced by > > > > > WARN(1, "%pOF: buggy DT: spidev listed directly in DT\n", > > > > > spi->dev.of_node); > > > > > > > > Yes, that'd be neater. > > > > > > How about: > > > > > > - if (spi->dev.of_node && !of_match_device(spidev_dt_ids, &spi->dev)) { > > > - dev_err(&spi->dev, "buggy DT: spidev listed directly in DT\n"); > > > - WARN_ON(spi->dev.of_node && > > > - !of_match_device(spidev_dt_ids, &spi->dev)); > > > - } > > > + WARN_ON(spi->dev.of_node && > > > + of_device_is_compatible(spi->dev.of_node, "spidev"), > > > + "%pOF: buggy DT: spidev listed directly in DT\n", spi->dev.of_node); > > > > s/WARN_ON/WARN/? > > > > The former doesn't take a message, the latter does. > > Yes, that's what I meant. If the dev_err is to be removed, no reason > to use WARN(1, ) instead of putting the condition in the WARN. Thanks, looks fine to me! Gr{oetje,eeting}s, Geert
On Thu, Sep 20, 2018 at 08:31:35AM +0200, Geert Uytterhoeven wrote: > On Wed, Sep 19, 2018 at 11:34 PM Trent Piepho <tpiepho@impinj.com> wrote: > > > The former doesn't take a message, the latter does. > > Yes, that's what I meant. If the dev_err is to be removed, no reason > > to use WARN(1, ) instead of putting the condition in the WARN. > Thanks, looks fine to me! Me too.
diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c index cda10719d1d1..eee976f8c399 100644 --- a/drivers/spi/spidev.c +++ b/drivers/spi/spidev.c @@ -724,10 +724,11 @@ static int spidev_probe(struct spi_device *spi) * compatible string, it is a Linux implementation thing * rather than a description of the hardware. */ - if (spi->dev.of_node && !of_match_device(spidev_dt_ids, &spi->dev)) { + if (spi->dev.of_node && + of_device_is_compatible(spi->dev.of_node, "spidev")) { dev_err(&spi->dev, "buggy DT: spidev listed directly in DT\n"); WARN_ON(spi->dev.of_node && - !of_match_device(spidev_dt_ids, &spi->dev)); + of_device_is_compatible(spi->dev.of_node, "spidev")); } spidev_probe_acpi(spi);
spidev will make a big fuss if a device tree node binds a device by using "spidev" as the node's compatible property. However, the logic for this isn't looking for "spidev" in the compatible, but rather checking that the device is NOT compatible with spidev's list of devices. This causes a false positive if a device not named "rohm,dh2228fv", etc. binds to spidev, even if a means other than putting "spidev" in the device tree was used. E.g., the sysfs driver_override attribute. Signed-off-by: Trent Piepho <tpiepho@impinj.com> --- drivers/spi/spidev.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)