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

[H Hartley Sweeten]

On Wednesday, April 21, 2010 12:16 AM, Mika Westerberg wrote:
>> 
>> 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.

So rename the combined function to ep93xx_spi_cs_control and pass the "control"
parameter as a bool; true = selected, false = deselected.

Having the two almost identical functions is a bit redundant.  It also gives you
two places to have to maintain basically the same operation.  I think this is
what Grant was referring to with his previous comment.

Regards,
Hartley