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
