* [patch] spi: pxa2xx: missing break in pxa2xx_ssp_get_clk_div()
@ 2015-03-30 10:55 ` Andy Shevchenko
0 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2015-03-30 10:55 UTC (permalink / raw)
To: linux-arm-kernel
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.
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [patch] spi: pxa2xx: missing break in pxa2xx_ssp_get_clk_div()
@ 2015-03-30 10:55 ` Andy Shevchenko
0 siblings, 0 replies; 9+ messages in thread
From: Andy Shevchenko @ 2015-03-30 10:55 UTC (permalink / raw)
To: Dan Carpenter
Cc: Mark Brown, Haojian Zhuang, Robert Jarzmik, Daniel Mack,
linux-arm-kernel, linux-spi, kernel-janitors
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.
--
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [patch] spi: pxa2xx: missing break in pxa2xx_ssp_get_clk_div()
@ 2015-03-31 8:49 ` Mark Brown
0 siblings, 0 replies; 9+ messages in thread
From: Mark Brown @ 2015-03-31 8:49 UTC (permalink / raw)
To: linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 680 bytes --]
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.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread* [patch] spi: pxa2xx: missing break in pxa2xx_ssp_get_clk_div()
@ 2015-03-31 8:49 ` Mark Brown
0 siblings, 0 replies; 9+ messages in thread
From: Mark Brown @ 2015-03-31 8:49 UTC (permalink / raw)
To: linux-arm-kernel
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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150331/643da208/attachment.sig>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [patch] spi: pxa2xx: missing break in pxa2xx_ssp_get_clk_div()
@ 2015-03-31 8:49 ` Mark Brown
0 siblings, 0 replies; 9+ messages in thread
From: Mark Brown @ 2015-03-31 8:49 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Dan Carpenter, Haojian Zhuang, Robert Jarzmik, Daniel Mack,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-spi-u79uwXL29TY76Z2rM5mHXA,
kernel-janitors-u79uwXL29TY76Z2rM5mHXA
[-- Attachment #1: Type: text/plain, Size: 680 bytes --]
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.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread