From: Kamel Bouhara <kamel.bouhara@bootlin.com>
Add driver for Maxim Integrated MAX7360 PWM controller, supporting up to
8 independent PWM outputs.
Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
Co-developed-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
---
drivers/pwm/Kconfig | 10 +++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-max7360.c | 180 ++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 191 insertions(+)
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 4731d5b90d7e..0b22141cbf85 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -755,4 +755,14 @@ config PWM_XILINX
To compile this driver as a module, choose M here: the module
will be called pwm-xilinx.
+config PWM_MAX7360
+ tristate "MAX7360 PWMs"
+ depends on MFD_MAX7360
+ help
+ PWM driver for Maxim Integrated MAX7360 multifunction device, with
+ support for up to 8 PWM outputs.
+
+ To compile this driver as a module, choose M here: the module
+ will be called pwm-max7360.
+
endif
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 539e0def3f82..9c7701d8070b 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
obj-$(CONFIG_PWM_LPSS) += pwm-lpss.o
obj-$(CONFIG_PWM_LPSS_PCI) += pwm-lpss-pci.o
obj-$(CONFIG_PWM_LPSS_PLATFORM) += pwm-lpss-platform.o
+obj-$(CONFIG_PWM_MAX7360) += pwm-max7360.o
obj-$(CONFIG_PWM_MESON) += pwm-meson.o
obj-$(CONFIG_PWM_MEDIATEK) += pwm-mediatek.o
obj-$(CONFIG_PWM_MICROCHIP_CORE) += pwm-microchip-core.o
diff --git a/drivers/pwm/pwm-max7360.c b/drivers/pwm/pwm-max7360.c
new file mode 100644
index 000000000000..a0c82c63d2ac
--- /dev/null
+++ b/drivers/pwm/pwm-max7360.c
@@ -0,0 +1,180 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright 2025 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/bits.h>
+#include <linux/dev_printk.h>
+#include <linux/err.h>
+#include <linux/math64.h>
+#include <linux/mfd/max7360.h>
+#include <linux/minmax.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/time.h>
+#include <linux/types.h>
+
+#define MAX7360_NUM_PWMS 8
+#define MAX7360_PWM_MAX_RES 255
+#define MAX7360_PWM_PERIOD_NS (2 * NSEC_PER_MSEC)
+
+struct max7360_pwm_waveform {
+ u8 duty_steps;
+ bool enabled;
+};
+
+static int max7360_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+ struct regmap *regmap = pwmchip_get_drvdata(chip);
+
+ return regmap_write_bits(regmap, MAX7360_REG_PWMCFG(pwm->hwpwm),
+ MAX7360_PORT_CFG_COMMON_PWM, 0);
+}
+
+static int max7360_pwm_round_waveform_tohw(struct pwm_chip *chip,
+ struct pwm_device *pwm,
+ const struct pwm_waveform *wf,
+ void *_wfhw)
+{
+ struct max7360_pwm_waveform *wfhw = _wfhw;
+ u64 duty_steps;
+
+ /*
+ * Ignore user provided values for period_length_ns and duty_offset_ns:
+ * we only support fixed period of MAX7360_PWM_PERIOD_NS and offset of 0.
+ */
+ duty_steps = mul_u64_u64_div_u64(wf->duty_length_ns, MAX7360_PWM_MAX_RES,
+ MAX7360_PWM_PERIOD_NS);
+
+ wfhw->duty_steps = min(MAX7360_PWM_MAX_RES, duty_steps);
+ wfhw->enabled = !!wf->duty_length_ns;
+
+ return 0;
+}
+
+static int max7360_pwm_round_waveform_fromhw(struct pwm_chip *chip, struct pwm_device *pwm,
+ const void *_wfhw, struct pwm_waveform *wf)
+{
+ const struct max7360_pwm_waveform *wfhw = _wfhw;
+
+ wf->period_length_ns = wfhw->enabled ? MAX7360_PWM_PERIOD_NS : 0;
+ wf->duty_offset_ns = 0;
+ wf->duty_length_ns = DIV_ROUND_UP(wfhw->duty_steps * MAX7360_PWM_PERIOD_NS,
+ MAX7360_PWM_MAX_RES);
+
+ return 0;
+}
+
+static int max7360_pwm_write_waveform(struct pwm_chip *chip,
+ struct pwm_device *pwm,
+ const void *_wfhw)
+{
+ struct regmap *regmap = pwmchip_get_drvdata(chip);
+ const struct max7360_pwm_waveform *wfhw = _wfhw;
+ unsigned int val;
+ int ret;
+
+ if (wfhw->enabled) {
+ ret = regmap_write(regmap, MAX7360_REG_PWM(pwm->hwpwm), wfhw->duty_steps);
+ if (ret)
+ return ret;
+ }
+
+ val = wfhw->enabled ? BIT(pwm->hwpwm) : 0;
+ return regmap_write_bits(regmap, MAX7360_REG_GPIOCTRL, BIT(pwm->hwpwm), val);
+}
+
+static int max7360_pwm_read_waveform(struct pwm_chip *chip,
+ struct pwm_device *pwm,
+ void *_wfhw)
+{
+ struct regmap *regmap = pwmchip_get_drvdata(chip);
+ struct max7360_pwm_waveform *wfhw = _wfhw;
+ unsigned int val;
+ int ret;
+
+ ret = regmap_read(regmap, MAX7360_REG_GPIOCTRL, &val);
+ if (ret)
+ return ret;
+
+ if (val & BIT(pwm->hwpwm)) {
+ wfhw->enabled = true;
+ ret = regmap_read(regmap, MAX7360_REG_PWM(pwm->hwpwm), &val);
+ if (ret)
+ return ret;
+
+ wfhw->duty_steps = val;
+ } else {
+ wfhw->enabled = false;
+ wfhw->duty_steps = 0;
+ }
+
+ return 0;
+}
+
+static const struct pwm_ops max7360_pwm_ops = {
+ .request = max7360_pwm_request,
+ .round_waveform_tohw = max7360_pwm_round_waveform_tohw,
+ .round_waveform_fromhw = max7360_pwm_round_waveform_fromhw,
+ .read_waveform = max7360_pwm_read_waveform,
+ .write_waveform = max7360_pwm_write_waveform,
+};
+
+static int max7360_pwm_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct pwm_chip *chip;
+ struct regmap *regmap;
+ int ret;
+
+ regmap = dev_get_regmap(dev->parent, NULL);
+ if (!regmap)
+ return dev_err_probe(dev, -ENODEV, "could not get parent regmap\n");
+
+ /*
+ * This MFD sub-device does not have any associated device tree node:
+ * properties are stored in the device node of the parent (MFD) device
+ * and this same node is used in phandles of client devices.
+ * Reuse this device tree node here, as otherwise the PWM subsystem
+ * would be confused by this topology.
+ */
+ device_set_of_node_from_dev(dev, dev->parent);
+
+ chip = devm_pwmchip_alloc(dev, MAX7360_NUM_PWMS, 0);
+ if (IS_ERR(chip))
+ return PTR_ERR(chip);
+ chip->ops = &max7360_pwm_ops;
+
+ pwmchip_set_drvdata(chip, regmap);
+
+ ret = devm_pwmchip_add(dev, chip);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to add PWM chip\n");
+
+ return 0;
+}
+
+static struct platform_driver max7360_pwm_driver = {
+ .driver = {
+ .name = "max7360-pwm",
+ },
+ .probe = max7360_pwm_probe,
+};
+module_platform_driver(max7360_pwm_driver);
+
+MODULE_DESCRIPTION("MAX7360 PWM driver");
+MODULE_AUTHOR("Kamel BOUHARA <kamel.bouhara@bootlin.com>");
+MODULE_AUTHOR("Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>");
+MODULE_LICENSE("GPL");
--
2.39.5On Fri, May 30, 2025 at 12:00:12PM +0200, mathieu.dubois-briand@bootlin.com wrote:
> From: Kamel Bouhara <kamel.bouhara@bootlin.com>
>
> Add driver for Maxim Integrated MAX7360 PWM controller, supporting up to
> 8 independent PWM outputs.
>
> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> Co-developed-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> ---
> drivers/pwm/Kconfig | 10 +++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-max7360.c | 180 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 191 insertions(+)
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 4731d5b90d7e..0b22141cbf85 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -755,4 +755,14 @@ config PWM_XILINX
> To compile this driver as a module, choose M here: the module
> will be called pwm-xilinx.
>
> +config PWM_MAX7360
> + tristate "MAX7360 PWMs"
> + depends on MFD_MAX7360
> + help
> + PWM driver for Maxim Integrated MAX7360 multifunction device, with
> + support for up to 8 PWM outputs.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called pwm-max7360.
Alphabetic ordering (by config name) please.
> endif
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 539e0def3f82..9c7701d8070b 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -36,6 +36,7 @@ obj-$(CONFIG_PWM_LPC32XX) += pwm-lpc32xx.o
> obj-$(CONFIG_PWM_LPSS) += pwm-lpss.o
> obj-$(CONFIG_PWM_LPSS_PCI) += pwm-lpss-pci.o
> obj-$(CONFIG_PWM_LPSS_PLATFORM) += pwm-lpss-platform.o
> +obj-$(CONFIG_PWM_MAX7360) += pwm-max7360.o
> obj-$(CONFIG_PWM_MESON) += pwm-meson.o
> obj-$(CONFIG_PWM_MEDIATEK) += pwm-mediatek.o
> obj-$(CONFIG_PWM_MICROCHIP_CORE) += pwm-microchip-core.o
> diff --git a/drivers/pwm/pwm-max7360.c b/drivers/pwm/pwm-max7360.c
> new file mode 100644
> index 000000000000..a0c82c63d2ac
> --- /dev/null
> +++ b/drivers/pwm/pwm-max7360.c
> @@ -0,0 +1,180 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright 2025 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/bits.h>
> +#include <linux/dev_printk.h>
> +#include <linux/err.h>
> +#include <linux/math64.h>
> +#include <linux/mfd/max7360.h>
> +#include <linux/minmax.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/time.h>
> +#include <linux/types.h>
> +
> +#define MAX7360_NUM_PWMS 8
> +#define MAX7360_PWM_MAX_RES 255
> +#define MAX7360_PWM_PERIOD_NS (2 * NSEC_PER_MSEC)
> +
> +struct max7360_pwm_waveform {
> + u8 duty_steps;
> + bool enabled;
> +};
> +
> +static int max7360_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> + struct regmap *regmap = pwmchip_get_drvdata(chip);
> +
> + return regmap_write_bits(regmap, MAX7360_REG_PWMCFG(pwm->hwpwm),
> + MAX7360_PORT_CFG_COMMON_PWM, 0);
> +}
Do you need to undo that in .free()?
> +static int max7360_pwm_round_waveform_tohw(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + const struct pwm_waveform *wf,
> + void *_wfhw)
> +{
> + struct max7360_pwm_waveform *wfhw = _wfhw;
> + u64 duty_steps;
> +
> + /*
> + * Ignore user provided values for period_length_ns and duty_offset_ns:
> + * we only support fixed period of MAX7360_PWM_PERIOD_NS and offset of 0.
> + */
> + duty_steps = mul_u64_u64_div_u64(wf->duty_length_ns, MAX7360_PWM_MAX_RES,
> + MAX7360_PWM_PERIOD_NS);
You don't need the 64 bit division (which is expensive on 32 bit archs)
if you use:
if (wf->duty_length_ns >= MAX7360_PWM_PERIOD_NS)
duty_steps = MAX7360_PWM_MAX_RES;
else
duty_steps = (u32)wf->duty_length_ns * MAX7360_PWM_MAX_RES / MAX7360_PWM_PERIOD_NS;
> + wfhw->duty_steps = min(MAX7360_PWM_MAX_RES, duty_steps);
> + wfhw->enabled = !!wf->duty_length_ns;
How does the output behave if you clean the respective bit in
MAX7360_REG_GPIOCTRL? Unless it emits a constant low signal (and not
e.g. High-Z) you have to do
wfhw->enabled = !!wf->period_length_ns;
here. Please document the behaviour in a paragraph at the top of
the driver. Look at other drivers for the right format. The questions to
answer are:
- How does the driver behave on disable? (Typical is constant low or
High-Z or freezing. Does it stop instantly or does it complete the
currently running period?)
- How does the driver behave on a (non-disabling) reconfiguration? Can
it happen that there are glitches? (Consider for example that
duty_cycle changes from 0.5 ms to 1.5ms while the hardware is just in
the middle of the 2ms period. Does the output go high immediately
then producing two 0.5ms pulses during that period?)
> + return 0;
> +}
> +
> +static int max7360_pwm_round_waveform_fromhw(struct pwm_chip *chip, struct pwm_device *pwm,
> + const void *_wfhw, struct pwm_waveform *wf)
> +{
> + const struct max7360_pwm_waveform *wfhw = _wfhw;
> +
> + wf->period_length_ns = wfhw->enabled ? MAX7360_PWM_PERIOD_NS : 0;
> + wf->duty_offset_ns = 0;
> + wf->duty_length_ns = DIV_ROUND_UP(wfhw->duty_steps * MAX7360_PWM_PERIOD_NS,
> + MAX7360_PWM_MAX_RES);
This should be 0 if !wfhw->enabled to make *wf a valid setting.
A check for that in the core (with CONFIG_PWM_DEBUG) would be great.
> + return 0;
> +}
> +
> +static int max7360_pwm_write_waveform(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + const void *_wfhw)
> +{
> + struct regmap *regmap = pwmchip_get_drvdata(chip);
> + const struct max7360_pwm_waveform *wfhw = _wfhw;
> + unsigned int val;
> + int ret;
> +
> + if (wfhw->enabled) {
> + ret = regmap_write(regmap, MAX7360_REG_PWM(pwm->hwpwm), wfhw->duty_steps);
> + if (ret)
> + return ret;
> + }
> +
> + val = wfhw->enabled ? BIT(pwm->hwpwm) : 0;
> + return regmap_write_bits(regmap, MAX7360_REG_GPIOCTRL, BIT(pwm->hwpwm), val);
> +}
> +
> +static int max7360_pwm_read_waveform(struct pwm_chip *chip,
> + struct pwm_device *pwm,
> + void *_wfhw)
> +{
> + struct regmap *regmap = pwmchip_get_drvdata(chip);
> + struct max7360_pwm_waveform *wfhw = _wfhw;
> + unsigned int val;
> + int ret;
> +
> + ret = regmap_read(regmap, MAX7360_REG_GPIOCTRL, &val);
> + if (ret)
> + return ret;
> +
> + if (val & BIT(pwm->hwpwm)) {
> + wfhw->enabled = true;
> + ret = regmap_read(regmap, MAX7360_REG_PWM(pwm->hwpwm), &val);
> + if (ret)
> + return ret;
> +
> + wfhw->duty_steps = val;
> + } else {
> + wfhw->enabled = false;
> + wfhw->duty_steps = 0;
> + }
> +
> + return 0;
> +}
> +
> +static const struct pwm_ops max7360_pwm_ops = {
> + .request = max7360_pwm_request,
> + .round_waveform_tohw = max7360_pwm_round_waveform_tohw,
> + .round_waveform_fromhw = max7360_pwm_round_waveform_fromhw,
> + .read_waveform = max7360_pwm_read_waveform,
> + .write_waveform = max7360_pwm_write_waveform,
> +};
> +
> +static int max7360_pwm_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct pwm_chip *chip;
> + struct regmap *regmap;
> + int ret;
> +
> + regmap = dev_get_regmap(dev->parent, NULL);
> + if (!regmap)
> + return dev_err_probe(dev, -ENODEV, "could not get parent regmap\n");
> +
> + /*
> + * This MFD sub-device does not have any associated device tree node:
> + * properties are stored in the device node of the parent (MFD) device
> + * and this same node is used in phandles of client devices.
> + * Reuse this device tree node here, as otherwise the PWM subsystem
> + * would be confused by this topology.
> + */
> + device_set_of_node_from_dev(dev, dev->parent);
> +
> + chip = devm_pwmchip_alloc(dev, MAX7360_NUM_PWMS, 0);
> + if (IS_ERR(chip))
> + return PTR_ERR(chip);
> + chip->ops = &max7360_pwm_ops;
> +
> + pwmchip_set_drvdata(chip, regmap);
> +
> + ret = devm_pwmchip_add(dev, chip);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to add PWM chip\n");
> +
> + return 0;
> +}
> +
> +static struct platform_driver max7360_pwm_driver = {
> + .driver = {
> + .name = "max7360-pwm",
Please consider setting
.probe_type = PROBE_PREFER_ASYNCHRONOUS,
here.
> + },
> + .probe = max7360_pwm_probe,
> +};
> +module_platform_driver(max7360_pwm_driver);
> +
> +MODULE_DESCRIPTION("MAX7360 PWM driver");
> +MODULE_AUTHOR("Kamel BOUHARA <kamel.bouhara@bootlin.com>");
> +MODULE_AUTHOR("Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>");
> +MODULE_LICENSE("GPL");
Best regards
Uwe
On Wed Jun 18, 2025 at 8:45 PM CEST, Uwe Kleine-König wrote:
> On Fri, May 30, 2025 at 12:00:12PM +0200, mathieu.dubois-briand@bootlin.com wrote:
>> From: Kamel Bouhara <kamel.bouhara@bootlin.com>
> ...
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-max7360.c
>> @@ -0,0 +1,180 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright 2025 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/bits.h>
>> +#include <linux/dev_printk.h>
>> +#include <linux/err.h>
>> +#include <linux/math64.h>
>> +#include <linux/mfd/max7360.h>
>> +#include <linux/minmax.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +#include <linux/regmap.h>
>> +#include <linux/time.h>
>> +#include <linux/types.h>
>> +
>> +#define MAX7360_NUM_PWMS 8
>> +#define MAX7360_PWM_MAX_RES 255
>> +#define MAX7360_PWM_PERIOD_NS (2 * NSEC_PER_MSEC)
>> +
>> +struct max7360_pwm_waveform {
>> + u8 duty_steps;
>> + bool enabled;
>> +};
>> +
>> +static int max7360_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> + struct regmap *regmap = pwmchip_get_drvdata(chip);
>> +
>> + return regmap_write_bits(regmap, MAX7360_REG_PWMCFG(pwm->hwpwm),
>> + MAX7360_PORT_CFG_COMMON_PWM, 0);
>> +}
>
> Do you need to undo that in .free()?
>
No, this is just to make sure we use the individual PWM configuration
register and not the global one, so there is no need to revert it later.
I'm adding a comment explaining that.
> ...
>
>> + wfhw->duty_steps = min(MAX7360_PWM_MAX_RES, duty_steps);
>> + wfhw->enabled = !!wf->duty_length_ns;
>
> How does the output behave if you clean the respective bit in
> MAX7360_REG_GPIOCTRL? Unless it emits a constant low signal (and not
> e.g. High-Z) you have to do
>
> wfhw->enabled = !!wf->period_length_ns;
>
> here. Please document the behaviour in a paragraph at the top of
> the driver. Look at other drivers for the right format. The questions to
> answer are:
>
> - How does the driver behave on disable? (Typical is constant low or
> High-Z or freezing. Does it stop instantly or does it complete the
> currently running period?)
>
> - How does the driver behave on a (non-disabling) reconfiguration? Can
> it happen that there are glitches? (Consider for example that
> duty_cycle changes from 0.5 ms to 1.5ms while the hardware is just in
> the middle of the 2ms period. Does the output go high immediately
> then producing two 0.5ms pulses during that period?)
>
Ok, I'm fixing the wfhw->enabled value.
About the comment, I believe we already have everything, I'm just adding
that on disable, the output is put in Hi-Z immediately.
>> + return 0;
>> +}
>> +
>> +static int max7360_pwm_round_waveform_fromhw(struct pwm_chip *chip, struct pwm_device *pwm,
>> + const void *_wfhw, struct pwm_waveform *wf)
>> +{
>> + const struct max7360_pwm_waveform *wfhw = _wfhw;
>> +
>> + wf->period_length_ns = wfhw->enabled ? MAX7360_PWM_PERIOD_NS : 0;
>> + wf->duty_offset_ns = 0;
>> + wf->duty_length_ns = DIV_ROUND_UP(wfhw->duty_steps * MAX7360_PWM_PERIOD_NS,
>> + MAX7360_PWM_MAX_RES);
>
> This should be 0 if !wfhw->enabled to make *wf a valid setting.
>
OK.
> A check for that in the core (with CONFIG_PWM_DEBUG) would be great.
>
I can submit a patch, but I'm not sure what that check should be.
So I believe the check would have to be made in __pwm_set_waveform(),
making sure wf_rounded.duty_length_ns is 0 if the PWM is not enabled or
in other words, if wf->period_length_ns is 0. I believe calling
pwm_wf_valid() on wf_rounded would be enough. Maybe I should add that as
a first check in pwm_check_rounding() to cover all call sites.
We already call pwm_check_rounding() code, so me already make sure that
if wf->period_length_ns is 0, wf_rounded->period_length_ns is 0. And
adding pwm_wf_valid(), would make sure that if
wf_rounded->period_length_ns is 0, wf_rounded->duty_length_ns is also 0.
Any opinion?
> ...
>
> Best regards
> Uwe
OK with all other comments.
Thanks for your review!
--
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Fri, May 30, 2025 at 12:00:12PM +0200, mathieu.dubois-briand@bootlin.com wrote: > > Add driver for Maxim Integrated MAX7360 PWM controller, supporting up to > 8 independent PWM outputs. I don't like that OF node propagation, but I have no alternative to propose right now. Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> -- With Best Regards, Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.