| Message ID | 1452528102-26458-1-git-send-email-mans@mansr.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 11, 2016 at 04:01:42PM +0000, Mans Rullgard wrote: > The driver currently chooses between internal chip-select or gpio > based on the existence of the cs-gpios DT property which fails on > non-DT systems and also enforces the same choice for all devices. Normally you shouldn't send incremental patches against patches that have already been applied. However in this case I'm going to drop your original patch anyway due to the issue that was found so it's OK.
Mark Brown <broonie@kernel.org> writes: > On Mon, Jan 11, 2016 at 04:01:42PM +0000, Mans Rullgard wrote: >> The driver currently chooses between internal chip-select or gpio >> based on the existence of the cs-gpios DT property which fails on >> non-DT systems and also enforces the same choice for all devices. > > Normally you shouldn't send incremental patches against patches that > have already been applied. Should or shouldn't? > However in this case I'm going to drop your original patch anyway due > to the issue that was found so it's OK. Sorry, I hadn't noticed it was already applied. Too many trees to keep track of.
On Mon, Jan 11, 2016 at 04:16:31PM +0000, Måns Rullgård wrote: > Mark Brown <broonie@kernel.org> writes: > > Normally you shouldn't send incremental patches against patches that > > have already been applied. > Should or shouldn't? Should, sorry.
Hi Mans, I've tried to apply your patch on a next-20160112 branch to test it but it failed. I've also looked at the for-next branch of the SPI sub-system git tree. It seems that one issue occurred within a chunk patching the atmel_spi_setup() function. Please see below. Le 11/01/2016 17:01, Mans Rullgard a écrit : [...] > diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c > index 08cbb3e43c76..d4a806e24060 100644 > --- a/drivers/spi/spi-atmel.c > +++ b/drivers/spi/spi-atmel.c [...] > @@ -1603,6 +1611,7 @@ static int atmel_spi_setup(struct spi_device *spi) > } > > asd->npcs_pin = npcs_pin; > + asd->use_cs_gpio = use_cs_gpio; > spi->controller_state = asd; > } else { > atmel_spi_lock(as); [...] There is no 'else' statement in the source code I'm looking at. Best regards, Cyrille
Cyrille Pitchen <cyrille.pitchen@atmel.com> writes: > Hi Mans, > > I've tried to apply your patch on a next-20160112 branch to test it but it > failed. I've also looked at the for-next branch of the SPI sub-system git > tree. > > It seems that one issue occurred within a chunk patching the atmel_spi_setup() > function. Please see below. > > Le 11/01/2016 17:01, Mans Rullgard a écrit : > [...] >> diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c >> index 08cbb3e43c76..d4a806e24060 100644 >> --- a/drivers/spi/spi-atmel.c >> +++ b/drivers/spi/spi-atmel.c > [...] >> @@ -1603,6 +1611,7 @@ static int atmel_spi_setup(struct spi_device *spi) >> } >> >> asd->npcs_pin = npcs_pin; >> + asd->use_cs_gpio = use_cs_gpio; >> spi->controller_state = asd; >> } else { >> atmel_spi_lock(as); > [...] > > There is no 'else' statement in the source code I'm looking at. Sorry, I'll send a new patch against the correct base. I forgot I had something else earlier in my local branch.
Le 11/01/2016 17:01, Mans Rullgard a écrit : > The driver currently chooses between internal chip-select or gpio > based on the existence of the cs-gpios DT property which fails on > non-DT systems and also enforces the same choice for all devices. > > This patch makes the method per device instead of per controller > and fixes the selection on non-DT systems. Sorry Mans, but it's still a NACK for me on this "mixed CS feature" (Cf. my answer to the previous version). Bye, > With these changes, > the chip-select method for each device is chosen according to the > following priority: > > 1. GPIO from device tree > 2. GPIO from platform data > 3. Internal chip-select > > Tested on AVR32 ATSTK1000. > > Signed-off-by: Mans Rullgard <mans@mansr.com> > --- > Changes: > - allow internal CS only hardware v2 > - retain seting of master->num_chipselect to 4 on hardware v2 if > cs-gpios property is missing > --- > drivers/spi/spi-atmel.c | 38 +++++++++++++++++++++++--------------- > 1 file changed, 23 insertions(+), 15 deletions(-) > > diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c > index 08cbb3e43c76..d4a806e24060 100644 > --- a/drivers/spi/spi-atmel.c > +++ b/drivers/spi/spi-atmel.c > @@ -312,7 +312,6 @@ struct atmel_spi { > > bool use_dma; > bool use_pdc; > - bool use_cs_gpios; > /* dmaengine data */ > struct atmel_spi_dma dma; > > @@ -323,6 +322,7 @@ struct atmel_spi { > struct atmel_spi_device { > unsigned int npcs_pin; > u32 csr; > + bool use_cs_gpio; > }; > > #define BUFFER_SIZE PAGE_SIZE > @@ -387,7 +387,7 @@ static void cs_activate(struct atmel_spi *as, struct spi_device *spi) > } > > mr = spi_readl(as, MR); > - if (as->use_cs_gpios) > + if (asd->use_cs_gpio) > gpio_set_value(asd->npcs_pin, active); > } else { > u32 cpol = (spi->mode & SPI_CPOL) ? SPI_BIT(CPOL) : 0; > @@ -404,7 +404,7 @@ static void cs_activate(struct atmel_spi *as, struct spi_device *spi) > > mr = spi_readl(as, MR); > mr = SPI_BFINS(PCS, ~(1 << spi->chip_select), mr); > - if (as->use_cs_gpios && spi->chip_select != 0) > + if (asd->use_cs_gpio && spi->chip_select != 0) > gpio_set_value(asd->npcs_pin, active); > spi_writel(as, MR, mr); > } > @@ -433,7 +433,7 @@ static void cs_deactivate(struct atmel_spi *as, struct spi_device *spi) > asd->npcs_pin, active ? " (low)" : "", > mr); > > - if (!as->use_cs_gpios) > + if (!asd->use_cs_gpio) > spi_writel(as, CR, SPI_BIT(LASTXFER)); > else if (atmel_spi_is_v2(as) || spi->chip_select != 0) > gpio_set_value(asd->npcs_pin, !active); > @@ -1539,6 +1539,7 @@ static int atmel_spi_setup(struct spi_device *spi) > unsigned int bits = spi->bits_per_word; > unsigned int npcs_pin; > int ret; > + bool use_cs_gpio; > > as = spi_master_get_devdata(spi->master); > > @@ -1565,8 +1566,6 @@ static int atmel_spi_setup(struct spi_device *spi) > csr |= SPI_BIT(CPOL); > if (!(spi->mode & SPI_CPHA)) > csr |= SPI_BIT(NCPHA); > - if (!as->use_cs_gpios) > - csr |= SPI_BIT(CSAAT); > > /* DLYBS is mostly irrelevant since we manage chipselect using GPIOs. > * > @@ -1577,13 +1576,22 @@ static int atmel_spi_setup(struct spi_device *spi) > csr |= SPI_BF(DLYBS, 0); > csr |= SPI_BF(DLYBCT, 0); > > - /* chipselect must have been muxed as GPIO (e.g. in board setup) */ > npcs_pin = (unsigned long)spi->controller_data; > > - if (!as->use_cs_gpios) > - npcs_pin = spi->chip_select; > - else if (gpio_is_valid(spi->cs_gpio)) > + if (gpio_is_valid(spi->cs_gpio)) { > + /* GPIO from DT */ > npcs_pin = spi->cs_gpio; > + use_cs_gpio = true; > + } else if (npcs_pin && gpio_is_valid(npcs_pin)) { > + /* GPIO from platform data */ > + use_cs_gpio = true; > + } else if (atmel_spi_is_v2(as)) { > + /* internal chip-select */ > + npcs_pin = spi->chip_select; > + use_cs_gpio = false; > + } else { > + return -EINVAL; > + } > > asd = spi->controller_state; > if (!asd) { > @@ -1591,7 +1599,7 @@ static int atmel_spi_setup(struct spi_device *spi) > if (!asd) > return -ENOMEM; > > - if (as->use_cs_gpios) { > + if (use_cs_gpio) { > ret = gpio_request(npcs_pin, dev_name(&spi->dev)); > if (ret) { > kfree(asd); > @@ -1603,6 +1611,7 @@ static int atmel_spi_setup(struct spi_device *spi) > } > > asd->npcs_pin = npcs_pin; > + asd->use_cs_gpio = use_cs_gpio; > spi->controller_state = asd; > } else { > atmel_spi_lock(as); > @@ -1612,6 +1621,8 @@ static int atmel_spi_setup(struct spi_device *spi) > atmel_spi_unlock(as); > } > > + if (!asd->use_cs_gpio) > + csr |= SPI_BIT(CSAAT); > asd->csr = csr; > > dev_dbg(&spi->dev, > @@ -1807,12 +1818,9 @@ static int atmel_spi_probe(struct platform_device *pdev) > > atmel_get_caps(as); > > - as->use_cs_gpios = true; > if (atmel_spi_is_v2(as) && > - !of_get_property(pdev->dev.of_node, "cs-gpios", NULL)) { > - as->use_cs_gpios = false; > + !of_get_property(pdev->dev.of_node, "cs-gpios", NULL)) > master->num_chipselect = 4; > - } > > as->use_dma = false; > as->use_pdc = false; >
diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c index 08cbb3e43c76..d4a806e24060 100644 --- a/drivers/spi/spi-atmel.c +++ b/drivers/spi/spi-atmel.c @@ -312,7 +312,6 @@ struct atmel_spi { bool use_dma; bool use_pdc; - bool use_cs_gpios; /* dmaengine data */ struct atmel_spi_dma dma; @@ -323,6 +322,7 @@ struct atmel_spi { struct atmel_spi_device { unsigned int npcs_pin; u32 csr; + bool use_cs_gpio; }; #define BUFFER_SIZE PAGE_SIZE @@ -387,7 +387,7 @@ static void cs_activate(struct atmel_spi *as, struct spi_device *spi) } mr = spi_readl(as, MR); - if (as->use_cs_gpios) + if (asd->use_cs_gpio) gpio_set_value(asd->npcs_pin, active); } else { u32 cpol = (spi->mode & SPI_CPOL) ? SPI_BIT(CPOL) : 0; @@ -404,7 +404,7 @@ static void cs_activate(struct atmel_spi *as, struct spi_device *spi) mr = spi_readl(as, MR); mr = SPI_BFINS(PCS, ~(1 << spi->chip_select), mr); - if (as->use_cs_gpios && spi->chip_select != 0) + if (asd->use_cs_gpio && spi->chip_select != 0) gpio_set_value(asd->npcs_pin, active); spi_writel(as, MR, mr); } @@ -433,7 +433,7 @@ static void cs_deactivate(struct atmel_spi *as, struct spi_device *spi) asd->npcs_pin, active ? " (low)" : "", mr); - if (!as->use_cs_gpios) + if (!asd->use_cs_gpio) spi_writel(as, CR, SPI_BIT(LASTXFER)); else if (atmel_spi_is_v2(as) || spi->chip_select != 0) gpio_set_value(asd->npcs_pin, !active); @@ -1539,6 +1539,7 @@ static int atmel_spi_setup(struct spi_device *spi) unsigned int bits = spi->bits_per_word; unsigned int npcs_pin; int ret; + bool use_cs_gpio; as = spi_master_get_devdata(spi->master); @@ -1565,8 +1566,6 @@ static int atmel_spi_setup(struct spi_device *spi) csr |= SPI_BIT(CPOL); if (!(spi->mode & SPI_CPHA)) csr |= SPI_BIT(NCPHA); - if (!as->use_cs_gpios) - csr |= SPI_BIT(CSAAT); /* DLYBS is mostly irrelevant since we manage chipselect using GPIOs. * @@ -1577,13 +1576,22 @@ static int atmel_spi_setup(struct spi_device *spi) csr |= SPI_BF(DLYBS, 0); csr |= SPI_BF(DLYBCT, 0); - /* chipselect must have been muxed as GPIO (e.g. in board setup) */ npcs_pin = (unsigned long)spi->controller_data; - if (!as->use_cs_gpios) - npcs_pin = spi->chip_select; - else if (gpio_is_valid(spi->cs_gpio)) + if (gpio_is_valid(spi->cs_gpio)) { + /* GPIO from DT */ npcs_pin = spi->cs_gpio; + use_cs_gpio = true; + } else if (npcs_pin && gpio_is_valid(npcs_pin)) { + /* GPIO from platform data */ + use_cs_gpio = true; + } else if (atmel_spi_is_v2(as)) { + /* internal chip-select */ + npcs_pin = spi->chip_select; + use_cs_gpio = false; + } else { + return -EINVAL; + } asd = spi->controller_state; if (!asd) { @@ -1591,7 +1599,7 @@ static int atmel_spi_setup(struct spi_device *spi) if (!asd) return -ENOMEM; - if (as->use_cs_gpios) { + if (use_cs_gpio) { ret = gpio_request(npcs_pin, dev_name(&spi->dev)); if (ret) { kfree(asd); @@ -1603,6 +1611,7 @@ static int atmel_spi_setup(struct spi_device *spi) } asd->npcs_pin = npcs_pin; + asd->use_cs_gpio = use_cs_gpio; spi->controller_state = asd; } else { atmel_spi_lock(as); @@ -1612,6 +1621,8 @@ static int atmel_spi_setup(struct spi_device *spi) atmel_spi_unlock(as); } + if (!asd->use_cs_gpio) + csr |= SPI_BIT(CSAAT); asd->csr = csr; dev_dbg(&spi->dev, @@ -1807,12 +1818,9 @@ static int atmel_spi_probe(struct platform_device *pdev) atmel_get_caps(as); - as->use_cs_gpios = true; if (atmel_spi_is_v2(as) && - !of_get_property(pdev->dev.of_node, "cs-gpios", NULL)) { - as->use_cs_gpios = false; + !of_get_property(pdev->dev.of_node, "cs-gpios", NULL)) master->num_chipselect = 4; - } as->use_dma = false; as->use_pdc = false;
The driver currently chooses between internal chip-select or gpio based on the existence of the cs-gpios DT property which fails on non-DT systems and also enforces the same choice for all devices. This patch makes the method per device instead of per controller and fixes the selection on non-DT systems. With these changes, the chip-select method for each device is chosen according to the following priority: 1. GPIO from device tree 2. GPIO from platform data 3. Internal chip-select Tested on AVR32 ATSTK1000. Signed-off-by: Mans Rullgard <mans@mansr.com> --- Changes: - allow internal CS only hardware v2 - retain seting of master->num_chipselect to 4 on hardware v2 if cs-gpios property is missing --- drivers/spi/spi-atmel.c | 38 +++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-)