diff mbox series

[net,v2] net: phy: spi_ks8995: Do not overwrite SPI mode flags

Message ID 20201110142032.24071-1-TheSven73@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net,v2] net: phy: spi_ks8995: Do not overwrite SPI mode flags | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net
netdev/subject_prefix success Link
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 10 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Sven Van Asbroeck Nov. 10, 2020, 2:20 p.m. UTC 
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. Which 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.

Fixes: a8e510f682fe ("phy: Micrel KS8995MA 5-ports 10/100 managed Ethernet switch support added")
Fixes: f3186dd87669 ("spi: Optionally use GPIO descriptors for CS GPIOs")
Link: https://patchwork.kernel.org/project/spi-devel-general/patch/20201106150706.29089-1-TheSven73@gmail.com/#23747737
Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com>
---

Tree: git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git # 989ef49bdf10

To be followed by a proposed spi helper function. Submit to net-next after net
gets merged into net-next.

# large number of people, from get_maintainer.pl
To: Andrew Lunn <andrew@lunn.ch>
To: Heiner Kallweit <hkallweit1@gmail.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Russell King <linux@armlinux.org.uk>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Cc: Mark Brown <broonie@kernel.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Frederic LAMBERT <frdrc66@gmail.com>
Cc: Gabor Juhos <juhosg@openwrt.org>
Cc: Jonathan Cameron <jonathan.cameron@huawei.com>

# Cc SPI group, suggested by Jakub Kicinski
Cc: linux-spi@vger.kernel.org

 drivers/net/phy/spi_ks8995.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Andy Shevchenko Nov. 10, 2020, 4:31 p.m. UTC | #1 
On Tue, Nov 10, 2020 at 4:20 PM Sven Van Asbroeck <thesven73@gmail.com> 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. Which 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.

I see that this is a fix for backporing, but maybe you can send a
patches on top of this to:
  1) introduce
 #define SPI_MODE_MASK  (SPI_CPHA | SPI_CPOL)

> +       /* use SPI_MODE_0 without changing any other mode flags */
> +       spi->mode &= ~(SPI_CPHA | SPI_CPOL);

2)
       spi->mode &= ~SPI_MODE_MASK;

> +       spi->mode |= SPI_MODE_0;

?
Sven Van Asbroeck Nov. 10, 2020, 4:50 p.m. UTC | #2 
Hi Andy, thank you for the feedback.

On Tue, Nov 10, 2020 at 11:30 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> I see that this is a fix for backporing, but maybe you can send a
> patches on top of this to:
>   1) introduce
>  #define SPI_MODE_MASK  (SPI_CPHA | SPI_CPOL)
>        spi->mode &= ~SPI_MODE_MASK;
>

Andrew Lunn suggested that a spi helper function would
probably fit the bill. I am planning to submit that to net-next
after this patch is accepted in next (and next is merged into
net-next).

I am learning that net is only for the most minimal of fixes.

See the previous discussion here:
https://patchwork.ozlabs.org/project/netdev/patch/20201109193117.2017-1-TheSven73@gmail.com/
Sven Van Asbroeck Nov. 10, 2020, 5:06 p.m. UTC | #3 
PING Jakub

On Tue, Nov 10, 2020 at 11:30 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> I see that this is a fix for backporing, but maybe you can send a
> patches on top of this to:
>   1) introduce
>  #define SPI_MODE_MASK  (SPI_CPHA | SPI_CPOL)
>        spi->mode &= ~SPI_MODE_MASK;
> > +       spi->mode |= SPI_MODE_0;
>
Jakub,

Is it possible to merge Andy's suggestion into net?
Or should this go into net-next?

Thank you,
Sven
Andrew Lunn Nov. 10, 2020, 10:53 p.m. UTC | #4 
On Tue, Nov 10, 2020 at 12:06:37PM -0500, Sven Van Asbroeck wrote:
> PING Jakub
> 
> On Tue, Nov 10, 2020 at 11:30 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > I see that this is a fix for backporing, but maybe you can send a
> > patches on top of this to:
> >   1) introduce
> >  #define SPI_MODE_MASK  (SPI_CPHA | SPI_CPOL)
> >        spi->mode &= ~SPI_MODE_MASK;
> > > +       spi->mode |= SPI_MODE_0;
> >
> Jakub,
> 
> Is it possible to merge Andy's suggestion into net?
> Or should this go into net-next?

I would keep with the minimal fix for the moment, it keeps the
dependencies simple.

When you add a helper, it should really be somewhere in the SPI code,
not the net code. So we need both the SPI and the net maintainers to
cooperate to get the helper merged, and then this driver using the
helper.

	Andrew
Sven Van Asbroeck Nov. 11, 2020, 4:03 p.m. UTC | #5 
A spi core fix has been accepted which makes this patch unnecessary.

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git/commit/?id=766c6b63aa044e84b045803b40b14754d69a2a1d

On Tue, Nov 10, 2020 at 9:20 AM Sven Van Asbroeck <thesven73@gmail.com> wrote:
>
>
> This driver makes sure the underlying SPI bus is set to "mode 0"
> by assigning SPI_MODE_0 to spi->mode. Which 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.
>
> Fixes: a8e510f682fe ("phy: Micrel KS8995MA 5-ports 10/100 managed Ethernet switch support added")
> Fixes: f3186dd87669 ("spi: Optionally use GPIO descriptors for CS GPIOs")
> Link: https://patchwork.kernel.org/project/spi-devel-general/patch/20201106150706.29089-1-TheSven73@gmail.com/#23747737
> Signed-off-by: Sven Van Asbroeck <thesven73@gmail.com>
> ---
diff mbox series

Patch

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