[PATCH] spi: Set cs-gpios to output direction

[PATCH] spi: Set cs-gpios to output direction

[Stephen Boyd]

Some gpios used for cs-gpios may not be configured for output by
default. In these cases gpio_set_value() won't have any effect
and so the chip select line won't toggle. Request the cs-gpios
and set them to output direction once we know if the chip select
is default high or default low.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: Richard Genoud <richard.genoud@gmail.com>
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---

I wonder if we should request the gpios when the master controller
probes or when a spi device is added? We only know what the default
value should be when the spi device is added. On the other hand,
we should probably fail probe if the gpio controller isn't ready when
the spi master controller probes.

Also, is it better to convert this over to the gpiod interfaces?

 drivers/spi/spi.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 074f55f5d5ec..00f6365b3f87 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -431,8 +431,10 @@ int spi_add_device(struct spi_device *spi)
 		goto done;
 	}
 
-	if (master->cs_gpios)
+	if (master->cs_gpios) {
 		spi->cs_gpio = master->cs_gpios[spi->chip_select];
+		gpio_direction_output(spi->cs_gpio, !(spi->mode & SPI_CS_HIGH));
+	}
 
 	/* Drivers may modify this initial i/o setup, but will
 	 * normally rely on the device being setup.  Devices
@@ -1512,8 +1514,11 @@ static int of_spi_register_master(struct spi_master *master)
 	for (i = 0; i < master->num_chipselect; i++)
 		cs[i] = -ENOENT;
 
-	for (i = 0; i < nb; i++)
+	for (i = 0; i < nb; i++) {
 		cs[i] = of_get_named_gpio(np, "cs-gpios", i);
+		devm_gpio_request(&master->dev, cs[i], "spi-cs");
+	}
+
 
 	return 0;
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] spi: Set cs-gpios to output direction

[Mark Brown]

On Fri, May 23, 2014 at 05:57:34PM -0700, Stephen Boyd wrote:
> Some gpios used for cs-gpios may not be configured for output by
> default. In these cases gpio_set_value() won't have any effect
> and so the chip select line won't toggle. Request the cs-gpios
> and set them to output direction once we know if the chip select
> is default high or default low.

Currently the SPI framework is expecting that the controller driver will
own the GPIOs so it's not requesting them at all - starting to request
them in the core without warning is likely to lead to double requests
which doesn't seem like the best idea ever.  The driver has to
understand that there are GPIO chip selects since it needs to figure out
what to do with any underlying hardware chip selects that it can't stop
toggling (there may be none or it may be directable into space with
pinmux but we can't rely on that).  

Note also that you've done this in a DT specific bit of code and this
needs to work with non-DT systems too.

> I wonder if we should request the gpios when the master controller
> probes or when a spi device is added? We only know what the default
> value should be when the spi device is added. On the other hand,
> we should probably fail probe if the gpio controller isn't ready when
> the spi master controller probes.

Right, plus the fact that each driver has to open code the requesting,
probe deferral handling and so on.  It's not super awesome, the whole
area around GPIO chip select handling needs a bit of a sorched earth
refactoring.

Ideally we'd be able to error out only the device using an individual
GPIO rather than the whole controller if a GPIO isn't there for some
reason so doing it at device time would be nicer but my recollection is
that this won't play nicely with deferred probe, it's a while since I
looked so I may be misremembering.

> Also, is it better to convert this over to the gpiod interfaces?

Yes, that too.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140524/8825c04a/attachment.sig>

[PATCH] spi: Set cs-gpios to output direction

[Stephen Boyd]

On 05/24/14 04:54, Mark Brown wrote:
> On Fri, May 23, 2014 at 05:57:34PM -0700, Stephen Boyd wrote:
>> Some gpios used for cs-gpios may not be configured for output by
>> default. In these cases gpio_set_value() won't have any effect
>> and so the chip select line won't toggle. Request the cs-gpios
>> and set them to output direction once we know if the chip select
>> is default high or default low.
> Currently the SPI framework is expecting that the controller driver will
> own the GPIOs so it's not requesting them at all - starting to request
> them in the core without warning is likely to lead to double requests
> which doesn't seem like the best idea ever.  The driver has to
> understand that there are GPIO chip selects since it needs to figure out
> what to do with any underlying hardware chip selects that it can't stop
> toggling (there may be none or it may be directable into space with
> pinmux but we can't rely on that).  

Ok. My SPI controller is relying on the pinctrl framework to request
these gpios and I didn't have that configured in DT.

>
>> I wonder if we should request the gpios when the master controller
>> probes or when a spi device is added? We only know what the default
>> value should be when the spi device is added. On the other hand,
>> we should probably fail probe if the gpio controller isn't ready when
>> the spi master controller probes.
> Right, plus the fact that each driver has to open code the requesting,
> probe deferral handling and so on.  It's not super awesome, the whole
> area around GPIO chip select handling needs a bit of a sorched earth
> refactoring.
>
> Ideally we'd be able to error out only the device using an individual
> GPIO rather than the whole controller if a GPIO isn't there for some
> reason so doing it at device time would be nicer but my recollection is
> that this won't play nicely with deferred probe, it's a while since I
> looked so I may be misremembering.

Yes. There would need to be some hook into the SPI core from the driver
core that notified of any new driver probes. Then we could try and get
any pending cs-gpios again and then add the device that uses that chip
select.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH] spi: Set cs-gpios to output direction

[Linus Walleij]

On Sat, May 24, 2014 at 2:57 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:

> Also, is it better to convert this over to the gpiod interfaces?

Yes :-)

Yours,
Linus Walleij