Message ID | 20201109193117.2017-1-TheSven73@gmail.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | [net-next,v1] net: phy: spi_ks8995: Do not overwrite SPI mode flags | expand |
Context | Check | Description |
---|---|---|
jkicinski/cover_letter | success | Link |
jkicinski/fixes_present | success | Link |
jkicinski/patch_count | success | Link |
jkicinski/tree_selection | success | Clearly marked for net-next |
jkicinski/subject_prefix | success | Link |
jkicinski/source_inline | success | Was 0 now: 0 |
jkicinski/verify_signedoff | success | Link |
jkicinski/module_param | success | Was 0 now: 0 |
jkicinski/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
jkicinski/verify_fixes | success | Link |
jkicinski/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 10 lines checked |
jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
jkicinski/header_inline | success | Link |
jkicinski/stable | success | Stable not CCed |
> +++ b/drivers/net/phy/spi_ks8995.c > @@ -491,7 +491,9 @@ static int ks8995_probe(struct spi_device *spi) > > spi_set_drvdata(spi, ks); > > - spi->mode = SPI_MODE_0; > + /* use SPI_MODE_0 without changing any other mode flags */ > + spi->mode &= ~(SPI_CPHA | SPI_CPOL); > + spi->mode |= SPI_MODE_0; > spi->bits_per_word = 8; Did you check to see if there is a help to set just the mode without changing any of the other bits? Andrew
On Mon, Nov 9, 2020 at 2:49 PM Andrew Lunn <andrew@lunn.ch> wrote: > > Did you check to see if there is a help to set just the mode without > changing any of the other bits? Absolutely, but it doesn't exist, AFAIK. It would be great if client spi drivers would use a helper function like that. The spi bus driver on my platform (imx ecspi) was recently changed upstream, which means SPI_CS_HIGH is now always set, irrespective of the actual chip select polarity. This change breaks every single spi driver which "plows over" the spi->mode flags. And there are quite a few... Sven
On Mon, Nov 09, 2020 at 02:56:38PM -0500, Sven Van Asbroeck wrote: > On Mon, Nov 9, 2020 at 2:49 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > Did you check to see if there is a help to set just the mode without > > changing any of the other bits? > > Absolutely, but it doesn't exist, AFAIK. > It would be great if client spi drivers would use a helper function like > that. Then you should consider adding it, and cross post the SPI list. Andrew
On Mon, Nov 9, 2020 at 3:26 PM Andrew Lunn <andrew@lunn.ch> wrote: > > Then you should consider adding it, and cross post the SPI list. > Good idea. I will give that a try.
On Mon, 9 Nov 2020 14:31:17 -0500 Sven Van Asbroeck wrote: > From: Sven Van Asbroeck <thesven73@gmail.com> > > This driver makes sure the underlying SPI bus is set to "mode 0" > by assigning SPI_MODE_0 to spi->mode. This overwrites all other > SPI mode flags. > > In some circumstances, this can break the underlying SPI bus driver. > For example, if SPI_CS_HIGH is set on the SPI bus, the driver > will clear that flag, which results in a chip-select polarity issue. > > Fix by changing only the SPI_MODE_N bits, i.e. SPI_CPHA and SPI_CPOL. > > Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com> This is a fix right? You seem to be targeting net-next and there is no Fixes tag but it sounds like a bug.
On Mon, Nov 9, 2020 at 4:09 PM Jakub Kicinski <kuba@kernel.org> wrote: > > This is a fix right? You seem to be targeting net-next and there is no > Fixes tag but it sounds like a bug. I'm not sure. The original code used to work for me, until the spi bus driver I'm using to communicate to this chip was changed to always require SPI_CS_HIGH. The current ks8995 driver will now plow over this flag, and spi communication breaks. Is this a bug? If so, what should its Fixes commit be? The spi commit upstream that enables SPI_CS_HIGH on my platform?
On Mon, 9 Nov 2020 16:20:46 -0500 Sven Van Asbroeck wrote: > Is this a bug? If so, what should its Fixes commit be? The spi commit > upstream that enables SPI_CS_HIGH on my platform? I'd put two fixes tags one for the spi commit and one for the commit which introduced the assignment in the client driver.
On Mon, Nov 9, 2020 at 4:24 PM Jakub Kicinski <kuba@kernel.org> wrote: > > the commit > which introduced the assignment in the client driver. That's the commit which adds the initial driver to the tree, back in 2011. Should I use that for Fixes?
On Mon, 9 Nov 2020 16:29:19 -0500 Sven Van Asbroeck wrote: > On Mon, Nov 9, 2020 at 4:24 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > the commit > > which introduced the assignment in the client driver. > > That's the commit which adds the initial driver to the tree, back in 2011. > Should I use that for Fixes? Yup
On Mon, Nov 9, 2020 at 5:04 PM Jakub Kicinski <kuba@kernel.org> wrote: > > Yup Just a minute. Earlier in the thread, Andrew Lunn is suggesting I create a new spi helper function, and cross-post to the spi group(s). That doesn't sound like a minimal fix, should it go to net or net-next? Thanks, Sven
On Mon, 9 Nov 2020 17:19:48 -0500 Sven Van Asbroeck wrote: > On Mon, Nov 9, 2020 at 5:04 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > Yup > > Just a minute. Earlier in the thread, Andrew Lunn is suggesting I > create a new spi helper function, and cross-post to the spi group(s). > > That doesn't sound like a minimal fix, should it go to net or net-next? Is it only broken for you in linux-next or just in the current 5.10 release?
On Mon, Nov 9, 2020 at 5:23 PM Jakub Kicinski <kuba@kernel.org> wrote: > > Is it only broken for you in linux-next or just in the current 5.10 > release? It's broken for me in the current 5.10 release. That means it should go to net, not net-next, correct?
On Mon, 9 Nov 2020 17:27:28 -0500 Sven Van Asbroeck wrote: > On Mon, Nov 9, 2020 at 5:23 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > Is it only broken for you in linux-next or just in the current 5.10 > > release? > > It's broken for me in the current 5.10 release. That means it should > go to net, not net-next, correct? Yes, most certainly. Especially with 5.10 being LTS. You can send a minimal fix (perhaps what you got already?), and follow up with the helper as suggested by Andrew after ~a week when net is merged into net-next. But please at least repost for net and CC Mark and the SPI list for input.
On Mon, Nov 9, 2020 at 5:36 PM Jakub Kicinski <kuba@kernel.org> wrote: > > Yes, most certainly. Especially with 5.10 being LTS. > > You can send a minimal fix (perhaps what you got already?), and follow > up with the helper as suggested by Andrew after ~a week when net is > merged into net-next. > What I already posted (as v1) should be the minimal fix. Can we proceed on that basis? I'll follow up with the helper after the net -> net-next merge, as you suggested.
On Mon, 9 Nov 2020 17:39:22 -0500 Sven Van Asbroeck wrote: > What I already posted (as v1) should be the minimal fix. > Can we proceed on that basis? I'll follow up with the helper > after the net -> net-next merge, as you suggested. Well, you cut off the relevant part of my email where I said: But please at least repost for net and CC Mark and the SPI list for input. Maybe Mark has a different idea on how client drivers should behave. Also please obviously CC the author of the patch who introduced the breakage.
On Mon, Nov 9, 2020 at 5:50 PM Jakub Kicinski <kuba@kernel.org> wrote: > > But please at least repost for net and CC Mark and the SPI list > for input. > > Maybe Mark has a different idea on how client drivers should behave. > > Also please obviously CC the author of the patch who introduced > the breakage. I believe they're aware: I tried to propose a patch to fix this in the spi core, but it looks like it was rejected - with feedback: "fix the client drivers instead" https://patchwork.kernel.org/project/spi-devel-general/patch/20201106150706.29089-1-TheSven73@gmail.com/#23747737 I will respin this minimal fix as v2, add Fixes tags and Cc the people involved, as you suggested.
diff --git a/drivers/net/phy/spi_ks8995.c b/drivers/net/phy/spi_ks8995.c index 4b198399bfa2..3c6c87a09b03 100644 --- a/drivers/net/phy/spi_ks8995.c +++ b/drivers/net/phy/spi_ks8995.c @@ -491,7 +491,9 @@ static int ks8995_probe(struct spi_device *spi) spi_set_drvdata(spi, ks); - spi->mode = SPI_MODE_0; + /* use SPI_MODE_0 without changing any other mode flags */ + spi->mode &= ~(SPI_CPHA | SPI_CPOL); + spi->mode |= SPI_MODE_0; spi->bits_per_word = 8; err = spi_setup(spi); if (err) {