Re: [RFC PATCH 1/7] drivers: Add motion control subsystem

Hello David,

just a few highlevel review comments inline.

On Thu, Feb 27, 2025 at 05:28:17PM +0100, David Jander wrote:

Order all <...> includes over the "..." ones.

+#define MOTION_PROFILE_VALID BIT(31)
+
+static LIST_HEAD(motion_list);
+static DEFINE_MUTEX(motion_mtx);
+static int motion_major;
+static DEFINE_IDA(motion_minors_ida);
+
+struct iio_motion_trigger_info {
+	unsigned int minor;
+};
+
+static int motion_minor_alloc(void)
+{
+	int ret;
+
+	ret = ida_alloc_range(&motion_minors_ida, 0, MINORMASK, GFP_KERNEL);
+	return ret;

This could be a one-liner.

+}
+
+static void motion_minor_free(int minor)
+{
+	ida_free(&motion_minors_ida, minor);
+}
+
+static int motion_open(struct inode *inode, struct file *file)
+{
+	int minor = iminor(inode);
+	struct motion_device *mdev = NULL, *iter;
+	int err;
+
+	mutex_lock(&motion_mtx);

If you use guard(), error handling gets a bit easier.

+	list_for_each_entry(iter, &motion_list, list) {
+		if (iter->minor != minor)
+			continue;
+		mdev = iter;
+		break;
+	}

This should be easier. If you use a cdev you can just do
container_of(inode->i_cdev, ...);

+	if (!mdev) {
+		err = -ENODEV;
+		goto fail;
+	}
+
+	dev_info(mdev->dev, "MOTION: open %d\n", mdev->minor);

degrade to dev_dbg.

+	file->private_data = mdev;
+
+	if (mdev->ops.device_open)
+		err = mdev->ops.device_open(mdev);
+	else
+		err = 0;
+fail:
+	mutex_unlock(&motion_mtx);
+	return err;
+}
+
+static int motion_release(struct inode *inode, struct file *file)
+{
+	struct motion_device *mdev = file->private_data;
+	int i;
+
+	if (mdev->ops.device_release)
+		mdev->ops.device_release(mdev);
+
+	for (i = 0; i < mdev->num_gpios; i++) {
+		int irq;
+		struct motion_gpio_input *gpio = &mdev->gpios[i];
+
+		if (gpio->function == MOT_INP_FUNC_NONE)
+			continue;
+		irq = gpiod_to_irq(gpio->gpio);
+		devm_free_irq(mdev->dev, irq, gpio);

It seems devm is just overhead here if you release by hand anyhow.

+		gpio->function = MOT_INP_FUNC_NONE;
+	}
+
+	if (!kfifo_is_empty(&mdev->events))
+		kfifo_reset(&mdev->events);
+
+	/* FIXME: Stop running motions? Probably not... */
+
+	return 0;
+}
+
+static ssize_t motion_read(struct file *file, char __user *buffer,
+			  size_t count, loff_t *ppos)
+{
+	struct motion_device *mdev = file->private_data;
+	unsigned int copied = 0L;
+	int ret;
+
+	if (!mdev->dev)
+		return -ENODEV;
+
+	if (count < sizeof(struct mot_event))
+		return -EINVAL;
+
+	do {
+		if (kfifo_is_empty(&mdev->events)) {
+			if (file->f_flags & O_NONBLOCK)
+				return -EAGAIN;
+
+			ret = wait_event_interruptible(mdev->wait,
+					!kfifo_is_empty(&mdev->events) ||
+					mdev->dev == NULL);
+			if (ret)
+				return ret;
+			if (mdev->dev == NULL)
+				return -ENODEV;
+		}
+
+		if (mutex_lock_interruptible(&mdev->read_mutex))
+			return -ERESTARTSYS;
+		ret = kfifo_to_user(&mdev->events, buffer, count, &copied);
+		mutex_unlock(&mdev->read_mutex);
+
+		if (ret)
+			return ret;
+	} while (!copied);
+
+	return copied;
+}
+
+static __poll_t motion_poll(struct file *file, poll_table *wait)
+{
+	struct motion_device *mdev = file->private_data;
+	__poll_t mask = 0;
+
+	poll_wait(file, &mdev->wait, wait);
+	if (!kfifo_is_empty(&mdev->events))
+		mask = EPOLLIN | EPOLLRDNORM;
+	dev_info(mdev->dev, "Obtained POLL events: 0x%08x\n", mask);

dev_dbg

+
+	return mask;
+}
+
[...]
+
+static long motion_start_locked(struct motion_device *mdev, struct mot_start *s)
+{
+	long ret = 0L;
+	mot_time_t conv_duration;
+
+	lockdep_assert_held(&mdev->mutex);
+
+	if (s->reserved1 || s->reserved2)
+		return -EINVAL;
+	if (s->channel >= mdev->capabilities.num_channels)
+		return -EINVAL;
+	if ((s->index >= MOT_MAX_PROFILES) || (s->direction > MOT_DIRECTION_RIGHT))
+		return -EINVAL;
+	if (!(mdev->profiles[s->index].index & MOTION_PROFILE_VALID))
+		return -EINVAL;
+	if (s->when >= MOT_WHEN_NUM_WHENS)
+		return -EINVAL;
+	if (s->duration && s->distance)
+		return -EINVAL;
+	if (!mdev->ops.motion_distance && !mdev->ops.motion_timed)
+		return -EOPNOTSUPP;

I would add empty lines between these ifs to improve readability. Maybe
thats subjective though.

+	if (s->duration) {
+		if (!mdev->ops.motion_timed)
+			return -EOPNOTSUPP;
+		/* FIXME: Implement time to distance conversion? */
+		return mdev->ops.motion_timed(mdev, s->channel, s->index,
+				s->direction, s->duration, s->when);
+	}
+	if (!mdev->ops.motion_distance) {
+		ret = motion_distance_to_time(mdev, s->index, s->distance,
+				&conv_duration);
+		if (ret)
+			return ret;
+		return mdev->ops.motion_timed(mdev, s->channel, s->index,
+				s->direction, conv_duration, s->when);
+	}
+	ret = mdev->ops.motion_distance(mdev, s->channel, s->index,
+			s->distance, s->when);
+
+	return ret;
+}
[...]
+
+static long motion_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct motion_device *mdev = file->private_data;
+	void __user *argp = (void __user *)arg;
+	long ret;
+
+	switch (cmd) {
+	case MOT_IOCTL_APIVER:
+		force_successful_syscall_return();
+		return MOT_UAPI_VERSION;

force_successful_syscall_return() is only needed if the return value is
negative but no error.

+	case MOT_IOCTL_BASIC_RUN: {
+		struct mot_speed_duration spd;
+
+		if (copy_from_user(&spd, argp, sizeof(spd)))
+			return -EFAULT;
+		if (!mdev->ops.basic_run)
+			return -EINVAL;
[...]
+
+static const struct class motion_class = {
+	.name		= "motion",
+	.devnode	= motion_devnode,

IIRC it's recommended to not create new classes, but a bus.

+};
+
+static const struct file_operations motion_fops = {
+	.owner		= THIS_MODULE,
+	.read		= motion_read,
+	.poll		= motion_poll,
+	.unlocked_ioctl = motion_ioctl,
+	.open		= motion_open,
+	.llseek		= noop_llseek,
+	.release	= motion_release,
+};
+
+static int motion_of_parse_gpios(struct motion_device *mdev)
+{
+	int ngpio, i;
+
+	ngpio = gpiod_count(mdev->parent, "motion,input");
+	if (ngpio < 0) {
+		if (ngpio == -ENOENT)
+			return 0;
+		return ngpio;
+	}
+
+	if (ngpio >= MOT_MAX_INPUTS)
+		return -EINVAL;
+
+	for (i = 0; i < ngpio; i++) {
+		mdev->gpios[i].gpio = devm_gpiod_get_index(mdev->parent,
+				"motion,input", i, GPIOD_IN);
+		if (IS_ERR(mdev->gpios[i].gpio))
+			return PTR_ERR(mdev->gpios[i].gpio);
+		mdev->gpios[i].function = MOT_INP_FUNC_NONE;
+		mdev->gpios[i].chmask = 0;
+		mdev->gpios[i].index = i;
+	}
+
+	mdev->num_gpios = ngpio;
+	mdev->capabilities.num_ext_triggers += ngpio;
+
+	return 0;
+}
+
+static void motion_trigger_work(struct irq_work *work)
+{
+	struct motion_device *mdev = container_of(work, struct motion_device,
+							iiowork);
+	iio_trigger_poll(mdev->iiotrig);
+}
+
+/**
+ * motion_register_device - Register a new Motion Device
+ * @mdev: description and handle of the motion device
+ *
+ * Register a new motion device with the motion subsystem core.
+ * It also handles OF parsing of external trigger GPIOs and registers an IIO
+ * trigger device if IIO support is configured.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+int motion_register_device(struct motion_device *mdev)
+{
+	dev_t devt;
+	int err = 0;
+	struct iio_motion_trigger_info *trig_info;
+
+	if (!mdev->capabilities.num_channels)
+		mdev->capabilities.num_channels = 1;
+	if (mdev->capabilities.features | MOT_FEATURE_PROFILE)
+		mdev->capabilities.max_profiles = MOT_MAX_PROFILES;
+	if (!mdev->capabilities.speed_conv_mul)
+		mdev->capabilities.speed_conv_mul = 1;
+	if (!mdev->capabilities.speed_conv_div)
+		mdev->capabilities.speed_conv_div = 1;
+	if (!mdev->capabilities.accel_conv_mul)
+		mdev->capabilities.accel_conv_mul = 1;
+	if (!mdev->capabilities.accel_conv_div)
+		mdev->capabilities.accel_conv_div = 1;
+
+	mutex_init(&mdev->mutex);
+	mutex_init(&mdev->read_mutex);
+	INIT_KFIFO(mdev->events);
+	init_waitqueue_head(&mdev->wait);
+
+	err = motion_of_parse_gpios(mdev);
+	if (err)
+		return err;
+
+	mdev->minor = motion_minor_alloc();
+
+	mdev->iiotrig = iio_trigger_alloc(NULL, "mottrig%d", mdev->minor);
+	if (!mdev->iiotrig) {
+		err = -ENOMEM;
+		goto error_free_minor;
+	}
+
+	trig_info = kzalloc(sizeof(*trig_info), GFP_KERNEL);
+	if (!trig_info) {
+		err = -ENOMEM;
+		goto error_free_trigger;
+	}
+
+	iio_trigger_set_drvdata(mdev->iiotrig, trig_info);
+
+	trig_info->minor = mdev->minor;
+	err = iio_trigger_register(mdev->iiotrig);
+	if (err)
+		goto error_free_trig_info;
+
+	mdev->iiowork = IRQ_WORK_INIT_HARD(motion_trigger_work);
+
+	INIT_LIST_HEAD(&mdev->list);
+
+	mutex_lock(&motion_mtx);
+
+	devt = MKDEV(motion_major, mdev->minor);
+	mdev->dev = device_create_with_groups(&motion_class, mdev->parent,
+				devt, mdev, mdev->groups, "motion%d", mdev->minor);

What makes sure that mdev doesn't go away while one of the attributes is
accessed?

+	if (IS_ERR(mdev->dev)) {
+		dev_err(mdev->parent, "Error creating motion device %d\n",
+				mdev->minor);
+		mutex_unlock(&motion_mtx);
+		goto error_free_trig_info;
+	}
+	list_add_tail(&mdev->list, &motion_list);
+	mutex_unlock(&motion_mtx);
+
+	return 0;
+
+error_free_trig_info:
+	kfree(trig_info);
+error_free_trigger:
+	iio_trigger_free(mdev->iiotrig);
+error_free_minor:
+	motion_minor_free(mdev->minor);
+	dev_info(mdev->parent, "Registering motion device err=%d\n", err);
+	return err;
+}
+EXPORT_SYMBOL(motion_register_device);
[...]
+struct mot_capabilities {
+	__u32 features;
+	__u8 type;
+	__u8 num_channels;
+	__u8 num_int_triggers;
+	__u8 num_ext_triggers;
+	__u8 max_profiles;
+	__u8 max_vpoints;
+	__u8 max_apoints;
+	__u8 reserved1;
+	__u32 subdiv; /* Position unit sub-divisions, microsteps, etc... */
+	/*
+	 * Coefficients for converting to/from controller time <--> seconds.
+	 * Speed[1/s] = Speed[controller_units] * conv_mul / conv_div
+	 * Accel[1/s^2] = Accel[controller_units] * conv_mul / conv_div
+	 */
+	__u32 speed_conv_mul;
+	__u32 speed_conv_div;
+	__u32 accel_conv_mul;
+	__u32 accel_conv_div;
+	__u32 reserved2;
+};

https://docs.kernel.org/gpu/imagination/uapi.html (which has some
generic bits that apply here, too) has: "The overall struct must be
padded to 64-bit alignment." If you drop reserved2 the struct is
properly sized (or I counted wrongly).

+struct mot_speed_duration {
+	__u32 channel;
+	speed_raw_t speed;

What is the unit here?

+	mot_time_t duration;

duration_ns? That makes usage much more ideomatic and there should be no
doubts what the unit is.

+	pos_raw_t distance;

What is the unit here?

+	__u32 reserved[3];

Again the padding is wrong here.

+};

Best regards
Uwe