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

[Chi-Wen Weng <cwweng.linux@gmail.com>]

sashiko-bot@kernel.org 於 2026/9/14 下午 07:17 寫道:
> 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.
> 

Hi,

Thanks for the review.

On MA35D1, the EPWM register context is retained across system suspend.
This includes CLKPSC as well as the other EPWM configuration registers
used by the driver.

Therefore, after resume, nuvoton_pwm_get_prescale() still reads the
same CLKPSC value that was present before suspend. The firmware-provided
prescaler value is not lost, so caching CLKPSC during probe would be
redundant.

For the same reason, suspend/resume callbacks are not required to save
and restore the EPWM register state on MA35D1.

So I don't think either of these issues requires a driver change.

Best regards,
Chi-Wen