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;elseperiod = 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/ |