Re: [PATCH 1/2] spi: support inter-word delay requirement for devices

[Geert Uytterhoeven]

Hi Jonas,

On Sat, Jan 26, 2019 at 4:40 PM Jonas Bonn <jonas@xxxxxxxxxxx> wrote:
> On 26/01/2019 11:25, Geert Uytterhoeven wrote:
> > On Sat, Jan 26, 2019 at 8:53 AM Jonas Bonn <jonas@xxxxxxxxxxx> wrote:
> >> On 25/01/2019 18:50, Mark Brown wrote:
> >>> On Fri, Jan 25, 2019 at 05:47:13PM +0000, Mark Brown wrote:
> >>>> On Fri, Jan 25, 2019 at 01:06:45PM +0100, Jonas Bonn wrote:
> >>>>> Having this as device property rather than a transfer property allows this
> >>>>> to be configured one time in setup() rather than having to fiddle with the
> >>>>> configuration register for every transfer.
> >>>
> >>>> That doesn't mean that the coniguration should be done in DT though, and
> >>>> given that this presumably is a property of the device there seems to be
> >>>> no reason why we'd have it in DT - if every instance of the device is
> >>>> going to need to set the property we should just figure it out from the
> >>>> compatble string instead.
> >>>
> >>> To be clear here: the suggestion is to add a parameter the slave device
> >>> can set in spi_device which sets the default word_delay similarly to how
> >>> max_speed_hz works.
> >>
> >> I'm confused... isn't that exactly what this patch does?  It adds a
> >> field word_delay to spi_device in the same manner as max_speed_hz.
> >>
> >> I also added the ability to set it via DT, which I can break out into a
> >> separate patch if that's an issue.  Or is the problem that it's set via
> >> DT, at all?  Documentation/devicetree/bindings/spi-bus.txt documents 10
> >> other slave-node properties related to transfer characteristics;
> >> word_delay is just another such characteristic.
> >>
> >> But again, I'm having trouble parsing your response  Is the patch wrong,
> >> should be broken up, or you just misunderstood it?
> >
> > IIUIC, Mark means that it may be a non-configurable property of the slave
> > device, and thus should be handled (fixed setting) in the SPI slave driver.
>
> OK, so given that the "compatible" property identifies the hardware and
> there is no other _hardware_ configuration detail that affects the
> required inter-word delay, then setting the property in the device tree
> is not allowed as the driver has enough info to set it properly.  Fair
> enough!
>
> >
> > Compare this to CPHA/CPOL, which are properties of the SPI slave device,
> > but which may be configurable. E.g. many SPI FLASHes support multiple
> > configurations. See e.g. commit 9c5becce21af35e5 ("ARM: shmobile: koelsch:
> > Fix QSPI mode of SPI-Flash into mode3").
>
> There is nothing about the hardware referenced in that patch that
> enforces either mode.  Why does the driver get to defer to DT here?

The default is non-cpol and non-pha.
The device supports both (perhaps even all four modes, didn't check).

> Looking at Documentation/devicetree/bindings/spi/spi-bus.txt:
>
> spi-lsb-first:  used only by MAXIM DS-1302 which is always LSB first;
> driver should set this

For DS1302, this is probable true.
For e.g. a generic shift register (e.g. hc595), the driver cannot know.

> spi-3wire: again, only set by MAXIM DS-1302 which always needs this
> setting; driver could set this

For DS1302, this is probable true.
Some devices may support both 4-wire or 3-wire mode?

> spi-tx-delay-us:
> spi-rx-delay-us:  only parsed by RMI4 driver... no DTS files in kernel
> set these.  I hardly see how these are "hardware" settings rather than
> message settings, but the driver can set these in any case.
>
> Anyway, the point is, looking at the current documentation, it's pretty
> difficult to understand whether devicetree is restricted to describing
> hardware or is also for configuring drivers...

True.

> > Again, max_speed_hz is something different: while both the SPI master and
> > slave may support high speeds, board wiring (capacitance/inductance) may
> > need to force a slower speed than supported by the devices, so it makes
> > sense to make that configurable from DT.
>
> Yeah, that one make sense.  But if the DT is only overriding the maximum
> device frequency that the driver should otherwise be setting, why is
> spi-max-frequency a _required_ property?

That's a good question. Legacy?

> Thanks for taking the time to provide the additional explanation.  I had
> to ruminate on it for a bit, but I think it's starting to sink in now!

You're welcome!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds