| Message ID | cc63df805aa126eede50aa11b3fb3c5bdef67f65.1390481413.git.baruch@tkos.co.il (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, Jan 23, 2014 at 03:05:58PM +0200, Baruch Siach wrote: > Signed-off-by: Baruch Siach <baruch@tkos.co.il> If you convert to the generic queue then there's generic support for managing a GPIO based /CS if you set cs_gpio in the spi_device. We should extend that for GPIO descriptors too. > + if (gpio_is_valid(spi->cs_gpio)) { > + ret = gpio_request(spi->cs_gpio, dev_name(&spi->dev)); > + if (ret) > + return ret; > + ret = gpio_direction_output(spi->cs_gpio, > + !(spi->mode & SPI_CS_HIGH)); devm_gpio_request_one().
Hi Mark, On Thu, Jan 23, 2014 at 01:26:23PM +0000, Mark Brown wrote: > On Thu, Jan 23, 2014 at 03:05:58PM +0200, Baruch Siach wrote: > > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > > If you convert to the generic queue then there's generic support for > managing a GPIO based /CS if you set cs_gpio in the spi_device. We > should extend that for GPIO descriptors too. Thank. This further simplifies things. AFAICS the generic gpio chip select code does not request the GPIO. Is there a reason for that? > > + if (gpio_is_valid(spi->cs_gpio)) { > > + ret = gpio_request(spi->cs_gpio, dev_name(&spi->dev)); > > + if (ret) > > + return ret; > > + ret = gpio_direction_output(spi->cs_gpio, > > + !(spi->mode & SPI_CS_HIGH)); > > devm_gpio_request_one(). Right. baruch
On Thu, Jan 23, 2014 at 04:05:46PM +0200, Baruch Siach wrote: > On Thu, Jan 23, 2014 at 01:26:23PM +0000, Mark Brown wrote: > > If you convert to the generic queue then there's generic support for > > managing a GPIO based /CS if you set cs_gpio in the spi_device. We > > should extend that for GPIO descriptors too. > Thank. This further simplifies things. AFAICS the generic gpio chip select > code does not request the GPIO. Is there a reason for that? Mostly because it might not interact so well with deferred probe, lots of drivers only request/figure out the GPIOs in setup at the minute but for deferred probe we ought to be doing it as part of the main driver probe(). We should also be switching away to the GPIO descriptor API is part of it, but also I want to do a pass and add standard DT bindings for this.
Hi Mark, On Thu, Jan 23, 2014 at 01:26:23PM +0000, Mark Brown wrote: > On Thu, Jan 23, 2014 at 03:05:58PM +0200, Baruch Siach wrote: > > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > > If you convert to the generic queue then there's generic support for > managing a GPIO based /CS if you set cs_gpio in the spi_device. We > should extend that for GPIO descriptors too. Generic queue GPIO chip-select management depends on switching from transfer_one_message to transfer_one as you suggested in another email. Unfortunately there is just too much logic going on in the pump_transfers workqueue, and I can't expect it to work correctly without testing on real hardware. baruch
diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c index bf98d63d92b3..595761593492 100644 --- a/drivers/spi/spi-dw.c +++ b/drivers/spi/spi-dw.c @@ -24,6 +24,7 @@ #include <linux/delay.h> #include <linux/slab.h> #include <linux/spi/spi.h> +#include <linux/gpio.h> #include "spi-dw.h" @@ -265,6 +266,8 @@ static void giveback(struct dw_spi *dws) struct spi_transfer *last_transfer; unsigned long flags; struct spi_message *msg; + int cs_gpio = dws->cur_msg->spi->cs_gpio; + u16 mode = dws->cur_msg->spi->mode; spin_lock_irqsave(&dws->lock, flags); msg = dws->cur_msg; @@ -280,8 +283,12 @@ static void giveback(struct dw_spi *dws) struct spi_transfer, transfer_list); - if (!last_transfer->cs_change && dws->cs_control) - dws->cs_control(MRST_SPI_DEASSERT); + if (!last_transfer->cs_change) { + if (dws->cs_control) + dws->cs_control(MRST_SPI_DEASSERT); + if (gpio_is_valid(cs_gpio)) + gpio_set_value(cs_gpio, !(mode & SPI_CS_HIGH)); + } msg->state = NULL; if (msg->complete) @@ -509,7 +516,8 @@ static void pump_transfers(unsigned long data) dw_writew(dws, DW_SPI_CTRL0, cr0); spi_set_clk(dws, clk_div ? clk_div : chip->clk_div); - spi_chip_sel(dws, spi->chip_select); + spi_chip_sel(dws, spi->chip_select, spi->cs_gpio, + spi->mode & SPI_CS_HIGH); /* Set the interrupt mask, for poll mode just disable all int */ spi_mask_intr(dws, 0xff); @@ -615,6 +623,7 @@ static int dw_spi_setup(struct spi_device *spi) { struct dw_spi_chip *chip_info = NULL; struct chip_data *chip; + int ret; /* Only alloc on first setup */ chip = spi_get_ctldata(spi); @@ -668,12 +677,30 @@ static int dw_spi_setup(struct spi_device *spi) | (spi->mode << SPI_MODE_OFFSET) | (chip->tmode << SPI_TMOD_OFFSET); + if (gpio_is_valid(spi->cs_gpio)) { + ret = gpio_request(spi->cs_gpio, dev_name(&spi->dev)); + if (ret) + return ret; + ret = gpio_direction_output(spi->cs_gpio, + !(spi->mode & SPI_CS_HIGH)); + if (ret) + goto err_gpio_free; + } + return 0; + +err_gpio_free: + if (gpio_is_valid(spi->cs_gpio)) + gpio_free(spi->cs_gpio); + + return ret; } static void dw_spi_cleanup(struct spi_device *spi) { struct chip_data *chip = spi_get_ctldata(spi); + if (gpio_is_valid(spi->cs_gpio)) + gpio_free(spi->cs_gpio); kfree(chip); } diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h index 587643dae11e..4a3a6d764b48 100644 --- a/drivers/spi/spi-dw.h +++ b/drivers/spi/spi-dw.h @@ -3,6 +3,7 @@ #include <linux/io.h> #include <linux/scatterlist.h> +#include <linux/gpio.h> /* Register offsets */ #define DW_SPI_CTRL0 0x00 @@ -186,13 +187,16 @@ static inline void spi_set_clk(struct dw_spi *dws, u16 div) dw_writel(dws, DW_SPI_BAUDR, div); } -static inline void spi_chip_sel(struct dw_spi *dws, u16 cs) +static inline void spi_chip_sel(struct dw_spi *dws, u16 cs, int cs_gpio, + int gpio_active_val) { if (cs > dws->num_cs) return; if (dws->cs_control) dws->cs_control(1); + if (gpio_is_valid(cs_gpio)) + gpio_set_value(cs_gpio, gpio_active_val); dw_writel(dws, DW_SPI_SER, 1 << cs); }
Signed-off-by: Baruch Siach <baruch@tkos.co.il> --- drivers/spi/spi-dw.c | 33 ++++++++++++++++++++++++++++++--- drivers/spi/spi-dw.h | 6 +++++- 2 files changed, 35 insertions(+), 4 deletions(-)