Re: [PATCH 4/4] spi: dw: drop unused struct dw_spi field

[Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>]

Hi Gerhard,

On Thu, Dec 26, 2013 at 04:33:46PM +0100, Gerhard Sittig wrote:
> On Thu, Dec 26, 2013 at 13:58 +0200, Baruch Siach wrote:
> > On Thu, Dec 26, 2013 at 12:42:58PM +0100, Gerhard Sittig wrote:
> > > On Thu, Dec 26, 2013 at 09:00 +0200, Baruch Siach wrote:
> > > > diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c
> > > > index c3354e8..4e01019 100644
> > > > --- a/drivers/spi/spi-dw.c
> > > > +++ b/drivers/spi/spi-dw.c
> > > > @@ -427,7 +427,6 @@ static void pump_transfers(unsigned long data)
> > > >  	dws->tx_end = dws->tx + transfer->len;
> > > >  	dws->rx = transfer->rx_buf;
> > > >  	dws->rx_end = dws->rx + transfer->len;
> > > > -	dws->cs_change = transfer->cs_change;
> > > >  	dws->len = dws->cur_transfer->len;
> > > >  	if (chip != dws->prev_chip)
> > > >  		cs_change = 1;
> > > > diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> > > > index 1ccfc18..cb52cfe 100644
> > > > --- a/drivers/spi/spi-dw.h
> > > > +++ b/drivers/spi/spi-dw.h
> > > > @@ -135,7 +135,6 @@ struct dw_spi {
> > > >  	u8			n_bytes;	/* current is a 1/2 bytes op */
> > > >  	u8			max_bits_per_word;	/* maxim is 16b */
> > > >  	u32			dma_width;
> > > > -	int			cs_change;
> > > >  	irqreturn_t		(*transfer_handler)(struct dw_spi *dws);
> > > >  	void			(*cs_control)(u32 command);
> > > >  
> > > > -- 
> > > 
> > > This looks suspicious.  Are you (the driver) ignoring the
> > > cs_change information which the caller (the code which emits the
> > > SPI message transfer call) provides?  This appears to be a
> > > deficiency in the SPI master's code then.  Callers should be able
> > > to control CS behaviour between the partial transfers of an SPI
> > > message.  It's part of the API.
> > 
> > The pump_transfers() local cs_change variable tracks chip select status 
> > between transfers. In practice this field is meaningless for this hardware 
> > without external chip select control, like GPIO.
> 
> Please re-check, or tell me if your code base is different from
> mainline and I'm missing something.
> 
> Inspection of local code (v3.13-rc5-4-gf399199c01c7) suggests
> that the local cs_change gets preset to 0 and enforced to 1 if
> the "chip" (the SPI slave?) changes.  I can't see any other
> condition that gets evaluated.  So the current driver does ignore
> the caller's request.

Right. I missed that.

> Your change removes the dws->cs_change member and its assignment
> from transfer->cs_change.  A ':grep cs_change drivers/spi/*dw*'
> suggests that the CS change condition as specified by callers
> completely gets ignored. :-O  This is a bug in my book.

Yes. But this patch doesn't change the situation.

> Regardless of whether the internal hardware implementation of the
> SPI controller's dedicated CS lines don't support software
> control of the line (which may be the cause of the bug, and
> causes trouble even for SPI messages which consist of a single
> transfer), and can get fixed or not, is just an implementation
> detail that's specific to this setup in combination with the
> internal CS line.  But please don't ignore the caller's spec and
> thus break the SPI master's use for all other setups as well.
> Especially when you already have GPIO controlled CS lines on your
> radar.  I still agree with others that the hardware CS line
> behaviour should be considered broken (dramatically reduces the
> usefulness of the controller), and only GPIO backed CS lines
> actually unbreak this master since they do introduce software
> control for the signal which previously was completely absent.

It seems that the cs_contorl callback is a driver specific hack that was meant 
to allow software control of the actual chip-select signal. However, there is 
no implementation of cs_control in current mainline code.

> > > While you are checking whether the SPI master obeys the caller's
> > > CS change spec, can you check the optional delay between partial
> > > transfers as well (I assume that both get handled in the same
> > > spots of the code path)?  ISTR that these two aspects (and the
> > > lack of GPIO backed chip selects given how the hardware CS line
> > > acts) were real obstacles when trying to use this DW SPI master.
> > 
> > The code in pump_transfers() seems to honor delay_usecs. What was the problem 
> > you encountered?
> 
> Since you made me look at the code:  The delay_usecs spec appears
> to only get obeyed _between_ transfers.  It gets ignored for the
> last or only transfer of a message.  Which may be unexpected, and
> differs from what other SPI controllers implement.  Can't tell
> whether this breaks existing SPI slaves, but the delay probably
> was introduced for some reason.  I guess deasserting CS while the
> slave still wants it does have implications...

Right. This is a bug.

> And I remember that the DW-SPI master code I've seen in the past
> may not have been outright wrong, but somehow was organized in
> unexpected ways.  IIRC it handled the end of a transfer and the
> end of a message in separate code paths, duplicating quite some
> code yet handling the last transfer in different ways.  I was
> irritated.  But that must have been a different implementation, I
> know for sure it wasn't mainline, or else I had done something
> about it.

Well, I once wrote a driver for this SPI master. I didn't get much response on 
the mailing list at the time, so I moved on, and this driver went in instead. 
I hope it's not my code that irritated you.

> Please also note that switching to common queue support for the
> SPI master (which you have in your queue as well) may in
> bypassing fix the last transfer's being special for the DW-SPI
> driver.  Then you should not care, it's all just transfers, no
> matter how many of them form a message.

Great. So this problem would solve itself if/when my common queue patch goes 
in.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html