linux-kernel - Re: [PATCH v11 04/10] pwm: max7360: Add MAX7360 PWM support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <j6zavgfpiq7s7cnfkghn2y6fv4h4ziqtpyp7igwmovqlyuasoq@hozlyjcpsxth>
Date: Fri, 11 Jul 2025 16:50:41 +0200
From: Uwe Kleine-König <ukleinek@...nel.org>
To: Mathieu Dubois-Briand <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>, Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH v11 04/10] pwm: max7360: Add MAX7360 PWM support

Hello Mathieu,

On Fri, Jul 11, 2025 at 11:29:44AM +0200, Mathieu Dubois-Briand wrote:
> diff --git a/drivers/pwm/pwm-max7360.c b/drivers/pwm/pwm-max7360.c
> new file mode 100644
> index 000000000000..0eb83135f658
> --- /dev/null
> +++ b/drivers/pwm/pwm-max7360.c
> @@ -0,0 +1,193 @@
> +// 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>
> + *

A link to the data sheet here would be awesome. I found it at

https://www.analog.com/media/en/technical-documentation/data-sheets/MAX7360.pdf

> [...]
> +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.
> +	 */
> +	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;

I read through the data sheet and I think the right formula for
duty_steps is:

	if (wf->duty_length_ns >= MAX7360_PWM_PERIOD_NS) {
		duty_steps = 255;
	} else {
		duty_steps = (u32)wf->duty_length_ns * 256 / MAX7360_PWM_PERIOD_NS;
		if (duty_steps == 255)
			duty_steps = 254;
	}

(Using magic constants here, but in the end these should be cpp symbols
of course.)

> +	wfhw->duty_steps = min(MAX7360_PWM_MAX_RES, duty_steps);
> +	wfhw->enabled = !!wf->period_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;
> +
> +	if (wfhw->enabled)
> +		wf->duty_length_ns = DIV_ROUND_UP(wfhw->duty_steps * MAX7360_PWM_PERIOD_NS,
> +						  MAX7360_PWM_MAX_RES);
> +	else
> +		wf->duty_length_ns = 0;

The matching code here is:

	if (wfhw->duty_steps == 255)
		wf->duty_length_ns = MAX7360_PWM_PERIOD_NS;
	else
		wf->duty_length_ns = DIV_ROUND_UP(wfhw->duty_steps * MAX7360_PWM_PERIOD_NS, 256)

This is arguably a strange design, but f_OSC = 128 kHz and the fixed
period being 2 ms is a strong indication that the divider is 256 and not
255. If you don't agree to the manual (e.g. because you measured the
output and saw your formula to be true), please add a code comment about
that.

When you have measureing equipment at hand it would be great if you
could verify that the right fromhw implementation isn't:

	wf->duty_length_ns = DIV_ROUND_UP(wfhw->duty_steps * MAX7360_PWM_PERIOD_NS, 256)

even for wfhw->duty_steps == 255. (Which would mean that the PWM cannot
provide a 100% duty cycle.)

> +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");

Please start error messages with a capital letter.

Best regards
Uwe

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