Date:	Wed, 13 May 2015 14:51:02 +0200
From:	Maxime Ripard <maxime.ripard@...e-electrons.com>
To:	Mark Brown <broonie@...nel.org>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	linux-kernel@...r.kernel.org, Hans de Goede <hdegoede@...hat.com>,
	linux-spi@...r.kernel.org, Martin Sperl <kernel@...tin.sperl.org>,
	Michal Suchanek <hramrach@...il.com>
Subject: Re: [PATCH] spi: Force the registration of the spidev devices

On Wed, May 13, 2015 at 12:26:04PM +0100, Mark Brown wrote:
> > Solve this by registering automatically spidev devices for all the unused chip
> > selects when a master registers itself against the spi core.
> 
> So, aside from the concern about this being generic the other thing here
> is that we often have devices offering more chip selects than can
> physically be used in a system but not doing anything to ensure that the
> invalid ones can't be used.  It's unclear to me if that's OK or not, it
> might be since it's root only I think but I need to think it through a
> bit.

I might plead guilty here... :)

I'd say we're also ok because if we delegate the device driving logic
to userspace, we should expect it to know what it does to first drive
the device properly, but also to open the right device for this.

What's the worst that could happen in such a case? The data are output
without any chipselect line being driven by the controller? Isn't that
supposed to be ignored by the devices?

> > This also adds an i2cdev-like feeling, where you get all the
> > spidev devices all the time, without any modification.
> 
> I2C is a bit safer here since it's a shared bus so you can't do
> anything to devices not connected to the bus by mistake.

I'm not sure to understand what you mean here. How is SPI different
from that aspect?

> > +		/*
> > +		 * This is far from perfect since an addition might be
> > +		 * done between here and the call to spi_add_device,
> > +		 * but we can't hold the lock and call spi_add_device
> > +		 * either, as it would trigger a deadlock.
> > +		 *
> > +		 * If such a race occurs, spi_add_device will still
> > +		 * catch it though, as it also checks for devices
> > +		 * being registered several times on the same chip
> > +		 * select.
> > +		*/
> > +		status = bus_for_each_dev(&spi_bus_type, NULL, spi,
> > +					  spi_dev_check);
> > +		if (status) {
> > +			dev_dbg(&master->dev, "Chipselect already in use.. Skipping.");
> > +			spi_dev_put(spi);
> > +			continue;
> > +		}
> 
> This still leaves us in the situation where if we do know the device
> that is connected we have to explicitly bind it in spidev which is
> apparently unreasonably difficult for people.

You can still do that, but the point is that you don't have to.

If you know what device you have, and want to use spidev on that
device, it will still work, since it will create an spidev device when
we will parse the DT.

That device will then be skipped by that logic, which is ok.

However, if you don't specify anything in your DT, and have no device
created because you don't really know what's on the other end, this
logic will create the spidev device so that you'll still get an spidev
node exported to the userspace.

> I'm also concerned about the interactions with DT overlays here -
> what happens if a DT overlay or other dynamic hardware instantiation
> comes along later and does bind something to this chip select?  It
> seems like we should be able to combine the two models, and the fact
> that we only create these devices with a Kconfig option is a bit of
> an interesting thing here.

I think the safe approach would be, just like I told in this thread,
to just check whether the modalias is spidev. If it is, destroy the
previous (spidev) device, create a new device as specified by the DT,
you're done.

Maxime

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

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists