On Mon 25 Oct 01:42 PDT 2021, Uwe Kleine-K?nig wrote:
> Hello,
>
> [replaced Andrzej Hajda's email address with his new one]
>
> On Wed, Sep 29, 2021 at 10:05:57PM -0500, Bjorn Andersson wrote:
> > The SN65DSI86 provides the ability to supply a PWM signal on GPIO 4,
> > with the primary purpose of controlling the backlight of the attached
> > panel. Add an implementation that exposes this using the standard PWM
> > framework, to allow e.g. pwm-backlight to expose this to the user.
>
> Sorry for the long delay in reviewing this.
>
No worries, glad to hear from you again.
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> > ---
> >
[..]
> > +static int ti_sn_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > + const struct pwm_state *state)
> > +{
> > + struct ti_sn65dsi86 *pdata = pwm_chip_to_ti_sn_bridge(chip);
> > + unsigned int pwm_en_inv;
> > + unsigned int backlight;
> > + unsigned int pre_div;
> > + unsigned int scale;
> > + u64 period_max;
> > + u64 period;
> > + int ret;
> > +
> > + if (!pdata->pwm_enabled) {
> > + ret = pm_runtime_get_sync(pdata->dev);
> > + if (ret < 0) {
> > + pm_runtime_put_sync(pdata->dev);
> > + return ret;
> > + }
> > + }
> > +
> > + if (state->enabled) {
> > + if (!pdata->pwm_enabled) {
> > + /*
> > + * The chip might have been powered down while we
> > + * didn't hold a PM runtime reference, so mux in the
> > + * PWM function on the GPIO pin again.
> > + */
> > + ret = regmap_update_bits(pdata->regmap, SN_GPIO_CTRL_REG,
> > + SN_GPIO_MUX_MASK << (2 * SN_PWM_GPIO_IDX),
> > + SN_GPIO_MUX_SPECIAL << (2 * SN_PWM_GPIO_IDX));
> > + if (ret) {
> > + dev_err(pdata->dev, "failed to mux in PWM function\n");
> > + goto out;
> > + }
> > + }
> > +
> > + /*
> > + * Per the datasheet the PWM frequency is given by:
> > + *
> > + * REFCLK_FREQ
> > + * PWM_FREQ = -----------------------------------
> > + * PWM_PRE_DIV * BACKLIGHT_SCALE + 1
> > + *
> > + * However, after careful review the author is convinced that
> > + * the documentation has lost some parenthesis around
> > + * "BACKLIGHT_SCALE + 1".
> > + * With that the formula can be written:
> > + *
> > + * T_pwm * REFCLK_FREQ = PWM_PRE_DIV * (BACKLIGHT_SCALE + 1)
>
> For my understanding: T_pwm = period length = 1 / PWM_FREQ, right? Maybe
> it's a good idea to state this more explicitly?
>
Correct. I've improved the comment accordingly.
> > + * In order to keep BACKLIGHT_SCALE within its 16 bits,
> > + * PWM_PRE_DIV must be:
> > + *
> > + * T_pwm * REFCLK_FREQ
> > + * PWM_PRE_DIV >= -------------------------
> > + * BACKLIGHT_SCALE_MAX + 1
> > + *
> > + * To simplify the search and to favour higher resolution of
> > + * the duty cycle over accuracy of the period, the lowest
> > + * possible PWM_PRE_DIV is used. Finally the scale is
> > + * calculated as:
> > + *
> > + * T_pwm * REFCLK_FREQ
> > + * BACKLIGHT_SCALE = ---------------------- - 1
> > + * PWM_PRE_DIV
> > + *
> > + * Here T_pwm is represented in seconds, so appropriate scaling
> > + * to nanoseconds is necessary.
> > + */
> > +
> > + /* Minimum T_pwm is 1 / REFCLK_FREQ */
> > + if (state->period <= NSEC_PER_SEC / pdata->pwm_refclk_freq) {
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + /*
> > + * Maximum T_pwm is 255 * (65535 + 1) / REFCLK_FREQ
> > + * Limit period to this to avoid overflows
> > + */
> > + period_max = div_u64((u64)NSEC_PER_SEC * 255 * (65535 + 1),
> > + pdata->pwm_refclk_freq);
> > + if (period > period_max)
>
> period is uninitialized here. This must be
>
> if (state->period > period_max)
>
> . Alternatively to the if you could use
>
> period = min(state->period, period_max);
>
Yes of course.
>
> Apart from this I'm happy with your patch set now.
>
Thank you.
> > + period = period_max;
> > + else
> > + period = state->period;
> > +
> > + pre_div = DIV64_U64_ROUND_UP(period * pdata->pwm_refclk_freq,
> > + (u64)NSEC_PER_SEC * (BACKLIGHT_SCALE_MAX + 1));
> > + scale = div64_u64(period * pdata->pwm_refclk_freq, (u64)NSEC_PER_SEC * pre_div) - 1;
>
> After thinking a while about this---I think I stumbled about this
> calculation already in earlier revisions of this patch set---I think I
> now understood it. I never saw something like this before because other
> drivers with similar HW conditions would pick:
>
> pre_div = div64_u64(period * pdata->pwm_refclk_freq,
> (u64)NSEC_PER_SEC * (BACKLIGHT_SCALE_MAX + 1));
>
> and then scale = BACKLIGHT_SCALE_MAX. This latter approach weights high
> resolution of duty_cycle still higher over period exactness than your
> approach.
Interesting.
> For me both approaches are fine.
>
Thanks, I'll respin with the two minor things above and leave the math
as is now :)
Regards,
Bjorn
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | https://www.pengutronix.de/ |
