linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] spi: qup: Fix cs-num DT property parsing
@ 2015-03-06 15:26 Ivan T. Ivanov
  2015-03-06 15:26 ` [PATCH 2/2] spi: qup: Request CS GPIO's during probe Ivan T. Ivanov
  2015-03-07 11:01 ` [PATCH 1/2] spi: qup: Fix cs-num DT property parsing Mark Brown
  0 siblings, 2 replies; 13+ messages in thread
From: Ivan T. Ivanov @ 2015-03-06 15:26 UTC (permalink / raw)
  To: Mark Brown; +Cc: linux-spi, linux-kernel, linux-arm-msm

num-cs is 32 bit property, don't read just upper 16 bits.

Fixes: 4a8573abe965 "spi: qup: Remove chip select function"
Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
---
 drivers/spi/spi-qup.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
index ff9cdbd..2b2c359 100644
--- a/drivers/spi/spi-qup.c
+++ b/drivers/spi/spi-qup.c
@@ -498,7 +498,7 @@ static int spi_qup_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct device *dev;
 	void __iomem *base;
-	u32 max_freq, iomode;
+	u32 max_freq, iomode, num_cs;
 	int ret, irq, size;

 	dev = &pdev->dev;
@@ -550,10 +550,11 @@ static int spi_qup_probe(struct platform_device *pdev)
 	}

 	/* use num-cs unless not present or out of range */
-	if (of_property_read_u16(dev->of_node, "num-cs",
-			&master->num_chipselect) ||
-			(master->num_chipselect > SPI_NUM_CHIPSELECTS))
+	if (of_property_read_u32(dev->of_node, "num-cs", &num_cs) ||
+	    num_cs > SPI_NUM_CHIPSELECTS)
 		master->num_chipselect = SPI_NUM_CHIPSELECTS;
+	else
+		master->num_chipselect = num_cs;

 	master->bus_num = pdev->id;
 	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LOOP;
--
1.9.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/2] spi: qup: Request CS GPIO's during probe
  2015-03-06 15:26 [PATCH 1/2] spi: qup: Fix cs-num DT property parsing Ivan T. Ivanov
@ 2015-03-06 15:26 ` Ivan T. Ivanov
  2015-03-06 18:34   ` Stephen Boyd
  2015-03-07 10:59   ` Mark Brown
  2015-03-07 11:01 ` [PATCH 1/2] spi: qup: Fix cs-num DT property parsing Mark Brown
  1 sibling, 2 replies; 13+ messages in thread
From: Ivan T. Ivanov @ 2015-03-06 15:26 UTC (permalink / raw)
  To: Mark Brown; +Cc: linux-spi, linux-kernel, linux-arm-msm

Ensure that driver is owner of the GPIO's used for CS signals.

Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
---
 drivers/spi/spi-qup.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
index 2b2c359..a07ba46 100644
--- a/drivers/spi/spi-qup.c
+++ b/drivers/spi/spi-qup.c
@@ -14,11 +14,13 @@
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/err.h>
+#include <linux/gpio.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/spi/spi.h>
@@ -499,7 +501,7 @@ static int spi_qup_probe(struct platform_device *pdev)
 	struct device *dev;
 	void __iomem *base;
 	u32 max_freq, iomode, num_cs;
-	int ret, irq, size;
+	int ret, irq, size, cs, cs_gpio;

 	dev = &pdev->dev;
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -556,6 +558,19 @@ static int spi_qup_probe(struct platform_device *pdev)
 	else
 		master->num_chipselect = num_cs;

+	for (cs = 0; cs < master->num_chipselect; cs++) {
+		cs_gpio = of_get_named_gpio(dev->of_node, "cs-gpios", cs);
+
+		if (!gpio_is_valid(cs_gpio))
+			continue;
+
+		ret = devm_gpio_request(&pdev->dev, cs_gpio, "spi-qup-cs");
+		if (ret) {
+			dev_err(&pdev->dev, "can't get cs gpios\n");
+			goto error;
+		}
+	}
+
 	master->bus_num = pdev->id;
 	master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_LOOP;
 	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(4, 32);
--
1.9.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] spi: qup: Request CS GPIO's during probe
  2015-03-06 15:26 ` [PATCH 2/2] spi: qup: Request CS GPIO's during probe Ivan T. Ivanov
@ 2015-03-06 18:34   ` Stephen Boyd
  2015-03-09  8:20     ` Ivan T. Ivanov
  2015-03-07 10:59   ` Mark Brown
  1 sibling, 1 reply; 13+ messages in thread
From: Stephen Boyd @ 2015-03-06 18:34 UTC (permalink / raw)
  To: Ivan T. Ivanov, Mark Brown; +Cc: linux-spi, linux-kernel, linux-arm-msm

On 03/06/15 07:26, Ivan T. Ivanov wrote:
> Ensure that driver is owner of the GPIO's used for CS signals.

Why? What happens if we don't?

>
> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> ---
>  drivers/spi/spi-qup.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
> index 2b2c359..a07ba46 100644
> --- a/drivers/spi/spi-qup.c
> +++ b/drivers/spi/spi-qup.c
> @@ -14,11 +14,13 @@
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
> +#include <linux/gpio.h>
>  #include <linux/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/list.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_gpio.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/spi/spi.h>
> @@ -499,7 +501,7 @@ static int spi_qup_probe(struct platform_device *pdev)
>  	struct device *dev;
>  	void __iomem *base;
>  	u32 max_freq, iomode, num_cs;
> -	int ret, irq, size;
> +	int ret, irq, size, cs, cs_gpio;
>
>  	dev = &pdev->dev;
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -556,6 +558,19 @@ static int spi_qup_probe(struct platform_device *pdev)
>  	else
>  		master->num_chipselect = num_cs;
>
> +	for (cs = 0; cs < master->num_chipselect; cs++) {
> +		cs_gpio = of_get_named_gpio(dev->of_node, "cs-gpios", cs);
> +
> +		if (!gpio_is_valid(cs_gpio))
> +			continue;
> +
> +		ret = devm_gpio_request(&pdev->dev, cs_gpio, "spi-qup-cs");
> +		if (ret) {
> +			dev_err(&pdev->dev, "can't get cs gpios\n");
> +			goto error;
> +		}
> +	}
> +
>  	master->bus_num = pdev->id;

Is this related to [1]? In that case I was just relying on DT/pinctrl to
properly request the gpios.

[1] https://lkml.org/lkml/2014/5/27/784

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] spi: qup: Request CS GPIO's during probe
  2015-03-06 15:26 ` [PATCH 2/2] spi: qup: Request CS GPIO's during probe Ivan T. Ivanov
  2015-03-06 18:34   ` Stephen Boyd
@ 2015-03-07 10:59   ` Mark Brown
       [not found]     ` <1425889415.2440.10.camel@mm-sol.com>
  1 sibling, 1 reply; 13+ messages in thread
From: Mark Brown @ 2015-03-07 10:59 UTC (permalink / raw)
  To: Ivan T. Ivanov; +Cc: linux-spi, linux-kernel, linux-arm-msm

[-- Attachment #1: Type: text/plain, Size: 400 bytes --]

On Fri, Mar 06, 2015 at 05:26:18PM +0200, Ivan T. Ivanov wrote:

> Ensure that driver is owner of the GPIO's used for CS signals.

> +	for (cs = 0; cs < master->num_chipselect; cs++) {
> +		cs_gpio = of_get_named_gpio(dev->of_node, "cs-gpios", cs);
> +

Any new GPIO users should really be using the gpiod API, however this is
duplicating core functionality so as Stephen says why are we doing this?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/2] spi: qup: Fix cs-num DT property parsing
  2015-03-06 15:26 [PATCH 1/2] spi: qup: Fix cs-num DT property parsing Ivan T. Ivanov
  2015-03-06 15:26 ` [PATCH 2/2] spi: qup: Request CS GPIO's during probe Ivan T. Ivanov
@ 2015-03-07 11:01 ` Mark Brown
  1 sibling, 0 replies; 13+ messages in thread
From: Mark Brown @ 2015-03-07 11:01 UTC (permalink / raw)
  To: Ivan T. Ivanov; +Cc: linux-spi, linux-kernel, linux-arm-msm

[-- Attachment #1: Type: text/plain, Size: 142 bytes --]

On Fri, Mar 06, 2015 at 05:26:17PM +0200, Ivan T. Ivanov wrote:
> num-cs is 32 bit property, don't read just upper 16 bits.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] spi: qup: Request CS GPIO's during probe
  2015-03-06 18:34   ` Stephen Boyd
@ 2015-03-09  8:20     ` Ivan T. Ivanov
  2015-03-09 18:53       ` Stephen Boyd
  0 siblings, 1 reply; 13+ messages in thread
From: Ivan T. Ivanov @ 2015-03-09  8:20 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Mark Brown, linus.walleij, linux-spi, linux-kernel, linux-arm-msm


Hi Stephen,

> On Mar 6, 2015, at 8:34 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 03/06/15 07:26, Ivan T. Ivanov wrote:
> > Ensure that driver is owner of the GPIO's used for CS signals.
> Why? What happens if we don’t?

We can have wrong DT configuration, which could reconfigure
GPIO’s without any warning or error.

> > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> > ---
> > drivers/spi/spi-qup.c | 17 ++++++++++++++++-
> > 1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
> > index 2b2c359..a07ba46 100644
> > --- a/drivers/spi/spi-qup.c
> > +++ b/drivers/spi/spi-qup.c
> > @@ -14,11 +14,13 @@
> > #include <linux/clk.h>
> > #include <linux/delay.h>
> > #include <linux/err.h>
> > +#include <linux/gpio.h>
> > #include <linux/interrupt.h>
> > #include <linux/io.h>
> > #include <linux/list.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > +#include <linux/of_gpio.h>
> > #include <linux/platform_device.h>
> > #include <linux/pm_runtime.h>
> > #include <linux/spi/spi.h>
> > @@ -499,7 +501,7 @@ static int spi_qup_probe(struct platform_device *pdev)
> >         struct device *dev;
> >         void __iomem *base;
> >         u32 max_freq, iomode, num_cs;
> > -       int ret, irq, size;
> > +       int ret, irq, size, cs, cs_gpio;
> > 
> >         dev = &pdev->dev;
> >         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > @@ -556,6 +558,19 @@ static int spi_qup_probe(struct platform_device *pdev)
> >         else
> >                 master->num_chipselect = num_cs;
> > 
> > +       for (cs = 0; cs < master->num_chipselect; cs++) {
> > +               cs_gpio = of_get_named_gpio(dev->of_node, "cs-gpios", cs);
> > +
> > +               if (!gpio_is_valid(cs_gpio))
> > +                       continue;
> > +
> > +               ret = devm_gpio_request(&pdev->dev, cs_gpio, "spi-qup-cs");
> > +               if (ret) {
> > +                       dev_err(&pdev->dev, "can't get cs gpios\n");
> > +                       goto error;
> > +               }
> > +       }
> > +
> >         master->bus_num = pdev->id;
> Is this related to [1]? In that case I was just relying on DT/pinctrl to
> properly request the gpios.

But the DT/pinctrl did not request GPIO’s, it just configure them.
For some reason we are ending without any pinctrl_map of type 
PIN_MAP_TYPE_MUX_GROUP, which is used for pin reservation. 

> [1] https://lkml.org/lkml/2014/5/27/784

I have missed this one. I have considered same approach, but abandoned
it, because of issues related to double request reasons mentioned by Mark.

Regards,
Ivan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] spi: qup: Request CS GPIO's during probe
  2015-03-09  8:20     ` Ivan T. Ivanov
@ 2015-03-09 18:53       ` Stephen Boyd
  2015-03-10  8:31         ` Ivan T. Ivanov
  2015-03-17 11:02         ` Ivan T. Ivanov
  0 siblings, 2 replies; 13+ messages in thread
From: Stephen Boyd @ 2015-03-09 18:53 UTC (permalink / raw)
  To: Ivan T. Ivanov
  Cc: Mark Brown, linus.walleij, linux-spi, linux-kernel, linux-arm-msm

On 03/09/15 01:20, Ivan T. Ivanov wrote:
> Hi Stephen,
>
>> On Mar 6, 2015, at 8:34 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> On 03/06/15 07:26, Ivan T. Ivanov wrote:
>>> Ensure that driver is owner of the GPIO's used for CS signals.
>> Why? What happens if we don’t?
> We can have wrong DT configuration, which could reconfigure
> GPIO’s without any warning or error.

Ouch. That sounds bad. Can you please add this information to the commit
text?

>
>>> Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
>>> ---
>>> drivers/spi/spi-qup.c | 17 ++++++++++++++++-
>>> 1 file changed, 16 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
>>> index 2b2c359..a07ba46 100644
>>> --- a/drivers/spi/spi-qup.c
>>> +++ b/drivers/spi/spi-qup.c
>>> @@ -14,11 +14,13 @@
>>> #include <linux/clk.h>
>>> #include <linux/delay.h>
>>> #include <linux/err.h>
>>> +#include <linux/gpio.h>
>>> #include <linux/interrupt.h>
>>> #include <linux/io.h>
>>> #include <linux/list.h>
>>> #include <linux/module.h>
>>> #include <linux/of.h>
>>> +#include <linux/of_gpio.h>
>>> #include <linux/platform_device.h>
>>> #include <linux/pm_runtime.h>
>>> #include <linux/spi/spi.h>
>>> @@ -499,7 +501,7 @@ static int spi_qup_probe(struct platform_device *pdev)
>>>         struct device *dev;
>>>         void __iomem *base;
>>>         u32 max_freq, iomode, num_cs;
>>> -       int ret, irq, size;
>>> +       int ret, irq, size, cs, cs_gpio;
>>>
>>>         dev = &pdev->dev;
>>>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> @@ -556,6 +558,19 @@ static int spi_qup_probe(struct platform_device *pdev)
>>>         else
>>>                 master->num_chipselect = num_cs;
>>>
>>> +       for (cs = 0; cs < master->num_chipselect; cs++) {
>>> +               cs_gpio = of_get_named_gpio(dev->of_node, "cs-gpios", cs);
>>> +
>>> +               if (!gpio_is_valid(cs_gpio))
>>> +                       continue;
>>> +
>>> +               ret = devm_gpio_request(&pdev->dev, cs_gpio, "spi-qup-cs");
>>> +               if (ret) {
>>> +                       dev_err(&pdev->dev, "can't get cs gpios\n");
>>> +                       goto error;
>>> +               }
>>> +       }
>>> +
>>>         master->bus_num = pdev->id;
>> Is this related to [1]? In that case I was just relying on DT/pinctrl to
>> properly request the gpios.
> But the DT/pinctrl did not request GPIO’s, it just configure them.
> For some reason we are ending without any pinctrl_map of type 
> PIN_MAP_TYPE_MUX_GROUP, which is used for pin reservation. 
>

Ah ok. I seem to be misremembering the details.

Can you please use the gpiod interfaces here (devm_gpiod_get_index)?
That's more modern. Also, don't print any error because -EPROBE_DEFER
may come out and because __gpiod_get_index() already prints an error on
failure at the debug level.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] spi: qup: Request CS GPIO's during probe
       [not found]       ` <20150309182841.GW28806@sirena.org.uk>
@ 2015-03-10  8:10         ` Ivan T. Ivanov
  2015-03-10 11:06           ` Mark Brown
  0 siblings, 1 reply; 13+ messages in thread
From: Ivan T. Ivanov @ 2015-03-10  8:10 UTC (permalink / raw)
  To: Mark Brown; +Cc: linux-spi, linux-kernel, linux-arm-msm


On Mon, 2015-03-09 at 18:28 +0000, Mark Brown wrote:
> On Mon, Mar 09, 2015 at 10:23:35AM +0200, Ivan T. Ivanov wrote:
> > Hi,
> 
> Don't reply off list unless there's a good reason...

Sorry, it was not intentional. Wrong button.

> 
> > > Any new GPIO users should really be using the gpiod API, however this is
> > > duplicating core functionality so as Stephen says why are we doing this?
> 
> > About the API usage, point taken. GPIO requesting part is more important
> > in this case. pinctrl core did not request pins and wrong DT configuration
> > could lead to surprises without any warnings or errors.
> 
> That doesn't answer my concern at all.

I am not sure that I am following you. 

I can not use spi_master::cs_gpios, which is populated by 
of_spi_register_master(), because spi_register_master() 
populate SPI devices and they could issue setup method.

Requesting GPIO's in core framework is also not a easy
option because of arguments here[1].

Probably I am still missing something.

Regards,
Ivan

[1] https://lkml.org/lkml/2014/5/24/75


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] spi: qup: Request CS GPIO's during probe
  2015-03-09 18:53       ` Stephen Boyd
@ 2015-03-10  8:31         ` Ivan T. Ivanov
  2015-03-17 11:02         ` Ivan T. Ivanov
  1 sibling, 0 replies; 13+ messages in thread
From: Ivan T. Ivanov @ 2015-03-10  8:31 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Mark Brown, linus.walleij, linux-spi, linux-kernel, linux-arm-msm

Hi, 

On Mon, 2015-03-09 at 11:53 -0700, Stephen Boyd wrote:
> On 03/09/15 01:20, Ivan T. Ivanov wrote:
> > Hi Stephen,
> > 
> > > On Mar 6, 2015, at 8:34 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > > On 03/06/15 07:26, Ivan T. Ivanov wrote:
> > > > Ensure that driver is owner of the GPIO's used for CS signals.
> > > Why? What happens if we don’t?
> > We can have wrong DT configuration, which could reconfigure
> > GPIO’s without any warning or error.
> 
> Ouch. That sounds bad. Can you please add this information to the commit
> text?

Sure.

> > > > +       for (cs = 0; cs < master->num_chipselect; cs++) {
> > > > +               cs_gpio = of_get_named_gpio(dev->of_node, "cs-gpios", cs);
> > > > +
> > > > +               if (!gpio_is_valid(cs_gpio))
> > > > +                       continue;
> > > > +
> > > > +               ret = devm_gpio_request(&pdev->dev, cs_gpio, "spi-qup-cs");
> > > > +               if (ret) {
> > > > +                       dev_err(&pdev->dev, "can't get cs gpios\n");
> > > > +                       goto error;
> > > > +               }
> > > > +       }
> > > > +
> > > >         master->bus_num = pdev->id;
> > > Is this related to [1]? In that case I was just relying on DT/pinctrl to
> > > properly request the gpios.
> > But the DT/pinctrl did not request GPIO’s, it just configure them.
> > For some reason we are ending without any pinctrl_map of type
> > PIN_MAP_TYPE_MUX_GROUP, which is used for pin reservation.
> > 
> 
> Ah ok. I seem to be misremembering the details.

:-) I still have version of downstream "msm-pinctrl" driver, 
which create PIN_MAP_TYPE_MUX_GROUP maps during node to map
parsing. 

> 
> Can you please use the gpiod interfaces here (devm_gpiod_get_index)?

Sure.

> That's more modern. Also, don't print any error because -EPROBE_DEFER
may come out and 

Well, this could not happen. Driver probe will not called until default 
pinctrl state is not available and we rely on this for proper driver
functionality. 

> because __gpiod_get_index() already prints an error on
> failure at the debug level.

Ok.

Thanks, 
Ivan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] spi: qup: Request CS GPIO's during probe
  2015-03-10  8:10         ` Ivan T. Ivanov
@ 2015-03-10 11:06           ` Mark Brown
  2015-03-10 12:53             ` Ivan T. Ivanov
  0 siblings, 1 reply; 13+ messages in thread
From: Mark Brown @ 2015-03-10 11:06 UTC (permalink / raw)
  To: Ivan T. Ivanov; +Cc: linux-spi, linux-kernel, linux-arm-msm

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

On Tue, Mar 10, 2015 at 10:10:56AM +0200, Ivan T. Ivanov wrote:
> On Mon, 2015-03-09 at 18:28 +0000, Mark Brown wrote:

> > > About the API usage, point taken. GPIO requesting part is more important
> > > in this case. pinctrl core did not request pins and wrong DT configuration
> > > could lead to surprises without any warnings or errors.

> > That doesn't answer my concern at all.

> I am not sure that I am following you. 

> I can not use spi_master::cs_gpios, which is populated by 
> of_spi_register_master(), because spi_register_master() 
> populate SPI devices and they could issue setup method.

I'm sorry but I can't parse the above.  What does "they could issue
setup method" mean and why is it a problem?

> Requesting GPIO's in core framework is also not a easy
> option because of arguments here[1].

We should really fix that though.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] spi: qup: Request CS GPIO's during probe
  2015-03-10 11:06           ` Mark Brown
@ 2015-03-10 12:53             ` Ivan T. Ivanov
  2015-03-10 20:47               ` Mark Brown
  0 siblings, 1 reply; 13+ messages in thread
From: Ivan T. Ivanov @ 2015-03-10 12:53 UTC (permalink / raw)
  To: Mark Brown; +Cc: linux-spi, linux-kernel, linux-arm-msm


On Tue, 2015-03-10 at 11:06 +0000, Mark Brown wrote:
> On Tue, Mar 10, 2015 at 10:10:56AM +0200, Ivan T. Ivanov wrote:
> > On Mon, 2015-03-09 at 18:28 +0000, Mark Brown wrote:
> 
> > > > About the API usage, point taken. GPIO requesting part is more important
> > > > in this case. pinctrl core did not request pins and wrong DT configuration
> > > > could lead to surprises without any warnings or errors.
> 
> > > That doesn't answer my concern at all.
> 
> > I am not sure that I am following you.
> 
> > I can not use spi_master::cs_gpios, which is populated by
> > of_spi_register_master(), because spi_register_master()
> > populate SPI devices and they could issue setup method.
> 
> I'm sorry but I can't parse the above.  What does "they could issue
> setup method" mean and why is it a problem?

Client drivers could execute spi_setup() in probe(), so we have
to ensure that CS GPIO's are available before this, no?

> 
> > Requesting GPIO's in core framework is also not a easy
> > option because of arguments here[1].
> 
> We should really fix that though.
> 

I think that pinctrl framework should automatically
request pins belonging to group when state is selected.

Regards,
Ivan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] spi: qup: Request CS GPIO's during probe
  2015-03-10 12:53             ` Ivan T. Ivanov
@ 2015-03-10 20:47               ` Mark Brown
  0 siblings, 0 replies; 13+ messages in thread
From: Mark Brown @ 2015-03-10 20:47 UTC (permalink / raw)
  To: Ivan T. Ivanov; +Cc: linux-spi, linux-kernel, linux-arm-msm

[-- Attachment #1: Type: text/plain, Size: 870 bytes --]

On Tue, Mar 10, 2015 at 02:53:17PM +0200, Ivan T. Ivanov wrote:
> 
> On Tue, 2015-03-10 at 11:06 +0000, Mark Brown wrote:

> > I'm sorry but I can't parse the above.  What does "they could issue
> > setup method" mean and why is it a problem?

> Client drivers could execute spi_setup() in probe(), so we have
> to ensure that CS GPIO's are available before this, no?

I see.  Yes, that's a concern.  It should be in the changelog.

> > > Requesting GPIO's in core framework is also not a easy
> > > option because of arguments here[1].

> > We should really fix that though.

> I think that pinctrl framework should automatically
> request pins belonging to group when state is selected.

That doesn't help users as not every GPIO is associated with a pin
controller - they'd still have to handle the case where they had to do
things themselves.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/2] spi: qup: Request CS GPIO's during probe
  2015-03-09 18:53       ` Stephen Boyd
  2015-03-10  8:31         ` Ivan T. Ivanov
@ 2015-03-17 11:02         ` Ivan T. Ivanov
  1 sibling, 0 replies; 13+ messages in thread
From: Ivan T. Ivanov @ 2015-03-17 11:02 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Mark Brown, linus.walleij, linux-spi, linux-kernel, linux-arm-msm

Hi, 

On Mon, 2015-03-09 at 11:53 -0700, Stephen Boyd wrote:
> On 03/09/15 01:20, Ivan T. Ivanov wrote:
> > Hi Stephen,
> > 
> > > On Mar 6, 2015, at 8:34 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > > On 03/06/15 07:26, Ivan T. Ivanov wrote:
> > > > Ensure that driver is owner of the GPIO's used for CS signals.
> > > Why? What happens if we don’t?
> > We can have wrong DT configuration, which could reconfigure
> > GPIO’s without any warning or error.
> 
> Ouch. That sounds bad. Can you please add this information to the commit
> text?
> 
> > > > Signed-off-by: Ivan T. Ivanov <iivanov@mm-sol.com>
> > > > ---
> > > > drivers/spi/spi-qup.c | 17 ++++++++++++++++-
> > > > 1 file changed, 16 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/spi/spi-qup.c b/drivers/spi/spi-qup.c
> > > > index 2b2c359..a07ba46 100644
> > > > --- a/drivers/spi/spi-qup.c
> > > > +++ b/drivers/spi/spi-qup.c
> > > > @@ -14,11 +14,13 @@
> > > > #include <linux/clk.h>
> > > > #include <linux/delay.h>
> > > > #include <linux/err.h>
> > > > +#include <linux/gpio.h>
> > > > #include <linux/interrupt.h>
> > > > #include <linux/io.h>
> > > > #include <linux/list.h>
> > > > #include <linux/module.h>
> > > > #include <linux/of.h>
> > > > +#include <linux/of_gpio.h>
> > > > #include <linux/platform_device.h>
> > > > #include <linux/pm_runtime.h>
> > > > #include <linux/spi/spi.h>
> > > > @@ -499,7 +501,7 @@ static int spi_qup_probe(struct platform_device *pdev)
> > > >         struct device *dev;
> > > >         void __iomem *base;
> > > >         u32 max_freq, iomode, num_cs;
> > > > -       int ret, irq, size;
> > > > +       int ret, irq, size, cs, cs_gpio;
> > > > 
> > > >         dev = &pdev->dev;
> > > >         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > > @@ -556,6 +558,19 @@ static int spi_qup_probe(struct platform_device *pdev)
> > > >         else
> > > >                 master->num_chipselect = num_cs;
> > > > 
> > > > +       for (cs = 0; cs < master->num_chipselect; cs++) {
> > > > +               cs_gpio = of_get_named_gpio(dev->of_node, "cs-gpios", cs);
> > > > +
> > > > +               if (!gpio_is_valid(cs_gpio))
> > > > +                       continue;
> > > > +
> > > > +               ret = devm_gpio_request(&pdev->dev, cs_gpio, "spi-qup-cs");
> > > > +               if (ret) {
> > > > +                       dev_err(&pdev->dev, "can't get cs gpios\n");
> > > > +                       goto error;
> > > > +               }
> > > > +       }
> > > > +
> > > >         master->bus_num = pdev->id;
> > > Is this related to [1]? In that case I was just relying on DT/pinctrl to
> > > properly request the gpios.
> > But the DT/pinctrl did not request GPIO’s, it just configure them.
> > For some reason we are ending without any pinctrl_map of type
> > PIN_MAP_TYPE_MUX_GROUP, which is used for pin reservation.

I will like to withdraw this patch. It fix the problem only
for CS lines, but misconfiguration could happen also for the
rest for the lines.

I will take a look in the pinctrl core code to see would it
be possible to print warning or something, when one driver
reconfigure pins already configured by another driver.

Regards,
Ivan  





^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2015-03-17 11:02 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-06 15:26 [PATCH 1/2] spi: qup: Fix cs-num DT property parsing Ivan T. Ivanov
2015-03-06 15:26 ` [PATCH 2/2] spi: qup: Request CS GPIO's during probe Ivan T. Ivanov
2015-03-06 18:34   ` Stephen Boyd
2015-03-09  8:20     ` Ivan T. Ivanov
2015-03-09 18:53       ` Stephen Boyd
2015-03-10  8:31         ` Ivan T. Ivanov
2015-03-17 11:02         ` Ivan T. Ivanov
2015-03-07 10:59   ` Mark Brown
     [not found]     ` <1425889415.2440.10.camel@mm-sol.com>
     [not found]       ` <20150309182841.GW28806@sirena.org.uk>
2015-03-10  8:10         ` Ivan T. Ivanov
2015-03-10 11:06           ` Mark Brown
2015-03-10 12:53             ` Ivan T. Ivanov
2015-03-10 20:47               ` Mark Brown
2015-03-07 11:01 ` [PATCH 1/2] spi: qup: Fix cs-num DT property parsing Mark Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).