Re: [PATCH] spi: Force the registration of the spidev devices

[Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 4986 bytes --]

On Tue, Apr 29, 2014 at 11:31:49PM +0200, Martin Sperl wrote:
> On 29.04.2014, at 20:37, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> > On Mon, Apr 28, 2014 at 10:22:54AM -0700, Maxime Ripard wrote:
> > 
> >> spidev device registration has always been a controversial subject since the
> >> move to DT.
> > 
> > Why can we not handle this by using sysfs to bind spidev to the device?
> 
> 
> 
> I think the question is actually more generic than just "spidev" related.
> 
> A solution should also help all those users of development boards like 
> raspberry pi, beagle board,... where people want to add spi devices without
> having to know too much about the fine details of the kernel configuration
> /kernel compiling/creating a new device tree or similar.... 
> They just want their device to work with the existing kernel 
> with minimal effort!
> 
> For just this purpose I got an out of tree module called spi-config that 
> - as of now - allows to define the binding of spi devices (not solely 
> focused on spidev) as per module argument while loading the driver.
> 
> This could easily get extended to work also via sysfs.
> 
> The problem here is mostly around the "passing" of driver specific platform 
> data, which this module currently handles in a sort of "binary blob" way, 
> which is suboptimal (as it is very architecture specific),
> but at least it works...
> 
> Some examples of what is there:
> 
> Bind spidev to spi0.1 with a speed of 2MHz and SPI mode 3:
> modprobe spi-config bus=0:cs=1:modalias=spidev:speed=2000000:mode=3
> 
> Bind an mcp2515 CAN-bus controller to spi0.0 with 10MHz speed and 
> IRQ triggered via GPIO25:
> modprobe spi-config bus=0:cs=0:modalias=mcp2515:speed=10000000:gpioirq=25:\
> mode=0:pd=20:pdu32-0=16000000:pdu32-4=0x2002
> 
> The extra "pd...=" args are the ones describing platform data that is needed
> by the mcp251x driver to set up the device correctly. 
> In the above example we define:
> * pd=20: the platform data size is 20 bytes (zero filled)
> * pdu32-0: a u32 at offset 0 with a value of 16MHz Quarz speed
> * pdu32-4: a u32 at offset 4 the interrupt flags used by the driver
>            (=IRQF_TRIGGER_FALLING|IRQF_ONESHOT)
> 
> The above mcp2515 device now configured via sysfs could look like this:
> echo "cs=0:modalias=mcp2515:speed=10000000:gpioirq=25:mode=0:\
> pd=20:pdu32-0=16000000:pdu32-4=0x2002" > /sys/class/spi_master/spi0/register
> 
> Unregistering the device:
> echo "cs=0" > /sys/class/spi_master/spi0/unregister
> 
> As said: the trickiest part is "platform data", all the "spi-parameters"
> are well-defined and are easily parseable.
> 
> Options that come to my mind to increase "readability" are:
> * "const char *extra_parameters" 
>   in spi_device containing all "unhandled" parameters left for the driver
>   to parse during probing.
> * "int (*config)(struct spi_device *,const char *key,const char *value)"
>   in spi_driver that sets the corresponding parameters
>   (creating platform data if it is not already set/allocated) 
>   and which is called prior to calling spi_driver->probe(spi)
> 
> So with those "readability" options the sysfs interface could 
> look like this:
> echo "cs=0:modalias=mcp2515:speed=10000000:gpioirq=25:mode=0:\
> can_oscillator_hz=16000000:interrupt_flags=0x2002" \
> > /sys/class/spi_master/spi0/register
> 
> When using the second option the framework could fall back to the
> voodoo-"pd...=" approach that I have show above to avoid having to touch 
> all spi drivers immediately to make use of this interface.
> 
> The biggest pitfall I see is that if a device driver expects platform
> data but none is provided, then loading the driver could crash the kernel.
> But that is something that probably should get fixed in the driver 
> itself and not in the framework.
> 
> A totally different approach could be the use of device-tree overlays, 
> which is also not included in the kernel, but would allow for similar
> config schemes.
> But that would leave systems not using device tree without this option.
> 
> So it seems as if we need to find an approach that is acceptable...

I'd really be in favor of using the DT overlays for this. All this is
nice, but very fragile. You can't really handle any weird
configurations that you might encounter, like weird interrupt
controllers that might be requiring more informations, or you don't
set any interrupt parents, and this parameter list will basically be
an ever-growing list of parameters to deal with all kind of crazy
corner-cases, and I'm not sure it's something we want.

You listed only ARM boards, that are not supposed to be non-DT. RPi is
using DT fine, just like the beaglebones. I don't really see why we
should care about !DT cases.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]