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

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

Hi Uwe,

Thank you for your reply.


On 2024/10/18 下午 04:22, Uwe Kleine-König wrote:
> Hello Sean,
>
> On Fri, Oct 18, 2024 at 08:46:28AM +0100, Sean Young wrote:
>>> +static int nuvoton_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>>> +			     const struct pwm_state *state)
>>> +{
>>> +	struct nuvoton_pwm *nvtpwm;
>>> +	unsigned int ch = pwm->hwpwm;
>>> +
>>> +	nvtpwm = to_nuvoton_pwm(chip);
>>> +	if (state->enabled) {
>>> +		u64 duty_cycles, period_cycles;
>>> +
>>> +		/* Calculate the duty and period cycles */
>>> +		duty_cycles = mul_u64_u64_div_u64(nvtpwm->clkrate,
>>> +						  state->duty_cycle, NSEC_PER_SEC);
>>> +		if (duty_cycles > 0xFFFF)
>>> +			duty_cycles = 0xFFFF;
>>> +
>>> +		period_cycles = mul_u64_u64_div_u64(nvtpwm->clkrate,
>>> +						    state->period, NSEC_PER_SEC);
>>> +		if (period_cycles > 0xFFFF)
>>> +			period_cycles = 0xFFFF;
>> If a period is not supported, return -EINVAL - maybe even do a dev_err().
>> Same for the duty cycle above. It might make sense to calculate the period
>> first, so that the error is more likely to be about the period than the
>> duty cycle.
> That's a wrong advice. Drivers are supposed to implement the highest
> period possible that is not bigger than the requested one. So clamping
> the value to 0xFFFF looks right.
Thanks for your explanation.
> However I wonder what happens in hardware if period_cycles == 0. If that
> disables the hardware that is something to catch and return an error
> for.
If period_cycles = 0, the waveform output will be always low.
>> Then again I don't know if all the drivers do this, but at least some of
>> them do.
> Yeah, and I hesitate to align them because their behaviour might be
> relied on. But for new drivers the above rule applies.
>
> (And with the new waveform stuff, consumers can rely on the rounding
> rule and even query the resulting waveform before calling the equivalent
> of pwm_apply_might_sleep().
>
>
>>> +	chip->ops = &nuvoton_pwm_ops;
>> I think you can add chip->atomic = true; here
> ack.
>
> Best regards
> Uwe

Thanks.

Chi-Wen Weng