Re: [PATCH v2 2/2] drm/bridge: ti-sn65dsi86: Implement the pwm_chip

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 5483 bytes --]

Hello Bjorn,

On Thu, Jun 17, 2021 at 01:06:51PM -0500, Bjorn Andersson wrote:
> On Thu 17 Jun 11:54 CDT 2021, Uwe Kleine-K?nig wrote:
> > 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.
> 
> Giving this some further thought.

Very appreciated, still more as you come to the same conclusions as I do
:-)

> In order to have a known state of the PWM signal we need the sn65dsi86
> to be powered. The documentation describes a "suspend mode", but this is
> currently not implemented in the driver, so there's a large power cost
> coming from just keeping the pin low when disabled.

In the past I already promoted the idea that a disabled PWM should give
no guarantees about the output level. In fact there are several
offenders, the ones I know off-hand are:

 - pwm-imx27 emits a low level independent of the programmed polarity
 - pwm-iqs620a makes the output tristated and so relies on an external
   pull to give the inactive level.
 - pwm-sl28cpld switches to a GPIO mode on disable which isn't
   controlled by the driver

and I assume there are more because before no one actively asked for
and tracked this kind of information.

IMHO a consumer who wants the output to stay inactive should configure

	.enabled = true
	.period = DC (or something low to allow quick reprogramming)
	.duty_cycle = 0

, so there is no loss of functionality and enabled=false should mean the
consumer doesn't care about the output which would allow some lowlevel
drivers to save some more energy. So this makes the API more expressive
because after dropping "disabled results in an inactive output"
consumers can differentiate between "I care about the output staying
inactive" and "I don't care". This in turn allows lowlevel drivers to
better know when they can more aggressively save power and when they
don't.

Back then Thierry didn't like that approach though. (The thread started
with a mail having Message-Id
20180820144357.7206-1-u.kleine-koenig@pengutronix.de, this is missing on
lore.kernel.org however and I didn't find it on another mirror.)

Thierry's arguments were:

 - "An API whose result is an undefined state is useless in my opinion."
   (from Message-Id: 20181009073407.GA5565@ulmo)
   Yes, this is a drawback for some consumers, but it matches reality
   that disabling the PWM counter on some PWM implementations doesn't
   result in an inactive level. And if they care about the output, they
   just use .duty_cycle = 0 + .enabled = true instead.
   In my book changing the semantic fixes a bug because the API promises
   more than some drivers are capable to do (with reasonable effort).

 - "[Emitting the inactive level] also matches what all other drivers,
   and users, assume." (also from Message-Id: 20181009073407.GA5565@ulmo)
   For the drivers this was an assumption, today we know it's wrong.
   Users can be fixed.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]