Date: Wed, 22 May 2024 13:13:35 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc: linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	linux-gpio@...r.kernel.org, linux-pwm@...r.kernel.org,
	Bartosz Golaszewski <brgl@...ev.pl>,
	Conor Dooley <conor+dt@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Lee Jones <lee@...nel.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Rob Herring <robh@...nel.org>,
	Uwe Kleine-König <ukleinek@...nel.org>,
	Clark Wang <xiaoning.wang@....com>
Subject: Re: [PATCH 5/5] pwm: adp5585: Add Analog Devices ADP5585 support

On Tue, May 21, 2024 at 03:05:53PM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> [dropping Alexandru Ardelean from Cc as their address bounces]
> 
> On Tue, May 21, 2024 at 01:09:22PM +0300, Laurent Pinchart wrote:
> > On Tue, May 21, 2024 at 10:51:26AM +0200, Uwe Kleine-König wrote:
> > > On Mon, May 20, 2024 at 10:59:41PM +0300, Laurent Pinchart wrote:
> > > > +	ret = regmap_update_bits(adp5585_pwm->regmap, ADP5585_GENERAL_CFG,
> > > > +				 ADP5585_OSC_EN, ADP5585_OSC_EN);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	return 0;
> > > 
> > > The last four lines are equivalent to
> > > 
> > > 	return ret;
> > 
> > I prefer the existing code but can also change it.
> 
> Well, I see the upside of your approach. If this was my only concern I
> wouldn't refuse to apply the patch.

While I have my preferences, I also favour consistency, so I'm happy to
comply with the preferred coding style for the subsystem :-) I'll
update this in the next version.

> > > > +	regmap_update_bits(adp5585_pwm->regmap, ADP5585_GENERAL_CFG,
> > > > +			   ADP5585_OSC_EN, 0);
> > > > +}
> > > > +
> > > > +static int pwm_adp5585_apply(struct pwm_chip *chip,
> > > > +			     struct pwm_device *pwm,
> > > > +			     const struct pwm_state *state)
> > > > +{
> > > > +	struct adp5585_pwm_chip *adp5585_pwm = to_adp5585_pwm_chip(chip);
> > > > +	u32 on, off;
> > > > +	int ret;
> > > > +
> > > > +	if (!state->enabled) {
> > > > +		guard(mutex)(&adp5585_pwm->lock);
> > > > +
> > > > +		return regmap_update_bits(adp5585_pwm->regmap, ADP5585_PWM_CFG,
> > > > +					  ADP5585_PWM_EN, 0);
> > > > +	}
> > > > +
> > > > +	if (state->period < ADP5585_PWM_MIN_PERIOD_NS ||
> > > > +	    state->period > ADP5585_PWM_MAX_PERIOD_NS)
> > > > +		return -EINVAL;
> > > 
> > > Make this:
> > > 
> > > 	if (state->period < ADP5585_PWM_MIN_PERIOD_NS)
> > > 		return -EINVAL;
> > > 
> > > 	period = min(ADP5585_PWM_MAX_PERIOD_NS, state->period)
> > > 	duty_cycle = min(period, state->period);
> > 
> > I haven't been able to find documentation about the expected behaviour.
> > What's the rationale for returning an error if the period is too low,
> > but silently clamping it if it's too high ?
> 
> Well, it's only implicitly documented in the implementation of
> PWM_DEBUG. The reasoning is a combination of the following thoughts:
> 
>  - Requiring exact matches is hard to work with, so some deviation
>    between request and configured value should be allowed.
>  - Rounding in both directions has strange and surprising effects. The
>    corner cases (for all affected parties (=consumer, lowlevel driver
>    and pwm core)) are easier if you only round in one direction.
>    One ugly corner case in your suggested patch is:
>    ADP5585_PWM_MAX_PERIOD_NS corresponds to 0xffff clock ticks.
>    If the consumer requests period=64000.2 clock ticks, you configure
>    for 64000. If the consumer requests period=65535.2 clock ticks you
>    return -EINVAL.
>    Another strange corner case is: Consider a hardware that can
>    implement the following periods 499.7 ns, 500.2 ns, 500.3 ns and then
>    only values >502 ns.
>    If you configure for 501 ns, you'd get 500.3 ns. get_state() would
>    tell you it's running at 500 ns. If you then configure 500 ns you
>    won't get 500.3 ns any more.
>  - If you want to allow 66535.2 clock ticks (and return 65535), what
>    should be the maximal value that should yield 65535? Each cut-off
>    value is arbitrary, so using \infty looks reasonable (to me at
>    least).
>  - Rounding down is easier than rounding up, because that's what C's /
>    does. (Well, this is admittedly a bit arbitrary, because if you round
>    down in .apply() you have to round up in .get_state().)

Thank you for the detailed explanation.

> > > round-closest is wrong. Testing with PWM_DEBUG should point that out.
> > > The right algorithm is:
> > > 
> > > 	on = duty_cycle / (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ)
> > > 	off = period / (NSEC_PER_SEC / ADP5585_PWM_OSC_FREQ_HZ) - on
> > > 
> > > 
> > > > +	if (state->polarity == PWM_POLARITY_INVERSED)
> > > > +		swap(on, off);
> > > 
> > > Uhh, no. Either you can do inverted polarity or you cannot. Don't claim
> > > you can.
> > 
> > OK, but what's the rationale ? This is also an area where I couldn't
> > find documentation.
> 
> I don't have a good rationale here. IMHO this inverted polarity stuff is
> only a convenience for consumers because the start of the period isn't
> visible from the output wave form (apart from (maybe) the moment where
> you change the configuration) and so
> 
> 	.period = 5000, duty_cycle = 1000, polarity = PWM_POLARITY_NORMAL
> 
> isn't distinguishable from
> 
> 	.period = 5000, duty_cycle = 4000, polarity = PWM_POLARITY_INVERSED
> 
> . But it's a historic assumption of the pwm core that there is a
> relevant difference between the two polarities and I want at least a
> consistent behaviour among the lowlevel drivers. BTW, this convenience
> is the reason I'm not yet clear how I want to implemement a duty_offset.

Consistency is certainly good. Inverting the duty cycle to implement
inverted polarity would belong in the PWM core if we wanted to implement
it in software I suppose. I'll drop it from the driver.

> > > > +	ret = devm_pwmchip_add(&pdev->dev, &adp5585_pwm->chip);
> > > > +	if (ret) {
> > > > +		mutex_destroy(&adp5585_pwm->lock);
> > > > +		return dev_err_probe(&pdev->dev, ret, "failed to add PWM chip\n");
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void adp5585_pwm_remove(struct platform_device *pdev)
> > > > +{
> > > > +	struct adp5585_pwm_chip *adp5585_pwm = platform_get_drvdata(pdev);
> > > > +
> > > > +	mutex_destroy(&adp5585_pwm->lock);
> > > 
> > > Huh, this is a bad idea. The mutex is gone while the pwmchip is still
> > > registered. AFAIK calling mutex_destroy() is optional, and
> > > adp5585_pwm_remove() can just be dropped. Ditto in the error paths of
> > > .probe().
> > 
> > mutex_destroy() is a no-op when !CONFIG_DEBUG_MUTEXES. When the config
> > option is selected, it gets more useful. I would prefer moving away from
> > the devm_* registration, and unregister the pwm_chip in .remove()
> > manually, before destroying the mutex.
> 
> In that case I'd prefer a devm_mutex_init()?!

Maybe that would be useful :-) Let's see if I can drop the mutex though.

-- 
Regards,

Laurent Pinchart

Powered by blists - more mailing lists