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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4065 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]