[PATCH v4 1/2] spi: implemented driver for Cirrus EP93xx SPI controller

[Mika Westerberg]

On Tue, Apr 20, 2010 at 12:24:26PM -0500, H Hartley Sweeten wrote:
> On Tuesday, April 20, 2010 8:12 AM, Mika Westerberg wrote:
> > This patch adds an SPI master driver for the Cirrus EP93xx SPI controller found
> > in EP93xx chips (EP9301, EP9302, EP9307, EP9312 and EP9315).
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg at iki.fi>
> 
> Mika,
> 
> I'm still looking this over but I have one quick comment.
> 
> > +/**
> > + * ep93xx_spi_select_device() - select given SPI device
> > + * @espi: ep93xx SPI controller struct
> > + * @spi: SPI device to select
> > + *
> > + * Function asserts chipselect line as specified in @spi->chip_select in board
> > + * specific manner.
> > + *
> > + * Note that this function is called from a thread context and can sleep.
> > + */
> > +static inline void ep93xx_spi_select_device(const struct ep93xx_spi *espi,
> > +					    struct spi_device *spi)
> > +{
> > +	if (espi->cs_control)
> > +		espi->cs_control(spi, !!(spi->mode & SPI_CS_HIGH));
> > +}
> > +
> > +/**
> > + * ep93xx_spi_deselect_device() - deselects given SPI device
> > + * @espi: ep93xx SPI controller struct
> > + * @spi: SPI device to deselect
> > + *
> > + * Function deasserts chipselect line as specified in @spi->chip_select in board
> > + * specific manner.
> > + *
> > + * Note that this function is called from a thread context and can sleep.
> > + */
> > +static inline void ep93xx_spi_deselect_device(const struct ep93xx_spi *espi,
> > +					      struct spi_device *spi)
> > +{
> > +	if (espi->cs_control)
> > +		espi->cs_control(spi, !(spi->mode & SPI_CS_HIGH));
> > +}
> 
> These two functions can be combined.
> 
> /**
>  * ep93xx_spi_chip_select() - select/deselect a given SPI device
>  * @espi: ep93xx SPI controller struct
>  * @spi: SPI device to select
>  * @assert: flag to assert the chip select line
>  *
>  * Note that this function is called from a thread context and can sleep.
>  */
> static void ep93xx_spi_chip_select(const struct ep93xx_spi *espi,
> 					     struct spi_device *spi, int assert)
> {
> 	int value = (spi->mode & SPI_CS_HIGH) ? !!assert : !assert;
> 
> 	if (espi->cs_control)
> 		espi->cs_control(spi, value);
> }
> 
> Then just do:
> 
> ep93xx_spi_select_device(espi, msg->spi, 1);	/* assert the spi chip select */
> 
> ep93xx_spi_select_device(espi, msg->spi, 0);	/* deassert the spi chip select */


I think it is more readable to do:

	ep93xx_spi_select_device(espi, msg->spi);

and

	ep93xx_spi_deselect_device(espi, msg->spi);

It can be seen from the function names what we are doing.

Thanks,
MW