| Message ID | eea1ede6e442b09a71ebb956c4411064038109f7.1528133970.git.jan.kundrat@cesnet.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 4, 2018 at 5:34 PM, Jan Kundrát <jan.kundrat@cesnet.cz> wrote: > The code did not de-assert any CS GPIOs before probing slaves. This > means that several CS signals could be active at once, garbling the > communication. Whether this was actually a problem depended on the type > of the SPI device attached (so my "spidev" for userspace access worked > correctly because its probe was effectively a no-op), and on the state > of the GPIO pins at SoC's boot. > > The code was already iterating through all DT children of the SPI > controller, so this change re-uses that loop for CS GPIO setup as well. > This means that this might change the number of the HW CS signal which > is picked for all GPIO CS devices. Previously, the lowest one was used, > but we now use the first one from the DT. > > With this move of the code, we can also finally initialize each GPIO CS > lane before registering the SPI controller (which in turn probes for > slaves). > > I tried to fix this in 544248623b95 already, but that only did it half > way by registering the GPIOs properly. That patch failed to set their > logic signals early enough, though. > Does it make sense to switch to GPIO descriptors at some point and use devm_gpiod_get_index() instead of two calls? > Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz> > --- > drivers/spi/spi-orion.c | 78 ++++++++++++++++++++++------------------- > 1 file changed, 41 insertions(+), 37 deletions(-) > > diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c > index d01a6adc726e..a12ec0814327 100644 > --- a/drivers/spi/spi-orion.c > +++ b/drivers/spi/spi-orion.c > @@ -20,6 +20,7 @@ > #include <linux/of.h> > #include <linux/of_address.h> > #include <linux/of_device.h> > +#include <linux/of_gpio.h> > #include <linux/clk.h> > #include <linux/sizes.h> > #include <linux/gpio.h> > @@ -681,9 +682,9 @@ static int orion_spi_probe(struct platform_device *pdev) > goto out_rel_axi_clk; > } > > - /* Scan all SPI devices of this controller for direct mapped devices */ > for_each_available_child_of_node(pdev->dev.of_node, np) { > u32 cs; > + int cs_gpio; > > /* Get chip-select number from the "reg" property */ > status = of_property_read_u32(np, "reg", &cs); > @@ -694,6 +695,45 @@ static int orion_spi_probe(struct platform_device *pdev) > continue; > } > > + /* > + * Initialize the CS GPIO: > + * - properly request the actual GPIO signal > + * - de-assert the logical signal so that all GPIO CS lines > + * are inactive when probing for slaves > + * - find an unused physical CS which will be driven for any > + * slave which uses a CS GPIO > + */ > + cs_gpio = of_get_named_gpio(pdev->dev.of_node, "cs-gpios", cs); > + if (cs_gpio > 0) { > + char *gpio_name; > + > + if (spi->unused_hw_gpio == -1) { > + dev_info(&pdev->dev, > + "Selected unused HW CS#%d for any GPIO CSes\n", > + cs); > + spi->unused_hw_gpio = cs; > + } > + > + gpio_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, > + "%s-CS%d", dev_name(&pdev->dev), cs); > + if (!gpio_name) { > + status = -ENOMEM; > + goto out_rel_axi_clk; > + } > + > + status = devm_gpio_request(&pdev->dev, cs_gpio, > + gpio_name); > + if (status) { > + dev_err(&pdev->dev, > + "Can't request GPIO for CS %d\n", cs); > + goto out_rel_axi_clk; > + } > + > + gpio_direction_output(cs_gpio, > + !of_property_read_bool(np, > + "spi-cs-high")); > + } > + > /* > * Check if an address is configured for this SPI device. If > * not, the MBus mapping via the 'ranges' property in the 'soc' > @@ -740,44 +780,8 @@ static int orion_spi_probe(struct platform_device *pdev) > if (status < 0) > goto out_rel_pm; > > - if (master->cs_gpios) { > - int i; > - for (i = 0; i < master->num_chipselect; ++i) { > - char *gpio_name; > - > - if (!gpio_is_valid(master->cs_gpios[i])) { > - continue; > - } > - > - gpio_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, > - "%s-CS%d", dev_name(&pdev->dev), i); > - if (!gpio_name) { > - status = -ENOMEM; > - goto out_rel_master; > - } > - > - status = devm_gpio_request(&pdev->dev, > - master->cs_gpios[i], gpio_name); > - if (status) { > - dev_err(&pdev->dev, > - "Can't request GPIO for CS %d\n", > - master->cs_gpios[i]); > - goto out_rel_master; > - } > - if (spi->unused_hw_gpio == -1) { > - dev_info(&pdev->dev, > - "Selected unused HW CS#%d for any GPIO CSes\n", > - i); > - spi->unused_hw_gpio = i; > - } > - } > - } > - > - > return status; > > -out_rel_master: > - spi_unregister_master(master); > out_rel_pm: > pm_runtime_disable(&pdev->dev); > out_rel_axi_clk: > -- > 2.17.1 > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-spi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
> Does it make sense to switch to GPIO descriptors at some point That comes from the SPI core. Struct spi_device defines cs_gpio as an int. > and use devm_gpiod_get_index() instead of two calls? Thanks, I've changed the code to use devm_gpio_request_one() now. Cheers, Jan -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 5, 2018 at 2:03 PM, Jan Kundrát <jan.kundrat@cesnet.cz> wrote: >> Does it make sense to switch to GPIO descriptors at some point > > > That comes from the SPI core. Struct spi_device defines cs_gpio as an int. Yep, it's kinda chicken-egg issue right now. We would like to move SPI core to GPIO descriptors, but we would like not to touch entire subsystem at once. If more stuff would be converted, the life would be easier to convert SPI core itself.
diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c index d01a6adc726e..a12ec0814327 100644 --- a/drivers/spi/spi-orion.c +++ b/drivers/spi/spi-orion.c @@ -20,6 +20,7 @@ #include <linux/of.h> #include <linux/of_address.h> #include <linux/of_device.h> +#include <linux/of_gpio.h> #include <linux/clk.h> #include <linux/sizes.h> #include <linux/gpio.h> @@ -681,9 +682,9 @@ static int orion_spi_probe(struct platform_device *pdev) goto out_rel_axi_clk; } - /* Scan all SPI devices of this controller for direct mapped devices */ for_each_available_child_of_node(pdev->dev.of_node, np) { u32 cs; + int cs_gpio; /* Get chip-select number from the "reg" property */ status = of_property_read_u32(np, "reg", &cs); @@ -694,6 +695,45 @@ static int orion_spi_probe(struct platform_device *pdev) continue; } + /* + * Initialize the CS GPIO: + * - properly request the actual GPIO signal + * - de-assert the logical signal so that all GPIO CS lines + * are inactive when probing for slaves + * - find an unused physical CS which will be driven for any + * slave which uses a CS GPIO + */ + cs_gpio = of_get_named_gpio(pdev->dev.of_node, "cs-gpios", cs); + if (cs_gpio > 0) { + char *gpio_name; + + if (spi->unused_hw_gpio == -1) { + dev_info(&pdev->dev, + "Selected unused HW CS#%d for any GPIO CSes\n", + cs); + spi->unused_hw_gpio = cs; + } + + gpio_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, + "%s-CS%d", dev_name(&pdev->dev), cs); + if (!gpio_name) { + status = -ENOMEM; + goto out_rel_axi_clk; + } + + status = devm_gpio_request(&pdev->dev, cs_gpio, + gpio_name); + if (status) { + dev_err(&pdev->dev, + "Can't request GPIO for CS %d\n", cs); + goto out_rel_axi_clk; + } + + gpio_direction_output(cs_gpio, + !of_property_read_bool(np, + "spi-cs-high")); + } + /* * Check if an address is configured for this SPI device. If * not, the MBus mapping via the 'ranges' property in the 'soc' @@ -740,44 +780,8 @@ static int orion_spi_probe(struct platform_device *pdev) if (status < 0) goto out_rel_pm; - if (master->cs_gpios) { - int i; - for (i = 0; i < master->num_chipselect; ++i) { - char *gpio_name; - - if (!gpio_is_valid(master->cs_gpios[i])) { - continue; - } - - gpio_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, - "%s-CS%d", dev_name(&pdev->dev), i); - if (!gpio_name) { - status = -ENOMEM; - goto out_rel_master; - } - - status = devm_gpio_request(&pdev->dev, - master->cs_gpios[i], gpio_name); - if (status) { - dev_err(&pdev->dev, - "Can't request GPIO for CS %d\n", - master->cs_gpios[i]); - goto out_rel_master; - } - if (spi->unused_hw_gpio == -1) { - dev_info(&pdev->dev, - "Selected unused HW CS#%d for any GPIO CSes\n", - i); - spi->unused_hw_gpio = i; - } - } - } - - return status; -out_rel_master: - spi_unregister_master(master); out_rel_pm: pm_runtime_disable(&pdev->dev); out_rel_axi_clk:
The code did not de-assert any CS GPIOs before probing slaves. This means that several CS signals could be active at once, garbling the communication. Whether this was actually a problem depended on the type of the SPI device attached (so my "spidev" for userspace access worked correctly because its probe was effectively a no-op), and on the state of the GPIO pins at SoC's boot. The code was already iterating through all DT children of the SPI controller, so this change re-uses that loop for CS GPIO setup as well. This means that this might change the number of the HW CS signal which is picked for all GPIO CS devices. Previously, the lowest one was used, but we now use the first one from the DT. With this move of the code, we can also finally initialize each GPIO CS lane before registering the SPI controller (which in turn probes for slaves). I tried to fix this in 544248623b95 already, but that only did it half way by registering the GPIOs properly. That patch failed to set their logic signals early enough, though. Signed-off-by: Jan Kundrát <jan.kundrat@cesnet.cz> --- drivers/spi/spi-orion.c | 78 ++++++++++++++++++++++------------------- 1 file changed, 41 insertions(+), 37 deletions(-)