Re: [PATCH v6 2/2] pwm: Add Nuvoton MA35D1 PWM controller support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The driver relies on the bootloader-configured CLKPSC (prescaler) register but does not cache its value, which will cause incorrect period calculations after a system suspend/resume cycle.
--

commit b9ac3c051c5908fe946a0ef1c7b809030aa32933
Author: Chi-Wen Weng <cwweng@nuvoton.com>

pwm: Add Nuvoton MA35D1 PWM controller support

This commit adds a generic PWM framework driver for the Nuvoton MA35D1 PWM
controller. The initial driver supports 6 channels in up-counting,
auto-reload, independent output, and period loading modes.

> diff --git a/drivers/pwm/pwm-ma35d1.c b/drivers/pwm/pwm-ma35d1.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/pwm/pwm-ma35d1.c
[ ... ]
> +static u32 nuvoton_pwm_get_prescale(struct nuvoton_pwm *nvtpwm,
> +				    unsigned int ch)
> +{
> +	return nuvoton_pwm_readl(nvtpwm, MA35D1_REG_PWM_CLKPSC(ch)) &
> +	       MA35D1_PWM_CLKPSC_MASK;
> +}

[Severity: Medium]
Does reading the hardware register directly here make the driver vulnerable 
to hardware state loss during system suspend? 

If the controller loses power during suspend, the clock prescaler register 
might reset to its default value. When the system resumes and a consumer 
subsequently calls into the apply callback, this function will return the 
reset value instead of the bootloader-configured value. This could result 
in drastically altered PWM output frequencies.

Would it be safer to cache the prescaler value during probe?

[ ... ]
> +static struct platform_driver nuvoton_pwm_driver = {
> +	.probe = nuvoton_pwm_probe,
> +	.driver = {
> +		.name = "nuvoton-pwm",
> +		.of_match_table = nuvoton_pwm_of_match,
> +	},
> +};

[Severity: Medium]
Should this driver include PM callbacks to handle saving and restoring the
hardware state?

Currently, there are no suspend/resume hooks provided. If the platform 
powers down the PWM domain during system suspend, the controller state 
will be lost upon resume and might not be appropriately reconfigured before
the next state application.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914110739.203479-1-cwweng.linux@gmail.com?part=2