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

[Chris Packham <Chris.Packham@xxxxxxxxxxxxxxxxxxx>]

On 05/06/18 23:08, Jan Kundrát 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.
> 
> Signed-off-by: Jan Kundrát <jan.kundrat@xxxxxxxxx>

Tested-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>

> ---
> Changes v1..v2: Use devm_gpio_request_one() instead of
> devm_gpio_request() and gpio_direction_output()
> ---
>   drivers/spi/spi-orion.c | 77 +++++++++++++++++++++--------------------
>   1 file changed, 40 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/spi/spi-orion.c b/drivers/spi/spi-orion.c
> index d01a6adc726e..47ef6b1a2e76 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,44 @@ 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;
> +			int cs_flags;
> +
> +			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;
> +			}
> +
> +			cs_flags = of_property_read_bool(np, "spi-cs-high") ?
> +				GPIOF_OUT_INIT_LOW : GPIOF_OUT_INIT_HIGH;
> +			status = devm_gpio_request_one(&pdev->dev, cs_gpio,
> +					cs_flags, gpio_name);
> +			if (status) {
> +				dev_err(&pdev->dev,
> +					"Can't request GPIO for CS %d\n", cs);
> +				goto out_rel_axi_clk;
> +			}
> +		}
> +
>   		/*
>   		 * Check if an address is configured for this SPI device. If
>   		 * not, the MBus mapping via the 'ranges' property in the 'soc'
> @@ -740,44 +779,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:
> 

--
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