Re: [PATCH 1/2] SPI: control CS via standard GPIO operations instead of SPI-HW

[Stephen Warren <swarren@xxxxxxxxxxxxx>]

On 03/04/2015 09:40 AM, kernel@xxxxxxxxxxxxxxxx wrote:
> From: Martin Sperl <kernel@xxxxxxxxxxxxxxxx>
> 
> Allow the cs-gpios property in DT to be used instead of the
> fixed two chip-selects provided by the SPI-HW itself
> 
> Signed-off-by: Martin Sperl <kernel@xxxxxxxxxxxxxxxx>
> 
> ---
> 
> There is the question if we still need to support the chip_selects
> provided by the hardware (plus the buggy CSPOL_HIGH support for those cases)
> or if we could just make the cs-gpios a required setting for this driver.
> Going with the GPIO only solution would clean up the code a bit.

Yes, I believe so. Any existing DT file needs to continue working with a
new kernel; DT is an ABI.

BTW, you forgot to Cc the SPI maintainer, so he probably won't see your
patch and apply it.

I assume that the SPI core parses the cs-gpios property from DT, hence
why this patch doesn't add any DT parsing, and doesn't modify any DT
bindings?

> diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c

> @@ -205,12 +219,24 @@ static int bcm2835_spi_start_transfer(struct spi_device *spi,
>  		cs |= BCM2835_SPI_CS_CPHA;
>  
>  	if (!(spi->mode & SPI_NO_CS)) {
> -		if (spi->mode & SPI_CS_HIGH) {
> -			cs |= BCM2835_SPI_CS_CSPOL;
> -			cs |= BCM2835_SPI_CS_CSPOL0 << spi->chip_select;
> -		}
> +		if (spi->cs_gpio >= 0)
> +			bcm2835_set_cs(spi, 1);
> +		else {
> +			/* do we need to support this ?

As mentioned above, yes I believe so. You'd need to amend this comment
to remove the question, I suspect.

This patch looks OK with that change made.

> +			 * note that there is a bug in this when there are
> +			 * multiple devices on the bus with at least one
> +			 * having SPI_CS_HIGH set (the other CS_CSPOLX get
> +			 * reset to 0 when any other device
> +			 * starts a transfer
> +			 */
> +			if (spi->mode & SPI_CS_HIGH) {
> +				cs |= BCM2835_SPI_CS_CSPOL;
> +				cs |= BCM2835_SPI_CS_CSPOL0
> +					<< spi->chip_select;
> +			}
>  
> -		cs |= spi->chip_select;
> +			cs |= spi->chip_select;
> +		}
>  	}

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