| Message ID | 1370168531-22718-1-git-send-email-shc_work@mail.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Jun 02, 2013 at 02:22:07PM +0400, Alexander Shiyan wrote: > @@ -869,56 +858,34 @@ static int spi_imx_probe(struct platform_device *pdev) > > master->dev.of_node = pdev->dev.of_node; > ret = spi_bitbang_start(&spi_imx->bitbang); > - if (ret) { > - dev_err(&pdev->dev, "bitbang start failed with %d\n", ret); > - goto out_clk_put; > - } > + if (!ret) > + return 0; > > - dev_info(&pdev->dev, "probed\n"); > + dev_err(&pdev->dev, "bitbang start failed with %d\n", ret); > > - return ret; > - > -out_clk_put: > clk_disable_unprepare(spi_imx->clk_per); > clk_disable_unprepare(spi_imx->clk_ipg); It seems that my comment on v1 is confusing. I meant the above code change is completely a diff stat churn to me. Shawn
On Sun, Jun 02, 2013 at 02:22:08PM +0400, Alexander Shiyan wrote: > Patch reuse existing "cs_gpios" field for master and "cs_gpio" for devices. > It seems that the patch does a little bit more than these. At least, it reuses chip_select for spi_device as well. > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > --- > drivers/spi/spi-imx.c | 76 +++++++++++++++++++++++++-------------------------- > 1 file changed, 37 insertions(+), 39 deletions(-) > > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c > index 1a53e7d..65f73d6 100644 > --- a/drivers/spi/spi-imx.c > +++ b/drivers/spi/spi-imx.c > @@ -56,7 +56,6 @@ struct spi_imx_config { > unsigned int speed_hz; > unsigned int bpw; > unsigned int mode; > - u8 cs; > }; > > enum spi_imx_devtype { > @@ -72,7 +71,7 @@ struct spi_imx_data; > > struct spi_imx_devtype_data { > void (*intctrl)(struct spi_imx_data *, int); > - int (*config)(struct spi_imx_data *, struct spi_imx_config *); > + int (*config)(struct spi_device *, struct spi_imx_config *); > void (*trigger)(struct spi_imx_data *); > int (*rx_available)(struct spi_imx_data *); > void (*reset)(struct spi_imx_data *); > @@ -96,7 +95,6 @@ struct spi_imx_data { > unsigned int txfifo; /* number of words pushed in tx FIFO */ > > const struct spi_imx_devtype_data *devtype_data; > - int chipselect[0]; > }; > > static inline int is_imx27_cspi(struct spi_imx_data *d) > @@ -259,9 +257,10 @@ static void __maybe_unused mx51_ecspi_trigger(struct spi_imx_data *spi_imx) > writel(reg, spi_imx->base + MX51_ECSPI_CTRL); > } > > -static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx, > +static int __maybe_unused mx51_ecspi_config(struct spi_device *spi, > struct spi_imx_config *config) > { > + struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master); > u32 ctrl = MX51_ECSPI_CTRL_ENABLE, cfg = 0; > > /* > @@ -277,21 +276,21 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx, > ctrl |= mx51_ecspi_clkdiv(spi_imx->spi_clk, config->speed_hz); > > /* set chip select to use */ > - ctrl |= MX51_ECSPI_CTRL_CS(config->cs); > + ctrl |= MX51_ECSPI_CTRL_CS(spi->chip_select); > > ctrl |= (config->bpw - 1) << MX51_ECSPI_CTRL_BL_OFFSET; > > - cfg |= MX51_ECSPI_CONFIG_SBBCTRL(config->cs); > + cfg |= MX51_ECSPI_CONFIG_SBBCTRL(spi->chip_select); > > if (config->mode & SPI_CPHA) > - cfg |= MX51_ECSPI_CONFIG_SCLKPHA(config->cs); > + cfg |= MX51_ECSPI_CONFIG_SCLKPHA(spi->chip_select); > > if (config->mode & SPI_CPOL) { > - cfg |= MX51_ECSPI_CONFIG_SCLKPOL(config->cs); > - cfg |= MX51_ECSPI_CONFIG_SCLKCTL(config->cs); > + cfg |= MX51_ECSPI_CONFIG_SCLKPOL(spi->chip_select); > + cfg |= MX51_ECSPI_CONFIG_SCLKCTL(spi->chip_select); > } > if (config->mode & SPI_CS_HIGH) > - cfg |= MX51_ECSPI_CONFIG_SSBPOL(config->cs); > + cfg |= MX51_ECSPI_CONFIG_SSBPOL(spi->chip_select); > > writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL); > writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG); <snip> > @@ -772,11 +767,17 @@ static int spi_imx_probe(struct platform_device *pdev) > return ret; > } > > - master = spi_alloc_master(&pdev->dev, > - sizeof(struct spi_imx_data) + sizeof(int) * num_cs); > + master = spi_alloc_master(&pdev->dev, sizeof(struct spi_imx_data)); > if (!master) > return -ENOMEM; > > + master->cs_gpios = devm_kzalloc(&pdev->dev, sizeof(int) * num_cs, > + GFP_KERNEL); > + if (!master->cs_gpios) { > + ret = -ENOMEM; > + goto out_master_free; > + } > + For DT case, of_spi_register_master() will set master->cs_gpios up over again with the memory it allocates, so the memory allocated here will be leaked? Shawn > platform_set_drvdata(pdev, master); > > master->bits_per_word_mask = SPI_BPW_RANGE_MASK(1, 32); > @@ -791,14 +792,10 @@ static int spi_imx_probe(struct platform_device *pdev) > if (!gpio_is_valid(cs_gpio) && mxc_platform_info) > cs_gpio = mxc_platform_info->chipselect[i]; > > - spi_imx->chipselect[i] = cs_gpio; > - if (!gpio_is_valid(cs_gpio)) > - continue; > - > - ret = devm_gpio_request(&pdev->dev, spi_imx->chipselect[i], > - DRIVER_NAME); > + master->cs_gpios[i] = cs_gpio; > + ret = devm_gpio_request(&pdev->dev, cs_gpio, DRIVER_NAME); > if (ret) { > - dev_err(&pdev->dev, "can't get cs gpios\n"); > + dev_err(&pdev->dev, "can't get cs gpio %i\n", cs_gpio); > goto out_master_put; > } > }
On Sun, Jun 02, 2013 at 02:22:09PM +0400, Alexander Shiyan wrote: > The SPI driver is being compiled for all i.MX platforms, so "maybe_unused" > attributes is no longer needed. This patch removes these attributes. > Though I'm not the fan of using __maybe_unused, the commit log is not quite correct. It's not true that all i.MX platforms have to be compiled together. I can easily build a kernel for a particular i.MX platform by deselect other platforms from defconfig. Shawn > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > --- > drivers/spi/spi-imx.c | 45 +++++++++++++++++++++------------------------ > 1 file changed, 21 insertions(+), 24 deletions(-) > > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c > index 65f73d6..3b703f8 100644 > --- a/drivers/spi/spi-imx.c > +++ b/drivers/spi/spi-imx.c > @@ -235,7 +235,7 @@ static unsigned int mx51_ecspi_clkdiv(unsigned int fin, unsigned int fspi) > (post << MX51_ECSPI_CTRL_POSTDIV_OFFSET); > } > > -static void __maybe_unused mx51_ecspi_intctrl(struct spi_imx_data *spi_imx, int enable) > +static void mx51_ecspi_intctrl(struct spi_imx_data *spi_imx, int enable) > { > unsigned val = 0; > > @@ -248,7 +248,7 @@ static void __maybe_unused mx51_ecspi_intctrl(struct spi_imx_data *spi_imx, int > writel(val, spi_imx->base + MX51_ECSPI_INT); > } > > -static void __maybe_unused mx51_ecspi_trigger(struct spi_imx_data *spi_imx) > +static void mx51_ecspi_trigger(struct spi_imx_data *spi_imx) > { > u32 reg; > > @@ -257,8 +257,8 @@ static void __maybe_unused mx51_ecspi_trigger(struct spi_imx_data *spi_imx) > writel(reg, spi_imx->base + MX51_ECSPI_CTRL); > } > > -static int __maybe_unused mx51_ecspi_config(struct spi_device *spi, > - struct spi_imx_config *config) > +static int mx51_ecspi_config(struct spi_device *spi, > + struct spi_imx_config *config) > { > struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master); > u32 ctrl = MX51_ECSPI_CTRL_ENABLE, cfg = 0; > @@ -298,12 +298,12 @@ static int __maybe_unused mx51_ecspi_config(struct spi_device *spi, > return 0; > } > > -static int __maybe_unused mx51_ecspi_rx_available(struct spi_imx_data *spi_imx) > +static int mx51_ecspi_rx_available(struct spi_imx_data *spi_imx) > { > return readl(spi_imx->base + MX51_ECSPI_STAT) & MX51_ECSPI_STAT_RR; > } > > -static void __maybe_unused mx51_ecspi_reset(struct spi_imx_data *spi_imx) > +static void mx51_ecspi_reset(struct spi_imx_data *spi_imx) > { > /* drain receive buffer */ > while (mx51_ecspi_rx_available(spi_imx)) > @@ -333,7 +333,7 @@ static void __maybe_unused mx51_ecspi_reset(struct spi_imx_data *spi_imx) > * the i.MX35 has a slightly different register layout for bits > * we do not use here. > */ > -static void __maybe_unused mx31_intctrl(struct spi_imx_data *spi_imx, int enable) > +static void mx31_intctrl(struct spi_imx_data *spi_imx, int enable) > { > unsigned int val = 0; > > @@ -345,7 +345,7 @@ static void __maybe_unused mx31_intctrl(struct spi_imx_data *spi_imx, int enable > writel(val, spi_imx->base + MXC_CSPIINT); > } > > -static void __maybe_unused mx31_trigger(struct spi_imx_data *spi_imx) > +static void mx31_trigger(struct spi_imx_data *spi_imx) > { > unsigned int reg; > > @@ -354,8 +354,7 @@ static void __maybe_unused mx31_trigger(struct spi_imx_data *spi_imx) > writel(reg, spi_imx->base + MXC_CSPICTRL); > } > > -static int __maybe_unused mx31_config(struct spi_device *spi, > - struct spi_imx_config *config) > +static int mx31_config(struct spi_device *spi, struct spi_imx_config *config) > { > struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master); > unsigned int reg = MX31_CSPICTRL_ENABLE | MX31_CSPICTRL_MASTER; > @@ -386,12 +385,12 @@ static int __maybe_unused mx31_config(struct spi_device *spi, > return 0; > } > > -static int __maybe_unused mx31_rx_available(struct spi_imx_data *spi_imx) > +static int mx31_rx_available(struct spi_imx_data *spi_imx) > { > return readl(spi_imx->base + MX31_CSPISTATUS) & MX31_STATUS_RR; > } > > -static void __maybe_unused mx31_reset(struct spi_imx_data *spi_imx) > +static void mx31_reset(struct spi_imx_data *spi_imx) > { > /* drain receive buffer */ > while (readl(spi_imx->base + MX31_CSPISTATUS) & MX31_STATUS_RR) > @@ -411,7 +410,7 @@ static void __maybe_unused mx31_reset(struct spi_imx_data *spi_imx) > #define MX21_CSPICTRL_DR_SHIFT 14 > #define MX21_CSPICTRL_CS_SHIFT 19 > > -static void __maybe_unused mx21_intctrl(struct spi_imx_data *spi_imx, int enable) > +static void mx21_intctrl(struct spi_imx_data *spi_imx, int enable) > { > unsigned int val = 0; > > @@ -423,7 +422,7 @@ static void __maybe_unused mx21_intctrl(struct spi_imx_data *spi_imx, int enable > writel(val, spi_imx->base + MXC_CSPIINT); > } > > -static void __maybe_unused mx21_trigger(struct spi_imx_data *spi_imx) > +static void mx21_trigger(struct spi_imx_data *spi_imx) > { > unsigned int reg; > > @@ -432,8 +431,7 @@ static void __maybe_unused mx21_trigger(struct spi_imx_data *spi_imx) > writel(reg, spi_imx->base + MXC_CSPICTRL); > } > > -static int __maybe_unused mx21_config(struct spi_device *spi, > - struct spi_imx_config *config) > +static int mx21_config(struct spi_device *spi, struct spi_imx_config *config) > { > struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master); > unsigned int reg = MX21_CSPICTRL_ENABLE | MX21_CSPICTRL_MASTER; > @@ -457,12 +455,12 @@ static int __maybe_unused mx21_config(struct spi_device *spi, > return 0; > } > > -static int __maybe_unused mx21_rx_available(struct spi_imx_data *spi_imx) > +static int mx21_rx_available(struct spi_imx_data *spi_imx) > { > return readl(spi_imx->base + MXC_CSPIINT) & MX21_INTREG_RR; > } > > -static void __maybe_unused mx21_reset(struct spi_imx_data *spi_imx) > +static void mx21_reset(struct spi_imx_data *spi_imx) > { > writel(1, spi_imx->base + MXC_RESET); > } > @@ -478,7 +476,7 @@ static void __maybe_unused mx21_reset(struct spi_imx_data *spi_imx) > #define MX1_CSPICTRL_MASTER (1 << 10) > #define MX1_CSPICTRL_DR_SHIFT 13 > > -static void __maybe_unused mx1_intctrl(struct spi_imx_data *spi_imx, int enable) > +static void mx1_intctrl(struct spi_imx_data *spi_imx, int enable) > { > unsigned int val = 0; > > @@ -490,7 +488,7 @@ static void __maybe_unused mx1_intctrl(struct spi_imx_data *spi_imx, int enable) > writel(val, spi_imx->base + MXC_CSPIINT); > } > > -static void __maybe_unused mx1_trigger(struct spi_imx_data *spi_imx) > +static void mx1_trigger(struct spi_imx_data *spi_imx) > { > unsigned int reg; > > @@ -499,8 +497,7 @@ static void __maybe_unused mx1_trigger(struct spi_imx_data *spi_imx) > writel(reg, spi_imx->base + MXC_CSPICTRL); > } > > -static int __maybe_unused mx1_config(struct spi_device *spi, > - struct spi_imx_config *config) > +static int mx1_config(struct spi_device *spi, struct spi_imx_config *config) > { > struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master); > unsigned int reg = MX1_CSPICTRL_ENABLE | MX1_CSPICTRL_MASTER; > @@ -519,12 +516,12 @@ static int __maybe_unused mx1_config(struct spi_device *spi, > return 0; > } > > -static int __maybe_unused mx1_rx_available(struct spi_imx_data *spi_imx) > +static int mx1_rx_available(struct spi_imx_data *spi_imx) > { > return readl(spi_imx->base + MXC_CSPIINT) & MX1_INTREG_RR; > } > > -static void __maybe_unused mx1_reset(struct spi_imx_data *spi_imx) > +static void mx1_reset(struct spi_imx_data *spi_imx) > { > writel(1, spi_imx->base + MXC_RESET); > } > -- > 1.8.1.5 >
diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index 7db4f43..1a53e7d 100644 --- a/drivers/spi/spi-imx.c +++ b/drivers/spi/spi-imx.c @@ -84,7 +84,6 @@ struct spi_imx_data { struct completion xfer_done; void __iomem *base; - int irq; struct clk *clk_per; struct clk *clk_ipg; unsigned long spi_clk; @@ -758,7 +757,7 @@ static int spi_imx_probe(struct platform_device *pdev) struct spi_master *master; struct spi_imx_data *spi_imx; struct resource *res; - int i, ret, num_cs; + int i, irq, ret, num_cs; if (!np && !mxc_platform_info) { dev_err(&pdev->dev, "can't get the platform data\n"); @@ -796,10 +795,11 @@ static int spi_imx_probe(struct platform_device *pdev) if (!gpio_is_valid(cs_gpio)) continue; - ret = gpio_request(spi_imx->chipselect[i], DRIVER_NAME); + ret = devm_gpio_request(&pdev->dev, spi_imx->chipselect[i], + DRIVER_NAME); if (ret) { dev_err(&pdev->dev, "can't get cs gpios\n"); - goto out_gpio_free; + goto out_master_put; } } @@ -816,46 +816,35 @@ static int spi_imx_probe(struct platform_device *pdev) (struct spi_imx_devtype_data *) pdev->id_entry->driver_data; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!res) { - dev_err(&pdev->dev, "can't get platform resource\n"); - ret = -ENOMEM; - goto out_gpio_free; + spi_imx->base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(spi_imx->base)) { + ret = PTR_ERR(spi_imx->base); + goto out_master_put; } - if (!request_mem_region(res->start, resource_size(res), pdev->name)) { - dev_err(&pdev->dev, "request_mem_region failed\n"); - ret = -EBUSY; - goto out_gpio_free; - } - - spi_imx->base = ioremap(res->start, resource_size(res)); - if (!spi_imx->base) { - ret = -EINVAL; - goto out_release_mem; - } - - spi_imx->irq = platform_get_irq(pdev, 0); - if (spi_imx->irq < 0) { + irq = platform_get_irq(pdev, 0); + if (irq < 0) { ret = -EINVAL; - goto out_iounmap; + goto out_master_put; } - ret = request_irq(spi_imx->irq, spi_imx_isr, 0, DRIVER_NAME, spi_imx); + ret = devm_request_irq(&pdev->dev, irq, spi_imx_isr, 0, DRIVER_NAME, + spi_imx); if (ret) { - dev_err(&pdev->dev, "can't get irq%d: %d\n", spi_imx->irq, ret); - goto out_iounmap; + dev_err(&pdev->dev, "can't get irq%d: %d\n", irq, ret); + goto out_master_put; } spi_imx->clk_ipg = devm_clk_get(&pdev->dev, "ipg"); if (IS_ERR(spi_imx->clk_ipg)) { ret = PTR_ERR(spi_imx->clk_ipg); - goto out_free_irq; + goto out_master_put; } spi_imx->clk_per = devm_clk_get(&pdev->dev, "per"); if (IS_ERR(spi_imx->clk_per)) { ret = PTR_ERR(spi_imx->clk_per); - goto out_free_irq; + goto out_master_put; } clk_prepare_enable(spi_imx->clk_per); @@ -869,56 +858,34 @@ static int spi_imx_probe(struct platform_device *pdev) master->dev.of_node = pdev->dev.of_node; ret = spi_bitbang_start(&spi_imx->bitbang); - if (ret) { - dev_err(&pdev->dev, "bitbang start failed with %d\n", ret); - goto out_clk_put; - } + if (!ret) + return 0; - dev_info(&pdev->dev, "probed\n"); + dev_err(&pdev->dev, "bitbang start failed with %d\n", ret); - return ret; - -out_clk_put: clk_disable_unprepare(spi_imx->clk_per); clk_disable_unprepare(spi_imx->clk_ipg); -out_free_irq: - free_irq(spi_imx->irq, spi_imx); -out_iounmap: - iounmap(spi_imx->base); -out_release_mem: - release_mem_region(res->start, resource_size(res)); -out_gpio_free: - while (--i >= 0) { - if (gpio_is_valid(spi_imx->chipselect[i])) - gpio_free(spi_imx->chipselect[i]); - } + +out_master_put: spi_master_put(master); kfree(master); + return ret; } static int spi_imx_remove(struct platform_device *pdev) { struct spi_master *master = platform_get_drvdata(pdev); - struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); struct spi_imx_data *spi_imx = spi_master_get_devdata(master); - int i; spi_bitbang_stop(&spi_imx->bitbang); writel(0, spi_imx->base + MXC_CSPICTRL); clk_disable_unprepare(spi_imx->clk_per); clk_disable_unprepare(spi_imx->clk_ipg); - free_irq(spi_imx->irq, spi_imx); - iounmap(spi_imx->base); - - for (i = 0; i < master->num_chipselect; i++) - if (gpio_is_valid(spi_imx->chipselect[i])) - gpio_free(spi_imx->chipselect[i]); spi_master_put(master); - - release_mem_region(res->start, resource_size(res)); + kfree(master); return 0; }
Use devm_* functions for the driver. This ensures more consistent error values and simplifies error paths. Signed-off-by: Alexander Shiyan <shc_work@mail.ru> --- drivers/spi/spi-imx.c | 81 +++++++++++++++------------------------------------ 1 file changed, 24 insertions(+), 57 deletions(-)