Message-ID: <amukbuzpu34jbcjhmzmvfgh6eik5isrwcicfmlqmsyibvhij72@nnmhdj3celnt>
Date: Wed, 18 Jun 2025 20:45:38 +0200
From: Uwe Kleine-König <ukleinek@...nel.org>
To: mathieu.dubois-briand@...tlin.com
Cc: Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Kamel Bouhara <kamel.bouhara@...tlin.com>, Linus Walleij <linus.walleij@...aro.org>, 
	Bartosz Golaszewski <brgl@...ev.pl>, Dmitry Torokhov <dmitry.torokhov@...il.com>, 
	Michael Walle <mwalle@...nel.org>, Mark Brown <broonie@...nel.org>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	Danilo Krummrich <dakr@...nel.org>, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-gpio@...r.kernel.org, linux-input@...r.kernel.org, linux-pwm@...r.kernel.org, 
	andriy.shevchenko@...el.com, Grégory Clement <gregory.clement@...tlin.com>, 
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v10 04/11] pwm: max7360: Add MAX7360 PWM support

On Fri, May 30, 2025 at 12:00:12PM +0200, mathieu.dubois-briand@...tlin.com wrote:
> From: Kamel Bouhara <kamel.bouhara@...tlin.com>
> 
> Add driver for Maxim Integrated MAX7360 PWM controller, supporting up to
> 8 independent PWM outputs.
> 
> Signed-off-by: Kamel Bouhara <kamel.bouhara@...tlin.com>
> Co-developed-by: Mathieu Dubois-Briand <mathieu.dubois-briand@...tlin.com>
> Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@...tlin.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@...tlin.com>
> + * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@...tlin.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@...tlin.com>");
> +MODULE_AUTHOR("Mathieu Dubois-Briand <mathieu.dubois-briand@...tlin.com>");
> +MODULE_LICENSE("GPL");

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists