Skip to contentSkip to conversation

spi: pxa2xx: missing break in pxa2xx_ssp_get_clk_div()

PATCH In review Dan Carpenter 3 participants 3 messages Mar 30, 2015 – Mar 31, 2015

Subsystems (4)
Kernel janitors SPI PXA2xx/PXA3xx support Intel LPSS (Low Power SubSystem) drivers
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 spi: pxa2xx: missing break in pxa2xx_ssp_get_clk_div()

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 <[email protected]>
drivers/spi/spi-pxa2xx.c +1 -0
index 60526a5..a081cfc 100644
@@ -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);
 	}
reviewer Acked-by Tested-by on v1 1/1
#1 lore raw
Quoted 3 lines · Dan Carpenter

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.

Quoted 8 lines
Fixes 025ffe88ee60 ('spi: pxa2xx: shift clk_div in one place')
Signed-off-by Dan Carpenter <[email protected]>
drivers/spi/spi-pxa2xx.c +0 -0
index 60526a5..a081cfc 100644

drivers/spi/spi-pxa2xx.c static unsigned int pxa2xx_ssp_get_clk_div(struct driver_data *drv_data, · line 796

Quoted 6 lines
drivers/spi/spi-pxa2xx.c +1 -0
@@ -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.

drivers/spi/spi-pxa2xx.c static unsigned int pxa2xx_ssp_get_clk_div(struct driver_data *drv_data, · line 797

Quoted 1 line
drivers/spi/spi-pxa2xx.c +0 -0
 	}

Whatever solution you choose

Acked-by Andy Shevchenko <[email protected]>

And actually

Tested-by Andy Shevchenko <[email protected]>

Now run with time to see the difference.

Signature · 2 lines
Andy Shevchenko <[email protected]>
Intel Finland Oy
maintainer
#2 lore raw
Quoted 12 lines · Andy Shevchenko

drivers/spi/spi-pxa2xx.c static unsigned int pxa2xx_ssp_get_clk_div(struct driver_data *drv_data, · line 796

Quoted 7 lines · Dan Carpenter
drivers/spi/spi-pxa2xx.c +1 -0
@@ -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.