[PATCHv2 0/2] gpio: mvebu: fixes for PWM/blink

[PATCHv2 0/2] gpio: mvebu: fixes for PWM/blink

[Richard Genoud]

These are 2 fixes for gpio-mvebu, related to the PWM support introduced
by commit 757642f9a584 ("gpio: mvebu: Add limited PWM support")
As that commit was merged in 4.12-rc1, I guess these commits are 4.12
material.

Changes from v1:
 - Add tags Fixes: and Reviewed-by:
 - Add some comments to motivate the mvpwm->chip->base = -1 choice
instead of mvpwm->chip->base = mvchip->chip.base

Richard Genoud (2):
  gpio: mvebu: fix blink counter register selection
  gpio: mvebu: fix gpio bank registration when pwm is used

 drivers/gpio/gpio-mvebu.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

[PATCHv2 1/2] gpio: mvebu: fix blink counter register selection

[Richard Genoud]

The blink counter A was always selected because 0 was forced in the
blink select counter register.
The variable 'set' was obviously there to be used as the register value,
selecting the B counter when id==1 and A counter when id==0.

Tested on clearfog-pro (Marvell 88F6828)

Fixes: 757642f9a584 ("gpio: mvebu: Add limited PWM support")
Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
Reviewed-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 drivers/gpio/gpio-mvebu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index 19a92efabbef..cdef2c78cb3b 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -747,7 +747,7 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
 		set = U32_MAX;
 	else
 		return -EINVAL;
-	writel_relaxed(0, mvebu_gpioreg_blink_counter_select(mvchip));
+	writel_relaxed(set, mvebu_gpioreg_blink_counter_select(mvchip));
 
 	mvpwm = devm_kzalloc(dev, sizeof(struct mvebu_pwm), GFP_KERNEL);
 	if (!mvpwm)

[PATCHv2 1/2] gpio: mvebu: fix blink counter register selection

[Linus Walleij]

On Thu, Jun 1, 2017 at 2:18 PM, Richard Genoud <richard.genoud@gmail.com> wrote:

> The blink counter A was always selected because 0 was forced in the
> blink select counter register.
> The variable 'set' was obviously there to be used as the register value,
> selecting the B counter when id==1 and A counter when id==0.
>
> Tested on clearfog-pro (Marvell 88F6828)
>
> Fixes: 757642f9a584 ("gpio: mvebu: Add limited PWM support")
> Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> Reviewed-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>

Patch applied for fixes.

It appears this will clash with patches on the development branch :(

I might screw up the merges so help me check the end result
later.

Yours,
Linus Walleij

[PATCHv2 1/2] gpio: mvebu: fix blink counter register selection

[Richard Genoud]

2017-06-09 9:37 GMT+02:00 Linus Walleij <linus.walleij@linaro.org>:
> On Thu, Jun 1, 2017 at 2:18 PM, Richard Genoud <richard.genoud@gmail.com> wrote:
>
>> The blink counter A was always selected because 0 was forced in the
>> blink select counter register.
>> The variable 'set' was obviously there to be used as the register value,
>> selecting the B counter when id==1 and A counter when id==0.
>>
>> Tested on clearfog-pro (Marvell 88F6828)
>>
>> Fixes: 757642f9a584 ("gpio: mvebu: Add limited PWM support")
>> Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> Reviewed-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
>> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
>
> Patch applied for fixes.
>
> It appears this will clash with patches on the development branch :(
>
> I might screw up the merges so help me check the end result
> later.
Ok, no problem !

>
> Yours,
> Linus Walleij
Thanks !
Richard.

[PATCHv2 1/2] gpio: mvebu: fix blink counter register selection

[Ralph Sennhauser]

On Fri, 9 Jun 2017 09:37:55 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Thu, Jun 1, 2017 at 2:18 PM, Richard Genoud
> <richard.genoud@gmail.com> wrote:
> 
> > The blink counter A was always selected because 0 was forced in the
> > blink select counter register.
> > The variable 'set' was obviously there to be used as the register
> > value, selecting the B counter when id==1 and A counter when id==0.
> >
> > Tested on clearfog-pro (Marvell 88F6828)
> >
> > Fixes: 757642f9a584 ("gpio: mvebu: Add limited PWM support")
> > Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> > Reviewed-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> > Signed-off-by: Richard Genoud <richard.genoud@gmail.com>  
> 
> Patch applied for fixes.
> 
> It appears this will clash with patches on the development branch :(

Hi Linus,

The commit 2233bf7a92e7 ("gpio: mvebu: switch to regmap for register
access") which you likely mean breaks gpio-keys, reported about an hour
ago, so you might just want to drop that one for now instead as it
needs mor work anyway.

Ralph

[PATCHv2 1/2] gpio: mvebu: fix blink counter register selection

[Linus Walleij]

On Fri, Jun 9, 2017 at 10:03 AM, Ralph Sennhauser
<ralph.sennhauser@gmail.com> wrote:
> On Fri, 9 Jun 2017 09:37:55 +0200
> Linus Walleij <linus.walleij@linaro.org> wrote:
>
>> On Thu, Jun 1, 2017 at 2:18 PM, Richard Genoud
>> <richard.genoud@gmail.com> wrote:
>>
>> > The blink counter A was always selected because 0 was forced in the
>> > blink select counter register.
>> > The variable 'set' was obviously there to be used as the register
>> > value, selecting the B counter when id==1 and A counter when id==0.
>> >
>> > Tested on clearfog-pro (Marvell 88F6828)
>> >
>> > Fixes: 757642f9a584 ("gpio: mvebu: Add limited PWM support")
>> > Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
>> > Reviewed-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
>> > Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
>>
>> Patch applied for fixes.
>>
>> It appears this will clash with patches on the development branch :(
>
> Hi Linus,
>
> The commit 2233bf7a92e7 ("gpio: mvebu: switch to regmap for register
> access") which you likely mean breaks gpio-keys, reported about an hour
> ago, so you might just want to drop that one for now instead as it
> needs mor work anyway.

I would like to get an indication from Thomas and/or Gregory if they think
it is a good idea to revert or if they just wanna fix it.

We still have development time before we even start merging for v4.13
and then we have all the release candidates. No point to throw up hands
this early?

Yours,
Linus Walleij

[PATCHv2 1/2] gpio: mvebu: fix blink counter register selection

[Ralph Sennhauser]

Hi Linus,

On Sun, 11 Jun 2017 23:48:34 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Fri, Jun 9, 2017 at 10:03 AM, Ralph Sennhauser
> <ralph.sennhauser@gmail.com> wrote:
> > On Fri, 9 Jun 2017 09:37:55 +0200
> > Linus Walleij <linus.walleij@linaro.org> wrote:
> >  
> >> On Thu, Jun 1, 2017 at 2:18 PM, Richard Genoud
> >> <richard.genoud@gmail.com> wrote:
> >>  
> >> > The blink counter A was always selected because 0 was forced in
> >> > the blink select counter register.
> >> > The variable 'set' was obviously there to be used as the register
> >> > value, selecting the B counter when id==1 and A counter when
> >> > id==0.
> >> >
> >> > Tested on clearfog-pro (Marvell 88F6828)
> >> >
> >> > Fixes: 757642f9a584 ("gpio: mvebu: Add limited PWM support")
> >> > Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> >> > Reviewed-by: Ralph Sennhauser <ralph.sennhauser@gmail.com>
> >> > Signed-off-by: Richard Genoud <richard.genoud@gmail.com>  
> >>
> >> Patch applied for fixes.
> >>
> >> It appears this will clash with patches on the development
> >> branch :(  
> >
> > Hi Linus,
> >
> > The commit 2233bf7a92e7 ("gpio: mvebu: switch to regmap for register
> > access") which you likely mean breaks gpio-keys, reported about an
> > hour ago, so you might just want to drop that one for now instead
> > as it needs mor work anyway.  
> 
> I would like to get an indication from Thomas and/or Gregory if they
> think it is a good idea to revert or if they just wanna fix it.

A fix patch by Gregory exists by now ([PATCH v2] gpio: mvebu: fix
regmap_update_bits usage).
 
> We still have development time before we even start merging for v4.13
> and then we have all the release candidates. No point to throw up
> hands this early?

Ah, not throwing hands. Just thought the author might prefer to rebase
the patch himself when fixing the bug so there is just one clean patch
for the change as there is still enough time for that before the merge
window opens.

Ralph

[PATCHv2 2/2] gpio: mvebu: fix gpio bank registration when pwm is used

[Richard Genoud]

If more than one gpio bank has the "pwm" property, only one will be
registered successfully, all the others will fail with:
mvebu-gpio: probe of f1018140.gpio failed with error -17

That's because in alloc_pwms(), the chip->base (aka "int pwm"), was not
set (thus, ==0) ; and 0 is a meaningful start value in alloc_pwm().
What was intended is mvpwm->chip->base = -1.
Like that, the numbering will be done auto-magically

Moreover, as the region might be already occupied by another pwm, we
shouldn't force:
mvpwm->chip->base = 0
nor
mvpwm->chip->base = id * MVEBU_MAX_GPIO_PER_BANK;

Tested on clearfog-pro (Marvell 88F6828)

Fixes: 757642f9a584 ("gpio: mvebu: Add limited PWM support")
Signed-off-by: Richard Genoud <richard.genoud@gmail.com>
---
 drivers/gpio/gpio-mvebu.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index cdef2c78cb3b..5104b6398139 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -768,6 +768,13 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
 	mvpwm->chip.dev = dev;
 	mvpwm->chip.ops = &mvebu_pwm_ops;
 	mvpwm->chip.npwm = mvchip->chip.ngpio;
+	/*
+	 * There may already be some PWM allocated, so we can't force
+	 * mvpwm->chip.base to a fixed point like mvchip->chip.base.
+	 * So, we let pwmchip_add() do the numbering and take the next free
+	 * region.
+	 */
+	mvpwm->chip.base = -1;
 
 	spin_lock_init(&mvpwm->lock);
 

[PATCHv2 2/2] gpio: mvebu: fix gpio bank registration when pwm is used

[Gregory CLEMENT]

Hi Richard,
 
 On jeu., juin 01 2017, Richard Genoud <richard.genoud@gmail.com> wrote:

> If more than one gpio bank has the "pwm" property, only one will be
> registered successfully, all the others will fail with:
> mvebu-gpio: probe of f1018140.gpio failed with error -17
>
> That's because in alloc_pwms(), the chip->base (aka "int pwm"), was not
> set (thus, ==0) ; and 0 is a meaningful start value in alloc_pwm().
> What was intended is mvpwm->chip->base = -1.
> Like that, the numbering will be done auto-magically
>
> Moreover, as the region might be already occupied by another pwm, we
> shouldn't force:
> mvpwm->chip->base = 0
> nor
> mvpwm->chip->base = id * MVEBU_MAX_GPIO_PER_BANK;
>
> Tested on clearfog-pro (Marvell 88F6828)
>
> Fixes: 757642f9a584 ("gpio: mvebu: Add limited PWM support")
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>

Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Thanks,

Gregory


> ---
>  drivers/gpio/gpio-mvebu.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> index cdef2c78cb3b..5104b6398139 100644
> --- a/drivers/gpio/gpio-mvebu.c
> +++ b/drivers/gpio/gpio-mvebu.c
> @@ -768,6 +768,13 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
>  	mvpwm->chip.dev = dev;
>  	mvpwm->chip.ops = &mvebu_pwm_ops;
>  	mvpwm->chip.npwm = mvchip->chip.ngpio;
> +	/*
> +	 * There may already be some PWM allocated, so we can't force
> +	 * mvpwm->chip.base to a fixed point like mvchip->chip.base.
> +	 * So, we let pwmchip_add() do the numbering and take the next free
> +	 * region.
> +	 */
> +	mvpwm->chip.base = -1;
>  
>  	spin_lock_init(&mvpwm->lock);
>  

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

[PATCHv2 2/2] gpio: mvebu: fix gpio bank registration when pwm is used

[Linus Walleij]

On Thu, Jun 1, 2017 at 2:18 PM, Richard Genoud <richard.genoud@gmail.com> wrote:

> If more than one gpio bank has the "pwm" property, only one will be
> registered successfully, all the others will fail with:
> mvebu-gpio: probe of f1018140.gpio failed with error -17
>
> That's because in alloc_pwms(), the chip->base (aka "int pwm"), was not
> set (thus, ==0) ; and 0 is a meaningful start value in alloc_pwm().
> What was intended is mvpwm->chip->base = -1.
> Like that, the numbering will be done auto-magically
>
> Moreover, as the region might be already occupied by another pwm, we
> shouldn't force:
> mvpwm->chip->base = 0
> nor
> mvpwm->chip->base = id * MVEBU_MAX_GPIO_PER_BANK;
>
> Tested on clearfog-pro (Marvell 88F6828)
>
> Fixes: 757642f9a584 ("gpio: mvebu: Add limited PWM support")
> Signed-off-by: Richard Genoud <richard.genoud@gmail.com>

Patch applied for fixes with Gregory's review tag.

Yours,
Linus Walleij