From: Richard Genoud <richard.genoud@gmail.com> To: Gregory CLEMENT <gregory.clement@free-electrons.com> Cc: Linus Walleij <linus.walleij@linaro.org>, Alexandre Courbot <gnurou@gmail.com>, Andrew Lunn <andrew@lunn.ch>, Jason Cooper <jason@lakedaemon.net>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, linux-pwm@vger.kernel.org, Mark Rutland <mark.rutland@arm.com>, Ralph Sennhauser <ralph.sennhauser@gmail.com>, Rob Herring <robh+dt@kernel.org>, Russell King <linux@armlinux.org.uk>, Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>, Thierry Reding <thierry.reding@gmail.com> Subject: Re: [PATCH 2/2] gpio: mvebu: fix gpio bank registration when pwm is used Date: Tue, 30 May 2017 16:45:24 +0200 [thread overview] Message-ID: <CACQ1gAhy_S16YfxQ1eOJUX45rdnjrsPFckYJm_1OHGW0CThg0g@mail.gmail.com> (raw) In-Reply-To: <87y3tetrg4.fsf@free-electrons.com> 2017-05-30 15:16 GMT+02:00 Gregory CLEMENT <gregory.clement@free-electrons.com>: > Hi Richard, Hi Greg ! > > On mar., mai 30 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 chip->base = -1. >> Like that, the numbering will be done auto-magically >> >> Tested on clearfog-pro (Marvell 88F6828) >> >> Signed-off-by: Richard Genoud <richard.genoud@gmail.com> >> --- >> drivers/gpio/gpio-mvebu.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c >> index cdef2c78cb3b..4734923e11fd 100644 >> --- a/drivers/gpio/gpio-mvebu.c >> +++ b/drivers/gpio/gpio-mvebu.c >> @@ -768,6 +768,7 @@ 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; >> + mvpwm->chip.base = -1; > > Why not using > mvpwm->chip.base = id * MVEBU_MAX_GPIO_PER_BANK; > as it is done in the mvebu_gpio_probe() function? Yes, that was my first move: mvpwm->chip.base = mvchip->chip.base; But after some reflexion, mvpwm->chip.base is not the GPIO base, it's the PWM base, (mvpwm->chip is a struct pwm_chip), so it would we weird to have "holes" in the declared PWMs. I'm not clear, so here's an example: If, in the DTS, we have: gpio0: gpio@18100 { compatible = "marvell,armada-370-xp-gpio", "marvell,orion-gpio"; reg = <0x18100 0x40>, <0x181c0 0x08>; reg-names = "gpio"; /* "pwm" missing */ [...] gpio1: gpio@18140 { compatible = "marvell,armada-370-xp-gpio", "marvell,orion-gpio"; reg = <0x18140 0x40>, <0x181c8 0x08>; reg-names = "gpio", "pwm"; In this case, if gpio0 is not declared as PWM capable, the PWM numbering will start at 32 if we have mvpwm->chip.base = mvchip->chip.base; but it will start at 0 if we have mvpwm->chip.base = -1; The pros for having mvpwm->chip.base = mvchip->chip.base; is mainly the stable numbering: if we add the "pwm" feature to gpio0 afterwards, the pwm numbering in sysfs will stay the same. And if we have mvpwm->chip.base = -1; the pwm numbering will be shifted. Looking back at the V5 of this patch https://www.spinics.net/lists/kernel/msg2484889.html There was the line: mvpwm->chip.base = mvchip->chip.base; I guess it got lost in the v6 rebase. So I could change it back, but I'm not sure which one is better. > > I think that if you use base = -1, then the number start from (512 - > number of pin already use). So starting from a low number for one > compatible and a high number for an other compatible could be confusing. > > Besides that I agree that mvpwm->chip.base must be initialized and here > again for adding mor context to this patch, we could add: > > Fixes: 757642f9a584 ("gpio: mvebu: Add limited PWM support") yes, definitely ! should I resend the patch with it or the maintainer will add it ? > Gregory > >> >> 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
WARNING: multiple messages have this Message-ID (diff)
From: richard.genoud@gmail.com (Richard Genoud) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 2/2] gpio: mvebu: fix gpio bank registration when pwm is used Date: Tue, 30 May 2017 16:45:24 +0200 [thread overview] Message-ID: <CACQ1gAhy_S16YfxQ1eOJUX45rdnjrsPFckYJm_1OHGW0CThg0g@mail.gmail.com> (raw) In-Reply-To: <87y3tetrg4.fsf@free-electrons.com> 2017-05-30 15:16 GMT+02:00 Gregory CLEMENT <gregory.clement@free-electrons.com>: > Hi Richard, Hi Greg ! > > On mar., mai 30 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 chip->base = -1. >> Like that, the numbering will be done auto-magically >> >> Tested on clearfog-pro (Marvell 88F6828) >> >> Signed-off-by: Richard Genoud <richard.genoud@gmail.com> >> --- >> drivers/gpio/gpio-mvebu.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c >> index cdef2c78cb3b..4734923e11fd 100644 >> --- a/drivers/gpio/gpio-mvebu.c >> +++ b/drivers/gpio/gpio-mvebu.c >> @@ -768,6 +768,7 @@ 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; >> + mvpwm->chip.base = -1; > > Why not using > mvpwm->chip.base = id * MVEBU_MAX_GPIO_PER_BANK; > as it is done in the mvebu_gpio_probe() function? Yes, that was my first move: mvpwm->chip.base = mvchip->chip.base; But after some reflexion, mvpwm->chip.base is not the GPIO base, it's the PWM base, (mvpwm->chip is a struct pwm_chip), so it would we weird to have "holes" in the declared PWMs. I'm not clear, so here's an example: If, in the DTS, we have: gpio0: gpio at 18100 { compatible = "marvell,armada-370-xp-gpio", "marvell,orion-gpio"; reg = <0x18100 0x40>, <0x181c0 0x08>; reg-names = "gpio"; /* "pwm" missing */ [...] gpio1: gpio at 18140 { compatible = "marvell,armada-370-xp-gpio", "marvell,orion-gpio"; reg = <0x18140 0x40>, <0x181c8 0x08>; reg-names = "gpio", "pwm"; In this case, if gpio0 is not declared as PWM capable, the PWM numbering will start at 32 if we have mvpwm->chip.base = mvchip->chip.base; but it will start at 0 if we have mvpwm->chip.base = -1; The pros for having mvpwm->chip.base = mvchip->chip.base; is mainly the stable numbering: if we add the "pwm" feature to gpio0 afterwards, the pwm numbering in sysfs will stay the same. And if we have mvpwm->chip.base = -1; the pwm numbering will be shifted. Looking back at the V5 of this patch https://www.spinics.net/lists/kernel/msg2484889.html There was the line: mvpwm->chip.base = mvchip->chip.base; I guess it got lost in the v6 rebase. So I could change it back, but I'm not sure which one is better. > > I think that if you use base = -1, then the number start from (512 - > number of pin already use). So starting from a low number for one > compatible and a high number for an other compatible could be confusing. > > Besides that I agree that mvpwm->chip.base must be initialized and here > again for adding mor context to this patch, we could add: > > Fixes: 757642f9a584 ("gpio: mvebu: Add limited PWM support") yes, definitely ! should I resend the patch with it or the maintainer will add it ? > Gregory > >> >> 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
next prev parent reply other threads:[~2017-05-30 14:45 UTC|newest] Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-05-30 12:28 [PATCH 1/2] gpio: mvebu: fix blink counter register selection Richard Genoud 2017-05-30 12:28 ` Richard Genoud 2017-05-30 12:28 ` [PATCH 2/2] gpio: mvebu: fix gpio bank registration when pwm is used Richard Genoud 2017-05-30 12:28 ` Richard Genoud 2017-05-30 13:16 ` Gregory CLEMENT 2017-05-30 13:16 ` Gregory CLEMENT 2017-05-30 14:45 ` Richard Genoud [this message] 2017-05-30 14:45 ` Richard Genoud 2017-05-30 14:45 ` Richard Genoud 2017-05-30 15:14 ` Ralph Sennhauser 2017-05-30 15:14 ` Ralph Sennhauser 2017-05-30 15:14 ` Ralph Sennhauser 2017-05-30 16:35 ` Richard Genoud 2017-05-30 16:35 ` Richard Genoud 2017-05-30 16:35 ` Richard Genoud 2017-05-30 13:01 ` [PATCH 1/2] gpio: mvebu: fix blink counter register selection Gregory CLEMENT 2017-05-30 13:01 ` Gregory CLEMENT 2017-05-30 13:06 ` Gregory CLEMENT 2017-05-30 13:06 ` Gregory CLEMENT 2017-05-30 15:18 ` Ralph Sennhauser 2017-05-30 15:18 ` Ralph Sennhauser
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CACQ1gAhy_S16YfxQ1eOJUX45rdnjrsPFckYJm_1OHGW0CThg0g@mail.gmail.com \ --to=richard.genoud@gmail.com \ --cc=andrew@lunn.ch \ --cc=gnurou@gmail.com \ --cc=gregory.clement@free-electrons.com \ --cc=jason@lakedaemon.net \ --cc=linus.walleij@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-gpio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pwm@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=mark.rutland@arm.com \ --cc=ralph.sennhauser@gmail.com \ --cc=robh+dt@kernel.org \ --cc=sebastian.hesselbarth@gmail.com \ --cc=thierry.reding@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.