diff mbox

spi: pxa2xx: missing break in pxa2xx_ssp_get_clk_div()

Message ID 20150330103040.GB27144@mwanda (mailing list archive)
State New, archived
Headers show

Commit Message

Dan Carpenter March 30, 2015, 10:30 a.m. UTC 
We refactored this code but accidentally left out a break statement so
QUARK_X1000_SSP isn't handled correctly.

Fixes: 025ffe88ee60 ('spi: pxa2xx: shift clk_div in one place')
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Comments

Andy Shevchenko March 30, 2015, 10:55 a.m. UTC | #1 
On Mon, 2015-03-30 at 13:30 +0300, Dan Carpenter wrote:
> We refactored this code but accidentally left out a break statement so
> QUARK_X1000_SSP isn't handled correctly.
> 

Thanks for catching this!
One comment below.


> Fixes: 025ffe88ee60 ('spi: pxa2xx: shift clk_div in one place')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
> index 60526a5..a081cfc 100644
> --- a/drivers/spi/spi-pxa2xx.c
> +++ b/drivers/spi/spi-pxa2xx.c
> @@ -791,6 +791,7 @@ static unsigned int pxa2xx_ssp_get_clk_div(struct driver_data *drv_data,
>  	switch (drv_data->ssp_type) {
>  	case QUARK_X1000_SSP:
>  		clk_div = quark_x1000_get_clk_div(rate, &chip->dds_rate);
> +		break;
>  	default:
>  		clk_div = ssp_get_clk_div(drv_data, rate);

It would be nice to have a break here as well, but I think we also may
change this to if-else-if construction.

>  	}

Whatever solution you choose
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

And actually 
Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Now run with time to see the difference.
Mark Brown March 31, 2015, 8:49 a.m. UTC | #2 
On Mon, Mar 30, 2015 at 01:55:12PM +0300, Andy Shevchenko wrote:
> On Mon, 2015-03-30 at 13:30 +0300, Dan Carpenter wrote:

> > @@ -791,6 +791,7 @@ static unsigned int pxa2xx_ssp_get_clk_div(struct driver_data *drv_data,
> >  	switch (drv_data->ssp_type) {
> >  	case QUARK_X1000_SSP:
> >  		clk_div = quark_x1000_get_clk_div(rate, &chip->dds_rate);
> > +		break;
> >  	default:
> >  		clk_div = ssp_get_clk_div(drv_data, rate);

> It would be nice to have a break here as well, but I think we also may
> change this to if-else-if construction.

No.  Using if/else chains is an antipattern - think what happens when a
new hardware variant needs slightly different handling again.
diff mbox

Patch

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 60526a5..a081cfc 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -791,6 +791,7 @@  static unsigned int pxa2xx_ssp_get_clk_div(struct driver_data *drv_data,
 	switch (drv_data->ssp_type) {
 	case QUARK_X1000_SSP:
 		clk_div = quark_x1000_get_clk_div(rate, &chip->dds_rate);
+		break;
 	default:
 		clk_div = ssp_get_clk_div(drv_data, rate);
 	}