[PATCH] spi/dw_spi: clean the cs_control code

[PATCH] spi/dw_spi: clean the cs_control code

[Feng Tang]

commit 052dc7c45 introduced cs_control code, which has a bug in setting
transfer mode, also it forces devices who don't need cs_control to
re-configured the control registers. This patch will fix them

Signed-off-by: Feng Tang <feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
Cc: George Shore <george-ofzTHo7+kyPpQY4QmZNtDQ@public.gmane.org>
---
 drivers/spi/dw_spi.c       |   17 +++++------------
 include/linux/spi/dw_spi.h |    3 +++
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/spi/dw_spi.c b/drivers/spi/dw_spi.c
index d256cb0..4b75a81 100644
--- a/drivers/spi/dw_spi.c
+++ b/drivers/spi/dw_spi.c
@@ -181,10 +181,6 @@ static void flush(struct dw_spi *dws)
 	wait_till_not_busy(dws);
 }
 
-static void null_cs_control(u32 command)
-{
-}
-
 static int null_writer(struct dw_spi *dws)
 {
 	u8 n_bytes = dws->n_bytes;
@@ -322,7 +318,7 @@ static void giveback(struct dw_spi *dws)
 					struct spi_transfer,
 					transfer_list);
 
-	if (!last_transfer->cs_change)
+	if (!last_transfer->cs_change && dws->cs_control)
 		dws->cs_control(MRST_SPI_DEASSERT);
 
 	msg->state = NULL;
@@ -544,13 +540,13 @@ static void pump_transfers(unsigned long data)
 	 */
 	if (dws->cs_control) {
 		if (dws->rx && dws->tx)
-			chip->tmode = 0x00;
+			chip->tmode = SPI_TMOD_TR;
 		else if (dws->rx)
-			chip->tmode = 0x02;
+			chip->tmode = SPI_TMOD_RO;
 		else
-			chip->tmode = 0x01;
+			chip->tmode = SPI_TMOD_TO;
 
-		cr0 &= ~(0x3 << SPI_MODE_OFFSET);
+		cr0 &= ~SPI_TMOD_MASK;
 		cr0 |= (chip->tmode << SPI_TMOD_OFFSET);
 	}
 
@@ -699,9 +695,6 @@ static int dw_spi_setup(struct spi_device *spi)
 		chip = kzalloc(sizeof(struct chip_data), GFP_KERNEL);
 		if (!chip)
 			return -ENOMEM;
-
-		chip->cs_control = null_cs_control;
-		chip->enable_dma = 0;
 	}
 
 	/*
diff --git a/include/linux/spi/dw_spi.h b/include/linux/spi/dw_spi.h
index cc813f9..cc9ded3 100644
--- a/include/linux/spi/dw_spi.h
+++ b/include/linux/spi/dw_spi.h
@@ -12,9 +12,12 @@
 #define SPI_FRF_RESV			0x3
 
 #define SPI_MODE_OFFSET			6
+#define SPI_MODE_MASK			(0x3 << SPI_MODE_OFFSET)
 #define SPI_SCPH_OFFSET			6
 #define SPI_SCOL_OFFSET			7
+
 #define SPI_TMOD_OFFSET			8
+#define SPI_TMOD_MASK			(0x3 << SPI_TMOD_OFFSET)
 #define	SPI_TMOD_TR			0x0		/* xmit & recv */
 #define SPI_TMOD_TO			0x1		/* xmit only */
 #define SPI_TMOD_RO			0x2		/* recv only */
-- 
1.7.0.4


------------------------------------------------------------------------------
This SF.net Dev2Dev email is sponsored by:

Show off your parallel programming skills.
Enter the Intel(R) Threading Challenge 2010.
http://p.sf.net/sfu/intel-thread-sfd

Re: [PATCH] spi/dw_spi: clean the cs_control code

[Grant Likely]

Hi Feng,

On Wed, Sep 01, 2010 at 01:35:59PM +0800, Feng Tang wrote:
> commit 052dc7c45 introduced cs_control code,

Be friendly to mere-mortals.  You should quote the patch title "spi/dw_spi:
conditional transfer mode changes" in addition to the sha1 id.

> which has a bug in setting
> transfer mode,

It would be helpful if you explained what the bug is.

> also it forces devices who don't need cs_control to
> re-configured the control registers. This patch will fix them
> 
> Signed-off-by: Feng Tang <feng.tang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> Cc: George Shore <george-ofzTHo7+kyPpQY4QmZNtDQ@public.gmane.org>
> ---
>  drivers/spi/dw_spi.c       |   17 +++++------------
>  include/linux/spi/dw_spi.h |    3 +++
>  2 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/spi/dw_spi.c b/drivers/spi/dw_spi.c
> index d256cb0..4b75a81 100644
> --- a/drivers/spi/dw_spi.c
> +++ b/drivers/spi/dw_spi.c
> @@ -181,10 +181,6 @@ static void flush(struct dw_spi *dws)
>  	wait_till_not_busy(dws);
>  }
>  
> -static void null_cs_control(u32 command)
> -{
> -}
> -
>  static int null_writer(struct dw_spi *dws)
>  {
>  	u8 n_bytes = dws->n_bytes;
> @@ -322,7 +318,7 @@ static void giveback(struct dw_spi *dws)
>  					struct spi_transfer,
>  					transfer_list);
>  
> -	if (!last_transfer->cs_change)
> +	if (!last_transfer->cs_change && dws->cs_control)
>  		dws->cs_control(MRST_SPI_DEASSERT);
>  
>  	msg->state = NULL;
> @@ -544,13 +540,13 @@ static void pump_transfers(unsigned long data)
>  	 */
>  	if (dws->cs_control) {
>  		if (dws->rx && dws->tx)
> -			chip->tmode = 0x00;
> +			chip->tmode = SPI_TMOD_TR;
>  		else if (dws->rx)
> -			chip->tmode = 0x02;
> +			chip->tmode = SPI_TMOD_RO;
>  		else
> -			chip->tmode = 0x01;
> +			chip->tmode = SPI_TMOD_TO;
>  
> -		cr0 &= ~(0x3 << SPI_MODE_OFFSET);
> +		cr0 &= ~SPI_TMOD_MASK;
>  		cr0 |= (chip->tmode << SPI_TMOD_OFFSET);

Changing these values isn't mentioned in the patch description.  I assume this is not the bug fix because the #defines are the same values.

>  	}
>  
> @@ -699,9 +695,6 @@ static int dw_spi_setup(struct spi_device *spi)
>  		chip = kzalloc(sizeof(struct chip_data), GFP_KERNEL);
>  		if (!chip)
>  			return -ENOMEM;
> -
> -		chip->cs_control = null_cs_control;
> -		chip->enable_dma = 0;
>  	}
>  
>  	/*
> diff --git a/include/linux/spi/dw_spi.h b/include/linux/spi/dw_spi.h
> index cc813f9..cc9ded3 100644
> --- a/include/linux/spi/dw_spi.h
> +++ b/include/linux/spi/dw_spi.h
> @@ -12,9 +12,12 @@
>  #define SPI_FRF_RESV			0x3
>  
>  #define SPI_MODE_OFFSET			6
> +#define SPI_MODE_MASK			(0x3 << SPI_MODE_OFFSET)

SPI_MODE_MASK is unused.

>  #define SPI_SCPH_OFFSET			6
>  #define SPI_SCOL_OFFSET			7
> +
>  #define SPI_TMOD_OFFSET			8
> +#define SPI_TMOD_MASK			(0x3 << SPI_TMOD_OFFSET)
>  #define	SPI_TMOD_TR			0x0		/* xmit & recv */
>  #define SPI_TMOD_TO			0x1		/* xmit only */
>  #define SPI_TMOD_RO			0x2		/* recv only */
> -- 
> 1.7.0.4
> 

------------------------------------------------------------------------------
This SF.net Dev2Dev email is sponsored by:

Show off your parallel programming skills.
Enter the Intel(R) Threading Challenge 2010.
http://p.sf.net/sfu/intel-thread-sfd

Re: [PATCH] spi/dw_spi: clean the cs_control code

[Feng Tang]

On Wed, 1 Sep 2010 23:07:57 +0800
Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:

> Hi Feng,
> 
> On Wed, Sep 01, 2010 at 01:35:59PM +0800, Feng Tang wrote:
> > commit 052dc7c45 introduced cs_control code,
> 
> Be friendly to mere-mortals.  You should quote the patch title
> "spi/dw_spi: conditional transfer mode changes" in addition to the
> sha1 id.

Got it, thanks, will add it in V2

> > @@ -544,13 +540,13 @@ static void pump_transfers(unsigned long data)
> >  	 */
> >  	if (dws->cs_control) {
> >  		if (dws->rx && dws->tx)
> > -			chip->tmode = 0x00;
> > +			chip->tmode = SPI_TMOD_TR;
> >  		else if (dws->rx)
> > -			chip->tmode = 0x02;
> > +			chip->tmode = SPI_TMOD_RO;
> >  		else
> > -			chip->tmode = 0x01;
> > +			chip->tmode = SPI_TMOD_TO;
> >  
> > -		cr0 &= ~(0x3 << SPI_MODE_OFFSET);
> > +		cr0 &= ~SPI_TMOD_MASK;
> >  		cr0 |= (chip->tmode << SPI_TMOD_OFFSET);
> 
> Changing these values isn't mentioned in the patch description.  I
> assume this is not the bug fix because the #defines are the same
> values.

OK, it's a little confusing, there are 2 OFFSET here:
	SPI_MODE_OFFSET (for spi mode 0/1/2 etc setting)
	SPI_TMOD_OFFSET (for tx only/rx only/duplex)

So simple fix should be
-		cr0 &= ~(0x3 << SPI_MODE_OFFSET); 
+		cr0 &= ~(0x3 << SPI_TMOD_OFFSET);
while my new introduced SPI_TMOD_MASK make things complex :)

Thanks,
Feng

------------------------------------------------------------------------------
This SF.net Dev2Dev email is sponsored by:

Show off your parallel programming skills.
Enter the Intel(R) Threading Challenge 2010.
http://p.sf.net/sfu/intel-thread-sfd