Re: [PATCH] spi: mt7621: allow GPIO chip select lines

From: Justin Swartz <justin.swartz@risingedge.co.za>
To: Mark Brown <broonie@kernel.org>
Cc: Matthias Brugger <matthias.bgg@gmail.com>,
	AngeloGioacchino Del Regno
	<angelogioacchino.delregno@collabora.com>,
	linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org
Subject: Re: [PATCH] spi: mt7621: allow GPIO chip select lines
Date: Fri, 15 Mar 2024 22:21:53 +0200	[thread overview]
Message-ID: <55c5f2e0723c18384c781e87985f0d22@risingedge.co.za> (raw)
In-Reply-To: <6c92fddd-f79b-40b5-bd52-61f43d6a7591@sirena.org.uk>

On 2024-03-15 19:47, Mark Brown wrote:
> On Fri, Mar 15, 2024 at 06:23:09PM +0200, Justin Swartz wrote:
>> On 2024-03-15 16:45, Mark Brown wrote:
> 
>> > The core should handle GPIO chip selects for you?
> 
>> As far as I can tell, it doesn't - at least as far the state
>> of spi-mt7621.c is concerned prior to the patch, plus kernel
>> configuration choices, device tree definition, and other
>> factors I might not be taking into account.
> 
>> But maybe I'm doing something wrong, or perhaps have a
>> misconfiguration somewhere. So, if you're able to point out
>> something I've done incorrectly, it would be appreciated.
> 
> Look at other drivers that support GPIO chip selects?

Of the 43 drivers (of drivers/spi/*.c) that setup the
spi_controller's use_gpio_descriptors as true:

   39 drivers use the transfer_one hook, and
    4 drivers use the transfer_one_message hook.

Drivers that use the transfer_one hook benefit from the core
taking care of chip selection on their behalf.

Drivers that use the transfer_one_message hook handle chip
selection on their own, within the function they've pointed
the hook at.

There's comment prior to the declaration of the spi_controller
structure (in include/linux/spi/spi.h) that says the following
about the transfer_one* hooks (beginning at line 493):

  *                    Note: transfer_one and transfer_one_message are 
mutually
  *                    exclusive; when both are set, the generic 
subsystem does
  *                    not call your transfer_one callback.

Considering spi-mt7621.c was implemented using the
transfer_one_message() hook, I'd assumed that it made more
sense to take the approach of determining whether a chip
select was native or GPIO, and then calling a function that
is responsible only for the control over a single type of
chip select to ensure that I was not influencing the previous
native chip select logic in any drastic manner.

To me that seems less intrusive, and allows less room for
potential breakage for existing users of this driver (who
are native chip select users only), than the outright
refactoring of the mt7621_spi_transfer_one_message() function
into an mt7621_spi_transfer_one() function instead.

Based on reading (some of) drivers/spi/spi.c and looking at
cflow-generated callgraph of drivers/spi/spi.c, to determine
where spi_set_cs() and any gpiod_* functions are called,
I believe that only the transfer_one() hook approach leads
to SPI core control of the GPIO chip select lines - via the
core's own spi_transfer_one_message().

>> To attempt to confirm if the core will handle my desired
>> GPIO chip select lines without explicit state toggling,
>> I tried to set the value of use_gpio_descriptors to true,
>> without any other modifications to spi-mt7621.c as of
>> commit 90d35da658da8cff0d4ecbb5113f5fac9d00eb72:
> 
> Please include human readable descriptions of things like commits and
> issues being discussed in e-mail in your mails, this makes them much
> easier for humans to read especially when they have no internet access.
> I do frequently catch up on my mail on flights or while otherwise
> travelling so this is even more pressing for me than just being about
> making things a bit easier to read.

I understand that. The unlabelled commit was:

$ git log | grep -A5 90d35da658da8cff0d4ecbb5113f5fac9d00eb72
commit 90d35da658da8cff0d4ecbb5113f5fac9d00eb72
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Sun Mar 3 13:02:52 2024 -0800

     Linux 6.8-rc7

> The core needs to know that the GPIO chip selects are there but once it
> knows that they're there things like setting the chip select should 
> just
> work.

This seems to be true for transfer_one() hookers only.