| Message ID | 20201109193117.2017-1-TheSven73@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next,v1] net: phy: spi_ks8995: Do not overwrite SPI mode flags | expand |
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, 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, 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, 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, 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) {