On Thu 17 Jun 01:24 CDT 2021, Uwe Kleine-K?nig wrote: > Hello Bjorn, > > On Wed, Jun 16, 2021 at 10:22:17PM -0500, Bjorn Andersson wrote: > > > > +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; > > > > + int ret; > > > > + > > > > + if (!pdata->pwm_enabled) { > > > > + ret = pm_runtime_get_sync(pdata->dev); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > + 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; > > > > + } > > > > > > Do you need to do this even if state->enabled is false? > > > > I presume I should be able to explicitly mux in the GPIO function and > > configure that to output low. But I am not able to find anything in the > > data sheet that would indicate this to be preferred. > > My question targetted a different case. If the PWM is off > (!pdata->pwm_enabled) and should remain off (state->enabled is false) > you can shortcut here, can you not? > Right, if we're going off->off then we don't need to touch the hardware. But am I expected to -EINVAL improper period and duty cycle even though enabled is false? And also, what is the supposed behavior of enabled = false? Is it supposedly equivalent of asking for a duty_cycle of 0? > > > Does this already modify the output pin? > > > > Yes, coming out of reset this pin is configured as input, so switching > > the mux here will effectively start driving the pin. > > So please document this in the format the recently added drivers do, > too. See e.g. drivers/pwm/pwm-sifive.c. (The idea is to start that with > " * Limitations:" to make it easy to grep it.) > Okay, will do. Although I believe that for this driver it makes sense to place such comment close to this function, rather than at the top of the driver. > > > Lets continue the above example with the fixed calculation. So we have: > > > > > > pdata->pwm_refclk_freq = 3333334 > > > state->period = 100000 [ns] > > > state->duty_cycle = 600 > > > scale = 332 > > > > > > so the actually emitted period = 99899.98002000399 ns > > > > > > Now you calculate: > > > > > > backlight = 1 > > > > > > which yields an actual duty_cycle of 299.99994 ns, with backlight = 2 > > > you would get an actual duty_cycle of 599.99988 ns, which is better. The > > > culprit here is that you divide by state->period but instead should > > > divide by the actual period. > > > > What do I do about the case where the actual period is lower than the > > requested one and thereby the duty cycle becomes larger than the period? > > The general principle is: Pick the biggest possible duty_cycle available > for the just picked period. So in your example you have to clamp it to > period (assuming you can, otherwise pick the next lower possible value). > Sounds good. Thank you, Bjorn > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |