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

[Michal Suchanek]

Hello,

On 26 June 2016 at 06:14, Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
> On Thu, Jun 23, 2016 at 10:41 AM, Michal Suchanek <hramrach@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>>  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@xxxxxxxxxx> 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