| Message ID | 20201106150706.29089-1-TheSven73@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 766c6b63aa044e84b045803b40b14754d69a2a1d |
| Headers | show |
| Series | [v1] spi: fix client driver breakages when using GPIO descriptors | expand |
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".
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.
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
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
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.
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
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
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.
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.
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
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 :/
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
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).
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
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.
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.
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.
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;