| Message ID | 1457513242-11202-2-git-send-email-shubhraj@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 09, 2016 at 02:17:21PM +0530, Shubhrajyoti Datta wrote: > + xspi->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(xspi->clk)) { As someone pointed out on the previous version of the series this will cause the driver to fail to probe with existing DTs. We probably need to explicitly handle a -ENOENT as a "this clock will never appear" or something. This also requests a single nameless clock but someone pointed out on the previous version there are multiple clocks into the IP. Even if you only want to add one clock right now the clock should probably be named so we can scale up. > + } > + ret = clk_prepare_enable(xspi->clk); Missing blank line here. > + if (ret) > + dev_err(&pdev->dev, "Unable to enable clock.\n"); > + This isn't really checking the return code - if we failed to enable the clock we should be failing the probe, not just carrying on.
> -----Original Message----- > From: Mark Brown [mailto:broonie@kernel.org] > Sent: Thursday, March 10, 2016 9:00 AM > To: Shubhrajyoti Datta > Cc: linux-spi@vger.kernel.org; Soren Brinkmann; > devicetree@vger.kernel.org; Michal Simek; Shubhrajyoti Datta > Subject: Re: [PATCHv2 2/3] spi/spi-xilinx: Add clock support > > On Wed, Mar 09, 2016 at 02:17:21PM +0530, Shubhrajyoti Datta wrote: > > > + xspi->clk = devm_clk_get(&pdev->dev, NULL); > > + if (IS_ERR(xspi->clk)) { > > As someone pointed out on the previous version of the series this will cause > the driver to fail to probe with existing DTs. We probably need to explicitly > handle a -ENOENT as a "this clock will never appear" or something. I got comments after I sent the v2. I will update it with Lars comments. > > This also requests a single nameless clock but someone pointed out on the > previous version there are multiple clocks into the IP. Even if you only want > to add one clock right now the clock should probably be named so we can > scale up I will add a name. . > > > + } > > + ret = clk_prepare_enable(xspi->clk); > > Missing blank line here. Will fix in next version > > > + if (ret) > > + dev_err(&pdev->dev, "Unable to enable clock.\n"); > > + > > This isn't really checking the return code - if we failed to enable the clock we > should be failing the probe, not just carrying on. Will fix. -- 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
diff --git a/drivers/spi/spi-xilinx.c b/drivers/spi/spi-xilinx.c index 3009121..7e12338 100644 --- a/drivers/spi/spi-xilinx.c +++ b/drivers/spi/spi-xilinx.c @@ -21,6 +21,7 @@ #include <linux/spi/spi_bitbang.h> #include <linux/spi/xilinx_spi.h> #include <linux/io.h> +#include <linux/clk.h> #define XILINX_SPI_MAX_CS 32 @@ -83,6 +84,7 @@ struct xilinx_spi { struct spi_bitbang bitbang; struct completion done; void __iomem *regs; /* virt. address of the control registers */ + struct clk *clk; int irq; @@ -428,6 +430,15 @@ static int xilinx_spi_probe(struct platform_device *pdev) goto put_master; } + xspi->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(xspi->clk)) { + dev_err(&pdev->dev, "input clock not found.\n"); + return PTR_ERR(xspi->clk); + } + ret = clk_prepare_enable(xspi->clk); + if (ret) + dev_err(&pdev->dev, "Unable to enable clock.\n"); + master->bus_num = pdev->id; master->num_chipselect = num_cs; master->dev.of_node = pdev->dev.of_node; @@ -485,6 +496,7 @@ static int xilinx_spi_probe(struct platform_device *pdev) put_master: spi_master_put(master); + clk_disable_unprepare(xspi->clk); return ret; } @@ -503,6 +515,7 @@ static int xilinx_spi_remove(struct platform_device *pdev) xspi->write_fn(0, regs_base + XIPIF_V123B_DGIER_OFFSET); spi_master_put(xspi->bitbang.master); + clk_disable_unprepare(xspi->clk); return 0; }