Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1430698 > unrolled thread

[PATCH 0/3] Another stab at making spidev usable with devicetree

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] Another stab at making spidev usable with devicetree Michal Suchanek <hramrach@gmail.com> - 2016-06-24 16:30 +0200
    [PATCH 3/3] drivers core: allow id match override when manually binding driver Michal Suchanek <hramrach@gmail.com> - 2016-06-24 16:30 +0200
      Re: [PATCH 3/3] drivers core: allow id match override when manually  binding driver Mark Brown <broonie@kernel.org> - 2016-06-26 03:20 +0200
      Re: [PATCH 3/3] drivers core: allow id match override when manually  binding driver Dan Williams <dan.j.williams@intel.com> - 2016-06-26 06:20 +0200
        Re: [PATCH 3/3] drivers core: allow id match override when manually  binding driver Michal Suchanek <hramrach@gmail.com> - 2016-06-26 08:50 +0200
          Re: [PATCH 3/3] drivers core: allow id match override when manually  binding driver Geert Uytterhoeven <geert@linux-m68k.org> - 2016-06-26 11:30 +0200
      Re: [PATCH 3/3] drivers core: allow id match override when manually  binding driver Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-06-26 21:00 +0200
        Re: [PATCH 3/3] drivers core: allow id match override when manually  binding driver Michal Suchanek <hramrach@gmail.com> - 2016-06-26 21:10 +0200
          Re: [PATCH 3/3] drivers core: allow id match override when manually  binding driver Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-06-27 21:10 +0200

#1430698 — [PATCH 0/3] Another stab at making spidev usable with devicetree

Hello,

This small patchset makes it possible to bind spidev manually to any SPI slave
without much hassle.

Thanks

Michal

Michal Suchanek (3):
  spi: spidev: fix the check for spidev in dt
  spi: of: allow instantiating slaves without a driver
  drivers core: allow id match override when manually binding driver

 drivers/base/Kconfig.debug | 14 +++++++++
 drivers/base/bus.c         | 72 +++++++++++++++++++++++++++++++++++++++++++++-
 drivers/spi/spi.c          | 10 +++----
 drivers/spi/spidev.c       |  9 ++++--
 lib/Kconfig.debug          |  2 ++
 5 files changed, 98 insertions(+), 9 deletions(-)
 create mode 100644 drivers/base/Kconfig.debug

-- 
2.8.1

[toc] | [next] | [standalone]

#1430699 — [PATCH 3/3] drivers core: allow id match override when manually binding driver

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.
+
+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
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1431269 — Re: [PATCH 3/3] drivers core: allow id match override when manually binding driver

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

#1431327 — Re: [PATCH 3/3] drivers core: allow id match override when manually binding driver

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".

[toc] | [prev] | [next] | [standalone]

#1431349 — Re: [PATCH 3/3] drivers core: allow id match override when manually binding driver

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

[toc] | [prev] | [next] | [standalone]

#1431390 — Re: [PATCH 3/3] drivers core: allow id match override when manually binding driver

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

[toc] | [prev] | [next] | [standalone]

#1431558 — Re: [PATCH 3/3] drivers core: allow id match override when manually binding driver

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

[toc] | [prev] | [next] | [standalone]

#1431561 — Re: [PATCH 3/3] drivers core: allow id match override when manually binding driver

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

[toc] | [prev] | [next] | [standalone]

#1432225 — Re: [PATCH 3/3] drivers core: allow id match override when manually binding driver

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web