[PATCH] mfd: stmpe: Pull IRQ GPIO number from DT during DT-based probe

[PATCH] mfd: stmpe: Pull IRQ GPIO number from DT during DT-based probe

[Marek Vasut]

In case of a DT-based probe of the stmpe MFD driver, the irq_gpio was zero,
which resulted in the driver failing to probe.

Implement DT properties "irq-over-gpio" and "irq-gpios" which are already used
in "arch/arm/boot/dts/spear320-hmi.dts" to circumvent these problems. The new
behaviour is the expected one and copies the behavior of platform_data-based
probe.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Vipul Kumar Samar <vipulkumar.samar@st.com>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/mfd/stmpe.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
index 4b11202..fb9cd6f 100644
--- a/drivers/mfd/stmpe.c
+++ b/drivers/mfd/stmpe.c
@@ -1018,6 +1018,11 @@ void stmpe_of_probe(struct stmpe_platform_data *pdata, struct device_node *np)
 	pdata->id = -1;
 	pdata->irq_trigger = IRQF_TRIGGER_NONE;
 
+	pdata->irq_over_gpio = of_get_property(np, "irq-over-gpio", NULL);
+	pdata->irq_gpio = of_get_named_gpio(np, "irq-gpios", 0);
+	if (!gpio_is_valid(pdata->irq_gpio))
+		pdata->irq_gpio = -1;
+
 	of_property_read_u32(np, "st,autosleep-timeout",
 			&pdata->autosleep_timeout);
 
-- 
1.7.10.4

Re: [PATCH] mfd: stmpe: Pull IRQ GPIO number from DT during DT-based probe

[Lee Jones]

On Mon, 07 Jan 2013, Marek Vasut wrote:

> In case of a DT-based probe of the stmpe MFD driver, the irq_gpio was zero,
> which resulted in the driver failing to probe.
> 
> Implement DT properties "irq-over-gpio" and "irq-gpios" which are already used
> in "arch/arm/boot/dts/spear320-hmi.dts" to circumvent these problems. 

This must have slipped through the gaps. It should be removed.

> The new
> behaviour is the expected one and copies the behavior of platform_data-based
> probe.

Blindly copying platform data behaviour to DT bindings is seldom a good idea.

> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Vipul Kumar Samar <vipulkumar.samar@st.com>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/mfd/stmpe.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
> index 4b11202..fb9cd6f 100644
> --- a/drivers/mfd/stmpe.c
> +++ b/drivers/mfd/stmpe.c
> @@ -1018,6 +1018,11 @@ void stmpe_of_probe(struct stmpe_platform_data *pdata, struct device_node *np)
>  	pdata->id = -1;
>  	pdata->irq_trigger = IRQF_TRIGGER_NONE;
>  
> +	pdata->irq_over_gpio = of_get_property(np, "irq-over-gpio", NULL);

This is a new DT binding and you haven't provided any documentation for it.

Also, there is no reason for the binding to exist.

Lots of drivers use GPIO pins as IRQs.

> +	pdata->irq_gpio = of_get_named_gpio(np, "irq-gpios", 0);
> +	if (!gpio_is_valid(pdata->irq_gpio))
> +		pdata->irq_gpio = -1;
> +
>  	of_property_read_u32(np, "st,autosleep-timeout",
>  			&pdata->autosleep_timeout);
>  
> -- 
> 1.7.10.4
> 

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mfd: stmpe: Pull IRQ GPIO number from DT during DT-based probe

[Marek Vasut]

Dear Lee Jones,

> On Mon, 07 Jan 2013, Marek Vasut wrote:
> > In case of a DT-based probe of the stmpe MFD driver, the irq_gpio was
> > zero, which resulted in the driver failing to probe.
> > 
> > Implement DT properties "irq-over-gpio" and "irq-gpios" which are already
> > used in "arch/arm/boot/dts/spear320-hmi.dts" to circumvent these
> > problems.
> 
> This must have slipped through the gaps. It should be removed.

True

> > The new
> > behaviour is the expected one and copies the behavior of
> > platform_data-based probe.
> 
> Blindly copying platform data behaviour to DT bindings is seldom a good
> idea.

Do you have suggestions how to pass these information? I suspect the irq-over-
gpio property can be killed, since if irq-gpios prop is there, it implies irq-
over-gpio anyway.

> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Lee Jones <lee.jones@linaro.org>
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Samuel Ortiz <sameo@linux.intel.com>
> > Cc: Vipul Kumar Samar <vipulkumar.samar@st.com>
> > Cc: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> > 
> >  drivers/mfd/stmpe.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/mfd/stmpe.c b/drivers/mfd/stmpe.c
> > index 4b11202..fb9cd6f 100644
> > --- a/drivers/mfd/stmpe.c
> > +++ b/drivers/mfd/stmpe.c
> > @@ -1018,6 +1018,11 @@ void stmpe_of_probe(struct stmpe_platform_data
> > *pdata, struct device_node *np)
> > 
> >  	pdata->id = -1;
> >  	pdata->irq_trigger = IRQF_TRIGGER_NONE;
> > 
> > +	pdata->irq_over_gpio = of_get_property(np, "irq-over-gpio", NULL);
> 
> This is a new DT binding and you haven't provided any documentation for it.
> 
> Also, there is no reason for the binding to exist.
> 
> Lots of drivers use GPIO pins as IRQs.

Right, this can be removed and only the 'irq-gpios' can be kept around. What do 
you say?

> > +	pdata->irq_gpio = of_get_named_gpio(np, "irq-gpios", 0);
> > +	if (!gpio_is_valid(pdata->irq_gpio))
> > +		pdata->irq_gpio = -1;
> > +
> > 
> >  	of_property_read_u32(np, "st,autosleep-timeout",
> >  	
> >  			&pdata->autosleep_timeout);

Best regards,
Marek Vasut

Re: [PATCH] mfd: stmpe: Pull IRQ GPIO number from DT during DT-based probe

[Lee Jones]

> > > +	pdata->irq_over_gpio = of_get_property(np, "irq-over-gpio", NULL);
> > 
> > This is a new DT binding and you haven't provided any documentation for it.
> > 
> > Also, there is no reason for the binding to exist.
> > 
> > Lots of drivers use GPIO pins as IRQs.
> 
> Right, this can be removed and only the 'irq-gpios' can be kept around. What do 
> you say?

I don't like it to be honest.

How many GPIOs does the STMPE driver need?

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mfd: stmpe: Pull IRQ GPIO number from DT during DT-based probe

[Viresh Kumar]

On 7 January 2013 20:43, Marek Vasut <marex@denx.de> wrote:
>> > The new
>> > behaviour is the expected one and copies the behavior of
>> > platform_data-based probe.
>>
>> Blindly copying platform data behaviour to DT bindings is seldom a good
>> idea.
>
> Do you have suggestions how to pass these information? I suspect the irq-over-
> gpio property can be killed, since if irq-gpios prop is there, it implies irq-
> over-gpio anyway.

Both can be killed. gpios as interrupt lines can be requested as:

 interrupt-parent = <&gpio-controller>;
 interrupts = <25 0x1>;

and probably the stmpe driver doesn't have to bother at all if
interrupt was over gpio
or not.

Re: [PATCH] mfd: stmpe: Pull IRQ GPIO number from DT during DT-based probe

[vipul kumar samar]

On 1/7/2013 11:49 PM, Viresh Kumar wrote:
> On 7 January 2013 20:43, Marek Vasut<marex@denx.de>  wrote:
>>>> The new
>>>> behaviour is the expected one and copies the behavior of
>>>> platform_data-based probe.
>>>
>>> Blindly copying platform data behaviour to DT bindings is seldom a good
>>> idea.
>>
>> Do you have suggestions how to pass these information? I suspect the irq-over-
>> gpio property can be killed, since if irq-gpios prop is there, it implies irq-
>> over-gpio anyway.
>
> Both can be killed. gpios as interrupt lines can be requested as:
>
>   interrupt-parent =<&gpio-controller>;
>   interrupts =<25 0x1>;
>
> and probably the stmpe driver doesn't have to bother at all if
> interrupt was over gpio
> or not.
> .
>

Yes, both can be killed and similarly we did for spear1340 machine but 
some how we missed it on spear320-hmi.

Regards
Vipul Samar

Re: [PATCH] mfd: stmpe: Pull IRQ GPIO number from DT during DT-based probe

[Lee Jones]

> >Both can be killed. gpios as interrupt lines can be requested as:
> >
> >  interrupt-parent =<&gpio-controller>;
> >  interrupts =<25 0x1>;
> >
> >and probably the stmpe driver doesn't have to bother at all if
> >interrupt was over gpio or not.

> Yes, both can be killed and similarly we did for spear1340 machine
> but some how we missed it on spear320-hmi.

Bingo!

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mfd: stmpe: Pull IRQ GPIO number from DT during DT-based probe

[Marek Vasut]

Dear Lee Jones,

> > >Both can be killed. gpios as interrupt lines can be requested as:
> > >  interrupt-parent =<&gpio-controller>;
> > >  interrupts =<25 0x1>;
> > >
> > >and probably the stmpe driver doesn't have to bother at all if
> > >interrupt was over gpio or not.
> > 
> > Yes, both can be killed and similarly we did for spear1340 machine
> > but some how we missed it on spear320-hmi.
> 
> Bingo!

All right, all right ... got it ;-)

I'll rework it and repost then.

Best regards,
Marek Vasut

Re: [PATCH] mfd: stmpe: Pull IRQ GPIO number from DT during DT-based probe

[Marek Vasut]

Dear Lee Jones,

> > > >Both can be killed. gpios as interrupt lines can be requested as:
> > > >  interrupt-parent =<&gpio-controller>;
> > > >  interrupts =<25 0x1>;
> > > >
> > > >and probably the stmpe driver doesn't have to bother at all if
> > > >interrupt was over gpio or not.
> > > 
> > > Yes, both can be killed and similarly we did for spear1340 machine
> > > but some how we missed it on spear320-hmi.
> > 
> > Bingo!
> 
> All right, all right ... got it ;-)
> 
> I'll rework it and repost then.

Maybe I was too quick about understanding the issue at hand. So allow me a 
stupid question -- who's gonna configure the GPIO as input for it to work as IRQ 
GPIO?

Best regards,
Marek Vasut

Re: [PATCH] mfd: stmpe: Pull IRQ GPIO number from DT during DT-based probe

[Viresh Kumar]

On 8 January 2013 15:18, Marek Vasut <marex@denx.de> wrote:
> Maybe I was too quick about understanding the issue at hand. So allow me a
> stupid question -- who's gonna configure the GPIO as input for it to work as IRQ
> GPIO?

Hmm.. I tried a bit, but couldn't find any such call :(
Probably an assumption is taken here. GPIO pins which are going to be used as
interrupt lines, wouldn't be getting set in output mode at all. So,
once they are put
in input mode in beginning, nobody would change it ever.

Much of gpio controllers configure gpio pins in input mode in their probe().

Maybe, there is something else :)

Re: [PATCH] mfd: stmpe: Pull IRQ GPIO number from DT during DT-based probe

[Lee Jones]

On Tue, 08 Jan 2013, Viresh Kumar wrote:

> On 8 January 2013 15:18, Marek Vasut <marex@denx.de> wrote:
> > Maybe I was too quick about understanding the issue at hand. So allow me a
> > stupid question -- who's gonna configure the GPIO as input for it to work as IRQ
> > GPIO?
> 
> Hmm.. I tried a bit, but couldn't find any such call :(
> Probably an assumption is taken here. GPIO pins which are going to be used as
> interrupt lines, wouldn't be getting set in output mode at all. So,
> once they are put
> in input mode in beginning, nobody would change it ever.
> 
> Much of gpio controllers configure gpio pins in input mode in their probe().
> 
> Maybe, there is something else :)

Pinctrl?

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mfd: stmpe: Pull IRQ GPIO number from DT during DT-based probe

[Viresh Kumar]

On 8 January 2013 16:41, Lee Jones <lee.jones@linaro.org> wrote:
>> Hmm.. I tried a bit, but couldn't find any such call :(
>> Probably an assumption is taken here. GPIO pins which are going to be used as
>> interrupt lines, wouldn't be getting set in output mode at all. So,
>> once they are put
>> in input mode in beginning, nobody would change it ever.
>>
>> Much of gpio controllers configure gpio pins in input mode in their probe().
>>
>> Maybe, there is something else :)
>
> Pinctrl?

I don't think pinctrl is playing with it. I searched for
"direction_input" string and
pinctrl routine also had similar name. I couldn't fine use of
direction_input anywhere
in kernel, for setting them as irqs for OF cases.

Re: [PATCH] mfd: stmpe: Pull IRQ GPIO number from DT during DT-based probe

[Laxman Dewangan]

On Tuesday 08 January 2013 04:44 PM, Viresh Kumar wrote:
> On 8 January 2013 16:41, Lee Jones <lee.jones@linaro.org> wrote:
>>> Hmm.. I tried a bit, but couldn't find any such call :(
>>> Probably an assumption is taken here. GPIO pins which are going to be used as
>>> interrupt lines, wouldn't be getting set in output mode at all. So,
>>> once they are put
>>> in input mode in beginning, nobody would change it ever.
>>>
>>> Much of gpio controllers configure gpio pins in input mode in their probe().
>>>
>>> Maybe, there is something else :)
>> Pinctrl?
> I don't think pinctrl is playing with it. I searched for
> "direction_input" string and
> pinctrl routine also had similar name. I couldn't fine use of
> direction_input anywhere
> in kernel, for setting them as irqs for OF cases.

I think we can do it in the gpio chip driver inthe function 
xxx_gpio_irq_set_type().
When you register any irq and if this callback is available from gpio 
chip driver then this will get called. and so the setting the pi to 
input mode as well as putting the pin in GPIO mode can be done here.  
This is what we are doing it in gpio-tegra driver.

Thanks,
Laxman

Re: [PATCH] mfd: stmpe: Pull IRQ GPIO number from DT during DT-based probe

[Viresh Kumar]

On 8 January 2013 16:48, Laxman Dewangan <ldewangan@nvidia.com> wrote:
> I think we can do it in the gpio chip driver inthe function
> xxx_gpio_irq_set_type().
> When you register any irq and if this callback is available from gpio chip
> driver then this will get called. and so the setting the pi to input mode as
> well as putting the pin in GPIO mode can be done here.  This is what we are
> doing it in gpio-tegra driver.

Hmm... So, the initial question was, is somebody setting gpio in input mode?
There were no bugs reported, it was just a query. As most of the stuff would
work as per my earlier explanation.

But yes, your suggestion makes sense too.. I would rather do it in the gpiolib
routine instead of adding this in every driver.

@Linus/Grant: What do you say?

Re: [PATCH] mfd: stmpe: Pull IRQ GPIO number from DT during DT-based probe

[Linus Walleij]

On Tue, Jan 8, 2013 at 12:14 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 8 January 2013 16:41, Lee Jones <lee.jones@linaro.org> wrote:
>>> Hmm.. I tried a bit, but couldn't find any such call :(
>>> Probably an assumption is taken here. GPIO pins which are going to be used as
>>> interrupt lines, wouldn't be getting set in output mode at all. So,
>>> once they are put
>>> in input mode in beginning, nobody would change it ever.
>>>
>>> Much of gpio controllers configure gpio pins in input mode in their probe().
>>>
>>> Maybe, there is something else :)
>>
>> Pinctrl?
>
> I don't think pinctrl is playing with it. I searched for
> "direction_input" string and
> pinctrl routine also had similar name. I couldn't fine use of
> direction_input anywhere
> in kernel, for setting them as irqs for OF cases.

pinctrl has pinctrl_gpio_direction_input() and
pinctrl_gpio_direction_output() which are supposed to
be called *only* by GPIOlib frontends using pinctrl
as backend to control the pins.

But if it's a pinctrl driver using standard pinconfig from
include/linux/pinctrl/pinconf-generic.h
I'm all for adding a PIN_CONFIG_INPUT_ENABLE
to these definintions so it can be set up as input
at boot from the device tree using hogs or something,
that make things easy when using GPIOs as IRQ
providers only.

So the alternative is to just set up the IRQ using the
gpiolib functions for this: of_get_gpio() if you need the
number from DT, then gpio_request() and
gpio_direction_input() as on any GPIO. This can be
done in the device driver or board code depending
on use case.

In the Nomadik I did this (maybe ugly) hack for a
similar case:

+/*
+ * The SMSC911x IRQ is connected to a GPIO pin, but the driver expects
+ * to simply request an IRQ passed as a resource. So the GPIO pin needs
+ * to be requested by this hog and set as input.
+ */
+static int __init cpu8815_eth_init(void)
+{
+       struct device_node *eth;
+       int gpio, irq, err;
+
+       eth = of_find_node_by_path("/external-bus@34000000/ethernet@300");
+       if (!eth) {
+               pr_info("could not find any ethernet controller\n");
+               return 0;
+       }
+       gpio = of_get_gpio(eth, 0);
+       err = gpio_request(gpio, "eth_irq");
+       if (err) {
+               pr_info("failed to request ethernet GPIO\n");
+               return -ENODEV;
+       }
+       err = gpio_direction_input(gpio);
+       if (err) {
+               pr_info("failed to set ehernet GPIO as input\n");
+               return -ENODEV;
+       }
+       irq = gpio_to_irq(gpio);
+       pr_info("enabled ethernet GPIO %d, IRQ %d\n", gpio, irq);
+       return 0;
+}
+device_initcall(cpu8815_eth_init);

I haven't read review comments on that patch.

Maybe it's not such a good idea to add the GPIO to the device itself
when it's being hogged by board code like this. It's a bit of a grey area
so I'm a bit confused here.

Maybe the GPIO lib actually needs a "hog" mechanism that can
request and set GPIO pins as input/output on boot and then
forget about them.

Yours,
Linus Walleij

Re: [PATCH] mfd: stmpe: Pull IRQ GPIO number from DT during DT-based probe

[Lee Jones]

On Thu, 10 Jan 2013, Linus Walleij wrote:

> On Tue, Jan 8, 2013 at 12:14 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 8 January 2013 16:41, Lee Jones <lee.jones@linaro.org> wrote:
> >>> Hmm.. I tried a bit, but couldn't find any such call :(
> >>> Probably an assumption is taken here. GPIO pins which are going to be used as
> >>> interrupt lines, wouldn't be getting set in output mode at all. So,
> >>> once they are put
> >>> in input mode in beginning, nobody would change it ever.
> >>>
> >>> Much of gpio controllers configure gpio pins in input mode in their probe().
> >>>
> >>> Maybe, there is something else :)
> >>
> >> Pinctrl?
> >
> > I don't think pinctrl is playing with it. I searched for
> > "direction_input" string and
> > pinctrl routine also had similar name. I couldn't fine use of
> > direction_input anywhere
> > in kernel, for setting them as irqs for OF cases.
> 
> pinctrl has pinctrl_gpio_direction_input() and
> pinctrl_gpio_direction_output() which are supposed to
> be called *only* by GPIOlib frontends using pinctrl
> as backend to control the pins.
> 
> But if it's a pinctrl driver using standard pinconfig from
> include/linux/pinctrl/pinconf-generic.h
> I'm all for adding a PIN_CONFIG_INPUT_ENABLE
> to these definintions so it can be set up as input
> at boot from the device tree using hogs or something,
> that make things easy when using GPIOs as IRQ
> providers only.
> 
> So the alternative is to just set up the IRQ using the
> gpiolib functions for this: of_get_gpio() if you need the
> number from DT, then gpio_request() and
> gpio_direction_input() as on any GPIO. This can be
> done in the device driver or board code depending
> on use case.
> 
> In the Nomadik I did this (maybe ugly) hack for a
> similar case:
> 
> +/*
> + * The SMSC911x IRQ is connected to a GPIO pin, but the driver expects
> + * to simply request an IRQ passed as a resource. So the GPIO pin needs
> + * to be requested by this hog and set as input.
> + */
> +static int __init cpu8815_eth_init(void)
> +{
> +       struct device_node *eth;
> +       int gpio, irq, err;
> +
> +       eth = of_find_node_by_path("/external-bus@34000000/ethernet@300");
> +       if (!eth) {
> +               pr_info("could not find any ethernet controller\n");
> +               return 0;
> +       }
> +       gpio = of_get_gpio(eth, 0);
> +       err = gpio_request(gpio, "eth_irq");
> +       if (err) {
> +               pr_info("failed to request ethernet GPIO\n");
> +               return -ENODEV;
> +       }
> +       err = gpio_direction_input(gpio);
> +       if (err) {
> +               pr_info("failed to set ehernet GPIO as input\n");
> +               return -ENODEV;
> +       }
> +       irq = gpio_to_irq(gpio);
> +       pr_info("enabled ethernet GPIO %d, IRQ %d\n", gpio, irq);
> +       return 0;
> +}
> +device_initcall(cpu8815_eth_init);

Yep, that looks pretty gross!

> I haven't read review comments on that patch.
> 
> Maybe it's not such a good idea to add the GPIO to the device itself
> when it's being hogged by board code like this. It's a bit of a grey area
> so I'm a bit confused here.
> 
> Maybe the GPIO lib actually needs a "hog" mechanism that can
> request and set GPIO pins as input/output on boot and then
> forget about them.
> 
> Yours,
> Linus Walleij

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mfd: stmpe: Pull IRQ GPIO number from DT during DT-based probe

[Grant Likely]

On Thu, 10 Jan 2013 12:42:53 +0100, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Jan 8, 2013 at 12:14 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 8 January 2013 16:41, Lee Jones <lee.jones@linaro.org> wrote:
> >>> Hmm.. I tried a bit, but couldn't find any such call :(
> >>> Probably an assumption is taken here. GPIO pins which are going to be used as
> >>> interrupt lines, wouldn't be getting set in output mode at all. So,
> >>> once they are put
> >>> in input mode in beginning, nobody would change it ever.
> >>>
> >>> Much of gpio controllers configure gpio pins in input mode in their probe().
> >>>
> >>> Maybe, there is something else :)
> >>
> >> Pinctrl?
> >
> > I don't think pinctrl is playing with it. I searched for
> > "direction_input" string and
> > pinctrl routine also had similar name. I couldn't fine use of
> > direction_input anywhere
> > in kernel, for setting them as irqs for OF cases.
> 
> pinctrl has pinctrl_gpio_direction_input() and
> pinctrl_gpio_direction_output() which are supposed to
> be called *only* by GPIOlib frontends using pinctrl
> as backend to control the pins.
> 
> But if it's a pinctrl driver using standard pinconfig from
> include/linux/pinctrl/pinconf-generic.h
> I'm all for adding a PIN_CONFIG_INPUT_ENABLE
> to these definintions so it can be set up as input
> at boot from the device tree using hogs or something,
> that make things easy when using GPIOs as IRQ
> providers only.
> 
> So the alternative is to just set up the IRQ using the
> gpiolib functions for this: of_get_gpio() if you need the
> number from DT, then gpio_request() and
> gpio_direction_input() as on any GPIO. This can be
> done in the device driver or board code depending
> on use case.
> 
> In the Nomadik I did this (maybe ugly) hack for a
> similar case:
> 
> +/*
> + * The SMSC911x IRQ is connected to a GPIO pin, but the driver expects
> + * to simply request an IRQ passed as a resource. So the GPIO pin needs
> + * to be requested by this hog and set as input.
> + */
> +static int __init cpu8815_eth_init(void)
> +{
> +       struct device_node *eth;
> +       int gpio, irq, err;
> +
> +       eth = of_find_node_by_path("/external-bus@34000000/ethernet@300");
> +       if (!eth) {
> +               pr_info("could not find any ethernet controller\n");
> +               return 0;
> +       }
> +       gpio = of_get_gpio(eth, 0);
> +       err = gpio_request(gpio, "eth_irq");
> +       if (err) {
> +               pr_info("failed to request ethernet GPIO\n");
> +               return -ENODEV;
> +       }
> +       err = gpio_direction_input(gpio);
> +       if (err) {
> +               pr_info("failed to set ehernet GPIO as input\n");
> +               return -ENODEV;
> +       }
> +       irq = gpio_to_irq(gpio);
> +       pr_info("enabled ethernet GPIO %d, IRQ %d\n", gpio, irq);
> +       return 0;
> +}
> +device_initcall(cpu8815_eth_init);
> 
> I haven't read review comments on that patch.
> 
> Maybe it's not such a good idea to add the GPIO to the device itself
> when it's being hogged by board code like this. It's a bit of a grey area
> so I'm a bit confused here.
> 
> Maybe the GPIO lib actually needs a "hog" mechanism that can
> request and set GPIO pins as input/output on boot and then
> forget about them.

That would be reasonable. Probably as properties in the gpio controller
node that specify how to set the inital state.

g.

Re: [PATCH] mfd: stmpe: Pull IRQ GPIO number from DT during DT-based probe

[Marek Vasut]

Dear Grant Likely,

> On Thu, 10 Jan 2013 12:42:53 +0100, Linus Walleij <linus.walleij@linaro.org> 
wrote:
> > On Tue, Jan 8, 2013 at 12:14 PM, Viresh Kumar <viresh.kumar@linaro.org> 
wrote:
> > > On 8 January 2013 16:41, Lee Jones <lee.jones@linaro.org> wrote:
> > >>> Hmm.. I tried a bit, but couldn't find any such call :(
> > >>> Probably an assumption is taken here. GPIO pins which are going to be
> > >>> used as interrupt lines, wouldn't be getting set in output mode at
> > >>> all. So, once they are put
> > >>> in input mode in beginning, nobody would change it ever.
> > >>> 
> > >>> Much of gpio controllers configure gpio pins in input mode in their
> > >>> probe().
> > >>> 
> > >>> Maybe, there is something else :)
> > >> 
> > >> Pinctrl?
> > > 
> > > I don't think pinctrl is playing with it. I searched for
> > > "direction_input" string and
> > > pinctrl routine also had similar name. I couldn't fine use of
> > > direction_input anywhere
> > > in kernel, for setting them as irqs for OF cases.
> > 
> > pinctrl has pinctrl_gpio_direction_input() and
> > pinctrl_gpio_direction_output() which are supposed to
> > be called *only* by GPIOlib frontends using pinctrl
> > as backend to control the pins.
> > 
> > But if it's a pinctrl driver using standard pinconfig from
> > include/linux/pinctrl/pinconf-generic.h
> > I'm all for adding a PIN_CONFIG_INPUT_ENABLE
> > to these definintions so it can be set up as input
> > at boot from the device tree using hogs or something,
> > that make things easy when using GPIOs as IRQ
> > providers only.
> > 
> > So the alternative is to just set up the IRQ using the
> > gpiolib functions for this: of_get_gpio() if you need the
> > number from DT, then gpio_request() and
> > gpio_direction_input() as on any GPIO. This can be
> > done in the device driver or board code depending
> > on use case.
> > 
> > In the Nomadik I did this (maybe ugly) hack for a
> > similar case:
> > 
> > +/*
> > + * The SMSC911x IRQ is connected to a GPIO pin, but the driver expects
> > + * to simply request an IRQ passed as a resource. So the GPIO pin needs
> > + * to be requested by this hog and set as input.
> > + */
> > +static int __init cpu8815_eth_init(void)
> > +{
> > +       struct device_node *eth;
> > +       int gpio, irq, err;
> > +
> > +       eth =
> > of_find_node_by_path("/external-bus@34000000/ethernet@300"); +       if
> > (!eth) {
> > +               pr_info("could not find any ethernet controller\n");
> > +               return 0;
> > +       }
> > +       gpio = of_get_gpio(eth, 0);
> > +       err = gpio_request(gpio, "eth_irq");
> > +       if (err) {
> > +               pr_info("failed to request ethernet GPIO\n");
> > +               return -ENODEV;
> > +       }
> > +       err = gpio_direction_input(gpio);
> > +       if (err) {
> > +               pr_info("failed to set ehernet GPIO as input\n");
> > +               return -ENODEV;
> > +       }
> > +       irq = gpio_to_irq(gpio);
> > +       pr_info("enabled ethernet GPIO %d, IRQ %d\n", gpio, irq);
> > +       return 0;
> > +}
> > +device_initcall(cpu8815_eth_init);
> > 
> > I haven't read review comments on that patch.
> > 
> > Maybe it's not such a good idea to add the GPIO to the device itself
> > when it's being hogged by board code like this. It's a bit of a grey area
> > so I'm a bit confused here.
> > 
> > Maybe the GPIO lib actually needs a "hog" mechanism that can
> > request and set GPIO pins as input/output on boot and then
> > forget about them.
> 
> That would be reasonable. Probably as properties in the gpio controller
> node that specify how to set the inital state.

Looks good.

btw. I have a feeling this stmpe touchscreen driver has a few more issues, but 
I'll need to investigate before I jump to any conclusion.

Best regards,
Marek Vasut