Re: [PATCH v4 03/10] pwm: max7360: Add MAX7360 PWM support

[Mathieu Dubois-Briand]

On Thu Mar 13, 2025 at 10:03 PM CET, Uwe Kleine-König wrote:
> Hello Mathieu,
>
> On Fri, Feb 14, 2025 at 12:49:53PM +0100, mathieu.dubois-briand@xxxxxxxxxxx wrote:
> > diff --git a/drivers/pwm/pwm-max7360.c b/drivers/pwm/pwm-max7360.c
> > new file mode 100644
> > index 000000000000..f1257c20add2
> ...
> > +
> > +static void max7360_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct max7360_pwm *max7360_pwm;
> > +
> > +	max7360_pwm = max7360_pwm_from_chip(chip);
> > +	max7360_port_pin_request(max7360_pwm->parent, pwm->hwpwm, false);
>
> Would be nice if pinmuxing would be abstracted as a pinctrl driver. Not
> sure how much effort that is. Maybe Linus has an idea?
>

Yes, I got some similar comments previously and I've been working on it:
the next version will gain a pinctrl driver.

> > +}
> > +
> > [...]
> > +
> > +static int max7360_pwm_write_waveform(struct pwm_chip *chip,
> > +				      struct pwm_device *pwm,
> > +				      const void *_wfhw)
> > +{
> > +	const struct max7360_pwm_waveform *wfhw = _wfhw;
> > +	struct max7360_pwm *max7360_pwm;
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	max7360_pwm = max7360_pwm_from_chip(chip);
> > +
> > +	val = (wfhw->duty_steps == 0) ? 0 : MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm);
>
> Does not setting MAX7360_PWM_CTRL_ENABLE result in the pin going to
> Hi-Z? If yes: That's wrong. You're only supposed to do that if
> period_length_ns = 0 was requested. If no: This needs a comment why
> duty_steps = 0 is special here.
>

Ok, I confirm this does set the pin in Hi-Z, I'm fixing it.

...

>
> Best regards
> Uwe

Thanks for your review.

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com