Re: [PATCH v3 1/3] misc: servo-pwm: driver for controlling servo motors via PWM

[Angelo Compagnucci]

On Mon, Feb 20, 2023 at 12:40 PM Thierry Reding
<thierry.reding@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxx>
> > +L:   linux-pwm@xxxxxxxxxxxxxxx
> > +S:   Maintained
> > +F:   drivers/misc/servo-pwm.c
> > +
> >  GENERIC RESISTIVE TOUCHSCREEN ADC DRIVER
> >  M:   Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>
> >  L:   linux-input@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxxxxxxxxx>
> > + * 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.

That's true, but anyway it's a big improvement over writing each one
of the servo parameters inside the sysfs one by one.
Moreover, it makes easier to handle a product based on several servo motors.

> Also, how are other aspects of the motor (such as velocity) controlled?
> Wouldn't you want to expose these other controls as well?

As far as I can tell, a basic servo motor only offers a way to change
the angle through PWM duty cycle.
The speed is controlled by the driver inside the motor: the bigger is
the angle, the faster it moves.

There are more complex servos out there, but they don't use plain
simple PWM like those, they usually use some sort of more complex
digital protocol on busses like I2C, SPI, DMX.

>
> Thierry



-- 

Angelo Compagnucci

Software Engineer

angelo@xxxxxxxxxxxxxxxxxxxx
__________________________________
Amarula Solutions SRL

Via le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 (0)42 243 5310
info@xxxxxxxxxxxxxxxxxxxx

www.amarulasolutions.com

[`as] https://www.amarulasolutions.com|