| Message ID | cover.1710670958.git.u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | pwm: Add support for character devices | expand |
Hello, On Sun, Mar 17, 2024 at 11:40:31AM +0100, Uwe Kleine-König wrote: > After the necessary changes to the lowlevel drivers got in for v6.9-rc1 > here come some changes to the core to implement /dev/pwmchipX character > devices. > > In my tests on an ARM STM32MP1 programming a PWM using the character > device is ~4 times faster than just changing duty_cycle via the sysfs > API. It also has the advantage that (similar to pwm_apply_*) the target > state is provided to the kernel with a single call, instead of having to > program the individual settings one after another via sysfs (in the > right order to not cross states not supported by the driver). > > Note the representation of a PWM waveform is different here compared to > the in-kernel representation. A PWM waveform is represented using: > > period > duty_cycle > duty_offset > > A disabled PWM is represented by period = 0. For an inversed wave use: > > duty_offset = duty_cycle > duty_cycle = period - duty_cycle; > > . However there are some difficulties yet that make it hard to provide a > consistent API to userspace and so for now duty_offset isn't (fully) > supported yet. That needs some more consideration and can be added > later. > > A userspace lib together with some simple test programs making use of > this new API can be found at > > https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/libpwm.git > > . > > The start of the series is some cleanup and preparation. The lifetime > and locking patches are needed to not crash the kernel when a character > device is open while a lowlevel driver goes away. This series is already in next for some time, but I'm not sure that I want to really send it to Linus in the next merge window as there are a few issues with it: - A (false positive) lockdep warning reported by Marek Szyprowski. See https://lore.kernel.org/all/5a49cadd-21b7-4384-9e7d-9105ccc288b3@samsung.com - A speculation warning flagged by smatch that I don't understand completely yet (and failed to attract attention by people that know more about about it) See https://lore.kernel.org/all/1e3dc81d-dcd4-4b04-85b1-23937e2f0acd@moroto.mountain - I'm a bit unhappy about the rounding behaviour. Actually I'd like to only provide userspace access via the character device to drivers that adhere to the rounding rules for new drivers (that is: First pick the maximal period that isn't bigger than the requested period. Then for the chosen period pick the maximal duty_cycle that isn't bigger than the requested one) to give a consistent behaviour. This is further complicated by the fact that the character device exposes a more flexible API (involving a duty_offset instead of polarity) and the natural extension for the rounding rules with duty_offset is different than for inverted polarity configurations. I currently consider introducing a new callback that in the long run should replace .apply() and that properly implements the duty_offset stuff. Then the character device could only be provided for the drivers implementing .apply2(). I'm open for feedback, e.g. suggestions for a better name for .apply2(). Best regards Uwe
On Sat, Apr 13, 2024 at 4:22 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello, > > On Sun, Mar 17, 2024 at 11:40:31AM +0100, Uwe Kleine-König wrote: > > After the necessary changes to the lowlevel drivers got in for v6.9-rc1 > > here come some changes to the core to implement /dev/pwmchipX character > > devices. > > > > In my tests on an ARM STM32MP1 programming a PWM using the character > > device is ~4 times faster than just changing duty_cycle via the sysfs > > API. It also has the advantage that (similar to pwm_apply_*) the target > > state is provided to the kernel with a single call, instead of having to > > program the individual settings one after another via sysfs (in the > > right order to not cross states not supported by the driver). > > > > Note the representation of a PWM waveform is different here compared to > > the in-kernel representation. A PWM waveform is represented using: > > > > period > > duty_cycle > > duty_offset > > > > A disabled PWM is represented by period = 0. For an inversed wave use: > > > > duty_offset = duty_cycle > > duty_cycle = period - duty_cycle; > > > > . However there are some difficulties yet that make it hard to provide a > > consistent API to userspace and so for now duty_offset isn't (fully) > > supported yet. That needs some more consideration and can be added > > later. > > > > A userspace lib together with some simple test programs making use of > > this new API can be found at > > > > https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/libpwm.git > > > > . > > > > The start of the series is some cleanup and preparation. The lifetime > > and locking patches are needed to not crash the kernel when a character > > device is open while a lowlevel driver goes away. > > This series is already in next for some time, but I'm not sure that I > want to really send it to Linus in the next merge window as there are a > few issues with it: > > - A (false positive) lockdep warning reported by Marek Szyprowski. > See https://lore.kernel.org/all/5a49cadd-21b7-4384-9e7d-9105ccc288b3@samsung.com > > - A speculation warning flagged by smatch that I don't understand > completely yet (and failed to attract attention by people that know > more about about it) > See https://lore.kernel.org/all/1e3dc81d-dcd4-4b04-85b1-23937e2f0acd@moroto.mountain > > - I'm a bit unhappy about the rounding behaviour. Actually I'd like to > only provide userspace access via the character device to drivers > that adhere to the rounding rules for new drivers (that is: First > pick the maximal period that isn't bigger than the requested period. > Then for the chosen period pick the maximal duty_cycle that isn't > bigger than the requested one) to give a consistent behaviour. This > is further complicated by the fact that the character device exposes > a more flexible API (involving a duty_offset instead of polarity) and > the natural extension for the rounding rules with duty_offset is > different than for inverted polarity configurations. > > I currently consider introducing a new callback that in the long run > should replace .apply() and that properly implements the duty_offset > stuff. Then the character device could only be provided for the drivers > implementing .apply2(). > > I'm open for feedback, e.g. suggestions for a better name for .apply2(). > Waiting to merge this and giving this some more thought first does seem like a wise idea.
Hello, After the necessary changes to the lowlevel drivers got in for v6.9-rc1 here come some changes to the core to implement /dev/pwmchipX character devices. In my tests on an ARM STM32MP1 programming a PWM using the character device is ~4 times faster than just changing duty_cycle via the sysfs API. It also has the advantage that (similar to pwm_apply_*) the target state is provided to the kernel with a single call, instead of having to program the individual settings one after another via sysfs (in the right order to not cross states not supported by the driver). Note the representation of a PWM waveform is different here compared to the in-kernel representation. A PWM waveform is represented using: period duty_cycle duty_offset A disabled PWM is represented by period = 0. For an inversed wave use: duty_offset = duty_cycle duty_cycle = period - duty_cycle; . However there are some difficulties yet that make it hard to provide a consistent API to userspace and so for now duty_offset isn't (fully) supported yet. That needs some more consideration and can be added later. A userspace lib together with some simple test programs making use of this new API can be found at https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/libpwm.git . The start of the series is some cleanup and preparation. The lifetime and locking patches are needed to not crash the kernel when a character device is open while a lowlevel driver goes away. I look forward to feedback, Uwe Uwe Kleine-König (8): pwm: Ensure that pwm_chips are allocated using pwmchip_alloc() pwm: Give some sysfs related variables and functions better names pwm: Move contents of sysfs.c into core.c pwm: Ensure a struct pwm has the same lifetime as its pwm_chip pwm: Add a struct device to struct pwm_chip pwm: Make pwmchip_[sg]et_drvdata() a wrapper around dev_set_drvdata() pwm: Add more locking pwm: Add support for pwmchip devices for faster and easier userspace access drivers/pwm/Kconfig | 4 - drivers/pwm/Makefile | 1 - drivers/pwm/core.c | 988 +++++++++++++++++++++++++++++++++++++-- drivers/pwm/sysfs.c | 545 --------------------- include/linux/pwm.h | 51 +- include/uapi/linux/pwm.h | 23 + 6 files changed, 993 insertions(+), 619 deletions(-) delete mode 100644 drivers/pwm/sysfs.c create mode 100644 include/uapi/linux/pwm.h base-commit: dd6c6d57ab61d496f6ff7d6ca38611062af142a1