lkml.org 
[lkml]   [2019]   [Aug]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 6/7] pwm: jz4740: Make PWM start with the active part
    On Mon, Aug 12, 2019 at 10:50:01PM +0200, Paul Cercueil wrote:
    >
    >
    > Le lun. 12 août 2019 à 7:55, Uwe =?iso-8859-1?q?Kleine-K=F6nig?=
    > <u.kleine-koenig@pengutronix.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@pengutronix.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?

    Yes, a constant inactive state is expected. This is consistent and in a
    similar way when using duty == period an constant active output is
    expected.

    > Then I guess I can just disable the PWM output in the driver when
    > configured with duty == 0.

    Some time ago I argued with Thierry that we could drop the concept of
    enabled/disabled for a PWM because a disabled PWM is supposed to behave
    identically to duty=0. This is however only nearly true because with
    duty=0 the time the PWM is inactive still is a multiple of the period.

    I tend to agree that disabling the PWM when duty=0 is requested is
    better than to fail the request (or configure for duty=1 $whateverunit).
    I'm looking forward to what Thierry's opinion is here.

    > > > 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.

    Note that the wrong border (because duty=0 is impossible for your
    hardware) shifts the whole space. The right inverse of duty = period - 1
    is duty = 1, isn't it?

    > > > > > Signed-off-by: Paul Cercueil <paul@crapouillou.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.

    Then please mention it in the commit log.

    Best regards
    Uwe

    --
    Pengutronix e.K. | Uwe Kleine-König |
    Industrial Linux Solutions | http://www.pengutronix.de/ |

    \
     
     \ /
      Last update: 2019-08-12 23:59    [W:3.725 / U:0.084 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site