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 <[email protected]>
---
drivers/spi/spi-atmel.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
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;
--
2.17.1
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 6/29/21 10:07 AM, Mark Brown wrote:
> On Tue, Jun 29, 2021 at 05:01:57PM +0000, [email protected] 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.
>
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 Tue, Jun 29, 2021 at 05:01:57PM +0000, [email protected] 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.