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

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@bootlin.com wrote:

quoted

diff --git a/drivers/pwm/pwm-max7360.c b/drivers/pwm/pwm-max7360.c
new file mode 100644
index 000000000000..f1257c20add2

...

quoted

+
+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.

quoted

+}
+
[...]
+
+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