Re: [linux-sunxi] [PATCH 2/3] spidev: Add DT binding example.

[Mark Brown <broonie@kernel.org>]

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

On Mon, Apr 27, 2015 at 06:25:26PM +0200, Martin Sperl wrote:
> > On 27.04.2015, at 17:27, Mark Brown <broonie@kernel.org> wrote:

> > OK, so that is just a default overlay which is abusing the fact that we
> > will bind to spidev without a DT compatible and when the binding is
> > undocumented (which also applies to other devices and buses sadly).

> > Unfortunately nobody ever mentioned this upstream and the feedback
> > upstream that listing spidev in a DT is a bad idea has been ignored.

> Maybe it should also have been documented as such in
> Documentation/spi/spidev or in Documentation/devicetree/bindings/spi/

It was documented in the DT bindings in that there was no documented
binding; people are in general expected to know that anything they're
using should be documented.  For the main spidev document I guess it's
something similar, though if you can think of something there by all
means send a patch.

> > The whole reason we're doing this in the first place is that we got sick
> > of telling people that using spidev in DTs like this was a bad idea.

> The only reference I found in my history of the spi-list is this email:

It mostly comes up in review of DT files for boards so won't be on the
SPI list.

> > It does sound like the people maintianing the u-boot fork for the Pi
> > need to talk to both u-boot upstream (nothing here is specific to the
> > Pi that I can see) and the kernel community a bit more.  I'd be a bit
> > worried that they may be relying on other things that just happen to
> > work without being intentional (and are therefore more vulnerable to
> > issues) and it's a bit depressing to see things like this stuck in a
> > fork where only a limited community can make use of them.

> Actually this functionality is not in u-boot, but in the firmware
> boot-loader itself, which can load the kernel (and the devicetree)
> without u-boot, but which can also load u-boot as an additional
> intermediate boot-stage.

Ugh, right.  The thing about talking to the kernel community does still
stand though.

> >> The only thing that could possibly be better would be that
> >> the user would define the "real" name of the device in the
> >> device tree and spidev would bind to it if there is no driver
> >> available (but that would require this "fallback" binding by
> >> spidev in case of no driver).

> > Yes, that is exactly the solution I'd suggest - change the UI to provide
> > a DT compatible to be used for the new device.  That would also have the
> > benefit of meaning that users who have connected some device that does
> > have a driver that works with a simple binding wouldn't need to write an
> > overlay which seems like it should be useful.

> Well then why did you just make the system complain loudly and bringing
> problems to people instead of solving it in a usable manner that does not
> require people to maintain an out of tree patch to work around that warning?

This is quite honestly the first time I've heard of this bootloader
interface that's been implemented for the Pi.  All the other users I've
been aware of have been writing DT files or overlays in tree and
therefore it's not a substantial effort (and indeed the misuse is
basically just people being lazy) - if you can use the shipped binaries
it's a very different tradeoff.

> We still have the one-line warning about using the depreciated 
> spi_master.transfer interface, but it is not such loud warning as this one.

Right, that is a purely in kernel interface visible only to people
directly working on implementing SPI controller drivers that isn't
especially open to misuse and therefore both less serious and more
likely to be noticed.

> I guess the time spent discussing this could have been better spent
> implementing that solution instead.

> All we need is a volunteer to get that implemented.

Yes.

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