Re: [PATCH v6 3/3] drm/bridge: ti-sn65dsi86: Implement the pwm_chip

[Bjorn Andersson]

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