Re: [PATCH] spi: orion: fix CS GPIO handling again

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Mon, Jun 4, 2018 at 5:34 PM, Jan Kundrát <jan.kundrat@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html