Hello Bjorn,
On Thu, Jun 17, 2021 at 11:38:26AM -0500, Bjorn Andersson wrote:
On Thu 17 Jun 01:24 CDT 2021, Uwe Kleine-K?nig wrote:
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?
In my book enabled = false is just syntactic sugar to say: "duty_cycle=0, period=something small". So to answer your questions: if enabled = false, the consumer doesn't really care about period and duty_cycle. Some care that the output becomes inactive, some others don't, so from my POV just emit the inactive level on the output and ignore period and duty_cycle.
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.
Yes, agreed.
Best regards Uwe