device-tree vs gpio-leds gpio_blink_set

device-tree vs gpio-leds gpio_blink_set

[Arnaud Patard (Rtp)]

Hi,

I'm not yet fully familiar with DT so I thought that adding support for 
device-tree on some kirkwood drivers would help, so I wrote a quick patch
to add device-tree support into kirkwood/orion/... GPIO (patch for rfc
in next mail). Unfortunately, I'm facing a problem that I don't know how
to solve.

The kirkwood/orion gpio have a bit that allows "gpio blinking" (I think
it a square signal but the manual doesn't say it clearly) and it's used
on dns323 and iconnect. When trying to use DT to declare the leds-gpio
stuff, I've noticed that there's no way clearly documented to set the
gpio_blink_set() hook. In fact, it even looks like it's not supported at
all, which seems to be a regression compared to non-DT support.
Does anyone know how to solve that ?

Thanks,
Arnaud

[patch 1/1] orion/kirkwood/mv78xx0: add DT support to GPIO

[Arnaud Patard (Rtp)]

An embedded and charset-unspecified text was scrubbed...
Name: gpio-test.patch
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120423/9613e1b1/attachment.ksh>

device-tree vs gpio-leds gpio_blink_set

[Arnd Bergmann]

On Monday 23 April 2012, Arnaud Patard wrote:
> The kirkwood/orion gpio have a bit that allows "gpio blinking" (I think
> it a square signal but the manual doesn't say it clearly) and it's used
> on dns323 and iconnect. When trying to use DT to declare the leds-gpio
> stuff, I've noticed that there's no way clearly documented to set the
> gpio_blink_set() hook. In fact, it even looks like it's not supported at
> all, which seems to be a regression compared to non-DT support.

Yes, that seems to be correct. You are only the third person using
gpio-led with blinking, aside from two uses in s3c24xx, and they
don't support DT either yet.

> Does anyone know how to solve that ?

One way to solve it would be to add a blink function to gpiolib so
that the gpio-led driver can access it through a common wrapper
and we can make the orion_gpio_set_blink() function a pointer in
the gpio_chip structure rather than an export from
arch/arm/plat-orion/gpio.c. I've put a few people on Cc that might
have an opion on this.

	Arnd

device-tree vs gpio-leds gpio_blink_set

[Mark Brown]

On Mon, Apr 23, 2012 at 11:22:43AM +0000, Arnd Bergmann wrote:
> On Monday 23 April 2012, Arnaud Patard wrote:

> > Does anyone know how to solve that ?

> One way to solve it would be to add a blink function to gpiolib so
> that the gpio-led driver can access it through a common wrapper
> and we can make the orion_gpio_set_blink() function a pointer in
> the gpio_chip structure rather than an export from
> arch/arm/plat-orion/gpio.c. I've put a few people on Cc that might
> have an opion on this.

Another idea would be to use pinctrl and represent the blink as a
special function.  Not sure that's sensible though.

device-tree vs gpio-leds gpio_blink_set

[Linus Walleij]

On Mon, Apr 23, 2012 at 7:49 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Apr 23, 2012 at 11:22:43AM +0000, Arnd Bergmann wrote:
>> On Monday 23 April 2012, Arnaud Patard wrote:
>
>> > Does anyone know how to solve that ?
>
>> One way to solve it would be to add a blink function to gpiolib so
>> that the gpio-led driver can access it through a common wrapper
>> and we can make the orion_gpio_set_blink() function a pointer in
>> the gpio_chip structure rather than an export from
>> arch/arm/plat-orion/gpio.c. I've put a few people on Cc that might
>> have an opion on this.
>
> Another idea would be to use pinctrl and represent the blink as a
> special function. ?Not sure that's sensible though.

I would rather say that anything automatically toggling
a GPIO pin on/off at specified intervals is HW-controlled
GPIO PWM.

So this would be something that gets to be modeled in the
PWM subsystem, but there is no such thing (I know Sascha et
al was working on that at one point.)

But in my dreams it would be in
drivers/pwm/pwm-gpio.c and then anything that needs
an auto-toggling GPIO pin could use that PWM interface.
And only then it would be given hooks into the gpio driver,
like with some #ifdef in the .h file for gpiolib that expose
this hook only to the PWM subsystem.

But I guess I won't have it my way :-)

Currently, drivers/leds/leds-gpio.c has a
platform-specific callback to set blink period. How do we
replace that with something that can call generic code on
a larger scale?

Maybe it should go into gpiolib then, but I'm not sure at
all.

BTW: can leds-pwm.c use leds-gpio.c and exploit the blink
callback there? (I hope so...)

Yours,
Linus Walleij

device-tree vs gpio-leds gpio_blink_set

[Mark Brown]

On Tue, Apr 24, 2012 at 02:24:16PM +0200, Linus Walleij wrote:
> On Mon, Apr 23, 2012 at 7:49 PM, Mark Brown

> > Another idea would be to use pinctrl and represent the blink as a
> > special function. ?Not sure that's sensible though.

> I would rather say that anything automatically toggling
> a GPIO pin on/off at specified intervals is HW-controlled
> GPIO PWM.

Yes, that's not a million miles away from what I was thinking - the
followup to what I was thinking was that the other function would be a
PWM function.  It's feeling like we might want some sort of generic
interface for handling situations like this and like the open drain
simulation code which was recently added to gpiolib where we end up
crossing subsystems.  In principle you should always use the subsystem
you want directly, the issue comes about when you want to use multiple
sets of functionality and still fall back on the emulations.

> So this would be something that gets to be modeled in the
> PWM subsystem, but there is no such thing (I know Sascha et
> al was working on that at one point.)

Thierry took over the work and last time I saw it things looked like
they were pretty much ready for merge, I'd really expect to see the code
in 3.5 based on the previous state (in fact I was pretty surprised 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120424/c0ba85ea/attachment.sig>

device-tree vs gpio-leds gpio_blink_set

[Thierry Reding]

* Mark Brown wrote:
> On Tue, Apr 24, 2012 at 02:24:16PM +0200, Linus Walleij wrote:
> > So this would be something that gets to be modeled in the
> > PWM subsystem, but there is no such thing (I know Sascha et
> > al was working on that at one point.)
> 
> Thierry took over the work and last time I saw it things looked like
> they were pretty much ready for merge, I'd really expect to see the code
> in 3.5 based on the previous state (in fact I was pretty surprised 

I posted a couple of patches about a week ago that are required to build !OF
configurations. They still need to be acked by Grant or Rob, though.

	http://lists.ozlabs.org/pipermail/devicetree-discuss/2012-April/014687.html
	http://lists.ozlabs.org/pipermail/devicetree-discuss/2012-April/014688.html

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120424/9209da77/attachment.sig>

device-tree vs gpio-leds gpio_blink_set

[jonsmirl at gmail.com]

On Tue, Apr 24, 2012 at 8:24 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Apr 23, 2012 at 7:49 PM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
>> On Mon, Apr 23, 2012 at 11:22:43AM +0000, Arnd Bergmann wrote:
>>> On Monday 23 April 2012, Arnaud Patard wrote:
>>
>>> > Does anyone know how to solve that ?
>>
>>> One way to solve it would be to add a blink function to gpiolib so
>>> that the gpio-led driver can access it through a common wrapper
>>> and we can make the orion_gpio_set_blink() function a pointer in
>>> the gpio_chip structure rather than an export from
>>> arch/arm/plat-orion/gpio.c. I've put a few people on Cc that might
>>> have an opion on this.

I have a board here with a NXP PCA9532 on i2c.
http://www.nxp.com/documents/data_sheet/PCA9532.pdf
drivers/leds/leds-pca9532.c

Pins can be:
GPIO
hardware blinking LED
PWM for backlight

It might even be an interrupt source, but I haven't dug into it that deep.

>>
>> Another idea would be to use pinctrl and represent the blink as a
>> special function. ?Not sure that's sensible though.
>
> I would rather say that anything automatically toggling
> a GPIO pin on/off at specified intervals is HW-controlled
> GPIO PWM.
>
> So this would be something that gets to be modeled in the
> PWM subsystem, but there is no such thing (I know Sascha et
> al was working on that at one point.)
>
> But in my dreams it would be in
> drivers/pwm/pwm-gpio.c and then anything that needs
> an auto-toggling GPIO pin could use that PWM interface.
> And only then it would be given hooks into the gpio driver,
> like with some #ifdef in the .h file for gpiolib that expose
> this hook only to the PWM subsystem.
>
> But I guess I won't have it my way :-)
>
> Currently, drivers/leds/leds-gpio.c has a
> platform-specific callback to set blink period. How do we
> replace that with something that can call generic code on
> a larger scale?
>
> Maybe it should go into gpiolib then, but I'm not sure at
> all.
>
> BTW: can leds-pwm.c use leds-gpio.c and exploit the blink
> callback there? (I hope so...)
>
> Yours,
> Linus Walleij
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel



-- 
Jon Smirl
jonsmirl at gmail.com

device-tree vs gpio-leds gpio_blink_set

[jonsmirl at gmail.com]

On Tue, Apr 24, 2012 at 8:39 AM, jonsmirl at gmail.com <jonsmirl@gmail.com> wrote:
> On Tue, Apr 24, 2012 at 8:24 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Mon, Apr 23, 2012 at 7:49 PM, Mark Brown
>> <broonie@opensource.wolfsonmicro.com> wrote:
>>> On Mon, Apr 23, 2012 at 11:22:43AM +0000, Arnd Bergmann wrote:
>>>> On Monday 23 April 2012, Arnaud Patard wrote:
>>>
>>>> > Does anyone know how to solve that ?
>>>
>>>> One way to solve it would be to add a blink function to gpiolib so
>>>> that the gpio-led driver can access it through a common wrapper
>>>> and we can make the orion_gpio_set_blink() function a pointer in
>>>> the gpio_chip structure rather than an export from
>>>> arch/arm/plat-orion/gpio.c. I've put a few people on Cc that might
>>>> have an opion on this.
>
> I have a board here with a NXP PCA9532 on i2c.
> http://www.nxp.com/documents/data_sheet/PCA9532.pdf
> drivers/leds/leds-pca9532.c
>
> Pins can be:
> GPIO
> hardware blinking LED
> PWM for backlight

The board is using all three of these modes simultaneously.
It is an eval board for the NXP LPC3131.

>
> It might even be an interrupt source, but I haven't dug into it that deep.
>
>>>
>>> Another idea would be to use pinctrl and represent the blink as a
>>> special function. ?Not sure that's sensible though.
>>
>> I would rather say that anything automatically toggling
>> a GPIO pin on/off at specified intervals is HW-controlled
>> GPIO PWM.
>>
>> So this would be something that gets to be modeled in the
>> PWM subsystem, but there is no such thing (I know Sascha et
>> al was working on that at one point.)
>>
>> But in my dreams it would be in
>> drivers/pwm/pwm-gpio.c and then anything that needs
>> an auto-toggling GPIO pin could use that PWM interface.
>> And only then it would be given hooks into the gpio driver,
>> like with some #ifdef in the .h file for gpiolib that expose
>> this hook only to the PWM subsystem.
>>
>> But I guess I won't have it my way :-)
>>
>> Currently, drivers/leds/leds-gpio.c has a
>> platform-specific callback to set blink period. How do we
>> replace that with something that can call generic code on
>> a larger scale?
>>
>> Maybe it should go into gpiolib then, but I'm not sure at
>> all.
>>
>> BTW: can leds-pwm.c use leds-gpio.c and exploit the blink
>> callback there? (I hope so...)
>>
>> Yours,
>> Linus Walleij
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
>
> --
> Jon Smirl
> jonsmirl at gmail.com



-- 
Jon Smirl
jonsmirl at gmail.com

device-tree vs gpio-leds gpio_blink_set

[Arnaud Patard (Rtp)]

Andrew Lunn <andrew@lunn.ch> writes:

>> One way to solve it would be to add a blink function to gpiolib so
>> that the gpio-led driver can access it through a common wrapper
>> and we can make the orion_gpio_set_blink() function a pointer in
>> the gpio_chip structure rather than an export from
>> arch/arm/plat-orion/gpio.c. I've put a few people on Cc that might
>> have an opion on this.
>
> Hi Arnd
>
> That would work for Orion based devices. However, take a look at
> mach-s3c24xx. It has two different blink_set functions, one in
> mach-s3c24xx/mach-h1940.c and another in mach-s3c24xx/mach-rx1950.c.

[ for the record, it happens that I worked on the h1940 support but it's
been quite some time I didn't boot it ]

>
> So the blink_set function probably needs to be board specific in some
> cases.

imho the kirkwood/orion/... case is an exception rather than the
rule. Moreover, nothing prevents a HW designer to use something else to
handle hw blinking instead of the kirkwood blinking method. After all,
this method has a fixed frequency of 2^24 tclk (which makes it 100ms on
most boards).

Arnaud