linux-kernel - Re: [PATCH v4] dt-bindings: iio: accel: add binding documentation for ADIS16240

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c725b1b1475148ded6168667fa0227bc18de0854.camel@analog.com>
Date:   Wed, 4 Dec 2019 07:18:15 +0000
From:   "Ardelean, Alexandru" <alexandru.Ardelean@...log.com>
To:     "Jonathan.Cameron@...wei.com" <Jonathan.Cameron@...wei.com>,
        "broonie@...nel.org" <broonie@...nel.org>
CC:     "kernel-usp@...glegroups.com" <kernel-usp@...glegroups.com>,
        "devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "jic23@...nel.org" <jic23@...nel.org>,
        "lars@...afoo.de" <lars@...afoo.de>,
        "Hennerich, Michael" <Michael.Hennerich@...log.com>,
        "pmeerw@...erw.net" <pmeerw@...erw.net>,
        "Popa, Stefan Serban" <StefanSerban.Popa@...log.com>,
        "rodrigorsdc@...il.com" <rodrigorsdc@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "knaack.h@....de" <knaack.h@....de>
Subject: Re: [PATCH v4] dt-bindings: iio: accel: add binding documentation for
 ADIS16240

On Tue, 2019-12-03 at 16:51 +0000, Jonathan Cameron wrote:
> On Tue, 3 Dec 2019 16:38:50 +0000
> Mark Brown <broonie@...nel.org> wrote:
> 
> > On Sun, Dec 01, 2019 at 11:40:32AM +0000, Jonathan Cameron wrote:
> > 
> > > +CC Mark as we probably need a more general view point on
> > > the question of whether SPI mode should be enforced by binding
> > > or in the driver.  
> > 
> > Not sure I see the question here, I think I was missing a bit of
> > the conversation?  It's perfectly fine for a driver to specify a
> > mode, if the hardware always uses some unusual mode then there's
> > no sense in forcing every single DT to set the same mode.  On the
> > other hand if there's some configuration for the driver that was
> > handling some board specific configuration that there's already
> > some generic SPI support for setting then it seems odd to have a
> > custom driver specific configuration mechanism.
> > 
> 
> If the driver picks a mode because that's what it says on the datasheet
> it prevents odd board configurations from working.  The question
> becomes whether it makes sense in general to assume those odd board
> conditions don't exist until we actually have one, or to assume that
> they might and push the burden on to all DT files.
> 
> Traditionally in IIO at least we've mostly taken the view the DT
> should be right and complete and had bindings state what normal
> parameters must be for it to work (assuming no inverters etc)
> 
> If we encode it in the driver, and we later meet such a board we
> end up with a custom dance to query the DT parameters again and
> only override if present.
> 
> We can't rely on the core SPI handling because I don't think
> there is any means of specifying a default.
> 
> We can adopt the view that in general these weird boards with inverters
> are weird and just handle them when they occur.  Sounds like that is your
> preference, at least for new parts.
> 
> For old ones we have no idea if there are boards out there using
> them with inverters so easiest is probably to just carry on putting them
> in the DT bindings.

There might be a few other options, which would require some SPI OF change.

One example (for spi-cpha):
        if (of_property_read_u32(nc, "spi-cpha", &tmp) == 0) {
                spi->mode |= SPI_CPHA_OVERRIDE;
                if (tmp)
                    spi->mode |= SPI_CPHA;
        } else 
             if (of_property_read_bool(nc, "spi-cpha"))
                    spi->mode |= SPI_CPHA;

Or another option could be:
        if (of_property_read_bool(nc, "spi-cpha-override")) {
                spi->mode |= SPI_CPHA_OVERRIDE;
        if
(of_property_read_bool(nc, "spi-cpha"))
                spi->mode |= SPI_CPHA;


Naturally, this would require that spi_setup() checks SPI_CPHA_OVERRIDE and
doesn't set SPI_CPHA if SPI_CPHA_OVERRIDE is set. 

Or maybe, a more complete solution would be an "spi-mode-conv" driver.
Similar to the fixed-factor-clock clk driver, which just does a computation
based on values from the DT.

To tell the truth, this would be a great idea, because we have something
like a passive 3-wire-to-4-wire HDL converter. This requires that the
driver be configured in 3-wire mode, the SPI controller in normal 4-wire.
That's because the SPI framework does a validation of the supported modes
(for the SPI controller) and invalidates what the device wants (which is
very reasonable).

An "spi-mode-conv" driver would also handle this 3-wire-4-wire dance, and
the level inversions, and other (similar) things.

Thoughts?
Alex

> 
> Jonathan
> 
>