Messages in this thread | | | Date | Fri, 5 Apr 2024 08:23:25 +0200 | From | Uwe Kleine-König <> | Subject | Re: [RFC PATCH 0/3] pwm: add support for duty_offset |
| |
Hello Trevor,
In general I really like your effort to generalize the pwm framework. Thanks a lot!
On Thu, Apr 04, 2024 at 08:30:22PM -0400, Trevor Gamblin wrote: > This series extends the PWM subsystem to support the duty_offset feature > found on some PWM devices. It includes a patch to enable this feature > for the axi-pwmgen driver, which can also serve as an example of how to > implement it for other devices. It also contains a patch adding a new > pwm_config_full() function mirroring the behavior of pwm_config() but
Please don't. pwm_config() is a function I want to get rid of in the long term. Consumers that want to make use of it should use pwm_apply_*().
> with duty_offset included, to help maintain compatibility for drivers > that don't support the feature. > > The series was tested on actual hardware using a Zedboard. An > oscilloscope was used to validate that the generated PWM signals matched > the requested ones. The libpwm [1] tool was also used for testing the > char device functionality. > > The series is marked RFC as there are some outstanding questions about > implementation: > > 1. In drivers/pwm/core.c, __pwm_apply() was modified to check that the > sum of state->duty_offset + state->duty_cycle does not exceed > state->period, but in the character device section these values are > being checked separately. Is this intentional? What is the intended > behavior?
state->duty_offset + state->duty_cycle doesn't necessarily need to be less or equal to state->period. Consider this waveform, where ^ marks the start of a period.
___ _________ __... \_____/ \_____/ ^ ^
This one has duty_offset = 9 and duty_cycle = 10 while period = 16 < 10 + 9.
> 2. Should __pwm_apply() explicitly disallow PWM_POLARITY_INVERSED and > duty_offset together?
While there is no technical need for that, a configuration with both PWM_POLARITY_INVERSED and duty_offset > 0 is irritating. So I'd say yes, it should be disallowed. When I created the cdev API I even considered dropping .polarity for lowlevel drivers and convert them all to .duty_offset.
Having said that I don't like the addition of .supports_offset to struct pwm_chip, which only signals a new incomplete evolution of the pwm framework. Better adapt all drivers and then assume all of them support it.
> 3. Are there other places that would need duty_offset handling which > have been missed?
I'm happy you adapted the tracing stuff. I didn't look closely, but I don't think something important was missed.
> Note that in addition to the other patches in this series, the changes > to the axi-pwmgen driver rely on [2] and [3], which haven't been picked > up yet.
Best regards Uwe
-- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [unhandled content-type:application/pgp-signature] | |