lkml.org 
[lkml]   [2021]   [Oct]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v6 3/3] drm/bridge: ti-sn65dsi86: Implement the pwm_chip
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@linaro.org>
> > ---
> >
[..]
> > +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/ |


\
 
 \ /
  Last update: 2021-10-25 17:26    [W:0.141 / U:0.392 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site