| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <1565643001.2007.2@crapouillou.net>
Date: Mon, 12 Aug 2019 22:50:01 +0200
From: Paul Cercueil <paul@...pouillou.net>
To: Uwe Kleine-König
<u.kleine-koenig@...gutronix.de>
Cc: Thierry Reding <thierry.reding@...il.com>, od@...c.me,
linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/7] pwm: jz4740: Make PWM start with the active part
Le lun. 12 août 2019 à 7:55, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
<u.kleine-koenig@...gutronix.de> a écrit :
> On Fri, Aug 09, 2019 at 07:33:24PM +0200, Paul Cercueil wrote:
>>
>>
>> Le ven. 9 août 2019 à 19:10, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
>> <u.kleine-koenig@...gutronix.de> a écrit :
>> > On Fri, Aug 09, 2019 at 02:30:30PM +0200, Paul Cercueil wrote:
>> > > The PWM will always start with the inactive part. To counter
>> that,
>> > > when PWM is enabled we switch the configured polarity, and use
>> > > 'period - duty + 1' as the real duty.
>> >
>> > Where does the + 1 come from? This looks wrong. (So if duty=0 is
>> > requested you use duty = period + 1?)
>>
>> You'd never request duty == 0, would you?
>>
>> Your duty must always be in the inclusive range [1, period]
>> (hardware values, not ns). A duty of 0 is a hardware fault
>> (on the jz4740 it is).
>
> From the PWM framework's POV duty cycle = 0 is perfectly valid.
> Similar
> to duty == period. Not supporting dutz cycle 0 is another limitation
> of
> your PWM that should be documented.
>
> For actual use cases of duty cycle = 0 see drivers/hwmon/pwm-fan.c or
> drivers/leds/leds-pwm.c.
Perfectly valid for the PWM framework, maybe; but what is the expected
output then? A constant inactive state? Then I guess I can just disable
the PWM output in the driver when configured with duty == 0.
>> If you request duty == 1 (the minimum), then the new duty is equal
>> to (period - 1 + 1) == period, which is the maximum of your range.
>>
>> If you request duty == period (the maximum), then the new duty
>> calculated is equal to (period - period + 1) == 1, which is the
>> minimum of your range.
>>
>>
>> > >
>> > > Signed-off-by: Paul Cercueil <paul@...pouillou.net>
>> > > ---
>> > > drivers/pwm/pwm-jz4740.c | 22 +++++++++++++---------
>> > > 1 file changed, 13 insertions(+), 9 deletions(-)
>> > >
>> > > diff --git a/drivers/pwm/pwm-jz4740.c
>> b/drivers/pwm/pwm-jz4740.c
>> > > index 85e2110aae4f..8df898429d47 100644
>> > > --- a/drivers/pwm/pwm-jz4740.c
>> > > +++ b/drivers/pwm/pwm-jz4740.c
>> > > @@ -121,6 +121,7 @@ static int jz4740_pwm_apply(struct pwm_chip
>> > > *chip, struct pwm_device *pwm,
>> > > *parent_clk = clk_get_parent(clk);
>> > > unsigned long rate, parent_rate, period, duty;
>> > > unsigned long long tmp;
>> > > + bool polarity_inversed;
>> > > int ret;
>> > >
>> > > parent_rate = clk_get_rate(parent_clk);
>> > > @@ -183,24 +184,27 @@ static int jz4740_pwm_apply(struct
>> pwm_chip
>> > > *chip, struct pwm_device *pwm,
>> > > /* Reset counter to 0 */
>> > > regmap_write(jz4740->map, TCU_REG_TCNTc(pwm->hwpwm), 0);
>> > >
>> > > - /* Set duty */
>> > > - regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), duty);
>> > > -
>> > > /* Set period */
>> > > regmap_write(jz4740->map, TCU_REG_TDFRc(pwm->hwpwm), period);
>> > >
>> > > + /*
>> > > + * The PWM will always start with the inactive part. To
>> counter that,
>> > > + * when PWM is enabled we switch the configured polarity,
>> and use
>> > > + * 'period - duty + 1' as the real duty.
>> > > + */
>> > > +
>> > > + /* Set duty */
>> > > + regmap_write(jz4740->map, TCU_REG_TDHRc(pwm->hwpwm), period
>> - duty + 1);
>> > > +
>> >
>> > Before you set duty first, then period, now you do it the other
>> way
>> > round. Is there a good reason?
>>
>> To move it below the comment that explains why we use 'period -
>> duty + 1'.
>>
>> We modify that line anyway, so it's not like it makes the patch
>> much more
>> verbose.
>
> It doesn't make it more verbose, but that's not the background of my
> question. For most(?) PWM implementation the order of hardware
> accesses
> matters and introducing such a difference as an unneeded side effect
> isn't optimal.
There's no side effect. The PWM is disabled when reconfigured.
> Why not add the comment above the line that already used to set the
> duty
> in hardware?
I thought it made sense to have the two parts of the trick closer
together
in the code, below the comment, so that it's clearer what it does.
>> > > /* Set polarity */
>> > > - switch (state->polarity) {
>> > > - case PWM_POLARITY_NORMAL:
>> > > + polarity_inversed = state->polarity == PWM_POLARITY_INVERSED;
>> > > + if (!polarity_inversed ^ state->enabled)
>> >
>> > Why does state->enabled suddenly matter here?
>>
>> The pin stay inactive when the PWM is disabled, but the level of the
>> inactive state depends on the polarity of the pin. So we need to
>> switch
>> the polarity only when the PWM is enabled.
>
> After some thought I got that. When knowing this, this is already
> mentioned in the comment you introduced as you write about enabled
> PWMs
> only. Maybe it's just me, but mentioning that case more explicit would
> have helped me. Something like:
>
> /*
> * The hardware always starts a period with the inactive part.
> * So invert polarity and duty cycle to yield the output that is
> * expected by the PWM framework and its users. This inversion
> * must not be done for a disabled PWM however because otherwise
> * it outputs a constant active level.
> */
Ok.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König
> |
> Industrial Linux Solutions |
> http://www.pengutronix.de/ |
Powered by blists - more mailing lists