Re: [PATCH v3 1/3] spi: spidev: create spidev device for all spi slaves.

[Mark Brown]

On Tue, Jul 19, 2016 at 01:17:52PM +0200, Michal Suchanek wrote:
> On 19 July 2016 at 00:59, Mark Brown <broonie@xxxxxxxxxx> wrote:

> Looking into SPI more this whole point is mostly moot, anyway. Most of the
> time the unused CS lines will not be multiplexed to the SPI controller so
> even if the SPI master tries to use them it does nothing. Also most of the
> time all the CS lines can be exported as GPIO regardless of their use for SPI.
> And set to any possible level using the GPIO userspace API.

Your system is not the entire world, Linux has to run on other systems
too.  There are actual devices out there with inflexible pinmuxing, we
can't just ignore them.

> In order to reduce the amount of objects and management I appended
> the link object at the end of struct spi_device.

> Is there a better way?

That seems mostly fine, the problem is one of code review - because the
patch does many logically distinct things in a single patch it is much
harder to review than if it were split out.  There's a lot of mechanical
refactoring which obscures the several more substantive changes that are
being made at the same time and since most of those substantive changes
are explicitly described at all it's hard to tell if they're intended or
doing what is expected.

> > I am getting completely fed up of saying this, it's not OK to just
> > expose pins to userspace when we have no idea what they are connected
> > to.

> I am getting fed up with this argument.

> First, this patchset only exposes to userspace pins that are configured as SPI
> slaves in devicetree.

Are you *absolutely* positive your changes won't do anything on non-DT
systems?  That definitely doesn't appear to be the case (and shouldn't
be, not all the world is DT), but now I look in more detail what it is
doing is exposing spidev for each explicitly described slave which is
substantially more safe and does address the main problem with
undescribed devices.

Please split this up so it can be reviewed, providing clear and specific
descriptions of each individual change (as covered in
SubmittingPatches).

This:

> -       status = register_chrdev(SPIDEV_MAJOR, "spi", &spidev_fops);
> +       status = register_chrdev(SPIDEV_MAJOR, "spidev", &spidev_fops);

also looks like a needless ABI change and the removal of the locking on
minor device allocation seems concerning and at least needs to be
covered in the changelog.

> The fact that devicetree does not allow talking to a SPI device from userspace
> without telling kernel what kind of protocol userspace is using is a regression
> from using board files. The kernel has no business knowing that. When you
> use a userspace driver it's userspace's job to manage protocols and devices.

The system as a whole, including both the kernel and userspace, has
every business knowing what the hardware is so it can understand how to
control it.  Userspace is no more able to discover what the magic
undescribed bits of hardware are automatically than the kernel is
without help from a hardware description, the DT isn't just something
for the kernel once you have userspace drivers.  

Directly enumerating spidev wasn't an especially great idea for board
files either, it was just an expedient hack that worked when the
hardware description was entirely in kernel and much easier to just go
and change but now we have an external ABI for hardware description and
we have to expect that people will use it as such.

> The purpose of this patchset is for the userspace applications to continue doing
> so with devicetree based systems without the need to fabricate fake hardware
> description. And the hardware description will necessarily be fake in the case
> when it is in fact userspace implementing and managing the drivers.

If userspace is managing to figure out how to control the device then
providing a description of the hardware is clearly within the bounds of
possibility and there is no need to fake anything.

Attachment: signature.asc
Description: PGP signature