| Message ID | 1394409705.12514.2.camel@phoenix (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi Axel, On Mon, Mar 10, 2014 at 08:01:45AM +0800, Axel Lin wrote: > This is a DT-only driver, so remove all non-DT paths. ok in general, but I have a few comments, see below. > Signed-off-by: Axel Lin <axel.lin@ingics.com> > --- > drivers/spi/spi-efm32.c | 28 +++++++--------------------- > 1 file changed, 7 insertions(+), 21 deletions(-) > > diff --git a/drivers/spi/spi-efm32.c b/drivers/spi/spi-efm32.c > index f53bbea..26e0362 100644 > --- a/drivers/spi/spi-efm32.c > +++ b/drivers/spi/spi-efm32.c > @@ -294,9 +294,6 @@ static int efm32_spi_probe_dt(struct platform_device *pdev, > u32 location; > int ret; > > - if (!np) > - return 1; > - > ret = of_property_read_u32(np, "location", &location); > if (!ret) { > dev_dbg(&pdev->dev, "using location %u\n", location); > @@ -318,9 +315,14 @@ static int efm32_spi_probe(struct platform_device *pdev) > int ret; > struct spi_master *master; > struct device_node *np = pdev->dev.of_node; > - unsigned int num_cs, i; > + int num_cs, i; > + > + if (!np) > + return -EINVAL; > > num_cs = of_gpio_named_count(np, "cs-gpios"); > + if (num_cs < 0) > + return num_cs; This wasn't checked before and doesn't fit into the "cleanup non-DT paths"-category. Maybe note that in the commit log? Also did you change the type of i on purpose? > master = spi_alloc_master(&pdev->dev, > sizeof(*ddata) + num_cs * sizeof(unsigned)); > @@ -412,23 +414,7 @@ static int efm32_spi_probe(struct platform_device *pdev) > goto err; > } > > - ret = efm32_spi_probe_dt(pdev, master, ddata); > - if (ret > 0) { > - /* not created by device tree */ > - const struct efm32_spi_pdata *pdata = > - dev_get_platdata(&pdev->dev); > - > - if (pdata) > - ddata->pdata = *pdata; > - else > - ddata->pdata.location = > - efm32_spi_get_configured_location(ddata); > - > - master->bus_num = pdev->id; > - > - } else if (ret < 0) { > - goto err_disable_clk; > - } > + efm32_spi_probe_dt(pdev, master, ddata); This must still be: ret = efm32_spi_probe_dt(pdev, master, ddata); if (ret < 0) goto err_disable_clk; Best regards Uwe
2014-03-11 17:09 GMT+08:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>: > Hi Axel, > > On Mon, Mar 10, 2014 at 08:01:45AM +0800, Axel Lin wrote: >> This is a DT-only driver, so remove all non-DT paths. > ok in general, but I have a few comments, see below. > >> Signed-off-by: Axel Lin <axel.lin@ingics.com> >> --- >> drivers/spi/spi-efm32.c | 28 +++++++--------------------- >> 1 file changed, 7 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/spi/spi-efm32.c b/drivers/spi/spi-efm32.c >> index f53bbea..26e0362 100644 >> --- a/drivers/spi/spi-efm32.c >> +++ b/drivers/spi/spi-efm32.c >> @@ -294,9 +294,6 @@ static int efm32_spi_probe_dt(struct platform_device *pdev, >> u32 location; >> int ret; >> >> - if (!np) >> - return 1; >> - >> ret = of_property_read_u32(np, "location", &location); >> if (!ret) { >> dev_dbg(&pdev->dev, "using location %u\n", location); >> @@ -318,9 +315,14 @@ static int efm32_spi_probe(struct platform_device *pdev) >> int ret; >> struct spi_master *master; >> struct device_node *np = pdev->dev.of_node; >> - unsigned int num_cs, i; >> + int num_cs, i; >> + >> + if (!np) >> + return -EINVAL; >> >> num_cs = of_gpio_named_count(np, "cs-gpios"); >> + if (num_cs < 0) >> + return num_cs; > This wasn't checked before and doesn't fit into the "cleanup non-DT > paths"-category. Maybe note that in the commit log? Ok, will update commit log in v2. > Also did you change the type of i on purpose? It's a "doesn't matter" case to me. If you really like to keep i as unsigned int, I can leave it as unsigned int in v2. > >> master = spi_alloc_master(&pdev->dev, >> sizeof(*ddata) + num_cs * sizeof(unsigned)); >> @@ -412,23 +414,7 @@ static int efm32_spi_probe(struct platform_device *pdev) >> goto err; >> } >> >> - ret = efm32_spi_probe_dt(pdev, master, ddata); >> - if (ret > 0) { >> - /* not created by device tree */ >> - const struct efm32_spi_pdata *pdata = >> - dev_get_platdata(&pdev->dev); >> - >> - if (pdata) >> - ddata->pdata = *pdata; >> - else >> - ddata->pdata.location = >> - efm32_spi_get_configured_location(ddata); >> - >> - master->bus_num = pdev->id; >> - >> - } else if (ret < 0) { >> - goto err_disable_clk; >> - } >> + efm32_spi_probe_dt(pdev, master, ddata); > This must still be: efm32_spi_probe_dt() always return 0, why *must* check the return value here? > > ret = efm32_spi_probe_dt(pdev, master, ddata); > if (ret < 0) > goto err_disable_clk; > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Axel, On Tue, Mar 11, 2014 at 06:33:03PM +0800, Axel Lin wrote: > 2014-03-11 17:09 GMT+08:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>: > > On Mon, Mar 10, 2014 at 08:01:45AM +0800, Axel Lin wrote: > >> diff --git a/drivers/spi/spi-efm32.c b/drivers/spi/spi-efm32.c > >> index f53bbea..26e0362 100644 > >> --- a/drivers/spi/spi-efm32.c > >> +++ b/drivers/spi/spi-efm32.c > >> @@ -318,9 +315,14 @@ static int efm32_spi_probe(struct platform_device *pdev) > >> int ret; > >> struct spi_master *master; > >> struct device_node *np = pdev->dev.of_node; > >> - unsigned int num_cs, i; > >> + int num_cs, i; > >> + > >> + if (!np) > >> + return -EINVAL; > >> > >> num_cs = of_gpio_named_count(np, "cs-gpios"); > >> + if (num_cs < 0) > >> + return num_cs; > > This wasn't checked before and doesn't fit into the "cleanup non-DT > > paths"-category. Maybe note that in the commit log? > Ok, will update commit log in v2. > > > Also did you change the type of i on purpose? > It's a "doesn't matter" case to me. > If you really like to keep i as unsigned int, I can leave it as > unsigned int in v2. I'd always use an unsigned type for it, but I don't care much. > >> master = spi_alloc_master(&pdev->dev, > >> sizeof(*ddata) + num_cs * sizeof(unsigned)); > >> @@ -412,23 +414,7 @@ static int efm32_spi_probe(struct platform_device *pdev) > >> goto err; > >> } > >> > >> - ret = efm32_spi_probe_dt(pdev, master, ddata); > >> - if (ret > 0) { > >> - /* not created by device tree */ > >> - const struct efm32_spi_pdata *pdata = > >> - dev_get_platdata(&pdev->dev); > >> - > >> - if (pdata) > >> - ddata->pdata = *pdata; > >> - else > >> - ddata->pdata.location = > >> - efm32_spi_get_configured_location(ddata); > >> - > >> - master->bus_num = pdev->id; > >> - > >> - } else if (ret < 0) { > >> - goto err_disable_clk; > >> - } > >> + efm32_spi_probe_dt(pdev, master, ddata); > > This must still be: > efm32_spi_probe_dt() always return 0, why *must* check the return value here? Then also change efm32_spi_probe_dt to return void, or now that platform device support is gone, inline it into efm32_spi_probe. Best regards Uwe
diff --git a/drivers/spi/spi-efm32.c b/drivers/spi/spi-efm32.c index f53bbea..26e0362 100644 --- a/drivers/spi/spi-efm32.c +++ b/drivers/spi/spi-efm32.c @@ -294,9 +294,6 @@ static int efm32_spi_probe_dt(struct platform_device *pdev, u32 location; int ret; - if (!np) - return 1; - ret = of_property_read_u32(np, "location", &location); if (!ret) { dev_dbg(&pdev->dev, "using location %u\n", location); @@ -318,9 +315,14 @@ static int efm32_spi_probe(struct platform_device *pdev) int ret; struct spi_master *master; struct device_node *np = pdev->dev.of_node; - unsigned int num_cs, i; + int num_cs, i; + + if (!np) + return -EINVAL; num_cs = of_gpio_named_count(np, "cs-gpios"); + if (num_cs < 0) + return num_cs; master = spi_alloc_master(&pdev->dev, sizeof(*ddata) + num_cs * sizeof(unsigned)); @@ -412,23 +414,7 @@ static int efm32_spi_probe(struct platform_device *pdev) goto err; } - ret = efm32_spi_probe_dt(pdev, master, ddata); - if (ret > 0) { - /* not created by device tree */ - const struct efm32_spi_pdata *pdata = - dev_get_platdata(&pdev->dev); - - if (pdata) - ddata->pdata = *pdata; - else - ddata->pdata.location = - efm32_spi_get_configured_location(ddata); - - master->bus_num = pdev->id; - - } else if (ret < 0) { - goto err_disable_clk; - } + efm32_spi_probe_dt(pdev, master, ddata); efm32_spi_write32(ddata, 0, REG_IEN); efm32_spi_write32(ddata, REG_ROUTE_TXPEN | REG_ROUTE_RXPEN |
This is a DT-only driver, so remove all non-DT paths. Signed-off-by: Axel Lin <axel.lin@ingics.com> --- drivers/spi/spi-efm32.c | 28 +++++++--------------------- 1 file changed, 7 insertions(+), 21 deletions(-)