Re: [PATCH v4 2/2] pwm: add Axiado AX3000 PWM driver
flat view
From: sashiko-bot@kernel.org
Date: 2026-09-08 08:38:13
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
+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;
[ ... ]
quoted hunk ↗ jump to hunk
+ 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