| Message ID | 5e0397742f887f656d67bb0d61c8e10782c0e5af.1466696079.git.hramrach@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 23, 2016 at 05:41:20PM -0000, Michal Suchanek wrote: > This allows binding spidev on any slave device by hand using sysfs > without adding superfluous compatibles or any other needless > complication. This says "spidev" but it's a change to the driver core, not something that is specific to spidev. It needs to be explained in terms ofn the driver core, especially given that other runtime ID setting is done in a particular bus. > +menuconfig DRIVER_MATCH_OVERRIDE > + bool "Allow manual driver binding to override id match (DANGEROUS)" > + default n > + help > + When binding a driver manually bypass the check of driver id table > + against device id in driver core. This can be useful for development > + or on buses that don't provide reliable device identification. > + > +config DRIVER_MATCH_OVERRIDE_BUSES > + string "Specify buses for which id matching will be overridden" > + default "spi" > + depends on DRIVER_MATCH_OVERRIDE > + help > + space separated bus names This doesn't seem at all idomatic for how I'd expect bus features to be implemented, or Linux configuration in general. If we're going to allow a feature like this at the core level we should just allow it, if we're going to opt buses into any such feature I'd expect it to be done unconditionally.
On Thu, Jun 23, 2016 at 10:41 AM, Michal Suchanek <hramrach@gmail.com> wrote: > This allows binding spidev on any slave device by hand using sysfs > without adding superfluous compatibles or any other needless > complication. > > Note that any slave driver that requires configuration will fail to > probe anyway. Only a driver that binds to anything can be bound > successfully. > > Signed-off-by: Michal Suchanek <hramrach@gmail.com> > --- > drivers/base/Kconfig.debug | 14 +++++++++ > drivers/base/bus.c | 72 +++++++++++++++++++++++++++++++++++++++++++++- > lib/Kconfig.debug | 2 ++ > 3 files changed, 87 insertions(+), 1 deletion(-) > create mode 100644 drivers/base/Kconfig.debug > Why change the driver core? The matching policy can be changed with the "match" operation of a custom "struct bus_type". -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On 26 June 2016 at 06:14, Dan Williams <dan.j.williams@intel.com> wrote: > On Thu, Jun 23, 2016 at 10:41 AM, Michal Suchanek <hramrach@gmail.com> wrote: >> This allows binding spidev on any slave device by hand using sysfs >> without adding superfluous compatibles or any other needless >> complication. >> >> Note that any slave driver that requires configuration will fail to >> probe anyway. Only a driver that binds to anything can be bound >> successfully. >> >> Signed-off-by: Michal Suchanek <hramrach@gmail.com> >> --- >> drivers/base/Kconfig.debug | 14 +++++++++ >> drivers/base/bus.c | 72 +++++++++++++++++++++++++++++++++++++++++++++- >> lib/Kconfig.debug | 2 ++ >> 3 files changed, 87 insertions(+), 1 deletion(-) >> create mode 100644 drivers/base/Kconfig.debug >> > > Why change the driver core? The matching policy can be changed with > the "match" operation of a custom "struct bus_type". It cannot. Only driver core knows that the match is result of writing the bind file in sysfs. This is information is discarded at this very point. On 26 June 2016 at 03:17, Mark Brown <broonie@kernel.org> wrote: > On Thu, Jun 23, 2016 at 05:41:20PM -0000, Michal Suchanek wrote: > >> This allows binding spidev on any slave device by hand using sysfs >> without adding superfluous compatibles or any other needless >> complication. > > This says "spidev" but it's a change to the driver core, not something > that is specific to spidev. It needs to be explained in terms ofn the > driver core, especially given that other runtime ID setting is done in a > particular bus. > >> +menuconfig DRIVER_MATCH_OVERRIDE >> + bool "Allow manual driver binding to override id match (DANGEROUS)" >> + default n >> + help >> + When binding a driver manually bypass the check of driver id table >> + against device id in driver core. This can be useful for development >> + or on buses that don't provide reliable device identification. >> + >> +config DRIVER_MATCH_OVERRIDE_BUSES >> + string "Specify buses for which id matching will be overridden" >> + default "spi" >> + depends on DRIVER_MATCH_OVERRIDE >> + help >> + space separated bus names > > This doesn't seem at all idomatic for how I'd expect bus features to be > implemented, or Linux configuration in general. If we're going to allow > a feature like this at the core level we should just allow it, if we're > going to opt buses into any such feature I'd expect it to be done > unconditionally. Allowing some control over enabling or disabling this may be more generally acceptable. I was even considering a sysfs file that would enable control of this feature at runtime. The main use case for this is spidev. I am not familiar with all kernel subsystems so there might be other uses. The problem with spidev is this: The SPI bus feature-wise is closest to serial UARTs form the other buses available in kernel. Serial uart does not even have a bus driver. The main hardware difference is that part of SPI specification is CS which makes it proper bus whereas serial UARTs have no CS which makes them point to point. You can use gpios to multiplex serial devices too if you wanted - it's just non-standard setup. The main kernel difference is that for serial UART a character device is created which a userspace program can use to send and receive data in the hope that whatever device is expected on the other side is actually there. This is the traditional use of serial lines. In addition there are serial drivers that expect to be driving a device that is on the mainboard and just happens to be connected by a serial line to the CPU. These might need additional GPIOs and can probably be configured by devicetree (iirc IR and Bluetooth devices of this type exist). For SPI the latter is the traditional use and recently boards with SPI connectors started to be common and people want to use the SPI bus the same way serial ports are used - connect some random device to the pins and try to talk to it from userspace. Spidev is the driver which allows the use of SPI similar to how serial ports are used. The boards typically use devicetree for hardware description. And spidev is not configured anywhere in the devicetree. The response was to generate random compatible for the SPI port, add that compatible to spidev, and load it as the driver. Actually, you were supposed to generate a new compatible for every kind of device. That does not make any sense. It's like asking people to generate a new kernel config and reboot just to connect a different kind of modem to their serial port. With devicetree overlays you can load an overlay for complicated devices that need GPIOs or extra settings and happen to have kernel driver (eg fbtft). Once loading the overlays is possible in mainline. With less complicated drivers like m25p80 you can get full features with overlays and limited features with this patch. And with spidev you cannot use overlays because 'it is not supposed to be specified in devicetree because it is linux implementation detail and not hardware'. Another option to do this outside of core is to generate a compatible for generic SPI external header, add that to spidev, and be done with it. No-go again. An alternative approach that has been tried was to probe the SPI slave and if no driver is found bind spidev automatically in the kernel. This has two problems. If you cannot specify the external connector in DT you cannot know which CS to probe and which to leave alone. The concern is that triggering unused CS could disturb other hardware. The other problem is you never know if a driver is going to be loaded later and will be blocked from binding by spidev. It would also mean that if you unbind spidev you cannot re-bind it because it does not match. Just leaving the device without a driver and let userspace decide to bind it or not seems like more general approach which can be potentially reused for other cases. The last option I see is to get rid of separate spidev driver and just create a character device for every spi slave and bind the spidev IOCTLs to it when spidev support is enabled. It is specific to spidev and needs no core changes. It may cause mild conflict with spi slaves creating their own character devices. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Michal, On Sun, Jun 26, 2016 at 8:44 AM, Michal Suchanek <hramrach@gmail.com> wrote: > The SPI bus feature-wise is closest to serial UARTs form the other > buses available in kernel. Serial uart does not even have a bus > driver. That's being worked on, as currently you can't describe the other side of the UART pins (e.g. a connected Bluetooth device). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 23, 2016 at 05:41:20PM -0000, Michal Suchanek wrote: > This allows binding spidev on any slave device by hand using sysfs > without adding superfluous compatibles or any other needless > complication. > > Note that any slave driver that requires configuration will fail to > probe anyway. Only a driver that binds to anything can be bound > successfully. > > Signed-off-by: Michal Suchanek <hramrach@gmail.com> > --- > drivers/base/Kconfig.debug | 14 +++++++++ > drivers/base/bus.c | 72 +++++++++++++++++++++++++++++++++++++++++++++- > lib/Kconfig.debug | 2 ++ > 3 files changed, 87 insertions(+), 1 deletion(-) > create mode 100644 drivers/base/Kconfig.debug > > diff --git a/drivers/base/Kconfig.debug b/drivers/base/Kconfig.debug > new file mode 100644 > index 0000000..e21d3cc > --- /dev/null > +++ b/drivers/base/Kconfig.debug > @@ -0,0 +1,14 @@ > +menuconfig DRIVER_MATCH_OVERRIDE > + bool "Allow manual driver binding to override id match (DANGEROUS)" > + default n > + help > + When binding a driver manually bypass the check of driver id table > + against device id in driver core. This can be useful for development > + or on buses that don't provide reliable device identification. Ick, no no no. Why would you ever want to let this happen? If you really want to override the check, just write things to the 'bind' file in sysfs, that will skip the driver id check entirely, right? > + > +config DRIVER_MATCH_OVERRIDE_BUSES > + string "Specify buses for which id matching will be overridden" > + default "spi" > + depends on DRIVER_MATCH_OVERRIDE > + help > + space separated bus names Gotta love parsers of config items :( Again, please no. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 26 June 2016 at 20:28, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Thu, Jun 23, 2016 at 05:41:20PM -0000, Michal Suchanek wrote: >> This allows binding spidev on any slave device by hand using sysfs >> without adding superfluous compatibles or any other needless >> complication. >> >> Note that any slave driver that requires configuration will fail to >> probe anyway. Only a driver that binds to anything can be bound >> successfully. >> >> Signed-off-by: Michal Suchanek <hramrach@gmail.com> >> --- >> drivers/base/Kconfig.debug | 14 +++++++++ >> drivers/base/bus.c | 72 +++++++++++++++++++++++++++++++++++++++++++++- >> lib/Kconfig.debug | 2 ++ >> 3 files changed, 87 insertions(+), 1 deletion(-) >> create mode 100644 drivers/base/Kconfig.debug >> >> diff --git a/drivers/base/Kconfig.debug b/drivers/base/Kconfig.debug >> new file mode 100644 >> index 0000000..e21d3cc >> --- /dev/null >> +++ b/drivers/base/Kconfig.debug >> @@ -0,0 +1,14 @@ >> +menuconfig DRIVER_MATCH_OVERRIDE >> + bool "Allow manual driver binding to override id match (DANGEROUS)" >> + default n >> + help >> + When binding a driver manually bypass the check of driver id table >> + against device id in driver core. This can be useful for development >> + or on buses that don't provide reliable device identification. > > Ick, no no no. Why would you ever want to let this happen? If you > really want to override the check, just write things to the 'bind' file > in sysfs, that will skip the driver id check entirely, right? Well, it does not. Hence this patch which enables skipping the check. It's the whole point of it. Thanks Michal -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Jun 26, 2016 at 09:07:08PM +0200, Michal Suchanek wrote: > On 26 June 2016 at 20:28, Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Thu, Jun 23, 2016 at 05:41:20PM -0000, Michal Suchanek wrote: > >> This allows binding spidev on any slave device by hand using sysfs > >> without adding superfluous compatibles or any other needless > >> complication. > >> > >> Note that any slave driver that requires configuration will fail to > >> probe anyway. Only a driver that binds to anything can be bound > >> successfully. > >> > >> Signed-off-by: Michal Suchanek <hramrach@gmail.com> > >> --- > >> drivers/base/Kconfig.debug | 14 +++++++++ > >> drivers/base/bus.c | 72 +++++++++++++++++++++++++++++++++++++++++++++- > >> lib/Kconfig.debug | 2 ++ > >> 3 files changed, 87 insertions(+), 1 deletion(-) > >> create mode 100644 drivers/base/Kconfig.debug > >> > >> diff --git a/drivers/base/Kconfig.debug b/drivers/base/Kconfig.debug > >> new file mode 100644 > >> index 0000000..e21d3cc > >> --- /dev/null > >> +++ b/drivers/base/Kconfig.debug > >> @@ -0,0 +1,14 @@ > >> +menuconfig DRIVER_MATCH_OVERRIDE > >> + bool "Allow manual driver binding to override id match (DANGEROUS)" > >> + default n > >> + help > >> + When binding a driver manually bypass the check of driver id table > >> + against device id in driver core. This can be useful for development > >> + or on buses that don't provide reliable device identification. > > > > Ick, no no no. Why would you ever want to let this happen? If you > > really want to override the check, just write things to the 'bind' file > > in sysfs, that will skip the driver id check entirely, right? > > Well, it does not. Hence this patch which enables skipping the check. > It's the whole point of it. Then write the id you wish to use for that driver to the new_id file for that driver. Doesn't that work? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/base/Kconfig.debug b/drivers/base/Kconfig.debug new file mode 100644 index 0000000..e21d3cc --- /dev/null +++ b/drivers/base/Kconfig.debug @@ -0,0 +1,14 @@ +menuconfig DRIVER_MATCH_OVERRIDE + bool "Allow manual driver binding to override id match (DANGEROUS)" + default n + help + When binding a driver manually bypass the check of driver id table + against device id in driver core. This can be useful for development + or on buses that don't provide reliable device identification. + +config DRIVER_MATCH_OVERRIDE_BUSES + string "Specify buses for which id matching will be overridden" + default "spi" + depends on DRIVER_MATCH_OVERRIDE + help + space separated bus names diff --git a/drivers/base/bus.c b/drivers/base/bus.c index 6470eb8..752c2a0 100644 --- a/drivers/base/bus.c +++ b/drivers/base/bus.c @@ -199,6 +199,73 @@ static ssize_t unbind_store(struct device_driver *drv, const char *buf, } static DRIVER_ATTR_WO(unbind); +#ifdef CONFIG_DRIVER_MATCH_OVERRIDE_BUSES + +/* nul separated "" terminated strings */ +static const char *driver_override_buses; + +static inline void init_overrides(void) +{ + const char *buses_str = CONFIG_DRIVER_MATCH_OVERRIDE_BUSES; + char *transcript; + int i, len = strlen(buses_str); + + if (!len) + return; + + transcript = kzalloc(len + 1, GFP_KERNEL); + if (!transcript) + return; + driver_override_buses = transcript; + + for (i = 0; i < len; i++) { + + while (buses_str[i] == ' ') + i++; + + if (buses_str[i]) { + const char *name_start = buses_str + i; + const char *name_end = strchrnul(name_start, ' '); + int name_len = name_end - name_start; + + strncpy(transcript, name_start, name_len); + i += name_len; + transcript += name_len; + transcript++; + } + } +} + +static inline bool driver_match_override(struct device_driver *drv, + struct device *dev) +{ + struct bus_type *bus = bus_get(drv->bus); + const char *cmp_name = driver_override_buses; + + while (cmp_name && *cmp_name) { + if (!strcmp(bus->name, cmp_name)) { + pr_notice("Overriding id match on manual driver binding:\n bus: %s driver: %s device: %s\n", + bus->name, drv->name, dev_name(dev)); + return true; + } + cmp_name += strlen(cmp_name); + cmp_name++; + } + + return false; +} + +#else /*CONFIG_DRIVER_MATCH_OVERRIDE_BUSES*/ + +static inline void init_overrides(void) +{} + +static inline bool driver_match_override(struct device_driver *drv, + struct device *dev) +{ return false; } + +#endif + /* * Manually attach a device to a driver. * Note: the driver must want to bind to the device, @@ -212,7 +279,8 @@ static ssize_t bind_store(struct device_driver *drv, const char *buf, int err = -ENODEV; dev = bus_find_device_by_name(bus, NULL, buf); - if (dev && dev->driver == NULL && driver_match_device(drv, dev)) { + if (dev && dev->driver == NULL && (driver_match_device(drv, dev) + || driver_match_override(drv, dev))) { if (dev->parent) /* Needed for USB */ device_lock(dev->parent); device_lock(dev); @@ -1280,5 +1348,7 @@ int __init buses_init(void) if (!system_kset) return -ENOMEM; + init_overrides(); + return 0; } diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 1e9a607..f388212 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -2002,3 +2002,5 @@ config IO_STRICT_DEVMEM if the driver using a given range cannot be disabled. If in doubt, say Y. + +source drivers/base/Kconfig.debug
This allows binding spidev on any slave device by hand using sysfs without adding superfluous compatibles or any other needless complication. Note that any slave driver that requires configuration will fail to probe anyway. Only a driver that binds to anything can be bound successfully. Signed-off-by: Michal Suchanek <hramrach@gmail.com> --- drivers/base/Kconfig.debug | 14 +++++++++ drivers/base/bus.c | 72 +++++++++++++++++++++++++++++++++++++++++++++- lib/Kconfig.debug | 2 ++ 3 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 drivers/base/Kconfig.debug