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