On 25/04/2022 19:16, Uwe Kleine-König wrote: >>> + /* >>> + * We currently avoid using 64bit arithmetic by using the >>> + * fact that anything faster than 1Hz is easily representable >>> + * by 32bits. >>> + */ >>> + if (state->period > NSEC_PER_SEC) >> >> Isn't this changing a bit logic in case of setting wrong period and >> inverted signal? >> >> Before, the config would return early and do nothing. Now, you disable >> the PWM, check the condition here and exit with PWM disabled. I think >> this should reverse the tasks done, or the check should be done early. > > The intension here is to just push the legacy implementation of .apply() > (i.e. pwm_apply_legacy()) into the driver, to be able to get rid of the > legacy handing in the core. I think the problem you point out is real, > but it is not introduced by my change which doesn't change behaviour. > > The problem is just that it's not possible to "see" that period is > invalid at the time the polarity is changed and so it exists since at > least 5ec803edcb703fe379836f13560b79dfac79b01d, my patch just made it > more obvious. > > So yes, there is potential to further improve the driver, but that's out > of scope for this 1:1 conversion patch. Sounds reasonable, thanks for explanation. Everything else was looking good, so: Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> Best regards, Krzysztof
