| Message ID | 20180918224525.28001-2-tpiepho@impinj.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | device tree spidev solution - driver_override for SPI | expand |
Hi Trent, Thanks for your patch! On Wed, Sep 19, 2018 at 12:50 AM Trent Piepho <tpiepho@impinj.com> wrote: > This attribute works the same was as the identically named attribute > for PCI, AMBA, and platform devices. For reference, see: Note that it doesn't work exactly the same, due to the device_attach() (see below). > commit 3cf385713460 ("ARM: 8256/1: driver coamba: add device binding > path 'driver_override'") > commit 3d713e0e382e ("driver core: platform: add device binding path > 'driver_override'") > commit 782a985d7af2 ("PCI: Introduce new device binding path using > pci_dev.driver_override") > > If the name of a driver is written to this attribute, then the device > will bind to the named driver and only the named driver. > > The device will bind to the driver even if the driver does not list the > device in its id table. This behavior is different than the driver's > bind attribute, which only allows binding to devices that are listed as > supported by the driver. > > It can be used to bind a generic driver, like spidev, to a device. > > Signed-off-by: Trent Piepho <tpiepho@impinj.com> > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -77,6 +77,54 @@ modalias_show(struct device *dev, struct device_attribute *a, char *buf) > } > static DEVICE_ATTR_RO(modalias); > > +static ssize_t driver_override_store(struct device *dev, > + struct device_attribute *a, > + const char *buf, size_t count) > +{ > + struct spi_device *spi = to_spi_device(dev); > + const char *end = memchr(buf, '\n', count); > + const size_t len = end ? end - buf : count; > + const char *driver_override, *old; > + int ret; > + > + /* We need to keep extra room for a newline when displaying value */ > + if (len >= (PAGE_SIZE - 1)) > + return -EINVAL; > + > + driver_override = kstrndup(buf, len, GFP_KERNEL); > + if (!driver_override) > + return -ENOMEM; > + > + device_lock(dev); > + old = spi->driver_override; > + if (len) { > + spi->driver_override = driver_override; > + } else { > + /* Emptry string, disable driver override */ > + spi->driver_override = NULL; > + kfree(driver_override); > + } > + device_unlock(dev); > + kfree(old); > + > + /* Attach device to new driver if it's not already attached */ > + ret = device_attach(dev); This is something the other buses don't do. Hence they require the user to explicitly bind the device by writing the device name to the driver's "bind" file in sysfs. Looks useful to have for the other buses, too. > + if (ret < 0 && ret != -EPROBE_DEFER) > + dev_warn(dev, "device attach to '%s' failed (%d)\n", > + spi->driver_override, ret); > + > + return count; > +} > + > +static ssize_t driver_override_show(struct device *dev, > + struct device_attribute *a, char *buf) > +{ > + const struct spi_device *spi = to_spi_device(dev); > + > + return snprintf(buf, PAGE_SIZE, "%s\n", spi->driver_override ? : ""); This needs to be protected by device_lock()/device_unlock(), as the string may be freed concurrently. > +} > +static DEVICE_ATTR_RW(driver_override); > + > #define SPI_STATISTICS_ATTRS(field, file) \ > static ssize_t spi_controller_##field##_show(struct device *dev, \ > struct device_attribute *attr, \ > @@ -158,6 +206,7 @@ SPI_STATISTICS_SHOW(transfers_split_maxsize, "%lu"); > > static struct attribute *spi_dev_attrs[] = { > &dev_attr_modalias.attr, > + &dev_attr_driver_override.attr, > NULL, > }; > > @@ -305,6 +354,10 @@ static int spi_match_device(struct device *dev, struct device_driver *drv) > const struct spi_device *spi = to_spi_device(dev); > const struct spi_driver *sdrv = to_spi_driver(drv); > > + /* Check override first, and if set, only use the named driver */ > + if (spi->driver_override) > + return strcmp(spi->driver_override, drv->name) == 0; > + > /* Attempt an OF style match */ > if (of_driver_match_device(dev, drv)) > return 1; > diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h > index 7bb36145e2ba..d90e55029704 100644 > --- a/include/linux/spi/spi.h > +++ b/include/linux/spi/spi.h > @@ -168,6 +168,7 @@ struct spi_device { > void *controller_state; > void *controller_data; > char modalias[SPI_NAME_SIZE]; > + const char *driver_override; > int cs_gpio; /* chip select gpio */ > > /* the statistics */ spidev_release() should call kfree(spi->driver_override), to avoid a leak. The same bug is present in the amba and pci variants. Care to send a fix? Gr{oetje,eeting}s, Geert
On Wed, Sep 19, 2018 at 08:38:09AM +0200, Geert Uytterhoeven wrote: > On Wed, Sep 19, 2018 at 12:50 AM Trent Piepho <tpiepho@impinj.com> wrote: > > + /* Attach device to new driver if it's not already attached */ > > + ret = device_attach(dev); > This is something the other buses don't do. Hence they require the user to > explicitly bind the device by writing the device name to the driver's "bind" > file in sysfs. Looks useful to have for the other buses, too. Or if there's a good reason to require the explicit bind then we should remove this from here so we're consistent between buses. I guess send patches to the other buses and see what happens?
On Wed, 2018-09-19 at 10:38 -0700, Mark Brown wrote: > On Wed, Sep 19, 2018 at 08:38:09AM +0200, Geert Uytterhoeven wrote: > > On Wed, Sep 19, 2018 at 12:50 AM Trent Piepho <tpiepho@impinj.com> wrote: > > > + /* Attach device to new driver if it's not already attached */ > > > + ret = device_attach(dev); > > This is something the other buses don't do. Hence they require the user to > > explicitly bind the device by writing the device name to the driver's "bind" > > file in sysfs. Looks useful to have for the other buses, too. > > Or if there's a good reason to require the explicit bind then we should > remove this from here so we're consistent between buses. I guess send > patches to the other buses and see what happens? Having the automatic attach makes it far easier to use with udev. I can send a patch for other busses. I hope it doesn't get rejected with, "That would have been a good idea, but we can't change it now because someone might care about it not attaching." Even though no one does.
On Wed, Sep 19, 2018 at 08:37:40PM +0000, Trent Piepho wrote: > On Wed, 2018-09-19 at 10:38 -0700, Mark Brown wrote: > > Or if there's a good reason to require the explicit bind then we should > > remove this from here so we're consistent between buses. I guess send > > patches to the other buses and see what happens? > Having the automatic attach makes it far easier to use with udev. I > can send a patch for other busses. I hope it doesn't get rejected > with, "That would have been a good idea, but we can't change it now > because someone might care about it not attaching." Even though no one > does. Yeah, I agree that it's a good default to have - my only concern here is that we have a consistent ABI between the different subsystems.
On Thu, Sep 20, 2018 at 09:10:08AM -0700, Mark Brown wrote: > On Wed, Sep 19, 2018 at 08:37:40PM +0000, Trent Piepho wrote: > > On Wed, 2018-09-19 at 10:38 -0700, Mark Brown wrote: > > > > Or if there's a good reason to require the explicit bind then we should > > > remove this from here so we're consistent between buses. I guess send > > > patches to the other buses and see what happens? > > > Having the automatic attach makes it far easier to use with udev. I > > can send a patch for other busses. I hope it doesn't get rejected > > with, "That would have been a good idea, but we can't change it now > > because someone might care about it not attaching." Even though no one > > does. > > Yeah, I agree that it's a good default to have - my only concern here is > that we have a consistent ABI between the different subsystems. BTW it might be useful to split this up for SPI as well so we can merge the feature without the attach and then add it later if we get all the other buses to agree to do it, that way people can get the feature in the next release even if the attach gets bogged down in review. Hopefully not but just in case...
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 03833924ca6c..2cdfc0dedafa 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -77,6 +77,54 @@ modalias_show(struct device *dev, struct device_attribute *a, char *buf) } static DEVICE_ATTR_RO(modalias); +static ssize_t driver_override_store(struct device *dev, + struct device_attribute *a, + const char *buf, size_t count) +{ + struct spi_device *spi = to_spi_device(dev); + const char *end = memchr(buf, '\n', count); + const size_t len = end ? end - buf : count; + const char *driver_override, *old; + int ret; + + /* We need to keep extra room for a newline when displaying value */ + if (len >= (PAGE_SIZE - 1)) + return -EINVAL; + + driver_override = kstrndup(buf, len, GFP_KERNEL); + if (!driver_override) + return -ENOMEM; + + device_lock(dev); + old = spi->driver_override; + if (len) { + spi->driver_override = driver_override; + } else { + /* Emptry string, disable driver override */ + spi->driver_override = NULL; + kfree(driver_override); + } + device_unlock(dev); + kfree(old); + + /* Attach device to new driver if it's not already attached */ + ret = device_attach(dev); + if (ret < 0 && ret != -EPROBE_DEFER) + dev_warn(dev, "device attach to '%s' failed (%d)\n", + spi->driver_override, ret); + + return count; +} + +static ssize_t driver_override_show(struct device *dev, + struct device_attribute *a, char *buf) +{ + const struct spi_device *spi = to_spi_device(dev); + + return snprintf(buf, PAGE_SIZE, "%s\n", spi->driver_override ? : ""); +} +static DEVICE_ATTR_RW(driver_override); + #define SPI_STATISTICS_ATTRS(field, file) \ static ssize_t spi_controller_##field##_show(struct device *dev, \ struct device_attribute *attr, \ @@ -158,6 +206,7 @@ SPI_STATISTICS_SHOW(transfers_split_maxsize, "%lu"); static struct attribute *spi_dev_attrs[] = { &dev_attr_modalias.attr, + &dev_attr_driver_override.attr, NULL, }; @@ -305,6 +354,10 @@ static int spi_match_device(struct device *dev, struct device_driver *drv) const struct spi_device *spi = to_spi_device(dev); const struct spi_driver *sdrv = to_spi_driver(drv); + /* Check override first, and if set, only use the named driver */ + if (spi->driver_override) + return strcmp(spi->driver_override, drv->name) == 0; + /* Attempt an OF style match */ if (of_driver_match_device(dev, drv)) return 1; diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index 7bb36145e2ba..d90e55029704 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -168,6 +168,7 @@ struct spi_device { void *controller_state; void *controller_data; char modalias[SPI_NAME_SIZE]; + const char *driver_override; int cs_gpio; /* chip select gpio */ /* the statistics */
This attribute works the same was as the identically named attribute for PCI, AMBA, and platform devices. For reference, see: commit 3cf385713460 ("ARM: 8256/1: driver coamba: add device binding path 'driver_override'") commit 3d713e0e382e ("driver core: platform: add device binding path 'driver_override'") commit 782a985d7af2 ("PCI: Introduce new device binding path using pci_dev.driver_override") If the name of a driver is written to this attribute, then the device will bind to the named driver and only the named driver. The device will bind to the driver even if the driver does not list the device in its id table. This behavior is different than the driver's bind attribute, which only allows binding to devices that are listed as supported by the driver. It can be used to bind a generic driver, like spidev, to a device. Signed-off-by: Trent Piepho <tpiepho@impinj.com> --- drivers/spi/spi.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/spi/spi.h | 1 + 2 files changed, 54 insertions(+)