Re: [PATCH v4 2/2] pwm: add Axiado AX3000 PWM driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] axiado_pwm_read_waveform() misreports the physical HIGH output (from disabled hardware or zero high time) as a logical LOW state (disabled or 0% duty). This causes subsequent legitimate disable/0% duty requests to be silently ignored by the PWM framework.
--

commit 18ec5556f23b44b5a016987a2b7c91a3e21eced6
Author: Petar Stepanovic <pstepanovic@axiado.com>

pwm: add Axiado AX3000 PWM driver

This commit introduces the PWM framework driver for the Axiado AX3000 and
AX3005 SoCs. It implements support for period, duty-cycle, and enable-state
configuration using a constant-low hardware representation for the logically
disabled state.

> diff --git a/drivers/pwm/pwm-axiado.c b/drivers/pwm/pwm-axiado.c
> new file mode 100644
> index 0000000000000..e1d4088096949
> --- /dev/null
> +++ b/drivers/pwm/pwm-axiado.c
[ ... ]
> +static int axiado_pwm_read_waveform(struct pwm_chip *chip,
> +				    struct pwm_device *pwm,
> +				    void *_wfhw)
> +{
> +	struct axiado_pwm_chip *axpwm = pwmchip_get_drvdata(chip);
> +	struct axiado_pwm_waveform *wfhw = _wfhw;
> +	u32 period;
> +	u32 duty;
> +	u32 ctrl;
[ ... ]
> +	ctrl = readl(axpwm->base + AXIADO_PWM_CTRL_REG);
> +	period = readl(axpwm->base + AXIADO_PWM_PERIOD_REG);
> +	duty = readl(axpwm->base + AXIADO_PWM_HIGH_REG);
> +
> +	/* The constant-low encoding doesn't hold the period, so restore it. */
> +	if (period == AXIADO_PWM_PERIOD_CONST_LOW) {
> +		period = axpwm->cached_period;
> +		duty = 0;
> +	} else if (duty > period) {
> +		duty = period;
> +	}
> +
> +	*wfhw = (struct axiado_pwm_waveform) {
> +		.period = period,
> +		.duty = duty,
> +		.enabled = !!(ctrl & AXIADO_PWM_CTRL_ENABLE),
> +	};
> +
> +	return 0;
> +}

[Severity: High]
Does this mistakenly report a physical HIGH state as a logical LOW state?

When the hardware is physically disabled (ctrl & AXIADO_PWM_CTRL_ENABLE is
0) or the high time is zero, the hardware drives the output HIGH according
to the commit message.

However, axiado_pwm_read_waveform() appears to map a disabled controller
to an enabled state of false, and a zero high time to a duty of 0. Shouldn't
a zero high time be translated to a 100% duty cycle to reflect the actual
physical output?

If the system boots with the hardware disabled or with zero high time, this
logic will cache the state as logically disabled or 0% duty. If a consumer
later requests to legitimately disable the PWM (which requires a physical LOW
output via the constant-low encoding), won't the core framework see an
exact state match and short-circuit the update?

This seems like it would silently skip the write operation and leave the
output permanently stuck HIGH when the system expects it to be safely turned
off.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908-axiado-ax3000-pwm-v4-0-19a03671cd5b@axiado.com?part=2