Re: [PATCH v10 04/11] pwm: max7360: Add MAX7360 PWM support

["Mathieu Dubois-Briand" <mathieu.dubois-briand@bootlin.com>]

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