Messages in this thread | | | Date | Mon, 20 Feb 2023 12:40:14 +0100 | From | Thierry Reding <> | Subject | Re: [PATCH v3 1/3] misc: servo-pwm: driver for controlling servo motors via PWM |
| |
On Fri, Feb 17, 2023 at 05:10:35PM +0100, Angelo Compagnucci wrote: > This patch adds a simple driver to control servo motor position via > PWM signal. > The driver allows to set the angle from userspace, while min/max > positions duty cycle and the motor degrees aperture are defined in > the dts. > > Signed-off-by: Angelo Compagnucci <angelo@amarulasolutions.com> > --- > v2: > * Driver mostly rewritten for kernel 6.2 > v3: > * Fixed sysfs_emit (greg k-h) > > MAINTAINERS | 6 ++ > drivers/misc/Kconfig | 11 +++ > drivers/misc/Makefile | 1 + > drivers/misc/servo-pwm.c | 149 +++++++++++++++++++++++++++++++++++++++ > 4 files changed, 167 insertions(+) > create mode 100644 drivers/misc/servo-pwm.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 39ff1a717625..8f4af64deb1b 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -8737,6 +8737,12 @@ F: Documentation/devicetree/bindings/power/power?domain* > F: drivers/base/power/domain*.c > F: include/linux/pm_domain.h > > +GENERIC PWM SERVO DRIVER > +M: "Angelo Compagnucci" <angelo@amarulasolutions.com> > +L: linux-pwm@vger.kernel.org > +S: Maintained > +F: drivers/misc/servo-pwm.c > + > GENERIC RESISTIVE TOUCHSCREEN ADC DRIVER > M: Eugen Hristev <eugen.hristev@microchip.com> > L: linux-input@vger.kernel.org > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > index 9947b7892bd5..8a74087149ac 100644 > --- a/drivers/misc/Kconfig > +++ b/drivers/misc/Kconfig > @@ -518,6 +518,17 @@ config VCPU_STALL_DETECTOR > > If you do not intend to run this kernel as a guest, say N. > > +config SERVO_PWM > + tristate "Servo motor positioning" > + depends on PWM > + help > + Driver to control generic servo motor positioning. > + Writing to the "angle" device attribute, the motor will move to > + the angle position. > + > + To compile this driver as a module, choose M here: the module > + will be called servo-pwm. > + > source "drivers/misc/c2port/Kconfig" > source "drivers/misc/eeprom/Kconfig" > source "drivers/misc/cb710/Kconfig" > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > index 87b54a4a4422..936629b648a9 100644 > --- a/drivers/misc/Makefile > +++ b/drivers/misc/Makefile > @@ -64,3 +64,4 @@ obj-$(CONFIG_HI6421V600_IRQ) += hi6421v600-irq.o > obj-$(CONFIG_OPEN_DICE) += open-dice.o > obj-$(CONFIG_GP_PCI1XXXX) += mchp_pci1xxxx/ > obj-$(CONFIG_VCPU_STALL_DETECTOR) += vcpu_stall_detector.o > +obj-$(CONFIG_SERVO_PWM) += servo-pwm.o > diff --git a/drivers/misc/servo-pwm.c b/drivers/misc/servo-pwm.c > new file mode 100644 > index 000000000000..1303ddda8d07 > --- /dev/null > +++ b/drivers/misc/servo-pwm.c > @@ -0,0 +1,149 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (c) 2023 Angelo Compagnucci <angelo@amarulasolutions.com> > + * servo-pwm.c - driver for controlling servo motors via pwm. > + */ > + > +#include <linux/module.h> > +#include <linux/kernel.h> > +#include <linux/platform_device.h> > +#include <linux/err.h> > +#include <linux/pwm.h> > + > +#define DEFAULT_DUTY_MIN 500000 > +#define DEFAULT_DUTY_MAX 2500000 > +#define DEFAULT_DEGREES 175 > +#define DEFAULT_ANGLE 0 > + > +struct servo_pwm_data { > + u32 duty_min; > + u32 duty_max; > + u32 degrees; > + u32 angle; > + > + struct mutex lock; > + struct pwm_device *pwm; > + struct pwm_state pwmstate; > +}; > + > +static int servo_pwm_set(struct servo_pwm_data *data, int val) > +{ > + u64 new_duty = (((data->duty_max - data->duty_min) / > + data->degrees) * val) + data->duty_min;
This one formula is basically the only thing that this driver adds. The remaining 150+ lines are essentially boilerplate to expose the "angle" property via sysfs.
We can already do everything that this driver does via the PWM sysfs, so I wonder if we really need this.
Also, how are other aspects of the motor (such as velocity) controlled? Wouldn't you want to expose these other controls as well?
Thierry [unhandled content-type:application/pgp-signature] | |