From: Sven Van Asbroeck <[email protected]>
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/[email protected]/#23747737
Signed-off-by: Sven Van Asbroeck <[email protected]>
---
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 <[email protected]>
To: Heiner Kallweit <[email protected]>
To: Jakub Kicinski <[email protected]>
Cc: Russell King <[email protected]>
Cc: "David S. Miller" <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: Mark Brown <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Frederic LAMBERT <[email protected]>
Cc: Gabor Juhos <[email protected]>
Cc: Jonathan Cameron <[email protected]>
# Cc SPI group, suggested by Jakub Kicinski
Cc: [email protected]
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
On Tue, Nov 10, 2020 at 4:20 PM Sven Van Asbroeck <[email protected]> wrote:
>
> From: Sven Van Asbroeck <[email protected]>
>
> 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
Hi Andy, thank you for the feedback.
On Tue, Nov 10, 2020 at 11:30 AM Andy Shevchenko
<[email protected]> 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/[email protected]/
PING Jakub
On Tue, Nov 10, 2020 at 11:30 AM Andy Shevchenko
<[email protected]> 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
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
> <[email protected]> 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
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 <[email protected]> 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/[email protected]/#23747737
> Signed-off-by: Sven Van Asbroeck <[email protected]>
> ---