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

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

[Sven Van Asbroeck]

From: Sven Van Asbroeck <thesven73@gmail.com>

Commit f3186dd87669 ("spi: Optionally use GPIO descriptors for CS GPIOs")
introduced the optional use of GPIO descriptors for chip selects.

A side-effect of this change: when a SPI bus uses GPIO descriptors,
all its client devices have SPI_CS_HIGH set in spi->mode. This flag is
required for the SPI bus to operate correctly.

This unfortunately breaks many client drivers, which use the following
pattern to configure their underlying SPI bus:

static int client_device_probe(struct spi_device *spi)
{
	...
	spi->mode = SPI_MODE_0;
	spi->bits_per_word = 8;
	err = spi_setup(spi);
	..
}

In short, many client drivers overwrite the SPI_CS_HIGH bit in
spi->mode, and break the underlying SPI bus driver.

This is especially true for Freescale/NXP imx ecspi, where large
numbers of spi client drivers now no longer work.

Proposed fix:
-------------
When using gpio descriptors, depend on gpiolib to handle CS polarity.
Existing quirks in gpiolib ensure that this is handled correctly.

Existing gpiolib behaviour will force the polarity of any chip-select
gpiod to active-high (if 'spi-active-high' devicetree prop present) or
active-low (if 'spi-active-high' absent). Irrespective of whether
the gpio is marked GPIO_ACTIVE_[HIGH|LOW] in the devicetree.

Loose ends:
-----------
If this fix is applied:
- is commit 138c9c32f090
  ("spi: spidev: Fix CS polarity if GPIO descriptors are used")
  still necessary / correct ?

Fixes: f3186dd87669 ("spi: Optionally use GPIO descriptors for CS GPIOs")
Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com>
---

Tree: v5.10-rc2

To: Mark Brown <broonie@kernel.org>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: Simon Han <z.han@kunbus.com>
Cc: Lukas Wunner <lukas@wunner.de>
Cc: linux-spi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

 drivers/spi/spi.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 0cab239d8e7f..7566482c052c 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -812,18 +812,16 @@ static void spi_set_cs(struct spi_device *spi, bool enable)
 		enable = !enable;
 
 	if (spi->cs_gpiod || gpio_is_valid(spi->cs_gpio)) {
-		/*
-		 * Honour the SPI_NO_CS flag and invert the enable line, as
-		 * active low is default for SPI. Execution paths that handle
-		 * polarity inversion in gpiolib (such as device tree) will
-		 * enforce active high using the SPI_CS_HIGH resulting in a
-		 * double inversion through the code above.
-		 */
 		if (!(spi->mode & SPI_NO_CS)) {
 			if (spi->cs_gpiod)
+				/* polarity handled by gpiolib */
 				gpiod_set_value_cansleep(spi->cs_gpiod,
-							 !enable);
+							 enable1);
 			else
+				/*
+				 * invert the enable line, as active low is
+				 * default for SPI.
+				 */
 				gpio_set_value_cansleep(spi->cs_gpio, !enable);
 		}
 		/* Some SPI masters need both GPIO CS & slave_select */
@@ -1992,15 +1990,6 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
 	}
 	spi->chip_select = value;
 
-	/*
-	 * For descriptors associated with the device, polarity inversion is
-	 * handled in the gpiolib, so all gpio chip selects are "active high"
-	 * in the logical sense, the gpiolib will invert the line if need be.
-	 */
-	if ((ctlr->use_gpio_descriptors) && ctlr->cs_gpiods &&
-	    ctlr->cs_gpiods[spi->chip_select])
-		spi->mode |= SPI_CS_HIGH;
-
 	/* Device speed */
 	if (!of_property_read_u32(nc, "spi-max-frequency", &value))
 		spi->max_speed_hz = value;
-- 
2.17.1

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

[Andy Shevchenko]

On Fri, Nov 6, 2020 at 5:08 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
>
> From: Sven Van Asbroeck <thesven73@gmail.com>
>
> Commit f3186dd87669 ("spi: Optionally use GPIO descriptors for CS GPIOs")
> introduced the optional use of GPIO descriptors for chip selects.
>
> A side-effect of this change: when a SPI bus uses GPIO descriptors,
> all its client devices have SPI_CS_HIGH set in spi->mode. This flag is
> required for the SPI bus to operate correctly.
>
> This unfortunately breaks many client drivers, which use the following
> pattern to configure their underlying SPI bus:
>
> static int client_device_probe(struct spi_device *spi)
> {
>         ...
>         spi->mode = SPI_MODE_0;
>         spi->bits_per_word = 8;
>         err = spi_setup(spi);
>         ..
> }
>
> In short, many client drivers overwrite the SPI_CS_HIGH bit in
> spi->mode, and break the underlying SPI bus driver.

Sounds like "many SPI drivers have to be fixed".



-- 
With Best Regards,
Andy Shevchenko

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

[Sven Van Asbroeck]

Hi Andy, thank you for looking at this patch !

On Mon, Nov 9, 2020 at 9:24 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> Sounds like "many SPI drivers have to be fixed".
>

I don't disagree. Fact is that after the imx cspi bus driver was converted
to gpio descriptors, most spi client drivers broke. It would be great if this
could be fixed. Any method that the community can find a consensus on,
would be great :)

One the one hand: the fact that many spi client drivers just overwrite
flags and values in their parent bus structure, doesn't sound idiomatic.
I guess those spi->... values should really be opaque, and we should
be using accessor functions, eg.:

    static int acme_probe(struct spi_device *spi)
    {
        ...
        // won't touch SPI_CS_HIGH flag
        spi_set_mode_clock(spi, SPI_MODE_0);
        ...
    }

On the other hand, it sounds very confusing to set SPI_CS_HIGH on
all spi buses that use gpio descriptors: especially because gpiolib
already handles absolutely everything related to polarity. And the
SPI_CS_HIGH flag gets set even for chip-selects that are active-low.

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

[Linus Walleij]

On Mon, Nov 9, 2020 at 3:41 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
> On Mon, Nov 9, 2020 at 9:24 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > Sounds like "many SPI drivers have to be fixed".
>
> I don't disagree. Fact is that after the imx cspi bus driver was converted
> to gpio descriptors, most spi client drivers broke. It would be great if this
> could be fixed. Any method that the community can find a consensus on,
> would be great :)

I think your patch is the quick fix.

I would say that anything that has:

spi->mode = ...

is essentially broken.

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.

> One the one hand: the fact that many spi client drivers just overwrite
> flags and values in their parent bus structure, doesn't sound idiomatic.
> I guess those spi->... values should really be opaque, and we should
> be using accessor functions, eg.:
>
>     static int acme_probe(struct spi_device *spi)
>     {
>         ...
>         // won't touch SPI_CS_HIGH flag
>         spi_set_mode_clock(spi, SPI_MODE_0);
>         ...
>     }

I would just make sure to affect the flags that matters to my driver,
it's just bits.

spi->mode &= ~FOO;
spi->mode |= BAR;

> On the other hand, it sounds very confusing to set SPI_CS_HIGH on
> all spi buses that use gpio descriptors: especially because gpiolib
> already handles absolutely everything related to polarity.

As long as gpiolib gets a 1 for asserted and a 0 for deasserted
it will be happy.

I'm not against your patch, it makes the codepath cleaner
so in a way it is good.

Yours,
Linus Walleij

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2008 bytes --]

On Wed, Nov 11, 2020 at 02:05:19AM +0100, Linus Walleij wrote:
> On Mon, Nov 9, 2020 at 3:41 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:

> > I don't disagree. Fact is that after the imx cspi bus driver was converted
> > to gpio descriptors, most spi client drivers broke. It would be great if this
> > could be fixed. Any method that the community can find a consensus on,
> > would be great :)

> I think your patch is the quick fix.

> 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.  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.

> 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)
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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Linus Walleij]

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 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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1930 bytes --]

On Wed, Nov 11, 2020 at 02:36:07PM +0100, 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:

> > 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.

That's from PowerPC days, not ARM, and frankly a lot of DT conversions
were just fairly mechanical conversions of what was in platform data to
DT so they may not have been especially considered.

> 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've been pushing this since forever as well, as far as I remember it's
been a thing since we started doing this.

> 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.

I think the main push in the other direction has always been people who
want to not have to write a driver at all and put absolutely everything
into DT which has scaling issues :/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Linus Walleij]

On Mon, Nov 16, 2020 at 10:06 PM Mark Brown <broonie@kernel.org> wrote:
> On Wed, Nov 11, 2020 at 02:36:07PM +0100, Linus Walleij wrote:

> > 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.
>
> I think the main push in the other direction has always been people who
> want to not have to write a driver at all and put absolutely everything
> into DT which has scaling issues :/

What I can't understand is what gave them that idea.

This thing looks like a dream to these people for example:
https://gist.github.com/Minecrell/56c2b20118ba00a9723f0785301bc5ec#file-dsi_panel_s6e88a0_ams452ef01_qhd_octa_video-dtsi
And it looks like a nightmare to me.

(There is even a tool to convert this description into a proper display
driver now.)

It just seems to be one of those golden hammer things: everything
start to look like nails.

Yours,
Linus Walleij

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1036 bytes --]

On Wed, Nov 18, 2020 at 02:03:41AM +0100, Linus Walleij wrote:
> On Mon, Nov 16, 2020 at 10:06 PM Mark Brown <broonie@kernel.org> wrote:

> > I think the main push in the other direction has always been people who
> > want to not have to write a driver at all and put absolutely everything
> > into DT which has scaling issues :/

> What I can't understand is what gave them that idea.

> This thing looks like a dream to these people for example:
> https://gist.github.com/Minecrell/56c2b20118ba00a9723f0785301bc5ec#file-dsi_panel_s6e88a0_ams452ef01_qhd_octa_video-dtsi
> And it looks like a nightmare to me.

> (There is even a tool to convert this description into a proper display
> driver now.)

> It just seems to be one of those golden hammer things: everything
> start to look like nails.

What people think they were sold was the idea that they shouldn't have
to write driver code or upstream things, something with more AML like
capabilities (not realising that AML works partly because ACPI hugely
constrains system design).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Linus Walleij]

On Wed, Nov 18, 2020 at 12:41 PM Mark Brown <broonie@kernel.org> wrote:

> What people think they were sold was the idea that they shouldn't have
> to write driver code or upstream things, something with more AML like
> capabilities (not realising that AML works partly because ACPI hugely
> constrains system design).

This makes a lot of sense.

I suppose what we need to think about is the bigger question of why
people/companies/managers are so worried about working upstream
that they will go to lengths to avoid it and jump at any chance of
raising a wall of abstraction between their internal development and
the in-kernel software development.

I think of this as vendor/community couples therapy or something,
there is some form of deep disconnect or mistrust going on at times
and having worked on both ends myself I would think I could
understand it but I can't.

Yours,
Linus Walleij

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1160 bytes --]

On Tue, Nov 24, 2020 at 04:21:48PM +0100, Linus Walleij wrote:

> > What people think they were sold was the idea that they shouldn't have
> > to write driver code or upstream things, something with more AML like
> > capabilities (not realising that AML works partly because ACPI hugely
> > constrains system design).

> This makes a lot of sense.

> I suppose what we need to think about is the bigger question of why
> people/companies/managers are so worried about working upstream
> that they will go to lengths to avoid it and jump at any chance of
> raising a wall of abstraction between their internal development and
> the in-kernel software development.

> I think of this as vendor/community couples therapy or something,
> there is some form of deep disconnect or mistrust going on at times
> and having worked on both ends myself I would think I could
> understand it but I can't.

In this case I think this is partly due to the way people were sold on
the DT conversion - part of the sales pitch was that you'd not need to
get board support upstream, which is a useful thing if you want to run
things like LTS or distro kernels on newer hardware.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Grant Likely]

On 18/11/2020 11:40, Mark Brown wrote:
> On Wed, Nov 18, 2020 at 02:03:41AM +0100, Linus Walleij wrote:
>> On Mon, Nov 16, 2020 at 10:06 PM Mark Brown <broonie@kernel.org> wrote:
> 
>>> I think the main push in the other direction has always been people who
>>> want to not have to write a driver at all and put absolutely everything
>>> into DT which has scaling issues :/
> 
>> What I can't understand is what gave them that idea.
> 
>> This thing looks like a dream to these people for example:
>> https://gist.github.com/Minecrell/56c2b20118ba00a9723f0785301bc5ec#file-dsi_panel_s6e88a0_ams452ef01_qhd_octa_video-dtsi
>> And it looks like a nightmare to me.
> 
>> (There is even a tool to convert this description into a proper display
>> driver now.)
> 
>> It just seems to be one of those golden hammer things: everything
>> start to look like nails.
> 
> What people think they were sold was the idea that they shouldn't have
> to write driver code or upstream things, something with more AML like
> capabilities (not realising that AML works partly because ACPI hugely
> constrains system design).

And is also untrue. AML only provides an API abstraction for a specific 
power management model. All the actual driving of the device still 
requires driver code and requires reading devices-specific properties 
out of the ACPI node.

g.

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

[Grant Likely]

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.

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

[Linus Walleij]

On Fri, Nov 6, 2020 at 4:07 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:

> From: Sven Van Asbroeck <thesven73@gmail.com>
>
> Commit f3186dd87669 ("spi: Optionally use GPIO descriptors for CS GPIOs")
> introduced the optional use of GPIO descriptors for chip selects.
>
> A side-effect of this change: when a SPI bus uses GPIO descriptors,
> all its client devices have SPI_CS_HIGH set in spi->mode. This flag is
> required for the SPI bus to operate correctly.
>
> This unfortunately breaks many client drivers, which use the following
> pattern to configure their underlying SPI bus:
>
> static int client_device_probe(struct spi_device *spi)
> {
>         ...
>         spi->mode = SPI_MODE_0;
>         spi->bits_per_word = 8;
>         err = spi_setup(spi);

I feel torn about it, there are so many weird corners of semantics
in this code. The patch makes the code easier to understand
too.

If it provedly fixes more than it breaks:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

[Mark Brown]

On Fri, 6 Nov 2020 10:07:06 -0500, Sven Van Asbroeck wrote:
> Commit f3186dd87669 ("spi: Optionally use GPIO descriptors for CS GPIOs")
> introduced the optional use of GPIO descriptors for chip selects.
> 
> A side-effect of this change: when a SPI bus uses GPIO descriptors,
> all its client devices have SPI_CS_HIGH set in spi->mode. This flag is
> required for the SPI bus to operate correctly.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: fix client driver breakages when using GPIO descriptors
      commit: 766c6b63aa044e84b045803b40b14754d69a2a1d

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

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

[Sven Van Asbroeck]

On Wed, Nov 11, 2020 at 10:48 AM Mark Brown <broonie@kernel.org> wrote:
>
> Applied to
>
>    https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thank you !

Now that our minds are still focused on this subject, should
commit 138c9c32f090 ("spi: spidev: Fix CS polarity if GPIO descriptors
are used")
be reverted?

This fixed spidev to deal with SPI_CS_HIGH on gpiod.
But after our fix, its behaviour will probably be broken again.

Another candidate for revert is
commit ada9e3fcc175 ("spi: dw: Correct handling of native chipselect")
although I don't understand that code well enough to be sure.

Adding Charles Keepax.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 367 bytes --]

On Wed, Nov 11, 2020 at 11:24:14AM -0500, Sven Van Asbroeck wrote:

> Now that our minds are still focused on this subject, should
> commit 138c9c32f090 ("spi: spidev: Fix CS polarity if GPIO descriptors
> are used")
> be reverted?

If you think changes should be made to the code please propose patches
making them - reverts are just normal patches with changelogs.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Charles Keepax]

On Wed, Nov 11, 2020 at 11:24:14AM -0500, Sven Van Asbroeck wrote:
> On Wed, Nov 11, 2020 at 10:48 AM Mark Brown <broonie@kernel.org> wrote:
> >
> > Applied to
> >
> >    https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next
> 
> Thank you !
> 
> Now that our minds are still focused on this subject, should
> commit 138c9c32f090 ("spi: spidev: Fix CS polarity if GPIO descriptors
> are used")
> be reverted?
> 
> This fixed spidev to deal with SPI_CS_HIGH on gpiod.
> But after our fix, its behaviour will probably be broken again.
> 
> Another candidate for revert is
> commit ada9e3fcc175 ("spi: dw: Correct handling of native chipselect")
> although I don't understand that code well enough to be sure.
> 
> Adding Charles Keepax.

Looks like the code has changed a fair amount since my patch. The
important detail from it was trying to clarify the semantics of the
controller->set_cs callback. That function is called with a boolean
argument and that argument could have two possible meanings:

1) True means apply a high logic level to the chip select line.
2) True mean apply chip select.

Under interpretation 2) the chip select line would be set to a
different logic level depending on if the device is active high or
active low.

If I remember correctly at the point of my patch the core had just
changed between the two a couple of times but now consistently did 1)
(and looks like it still does), my patch intended to updated the
spi-dw driver to match that, as my SPI had stopped working. I think
it then turned out, my patch broke some other use-cases and that
the bit in the IP basically had 2) semantics in hardware. Which is
what this patch fixed:

commit 9aea644ca17b ("spi: dw: Fix native CS being unset")

After that patch my patch is mostly replaced so I don't think it
would make any sense to revert my patch at this point, and I
don't think your patch will break the spi-dw driver. I don't
have easy access to the hardware right now to test, but I will
give it is quick run when that option becomes available to me
again.

Your fix looks good to me, but I suspect you do need to fix the
spidev stuff although I have haven't looked at that in detail.

Thanks,
Charles