On 11/11/2020 13:36, Linus Walleij wrote:
On Wed, Nov 11, 2020 at 1:33 PM Mark Brown <broo...@kernel.org> wrote:
On Wed, Nov 11, 2020 at 02:05:19AM +0100, Linus Walleij wrote:
I would say that anything that has:
spi->mode = ...
is essentially broken.
This is not clear to me, most of these settings are things that are
constant for the device so it's not clear that they should be being set
by the device tree in the first place.
This was added initially with some two properties
in drivers/of/of_spi.c in 2008:
commit 284b01897340974000bcc84de87a4e1becc8a83d
"spi: Add OF binding support for SPI busses"
This was around the time ARM was first starting to migrate
to device tree, so I suppose it made sense to them/us back
then.
Some properties were the accumulated over time.
commit d57a4282d04810417c4ed2a49cbbeda8b3569b18
"spi/devicetree: Move devicetree support code into spi directory"
made this part of the SPI subsystem.
This seems as simple as nobody was there to push back and
say "wait the devices can specify that with code, don't put it
as properties in device tree". To be honest we have kind of
moved back and forward on that topic over time. :/
The idea that the chip select
might be being inverted like it is by this whole gpiolib/DT/new binding
thing is breaking expectations too.
OK I think you're right, then this patch probably brings the behaviour
back to expectations and it's how I should have done it in the first
place. My bad code :/
Reviewed-by: Linus Walleij <linus.wall...@linaro.org>
The core sets up vital things in .mode from e.g. device tree in
of_spi_parse_dt():
/* Mode (clock phase/polarity/etc.) */
if (of_property_read_bool(nc, "spi-cpha"))
spi->mode |= SPI_CPHA;
if (of_property_read_bool(nc, "spi-cpol"))
spi->mode |= SPI_CPOL;
if (of_property_read_bool(nc, "spi-3wire"))
spi->mode |= SPI_3WIRE;
if (of_property_read_bool(nc, "spi-lsb-first"))
spi->mode |= SPI_LSB_FIRST;
if (of_property_read_bool(nc, "spi-cs-high"))
spi->mode |= SPI_CS_HIGH;
All this gets overwritten and ignored when a client just assigns mode
like that. Not just SPI_CS_HIGH. I doubt things are different
with ACPI.
OTOH most of these are things the device driver should just get right
without needing any input from DT, there's a few where there's plausible
options (eg, you can imagine pin strap configuration for 3 wire mode)
Yes I actually ran into a case where the same Samsung display support
both 4 and 3-wire mode so that needs to be configured in the device
tree depending on the layout of the electronics. Arguably we should have
just standardized the device tree bindings and let the individual SPI
drivers parse that themselves in such cases.
so generally it's not clear how many of these make sense for anything
other than spidev. This binding all predates my involvement so I don't
know the thought process here.
I dug out some details, let's see if Grant has some historical anecdotes
to add. The usage document from back then doesn't really say what
device properties should be encoded in the device tree and what
should just be assigned by code and e.g. determined from the
compatible-string. It was later that especially Rob pointed out that
random properties on device nodes was overused and that simply
knowing the compatible is often enough.
I think your analysis is correct. When this was done we were still
figuring stuff out and the abstraction between device and bus in SPI
isn't exactly clean. I don't have anything to add.
g.
I don't know if we ever formalized it, there is nowadays a rule akin to
"if a property can be determined from the compatible-string, and if the
compatible-string is identifying the variant of the electronic component,
then do not add this property to the device tree description. Just
deduce it from the compatible-string, assign it with code to the device
model of the operating system and handle it inside the operating system."
I think this, while clear and intuitive, wasn't at all clear and intuitive in
the recent past.
Yours,
Linus Walleij
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.