Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1664440 > unrolled thread

[PATCH v2 2/4] spi: rockchip: add support for "cs-gpios" dts property

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 2/4] spi: rockchip: add support for "cs-gpios" dts property Jeffy Chen <jeffy.chen@rock-chips.com> - 2017-06-13 07:30 +0200
    Re: [PATCH v2 2/4] spi: rockchip: add support for "cs-gpios" dts  property kbuild test robot <lkp@intel.com> - 2017-06-13 19:30 +0200
    Re: [PATCH v2 2/4] spi: rockchip: add support for "cs-gpios" dts  property Brian Norris <briannorris@chromium.org> - 2017-06-13 19:40 +0200
      Re: [PATCH v2 2/4] spi: rockchip: add support for "cs-gpios" dts  property jeffy <jeffy.chen@rock-chips.com> - 2017-06-14 03:30 +0200

#1664440 — [PATCH v2 2/4] spi: rockchip: add support for "cs-gpios" dts property

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);
+	}
+
+	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");
-- 
2.1.4

[toc] | [next] | [standalone]

#1665047 — Re: [PATCH v2 2/4] spi: rockchip: add support for "cs-gpios" dts property

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1665049 — Re: [PATCH v2 2/4] spi: rockchip: add support for "cs-gpios" dts property

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
> 
> 

[toc] | [prev] | [next] | [standalone]

#1665352 — Re: [PATCH v2 2/4] spi: rockchip: add support for "cs-gpios" dts property

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
>>
>>
>
>
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web