Re: [PATCH v4 03/10] pwm: max7360: Add MAX7360 PWM support
From: Uwe Kleine-König <ukleinek@kernel.org>
Date: 2025-03-13 21:03:30
Also in:
linux-devicetree, linux-gpio, linux-pwm, lkml
Hello Mathieu, On Fri, Feb 14, 2025 at 12:49:53PM +0100, mathieu.dubois-briand@bootlin.com wrote:
quoted hunk ↗ jump to hunk
diff --git a/drivers/pwm/pwm-max7360.c b/drivers/pwm/pwm-max7360.c new file mode 100644 index 000000000000..f1257c20add2 --- /dev/null +++ b/drivers/pwm/pwm-max7360.c@@ -0,0 +1,213 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright 2024 Bootlin + * + * Author: Kamel BOUHARA <kamel.bouhara@bootlin.com> + * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> + * + * Limitations: + * - Only supports normal polarity. + * - The period is fixed to 2 ms. + * - Only the duty cycle can be changed, new values are applied at the beginning + * of the next cycle. + * - When disabled, the output is put in Hi-Z. + */ +#include <linux/err.h> +#include <linux/math.h> +#include <linux/mfd/max7360.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/regmap.h> + +#define MAX7360_NUM_PWMS 8 +#define MAX7360_PWM_MAX_RES 255 +#define MAX7360_PWM_PERIOD_NS 2000000 /* 500 Hz */ +#define MAX7360_PWM_COMMON_PWN BIT(5) +#define MAX7360_PWM_CTRL_ENABLE(n) BIT(n) +#define MAX7360_PWM_PORT(n) BIT(n) + +struct max7360_pwm { + struct device *parent; + struct regmap *regmap; +}; + +struct max7360_pwm_waveform { + u8 duty_steps; +}; + +static inline struct max7360_pwm *max7360_pwm_from_chip(struct pwm_chip *chip) +{ + return pwmchip_get_drvdata(chip); +} + +static int max7360_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct max7360_pwm *max7360_pwm; + int ret; + + max7360_pwm = max7360_pwm_from_chip(chip); + ret = max7360_port_pin_request(max7360_pwm->parent, pwm->hwpwm, true); + if (ret) { + dev_warn(&chip->dev, "failed to request pwm-%d\n", pwm->hwpwm);
Please drop this warning, just returning ret here is fine. (The rule of thumb is: Emit runtime messages only in probe, not during usage.)
+ return ret;
+ }
+
+ ret = regmap_write_bits(max7360_pwm->regmap,
+ MAX7360_REG_PWMCFG(pwm->hwpwm),
+ MAX7360_PWM_COMMON_PWN,
+ 0);
+ if (ret)
+ return ret;
+
+ return regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_PORTS,
+ MAX7360_PWM_PORT(pwm->hwpwm),
+ MAX7360_PWM_PORT(pwm->hwpwm));
+}
+
+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?
+}
+
[...]
+
+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.
+ ret = regmap_write_bits(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL,
+ MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm), val);
+
+ if (!ret && wfhw->duty_steps != 0) {
+ ret = regmap_write(max7360_pwm->regmap, MAX7360_REG_PWM(pwm->hwpwm),
+ wfhw->duty_steps);
+ }
+
+ return ret;
+}
+
+static int max7360_pwm_read_waveform(struct pwm_chip *chip,
+ struct pwm_device *pwm,
+ void *_wfhw)
+{
+ struct max7360_pwm_waveform *wfhw = _wfhw;
+ struct max7360_pwm *max7360_pwm;
+ unsigned int val;
+ int ret;
+
+ max7360_pwm = max7360_pwm_from_chip(chip);
+
+ ret = regmap_read(max7360_pwm->regmap, MAX7360_REG_GPIOCTRL, &val);
+ if (ret)
+ return ret;
+
+ if (val & MAX7360_PWM_CTRL_ENABLE(pwm->hwpwm)) {
+ ret = regmap_read(max7360_pwm->regmap, MAX7360_REG_PWM(pwm->hwpwm),
+ &val);
+ val = wfhw->duty_steps;wfhw->duty_steps = val;
+ } else {
+ wfhw->duty_steps = 0;
+ }
+
+ return ret;
+}Best regards Uwe
Attachments
- signature.asc [application/pgp-signature] 488 bytes