linux-kernel - Re: [PATCH v3 3/7] pwm: max7360: Add MAX7360 PWM support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <v4bf6bharih6zgz52ya5twfyf47wh3fu56ovic5gjxak2jhufy@q3eudujjwrhm>
Date: Fri, 17 Jan 2025 15:40:16 +0100
From: Uwe Kleine-König <ukleinek@...nel.org>
To: Mathieu Dubois-Briand <mathieu.dubois-briand@...tlin.com>
Cc: Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Kamel Bouhara <kamel.bouhara@...tlin.com>, Linus Walleij <linus.walleij@...aro.org>, 
	Bartosz Golaszewski <brgl@...ev.pl>, Dmitry Torokhov <dmitry.torokhov@...il.com>, 
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org, 
	linux-input@...r.kernel.org, linux-pwm@...r.kernel.org, 
	Grégory Clement <gregory.clement@...tlin.com>, Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v3 3/7] pwm: max7360: Add MAX7360 PWM support

On Fri, Jan 17, 2025 at 03:11:29PM +0100, Mathieu Dubois-Briand wrote:
> On Fri Jan 17, 2025 at 10:33 AM CET, Uwe Kleine-König wrote:
> > Hello Mathieu,
> >
> > On Mon, Jan 13, 2025 at 01:42:27PM +0100, mathieu.dubois-briand@...tlin.com wrote:
> > > From: Kamel Bouhara <kamel.bouhara@...tlin.com>
> ...
> > > +static int max7360_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > +			     const struct pwm_state *state)
> > > +{
> > > +	struct max7360_pwm *max7360_pwm;
> > > +	u64 duty_steps;
> > > +	int ret;
> > > +
> > > +	if (state->polarity != PWM_POLARITY_NORMAL)
> > > +		return -EINVAL;
> > > +
> > > +	if (state->period != MAX7360_PWM_PERIOD_NS) {
> > > +		dev_warn(&chip->dev,
> > > +			 "unsupported pwm period: %llu, should be %u\n",
> > > +			 state->period, MAX7360_PWM_PERIOD_NS);
> > > +		return -EINVAL;
> >
> > Please don't emit error messages in .apply(). Also a driver is supposed
> > to round down .period, so any value >= MAX7360_PWM_PERIOD_NS should be
> > accepted.
> >
> > Also note that you might want to implement the waveform callbacks
> > instead of .apply() and .get_state() for the more modern abstraction
> > (with slightly different rounding rules).
> >
> 
> Sure, I just switched to the waveform callbacks, it was quite
> straightforward.

sounds great. Note that the detail in rounding that is different for
waveforms is that a value that cannot be round down to a valid value
(because it's too small) is round up. This is a bit ugly in the drivers
but simplifies usage considerably. So you never return -EINVAL because
the values don't fit.

> Thanks for the reproduce steps: I saw the bug and fixed it. Also
> MAX7360_PWM_MAX_RES had to be set to 255 and not 256...

A good test (for a driver doing .apply/.get_state) is a sequence of
increasing settings. So something like:

	for p in range(1000, 10000):
	    pwm_apply(period=p, duty_cycle=0, ...)

and also do the same for duty_cycle and also try decreasing series.

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)