Message-ID: <20210623082915.tj7pid46wm3dl5jf@pengutronix.de>
Date:   Wed, 23 Jun 2021 10:29:15 +0200
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     Andrzej Hajda <a.hajda@...sung.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Robert Foss <robert.foss@...aro.org>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Jonas Karlman <jonas@...boo.se>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Thierry Reding <thierry.reding@...il.com>,
        Lee Jones <lee.jones@...aro.org>,
        Doug Anderson <dianders@...omium.org>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        linux-pwm@...r.kernel.org, kernel@...gutronix.de
Subject: Re: [PATCH v3 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip

Hello Bjorn,

On Tue, Jun 22, 2021 at 08:12:48PM -0500, Bjorn Andersson wrote:
> On Tue 22 Jun 15:29 CDT 2021, Uwe Kleine-K?nig wrote:
> > On Mon, Jun 21, 2021 at 10:09:48PM -0500, Bjorn Andersson wrote:
> > > +		/*
> > > +		 * PWM duty cycle is given as:
> > > +		 *
> > > +		 *   duty = BACKLIGHT / (BACKLIGHT_SCALE + 1)
> > > +		 *
> > > +		 * The documentation is however inconsistent in its examples,
> > > +		 * so the interpretation used here is that the duty cycle is
> > > +		 * the period of BACKLIGHT * PRE_DIV / REFCLK_FREQ.
> > 
> > I don't understand this.
> > 
> > > +		 *
> > > +		 * The ratio PRE_DIV / REFCLK_FREQ is rounded up to whole
> > > +		 * nanoseconds in order to ensure that the calculations are
> > > +		 * idempotent and gives results that are smaller than the
> > > +		 * requested value.
> > > +		 */
> > > +		tick = DIV_ROUND_UP(NSEC_PER_SEC * pre_div, pdata->pwm_refclk_freq);
> > > +		backlight = state->duty_cycle / tick;
> > 
> > You're loosing precision here by dividing by the result of a division.
> 
> The actual period is also a result of a division and after spending too
> many hours scratching my head I reached to conclusion that this was the
> reason why I wasn't able to get the duty cycle calculation idempotent
> over the ranges I tested.

How did you test? Using the sysfs interface?
 
> But in my effort to describe this to you here, I finally spotted the
> error and will follow up with a new version that does:
> 
>                 actual = NSEC_PER_SEC * (pre_div * scale + 1) / pdata->pwm_refclk_freq);
>                 backlight = state->duty_cycle * (scale + 1) / actual;

So the first term ("actual") is the period that you get for a given
pre_div, scale and pwm_refclk_freq, right? And the 2nd ("backlight")
defines the register value to configure the duty_cycle, right?

I wonder: Is it possible to configure a 100% relative duty cycle? Then
backlight would be scale + 1 which (at least if scale is 0xffff) would
overflow the 16 bit register width?!

> > > +static void ti_sn_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +				struct pwm_state *state)
> > > +{
> > > +	struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > > +	unsigned int pwm_en_inv;
> > > +	unsigned int pre_div;
> > > +	u16 backlight;
> > > +	u16 scale;
> > > +	int ret;
> > > +
> > > +	ret = regmap_read(pdata->regmap, SN_PWM_EN_INV_REG, &pwm_en_inv);
> > > +	if (ret)
> > > +		return;
> > > +
> > > +	ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_SCALE_REG, &scale);
> > > +	if (ret)
> > > +		return;
> > > +
> > > +	ret = ti_sn65dsi86_read_u16(pdata, SN_BACKLIGHT_REG, &backlight);
> > > +	if (ret)
> > > +		return;
> > > +
> > > +	ret = regmap_read(pdata->regmap, SN_PWM_PRE_DIV_REG, &pre_div);
> > > +	if (ret)
> > > +		return;
> > > +
> > > +	state->enabled = FIELD_GET(SN_PWM_EN_MASK, pwm_en_inv);
> > > +	if (FIELD_GET(SN_PWM_INV_MASK, pwm_en_inv))
> > > +		state->polarity = PWM_POLARITY_INVERSED;
> > > +	else
> > > +		state->polarity = PWM_POLARITY_NORMAL;
> > > +
> > > +	state->period = DIV_ROUND_UP(NSEC_PER_SEC * (pre_div * scale + 1), pdata->pwm_refclk_freq);
> > > +	state->duty_cycle = backlight * DIV_ROUND_UP(NSEC_PER_SEC * pre_div, pdata->pwm_refclk_freq);
> > 
> > If you use
> > 
> > 	state->duty_cycle = DIV_ROUND_UP(backlight * NSEC_PER_SEC * pre_div, pdata->pwm_refclk_freq);
> > 
> > instead (with a cast to u64 to not yield an overflow) the result is more
> > exact.
> > 
> 
> The problem with this is that it sometimes yields duty_cycles larger
> than what was requested... But going back to describing this as a ratio
> of the period this becomes:
> 
>         state->duty_cycle = DIV_ROUND_UP_ULL(state->period * backlight, scale + 1);

I saw your next iteration of this patch set, but didn't look into it
yet. Note that if it uses this formula it sill looses precision.
Consider:

	pwm_refclk_freq = 1333333
	pre_div = 4
	scale = 60000
	backlight = 59999

then you calculate:

	state->period = 180000796 (exact value: 180000795.00019875)
	state->duty_cycle = 179994797 (exact value: 179994795.0736975)

so duty_cycle should actually be reported as 179994796. That happens
because state->period is already the result of a division, you get the
right value when doing:

	state->duty_cycle = round_up(NSEC_PER_SEC * (pre_div * scale + 1) * backlight, (scale + 1) * pdata->pwm_refclk_freq)

> > I still find this surprising, I'd expect that SCALE also matters for the
> > duty_cycle. With the assumption implemented here modifying SCALE only
> > affects the period. This should be easy to verify?! I would expect that
> > changing SCALE doesn't affect the relative duty_cycle, so the brightness
> > of an LED is unaffected (unless the period gets too big of course).
> > 
> 
> I think the hardware is two nested counters, one (A) ticking at REFCLK_FREQ
> and as that hits PRE_DIV, it kicks the second counter (B) (and resets).
> 
> As counter A is reset the output signal goes high, when A hits BACKLIGHT the
> signal goes low and when A hits BACKLIGHT_SCALE it resets.

then we would probably have:

	period = (scale + 1) * pre_div / refclk

but not

	period = (scale * pre_div + 1) / refclk

. The former would be nicer because then in the calculation for
duty_cycle the factor (scale + 1) would cancel.

> Per this implementation the actual length of the duty cycle would indeed
> be independent of the BACKLIGHT_SCALE,

In your formula for duty_cycle scale actually does matter. So I think
we're not there yet?

> but the length of the low signal (and hence the ratio, which results
> in the actual brightness) does depend on BACKLIGHT_SCALE.

Best regards
Uwe

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists