Re: spi_bitbang cs_change hinting fix

[Jan Nikitenko <jan.nikitenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Hi David,

Jan Nikitenko wrote:
> Fix condition for deactivating chipselect after processing of
> spi_message - if cs_change==0, chipselect should not be touched and left
> active for next spi_message to follow.
> 
> Signed-off-by: Jan Nikitenko <jan.nikitenko-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> ---
> 
> I am not sure about the meaning of spi message transfer cs_change attribute.
> 
> I assume that non-zero cs_change indicate that chipselect should be
> toggled between spi_transfers.
> 
> What about cs_change in last spi_transfer of spi_message?
> Should zero cs_change provide hinting that chipselect should be left
> active, because spi_message to follow will be for the same spi device?
> 
> If that is so, the handling in spi_bitbang.c would be incorrect, because
> chipselect would be deactivated when cs_change==0.
> 
> Or is the meaning of cs_change in the last spi_transfer of spi_message
> different than in previous spi_transfers in single spi_message?
> Like cs_change==0 in last spi_transfer would indicate to deactivate
> chipselect and cs_change!=0 in last spi_transfer would provide hinting
> that next spi_message will be for the same spi device and chipselect
> should be left active?
> If that is so, it's quite confusing (and not documented), as the meaning
> of cs_change in the last spi_transfer would be opposite than in other
> spi_transfers in single spi_message.
> 
> The patch here fixes the condition for the case that cs_change meaning
> is the first variant described.
> 
>  spi_bitbang.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi_bitbang.c b/drivers/spi/spi_bitbang.c
> index f7f8580..7ff31a3 100644
> --- a/drivers/spi/spi_bitbang.c
> +++ b/drivers/spi/spi_bitbang.c
> @@ -380,7 +380,7 @@ static void bitbang_work(struct work_struct *work)
>  		 * cs_change has hinted that the next message will probably
>  		 * be for this chip too.
>  		 */
> -		if (!(status == 0 && cs_change)) {
> +		if (!(status == 0 && !cs_change)) {
>  			ndelay(nsecs);
>  			bitbang->chipselect(spi, BITBANG_CS_INACTIVE);
>  			ndelay(nsecs);


Could you, please, comment this issue and clarify the meaning of
cs_change in the last spi_transfer of spi_message?

It seems that I am not the only one confused about that:
http://lkml.org/lkml/2006/2/28/2
SPI: cs_change usage

Thanks and best regards,
Jan

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace