| Message ID | 1497331543-8565-2-git-send-email-jeffy.chen@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jeffy,
[auto build test ERROR on rockchip/for-next]
[also build test ERROR on v4.12-rc5 next-20170613]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Jeffy-Chen/spi-rockchip-fix-error-handling-when-probe/20170613-172725
base: https://git.kernel.org/pub/scm/linux/kernel/git/mmind/linux-rockchip.git for-next
config: x86_64-randconfig-s5-06132355 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
drivers/spi/spi-rockchip.c: In function 'rockchip_spi_setup_cs_gpios':
>> drivers/spi/spi-rockchip.c:678:13: error: implicit declaration of function 'devm_gpiod_get_index_optional' [-Werror=implicit-function-declaration]
cs_gpio = devm_gpiod_get_index_optional(dev, "cs",
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/spi/spi-rockchip.c:679:11: error: 'GPIOD_ASIS' undeclared (first use in this function)
i, GPIOD_ASIS);
^~~~~~~~~~
drivers/spi/spi-rockchip.c:679:11: note: each undeclared identifier is reported only once for each function it appears in
cc1: some warnings being treated as errors
vim +/devm_gpiod_get_index_optional +678 drivers/spi/spi-rockchip.c
672 if (!np)
673 return 0;
674
675 nb = of_gpio_named_count(np, "cs-gpios");
676 for (i = 0; i < nb; i++) {
677 /* We support both GPIO CS and HW CS */
> 678 cs_gpio = devm_gpiod_get_index_optional(dev, "cs",
> 679 i, GPIOD_ASIS);
680 if (IS_ERR(cs_gpio))
681 return PTR_ERR(cs_gpio);
682 }
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Jeffy, On Tue, Jun 13, 2017 at 01:25:41PM +0800, Jeffy Chen wrote: > Support using "cs-gpios" property to specify cs gpios. > > Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> > 1/ request cs gpios in probe for better error handling > 2/ use gpiod* function > (suggested by Heiko Stuebner) > > 3/ split dt-binding changes to new patch > (suggested by Shawn Lin & Heiko Stuebner) > > --- > > Changes in v2: None > > drivers/spi/spi-rockchip.c | 30 +++++++++++++++++++++++++++++- > 1 file changed, 29 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c > index bab9b13..ad8997b 100644 > --- a/drivers/spi/spi-rockchip.c > +++ b/drivers/spi/spi-rockchip.c > @@ -16,7 +16,7 @@ > #include <linux/clk.h> > #include <linux/dmaengine.h> > #include <linux/module.h> > -#include <linux/of.h> > +#include <linux/of_gpio.h> > #include <linux/pinctrl/consumer.h> > #include <linux/platform_device.h> > #include <linux/spi/spi.h> > @@ -663,6 +663,27 @@ static bool rockchip_spi_can_dma(struct spi_master *master, > return (xfer->len > rs->fifo_len); > } > > +static int rockchip_spi_setup_cs_gpios(struct device *dev) > +{ > + struct device_node *np = dev->of_node; > + struct gpio_desc *cs_gpio; > + int i, nb; > + > + if (!np) > + return 0; > + > + nb = of_gpio_named_count(np, "cs-gpios"); > + for (i = 0; i < nb; i++) { > + /* We support both GPIO CS and HW CS */ > + cs_gpio = devm_gpiod_get_index_optional(dev, "cs", > + i, GPIOD_ASIS); > + if (IS_ERR(cs_gpio)) > + return PTR_ERR(cs_gpio); I'm a bit confused why you need this function at all. You aren't using the references that you're grabbing here, so essentially this is just error-checking. Are you doing anything here that isn't covered in of_spi_register_master()? > + } > + > + return 0; > +} > + > static int rockchip_spi_probe(struct platform_device *pdev) > { > int ret = 0; > @@ -749,6 +770,7 @@ static int rockchip_spi_probe(struct platform_device *pdev) > master->transfer_one = rockchip_spi_transfer_one; > master->max_transfer_size = rockchip_spi_max_transfer_size; > master->handle_err = rockchip_spi_handle_err; > + master->flags = SPI_MASTER_GPIO_SS; I'm curious, do you actually need to assert both the HW and GPIO CS? Brian > > rs->dma_tx.ch = dma_request_chan(rs->dev, "tx"); > if (IS_ERR(rs->dma_tx.ch)) { > @@ -783,6 +805,12 @@ static int rockchip_spi_probe(struct platform_device *pdev) > master->dma_rx = rs->dma_rx.ch; > } > > + ret = rockchip_spi_setup_cs_gpios(&pdev->dev); > + if (ret) { > + dev_err(&pdev->dev, "Failed to setup cs gpios\n"); > + goto err_free_dma_rx; > + } > + > ret = devm_spi_register_master(&pdev->dev, master); > if (ret) { > dev_err(&pdev->dev, "Failed to register master\n"); > -- > 2.1.4 > > -- 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
Hi Brian, Thanx for your comments :) On 06/14/2017 01:33 AM, Brian Norris wrote: > Hi Jeffy, > > On Tue, Jun 13, 2017 at 01:25:41PM +0800, Jeffy Chen wrote: >> Support using "cs-gpios" property to specify cs gpios. >> >> Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> >> 1/ request cs gpios in probe for better error handling >> 2/ use gpiod* function >> (suggested by Heiko Stuebner) >> >> 3/ split dt-binding changes to new patch >> (suggested by Shawn Lin & Heiko Stuebner) >> >> --- >> >> Changes in v2: None >> >> drivers/spi/spi-rockchip.c | 30 +++++++++++++++++++++++++++++- >> 1 file changed, 29 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/spi/spi-rockchip.c b/drivers/spi/spi-rockchip.c >> index bab9b13..ad8997b 100644 >> --- a/drivers/spi/spi-rockchip.c >> +++ b/drivers/spi/spi-rockchip.c >> @@ -16,7 +16,7 @@ >> #include <linux/clk.h> >> #include <linux/dmaengine.h> >> #include <linux/module.h> >> -#include <linux/of.h> >> +#include <linux/of_gpio.h> >> #include <linux/pinctrl/consumer.h> >> #include <linux/platform_device.h> >> #include <linux/spi/spi.h> >> @@ -663,6 +663,27 @@ static bool rockchip_spi_can_dma(struct spi_master *master, >> return (xfer->len > rs->fifo_len); >> } >> >> +static int rockchip_spi_setup_cs_gpios(struct device *dev) >> +{ >> + struct device_node *np = dev->of_node; >> + struct gpio_desc *cs_gpio; >> + int i, nb; >> + >> + if (!np) >> + return 0; >> + >> + nb = of_gpio_named_count(np, "cs-gpios"); >> + for (i = 0; i < nb; i++) { >> + /* We support both GPIO CS and HW CS */ >> + cs_gpio = devm_gpiod_get_index_optional(dev, "cs", >> + i, GPIOD_ASIS); >> + if (IS_ERR(cs_gpio)) >> + return PTR_ERR(cs_gpio); > > I'm a bit confused why you need this function at all. You aren't using > the references that you're grabbing here, so essentially this is just > error-checking. actually this is error-checking plus gpiod_request(see gpiod_get_index in gpiolib.c) > > Are you doing anything here that isn't covered in > of_spi_register_master()? expect for gpiod_request, another difference would be when the of_spi_register_master calls of_get_named_gpio to parse cs-gpios, it would not do error handling here(fallback to HW CS): for (i = 0; i < nb; i++) cs[i] = of_get_named_gpio(np, "cs-gpios", i); but in our case, if something wrong happens(except for ENOENT), we cannot fallback to HW CS, because we already let pinctrl config GPIO CS. > >> + } >> + >> + return 0; >> +} >> + >> static int rockchip_spi_probe(struct platform_device *pdev) >> { >> int ret = 0; >> @@ -749,6 +770,7 @@ static int rockchip_spi_probe(struct platform_device *pdev) >> master->transfer_one = rockchip_spi_transfer_one; >> master->max_transfer_size = rockchip_spi_max_transfer_size; >> master->handle_err = rockchip_spi_handle_err; >> + master->flags = SPI_MASTER_GPIO_SS; > > I'm curious, do you actually need to assert both the HW and GPIO CS? yes, it would hang if we do spi xfer with wrong HW CS register config(seems to be another controller limit)... > > Brian > >> >> rs->dma_tx.ch = dma_request_chan(rs->dev, "tx"); >> if (IS_ERR(rs->dma_tx.ch)) { >> @@ -783,6 +805,12 @@ static int rockchip_spi_probe(struct platform_device *pdev) >> master->dma_rx = rs->dma_rx.ch; >> } >> >> + ret = rockchip_spi_setup_cs_gpios(&pdev->dev); >> + if (ret) { >> + dev_err(&pdev->dev, "Failed to setup cs gpios\n"); >> + goto err_free_dma_rx; >> + } >> + >> ret = devm_spi_register_master(&pdev->dev, master); >> if (ret) { >> dev_err(&pdev->dev, "Failed to register master\n"); >> -- >> 2.1.4 >> >> > > > -- 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-rockchip.c b/drivers/spi/spi-rockchip.c index bab9b13..ad8997b 100644 --- a/drivers/spi/spi-rockchip.c +++ b/drivers/spi/spi-rockchip.c @@ -16,7 +16,7 @@ #include <linux/clk.h> #include <linux/dmaengine.h> #include <linux/module.h> -#include <linux/of.h> +#include <linux/of_gpio.h> #include <linux/pinctrl/consumer.h> #include <linux/platform_device.h> #include <linux/spi/spi.h> @@ -663,6 +663,27 @@ static bool rockchip_spi_can_dma(struct spi_master *master, return (xfer->len > rs->fifo_len); } +static int rockchip_spi_setup_cs_gpios(struct device *dev) +{ + struct device_node *np = dev->of_node; + struct gpio_desc *cs_gpio; + int i, nb; + + if (!np) + return 0; + + nb = of_gpio_named_count(np, "cs-gpios"); + for (i = 0; i < nb; i++) { + /* We support both GPIO CS and HW CS */ + cs_gpio = devm_gpiod_get_index_optional(dev, "cs", + i, GPIOD_ASIS); + if (IS_ERR(cs_gpio)) + return PTR_ERR(cs_gpio); + } + + return 0; +} + static int rockchip_spi_probe(struct platform_device *pdev) { int ret = 0; @@ -749,6 +770,7 @@ static int rockchip_spi_probe(struct platform_device *pdev) master->transfer_one = rockchip_spi_transfer_one; master->max_transfer_size = rockchip_spi_max_transfer_size; master->handle_err = rockchip_spi_handle_err; + master->flags = SPI_MASTER_GPIO_SS; rs->dma_tx.ch = dma_request_chan(rs->dev, "tx"); if (IS_ERR(rs->dma_tx.ch)) { @@ -783,6 +805,12 @@ static int rockchip_spi_probe(struct platform_device *pdev) master->dma_rx = rs->dma_rx.ch; } + ret = rockchip_spi_setup_cs_gpios(&pdev->dev); + if (ret) { + dev_err(&pdev->dev, "Failed to setup cs gpios\n"); + goto err_free_dma_rx; + } + ret = devm_spi_register_master(&pdev->dev, master); if (ret) { dev_err(&pdev->dev, "Failed to register master\n");
Support using "cs-gpios" property to specify cs gpios. Signed-off-by: Jeffy Chen <jeffy.chen@rock-chips.com> 1/ request cs gpios in probe for better error handling 2/ use gpiod* function (suggested by Heiko Stuebner) 3/ split dt-binding changes to new patch (suggested by Shawn Lin & Heiko Stuebner) --- Changes in v2: None drivers/spi/spi-rockchip.c | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-)