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

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

[Sven Van Asbroeck]

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

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

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

[Andy Shevchenko]

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;

?

-- 
With Best Regards,
Andy Shevchenko

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

[Sven Van Asbroeck]

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/

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

[Sven Van Asbroeck]

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

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

[Andrew Lunn]

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

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

[Sven Van Asbroeck]

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