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

[Uwe Kleine-König]

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;

You're loosing precision here. Always divide as late as possible. (If
you need an example: With

	duty_max = 1000
	duty_min = 0
	degrees = 251
	val = 79

the exact result for new_duty would be 314.7410358565737. Your term
yields 237. If you divide after multiplying with val you get 314.

All in all I think this driver is too specialized on a single motor
type. IMHO what we would be more helpful is a generic framework that can
abstract various different motors with the same API. 

A while back I already thought about a suitable driver API. The
abstraction I came up with back then was:

	.setspeed(struct motor_device *motor, signed int speed)
	.disable(struct motor_device *motor)

In the end this is probably too simple because it doesn't allow the
consumer to specify how fast the new target speed is to be reached.
(Consider the motor running at 1000 and .setspeed(mymotor, 0) is called.
Should this mean "actively brake", or "stop driving and just coast
down"? I don't have a good idea yet that is both simple enough and still
expressive that it's both useful to consumers and sensible to implement
for different motor types.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature