Re: [PATCH v1] spi: fix client driver breakages when using GPIO descriptors

From: Grant Likely <grant.likely@arm.com>
To: Linus Walleij <linus.walleij@linaro.org>,
	Mark Brown <broonie@kernel.org>, Rob Herring <robh+dt@kernel.org>
Cc: Sven Van Asbroeck <thesven73@gmail.com>,
	Andy Shevchenko <andy.shevchenko@gmail.com>,
	Jonathan Cameron <jonathan.cameron@huawei.com>,
	Simon Han <z.han@kunbus.com>, Lukas Wunner <lukas@wunner.de>,
	linux-spi <linux-spi@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v1] spi: fix client driver breakages when using GPIO descriptors
Date: Wed, 25 Nov 2020 09:17:17 +0000	[thread overview]
Message-ID: <30299db4-e149-ea7e-8f30-bb37187909d5@arm.com> (raw)
In-Reply-To: <CACRpkdZW3G48Yj3yGMTKZGwVEQOSs1VeVTTGLgyoJViM3=Yedg@mail.gmail.com>

On 11/11/2020 13:36, Linus Walleij wrote:
> On Wed, Nov 11, 2020 at 1:33 PM Mark Brown <broonie@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.walleij@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.