Re: [PATCH v10 0/3] Change PWM-controlled LED pin active mode and algorithm

[Uwe Kleine-König]

Hello,

On Sun, Jan 19, 2025 at 03:03:16PM +0800, Nylon Chen wrote:
> I ran some basic tests by changing the period and duty cycle in both
> decreasing and increasing sequences (see the script below).

What is clk_get_rate(ddata->clk) for you?

> # Backward testing for period (decreasing)
> echo "Testing period backward..."
> 
> seq 15000 -1 5000 | while read p; do
> 
>    echo $p > /sys/class/pwm/pwmchip1/pwm0/period
> 
>    echo "Testing period: $p"
> 
> done
> 
> 
> # Forward testing for period (increasing)
> echo "Testing period forward..."
> 
> seq 5000 1 15000 | while read p; do
> 
>    echo $p > /sys/class/pwm/pwmchip1/pwm0/period
> 
>    echo "Testing period: $p"
> 
> done
> 
> 
> # Backward testing for duty cycle (decreasing)
> echo "Testing duty cycle backward..."
> 
> for duty in $(seq 10000 -1 0); do
> 
>    echo $duty > /sys/class/pwm/pwmchip1/pwm0/duty_cycle
> 
>    echo "Testing duty cycle: $duty"
> 
> done
> 
> 
> # Forward testing for duty cycle (increasing)
> 
> echo "Testing duty cycle forward..."
> 
> for duty in $(seq 0 1 10000); do
> 
>    echo $duty > /sys/class/pwm/pwmchip1/pwm0/duty_cycle
> 
>    echo "Testing duty cycle: $duty"
> 
> done
> 
> 
> 
> In these particular tests, I didn’t see any functional difference or
> unexpected behavior whether I used DIV64_U64_ROUND_CLOSEST() or
> DIV64_U64_ROUND_UP.
> Of course, there’s a chance my tests haven’t covered every scenario,
> so there could still be edge cases I missed.

Just to be sure: You have PWM_DEBUG enabled?

> From what I understand, your main concern is to ensure we never end up
> with a duty cycle that’s smaller than what the user requested, which a
> round-up approach would help guarantee. If you still recommend making
> that change to achieve the desired behavior, I’m happy to update the
> code accordingly(CLOSEST->UP).

No, .apply should round down and so to ensure that

	pwm_get_state(mypwm, &state);
	pwm_apply(mypwm, &state);

doesn't modify the hardware setting, .get_state has to round up.

Best regards
Uwe

Attachment: signature.asc
Description: PGP signature