Re: [PATCH 5/6] spi: spi-bfin5xx: reverse if condition in interrupt mode

On Mon, 23 Apr 2012 18:18:12 -0400, Scott Jiang
<scott.jiang.linux@...> wrote:
> This condition is used to determine 8 bits or 16 and 32 bits transfer. Obviously it is reversed.
> 
> Signed-off-by: Scott Jiang <scott.jiang.linux@...>

Applied, thanks.

g.

> ---
>  drivers/spi/spi-bfin5xx.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-bfin5xx.c b/drivers/spi/spi-bfin5xx.c
> index c0cdcb7..432d019 100644
> --- a/drivers/spi/spi-bfin5xx.c
> +++ b/drivers/spi/spi-bfin5xx.c
>  <at>  <at>  -396,7 +396,7  <at>  <at>  static irqreturn_t bfin_spi_pio_irq_handler(int irq, void *dev_id)
>  		/* last read */
>  		if (drv_data->rx) {
>  			dev_dbg(&drv_data->pdev->dev, "last read\n");
> -			if (n_bytes % 2) {
> +			if (!(n_bytes % 2)) {
>  				u16 *buf = (u16 *)drv_data->rx;
>  				for (loop = 0; loop < n_bytes / 2; loop++)
>  					*buf++ = bfin_read(&drv_data->regs->rdbr);
>  <at>  <at>  -424,7 +424,7  <at>  <at>  static irqreturn_t bfin_spi_pio_irq_handler(int irq, void *dev_id)
>  	if (drv_data->rx && drv_data->tx) {
>  		/* duplex */

Re: [PATCH 3/6] spi/bfin_spi: drop bits_per_word from client data

On Mon, 23 Apr 2012 18:18:10 -0400, Scott Jiang
<scott.jiang.linux@...> wrote:
> No other SPI controller has this field, and SPI clients should be setting
> this up in their own drivers.  So drop it from the Blackfin controller to
> keep people from using it.
> 
> Signed-off-by: Mike Frysinger <vapier@...>
> Signed-off-by: Scott Jiang <scott.jiang.linux@...>

Applied, thanks.

g.

> ---
>  drivers/spi/spi-bfin5xx.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/spi/spi-bfin5xx.c b/drivers/spi/spi-bfin5xx.c
> index 3b83ff8..c0cdcb7 100644
> --- a/drivers/spi/spi-bfin5xx.c
> +++ b/drivers/spi/spi-bfin5xx.c
>  <at>  <at>  -1026,7 +1026,6  <at>  <at>  static int bfin_spi_setup(struct spi_device *spi)
>  		chip->cs_chg_udelay = chip_info->cs_chg_udelay;
>  		chip->idle_tx_val = chip_info->idle_tx_val;
>  		chip->pio_interrupt = chip_info->pio_interrupt;
> -		spi->bits_per_word = chip_info->bits_per_word;
>  	} else {
>  		/* force a default base state */
>  		chip->ctl_reg &= bfin_ctl_reg;
> -- 

Re: [PATCH 4/6] spi/spi_bfin_sport: drop bits_per_word from client data

On Mon, 23 Apr 2012 18:18:11 -0400, Scott Jiang
<scott.jiang.linux@...> wrote:
> Since the member was dropped from the common Blackfin header, we need
> to stop using it in the SPORT driver too.
> 
> Signed-off-by: Mike Frysinger <vapier@...>
> Signed-off-by: Scott Jiang <scott.jiang.linux@...>

Applied, thanks

g.

> ---
>  drivers/spi/spi-bfin-sport.c |   14 ++++++++------
>  1 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/spi/spi-bfin-sport.c b/drivers/spi/spi-bfin-sport.c
> index bfd9047..1fe5119 100644
> --- a/drivers/spi/spi-bfin-sport.c
> +++ b/drivers/spi/spi-bfin-sport.c
>  <at>  <at>  -416,11 +416,12  <at>  <at>  bfin_sport_spi_pump_transfers(unsigned long data)
>  	drv_data->cs_change = transfer->cs_change;
>  
>  	/* Bits per word setup */
> -	bits_per_word = transfer->bits_per_word ? : message->spi->bits_per_word;
> -	if (bits_per_word == 8)
> -		drv_data->ops = &bfin_sport_transfer_ops_u8;
> -	else
> +	bits_per_word = transfer->bits_per_word ? :
> +		message->spi->bits_per_word ? : 8;

Re: [PATCH 6/6] spi: spi-bfin5xx: flush spi after each transfer

On Mon, 23 Apr 2012 18:18:13 -0400, Scott Jiang
<scott.jiang.linux@...> wrote:
> This can make sure last bit has been shifted out of register.
> 
> Signed-off-by: Scott Jiang <scott.jiang.linux@...>

Okay, so I have a few issues about the way this series was
submitted, but I've gone ahead and applied them anyway to get them off
my plate since they look like bug fixes.

The problem is that the commit text is far from complete.  Take for
example the commit text from this patch:

> This can make sure last bit has been shifted out of register.

So, what does that tell me?  Not a whole lot.  It tells me that a bit
needs to be shifted out, but nothing about when or where that is
needed.  I can *guess* that this is a bug fix, but there is nowhere
that says that is what it does.

The commit text needs to give a lot more information about why a patch
is needed.  It should answer the following questions:
1) What is the problem
2) how does the problem occur; what are the symptoms
3) A description of the fix and/or how this patch fixes the problem
4) some details about how the patch was tested.

So, for example, I might write something like this for the commit
text (I'm guessing at details here for the purpose of an example):

Re: [PATCH 2/6] spi: spi-bfin-sport: move word length setup to transfer handler

On Mon, 23 Apr 2012 18:18:09 -0400, Scott Jiang
<scott.jiang.linux@...> wrote:
> Each transfer may have its own bits per word.
> 
> Signed-off-by: Scott Jiang <scott.jiang.linux@...>

Applied, thanks.

g.

> ---
>  drivers/spi/spi-bfin-sport.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-bfin-sport.c b/drivers/spi/spi-bfin-sport.c
> index 248a2cc..bfd9047 100644
> --- a/drivers/spi/spi-bfin-sport.c
> +++ b/drivers/spi/spi-bfin-sport.c
>  <at>  <at>  -252,19 +252,15  <at>  <at>  static void
>  bfin_sport_spi_restore_state(struct bfin_sport_spi_master_data *drv_data)
>  {
>  	struct bfin_sport_spi_slave_data *chip = drv_data->cur_chip;
> -	unsigned int bits = (drv_data->ops == &bfin_sport_transfer_ops_u8 ? 7 : 15);
>  
>  	bfin_sport_spi_disable(drv_data);
>  	dev_dbg(drv_data->dev, "restoring spi ctl state\n");
>  
>  	bfin_write(&drv_data->regs->tcr1, chip->ctl_reg);
> -	bfin_write(&drv_data->regs->tcr2, bits);
>  	bfin_write(&drv_data->regs->tclkdiv, chip->baud);

Re: [PATCH 2/6] spi: spi-bfin-sport: move word length setup to transfer handler

On Mon, 23 Apr 2012 18:18:09 -0400, Scott Jiang
<scott.jiang.linux@...> wrote:
> Each transfer may have its own bits per word.
> 
> Signed-off-by: Scott Jiang <scott.jiang.linux@...>

Applied, thanks.

g.

> ---
>  drivers/spi/spi-bfin-sport.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-bfin-sport.c b/drivers/spi/spi-bfin-sport.c
> index 248a2cc..bfd9047 100644
> --- a/drivers/spi/spi-bfin-sport.c
> +++ b/drivers/spi/spi-bfin-sport.c
>  <at>  <at>  -252,19 +252,15  <at>  <at>  static void
>  bfin_sport_spi_restore_state(struct bfin_sport_spi_master_data *drv_data)
>  {
>  	struct bfin_sport_spi_slave_data *chip = drv_data->cur_chip;
> -	unsigned int bits = (drv_data->ops == &bfin_sport_transfer_ops_u8 ? 7 : 15);
>  
>  	bfin_sport_spi_disable(drv_data);
>  	dev_dbg(drv_data->dev, "restoring spi ctl state\n");
>  
>  	bfin_write(&drv_data->regs->tcr1, chip->ctl_reg);
> -	bfin_write(&drv_data->regs->tcr2, bits);
>  	bfin_write(&drv_data->regs->tclkdiv, chip->baud);

Re: [PATCH 1/6] spi: rename config macro name for bfin5xx spi controller driver

On Friday 27 April 2012 13:21:51 Grant Likely wrote:
> On Mon, 23 Apr 2012 18:18:08 -0400, Scott Jiang wrote:
> > This controller is only for blackfin 5xx soc, so rename it to BFIN5XX
> > 
> > Signed-off-by: Scott Jiang <scott.jiang.linux@...>
> 
> Is this series bug fixes that need to get into v3.4?

i think so ... i asked Scott to mark the ones that needed to be in 3.4, but i 
guess that msg got missed :/
-mike

_______________________________________________
Uclinux-dist-devel mailing list
Uclinux-dist-devel@...
https://blackfin.uclinux.org/mailman/listinfo/uclinux-dist-devel