| Message ID | 20210629162914.23286-1-dan.sneddon@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: atmel: Fix CS and initialization bug | expand |
On Tue, Jun 29, 2021 at 09:29:14AM -0700, Dan Sneddon wrote: > spi_setup won't get programmed into the hardware. This patch makes > sure the cs_activate call is made even with a gpio controlled chip > select. In what way does it do that? I can't tell what the patch is supposed to do. > - enable = (!!(spi->mode & SPI_CS_HIGH) == enable); > > - if (enable) { > + if ((enable && (spi->mode & SPI_CS_HIGH)) > + || (!enable && !(spi->mode & SPI_CS_HIGH))) { This looks especially suspicious.
On 6/29/21 9:47 AM, Mark Brown wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Tue, Jun 29, 2021 at 09:29:14AM -0700, Dan Sneddon wrote: > >> spi_setup won't get programmed into the hardware. This patch makes >> sure the cs_activate call is made even with a gpio controlled chip >> select. > >In what way does it do that? I can't tell what the patch is supposed >to >do. The SPI_MASTER_GPIO_SS flag has to be set so that the set_cs function gets called even when using gpio cs pins. > >> - enable =3D (!!(spi->mode & SPI_CS_HIGH) =3D=3D enable); >> =20 >> - if (enable) { >> + if ((enable && (spi->mode & SPI_CS_HIGH)) >> + || (!enable && !(spi->mode & SPI_CS_HIGH))) { > >This looks especially suspicious. It's due to the fact that the spi core tells set_cs if the cs should be high or low, not active or disabled. This logic is to convert from high/low to active/disabled.
On Tue, Jun 29, 2021 at 05:01:57PM +0000, Dan.Sneddon@microchip.com wrote: > On 6/29/21 9:47 AM, Mark Brown wrote: > >In what way does it do that? I can't tell what the patch is supposed >to > >do. > The SPI_MASTER_GPIO_SS flag has to be set so that the set_cs function > gets called even when using gpio cs pins. This all needs to be clear in the changelog. > >> - enable =3D (!!(spi->mode & SPI_CS_HIGH) =3D=3D enable); > >> =20 > >> - if (enable) { > >> + if ((enable && (spi->mode & SPI_CS_HIGH)) > >> + || (!enable && !(spi->mode & SPI_CS_HIGH))) { > >This looks especially suspicious. > It's due to the fact that the spi core tells set_cs if the cs should be > high or low, not active or disabled. This logic is to convert from > high/low to active/disabled. spi_set_cs() handles SPI_CS_HIGH... this looks like a separate existing driver bug, it should just be ignoring SPI_CS_HIGH if it's providing a set_cs() operation and letting the core implement SPI_CS_HIGH for it. I only checked breifly but it looks like spi-atmel is trying to use the core support for chipselect handling here.
On 6/29/21 10:07 AM, Mark Brown wrote: > On Tue, Jun 29, 2021 at 05:01:57PM +0000, Dan.Sneddon@microchip.com wrote: >> On 6/29/21 9:47 AM, Mark Brown wrote: > >> >In what way does it do that? I can't tell what the patch is supposed >to >> >do. > >> The SPI_MASTER_GPIO_SS flag has to be set so that the set_cs function >> gets called even when using gpio cs pins. > > This all needs to be clear in the changelog. I'll update the commit message. > >> >> - enable =3D (!!(spi->mode & SPI_CS_HIGH) =3D=3D enable); >> >> =20 >> >> - if (enable) { >> >> + if ((enable && (spi->mode & SPI_CS_HIGH)) >> >> + || (!enable && !(spi->mode & SPI_CS_HIGH))) { > >> >This looks especially suspicious. > >> It's due to the fact that the spi core tells set_cs if the cs should be >> high or low, not active or disabled. This logic is to convert from >> high/low to active/disabled. > > spi_set_cs() handles SPI_CS_HIGH... this looks like a separate existing > driver bug, it should just be ignoring SPI_CS_HIGH if it's providing a > set_cs() operation and letting the core implement SPI_CS_HIGH for it. I > only checked breifly but it looks like spi-atmel is trying to use the > core support for chipselect handling here. >
diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c index 2ef74885ffa2..84d902c469cf 100644 --- a/drivers/spi/spi-atmel.c +++ b/drivers/spi/spi-atmel.c @@ -352,8 +352,6 @@ static void cs_activate(struct atmel_spi *as, struct spi_device *spi) } mr = spi_readl(as, MR); - if (spi->cs_gpiod) - gpiod_set_value(spi->cs_gpiod, 1); } else { u32 cpol = (spi->mode & SPI_CPOL) ? SPI_BIT(CPOL) : 0; int i; @@ -369,8 +367,6 @@ static void cs_activate(struct atmel_spi *as, struct spi_device *spi) mr = spi_readl(as, MR); mr = SPI_BFINS(PCS, ~(1 << chip_select), mr); - if (spi->cs_gpiod) - gpiod_set_value(spi->cs_gpiod, 1); spi_writel(as, MR, mr); } @@ -400,8 +396,6 @@ static void cs_deactivate(struct atmel_spi *as, struct spi_device *spi) if (!spi->cs_gpiod) spi_writel(as, CR, SPI_BIT(LASTXFER)); - else - gpiod_set_value(spi->cs_gpiod, 0); } static void atmel_spi_lock(struct atmel_spi *as) __acquires(&as->lock) @@ -1271,9 +1265,9 @@ static void atmel_spi_set_cs(struct spi_device *spi, bool enable) * since we already have routines for activate/deactivate translate * high/low to active/inactive */ - enable = (!!(spi->mode & SPI_CS_HIGH) == enable); - if (enable) { + if ((enable && (spi->mode & SPI_CS_HIGH)) + || (!enable && !(spi->mode & SPI_CS_HIGH))) { cs_activate(as, spi); } else { cs_deactivate(as, spi); @@ -1483,7 +1477,8 @@ static int atmel_spi_probe(struct platform_device *pdev) master->bus_num = pdev->id; master->num_chipselect = 4; master->setup = atmel_spi_setup; - master->flags = (SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX); + master->flags = (SPI_MASTER_MUST_RX | SPI_MASTER_MUST_TX | + SPI_MASTER_GPIO_SS); master->transfer_one = atmel_spi_one_transfer; master->set_cs = atmel_spi_set_cs; master->cleanup = atmel_spi_cleanup;
Commit 5fa5e6dec762 ("spi: atmel: Switch to transfer_one transfer method") switched to using transfer_one and set_cs. However, the driver expects to call cs_activate even with a gpio controlled chip select. If cs_activate isn't called the settings programmed with the call to spi_setup won't get programmed into the hardware. This patch makes sure the cs_activate call is made even with a gpio controlled chip select. Fixes: 5fa5e6dec762 ("spi: atmel: Switch to transfer_one transfer method") Signed-off-by: Dan Sneddon <dan.sneddon@microchip.com> --- drivers/spi/spi-atmel.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)