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

Groups > linux.kernel > #1653248 > unrolled thread

Re: [PATCH v6 1/4] gpio: mvebu: Add limited PWM support

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

  Re: [PATCH v6 1/4] gpio: mvebu: Add limited PWM support Gregory CLEMENT <gregory.clement@free-electrons.com> - 2017-05-30 15:50 +0200
    Re: [PATCH v6 1/4] gpio: mvebu: Add limited PWM support Ralph Sennhauser <ralph.sennhauser@gmail.com> - 2017-05-30 17:00 +0200
      Re: [PATCH v6 1/4] gpio: mvebu: Add limited PWM support Richard Genoud <richard.genoud@gmail.com> - 2017-05-30 17:40 +0200
        Re: [PATCH v6 1/4] gpio: mvebu: Add limited PWM support Ralph Sennhauser <ralph.sennhauser@gmail.com> - 2017-05-31 14:30 +0200
          Re: [PATCH v6 1/4] gpio: mvebu: Add limited PWM support Gregory CLEMENT <gregory.clement@free-electrons.com> - 2017-05-31 14:40 +0200

#1653248 — Re: [PATCH v6 1/4] gpio: mvebu: Add limited PWM support

Hi Ralph and Linus,

I resurrect this thread following the series post by Richard Genoud:
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-May/509461.html

 On lun., avril 24 2017, Linus Walleij <linus.walleij@linaro.org> wrote:

> On Fri, Apr 14, 2017 at 5:40 PM, Ralph Sennhauser
> <ralph.sennhauser@gmail.com> wrote:
>
>> From: Andrew Lunn <andrew@lunn.ch>
>>
>> Armada 370/XP devices can 'blink' GPIO lines with a configurable on
>> and off period. This can be modelled as a PWM.
>>
>> However, there are only two sets of PWM configuration registers for
>> all the GPIO lines. This driver simply allows a single GPIO line per
>> GPIO chip of 32 lines to be used as a PWM. Attempts to use more return
>> EBUSY.
>>
>> Due to the interleaving of registers it is not simple to separate the
>> PWM driver from the GPIO driver. Thus the GPIO driver has been
>> extended with a PWM driver.
>>
>> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
>> URL: https://patchwork.ozlabs.org/patch/427287/
>> URL: https://patchwork.ozlabs.org/patch/427295/
>> [Ralph Sennhauser:
>>   * Port forward
>>   * Merge PWM portion into gpio-mvebu.c
>>   * Switch to atomic PWM API
>>   * Add new compatible string marvell,armada-370-xp-gpio

The fact that Richard managed to run the code on Armada 38x draw my
attention on the documentation that need to be amend.

And it makes me realized that I missed the bad naming of the compatible
string. We don't use family name for the compatible string, but the name
of the first SoC compatible with. So in this case we should use
"marvell,armada-370", as it is still in rc and not yet deployed. What
about fixing the name now?

I can send a patch for it if you want and fix the dts that are still in
my mvebu branch.

Thanks,

Gregory


>>   * Update and merge documentation patch
>>   * Update MAINTAINERS]
>> Signed-off-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
>> Tested-by: Andrew Lunn <andrew@lunn.ch>
>> Acked-by: Thierry Reding <thierry.reding@gmail.com>
>> Acked-by: Rob Herring <robh@kernel.org>
>
> Patch applied.
>
> Yours,
> Linus Walleij

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[toc] | [next] | [standalone]

#1653291

On Tue, 30 May 2017 15:40:00 +0200
Gregory CLEMENT <gregory.clement@free-electrons.com> wrote:

> Hi Ralph and Linus,
> 
> I resurrect this thread following the series post by Richard Genoud:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2017-May/509461.html
> 
>  On lun., avril 24 2017, Linus Walleij <linus.walleij@linaro.org>
> wrote:
> 
> > On Fri, Apr 14, 2017 at 5:40 PM, Ralph Sennhauser
> > <ralph.sennhauser@gmail.com> wrote:
> >  
> >> From: Andrew Lunn <andrew@lunn.ch>
> >>
> >> Armada 370/XP devices can 'blink' GPIO lines with a configurable on
> >> and off period. This can be modelled as a PWM.
> >>
> >> However, there are only two sets of PWM configuration registers for
> >> all the GPIO lines. This driver simply allows a single GPIO line
> >> per GPIO chip of 32 lines to be used as a PWM. Attempts to use
> >> more return EBUSY.
> >>
> >> Due to the interleaving of registers it is not simple to separate
> >> the PWM driver from the GPIO driver. Thus the GPIO driver has been
> >> extended with a PWM driver.
> >>
> >> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> >> URL: https://patchwork.ozlabs.org/patch/427287/
> >> URL: https://patchwork.ozlabs.org/patch/427295/
> >> [Ralph Sennhauser:
> >>   * Port forward
> >>   * Merge PWM portion into gpio-mvebu.c
> >>   * Switch to atomic PWM API
> >>   * Add new compatible string marvell,armada-370-xp-gpio  
> 
> The fact that Richard managed to run the code on Armada 38x draw my
> attention on the documentation that need to be amend.

Hi Gregory

But does it behave as a pwm-fan? I don't have access to the 38x
functional spec (no NDA with Marvell), the one for XP is public.
According to Andrew the 38x doesn't have the blinking hardware and so I
expect the pwm-fan is just an overblown gpio-fan there, additionally
writing some random values to random registers where luck has it nothing
goes wrong.

Kirkwood apparently has blinking support but only at fixed frequency of
10Hz, so not usable for this purpose either.

So to my knowledge it really is just 370 and XP.

> 
> And it makes me realized that I missed the bad naming of the
> compatible string. We don't use family name for the compatible
> string, but the name of the first SoC compatible with. So in this
> case we should use "marvell,armada-370", as it is still in rc and not
> yet deployed. What about fixing the name now?

I don't mind but certainly not for me to decide at this point.

> 
> I can send a patch for it if you want and fix the dts that are still
> in my mvebu branch.

Sure, or if you prefer I can do it as well if we decide for the change.
No preference.

Ralph

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

#1653324

2017-05-30 16:51 GMT+02:00 Ralph Sennhauser <ralph.sennhauser@gmail.com>:
> On Tue, 30 May 2017 15:40:00 +0200
> Gregory CLEMENT <gregory.clement@free-electrons.com> wrote:
>
>> Hi Ralph and Linus,
>>
>> I resurrect this thread following the series post by Richard Genoud:
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2017-May/509461.html
>>
>>  On lun., avril 24 2017, Linus Walleij <linus.walleij@linaro.org>
>> wrote:
>>
>> > On Fri, Apr 14, 2017 at 5:40 PM, Ralph Sennhauser
>> > <ralph.sennhauser@gmail.com> wrote:
>> >
>> >> From: Andrew Lunn <andrew@lunn.ch>
>> >>
>> >> Armada 370/XP devices can 'blink' GPIO lines with a configurable on
>> >> and off period. This can be modelled as a PWM.
>> >>
>> >> However, there are only two sets of PWM configuration registers for
>> >> all the GPIO lines. This driver simply allows a single GPIO line
>> >> per GPIO chip of 32 lines to be used as a PWM. Attempts to use
>> >> more return EBUSY.
>> >>
>> >> Due to the interleaving of registers it is not simple to separate
>> >> the PWM driver from the GPIO driver. Thus the GPIO driver has been
>> >> extended with a PWM driver.
>> >>
>> >> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
>> >> URL: https://patchwork.ozlabs.org/patch/427287/
>> >> URL: https://patchwork.ozlabs.org/patch/427295/
>> >> [Ralph Sennhauser:
>> >>   * Port forward
>> >>   * Merge PWM portion into gpio-mvebu.c
>> >>   * Switch to atomic PWM API
>> >>   * Add new compatible string marvell,armada-370-xp-gpio
>>
>> The fact that Richard managed to run the code on Armada 38x draw my
>> attention on the documentation that need to be amend.
>
> Hi Gregory
>
> But does it behave as a pwm-fan? I don't have access to the 38x
> functional spec (no NDA with Marvell), the one for XP is public.
> According to Andrew the 38x doesn't have the blinking hardware and so I
> expect the pwm-fan is just an overblown gpio-fan there, additionally
> writing some random values to random registers where luck has it nothing
> goes wrong.
>
> Kirkwood apparently has blinking support but only at fixed frequency of
> 10Hz, so not usable for this purpose either.
>
> So to my knowledge it really is just 370 and XP.
Hi Ralph,

I have the functional spec (no NDA needed, but it's not the full one) :
A38x-Functional-Spec-PU0A.pdf
https://marvellcorp.wufoo.com/forms/marvell-armada-38x-functional-specifications/
(just an email needed, no blood signing nor chicken slaughtering)
There are the GPIO Blink Counter A/B is ON/OFF Duration Registers as well as the
Blink Enable Registers.

I've done a pwm with different periods (8ms, 4ms, 100ns).
Looking at the scope, it seems to work pretty well :)

>>
>> And it makes me realized that I missed the bad naming of the
>> compatible string. We don't use family name for the compatible
>> string, but the name of the first SoC compatible with. So in this
>> case we should use "marvell,armada-370", as it is still in rc and not
>> yet deployed. What about fixing the name now?
>
> I don't mind but certainly not for me to decide at this point.
>
>>
>> I can send a patch for it if you want and fix the dts that are still
>> in my mvebu branch.
>
> Sure, or if you prefer I can do it as well if we decide for the change.
> No preference.
>
> Ralph

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

#1654181

On Tue, 30 May 2017 17:35:33 +0200
Richard Genoud <richard.genoud@gmail.com> wrote:
  
> Hi Ralph,
> 
> I have the functional spec (no NDA needed, but it's not the full
> one) : A38x-Functional-Spec-PU0A.pdf
> https://marvellcorp.wufoo.com/forms/marvell-armada-38x-functional-specifications/
> (just an email needed, no blood signing nor chicken slaughtering)
> There are the GPIO Blink Counter A/B is ON/OFF Duration Registers as
> well as the Blink Enable Registers.
> 

Hi Richard,

Thanks for the link, as the terms only talk about materials obtained
from www.marvell.com this one from wufoo.com should be exempt ;)
Also at a glance looks like the complete one. Also says "Functional
Specifications – Unrestricted". Maybe you want to re-download it.

Regardless, as you said the blinking registers are all described. So
it's probably safe to assume 39x will have them as well.

> I've done a pwm with different periods (8ms, 4ms, 100ns).
> Looking at the scope, it seems to work pretty well :)
> 
> >>
> >> And it makes me realized that I missed the bad naming of the
> >> compatible string. We don't use family name for the compatible
> >> string, but the name of the first SoC compatible with. So in this
> >> case we should use "marvell,armada-370", as it is still in rc and
> >> not yet deployed. What about fixing the name now?  

Gregory,

Knowing it's not limited to 370/XP makes "marvell,armada-370-gpio" an
obviously better choice for the compatible string. Guess you didn't
mean to drop the "-gpio" suffix.

Will work on a patch changing the compatible string / documentation for
4.12 and an updated patch for armada 370/XP dtsi as well as a patch
adding the properties to 38x for 4.13+. Expect them tomorrow, probably
won't get around to it today anymore.

Ralph

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

#1654184

Hi Ralph,
 
 On mer., mai 31 2017, Ralph Sennhauser <ralph.sennhauser@gmail.com> wrote:

> On Tue, 30 May 2017 17:35:33 +0200
> Richard Genoud <richard.genoud@gmail.com> wrote:
>   
>> Hi Ralph,
>> 
>> I have the functional spec (no NDA needed, but it's not the full
>> one) : A38x-Functional-Spec-PU0A.pdf
>> https://marvellcorp.wufoo.com/forms/marvell-armada-38x-functional-specifications/
>> (just an email needed, no blood signing nor chicken slaughtering)
>> There are the GPIO Blink Counter A/B is ON/OFF Duration Registers as
>> well as the Blink Enable Registers.
>> 
>
> Hi Richard,
>
> Thanks for the link, as the terms only talk about materials obtained
> from www.marvell.com this one from wufoo.com should be exempt ;)
> Also at a glance looks like the complete one. Also says "Functional
> Specifications – Unrestricted". Maybe you want to re-download it.
>
> Regardless, as you said the blinking registers are all described. So
> it's probably safe to assume 39x will have them as well.

I guess you meant 38x. (But I suppose it is also true for 39x)


>
>> I've done a pwm with different periods (8ms, 4ms, 100ns).
>> Looking at the scope, it seems to work pretty well :)
>> 
>> >>
>> >> And it makes me realized that I missed the bad naming of the
>> >> compatible string. We don't use family name for the compatible
>> >> string, but the name of the first SoC compatible with. So in this
>> >> case we should use "marvell,armada-370", as it is still in rc and
>> >> not yet deployed. What about fixing the name now?  
>
> Gregory,
>
> Knowing it's not limited to 370/XP makes "marvell,armada-370-gpio" an
> obviously better choice for the compatible string. Guess you didn't
> mean to drop the "-gpio" suffix.

You're right I meant "marvell,armada-370-gpio".

>
> Will work on a patch changing the compatible string / documentation for
> 4.12 and an updated patch for armada 370/XP dtsi as well as a patch
> adding the properties to 38x for 4.13+. Expect them tomorrow, probably
> won't get around to it today anymore.

Great!

Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web