| Message ID | 20240405003025.739603-1-tgamblin@baylibre.com |
|---|---|
| Headers | show |
| Series | pwm: add support for duty_offset | expand |
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
On 2024-04-05 08:23, Uwe Kleine-König wrote: > Hello Trevor, > > In general I really like your effort to generalize the pwm framework. > Thanks a lot! Thanks! I'm glad that it (mostly) fits. > > 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_*(). Okay, I'll drop this patch. > >> 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. Alright, I'll make a change here. > >> 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. Will do. > > 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. Can you clarify what you mean here - is the intent to put basic handling of duty_offset (even if that means simply setting it to 0) in each driver? > >> 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 >
Hello Trevor, On Fri, Apr 05, 2024 at 10:19:20AM -0400, Trevor Gamblin wrote: > On 2024-04-05 08:23, Uwe Kleine-König wrote: > > 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. > Can you clarify what you mean here - is the intent to put basic handling of > duty_offset (even if that means simply setting it to 0) in each driver? Well, it's a bit more complicated than setting it to 0. It involves translating a setting with inverted polarity to one using .duty_offset and make all drivers implement that accordingly. For drivers that support both polarities the logic in .apply should be: if (.duty_offset >= .period - .duty_cycle) ... set inverted polarity else ... set normal polarity I'm usure how to do the transformation in reviewable chunks. Maybe the easiest option is a new .apply callback that honors .duty_offset? Best regards Uwe
On Fri, Apr 5, 2024 at 1:23 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Thu, Apr 04, 2024 at 08:30:22PM -0400, Trevor Gamblin wrote: ... > > 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. But not all chips can fully support this feature. I envisioned this flag as something consumer drivers would check when they require a chip capable of providing a phase offset between two PWM output channels. This way, the consumer driver could fail to probe if the PWM chip is not up to the task. For example the consumer driver might require two coordinated signals like this: _ _ PWM0 | |_________________| |_________________ ___ ___ PWM1 _____| |_______________| |__________ PWM0: duty_offset = 0, duty_cycle = 1, period = 10 PWM1: duty_offset = 2, duty_cycle = 2, period = 10 Do we need to do additional work to support cases like this? Or should we just let it fail silently and let it generate incorrect signals if someone attempts to use an unsupported hardware configuration?
Hello David, On Fri, Apr 05, 2024 at 12:03:56PM -0500, David Lechner wrote: > On Fri, Apr 5, 2024 at 1:23 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > On Thu, Apr 04, 2024 at 08:30:22PM -0400, Trevor Gamblin wrote: > > ... > > > > 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. > > But not all chips can fully support this feature. I envisioned this > flag as something consumer drivers would check when they require a > chip capable of providing a phase offset between two PWM output > channels. This way, the consumer driver could fail to probe if the PWM > chip is not up to the task. > > For example the consumer driver might require two coordinated signals like this: > _ _ > PWM0 | |_________________| |_________________ > ___ ___ > PWM1 _____| |_______________| |__________ > > PWM0: duty_offset = 0, duty_cycle = 1, period = 10 > PWM1: duty_offset = 2, duty_cycle = 2, period = 10 > > Do we need to do additional work to support cases like this? Or should > we just let it fail silently and let it generate incorrect signals if > someone attempts to use an unsupported hardware configuration? My vision here is that you can do the following: state.duty_offset = 0; state.duty_cycle = 1; state.period = 10; ret = pwm_round_state(pwm, &state); if (!is_good_enough(state)) goto err; This way pwm_apply_* could just continue to work as it does now (i.e. implement the biggest period not bigger than requested. For that implement the biggest duty_offset not bigger than requested and for these values of period and duty_offset implement the biggest duty_cycle not bigger than requested.) This has the advantage that the lowlevel driver doesn't need to judge if the setting it implements is good enough. To get there, quite some work has to be invested. Best regards Uwe