| Message ID | 20250227162823.3585810-2-david@protonic.nl (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add Linux Motion Control subsystem | expand |
Hello David, just a few highlevel review comments inline. On Thu, Feb 27, 2025 at 05:28:17PM +0100, David Jander wrote: > diff --git a/drivers/motion/motion-core.c b/drivers/motion/motion-core.c > new file mode 100644 > index 000000000000..2963f1859e8b > --- /dev/null > +++ b/drivers/motion/motion-core.c > @@ -0,0 +1,823 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Motion Control Subsystem - Core > + * > + * Copyright (C) 2024 Protonic Holland > + * David Jander <david@protonic.nl> > + */ > + > +#include <asm-generic/bitops/builtin-fls.h> > +#include <asm-generic/errno-base.h> > +#include <linux/interrupt.h> > +#include <linux/irqreturn.h> > +#include <linux/container_of.h> > +#include <linux/hrtimer_types.h> > +#include <linux/gfp_types.h> > +#include <linux/module.h> > + > +#include <linux/fs.h> > +#include <linux/errno.h> > +#include <linux/kernel.h> > +#include <linux/major.h> > +#include <linux/init.h> > +#include <linux/device.h> > +#include <linux/kmod.h> > +#include <linux/motion.h> > +#include <linux/poll.h> > +#include <linux/ptrace.h> > +#include <linux/ktime.h> > +#include <linux/iio/trigger.h> > +#include <linux/gpio/consumer.h> > + > +#include "motion-core.h" > +#include "motion-helpers.h" > +#include <linux/time.h> > +#include <linux/uaccess.h> > +#include <linux/string.h> > +#include <linux/math64.h> > +#include <linux/mutex.h> > +#include <linux/math.h> > +#include <linux/math64.h> 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
On 2/27/25 10:28 AM, David Jander wrote: > The Linux Motion Control subsystem (LMC) is a new driver subsystem for > peripheral devices that control mechanical motion in some form or another. > This could be different kinds of motors (stepper, DC, AC, SRM, BLDC...) > or even linear actuators. > The subsystem presents a unified UAPI for those devices, based on char > devices with ioctl's. > It can make use of regular gpio's to function as trigger inputs, like > end-stops, fixed position- or motion start triggers and also generate > events not only to user-space but also to the IIO subsystem in the form of > IIO triggers. > > Signed-off-by: David Jander <david@protonic.nl> > --- > MAINTAINERS | 8 + > drivers/Kconfig | 2 + > drivers/Makefile | 2 + > drivers/motion/Kconfig | 19 + > drivers/motion/Makefile | 3 + > drivers/motion/motion-core.c | 823 ++++++++++++++++++++++++++++++++ > drivers/motion/motion-core.h | 172 +++++++ > drivers/motion/motion-helpers.c | 590 +++++++++++++++++++++++ > drivers/motion/motion-helpers.h | 23 + > include/uapi/linux/motion.h | 229 +++++++++ > 10 files changed, 1871 insertions(+) Ooof, this is really a lot for one patch. Makes it hard to review. 500 lines in a patch is much easier to digest. But before commenting on the details of the code I have some more high-level comments. As I mentioned in my reply to the cover letter, I've gone through the exercise of writing some motor control divers in the Linux kernel that have been used by 1000s of people that used them to build everything imaginable using LEGO robotics over the last 10+ years. From what I see here (I didn't have time to really get into the details of it yet, so maybe missed some important details), it looks like you are trying to do motor control stuff in the kernel so that the interface for a basic H-bridge will be close to the same as a fancy stepper motor controller. We tried doing something very similar because it sounds like a really nice thing to do. The kernel does everything and makes it really easy for the users. But what we actually found is that it is not possible to make a solution in the kernel that can handle every possible use case. In the end, we wished that we had a much more low-level interface to the motor controllers to give us more flexibility for the many different types of applications this ended up getting used for. Having to modify the kernel for your use case is too high of a bar for most users and not practical even if you are a kernel hacker. When writing kernel drivers for this sort of thing, I think the rule of thumb should be to keep the driver as "thin" as possible. If the hardware doesn't provide a feature, the kernel should not be trying to emulate it. So for an H-bridge I would want something that just provides a way to tell it I want fast-decay mode with some normalized duty cycle between -1 and 1 (obviously we will have to multiply this by some factor since the kernel doesn't do floating point). A duty cycle of 0 will "brake" the motor. And then we would need one more control parameter to tell it to remove power completely to "coast" the motor. I guess this is what the "basic_run" and "basic_stop" are other than the run seems to have speed instead of duty cycle? The kernel shouldn't be trying to convert this duty cycle to speed or have a background task that tries to provide an acceleration profile or turn off the power after some time. Just let the kernel provide direct, low-level access to the hardware and let userspace handle all of the rest in a way that makes the most sense for the specific application. Sometimes they might not even be connected to a motor! With the LEGO MINDSTORMS and BeableBone Blue, the H-bridge outputs are hot-pluggable, so they can even be connected to things like LEDs or used as a general power supply. (A reason to call this subsystem "actuation" rather than "motion".) Another way of putting this is that it was very tempting to model the actual motor in the kernel. But that didn't work well because there are so many different kinds of motors and related mechanical systems that you can connect to the same motor driver chip. So the driver really should just be for the H-bridge chip itself and not care about the motor. And the rest can be put in a libmotion userspace library and have that be the convenient API for users that want to get something up and running quickly.
On Fri, 28 Feb 2025 16:36:41 -0600 David Lechner <dlechner@baylibre.com> wrote: > On 2/27/25 10:28 AM, David Jander wrote: > > The Linux Motion Control subsystem (LMC) is a new driver subsystem for > > peripheral devices that control mechanical motion in some form or another. > > This could be different kinds of motors (stepper, DC, AC, SRM, BLDC...) > > or even linear actuators. > > The subsystem presents a unified UAPI for those devices, based on char > > devices with ioctl's. > > It can make use of regular gpio's to function as trigger inputs, like > > end-stops, fixed position- or motion start triggers and also generate > > events not only to user-space but also to the IIO subsystem in the form of > > IIO triggers. > > > > Signed-off-by: David Jander <david@protonic.nl> > > --- > > MAINTAINERS | 8 + > > drivers/Kconfig | 2 + > > drivers/Makefile | 2 + > > drivers/motion/Kconfig | 19 + > > drivers/motion/Makefile | 3 + > > drivers/motion/motion-core.c | 823 ++++++++++++++++++++++++++++++++ > > drivers/motion/motion-core.h | 172 +++++++ > > drivers/motion/motion-helpers.c | 590 +++++++++++++++++++++++ > > drivers/motion/motion-helpers.h | 23 + > > include/uapi/linux/motion.h | 229 +++++++++ > > 10 files changed, 1871 insertions(+) > > Ooof, this is really a lot for one patch. Makes it hard to review. 500 lines in > a patch is much easier to digest. Sorry for that. I wouldn't know how to split up this patch to make it any easier. It's just a complete new subsystem, and I think it is the bare minimum to start and also to give a good idea of what it is supposed to be able to do. > But before commenting on the details of the code I have some more high-level > comments. As I mentioned in my reply to the cover letter, I've gone through the > exercise of writing some motor control divers in the Linux kernel that have been > used by 1000s of people that used them to build everything imaginable using LEGO > robotics over the last 10+ years. > > From what I see here (I didn't have time to really get into the details of it > yet, so maybe missed some important details), it looks like you are trying to > do motor control stuff in the kernel so that the interface for a basic H-bridge > will be close to the same as a fancy stepper motor controller. We tried doing > something very similar because it sounds like a really nice thing to do. The > kernel does everything and makes it really easy for the users. But what we > actually found is that it is not possible to make a solution in the kernel that > can handle every possible use case. In the end, we wished that we had a much > more low-level interface to the motor controllers to give us more flexibility > for the many different types of applications this ended up getting used for. > Having to modify the kernel for your use case is too high of a bar for most > users and not practical even if you are a kernel hacker. The idea for LMC is to be able to support hardware that realistically can be used by an OS as complex as the Linux kernel. There are a lot of motor controllers out there that suit that category, like the TMC5240 chip for example. But also many bigger systems, like the ones Pavel Pisa works with. That said, I think the Linux kernel on modestly modern SoC's, like the STM32MP1xx is pretty capable of doing more than that, but we have to draw the line somewhere. Like I hinted in the cover letter, I think it might even be possible to do crazy stuff like bit-banging STEP/DIR type controllers in the kernel, possibly aided by PREEMPT_RT, but that is not the main purpose of LMC. The main purpose is to talk to controllers that can receive motion profiles and execute movements (semi-)autonomously. Going just one step lower is the simple PWM based driver in this patch set: It takes the execution of the profile into the kernel by using a HRTIMER to sample the profile curve using a sample period of 20ms, which is fast enough for most applications while not imposing a high CPU load on the kernel on slower CPU cores. motion-helper.c contains the trapezoidal ramp generator code that can be used by any LMC driver that can work with speed information but doesn't have its own ramp generator in hardware. It could be extended to support other type of profiles if needed in the future, which would automatically upgrade all drivers that use it. > When writing kernel drivers for this sort of thing, I think the rule of thumb > should be to keep the driver as "thin" as possible. If the hardware doesn't > provide a feature, the kernel should not be trying to emulate it. In general I would agree with this, but in this case some limited "emulation" seems adequate. As a rule of thumb, code is better placed in an FPGA/uC, the kernel or in user-space according to its latency requirements. Microsecond stuff shouldn't be done on the application SoC at all, millisecond stuff (or maybe 100s of microseconds if you like) can be done in the kernel, 10s of milliseconds or slower is good for user-space. A very general guideline. So if you have a device that can take a speed-setpoint but doesn't have a ramp generator, that is not a device that LMC is made for in the first place, but still pretty usable if we do the ramp generator in the kernel. To steal Pavel's analogy: Think of the TMC5240 driver as a FullMAC Wifi device and the simple PWM driver as a SoftMAC Wifi device. For an LMC device, same as for a wifi interface, we want to talk TCP/IP to it from user-space, not radio-packets. ;-) > So for an > H-bridge I would want something that just provides a way to tell it I want > fast-decay mode with some normalized duty cycle between -1 and 1 (obviously we > will have to multiply this by some factor since the kernel doesn't do floating > point). A duty cycle of 0 will "brake" the motor. And then we would need one > more control parameter to tell it to remove power completely to "coast" the > motor. I guess this is what the "basic_run" and "basic_stop" are other than > the run seems to have speed instead of duty cycle? The kernel shouldn't be > trying to convert this duty cycle to speed or have a background task that tries > to provide an acceleration profile or turn off the power after some time. Just > let the kernel provide direct, low-level access to the hardware and let > userspace handle all of the rest in a way that makes the most sense for the > specific application. Sometimes they might not even be connected to a motor! > With the LEGO MINDSTORMS and BeableBone Blue, the H-bridge outputs are > hot-pluggable, so they can even be connected to things like LEDs or used as a > general power supply. (A reason to call this subsystem "actuation" rather than > "motion".) LMC aims to offer a common interface to different sorts of motion devices (think different types of motors), so offering access to the lowest level of each device is kinda defeating of that purpose. Nevertheless, all of the things you mention are possible with LMC. The relationship between speed and the PWM duty-cycle of a simple DC motor H-bridge for example, is determined by the speed_conv* and accel_conv* parameters. If you want a 1:1 relation, just make them 1 in your device tree. OTOH, if you had only a duty-cycle setting from user-space, you would need to have code that generates an acceleration profile in user-space, which would be a much higher CPU load and have a much higher latency jitter, much more likely to cause mechanical and audible effects than if done in the kernel. It's still possible though. And talking about mis-using a motor driver for something else, that's exactly what one of our customers is doing with a DC motor H-bridge via LMC. They just set each output to 0% or 100% duty-cycle (max speed without profile) to control 2 warning lights. > Another way of putting this is that it was very tempting to model the actual > motor in the kernel. But that didn't work well because there are so many > different kinds of motors and related mechanical systems that you can connect > to the same motor driver chip. Yes, but on the other side, you have many different driver chips that all control motors that do the same thing: move in different directions with different speeds and different acceleration profiles. As a user, I want to be able to tell that to the motor in the same sense as I want to send a data through an ethernet interface without having to know what kind of interface I have to an extend reasonably possible. Of course each motor/driver has different limitations (just as different network interfaces have), and more often than not I will have to know some of those in user-space, but the whole idea of a subsystem UAPI is to abstract as much as possible of that from user-space. With LMC I could easily swap a stepper motor for a BLDC or DC motor for example. If they have an encoder to provide accurate position feedback, it could even work as well as a stepper for many applications. No changes in user-space code required. > So the driver really should just be for the > H-bridge chip itself and not care about the motor. And the rest can be put in > a libmotion userspace library and have that be the convenient API for users > that want to get something up and running quickly. I get your point, and from the standpoint of Lego hardware and hobby tinkerers doing all sorts of stuff with it, I can understand the desire to have low-level access from user-space to the H-bridge. It is similar to how Raspberry-pi exposes direct access to their GPIO controllers to user-space. It is nice for tinkerers, but it is inadequate for anything else, not to mention potentially unsafe. I remember when I first installed Linux on my PC in 1994, we had the X server access the video card directly from user-space. That was a really bad idea. ;-) Best regards,
Hello Davids, On Monday 03 of March 2025 09:36:11 David Jander wrote: > On Fri, 28 Feb 2025 16:36:41 -0600 > > David Lechner <dlechner@baylibre.com> wrote: > > On 2/27/25 10:28 AM, David Jander wrote: > > > The Linux Motion Control subsystem (LMC) is a new driver subsystem for > > > peripheral devices that control mechanical motion in some form or > > > another. This could be different kinds of motors (stepper, DC, AC, SRM, > > > BLDC...) or even linear actuators. > > > The subsystem presents a unified UAPI for those devices, based on char > > > devices with ioctl's. > > > It can make use of regular gpio's to function as trigger inputs, like > > > end-stops, fixed position- or motion start triggers and also generate > > > events not only to user-space but also to the IIO subsystem in the form > > > of IIO triggers. > > > > > > Signed-off-by: David Jander <david@protonic.nl> > > > --- > > > MAINTAINERS | 8 + > > > drivers/Kconfig | 2 + > > > drivers/Makefile | 2 + > > > drivers/motion/Kconfig | 19 + > > > drivers/motion/Makefile | 3 + > > > drivers/motion/motion-core.c | 823 ++++++++++++++++++++++++++++++++ > > > drivers/motion/motion-core.h | 172 +++++++ > > > drivers/motion/motion-helpers.c | 590 +++++++++++++++++++++++ > > > drivers/motion/motion-helpers.h | 23 + > > > include/uapi/linux/motion.h | 229 +++++++++ > > > 10 files changed, 1871 insertions(+) > > > > Ooof, this is really a lot for one patch. Makes it hard to review. 500 > > lines in a patch is much easier to digest. > > Sorry for that. I wouldn't know how to split up this patch to make it any > easier. It's just a complete new subsystem, and I think it is the bare > minimum to start and also to give a good idea of what it is supposed to be > able to do. > > > But before commenting on the details of the code I have some more > > high-level comments. As I mentioned in my reply to the cover letter, I've > > gone through the exercise of writing some motor control divers in the > > Linux kernel that have been used by 1000s of people that used them to > > build everything imaginable using LEGO robotics over the last 10+ years. > > > > From what I see here (I didn't have time to really get into the details > > of it yet, so maybe missed some important details), it looks like you are > > trying to do motor control stuff in the kernel so that the interface for > > a basic H-bridge will be close to the same as a fancy stepper motor > > controller. We tried doing something very similar because it sounds like > > a really nice thing to do. The kernel does everything and makes it really > > easy for the users. But what we actually found is that it is not possible > > to make a solution in the kernel that can handle every possible use case. > > In the end, we wished that we had a much more low-level interface to the > > motor controllers to give us more flexibility for the many different > > types of applications this ended up getting used for. Having to modify > > the kernel for your use case is too high of a bar for most users and not > > practical even if you are a kernel hacker. > > The idea for LMC is to be able to support hardware that realistically can > be used by an OS as complex as the Linux kernel. There are a lot of motor > controllers out there that suit that category, like the TMC5240 chip for > example. But also many bigger systems, like the ones Pavel Pisa works with. > That said, I think the Linux kernel on modestly modern SoC's, like the > STM32MP1xx is pretty capable of doing more than that, but we have to draw > the line somewhere. Like I hinted in the cover letter, I think it might > even be possible to do crazy stuff like bit-banging STEP/DIR type > controllers in the kernel, possibly aided by PREEMPT_RT, but that is not > the main purpose of LMC. I look even to take into account these options for the hobbyist projects. But stepper motors drivers step, dir interface is really demanding case. With microstepping it can require pulse control with frequency 100 kHz or even MHz and corresponding jitter demand. So that is really not area for PREEMPT_RT in userspace nor kernel space. On the other hand, I can imagine that some SoftMAC like controller for Raspberry Pi 1 to 4 to combine its specific DMA and GPIO capabilities could be good target for some LMC like abstraction. Doing DMA from userspace is pain for regular users, yes we use that for Marek Peca's Zlogan https://github.com/eltvor/zlogan and other projects, but that is not for users which I see between our studnets to attempt to stepper motors by step, dir from RPi without previous RT knowledge. The question is if the API to RPi and its DMA should be covered by kernel LMC or some more generic DMA interface. I have found some links for students working on the project (it is project led by my colleagues not mine, https://github.com/hzeller/rpi-gpio-dma-demo https://github.com/innot/AdvPiStepper https://github.com/richardghirst/PiBits/tree/master/ServoBlaster I would be happy for pointers for alive and newwer project if somebody has some more links for these kind of users. I prefer other approaches myself, i.e. small RTOS based solution with background and IRQ tasks and D/Q control where the risk to lose step is much smaller, because you control at sin, cos level, so you have more time at high speeds where current control is not realized on these MHz periods required by dumb step, dir driver with microstepping. Se my previously reported project SaMoCon project for idea. It has four outputs per each axis and can drive PMSM, DC, stepper etc... https://gitlab.fel.cvut.cz/otrees/motion/samocon The PID or even D-Q DC, PMSM and may it be stepper motors at something like 5 kHz is achievable in userspace or kernel RT_REEMPT therad. May it be even with commutation for stepper, but 20 kHz is probably only for kernel and not for professional use according to my taste. If the commutation is pushed even to small FPGA, then some interface as NuttX FOC https://nuttx.apache.org/docs/12.1.0/components/drivers/character/foc.html is good match and yes, proposed LMC pushes much more complexity into the kernel. But for these people who want to stuck on step, dir because it is like that should be done in 3D printers and other hobbits world could profit a lot from some segments list controlled motion and I can image to combine it with that RPi or some simple FPGA block on Zynq, Beagle or iCE40 connected to SPI. So I see value of some unification under LMC > The main purpose is to talk to controllers that > can receive motion profiles and execute movements (semi-)autonomously. > Going just one step lower is the simple PWM based driver in this patch set: > It takes the execution of the profile into the kernel by using a HRTIMER to > sample the profile curve using a sample period of 20ms, which is fast > enough for most applications while not imposing a high CPU load on the > kernel on slower CPU cores. > motion-helper.c contains the trapezoidal ramp generator code that can be > used by any LMC driver that can work with speed information but doesn't > have its own ramp generator in hardware. It could be extended to support > other type of profiles if needed in the future, which would automatically > upgrade all drivers that use it. I agree, but on the other hand, the optimal combination of HW and matching upper abstraction level is extremely variable, and finding a good API for LMC, which would not be a root to stumble upon in the future, is a really demanding task. On the other hand, if some, even suboptimal, solution is available in staging drivers kernel area without guarantee of having fixed API forever, then it can evolve slowly to something valuable. There would be incompatible changes, but the transition can be smooth. So may it be some COMEDI like approach. And there should be matching userspace library which could choose between APIs at different level of abstraction according to the target HW and some these APIs can be emulated by SoftMAC approach in the kernel. Usually for there more hobbits like solutions. I would personally tend to something like our PXMC at these API level but I am predetermined/skewed as author and its 25 years use and connection to older assembly API with more than 30 years history. But I hope I am critical enough to be able to say what is problematic on our design and what can be designed better today. I.e. there is compromise to keep all positions 32 bits, sometimes even reduced to allows IRC sensors position subdivision on ramps generators side, some there is often range reduced to +/-2^23 bits. On the other hand, all is computed in modulo arithmetic, so there is no problem to control speed for infinite time... For very precise and slow syringe piston control in infusion systems, we have used 32 bits position where 8 bits have been fractions and in generators another short extended fractions to 24 bits. It has been able to run with this precision even on 16-bit MSP430. But if that is ported and used in our solutions on 32-bit ARMs or even 64-bit chips like PoalFire, then it loses purpose... So it would worth to reinvent/redefine APIs... But I would tend to this userspace lib and multiple API levels approach... > > When writing kernel drivers for this sort of thing, I think the rule of > > thumb should be to keep the driver as "thin" as possible. If the hardware > > doesn't provide a feature, the kernel should not be trying to emulate it. > > In general I would agree with this, but in this case some limited > "emulation" seems adequate. As a rule of thumb, code is better placed in an > FPGA/uC, the kernel or in user-space according to its latency requirements. > Microsecond stuff shouldn't be done on the application SoC at all, Sure > millisecond stuff (or maybe 100s of microseconds if you like) can be done > in the kernel, 10s of milliseconds or slower is good for user-space. A very > general guideline. I would be open to higher frequencies in userspace with PREEMPT_RT on Zynq, iMX6,7,8,9 or AM3,4,... industrial targetting hardware. We have run complex Simulik generated models on x86 at 30 kHz many years ago without missing code. But with our Simulink RT target https://github.com/aa4cc/ert_linux not with Mathwork's offer. But even serious, rich company came to us to solve their problems on RPi with CAN controller on SPI with sticking on Mathwork's delivered Windows supported target and it has been problem https://dspace.cvut.cz/bitstream/handle/10467/68605/F3-DP-2017-Prudek-Martin-Dp_2017_prudek_martin.pdf So if we speak about enthusiasts with common x86 without SMI interrupt checking before buying the motherboard or even attempting to use laptops or sticking on Raspberry Pi where some versions have used FIQ to solve HW deficiencies, then I would agree to stay even with PREEMP_RT on safe side i.e. under 1 kHz... > So if you have a device that can take a speed-setpoint but doesn't have a > ramp generator, that is not a device that LMC is made for in the first > place, but still pretty usable if we do the ramp generator in the kernel. > To steal Pavel's analogy: Think of the TMC5240 driver as a FullMAC Wifi > device and the simple PWM driver as a SoftMAC Wifi device. > For an LMC device, same as for a wifi interface, we want to talk TCP/IP to > it from user-space, not radio-packets. ;-) Somebody mentioned to me ASICy Nova MCX314 or NPM PCD4541 as reaction to my discussion, I am not sure about their API, but may it be some segmented API with fast, guaranteed, IRQ driven switch/preparation of next segment would support for LMC API. > > So for an > > H-bridge I would want something that just provides a way to tell it I > > want fast-decay mode with some normalized duty cycle between -1 and 1 > > (obviously we will have to multiply this by some factor since the kernel > > doesn't do floating point). A duty cycle of 0 will "brake" the motor. And > > then we would need one more control parameter to tell it to remove power > > completely to "coast" the motor. I guess this is what the "basic_run" and > > "basic_stop" are other than the run seems to have speed instead of duty > > cycle? The kernel shouldn't be trying to convert this duty cycle to speed > > or have a background task that tries to provide an acceleration profile > > or turn off the power after some time. Just let the kernel provide > > direct, low-level access to the hardware and let userspace handle all of > > the rest in a way that makes the most sense for the specific application. > > Sometimes they might not even be connected to a motor! With the LEGO > > MINDSTORMS and BeableBone Blue, the H-bridge outputs are hot-pluggable, > > so they can even be connected to things like LEDs or used as a general > > power supply. (A reason to call this subsystem "actuation" rather than > > "motion".) > > LMC aims to offer a common interface to different sorts of motion devices > (think different types of motors), so offering access to the lowest level > of each device is kinda defeating of that purpose. Nevertheless, all of the > things you mention are possible with LMC. The relationship between speed > and the PWM duty-cycle of a simple DC motor H-bridge for example, is > determined by the speed_conv* and accel_conv* parameters. If you want a 1:1 > relation, just make them 1 in your device tree. But you need full PID or even current D-Q multiple PI and procession, sines, cosines for commutation etc. to connect higher level with lower level. We have all these in fixed point and focused on processing not only in RTOSes kernel but even in IRQs. So it can be done in Linux kernel as we ported, demonstrated it in GNU/Linux userspace... > OTOH, if you had only a duty-cycle setting from user-space, you would need > to have code that generates an acceleration profile in user-space, which > would be a much higher CPU load and have a much higher latency jitter, much > more likely to cause mechanical and audible effects than if done in the > kernel. It's still possible though. > And talking about mis-using a motor driver for something else, that's > exactly what one of our customers is doing with a DC motor H-bridge via > LMC. They just set each output to 0% or 100% duty-cycle (max speed without > profile) to control 2 warning lights. > > > Another way of putting this is that it was very tempting to model the > > actual motor in the kernel. But that didn't work well because there are > > so many different kinds of motors and related mechanical systems that you > > can connect to the same motor driver chip. > > Yes, but on the other side, you have many different driver chips that all > control motors that do the same thing: move in different directions with > different speeds and different acceleration profiles. As a user, I want to > be able to tell that to the motor in the same sense as I want to send a > data through an ethernet interface without having to know what kind of > interface I have to an extend reasonably possible. Of course each > motor/driver has different limitations (just as different network > interfaces have), and more often than not I will have to know some of those > in user-space, but the whole idea of a subsystem UAPI is to abstract as > much as possible of that from user-space. > > With LMC I could easily swap a stepper motor for a BLDC or DC motor for > example. If they have an encoder to provide accurate position feedback, it > could even work as well as a stepper for many applications. No changes in > user-space code required. > > > So the driver really should just be for the > > H-bridge chip itself and not care about the motor. And the rest can be > > put in a libmotion userspace library and have that be the convenient API > > for users that want to get something up and running quickly. > > I get your point, and from the standpoint of Lego hardware and hobby > tinkerers doing all sorts of stuff with it, I can understand the desire to > have low-level access from user-space to the H-bridge. It is similar to how > Raspberry-pi exposes direct access to their GPIO controllers to user-space. > It is nice for tinkerers, but it is inadequate for anything else, not to > mention potentially unsafe. I remember when I first installed Linux on my > PC in 1994, we had the X server access the video card directly from > user-space. That was a really bad idea. ;-) It is questionable, some solution where kernel is RT, allows fast delivery of data and events between kernel and userspace memory contexts, like IO-uring and DRI, shared memory, PRIME buffers etc. is often better than push of all into kernel. In the fact, today lot of graphic stack is pushed directly into applications through acceleration API libraries which talks directly with allocated context in GPUs and the result is combined by compositor, again in userspace. So this is not so convicing argument. But SoftMAC and 802.11 is the case which supports push of more levels of LMC into kernel. Anyway, the motion control is really broad area, there are companies with experience with Linux in their highest range of CNC machines, laser cutters, welding robots etc... So there is lot of prior experience. Question is, how much they are willing to share. But preparing some common guidelines and collect information to not reinvent wheel again and again and then proposing some more generally reusable APIs and drivers etc. would be great win for all and would make GNU/Linux much easier and cheaper usable in the are in future. So my actual vote is to find location where the already available knowledge and projects could be collected and where input even from PREEMP_RT and other professionals can be obtained... May it be somebody from LinuxCNC should step in and may it be there are such resources collected. Linux Foundation or similar body should be contacted to check if such project is already there. I am adding some PREEMP_RT experts which I am in contact with, if they have idea about right place for motion control on Linux discussion, coordination. Best wishes, Pavel - Pavel Pisa phone: +420 603531357 e-mail: pisa@cmp.felk.cvut.cz Department of Control Engineering FEE CVUT Karlovo namesti 13, 121 35, Prague 2 university: http://control.fel.cvut.cz/ personal: http://cmp.felk.cvut.cz/~pisa company: https://pikron.com/ PiKRON s.r.o. Kankovskeho 1235, 182 00 Praha 8, Czech Republic projects: https://www.openhub.net/accounts/ppisa social: https://social.kernel.org/ppisa CAN related:http://canbus.pages.fel.cvut.cz/ RISC-V education: https://comparch.edu.cvut.cz/ Open Technologies Research Education and Exchange Services https://gitlab.fel.cvut.cz/otrees/org/-/wikis/home
Hi Pavel, On Mon, 3 Mar 2025 12:01:33 +0100 Pavel Pisa <ppisa@pikron.com> wrote: > Hello Davids, > > On Monday 03 of March 2025 09:36:11 David Jander wrote: > > On Fri, 28 Feb 2025 16:36:41 -0600 > > > > David Lechner <dlechner@baylibre.com> wrote: > > > On 2/27/25 10:28 AM, David Jander wrote: > > > > The Linux Motion Control subsystem (LMC) is a new driver subsystem for > > > > peripheral devices that control mechanical motion in some form or > > > > another. This could be different kinds of motors (stepper, DC, AC, SRM, > > > > BLDC...) or even linear actuators. > > > > The subsystem presents a unified UAPI for those devices, based on char > > > > devices with ioctl's. > > > > It can make use of regular gpio's to function as trigger inputs, like > > > > end-stops, fixed position- or motion start triggers and also generate > > > > events not only to user-space but also to the IIO subsystem in the form > > > > of IIO triggers. > > > > > > > > Signed-off-by: David Jander <david@protonic.nl> > > > > --- > > > > MAINTAINERS | 8 + > > > > drivers/Kconfig | 2 + > > > > drivers/Makefile | 2 + > > > > drivers/motion/Kconfig | 19 + > > > > drivers/motion/Makefile | 3 + > > > > drivers/motion/motion-core.c | 823 ++++++++++++++++++++++++++++++++ > > > > drivers/motion/motion-core.h | 172 +++++++ > > > > drivers/motion/motion-helpers.c | 590 +++++++++++++++++++++++ > > > > drivers/motion/motion-helpers.h | 23 + > > > > include/uapi/linux/motion.h | 229 +++++++++ > > > > 10 files changed, 1871 insertions(+) > > > > > > Ooof, this is really a lot for one patch. Makes it hard to review. 500 > > > lines in a patch is much easier to digest. > > > > Sorry for that. I wouldn't know how to split up this patch to make it any > > easier. It's just a complete new subsystem, and I think it is the bare > > minimum to start and also to give a good idea of what it is supposed to be > > able to do. > > > > > But before commenting on the details of the code I have some more > > > high-level comments. As I mentioned in my reply to the cover letter, I've > > > gone through the exercise of writing some motor control divers in the > > > Linux kernel that have been used by 1000s of people that used them to > > > build everything imaginable using LEGO robotics over the last 10+ years. > > > > > > From what I see here (I didn't have time to really get into the details > > > of it yet, so maybe missed some important details), it looks like you are > > > trying to do motor control stuff in the kernel so that the interface for > > > a basic H-bridge will be close to the same as a fancy stepper motor > > > controller. We tried doing something very similar because it sounds like > > > a really nice thing to do. The kernel does everything and makes it really > > > easy for the users. But what we actually found is that it is not possible > > > to make a solution in the kernel that can handle every possible use case. > > > In the end, we wished that we had a much more low-level interface to the > > > motor controllers to give us more flexibility for the many different > > > types of applications this ended up getting used for. Having to modify > > > the kernel for your use case is too high of a bar for most users and not > > > practical even if you are a kernel hacker. > > > > The idea for LMC is to be able to support hardware that realistically can > > be used by an OS as complex as the Linux kernel. There are a lot of motor > > controllers out there that suit that category, like the TMC5240 chip for > > example. But also many bigger systems, like the ones Pavel Pisa works with. > > That said, I think the Linux kernel on modestly modern SoC's, like the > > STM32MP1xx is pretty capable of doing more than that, but we have to draw > > the line somewhere. Like I hinted in the cover letter, I think it might > > even be possible to do crazy stuff like bit-banging STEP/DIR type > > controllers in the kernel, possibly aided by PREEMPT_RT, but that is not > > the main purpose of LMC. > > I look even to take into account these options for the hobbyist projects. > But stepper motors drivers step, dir interface is really demanding > case. With microstepping it can require pulse control with frequency > 100 kHz or even MHz and corresponding jitter demand. So that is really > not area for PREEMPT_RT in userspace nor kernel space. Yes, I agree we should try this for hobby purposes to the extent it is possible to make it work. There are some semi-dumb controllers that can do micro-step interpolation automatically, so one doesn't need to toggle STEP for every microstep. Also some controllers can adjust the micro-step resolution on-the-fly via extra GPIO pins. If done synchronized with the corresponding step-count frequency change (easy to do when bit-banging), this can be done without any jerk. But I want to be clear here: This is not the primary intention of LMC. It is a cool experiment to show off how RT-capable the Linux kernel is, but nothing more than that. That said, I do intend to write such a driver for LMC. But use of it for anything other than hobby and tinkering is highly discouraged. ;-) > On the other hand, I can imagine that some SoftMAC like controller > for Raspberry Pi 1 to 4 to combine its specific DMA and GPIO > capabilities could be good target for some LMC like abstraction. Oh sure! I'm really looking forward for other people coming up with interesting drivers for LMC. Although the Rpi hobby-scene is not exactly known for writing kernel drivers to control stuff ;-) > Doing DMA from userspace is pain for regular users, yes we use > that for Marek Peca's Zlogan https://github.com/eltvor/zlogan > and other projects, but that is not for users which I see > between our studnets to attempt to stepper motors by step, dir > from RPi without previous RT knowledge. The question is if the > API to RPi and its DMA should be covered by kernel LMC > or some more generic DMA interface. I don't know what the current status of support for that DMA controller is in the kernel right now, but I think it is at least a tempting target to try something like this. If so, I do think it should be properly abstracted in the kernel for all of its possible applications, and a potential LMC driver should be a user of that abstraction. > I have found some links for students working on the > project (it is project led by my colleagues not mine, > > https://github.com/hzeller/rpi-gpio-dma-demo > > https://github.com/innot/AdvPiStepper > > https://github.com/richardghirst/PiBits/tree/master/ServoBlaster > > I would be happy for pointers for alive and newwer project > if somebody has some more links for these kind of users. > > I prefer other approaches myself, i.e. small RTOS based > solution with background and IRQ tasks and D/Q control > where the risk to lose step is much smaller, because you > control at sin, cos level, so you have more time at high > speeds where current control is not realized on these MHz > periods required by dumb step, dir driver with microstepping. > Se my previously reported project SaMoCon project for > idea. It has four outputs per each axis and can drive > PMSM, DC, stepper etc... > > https://gitlab.fel.cvut.cz/otrees/motion/samocon > > The PID or even D-Q DC, PMSM and may it be stepper > motors at something like 5 kHz is achievable > in userspace or kernel RT_REEMPT therad. May it be > even with commutation for stepper, but 20 kHz > is probably only for kernel and not for professional > use according to my taste. If the commutation > is pushed even to small FPGA, then some interface > as NuttX FOC > > https://nuttx.apache.org/docs/12.1.0/components/drivers/character/foc.html This reminds me of a R&D board I once designed, based on the i.MX8MP SoC, which has a secondary ARM Cortex-M7 core clocked at a whopping 600MHz! I wanted to use that extra core to do bit-banged 3-phase PWM for a BLDC motor controller at high-frequency using GaN bridges and small inductors. I was hoping to achieve up to 100kHz+ PWM cycle frequency by programming a tightly timed loop and no IRQ's. A fixed time-slot in this loop would be used to load PWM parameters from the linux kernel running on the A53 cores via remoteproc. I still have the hardware with GaN drivers and everything... just never got around to actually writing the software :-( > is good match and yes, proposed LMC pushes much more > complexity into the kernel. > > But for these people who want to stuck on step, dir > because it is like that should be done in 3D printers > and other hobbits world could profit a lot from > some segments list controlled motion and I can image > to combine it with that RPi or some simple FPGA > block on Zynq, Beagle or iCE40 connected to SPI. > > So I see value of some unification under LMC Thanks! That is my intention with this project. > > The main purpose is to talk to controllers that > > can receive motion profiles and execute movements (semi-)autonomously. > > Going just one step lower is the simple PWM based driver in this patch set: > > It takes the execution of the profile into the kernel by using a HRTIMER to > > sample the profile curve using a sample period of 20ms, which is fast > > enough for most applications while not imposing a high CPU load on the > > kernel on slower CPU cores. > > motion-helper.c contains the trapezoidal ramp generator code that can be > > used by any LMC driver that can work with speed information but doesn't > > have its own ramp generator in hardware. It could be extended to support > > other type of profiles if needed in the future, which would automatically > > upgrade all drivers that use it. > > I agree, but on the other hand, the optimal combination of HW and matching > upper abstraction level is extremely variable, and finding a good API for > LMC, which would not be a root to stumble upon in the future, is a really > demanding task. On the other hand, if some, even suboptimal, solution is > available in staging drivers kernel area without guarantee of having fixed > API forever, then it can evolve slowly to something valuable. There would be > incompatible changes, but the transition can be smooth. That is also an option of course. My intention was to make the current API extensible in a backward-compatible manner. It is also versioned, so one can add newer versions that offer more functionality if needed, but of course this is more like a last resort. > So may it be some COMEDI like approach. And there should be matching > userspace library which could choose between APIs at different level > of abstraction according to the target HW and some these APIs can > be emulated by SoftMAC approach in the kernel. Usually for there > more hobbits like solutions. A user-space library can always be made, but I'd like to keep that simple and thin if possible. Right now I have a simple user-space library written purely in python. In fact the whole user-space application for this machine is pure python code (asyncio). Many people might declare me crazy for saying this, but it works like a charm and has no trouble handling 16 motors, 26 inputs, 10 outputs with current-feedback and 6 extra ADC channels at sample rates of more than 16kHz all at once. All that on a rather slow Cortex-A7 single core. The trick is to have a good UAPI and make use of epoll with python asyncio wherever possible. It's a good litmus test. > I would personally tend to something like our PXMC at these API > level but I am predetermined/skewed as author and its 25 years > use and connection to older assembly API with more than 30 years > history. Yes, I know this can be challenging, but I'd really like to discuss things with a fresh look if possible. The idea isn't that complicated. I'd attempt to break it down like this: Find as many different motion applications as possible. Describe all the involved movements in terms of their most basic physical aspects involving time and distance (amount or angle of rotations) as well as their 1st and 2nd order time-derivatives speed and acceleration. 2nd order time-derivatives can optionally be described as forces when combined with mass, so if acceleration can vary over time, so can force. Verify that any higher-order time-derivatives can be described either as constant limits (jerk, snap, crackle and pop) or form part of the hardware/firmware. Verify that speed at this level of abstraction is always the result of either an acceleration- or a force profile. If fitting the application into this domain requires fast close-loop control of some variable, consider incorporating the needed closed loop control into the hardware/firmware, and try again. If the required control loop can be made slow and simple enough to not be problematic to be implemented in the kernel, that's worth considering. If you can not fit the application into this domain at all, we need to go back to the drawing board or add stuff. Next, try to find ways to define trajectories of position and speed (acceleration or force profiles) in a common language. Then reduce that language to the minimum common denominator that covers all use-cases. This is the language I've come up with so far: https://github.com/yope/linux/blob/mainline-lmc-preparation/Documentation/userspace-api/motion.rst We already established that it is probably worth and valuable adding some way of conveying spline interpolation parameters for position and speed points. I suggest modeling that according to some existing hardware that uses this, while keeping an eye on it not being machine-specific. This would require to have a functioning LMC driver for this use-case, not to be included in this version, but to test the UAPI to the extent it needs to be set in stone. Lastly, if there are several fitting motion controllers that could be used for a certain target application, it should be possible to swap them without the need to change the user-space software more than maybe changing some configuration setting that doesn't affect how it talks to the kernel. Let's test it with more use-cases. > But I hope I am critical enough to be able to say what is problematic > on our design and what can be designed better today. I.e. there > is compromise to keep all positions 32 bits, sometimes even > reduced to allows IRC sensors position subdivision on ramps > generators side, some there is often range reduced to +/-2^23 > bits. On the other hand, all is computed in modulo arithmetic, > so there is no problem to control speed for infinite time... > For very precise and slow syringe piston control in infusion > systems, we have used 32 bits position where 8 bits have been > fractions and in generators another short extended fractions > to 24 bits. It has been able to run with this precision > even on 16-bit MSP430. But if that is ported and used in our > solutions on 32-bit ARMs or even 64-bit chips like PoalFire, > then it loses purpose... So it would worth to reinvent/redefine > APIs... I know how it is when older use-cases evolve throughout many years. Technical limitations and concessions tend to disappear over time. In this case we should probably assume a modern FPGA, uC or custom controller chip connected to a modern embedded Linux system. The communication between the Linux system and the control circuit can be anything from a simple GPIO or PWM signal, through I2C, SPI or CAN up to modern 1000Base-T1 ethernet. The possibility of using remoteproc when adequate should be considered. The UAPI should be generally usable on basically any platform the Linux kernel can run on. > But I would tend to this userspace lib and multiple API levels > approach... Let's try to see if we can do with the current UAPI. It does have some notion of "levels of complexity", where the driver/hardware is interrogated for the capabilities it supports. See here: https://github.com/yope/linux/blob/mainline-lmc-preparation/Documentation/userspace-api/motion.rst#structs-used-in-the-ioctl-api So some drivers might only support "basic motion control", like only setting a fixed speed (even if it is only on/off, left/right). All drivers can be augmented with GPIO-based trigger/end-stop interrupts defined in the device-tree as "gpios" vector. Other drivers might be able to use locally connected feedback inputs ("feedback control"). More advanced drivers might support "profile-based control", that accepts speed/acceleration profiles with varying complexity. Lastly (not yet implemented) drivers can support also torque limit profiles and/or multi-axis movements (n-dimensional position vectors). If adequate, a user-space library could hide some of the decision-making based on the capabilities and maybe provide some user-space emulation of some missing features, but I haven't really thought very deeply about that. > > > When writing kernel drivers for this sort of thing, I think the rule of > > > thumb should be to keep the driver as "thin" as possible. If the hardware > > > doesn't provide a feature, the kernel should not be trying to emulate it. > > > > In general I would agree with this, but in this case some limited > > "emulation" seems adequate. As a rule of thumb, code is better placed in an > > FPGA/uC, the kernel or in user-space according to its latency requirements. > > Microsecond stuff shouldn't be done on the application SoC at all, > > Sure > > > millisecond stuff (or maybe 100s of microseconds if you like) can be done > > in the kernel, 10s of milliseconds or slower is good for user-space. A very > > general guideline. > > I would be open to higher frequencies in userspace with PREEMPT_RT > on Zynq, iMX6,7,8,9 or AM3,4,... industrial targetting hardware. That may be possible, but would that be a desirable design from an architectural standpoint? What would be the reason for this. Can you explain a use-case where that's required or desirable? > We have run complex Simulik generated models on x86 at 30 kHz > many years ago without missing code. But with our Simulink RT > target https://github.com/aa4cc/ert_linux This sounds pretty cool. We might even want this. :-) But isn't it the idea to use Simulink for the R&D phase to develop control strategies, to then implement them in C in a microcontroller or FPGA for the end-application? Wouldn't the end result be a microcontroller being talked to through some standard interface from a Linux host that sends higher-level commands to it? We also have a motor lab where we can simulate mechanical loads with SRL or AC machines to simulate a variable loads to another motor (also AC, DC, BLDC or SRL machine) controlled from Simulink (using dspace). Once the solution is found, developed an tested, the final implementation is made in some sort of microcontroller that has maybe a CAN interface or SPI or similar... While being able to use LMC for these kind of lab-applications sounds cool, I am not sure we would want that. Or at least, IMHO, it shouldn't be its main focus, but rather the end application should be. > not with Mathwork's > offer. But even serious, rich company came to us to solve their > problems on RPi with CAN controller on SPI with sticking > on Mathwork's delivered Windows supported target and it has been > problem > > https://dspace.cvut.cz/bitstream/handle/10467/68605/F3-DP-2017-Prudek-Martin-Dp_2017_prudek_martin.pdf Oh dear. dspace. ;-) > So if we speak about enthusiasts with common x86 without > SMI interrupt checking before buying the motherboard or even > attempting to use laptops or sticking on Raspberry Pi where > some versions have used FIQ to solve HW deficiencies, then > I would agree to stay even with PREEMP_RT on safe side > i.e. under 1 kHz... Don't get me wrong, if there is merit to it, I am all for attempting faster stuff in the kernel, specially if that is the best solution. But for user-space I'd rather stay at 1kHz or less as a requirement. If you want to feed micrometer long linear segments of a spline interpolation at 10kHz to the kernel from user-space, fine if your platform can handle that, but let's not make it a requirement or assume this level of latency and timing control is available universally. > > So if you have a device that can take a speed-setpoint but doesn't have a > > ramp generator, that is not a device that LMC is made for in the first > > place, but still pretty usable if we do the ramp generator in the kernel. > > To steal Pavel's analogy: Think of the TMC5240 driver as a FullMAC Wifi > > device and the simple PWM driver as a SoftMAC Wifi device. > > For an LMC device, same as for a wifi interface, we want to talk TCP/IP to > > it from user-space, not radio-packets. ;-) > > Somebody mentioned to me ASICy Nova MCX314 or NPM PCD4541 Wow, cool. The MCX314 actually sound like a good candidate for an LMC driver. Only the CPU bus interface is a bit... outdated ;-) > as reaction to my discussion, I am not sure about their API, > but may it be some segmented API with fast, guaranteed, > IRQ driven switch/preparation of next segment would support > for LMC API. Apparently it can generate interrupts for passing a pre-set position, changing from acceleration fase to constant speed, changing to deceleration or reaching the target. > > > So for an > > > H-bridge I would want something that just provides a way to tell it I > > > want fast-decay mode with some normalized duty cycle between -1 and 1 > > > (obviously we will have to multiply this by some factor since the kernel > > > doesn't do floating point). A duty cycle of 0 will "brake" the motor. And > > > then we would need one more control parameter to tell it to remove power > > > completely to "coast" the motor. I guess this is what the "basic_run" and > > > "basic_stop" are other than the run seems to have speed instead of duty > > > cycle? The kernel shouldn't be trying to convert this duty cycle to speed > > > or have a background task that tries to provide an acceleration profile > > > or turn off the power after some time. Just let the kernel provide > > > direct, low-level access to the hardware and let userspace handle all of > > > the rest in a way that makes the most sense for the specific application. > > > Sometimes they might not even be connected to a motor! With the LEGO > > > MINDSTORMS and BeableBone Blue, the H-bridge outputs are hot-pluggable, > > > so they can even be connected to things like LEDs or used as a general > > > power supply. (A reason to call this subsystem "actuation" rather than > > > "motion".) > > > > LMC aims to offer a common interface to different sorts of motion devices > > (think different types of motors), so offering access to the lowest level > > of each device is kinda defeating of that purpose. Nevertheless, all of the > > things you mention are possible with LMC. The relationship between speed > > and the PWM duty-cycle of a simple DC motor H-bridge for example, is > > determined by the speed_conv* and accel_conv* parameters. If you want a 1:1 > > relation, just make them 1 in your device tree. > > But you need full PID or even current D-Q multiple PI and procession, > sines, cosines for commutation etc. to connect higher level with lower > level. We have all these in fixed point and focused on processing > not only in RTOSes kernel but even in IRQs. So it can be done > in Linux kernel as we ported, demonstrated it in GNU/Linux userspace... Hmm.. I was referring to simple brushed DC motors, not something like BLDC motors when I mentioned a "simple DC motor" H-bridge. I don't intend to support real-time control of a BLDC motor in the kernel. That should definitely be done by specialize hardware (uC, FPGA or specialized control chip). > > OTOH, if you had only a duty-cycle setting from user-space, you would need > > to have code that generates an acceleration profile in user-space, which > > would be a much higher CPU load and have a much higher latency jitter, much > > more likely to cause mechanical and audible effects than if done in the > > kernel. It's still possible though. > > And talking about mis-using a motor driver for something else, that's > > exactly what one of our customers is doing with a DC motor H-bridge via > > LMC. They just set each output to 0% or 100% duty-cycle (max speed without > > profile) to control 2 warning lights. > > > > > Another way of putting this is that it was very tempting to model the > > > actual motor in the kernel. But that didn't work well because there are > > > so many different kinds of motors and related mechanical systems that you > > > can connect to the same motor driver chip. > > > > Yes, but on the other side, you have many different driver chips that all > > control motors that do the same thing: move in different directions with > > different speeds and different acceleration profiles. As a user, I want to > > be able to tell that to the motor in the same sense as I want to send a > > data through an ethernet interface without having to know what kind of > > interface I have to an extend reasonably possible. Of course each > > motor/driver has different limitations (just as different network > > interfaces have), and more often than not I will have to know some of those > > in user-space, but the whole idea of a subsystem UAPI is to abstract as > > much as possible of that from user-space. > > > > With LMC I could easily swap a stepper motor for a BLDC or DC motor for > > example. If they have an encoder to provide accurate position feedback, it > > could even work as well as a stepper for many applications. No changes in > > user-space code required. > > > > > So the driver really should just be for the > > > H-bridge chip itself and not care about the motor. And the rest can be > > > put in a libmotion userspace library and have that be the convenient API > > > for users that want to get something up and running quickly. > > > > I get your point, and from the standpoint of Lego hardware and hobby > > tinkerers doing all sorts of stuff with it, I can understand the desire to > > have low-level access from user-space to the H-bridge. It is similar to how > > Raspberry-pi exposes direct access to their GPIO controllers to user-space. > > It is nice for tinkerers, but it is inadequate for anything else, not to > > mention potentially unsafe. I remember when I first installed Linux on my > > PC in 1994, we had the X server access the video card directly from > > user-space. That was a really bad idea. ;-) > > It is questionable, some solution where kernel is RT, allows fast delivery > of data and events between kernel and userspace memory contexts, > like IO-uring and DRI, shared memory, PRIME buffers etc. is often > better than push of all into kernel. In the fact, today lot of graphic > stack is pushed directly into applications through acceleration API > libraries which talks directly with allocated context in GPUs > and the result is combined by compositor, again in userspace. Yes, but there is always a kernel driver involved that does memory management, checks and filters the GPU command stream and does error handling and recovery. What I meant that back in 1994 the X-server simply asked the kernel "give me access to this range of IO-registers and memory". This is a big vulnerability of course and can lead to user-space locking up a machine. Not good. > So this is not so convicing argument. But SoftMAC and 802.11 > is the case which supports push of more levels of LMC > into kernel. > > Anyway, the motion control is really broad area, there are companies > with experience with Linux in their highest range of CNC machines, > laser cutters, welding robots etc... So there is lot of prior > experience. Question is, how much they are willing to share. We will have to see. > But preparing some common guidelines and collect information > to not reinvent wheel again and again and then proposing > some more generally reusable APIs and drivers etc. would be > great win for all and would make GNU/Linux much easier > and cheaper usable in the are in future. I agree. > So my actual vote is to find location where the already > available knowledge and projects could be collected > and where input even from PREEMP_RT and other professionals > can be obtained... Sounds like a good idea. Let's look for such a place. You seem to have more contacts than I have, so maybe you can suggest someone to ask? > May it be somebody from LinuxCNC should step in and may it > be there are such resources collected. Linux Foundation > or similar body should be contacted to check if such > project is already there. I am adding some PREEMP_RT > experts which I am in contact with, if they have idea > about right place for motion control on Linux discussion, > coordination. Ok. Great. I will try to read up a bit on LinuxCNC and see if I can contact some of the developers. It would be good to at least give them a heads-up about the existence of this project to begin with. Best regards,
Hi Uwe, On Fri, 28 Feb 2025 17:44:27 +0100 Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > Hello David, > > just a few highlevel review comments inline. Thanks... > On Thu, Feb 27, 2025 at 05:28:17PM +0100, David Jander wrote: > [...] > > +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. This looks interesting. I didn't know about guard(). Thanks. I see the benefits, but in some cases it also makes the locked region less clearly visible. While I agree that guard() in this particular place is nice, I'm hesitant to try and replace all mutex_lock()/_unlock() calls with guard(). Let me know if my assessment of the intended use of guard() is incorrect. > > + 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, ...); Hmm... I don't yet really understand what you mean. I will have to study the involved code a bit more. > [...] > > +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. Ack. This looks indeed unnecessary... I'll try to use non-devres calls here. > > [...] > > + > > +static const struct class motion_class = { > > + .name = "motion", > > + .devnode = motion_devnode, > > IIRC it's recommended to not create new classes, but a bus. Interesting. I did some searching, and all I could find was that the chapter in driver-api/driver-model about classes magically vanished between versions 5.12 and 5.13. Does anyone know where I can find some information about this? Sorry if I'm being blind... > [...] > > +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? Good question. I suppose you mean that since mdev is devres-managed and device_create_with_groups() apparently isn't aware of that, so there is no internal lock somewhere that prevents read() or ioctl() being called while the devres code is freeing the memory of mdev? I will try to search for some example code to see how something like this is handled in other places. I assume I'd need to add a per-mdev lock or use the big motion_mtx everywhere... which sounds like a performance penalty that should be avoidable. If you know of a good example to learn from, I'd be grateful to know. > > + 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). Oh, thanks for pointing that out... I wouldn't have searched for that information in that particular place tbh. ;-) I am tempted to add another __u32 reserved3 though instead. Better to have some leeway if something needs to be added in a backwards-compatible way later. > > +struct mot_speed_duration { > > + __u32 channel; > > + speed_raw_t speed; > > What is the unit here? Speed doesn't have a fixed unit in this case. Or rather, the unit is device-dependent. For a motor it could be rotations per second, micro-steps per second, etc... while for a linear actuator, it could be micrometers per second. Why no fixed unit? That's because in practice many devices (controllers) have their inherent base-unit, and it would get overly complicated if one needed to convert back and forth between that and some universal unit just for the sake of uniformity, and user-space most certainly expects the same unit as the hardware device it was initially designed for. So in this case it is a design decision to make user-space deal with unit-conversion if it is necessary to do so. > > + mot_time_t duration; > > duration_ns? That makes usage much more ideomatic and there should be no > doubts what the unit is. Yes, mot_time_t is defined as nanoseconds, so I'll add the _ns suffix here. > > + pos_raw_t distance; > > What is the unit here? Again this unit can have different meanings: micrometers, micro-steps, angle-degrees, etc... so what suffix to use? > > + __u32 reserved[3]; > > Again the padding is wrong here. Will fix. thanks. Best regards,
Hello David, On Wed, Mar 05, 2025 at 04:40:45PM +0100, David Jander wrote: > On Fri, 28 Feb 2025 17:44:27 +0100 > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > > On Thu, Feb 27, 2025 at 05:28:17PM +0100, David Jander wrote: > > [...] > > > +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. > > This looks interesting. I didn't know about guard(). Thanks. I see the > benefits, but in some cases it also makes the locked region less clearly > visible. While I agree that guard() in this particular place is nice, > I'm hesitant to try and replace all mutex_lock()/_unlock() calls with guard(). > Let me know if my assessment of the intended use of guard() is incorrect. I agree that guard() makes it harder for non-trivial functions to spot the critical section. In my eyes this is outweight by not having to unlock in all exit paths, but that might be subjective. Annother downside of guard is that sparse doesn't understand it and reports unbalanced locking. > > > + 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, ...); > > Hmm... I don't yet really understand what you mean. I will have to study the > involved code a bit more. The code that I'm convinced is correct is https://lore.kernel.org/linux-pwm/00c9f1181dc351e1e6041ba6e41e4c30b12b6a27.1725635013.git.u.kleine-koenig@baylibre.com/ This isn't in mainline because there is some feedback I still have to address, but I think it might serve as an example anyhow. > > > [...] > > > + > > > +static const struct class motion_class = { > > > + .name = "motion", > > > + .devnode = motion_devnode, > > > > IIRC it's recommended to not create new classes, but a bus. > > Interesting. I did some searching, and all I could find was that the chapter > in driver-api/driver-model about classes magically vanished between versions > 5.12 and 5.13. Does anyone know where I can find some information about this? > Sorry if I'm being blind... Half knowledge on my end at best. I would hope that Greg knows some details (which might even be "no, classes are fine"). I added him to Cc: > > [...] > > > + 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? > > Good question. I suppose you mean that since mdev is devres-managed and > device_create_with_groups() apparently isn't aware of that, so there is no > internal lock somewhere that prevents read() or ioctl() being called while the > devres code is freeing the memory of mdev? I'm not sure there is an issue, but when I developed the above mentioned patch it helped me to test these possible races. Just open the sysfs file, unbind the device (or unload the module) and only then start reading (or writing). > > > + 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). > > Oh, thanks for pointing that out... I wouldn't have searched for that > information in that particular place tbh. ;-) > > I am tempted to add another __u32 reserved3 though instead. Better to have > some leeway if something needs to be added in a backwards-compatible way later. Note that you don't need reserved fields at the end because in the ioctl handler you know the size of the passed struct. So if the need to add members to the struct arise, you can do that by checking for the size. This is even more flexible because otherwise you can only add fields that must be 0 when the old behaviour is intended. Most of the time this is no problem. But only most. > > > +struct mot_speed_duration { > > > + __u32 channel; > > > + speed_raw_t speed; > > > > What is the unit here? > > Speed doesn't have a fixed unit in this case. Or rather, the unit is > device-dependent. For a motor it could be rotations per second, micro-steps per > second, etc... while for a linear actuator, it could be micrometers per second. > > Why no fixed unit? That's because in practice many devices (controllers) have > their inherent base-unit, and it would get overly complicated if one needed to > convert back and forth between that and some universal unit just for the sake > of uniformity, and user-space most certainly expects the same unit as the > hardware device it was initially designed for. So in this case it is a design > decision to make user-space deal with unit-conversion if it is necessary to do > so. Sad, so a userspace process still has to know some internal things about the motor it drives. :-\ Best regards Uwe
On Thu, Mar 06, 2025 at 12:21:22AM +0100, Uwe Kleine-König wrote: > Hello David, > > On Wed, Mar 05, 2025 at 04:40:45PM +0100, David Jander wrote: > > On Fri, 28 Feb 2025 17:44:27 +0100 > > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > > > On Thu, Feb 27, 2025 at 05:28:17PM +0100, David Jander wrote: > > > [...] > > > > +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. > > > > This looks interesting. I didn't know about guard(). Thanks. I see the > > benefits, but in some cases it also makes the locked region less clearly > > visible. While I agree that guard() in this particular place is nice, > > I'm hesitant to try and replace all mutex_lock()/_unlock() calls with guard(). > > Let me know if my assessment of the intended use of guard() is incorrect. > > I agree that guard() makes it harder for non-trivial functions to spot > the critical section. In my eyes this is outweight by not having to > unlock in all exit paths, but that might be subjective. Annother > downside of guard is that sparse doesn't understand it and reports > unbalanced locking. > > > > > + 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, ...); > > > > Hmm... I don't yet really understand what you mean. I will have to study the > > involved code a bit more. > > The code that I'm convinced is correct is > https://lore.kernel.org/linux-pwm/00c9f1181dc351e1e6041ba6e41e4c30b12b6a27.1725635013.git.u.kleine-koenig@baylibre.com/ > > This isn't in mainline because there is some feedback I still have to > address, but I think it might serve as an example anyhow. > > > > > [...] > > > > + > > > > +static const struct class motion_class = { > > > > + .name = "motion", > > > > + .devnode = motion_devnode, > > > > > > IIRC it's recommended to not create new classes, but a bus. > > > > Interesting. I did some searching, and all I could find was that the chapter > > in driver-api/driver-model about classes magically vanished between versions > > 5.12 and 5.13. Does anyone know where I can find some information about this? > > Sorry if I'm being blind... > > Half knowledge on my end at best. I would hope that Greg knows some > details (which might even be "no, classes are fine"). I added him to Cc: A class is there for when you have a common api that devices of different types can talk to userspace (i.e. the UAPI is common, not the hardware type). Things like input devices, tty, disks, etc. A bus is there to be able to write different drivers to bind to for that hardware bus type (pci, usb, i2c, platform, etc.) So you need both, a bus to talk to the hardware, and a class to talk to userspace in a common way (ignore the fact that we can also talk to hardware directly from userspace like raw USB or i2c or PCI config space, that's all bus-specific stuff). Did that help? thanks, greg k-h
On Thu, 6 Mar 2025 08:18:46 +0100 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Thu, Mar 06, 2025 at 12:21:22AM +0100, Uwe Kleine-König wrote: > > Hello David, > > > > On Wed, Mar 05, 2025 at 04:40:45PM +0100, David Jander wrote: > > > On Fri, 28 Feb 2025 17:44:27 +0100 > > > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > > > > On Thu, Feb 27, 2025 at 05:28:17PM +0100, David Jander wrote: > > > > [...] > > > > > +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. > > > > > > This looks interesting. I didn't know about guard(). Thanks. I see the > > > benefits, but in some cases it also makes the locked region less clearly > > > visible. While I agree that guard() in this particular place is nice, > > > I'm hesitant to try and replace all mutex_lock()/_unlock() calls with guard(). > > > Let me know if my assessment of the intended use of guard() is incorrect. > > > > I agree that guard() makes it harder for non-trivial functions to spot > > the critical section. In my eyes this is outweight by not having to > > unlock in all exit paths, but that might be subjective. Annother > > downside of guard is that sparse doesn't understand it and reports > > unbalanced locking. > > > > > > > + 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, ...); > > > > > > Hmm... I don't yet really understand what you mean. I will have to study the > > > involved code a bit more. > > > > The code that I'm convinced is correct is > > https://lore.kernel.org/linux-pwm/00c9f1181dc351e1e6041ba6e41e4c30b12b6a27.1725635013.git.u.kleine-koenig@baylibre.com/ > > > > This isn't in mainline because there is some feedback I still have to > > address, but I think it might serve as an example anyhow. > > > > > > > [...] > > > > > + > > > > > +static const struct class motion_class = { > > > > > + .name = "motion", > > > > > + .devnode = motion_devnode, > > > > > > > > IIRC it's recommended to not create new classes, but a bus. > > > > > > Interesting. I did some searching, and all I could find was that the chapter > > > in driver-api/driver-model about classes magically vanished between versions > > > 5.12 and 5.13. Does anyone know where I can find some information about this? > > > Sorry if I'm being blind... > > > > Half knowledge on my end at best. I would hope that Greg knows some > > details (which might even be "no, classes are fine"). I added him to Cc: > > A class is there for when you have a common api that devices of > different types can talk to userspace (i.e. the UAPI is common, not the > hardware type). Things like input devices, tty, disks, etc. A bus is > there to be able to write different drivers to bind to for that hardware > bus type (pci, usb, i2c, platform, etc.) > > So you need both, a bus to talk to the hardware, and a class to talk to > userspace in a common way (ignore the fact that we can also talk to > hardware directly from userspace like raw USB or i2c or PCI config > space, that's all bus-specific stuff). Thanks for chiming in. Let me see if I understand this correctly: In this case, I have a UAPI that is common to different types of motion control devices. So I need a class. check. Do I need a bus? If one can conceive other drivers or kernel parts that talk to motion drivers, I would need a bus. If that doesn't make sense, I don't. Right? I actually can think of a new motion device that acts as an aggregator of several single-channel motion devices into a single "virtual" multi-channel device... so do I need also a bus? I suppose...? Then the question remains: why did the chapter about classes vanish? Best regards,
On Thu, Mar 06, 2025 at 09:20:13AM +0100, David Jander wrote: > On Thu, 6 Mar 2025 08:18:46 +0100 > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > On Thu, Mar 06, 2025 at 12:21:22AM +0100, Uwe Kleine-König wrote: > > > Hello David, > > > > > > On Wed, Mar 05, 2025 at 04:40:45PM +0100, David Jander wrote: > > > > On Fri, 28 Feb 2025 17:44:27 +0100 > > > > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > > > > > On Thu, Feb 27, 2025 at 05:28:17PM +0100, David Jander wrote: > > > > > [...] > > > > > > +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. > > > > > > > > This looks interesting. I didn't know about guard(). Thanks. I see the > > > > benefits, but in some cases it also makes the locked region less clearly > > > > visible. While I agree that guard() in this particular place is nice, > > > > I'm hesitant to try and replace all mutex_lock()/_unlock() calls with guard(). > > > > Let me know if my assessment of the intended use of guard() is incorrect. > > > > > > I agree that guard() makes it harder for non-trivial functions to spot > > > the critical section. In my eyes this is outweight by not having to > > > unlock in all exit paths, but that might be subjective. Annother > > > downside of guard is that sparse doesn't understand it and reports > > > unbalanced locking. > > > > > > > > > + 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, ...); > > > > > > > > Hmm... I don't yet really understand what you mean. I will have to study the > > > > involved code a bit more. > > > > > > The code that I'm convinced is correct is > > > https://lore.kernel.org/linux-pwm/00c9f1181dc351e1e6041ba6e41e4c30b12b6a27.1725635013.git.u.kleine-koenig@baylibre.com/ > > > > > > This isn't in mainline because there is some feedback I still have to > > > address, but I think it might serve as an example anyhow. > > > > > > > > > [...] > > > > > > + > > > > > > +static const struct class motion_class = { > > > > > > + .name = "motion", > > > > > > + .devnode = motion_devnode, > > > > > > > > > > IIRC it's recommended to not create new classes, but a bus. > > > > > > > > Interesting. I did some searching, and all I could find was that the chapter > > > > in driver-api/driver-model about classes magically vanished between versions > > > > 5.12 and 5.13. Does anyone know where I can find some information about this? > > > > Sorry if I'm being blind... > > > > > > Half knowledge on my end at best. I would hope that Greg knows some > > > details (which might even be "no, classes are fine"). I added him to Cc: > > > > A class is there for when you have a common api that devices of > > different types can talk to userspace (i.e. the UAPI is common, not the > > hardware type). Things like input devices, tty, disks, etc. A bus is > > there to be able to write different drivers to bind to for that hardware > > bus type (pci, usb, i2c, platform, etc.) > > > > So you need both, a bus to talk to the hardware, and a class to talk to > > userspace in a common way (ignore the fact that we can also talk to > > hardware directly from userspace like raw USB or i2c or PCI config > > space, that's all bus-specific stuff). > > Thanks for chiming in. Let me see if I understand this correctly: In this > case, I have a UAPI that is common to different types of motion control > devices. So I need a class. check. Correct. > Do I need a bus? If one can conceive other drivers or kernel parts that talk to > motion drivers, I would need a bus. If that doesn't make sense, I don't. Right? Correct. > I actually can think of a new motion device that acts as an aggregator of > several single-channel motion devices into a single "virtual" multi-channel > device... so do I need also a bus? I suppose...? Nope, that should just be another class driver. Think about how input does this, some input /dev/ nodes are the sum of ALL input /dev/ nodes together, while others are just for individual input devices. > Then the question remains: why did the chapter about classes vanish? What are you specifically referring to? I don't remember deleting any documentation, did files move around somehow and the links not get updated? thanks, greg k-h
On Thu, 6 Mar 2025 00:21:22 +0100 Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > Hello David, > > On Wed, Mar 05, 2025 at 04:40:45PM +0100, David Jander wrote: > > On Fri, 28 Feb 2025 17:44:27 +0100 > > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > > > On Thu, Feb 27, 2025 at 05:28:17PM +0100, David Jander wrote: > > > [...] > > > > +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. > > > > This looks interesting. I didn't know about guard(). Thanks. I see the > > benefits, but in some cases it also makes the locked region less clearly > > visible. While I agree that guard() in this particular place is nice, > > I'm hesitant to try and replace all mutex_lock()/_unlock() calls with guard(). > > Let me know if my assessment of the intended use of guard() is incorrect. > > I agree that guard() makes it harder for non-trivial functions to spot > the critical section. In my eyes this is outweight by not having to > unlock in all exit paths, but that might be subjective. Annother > downside of guard is that sparse doesn't understand it and reports > unbalanced locking. What I was referring to, and what I want to know is, is it okay to mix guard() with lock/unlock? I.e. Use guard() when there are multiple exit paths involved and revert back to simple lock/unlock if it is just to encase a handful of non-exiting operations? > > > > + 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, ...); > > > > Hmm... I don't yet really understand what you mean. I will have to study the > > involved code a bit more. > > The code that I'm convinced is correct is > https://lore.kernel.org/linux-pwm/00c9f1181dc351e1e6041ba6e41e4c30b12b6a27.1725635013.git.u.kleine-koenig@baylibre.com/ > > This isn't in mainline because there is some feedback I still have to > address, but I think it might serve as an example anyhow. Thanks. I will study this example. > > > > [...] > > > > + > > > > +static const struct class motion_class = { > > > > + .name = "motion", > > > > + .devnode = motion_devnode, > > > > > > IIRC it's recommended to not create new classes, but a bus. > > > > Interesting. I did some searching, and all I could find was that the chapter > > in driver-api/driver-model about classes magically vanished between versions > > 5.12 and 5.13. Does anyone know where I can find some information about this? > > Sorry if I'm being blind... > > Half knowledge on my end at best. I would hope that Greg knows some > details (which might even be "no, classes are fine"). I added him to Cc: > > > > [...] > > > > + 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? > > > > Good question. I suppose you mean that since mdev is devres-managed and > > device_create_with_groups() apparently isn't aware of that, so there is no > > internal lock somewhere that prevents read() or ioctl() being called while the > > devres code is freeing the memory of mdev? > > I'm not sure there is an issue, but when I developed the above mentioned > patch it helped me to test these possible races. Just open the sysfs > file, unbind the device (or unload the module) and only then start > reading (or writing). Will check this. Thanks. > > > > + 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). > > > > Oh, thanks for pointing that out... I wouldn't have searched for that > > information in that particular place tbh. ;-) > > > > I am tempted to add another __u32 reserved3 though instead. Better to have > > some leeway if something needs to be added in a backwards-compatible way later. > > Note that you don't need reserved fields at the end because in the > ioctl handler you know the size of the passed struct. So if the need to > add members to the struct arise, you can do that by checking for the > size. This is even more flexible because otherwise you can only add > fields that must be 0 when the old behaviour is intended. Most of the > time this is no problem. But only most. You are right. Thanks. > > > > +struct mot_speed_duration { > > > > + __u32 channel; > > > > + speed_raw_t speed; > > > > > > What is the unit here? > > > > Speed doesn't have a fixed unit in this case. Or rather, the unit is > > device-dependent. For a motor it could be rotations per second, micro-steps per > > second, etc... while for a linear actuator, it could be micrometers per second. > > > > Why no fixed unit? That's because in practice many devices (controllers) have > > their inherent base-unit, and it would get overly complicated if one needed to > > convert back and forth between that and some universal unit just for the sake > > of uniformity, and user-space most certainly expects the same unit as the > > hardware device it was initially designed for. So in this case it is a design > > decision to make user-space deal with unit-conversion if it is necessary to do > > so. > > Sad, so a userspace process still has to know some internal things about > the motor it drives. :-\ Unfortunately that is almost impossible to avoid entirely. You can replace one stepper motor driver with another that might have different micro-stepping subdivision, by looking at struct mot_capabilities.subdiv, but a simple brushed DC motor just isn't able to replace a stepper motor in all but the most trivial applications. I also think that burdening the kernel with all sorts of complicated math to model the mechanical conversion factors involved in anything that's connected to the motor drive shaft is overkill. As well as trying to emulate all missing capabilities from a motion device that is lacking that functionality natively. So just like in IIO you cannot just replace one ADC with any other, in LMC you also cannot replace any device with any other. That's why there is struct mot_capabilities and MOT_IOCTL_GET_CAPA. It enables user-space to optionally support different devices more easily. It is probably best used in conjunction with a LMC user-space library, although I don't want to rely on such a library for being able to use LMC. There is some middle ground here I guess... just like in IIO. One thing I could try to improve though, is to include some additional information in struct mot_capabilities that tells something more about the nature of the used units, just like the speed_conv and accel_conv constants do for time conversion. Something that can be placed in the device tree (possibly in a motor child-node connected to the motor-controller) that contains some conversion constant for distance. That way, if one were to (for example) replace a stepper motor with a BLDC motor + encoder in a new hardware revision, this constant could be used to make the units backwards compatible. As background information: A stepper motor controller counts distance in steps and/or micro-steps. There are mot_capabilities.subdiv micro-steps in each step. The amount of angle the actual motor shaft advances with each whole step depends on the motor construction and is often 200 steps per revolution (1.8 degrees), but can vary from 4 to 400 steps per revolution depending on the motor. So it is not only the controller that matters but also the type of motor. This suggests the need of motor sub-nodes in the device-tree if one wanted to extend the hardware knowledge further down from the motor driver. But then there are gear boxes, pulleys, etc... it's basically conversion factors all the way down. How many of them is sensible to bother the kernel with? Best regards,
On Thu, 6 Mar 2025 10:03:26 +0100 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Thu, Mar 06, 2025 at 09:20:13AM +0100, David Jander wrote: > > On Thu, 6 Mar 2025 08:18:46 +0100 > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > > > On Thu, Mar 06, 2025 at 12:21:22AM +0100, Uwe Kleine-König wrote: > > > > Hello David, > > > > > > > > On Wed, Mar 05, 2025 at 04:40:45PM +0100, David Jander wrote: > > > > > On Fri, 28 Feb 2025 17:44:27 +0100 > > > > > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > > > > > > On Thu, Feb 27, 2025 at 05:28:17PM +0100, David Jander wrote: > > > > > > [...] > > > > > > > +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. > > > > > > > > > > This looks interesting. I didn't know about guard(). Thanks. I see the > > > > > benefits, but in some cases it also makes the locked region less clearly > > > > > visible. While I agree that guard() in this particular place is nice, > > > > > I'm hesitant to try and replace all mutex_lock()/_unlock() calls with guard(). > > > > > Let me know if my assessment of the intended use of guard() is incorrect. > > > > > > > > I agree that guard() makes it harder for non-trivial functions to spot > > > > the critical section. In my eyes this is outweight by not having to > > > > unlock in all exit paths, but that might be subjective. Annother > > > > downside of guard is that sparse doesn't understand it and reports > > > > unbalanced locking. > > > > > > > > > > > + 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, ...); > > > > > > > > > > Hmm... I don't yet really understand what you mean. I will have to study the > > > > > involved code a bit more. > > > > > > > > The code that I'm convinced is correct is > > > > https://lore.kernel.org/linux-pwm/00c9f1181dc351e1e6041ba6e41e4c30b12b6a27.1725635013.git.u.kleine-koenig@baylibre.com/ > > > > > > > > This isn't in mainline because there is some feedback I still have to > > > > address, but I think it might serve as an example anyhow. > > > > > > > > > > > [...] > > > > > > > + > > > > > > > +static const struct class motion_class = { > > > > > > > + .name = "motion", > > > > > > > + .devnode = motion_devnode, > > > > > > > > > > > > IIRC it's recommended to not create new classes, but a bus. > > > > > > > > > > Interesting. I did some searching, and all I could find was that the chapter > > > > > in driver-api/driver-model about classes magically vanished between versions > > > > > 5.12 and 5.13. Does anyone know where I can find some information about this? > > > > > Sorry if I'm being blind... > > > > > > > > Half knowledge on my end at best. I would hope that Greg knows some > > > > details (which might even be "no, classes are fine"). I added him to Cc: > > > > > > A class is there for when you have a common api that devices of > > > different types can talk to userspace (i.e. the UAPI is common, not the > > > hardware type). Things like input devices, tty, disks, etc. A bus is > > > there to be able to write different drivers to bind to for that hardware > > > bus type (pci, usb, i2c, platform, etc.) > > > > > > So you need both, a bus to talk to the hardware, and a class to talk to > > > userspace in a common way (ignore the fact that we can also talk to > > > hardware directly from userspace like raw USB or i2c or PCI config > > > space, that's all bus-specific stuff). > > > > Thanks for chiming in. Let me see if I understand this correctly: In this > > case, I have a UAPI that is common to different types of motion control > > devices. So I need a class. check. > > Correct. > > > Do I need a bus? If one can conceive other drivers or kernel parts that talk to > > motion drivers, I would need a bus. If that doesn't make sense, I don't. Right? > > Correct. > > > I actually can think of a new motion device that acts as an aggregator of > > several single-channel motion devices into a single "virtual" multi-channel > > device... so do I need also a bus? I suppose...? > > Nope, that should just be another class driver. Think about how input > does this, some input /dev/ nodes are the sum of ALL input /dev/ nodes > together, while others are just for individual input devices. Understood. Thanks! > > Then the question remains: why did the chapter about classes vanish? > > What are you specifically referring to? I don't remember deleting any > documentation, did files move around somehow and the links not get > updated? This: https://www.kernel.org/doc/html/v5.12/driver-api/driver-model/index.html vs this: https://www.kernel.org/doc/html/v5.13/driver-api/driver-model/index.html Maybe it moved somewhere else, but I can't find it... I'd have to git bisect or git blame between the two releases maybe. Best regards,
On Thu, Mar 06, 2025 at 10:34:02AM +0100, David Jander wrote: > On Thu, 6 Mar 2025 10:03:26 +0100 > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > On Thu, Mar 06, 2025 at 09:20:13AM +0100, David Jander wrote: > > > On Thu, 6 Mar 2025 08:18:46 +0100 > > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > > > > > On Thu, Mar 06, 2025 at 12:21:22AM +0100, Uwe Kleine-König wrote: > > > > > Hello David, > > > > > > > > > > On Wed, Mar 05, 2025 at 04:40:45PM +0100, David Jander wrote: > > > > > > On Fri, 28 Feb 2025 17:44:27 +0100 > > > > > > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > > > > > > > On Thu, Feb 27, 2025 at 05:28:17PM +0100, David Jander wrote: > > > > > > > [...] > > > > > > > > +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. > > > > > > > > > > > > This looks interesting. I didn't know about guard(). Thanks. I see the > > > > > > benefits, but in some cases it also makes the locked region less clearly > > > > > > visible. While I agree that guard() in this particular place is nice, > > > > > > I'm hesitant to try and replace all mutex_lock()/_unlock() calls with guard(). > > > > > > Let me know if my assessment of the intended use of guard() is incorrect. > > > > > > > > > > I agree that guard() makes it harder for non-trivial functions to spot > > > > > the critical section. In my eyes this is outweight by not having to > > > > > unlock in all exit paths, but that might be subjective. Annother > > > > > downside of guard is that sparse doesn't understand it and reports > > > > > unbalanced locking. > > > > > > > > > > > > > + 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, ...); > > > > > > > > > > > > Hmm... I don't yet really understand what you mean. I will have to study the > > > > > > involved code a bit more. > > > > > > > > > > The code that I'm convinced is correct is > > > > > https://lore.kernel.org/linux-pwm/00c9f1181dc351e1e6041ba6e41e4c30b12b6a27.1725635013.git.u.kleine-koenig@baylibre.com/ > > > > > > > > > > This isn't in mainline because there is some feedback I still have to > > > > > address, but I think it might serve as an example anyhow. > > > > > > > > > > > > > [...] > > > > > > > > + > > > > > > > > +static const struct class motion_class = { > > > > > > > > + .name = "motion", > > > > > > > > + .devnode = motion_devnode, > > > > > > > > > > > > > > IIRC it's recommended to not create new classes, but a bus. > > > > > > > > > > > > Interesting. I did some searching, and all I could find was that the chapter > > > > > > in driver-api/driver-model about classes magically vanished between versions > > > > > > 5.12 and 5.13. Does anyone know where I can find some information about this? > > > > > > Sorry if I'm being blind... > > > > > > > > > > Half knowledge on my end at best. I would hope that Greg knows some > > > > > details (which might even be "no, classes are fine"). I added him to Cc: > > > > > > > > A class is there for when you have a common api that devices of > > > > different types can talk to userspace (i.e. the UAPI is common, not the > > > > hardware type). Things like input devices, tty, disks, etc. A bus is > > > > there to be able to write different drivers to bind to for that hardware > > > > bus type (pci, usb, i2c, platform, etc.) > > > > > > > > So you need both, a bus to talk to the hardware, and a class to talk to > > > > userspace in a common way (ignore the fact that we can also talk to > > > > hardware directly from userspace like raw USB or i2c or PCI config > > > > space, that's all bus-specific stuff). > > > > > > Thanks for chiming in. Let me see if I understand this correctly: In this > > > case, I have a UAPI that is common to different types of motion control > > > devices. So I need a class. check. > > > > Correct. > > > > > Do I need a bus? If one can conceive other drivers or kernel parts that talk to > > > motion drivers, I would need a bus. If that doesn't make sense, I don't. Right? > > > > Correct. > > > > > I actually can think of a new motion device that acts as an aggregator of > > > several single-channel motion devices into a single "virtual" multi-channel > > > device... so do I need also a bus? I suppose...? > > > > Nope, that should just be another class driver. Think about how input > > does this, some input /dev/ nodes are the sum of ALL input /dev/ nodes > > together, while others are just for individual input devices. > > Understood. Thanks! > > > > Then the question remains: why did the chapter about classes vanish? > > > > What are you specifically referring to? I don't remember deleting any > > documentation, did files move around somehow and the links not get > > updated? > > This: > https://www.kernel.org/doc/html/v5.12/driver-api/driver-model/index.html > > vs this: > https://www.kernel.org/doc/html/v5.13/driver-api/driver-model/index.html > > Maybe it moved somewhere else, but I can't find it... I'd have to git bisect > or git blame between the two releases maybe. Ah, this was removed in: 1364c6787525 ("docs: driver-model: Remove obsolete device class documentation") as the information there was totally incorrect, since the 2.5.69 kernel release. "device classes" aren't a thing, "classes" are a thing :) thanks, greg k-h
On Thu, 6 Mar 2025 14:39:16 +0100 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Thu, Mar 06, 2025 at 10:34:02AM +0100, David Jander wrote: > > On Thu, 6 Mar 2025 10:03:26 +0100 > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > > > On Thu, Mar 06, 2025 at 09:20:13AM +0100, David Jander wrote: > > > > On Thu, 6 Mar 2025 08:18:46 +0100 > > > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > > > > > > > On Thu, Mar 06, 2025 at 12:21:22AM +0100, Uwe Kleine-König wrote: > > > > > > Hello David, > > > > > > > > > > > > On Wed, Mar 05, 2025 at 04:40:45PM +0100, David Jander wrote: > > > > > > > On Fri, 28 Feb 2025 17:44:27 +0100 > > > > > > > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > > > > > > > > On Thu, Feb 27, 2025 at 05:28:17PM +0100, David Jander wrote: > > > > > > > > [...] > > > > > > > > > +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. > > > > > > > > > > > > > > This looks interesting. I didn't know about guard(). Thanks. I see the > > > > > > > benefits, but in some cases it also makes the locked region less clearly > > > > > > > visible. While I agree that guard() in this particular place is nice, > > > > > > > I'm hesitant to try and replace all mutex_lock()/_unlock() calls with guard(). > > > > > > > Let me know if my assessment of the intended use of guard() is incorrect. > > > > > > > > > > > > I agree that guard() makes it harder for non-trivial functions to spot > > > > > > the critical section. In my eyes this is outweight by not having to > > > > > > unlock in all exit paths, but that might be subjective. Annother > > > > > > downside of guard is that sparse doesn't understand it and reports > > > > > > unbalanced locking. > > > > > > > > > > > > > > > + 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, ...); > > > > > > > > > > > > > > Hmm... I don't yet really understand what you mean. I will have to study the > > > > > > > involved code a bit more. > > > > > > > > > > > > The code that I'm convinced is correct is > > > > > > https://lore.kernel.org/linux-pwm/00c9f1181dc351e1e6041ba6e41e4c30b12b6a27.1725635013.git.u.kleine-koenig@baylibre.com/ > > > > > > > > > > > > This isn't in mainline because there is some feedback I still have to > > > > > > address, but I think it might serve as an example anyhow. > > > > > > > > > > > > > > > [...] > > > > > > > > > + > > > > > > > > > +static const struct class motion_class = { > > > > > > > > > + .name = "motion", > > > > > > > > > + .devnode = motion_devnode, > > > > > > > > > > > > > > > > IIRC it's recommended to not create new classes, but a bus. > > > > > > > > > > > > > > Interesting. I did some searching, and all I could find was that the chapter > > > > > > > in driver-api/driver-model about classes magically vanished between versions > > > > > > > 5.12 and 5.13. Does anyone know where I can find some information about this? > > > > > > > Sorry if I'm being blind... > > > > > > > > > > > > Half knowledge on my end at best. I would hope that Greg knows some > > > > > > details (which might even be "no, classes are fine"). I added him to Cc: > > > > > > > > > > A class is there for when you have a common api that devices of > > > > > different types can talk to userspace (i.e. the UAPI is common, not the > > > > > hardware type). Things like input devices, tty, disks, etc. A bus is > > > > > there to be able to write different drivers to bind to for that hardware > > > > > bus type (pci, usb, i2c, platform, etc.) > > > > > > > > > > So you need both, a bus to talk to the hardware, and a class to talk to > > > > > userspace in a common way (ignore the fact that we can also talk to > > > > > hardware directly from userspace like raw USB or i2c or PCI config > > > > > space, that's all bus-specific stuff). > > > > > > > > Thanks for chiming in. Let me see if I understand this correctly: In this > > > > case, I have a UAPI that is common to different types of motion control > > > > devices. So I need a class. check. > > > > > > Correct. > > > > > > > Do I need a bus? If one can conceive other drivers or kernel parts that talk to > > > > motion drivers, I would need a bus. If that doesn't make sense, I don't. Right? > > > > > > Correct. > > > > > > > I actually can think of a new motion device that acts as an aggregator of > > > > several single-channel motion devices into a single "virtual" multi-channel > > > > device... so do I need also a bus? I suppose...? > > > > > > Nope, that should just be another class driver. Think about how input > > > does this, some input /dev/ nodes are the sum of ALL input /dev/ nodes > > > together, while others are just for individual input devices. > > > > Understood. Thanks! > > > > > > Then the question remains: why did the chapter about classes vanish? > > > > > > What are you specifically referring to? I don't remember deleting any > > > documentation, did files move around somehow and the links not get > > > updated? > > > > This: > > https://www.kernel.org/doc/html/v5.12/driver-api/driver-model/index.html > > > > vs this: > > https://www.kernel.org/doc/html/v5.13/driver-api/driver-model/index.html > > > > Maybe it moved somewhere else, but I can't find it... I'd have to git bisect > > or git blame between the two releases maybe. > > Ah, this was removed in: > 1364c6787525 ("docs: driver-model: Remove obsolete device class documentation") > as the information there was totally incorrect, since the 2.5.69 kernel > release. "device classes" aren't a thing, "classes" are a thing :) Aha. Thanks for pointing this out. The sheer removal of this, combined with other indirect indications, such as /sys/class/gpio being replaced with /sys/bus/gpio in the new api, Uwe's comment, etc... derailed my interpretation. :-) Btw, sorry to ask here and now @Greg: I didn't CC you with this whole series while I probably should have... now I am tempted to move V2 of this series to staging, due to higher chances of potentially breaking UAPI changes during initial development, and in order to have a more flexible discussions over the UAPI of LMC in general. Is that advisable or should we better make sure that the version to get merged upstream (I hope it eventually will be) is set in stone? Best regards,
On Thu, Mar 06, 2025 at 03:25:29PM +0100, David Jander wrote: > On Thu, 6 Mar 2025 14:39:16 +0100 > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > On Thu, Mar 06, 2025 at 10:34:02AM +0100, David Jander wrote: > > > On Thu, 6 Mar 2025 10:03:26 +0100 > > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > > > > > On Thu, Mar 06, 2025 at 09:20:13AM +0100, David Jander wrote: > > > > > On Thu, 6 Mar 2025 08:18:46 +0100 > > > > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > > > > > > > > > On Thu, Mar 06, 2025 at 12:21:22AM +0100, Uwe Kleine-König wrote: > > > > > > > Hello David, > > > > > > > > > > > > > > On Wed, Mar 05, 2025 at 04:40:45PM +0100, David Jander wrote: > > > > > > > > On Fri, 28 Feb 2025 17:44:27 +0100 > > > > > > > > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > > > > > > > > > On Thu, Feb 27, 2025 at 05:28:17PM +0100, David Jander wrote: > > > > > > > > > [...] > > > > > > > > > > +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. > > > > > > > > > > > > > > > > This looks interesting. I didn't know about guard(). Thanks. I see the > > > > > > > > benefits, but in some cases it also makes the locked region less clearly > > > > > > > > visible. While I agree that guard() in this particular place is nice, > > > > > > > > I'm hesitant to try and replace all mutex_lock()/_unlock() calls with guard(). > > > > > > > > Let me know if my assessment of the intended use of guard() is incorrect. > > > > > > > > > > > > > > I agree that guard() makes it harder for non-trivial functions to spot > > > > > > > the critical section. In my eyes this is outweight by not having to > > > > > > > unlock in all exit paths, but that might be subjective. Annother > > > > > > > downside of guard is that sparse doesn't understand it and reports > > > > > > > unbalanced locking. > > > > > > > > > > > > > > > > > + 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, ...); > > > > > > > > > > > > > > > > Hmm... I don't yet really understand what you mean. I will have to study the > > > > > > > > involved code a bit more. > > > > > > > > > > > > > > The code that I'm convinced is correct is > > > > > > > https://lore.kernel.org/linux-pwm/00c9f1181dc351e1e6041ba6e41e4c30b12b6a27.1725635013.git.u.kleine-koenig@baylibre.com/ > > > > > > > > > > > > > > This isn't in mainline because there is some feedback I still have to > > > > > > > address, but I think it might serve as an example anyhow. > > > > > > > > > > > > > > > > > [...] > > > > > > > > > > + > > > > > > > > > > +static const struct class motion_class = { > > > > > > > > > > + .name = "motion", > > > > > > > > > > + .devnode = motion_devnode, > > > > > > > > > > > > > > > > > > IIRC it's recommended to not create new classes, but a bus. > > > > > > > > > > > > > > > > Interesting. I did some searching, and all I could find was that the chapter > > > > > > > > in driver-api/driver-model about classes magically vanished between versions > > > > > > > > 5.12 and 5.13. Does anyone know where I can find some information about this? > > > > > > > > Sorry if I'm being blind... > > > > > > > > > > > > > > Half knowledge on my end at best. I would hope that Greg knows some > > > > > > > details (which might even be "no, classes are fine"). I added him to Cc: > > > > > > > > > > > > A class is there for when you have a common api that devices of > > > > > > different types can talk to userspace (i.e. the UAPI is common, not the > > > > > > hardware type). Things like input devices, tty, disks, etc. A bus is > > > > > > there to be able to write different drivers to bind to for that hardware > > > > > > bus type (pci, usb, i2c, platform, etc.) > > > > > > > > > > > > So you need both, a bus to talk to the hardware, and a class to talk to > > > > > > userspace in a common way (ignore the fact that we can also talk to > > > > > > hardware directly from userspace like raw USB or i2c or PCI config > > > > > > space, that's all bus-specific stuff). > > > > > > > > > > Thanks for chiming in. Let me see if I understand this correctly: In this > > > > > case, I have a UAPI that is common to different types of motion control > > > > > devices. So I need a class. check. > > > > > > > > Correct. > > > > > > > > > Do I need a bus? If one can conceive other drivers or kernel parts that talk to > > > > > motion drivers, I would need a bus. If that doesn't make sense, I don't. Right? > > > > > > > > Correct. > > > > > > > > > I actually can think of a new motion device that acts as an aggregator of > > > > > several single-channel motion devices into a single "virtual" multi-channel > > > > > device... so do I need also a bus? I suppose...? > > > > > > > > Nope, that should just be another class driver. Think about how input > > > > does this, some input /dev/ nodes are the sum of ALL input /dev/ nodes > > > > together, while others are just for individual input devices. > > > > > > Understood. Thanks! > > > > > > > > Then the question remains: why did the chapter about classes vanish? > > > > > > > > What are you specifically referring to? I don't remember deleting any > > > > documentation, did files move around somehow and the links not get > > > > updated? > > > > > > This: > > > https://www.kernel.org/doc/html/v5.12/driver-api/driver-model/index.html > > > > > > vs this: > > > https://www.kernel.org/doc/html/v5.13/driver-api/driver-model/index.html > > > > > > Maybe it moved somewhere else, but I can't find it... I'd have to git bisect > > > or git blame between the two releases maybe. > > > > Ah, this was removed in: > > 1364c6787525 ("docs: driver-model: Remove obsolete device class documentation") > > as the information there was totally incorrect, since the 2.5.69 kernel > > release. "device classes" aren't a thing, "classes" are a thing :) > > Aha. Thanks for pointing this out. The sheer removal of this, combined with > other indirect indications, such as /sys/class/gpio being replaced with > /sys/bus/gpio in the new api, Uwe's comment, etc... derailed my interpretation. > :-) > > Btw, sorry to ask here and now @Greg: I didn't CC you with this whole series > while I probably should have... now I am tempted to move V2 of this series to > staging, due to higher chances of potentially breaking UAPI changes during > initial development, and in order to have a more flexible discussions over the > UAPI of LMC in general. Is that advisable or should we better make sure that > the version to get merged upstream (I hope it eventually will be) is set in > stone? Just because something is in drivers/staging/ does not mean you can break the user/kernel api, that is NOT what staging is for at all. Take the time to get this right, there's no rush here. Make sure userspace works well with what you have before committing to it. If you want to cc: me on the next series so I can review the driver-core interaction bits, I'll be glad to do so. thanks, greg k-h
On Thu, 6 Mar 2025 10:25:40 +0100 David Jander <david@protonic.nl> wrote: > On Thu, 6 Mar 2025 00:21:22 +0100 > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > > > Hello David, > > > > On Wed, Mar 05, 2025 at 04:40:45PM +0100, David Jander wrote: > > > On Fri, 28 Feb 2025 17:44:27 +0100 > > > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > > > > On Thu, Feb 27, 2025 at 05:28:17PM +0100, David Jander wrote: > > > > [...] > > > > > +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. > > > > > > This looks interesting. I didn't know about guard(). Thanks. I see the > > > benefits, but in some cases it also makes the locked region less clearly > > > visible. While I agree that guard() in this particular place is nice, > > > I'm hesitant to try and replace all mutex_lock()/_unlock() calls with guard(). > > > Let me know if my assessment of the intended use of guard() is incorrect. > > > > I agree that guard() makes it harder for non-trivial functions to spot > > the critical section. In my eyes this is outweight by not having to > > unlock in all exit paths, but that might be subjective. Annother > > downside of guard is that sparse doesn't understand it and reports > > unbalanced locking. > > What I was referring to, and what I want to know is, is it okay to mix guard() > with lock/unlock? I.e. Use guard() when there are multiple exit paths involved > and revert back to simple lock/unlock if it is just to encase a handful of > non-exiting operations? Mixing is fine. In some cases scoped_guard() can also make things clearer though at the cost of increased indent. > > > > Sad, so a userspace process still has to know some internal things about > > the motor it drives. :-\ > > Unfortunately that is almost impossible to avoid entirely. > You can replace one stepper motor driver with another that might have > different micro-stepping subdivision, by looking at struct > mot_capabilities.subdiv, but a simple brushed DC motor just isn't able to > replace a stepper motor in all but the most trivial applications. I also think > that burdening the kernel with all sorts of complicated math to model the > mechanical conversion factors involved in anything that's connected to the > motor drive shaft is overkill. As well as trying to emulate all missing > capabilities from a motion device that is lacking that functionality natively. > > So just like in IIO you cannot just replace one ADC with any other, in LMC you > also cannot replace any device with any other. > > That's why there is struct mot_capabilities and MOT_IOCTL_GET_CAPA. It enables > user-space to optionally support different devices more easily. It is probably > best used in conjunction with a LMC user-space library, although I don't want > to rely on such a library for being able to use LMC. There is some middle > ground here I guess... just like in IIO. > > One thing I could try to improve though, is to include some additional > information in struct mot_capabilities that tells something more about the > nature of the used units, just like the speed_conv and accel_conv constants do > for time conversion. Something that can be placed in the device tree (possibly > in a motor child-node connected to the motor-controller) that contains some > conversion constant for distance. That way, if one were to (for example) > replace a stepper motor with a BLDC motor + encoder in a new hardware > revision, this constant could be used to make the units backwards compatible. > > As background information: A stepper motor controller counts distance in steps > and/or micro-steps. There are mot_capabilities.subdiv micro-steps in each > step. The amount of angle the actual motor shaft advances with each whole step > depends on the motor construction and is often 200 steps per revolution (1.8 > degrees), but can vary from 4 to 400 steps per revolution depending on the > motor. So it is not only the controller that matters but also the type of > motor. This suggests the need of motor sub-nodes in the device-tree if one > wanted to extend the hardware knowledge further down from the motor driver. > But then there are gear boxes, pulleys, etc... it's basically conversion > factors all the way down. How many of them is sensible to bother the kernel > with? I'd have a motor description that is sufficient to be able to swap steppers between hardware versions and present sufficient info to userspace to allow a library to hide those differences. That description might well be of an aggregate device consisting of motor and whatever mechanics to get you to the point you care about (actuator motion). Hardest bit will be documenting 'where' in the system the DT is describing. It's not that heavily used but we do have analog front ends in IIO that provide a not dissimilar thing to the various potential mechanisms here. Jonathan > > Best regards, >
Hi Jonathan, On Sun, 9 Mar 2025 17:32:50 +0000 Jonathan Cameron <jic23@kernel.org> wrote: > On Thu, 6 Mar 2025 10:25:40 +0100 > David Jander <david@protonic.nl> wrote: > > > On Thu, 6 Mar 2025 00:21:22 +0100 > > Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote: > > > [...] > > > > > > Sad, so a userspace process still has to know some internal things about > > > the motor it drives. :-\ > > > > Unfortunately that is almost impossible to avoid entirely. > > You can replace one stepper motor driver with another that might have > > different micro-stepping subdivision, by looking at struct > > mot_capabilities.subdiv, but a simple brushed DC motor just isn't able to > > replace a stepper motor in all but the most trivial applications. I also think > > that burdening the kernel with all sorts of complicated math to model the > > mechanical conversion factors involved in anything that's connected to the > > motor drive shaft is overkill. As well as trying to emulate all missing > > capabilities from a motion device that is lacking that functionality natively. > > > > So just like in IIO you cannot just replace one ADC with any other, in LMC you > > also cannot replace any device with any other. > > > > That's why there is struct mot_capabilities and MOT_IOCTL_GET_CAPA. It enables > > user-space to optionally support different devices more easily. It is probably > > best used in conjunction with a LMC user-space library, although I don't want > > to rely on such a library for being able to use LMC. There is some middle > > ground here I guess... just like in IIO. > > > > One thing I could try to improve though, is to include some additional > > information in struct mot_capabilities that tells something more about the > > nature of the used units, just like the speed_conv and accel_conv constants do > > for time conversion. Something that can be placed in the device tree (possibly > > in a motor child-node connected to the motor-controller) that contains some > > conversion constant for distance. That way, if one were to (for example) > > replace a stepper motor with a BLDC motor + encoder in a new hardware > > revision, this constant could be used to make the units backwards compatible. > > > > As background information: A stepper motor controller counts distance in steps > > and/or micro-steps. There are mot_capabilities.subdiv micro-steps in each > > step. The amount of angle the actual motor shaft advances with each whole step > > depends on the motor construction and is often 200 steps per revolution (1.8 > > degrees), but can vary from 4 to 400 steps per revolution depending on the > > motor. So it is not only the controller that matters but also the type of > > motor. This suggests the need of motor sub-nodes in the device-tree if one > > wanted to extend the hardware knowledge further down from the motor driver. > > But then there are gear boxes, pulleys, etc... it's basically conversion > > factors all the way down. How many of them is sensible to bother the kernel > > with? > > I'd have a motor description that is sufficient to be able to swap steppers > between hardware versions and present sufficient info to userspace to allow > a library to hide those differences. That description might well be of > an aggregate device consisting of motor and whatever mechanics to get you > to the point you care about (actuator motion). Hardest bit will be documenting > 'where' in the system the DT is describing. Is it really the purpose of a DT to describe mechanical aspects of a machine? Aren't we stretching things here? In case this makes sense, it would need to be optional. Only the electronics are on the "board", the rest can vary beyond the scope of the DT: one can connect different motors to a stepper motor controller and often that's the purpose. Just as one can connect anything to a USB port. Sometimes the controller is single purpose, just as some USB ports that have a fixed device connected to it on the board, like a USB-ethernet interface for example, which also has a certain fixed phy hooked up to it, etc... If the purpose of the DT is to potentially go beyond the borders of a PCB (or stack of PCB's) and even beyond the realm of electronic parts into mechanical parts, wouldn't we need a way to describe a lot of things, like gearboxes, pulleys, etc...? Also, in the spirit of the DT, shouldn't that description be complete and independent of the software (or even OS) involved? If that's the case, I fear that "...to the point you care about..." is probably not enough? > It's not that heavily used but we do have analog front ends in IIO that > provide a not dissimilar thing to the various potential mechanisms here. AFE's are still part of the electronics and often also part of the same board, so there it does make more sense. I've used them. Don't get me wrong, if there is general agreement that information about mechanical things like a motor, gearbox or other mechanical transformation of motion should be contained in the DT, I'm fine with it. Maybe we should create an example to have something to talk about? Take for example the extruder of a 3D printer: Complete description of the mechanics, including the stepper motor of type 17HS4401 from a Chinese manufacturer called "songshine" and the extruder, which consists of a gear reduction and a hobbled shaft pressing the filament into the hot-end. The stepper motor does one full 360 degree turn per 200 whole steps. The extruder then pushes 2178 micrometer of filament into the hot-end per whole turn of the motor shaft. We will assume that the extruder itself is not a standard part that can be ordered and could not have a standardize compatible string. The motor... could. Or it could be a generic motor with some parameters in the DT, like this: &spi2 { ... stepper-controller3: tmc5240@0 { compatible = "adi,tmc5240"; reg = <3>; spi-max-frequency = <10000000>; interrupts-extended = <&gpiok 6 IRQ_TYPE_LEVEL_LOW>; clocks = <&clock_tmc5240>; motor0 { compatible = "bipolar-stepper-motor"; run-current-ma = <1300>; hold-current-ma = <260>; rated-voltage-mv = <3600>; induction-uh = <2800>; dc-resistance-mohm = <1500>; holding-torque-nmm = <400>; detent-torque-nmm = <22>; steps-per-turn = <200>; extruder0 { compatible = "rotation-to-linear"; distance-per-turn-um = <2178>; }; }; }; }; Or the motor could be a reusable part with its own entry in some "bipolar-stepper-motors.c" database under a compatible string: &spi2 { ... stepper-controller3: tmc5240@0 { compatible = "adi,tmc5240"; reg = <3>; spi-max-frequency = <10000000>; interrupts-extended = <&gpiok 6 IRQ_TYPE_LEVEL_LOW>; clocks = <&clock_tmc5240>; motor0 { compatible = "songshine,17HS4401"; extruder0 { compatible = "rotation-to-linear"; distance-per-turn-um = <2178>; }; }; }; }; Or one could just condense all the information of the motor and mechanics into one node with the needed properties to obtain the conversion factors the motion framework needs. But that would make the DT dependent on the software used, and AFAIK this is not the purpose of a DT, right? It might look like this: &spi2 { ... stepper-controller3: tmc5240@0 { compatible = "adi,tmc5240"; reg = <3>; spi-max-frequency = <10000000>; interrupts-extended = <&gpiok 6 IRQ_TYPE_LEVEL_LOW>; clocks = <&clock_tmc5240>; motor0 { compatible = "stepper-linear-actuator"; run-current-ma = <1300>; hold-current-ma = <260>; steps-per-turn = <200>; ... distance-per-turn-um = <2178>; }; }; }; I've left out properties like reg, #address-cells and #size-cells here for brevity. Let me know if you think any of this makes sense. Best regards,
diff --git a/MAINTAINERS b/MAINTAINERS index efee40ea589f..57267584166c 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -13418,6 +13418,14 @@ F: Documentation/litmus-tests/ F: Documentation/memory-barriers.txt F: tools/memory-model/ +LINUX MOTION CONTROL +M: David Jander <david@protonic.nl> +L: linux-kernel@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/motion/ +F: Documentation/motion/ +F: drivers/motion/ + LINUX-NEXT TREE M: Stephen Rothwell <sfr@canb.auug.org.au> L: linux-next@vger.kernel.org diff --git a/drivers/Kconfig b/drivers/Kconfig index 7bdad836fc62..6b3482187f38 100644 --- a/drivers/Kconfig +++ b/drivers/Kconfig @@ -245,4 +245,6 @@ source "drivers/cdx/Kconfig" source "drivers/dpll/Kconfig" +source "drivers/motion/Kconfig" + endmenu diff --git a/drivers/Makefile b/drivers/Makefile index 45d1c3e630f7..39476f2b5e55 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -195,3 +195,5 @@ obj-$(CONFIG_CDX_BUS) += cdx/ obj-$(CONFIG_DPLL) += dpll/ obj-$(CONFIG_S390) += s390/ + +obj-$(CONFIG_MOTION) += motion/ diff --git a/drivers/motion/Kconfig b/drivers/motion/Kconfig new file mode 100644 index 000000000000..085f9647b47b --- /dev/null +++ b/drivers/motion/Kconfig @@ -0,0 +1,19 @@ +# SPDX-License-Identifier: GPL-2.0 + +menuconfig MOTION + bool "Linux Motion Control support" + select IIO + help + The Linux Motion Control subsystem contains drivers for different + types of motion control hardware, like (stepper-)motor drivers and + linear actuators. + Say Y here if you want to chose motion control devices. + +if MOTION + +config MOTION_HELPERS + bool + depends on MOTION + +endif # MOTION + diff --git a/drivers/motion/Makefile b/drivers/motion/Makefile new file mode 100644 index 000000000000..ed912a8ed605 --- /dev/null +++ b/drivers/motion/Makefile @@ -0,0 +1,3 @@ +# SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_MOTION) += motion-core.o +obj-$(CONFIG_MOTION_HELPERS) += motion-helpers.o diff --git a/drivers/motion/motion-core.c b/drivers/motion/motion-core.c new file mode 100644 index 000000000000..2963f1859e8b --- /dev/null +++ b/drivers/motion/motion-core.c @@ -0,0 +1,823 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Motion Control Subsystem - Core + * + * Copyright (C) 2024 Protonic Holland + * David Jander <david@protonic.nl> + */ + +#include <asm-generic/bitops/builtin-fls.h> +#include <asm-generic/errno-base.h> +#include <linux/interrupt.h> +#include <linux/irqreturn.h> +#include <linux/container_of.h> +#include <linux/hrtimer_types.h> +#include <linux/gfp_types.h> +#include <linux/module.h> + +#include <linux/fs.h> +#include <linux/errno.h> +#include <linux/kernel.h> +#include <linux/major.h> +#include <linux/init.h> +#include <linux/device.h> +#include <linux/kmod.h> +#include <linux/motion.h> +#include <linux/poll.h> +#include <linux/ptrace.h> +#include <linux/ktime.h> +#include <linux/iio/trigger.h> +#include <linux/gpio/consumer.h> + +#include "motion-core.h" +#include "motion-helpers.h" +#include <linux/time.h> +#include <linux/uaccess.h> +#include <linux/string.h> +#include <linux/math64.h> +#include <linux/mutex.h> +#include <linux/math.h> +#include <linux/math64.h> + +#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; +} + +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); + + list_for_each_entry(iter, &motion_list, list) { + if (iter->minor != minor) + continue; + mdev = iter; + break; + } + + if (!mdev) { + err = -ENODEV; + goto fail; + } + + dev_info(mdev->dev, "MOTION: open %d\n", mdev->minor); + 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); + 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); + + return mask; +} + +static long motion_move_distance(struct motion_device *mdev, + channel_mask_t ch, speed_raw_t speed, pos_raw_t distance) +{ + ktime_t time; + u64 tmp; + u64 tmpmul = NSEC_PER_SEC; /* Convert speed (1/s) to time in nsec */ + + if (mdev->ops.move_distance) + return mdev->ops.move_distance(mdev, ch, speed, distance); + + if (!mdev->ops.move_timed) + return -EOPNOTSUPP; + + if (!speed) + return -EINVAL; + + /* + * Handling of potential integer overflows when converting distance + * to time duration without sacrificing too much precision: + * speed_conv_div and speed_conv_mul can be very large, yet the + * resulting quotient is most likely a lot smaller. If we do the + * multiplication first we retain the highest precision, but we need + * to be mindful of integer overflows, so we do one test to see if there + * are enough bits left to increase the precision further. + */ + tmp = ((u64)distance * mdev->capabilities.speed_conv_div); + if (tmp < (1ULL << 48)) { + tmpmul = NSEC_PER_MSEC; + tmp *= MSEC_PER_SEC; + } + tmp = div_u64(tmp, mdev->capabilities.speed_conv_mul); + tmp = div_u64(tmp, speed); + time = tmp * tmpmul; + return mdev->ops.move_timed(mdev, ch, speed, time); +} + +static long motion_move_timed(struct motion_device *mdev, channel_mask_t ch, + speed_raw_t speed, mot_time_t duration) +{ + ktime_t t; + + if (mdev->ops.move_timed) { + t = mot_time2ktime(duration); + return mdev->ops.move_timed(mdev, ch, speed, t); + } + + return -EOPNOTSUPP; +} + +static long motion_set_profile_locked(struct motion_device *mdev, + struct mot_profile *prof) +{ + long ret; + struct mot_profile *dst; + int i; + + lockdep_assert_held(&mdev->mutex); + + if ((prof->na > mdev->capabilities.max_apoints) || + (prof->nv > mdev->capabilities.max_vpoints)) + return -EINVAL; + + /* Check if used acceleration values are positive and non zero */ + for (i = 0; i < prof->na; i++) + if (prof->acc[i] <= 0) + return -EINVAL; + + if (!mdev->ops.validate_profile || !mdev->ops.set_profile) + return -EOPNOTSUPP; + + if (prof->index >= MOT_MAX_PROFILES) + return -EINVAL; + + ret = mdev->ops.validate_profile(mdev, prof); + if (ret) + return ret; + + dst = &mdev->profiles[prof->index]; + + *dst = *prof; + dst->index |= MOTION_PROFILE_VALID; + + return 0L; +} + +static long motion_get_profile_locked(struct motion_device *mdev, u32 index, + struct mot_profile *dst) +{ + struct mot_profile *src; + + lockdep_assert_held(&mdev->mutex); + + if (index >= MOT_MAX_PROFILES) + return -EINVAL; + + if (!(mdev->profiles[index].index & MOTION_PROFILE_VALID)) + return -EINVAL; + + src = &mdev->profiles[index]; + *dst = *src; + + return 0L; +} + +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; + 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 irqreturn_t motion_gpio_interrupt(int irq, void *dev_id) +{ + struct motion_gpio_input *gpio = dev_id; + struct motion_device *mdev = container_of(gpio, struct motion_device, + gpios[gpio->index]); + struct mot_event evt = {0}; + struct mot_status st; + int val = gpiod_get_raw_value(gpio->gpio); + channel_mask_t chmsk; + channel_mask_t chmsk_l = 0; + channel_mask_t chmsk_r = 0; + + dev_info(mdev->dev, "GPIO IRQ val=%d edge=%d\n", val, gpio->edge); + /* FIXME: This is racy and we shouldn't try to support shared IRQ! */ + if ((gpio->edge == MOT_EDGE_FALLING) && val) + return IRQ_NONE; + + if ((gpio->edge == MOT_EDGE_RISING) && !val) + return IRQ_NONE; + + evt.event = MOT_EVENT_INPUT; + evt.input_index = gpio->index; + evt.timestamp = ktime2mot_time(ktime_get()); + + mutex_lock(&mdev->mutex); + /* FIXME: It may be possible and desirable to obtain position and + * speed from multiple channels with one call to the driver. + */ + chmsk = gpio->chmask; + while (chmsk) { + unsigned int ch = ffs(chmsk) - 1; + + chmsk &= ~(1 << ch); + evt.channel = ch; + st.channel = ch; + mdev->ops.get_status(mdev, &st); + evt.speed = st.speed; + evt.position = st.position; + motion_report_event(mdev, &evt); + if (st.speed < 0) + chmsk_l |= (1 << ch); + else if (st.speed > 0) + chmsk_r |= (1 << ch); + } + + switch (gpio->function) { + case MOT_INP_FUNC_STOP_NEG: + if (chmsk_l) + mdev->ops.basic_stop(mdev, chmsk_l); + break; + case MOT_INP_FUNC_STOP_POS: + if (chmsk_r) + mdev->ops.basic_stop(mdev, chmsk_r); + break; + case MOT_INP_FUNC_STOP: + mdev->ops.basic_stop(mdev, gpio->chmask); + break; + case MOT_INP_FUNC_DECEL_NEG: + if (chmsk_l) + mdev->ops.motion_stop(mdev, chmsk_l, MOT_WHEN_IMMEDIATE); + break; + case MOT_INP_FUNC_DECEL_POS: + if (chmsk_r) + mdev->ops.motion_stop(mdev, chmsk_r, MOT_WHEN_IMMEDIATE); + break; + case MOT_INP_FUNC_DECEL: + mdev->ops.motion_stop(mdev, gpio->chmask, MOT_WHEN_IMMEDIATE); + break; + case MOT_INP_FUNC_START: + if (mdev->ops.external_trigger) + mdev->ops.external_trigger(mdev, gpio->index, + gpio->chmask); + break; + default: + break; + } + mutex_unlock(&mdev->mutex); + + return IRQ_HANDLED; +} + +static int motion_config_gpio(struct motion_device *mdev, int idx, + unsigned int func, unsigned int edge, channel_mask_t chmsk) +{ + struct motion_gpio_input *gpio = &mdev->gpios[idx]; + bool irq_claimed = false; + int irq = gpiod_to_irq(gpio->gpio); + int flags; + + if (gpio->function != MOT_INP_FUNC_NONE) { + if (func == MOT_INP_FUNC_NONE) + devm_free_irq(mdev->dev, irq, mdev); + irq_claimed = true; + } + gpio->chmask = chmsk; + gpio->function = func; + gpio->edge = edge; + if (!irq_claimed) { + if (edge == MOT_EDGE_FALLING) + flags = IRQF_TRIGGER_FALLING; + else + flags = IRQF_TRIGGER_RISING; + flags |= IRQF_SHARED | IRQF_ONESHOT; + dev_info(mdev->dev, "Claiming GPIO IRQ %d\n", irq); + return devm_request_threaded_irq(mdev->dev, irq, NULL, + motion_gpio_interrupt, flags, + dev_name(mdev->dev), gpio); + } + + return 0; +} + +static long motion_config_input_locked(struct motion_device *mdev, struct mot_input *inp) +{ + int idx; + + lockdep_assert_held(&mdev->mutex); + + if (!inp->external) + return mdev->ops.config_trigger(mdev, inp->index, inp->function, + inp->edge, inp->chmask); + + idx = inp->index; + idx -= mdev->capabilities.num_ext_triggers - mdev->num_gpios; + /* + * FIXME: idx is now the index of GPIO external trigger. + * Other types of external triggers are not yet supported. + */ + if ((idx >= mdev->num_gpios) || (idx < 0)) { + WARN_ONCE(true, "Input index unexpectedly out of range."); + return -EINVAL; + } + return motion_config_gpio(mdev, idx, inp->function, inp->edge, + inp->chmask); +} + +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; + 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; + if (spd.channel >= mdev->capabilities.num_channels) + return -EINVAL; + if (spd.distance && spd.duration) + return -EINVAL; + /* FIXME: Check reserved for zero! */ + mutex_lock(&mdev->mutex); + if (!spd.distance && !spd.duration) + ret = mdev->ops.basic_run(mdev, spd.channel, spd.speed); + else if (spd.distance) + ret = motion_move_distance(mdev, spd.channel, + spd.speed, spd.distance); + else + ret = motion_move_timed(mdev, spd.channel, spd.speed, + mot_time2ktime(spd.duration)); + mutex_unlock(&mdev->mutex); + break; + } + case MOT_IOCTL_BASIC_STOP: { + u32 ch; + + if (copy_from_user(&ch, argp, sizeof(ch))) + return -EFAULT; + /* Stop takes channel mask as only argument */ + if (fls(ch) > mdev->capabilities.num_channels) + return -EINVAL; + mutex_lock(&mdev->mutex); + ret = mdev->ops.basic_stop(mdev, ch); + mutex_unlock(&mdev->mutex); + break; + } + case MOT_IOCTL_GET_CAPA: + ret = copy_to_user(argp, &mdev->capabilities, sizeof(struct mot_capabilities)); + break; + case MOT_IOCTL_GET_STATUS: { + struct mot_status st; + + if (copy_from_user(&st, argp, sizeof(st))) + return -EFAULT; + if (st.channel >= mdev->capabilities.num_channels) + return -EINVAL; + if (!mdev->ops.get_status) + return -EINVAL; + mutex_lock(&mdev->mutex); + ret = mdev->ops.get_status(mdev, &st); + mutex_unlock(&mdev->mutex); + if (ret) + break; + ret = copy_to_user(argp, &st, sizeof(struct mot_status)); + break; + } + case MOT_IOCTL_SET_PROFILE: { + struct mot_profile prof; + + if (copy_from_user(&prof, argp, sizeof(prof))) + return -EFAULT; + mutex_lock(&mdev->mutex); + ret = motion_set_profile_locked(mdev, &prof); + mutex_unlock(&mdev->mutex); + break; + } + case MOT_IOCTL_GET_PROFILE: { + struct mot_profile prof; + + if (copy_from_user(&prof, argp, sizeof(prof))) + return -EFAULT; + mutex_lock(&mdev->mutex); + ret = motion_get_profile_locked(mdev, prof.index, &prof); + mutex_unlock(&mdev->mutex); + if (ret) + break; + ret = copy_to_user(argp, &prof, sizeof(prof)); + break; + } + case MOT_IOCTL_START: { + struct mot_start start; + + if (copy_from_user(&start, argp, sizeof(start))) + return -EFAULT; + mutex_lock(&mdev->mutex); + ret = motion_start_locked(mdev, &start); + mutex_unlock(&mdev->mutex); + break; + } + case MOT_IOCTL_STOP: { + struct mot_stop stop; + + if (copy_from_user(&stop, argp, sizeof(stop))) + return -EFAULT; + if (fls(stop.chmask) > mdev->capabilities.num_channels) + return -EINVAL; + if (stop.when >= MOT_WHEN_NUM_WHENS) + return -EINVAL; + if (!mdev->ops.motion_stop) + return -EINVAL; + mutex_lock(&mdev->mutex); + ret = mdev->ops.motion_stop(mdev, stop.chmask, stop.when); + mutex_unlock(&mdev->mutex); + break; + } + case MOT_IOCTL_CONFIG_INPUT: { + struct mot_input inp; + + if (copy_from_user(&inp, argp, sizeof(inp))) + return -EFAULT; + if (fls(inp.chmask) > mdev->capabilities.num_channels) + return -EINVAL; + if ((inp.external > 1) || (inp.function > MOT_INP_FUNC_NUM_FUNCS)) + return -EINVAL; + if (!inp.external && (inp.index >= mdev->capabilities.num_int_triggers)) + return -EINVAL; + if (inp.external && (inp.index >= mdev->capabilities.num_ext_triggers)) + return -EINVAL; + if (!inp.external && !mdev->ops.config_trigger) + return -EOPNOTSUPP; + mutex_lock(&mdev->mutex); + ret = motion_config_input_locked(mdev, &inp); + mutex_unlock(&mdev->mutex); + break; + } + default: + ret = -EINVAL; + break; + } + + return ret; +} + +static char *motion_devnode(const struct device *dev, umode_t *mode) +{ + const struct motion_device *mdev = dev_get_drvdata(dev); + + if (mode && mdev->mode) + *mode = mdev->mode; + if (mdev->nodename) + return kstrdup(mdev->nodename, GFP_KERNEL); + return NULL; +} + +static const struct class motion_class = { + .name = "motion", + .devnode = motion_devnode, +}; + +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); + 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); + +void motion_unregister_device(struct motion_device *mdev) +{ + struct iio_motion_trigger_info *trig_info; + + trig_info = iio_trigger_get_drvdata(mdev->iiotrig); + iio_trigger_unregister(mdev->iiotrig); + kfree(trig_info); + iio_trigger_free(mdev->iiotrig); + mutex_lock(&motion_mtx); + list_del(&mdev->list); + device_destroy(&motion_class, MKDEV(motion_major, mdev->minor)); + motion_minor_free(mdev->minor); + mdev->dev = NULL; /* Trigger chardev read abort */ + mutex_unlock(&motion_mtx); +} +EXPORT_SYMBOL(motion_unregister_device); + +/** + * motion_report_event - Report an event to the motion core. + * @mdev: The motion device reporting the event + * @evt: The event to be reported and queued. + * + * Drivers should call this function when there is a motion event, such as + * target reached or a (virtual-) stop triggered. This applies only to internal + * trigger inputs; external GPIO trigger events are handled by the core. + */ +void motion_report_event(struct motion_device *mdev, struct mot_event *evt) +{ + int ret; + + dev_info(mdev->dev, "Report event: %d\n", evt->event); + switch (evt->event) { + case MOT_EVENT_INPUT: + case MOT_EVENT_TARGET: + case MOT_EVENT_STOP: + ret = kfifo_put(&mdev->events, *evt); + if (ret) + wake_up_poll(&mdev->wait, EPOLLIN); + irq_work_queue(&mdev->iiowork); + break; + default: + break; + } + +} +EXPORT_SYMBOL(motion_report_event); + +static int __init motion_init(void) +{ + int err; + + err = class_register(&motion_class); + if (err) + return err; + + motion_major = register_chrdev(0, "motion", &motion_fops); + if (motion_major <= 0) { + err = -EIO; + goto fail; + } + return 0; + +fail: + pr_err("unable to get major number for motion devices\n"); + class_unregister(&motion_class); + return err; +} +subsys_initcall(motion_init); diff --git a/drivers/motion/motion-core.h b/drivers/motion/motion-core.h new file mode 100644 index 000000000000..92d0fc816265 --- /dev/null +++ b/drivers/motion/motion-core.h @@ -0,0 +1,172 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +#ifndef __MOTION_CORE_H__ +#define __MOTION_CORE_H__ + +#include <linux/major.h> +#include <linux/list.h> +#include <linux/types.h> +#include <linux/device.h> +#include <linux/kfifo.h> +#include <linux/irq_work.h> +#include <uapi/linux/motion.h> + +struct motion_device; + +/** + * struct motion_ops - Motion driver API + * @device_open: Called when the associated device is opened + * @device_release: Called when the associated device is closed + * @basic_run: Start a basic movement of channel @ch with speed @v. Speed is a + * signed value. Sign indicates direction. + * @basic_stop: Immediately stops all active movements of all channels set in + * channel mask @ch. + * @get_status: Get current speed and position (if supported) from the channel + * specified in st->channel. + * @move_distance: Start a movement just like basic_run(), but stop after + * reaching the specified distance. Optional. + * @move_timed: Start a movement just like basic_run(), but stop after a + * specified time. Optional. + * @validate_profile: Check if all parameters of a specified movement profile + * (acceleration/speed curve) is valid for this driver. Optional. + * @motion_distance: Start or prepare to start a movement following a specified + * motion profile until reaching the target distance. Optional. + * @motion_timed: Start or prepare to start a movement following a specified + * motion profile that takes exactly @t time. Optional. + * @motion_stop: Stop or prepare to stop a movement that was initiated with + * either motion_timed() or motion_distance() prematurely while following + * the deceleration segment of the profile the movement was started with. + * Optional. + * @config_trigger: Setup a trigger @index for a certaing function @func that + * applies to all channels set in channel mask @ch. Only applies to + * internal triggers. Optional. + * @external_trigger: Initiate a movement by external trigger on all channels + * set in channel mask @ch. Optional. + * + * Channel mask parameters of typo channel_mask_t are bitmasks that specify + * multiple channels the call applies to simultaneously. + * + * The parameter @when specifies one of the MOT_WHEN_* values defined in the + * motion UAPI. + * The parameter @func specifies one of the MOT_FUNC_* values defined in the + * motion UAPI. + * The parameter @edge can be either MOT_EDGE_FALLING or MOT_EDGE_RISING. + * The parameter @index either refers to the index of a motion profile, or the + * index of an internal trigger intput depending on the context. + * + * All function calls specified as "Optional" above need to be implemented only + * if the driver can support the required functionality. + */ +struct motion_ops { + int (*device_open)(struct motion_device *mdev); + int (*device_release)(struct motion_device *mdev); + int (*basic_run)(struct motion_device *mdev, unsigned int ch, s32 v); + int (*basic_stop)(struct motion_device *mdev, channel_mask_t ch); + int (*get_status)(struct motion_device *mdev, struct mot_status *st); + int (*move_distance)(struct motion_device *mdev, unsigned int ch, + s32 v, u32 d); + int (*move_timed)(struct motion_device *mdev, unsigned int ch, s32 v, + ktime_t t); + int (*validate_profile)(struct motion_device *mdev, + struct mot_profile *p); + int (*set_profile)(struct motion_device *mdev, struct mot_profile *p); + int (*motion_distance)(struct motion_device *mdev, unsigned int ch, + unsigned int index, s32 d, unsigned int when); + int (*motion_timed)(struct motion_device *mdev, unsigned int ch, + unsigned int index, unsigned int dir, ktime_t t, + unsigned int when); + int (*motion_stop)(struct motion_device *mdev, channel_mask_t ch, + unsigned int when); + int (*config_trigger)(struct motion_device *mdev, unsigned int index, + unsigned int func, unsigned int edge, channel_mask_t ch); + void (*external_trigger)(struct motion_device *mdev, unsigned int index, + channel_mask_t ch); +}; + +struct motion_gpio_input { + struct gpio_desc *gpio; + unsigned int function; + unsigned int edge; + unsigned int index; + channel_mask_t chmask; +}; + +/** + * struct motion_device - Represents a motion control subsystem device + * @ops: struct motion_ops implementing the functionality of the device. + * @parent: Parent struct device. This can be an underlying SPI/I2C device or + * a platform device, etc... This is mandatory. + * @dev: Newly created motion device associated with the denode. Filled in + * by motion_register_device(). + * @minor: The motion device minor number allocated by motion_register_device(). + * @list: Internal housekeeping. + * @groups: Attribute groups of the device. The driver can add an entry to the + * attributes table if required. Should be used for all run-time parameters + * of the underlying hardware, like current limits, virtual stop positions, + * etc... + * @nodename: Optional name of the devnode. Default NULL will use motionXX + * @mode: Optional mode for the devnode. + * @mutex: Mutex for serializing access to the device. Used by the core and + * locked during calls to the @ops. Should be locked by the driver if + * entered from other places, like interrupt threads. + * @read_mutex: Mutex used by the core for serializing read() calls to the + * device. + * @capabilities: struct mot_capabilities, describes the capabilities of the + * particular driver. + * @profiles: Statically allocated list of motion profiles. The core stores + * motion profiles supplied by user-space in this list. The @index + * parameter in @ops calls is an index into this list if applicable. + * @gpios: Statically allocated list of external trigger inputs associated with + * this device. These are specified in the fwnode. + * @num_gpios: Number of external GPIO trigger inputs parsed from the fwnode. + * @wait: poll() waitqueue for motion events to user-space. + * @events: KFIFO of motion events. + * @iiotrig: IIO trigger of motion events. + * @iiowork: The irq_work that dispatches the IIO trigger events. + * @helper_cookie: internal data for helper functions such as timed_speed + * helpers. + * + * Motion device drivers should (devm_)kzalloc this struct and fill in all + * required information (@ops, @parent and @capabilities) and then call + * motion_register_device() from their probe function. + * + * @parent should hold any drvdata for the driver if needed, and the drvdata + * struct should contain this struct motion_device as a member, so that it can + * be retrieved with container_of() macros. + */ +struct motion_device { + struct motion_ops ops; + struct device *parent; + struct device *dev; + int minor; + struct list_head list; + const struct attribute_group *groups[3]; + const char *nodename; + umode_t mode; + struct mutex mutex; + struct mutex read_mutex; + struct mot_capabilities capabilities; + struct mot_profile profiles[MOT_MAX_PROFILES]; + struct motion_gpio_input gpios[MOT_MAX_INPUTS]; + unsigned int num_gpios; + wait_queue_head_t wait; + DECLARE_KFIFO(events, struct mot_event, 16); + struct iio_trigger *iiotrig; + struct irq_work iiowork; + void *helper_cookie; +}; + +static inline ktime_t mot_time2ktime(mot_time_t t) +{ + return (ktime_t)t; +} + +static inline mot_time_t ktime2mot_time(ktime_t t) +{ + return (mot_time_t)t; +} + +int motion_register_device(struct motion_device *mdev); +void motion_unregister_device(struct motion_device *mdev); +void motion_report_event(struct motion_device *mdev, struct mot_event *evt); + +#endif /* __MOTION_CORE_H__ */ diff --git a/drivers/motion/motion-helpers.c b/drivers/motion/motion-helpers.c new file mode 100644 index 000000000000..b4c8dda84aa7 --- /dev/null +++ b/drivers/motion/motion-helpers.c @@ -0,0 +1,590 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Motion Control Subsystem - helper functions + * + * Copyright (C) 2024 Protonic Holland + * David Jander <david@protonic.nl> + */ + +#include <linux/fwnode.h> +#include <linux/property.h> +#include <linux/device.h> +#include <linux/gfp_types.h> +#include <linux/hrtimer_types.h> +#include <linux/export.h> +#include <linux/types.h> +#include <linux/motion.h> +#include "motion-helpers.h" +#include "motion-core.h" + +#define MOTION_TIMER_PERIOD (20 * NSEC_PER_MSEC) + +struct motion_timed_speed { + struct motion_device *mdev; + struct motion_timed_speed_ops *ops; + unsigned int speed_full_scale; + spinlock_t lock; + struct hrtimer timer; + unsigned int speed_actual; + unsigned int dir; + unsigned int speed_max; + unsigned int speed_start; + unsigned int speed_end; + unsigned int deceleration; + ktime_t taccel; + ktime_t tdecel; + ktime_t duration; + ktime_t ts_start; + unsigned int next_index; + unsigned int next_dir; + ktime_t next_duration; + unsigned int ext_trg_index; + unsigned int ext_trg_dir; + ktime_t ext_trg_duration; +}; + +static inline int __to_signed_speed(unsigned int dir, unsigned int speed) +{ + if (dir) + return speed; + return -speed; +} + +static int motion_timed_speed_open(struct motion_device *mdev) +{ + struct motion_timed_speed *mts = + (struct motion_timed_speed *)mdev->helper_cookie; + + if (mts->ops->startup) + mts->ops->startup(mdev); + + return 0; +} + +static int motion_timed_speed_release(struct motion_device *mdev) +{ + struct motion_timed_speed *mts = + (struct motion_timed_speed *)mdev->helper_cookie; + + dev_info(mdev->dev, "Release\n"); + hrtimer_cancel(&mts->timer); + if (mts->ops->powerdown) + mts->ops->powerdown(mdev); + else + mts->ops->set_speed(mdev, 0, 0); + mts->next_duration = 0; + mts->speed_actual = 0; + mts->dir = 0; + + return 0; +} + +static int motion_timed_speed_get_status(struct motion_device *mdev, struct mot_status *st) +{ + struct motion_timed_speed *mts = + (struct motion_timed_speed *)mdev->helper_cookie; + + st->speed = __to_signed_speed(mts->dir, mts->speed_actual); + st->position = 0; /* FIXME: Not yet supported */ + + return 0; +} + +static int motion_timed_speed_basic_run(struct motion_device *mdev, unsigned int ch, s32 v) +{ + struct motion_timed_speed *mts = + (struct motion_timed_speed *)mdev->helper_cookie; + int ret; + unsigned int dir = (v < 0) ? 0 : 1; + unsigned int dc = abs(v); + unsigned long flags; + + hrtimer_cancel(&mts->timer); + + spin_lock_irqsave(&mts->lock, flags); + ret = mts->ops->check_speed(mdev, dir, dc); + if (!ret) { + mts->speed_max = dc; + mts->ops->set_speed(mdev, dir, dc); + mts->speed_actual = dc; + mts->dir = dir; + } + spin_unlock_irqrestore(&mts->lock, flags); + + return ret; +} + +static int motion_timed_speed_basic_stop(struct motion_device *mdev, channel_mask_t ch) +{ + struct motion_timed_speed *mts = + (struct motion_timed_speed *)mdev->helper_cookie; + unsigned long flags; + + hrtimer_cancel(&mts->timer); + + spin_lock_irqsave(&mts->lock, flags); + mts->ops->set_speed(mdev, 0, 0); + mts->dir = 0; + mts->speed_actual = 0; + mts->speed_max = 0; + spin_unlock_irqrestore(&mts->lock, flags); + + return 0; +} + +static int motion_timed_speed_move_timed(struct motion_device *mdev, unsigned int ch, + s32 v, ktime_t t) +{ + struct motion_timed_speed *mts = + (struct motion_timed_speed *)mdev->helper_cookie; + unsigned long flags; + unsigned int dir = (v < 0) ? 0 : 1; + unsigned int dc = abs(v); + int ret; + + hrtimer_cancel(&mts->timer); + + spin_lock_irqsave(&mts->lock, flags); + ret = mts->ops->check_speed(mdev, dir, dc); + if (!ret) { + mts->ops->set_speed(mdev, dir, dc); + mts->speed_actual = dc; + mts->dir = dir; + mts->ts_start = ktime_get(); + mts->duration = t; + mts->speed_max = dc; + mts->deceleration = 0; + mts->taccel = 0; + mts->tdecel = 0; + mts->speed_start = 0; + mts->speed_end = 0; + } + spin_unlock_irqrestore(&mts->lock, flags); + if (ret) + return ret; + + hrtimer_start(&mts->timer, MOTION_TIMER_PERIOD, HRTIMER_MODE_REL_SOFT); + + return ret; +} + +static int motion_timed_speed_validate_profile(struct motion_device *mdev, + struct mot_profile *p) +{ + struct motion_timed_speed *mts = + (struct motion_timed_speed *)mdev->helper_cookie; + + if ((p->na != 2) || (p->nv != 3)) + return -EINVAL; + if ((p->acc[0] <= 0) || (p->acc[1] <= 0)) + return -EINVAL; + if ((p->vel[0] > p->vel[1]) || (p->vel[2] > p->vel[1])) + return -EINVAL; + if ((p->vel[0] < 0) || (p->vel[1] <= 0) || (p->vel[2] < 0)) + return -EINVAL; + if (p->vel[1] > mts->speed_full_scale) + return -EINVAL; + return 0; +} + +static int motion_timed_speed_set_profile(struct motion_device *mdev, + struct mot_profile *p) +{ + struct motion_timed_speed *mts = + (struct motion_timed_speed *)mdev->helper_cookie; + unsigned long flags; + unsigned int smd; + unsigned int esd; + + spin_lock_irqsave(&mts->lock, flags); + mts->speed_start = p->vel[0]; + mts->speed_max = p->vel[1]; + mts->speed_end = p->vel[2]; + mts->deceleration = p->acc[1]; + smd = mts->speed_max - mts->speed_start; + esd = mts->speed_max - mts->speed_end; + mts->taccel = div_u64((u64)smd * NSEC_PER_SEC, p->acc[0]); + mts->tdecel = div_u64((u64)esd * NSEC_PER_SEC, mts->deceleration); + spin_unlock_irqrestore(&mts->lock, flags); + + return 0; +} + +static void motion_timed_with_index(struct motion_device *mdev, + unsigned int index, int dir, ktime_t t) +{ + struct motion_timed_speed *mts = + (struct motion_timed_speed *)mdev->helper_cookie; + + motion_timed_speed_set_profile(mdev, &mdev->profiles[index]); + mts->ops->set_speed(mdev, dir, mts->speed_start); + mts->speed_actual = mts->speed_start; + mts->dir = dir; + mts->duration = t; + mts->ts_start = ktime_get(); +} + +static int calc_speed(struct motion_timed_speed *mts, ktime_t now, ktime_t trem) +{ + int smd = mts->speed_max - mts->speed_actual; + int dva = mts->speed_max - mts->speed_start; + int dvd = mts->speed_actual - mts->speed_end; + ktime_t tel = ktime_sub(now, mts->ts_start); + + if (trem <= 0) + return 0; + + mts->tdecel = mts->deceleration ? + div_u64((u64)dvd * NSEC_PER_SEC, mts->deceleration) : 0; + + if ((smd <= 0) && (ktime_compare(trem, mts->tdecel) > 0)) + return mts->speed_max; + + /* Due to (trem > 0), zerodivision can't happen here */ + if (ktime_compare(trem, mts->tdecel) < 0) + return mts->speed_end + div64_s64((dvd * trem), mts->tdecel); + + /* Due to (tel > 0) zerodivision can't happen here */ + if (ktime_compare(tel, mts->taccel) < 0) + return mts->speed_start + div64_s64((dva * tel), mts->taccel); + + return mts->speed_actual; +} + +static enum hrtimer_restart motion_timed_speed_timer(struct hrtimer *timer) +{ + struct motion_timed_speed *mts = container_of(timer, + struct motion_timed_speed, timer); + struct motion_device *mdev = mts->mdev; + struct mot_event evt = {0}; + unsigned long flags; + ktime_t now = ktime_get(); + ktime_t trem = ktime_sub(ktime_add(mts->ts_start, mts->duration), now); + int speed; + int ret = HRTIMER_RESTART; + + spin_lock_irqsave(&mts->lock, flags); + speed = calc_speed(mts, now, trem); + if (speed != mts->speed_actual) { + mts->ops->set_speed(mdev, mts->dir, speed); + mts->speed_actual = speed; + mts->dir = mts->dir; + } + spin_unlock_irqrestore(&mts->lock, flags); + if (trem <= 0) { + mutex_lock(&mdev->mutex); + if (mts->next_duration) { + motion_timed_with_index(mdev, mts->next_index, + mts->next_dir, mts->next_duration); + mts->next_duration = 0; + } else { + ret = HRTIMER_NORESTART; + } + evt.speed = __to_signed_speed(mts->dir, mts->speed_actual); + evt.timestamp = ktime2mot_time(now); + evt.event = MOT_EVENT_TARGET; + motion_report_event(mdev, &evt); + mutex_unlock(&mdev->mutex); + } + + if (ret == HRTIMER_RESTART) + hrtimer_add_expires_ns(timer, MOTION_TIMER_PERIOD); + + return ret; +} + +static int motion_timed_speed_motion_timed(struct motion_device *mdev, unsigned int ch, + unsigned int index, unsigned int dir, ktime_t t, + unsigned int when) +{ + struct motion_timed_speed *mts = + (struct motion_timed_speed *)mdev->helper_cookie; + int ret = 0; + + ret = mts->ops->check_speed(mdev, dir, 0); + if (ret) + return -EINVAL; + + switch (when) { + case MOT_WHEN_NEXT: + if (mts->next_duration) { + ret = -EAGAIN; + } else { + mts->next_duration = t; + mts->next_index = index; + mts->next_dir = dir; + } + break; + case MOT_WHEN_EXT_TRIGGER: + if (mts->ext_trg_duration) { + ret = -EAGAIN; + } else { + mts->ext_trg_duration = t; + mts->ext_trg_index = index; + mts->ext_trg_dir = dir; + } + break; + case MOT_WHEN_IMMEDIATE: + motion_timed_with_index(mdev, index, dir, t); + hrtimer_start(&mts->timer, MOTION_TIMER_PERIOD, + HRTIMER_MODE_REL_SOFT); + break; + default: + ret = -EINVAL; + break; + } + + return ret; +} + +static int motion_timed_speed_motion_stop(struct motion_device *mdev, channel_mask_t ch, + unsigned int when) +{ + struct motion_timed_speed *mts = + (struct motion_timed_speed *)mdev->helper_cookie; + unsigned long flags; + + if (when != MOT_WHEN_IMMEDIATE) + return -EINVAL; + + spin_lock_irqsave(&mts->lock, flags); + if (hrtimer_active(&mts->timer)) { + mts->duration = mts->tdecel; + mts->ts_start = ktime_get(); + } + spin_unlock_irqrestore(&mts->lock, flags); + + return 0; +} + +static void motion_timed_speed_ext_trigger(struct motion_device *mdev, unsigned int index, + channel_mask_t ch) +{ + struct motion_timed_speed *mts = + (struct motion_timed_speed *)mdev->helper_cookie; + + if (mts->ext_trg_duration) { + hrtimer_cancel(&mts->timer); + + motion_timed_with_index(mdev, mts->ext_trg_index, + mts->ext_trg_dir, mts->ext_trg_duration); + mts->ext_trg_duration = 0; + hrtimer_start(&mts->timer, MOTION_TIMER_PERIOD, + HRTIMER_MODE_REL_SOFT); + } +} + +static struct motion_ops motion_timed_speed_motion_ops = { + .device_open = motion_timed_speed_open, + .device_release = motion_timed_speed_release, + .get_status = motion_timed_speed_get_status, + .basic_run = motion_timed_speed_basic_run, + .basic_stop = motion_timed_speed_basic_stop, + .move_timed = motion_timed_speed_move_timed, + .validate_profile = motion_timed_speed_validate_profile, + .set_profile = motion_timed_speed_set_profile, + .motion_timed = motion_timed_speed_motion_timed, + .motion_stop = motion_timed_speed_motion_stop, + .external_trigger = motion_timed_speed_ext_trigger +}; + +/** + * motion_timed_speed_init - Initialize a simple timed-speed motion device + * @mdev: Motion device that shall be initialized + * @ops: API functions provided by driver + * @full_scale: The maximum integer value for "full speed" for this device + * + * Allows a motion control driver that only has a means of adjusting motor + * speed and optionally -direction to augment its functionality to support + * trapezoidal motion profiles. + * + * Caller should create a struct motion_device and, populate + * capabilities.type, capabilities.subdiv and optionally the scaling factors + * and then call this function, which will add mdev->ops and fill in the + * rest. It is responsibility of the driver to call motion_register_device() + * afterwards. + * + * Return: 0 in case of success or a negative errno. + */ +int motion_timed_speed_init(struct motion_device *mdev, + struct motion_timed_speed_ops *ops, unsigned int full_scale) +{ + struct motion_timed_speed *mts; + + mts = devm_kzalloc(mdev->parent, sizeof(struct motion_timed_speed), + GFP_KERNEL); + if (!mts) + return -ENOMEM; + + mts->ops = ops; + mts->mdev = mdev; + mts->speed_full_scale = full_scale; + mdev->ops = motion_timed_speed_motion_ops; + mdev->capabilities.features |= MOT_FEATURE_SPEED | MOT_FEATURE_ACCEL | + MOT_FEATURE_PROFILE; + mdev->capabilities.num_channels = 1; + mdev->capabilities.max_apoints = 2; + mdev->capabilities.max_vpoints = 3; + mdev->capabilities.num_int_triggers = 0; + mdev->capabilities.num_ext_triggers = 0; /* Filled in by core */ + mdev->capabilities.subdiv = 1; + mdev->helper_cookie = mts; + + spin_lock_init(&mts->lock); + hrtimer_init(&mts->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL_SOFT); + mts->timer.function = motion_timed_speed_timer; + + return 0; +} +EXPORT_SYMBOL(motion_timed_speed_init); + +/** + * motion_fwnode_get_capabilities - Get motion specific properties from fwnode + * @mdev: Motion device to populate + * @fwnode: fwnode handle to read properties from. + * + * Reads motion specific properties from @fwnode and populates @mdev + * capabilities. + */ +void motion_fwnode_get_capabilities(struct motion_device *mdev, + struct fwnode_handle *fwnode) +{ + unsigned int val, err; + + err = fwnode_property_read_u32(fwnode, "motion,speed-conv-mul", &val); + if (!err) + mdev->capabilities.speed_conv_mul = val; + err = fwnode_property_read_u32(fwnode, "motion,speed-conv-div", &val); + if (!err && val) + mdev->capabilities.speed_conv_div = val; + err = fwnode_property_read_u32(fwnode, "motion,acceleration-conv-mul", &val); + if (!err) + mdev->capabilities.accel_conv_mul = val; + err = fwnode_property_read_u32(fwnode, "motion,acceleration-conv-div", &val); + if (!err && val) + mdev->capabilities.accel_conv_div = val; +} +EXPORT_SYMBOL(motion_fwnode_get_capabilities); + +static inline int __d2t_vmax(u64 a, u64 d, u32 Vmax32, u64 Vs, u64 Ve, u64 Xt, + u64 t2, u64 *Vs2, u64 *Ve2, ktime_t *t) +{ + u64 Vmax = (u64)Vmax32; + u64 Vm2 = Vmax * Vmax32; + u64 dva = Vmax32 - Vs; + u64 dvd = Vmax32 - Ve; + u64 ta = div_u64(dva * MSEC_PER_SEC, (u32)a); + u64 td = div_u64(dvd * MSEC_PER_SEC, (u32)d); + u64 X1; + u64 X3; + u64 Xtv = div_u64(Vmax * t2, MSEC_PER_SEC); + u64 tms; + + *Vs2 = (u64)Vs * Vs; + *Ve2 = (u64)Ve * Ve; + X1 = div64_u64(Vm2 - *Vs2, a << 1); + X3 = div64_u64(Vm2 - *Ve2, d << 1); + + /* Check if we can reach Vmax. If not try again with new Vmax */ + if (Xt > (X1 + X3 + Xtv)) { + tms = ta + td; + tms += div_u64(MSEC_PER_SEC * (Xt - X1 - X3), Vmax32); + *t = ktime_add_ms(0, tms); + return 0; + } + + return -EAGAIN; +} + +/** + * motion_distance_to_time - Convert distance to time period + * @mdev: Motion device + * @index: The index of the motion profile to use + * @distance: The covered distance of the complete movement + * @t: Pointer to ktime_t result + * + * Converts the @distance of a movement using a motion (acceleration) profile + * specified by @index into a time interval this movement would take. + * + * The only supported profile type is trapezoidal (3 velocity points and 2 + * acceleration values), and it takes into account Tvmax and the case where + * Vmax cannot be reached because the distance is too short. + * + * Return: 0 on success and -ENOTSUPP if profile is not trapezoidal. + */ +long motion_distance_to_time(struct motion_device *mdev, + unsigned int index, int distance, ktime_t *t) +{ + struct mot_profile *p = &mdev->profiles[index]; + unsigned int Vs = p->vel[0]; + unsigned int Ve = p->vel[2]; + u64 Vmax; + u64 a = p->acc[0]; /* Has been checked to be non-zero */ + u64 d = p->acc[1]; /* Has been checked to be non-zero */ + u64 Xt = abs(distance); + u64 t2 = ktime_to_ms(p->tvmax); + u64 Ve2, Vs2, Bt, disc; + s64 ACt; + unsigned int bl; + + if ((p->na != 2) || (p->nv != 3)) + return -EOPNOTSUPP; + + if (!__d2t_vmax(a, d, p->vel[1], Vs, Ve, Xt, t2, &Vs2, &Ve2, t)) + return 0; + + /* + * We can't reach Vmax, so we need to determine Vmax that + * satisfies tvmax and distance, given a and d. + * For that we need to solve a quadratic equation in the form: + * + * 0 = Vm^2*(1/2a + 1/2d) + Vm * tvmax - Vs^2/2a - Ve^2/2d - Xt + * + * Doing this with only 64-bit integers will require scaling to + * adequate bit-lengths and an inevitable loss of precision. + * Precision is not critical since this function will be used + * to approximate a mechanical movement's distance by timing. + */ + bl = fls(a) + fls(d) + fls(Ve) + fls(Vs); + bl = max(0, (bl >> 1) - 16); + Bt = div_u64(a * d * t2, MSEC_PER_SEC); + + /* + * All terms are shifted left by bl bits *twice* (!) + * This will go into the square-root, so the result needs to be + * shifted right by bl bits only *once*. + */ + ACt = -((a*a) >> bl)*(Ve2 >> bl) - ((d*d) >> bl)*(Vs2 >> bl) - + ((a*d) >> bl)*((2*d*Xt + 2*a*Xt + Vs2 + Ve2) >> bl); + disc = (Bt >> bl) * (Bt >> bl) - ACt; + if (disc < 0) { + /* This should not be possible if (Ve, Vs) < Vm */ + WARN_ONCE(true, "Discriminator is negative!"); + disc = 0; + } + + /* + * We have all the parts of the quadratic formula, so we can + * calculate Vmax. There are some constraints we can take + * for granted here: + * - The term 4AC (ACt) is strictly negative, so the + * discriminant will always be bigger than Bt^2. + * - Due to this, the result of the square root will be + * bigger than Bt, which means there will always be one + * positive real solution for Vmax. + * - The dividend (a + d) cannot be zero, since a and d are + * both tested to be positive and non zero in + * motion_set_profile(). + */ + /* NOLINTNEXTLINE(clang-analyzer-core.DivideZero) */ + Vmax = div64_u64(-Bt + ((u64)int_sqrt64(disc) << bl), a + d); + Ve = min(Ve, Vmax); + Vs = min(Vs, Vmax); + dev_info(mdev->dev, "D2T: Vs=%u, Vmax=%llu, Ve=%u\n", Vs, Vmax, Ve); + + /* Try again with new Vmax. This time will always succeed. */ + __d2t_vmax(a, d, Vmax, Vs, Ve, Xt, t2, &Vs2, &Ve2, t); + + return 0; +} +EXPORT_SYMBOL(motion_distance_to_time); diff --git a/drivers/motion/motion-helpers.h b/drivers/motion/motion-helpers.h new file mode 100644 index 000000000000..0752390bf33a --- /dev/null +++ b/drivers/motion/motion-helpers.h @@ -0,0 +1,23 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +#ifndef __MOTION_HELPERS_H__ +#define __MOTION_HELPERS_H__ + +#include "motion-core.h" + +struct motion_timed_speed_ops { + void (*set_speed)(struct motion_device *mdev, unsigned int dir, + unsigned int speed); + int (*check_speed)(struct motion_device *mdev, unsigned int dir, + unsigned int speed); + void (*startup)(struct motion_device *mdev); + void (*powerdown)(struct motion_device *mdev); +}; + +int motion_timed_speed_init(struct motion_device *mdev, + struct motion_timed_speed_ops *ops, unsigned int full_scale); +void motion_fwnode_get_capabilities(struct motion_device *mdev, + struct fwnode_handle *fwnode); +long motion_distance_to_time(struct motion_device *mdev, + unsigned int index, int distance, ktime_t *t); + +#endif /* __MOTION_HELPERS_H__ */ diff --git a/include/uapi/linux/motion.h b/include/uapi/linux/motion.h new file mode 100644 index 000000000000..72a7e564114d --- /dev/null +++ b/include/uapi/linux/motion.h @@ -0,0 +1,229 @@ +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */ +#ifndef _UAPI_LINUX_MOTION_H +#define _UAPI_LINUX_MOTION_H + +#include <linux/const.h> +#include <linux/ioctl.h> +#include <linux/types.h> + +/* NOTE: Proposing to use IOC 'M' seq 0x80-0xc0 */ + +#define MOT_MAX_PROFILES 32 +#define MOT_MAX_SPEEDPTS 5 +#define MOT_MAX_ACCELPTS 6 +#define MOT_MAX_INPUTS 32 +#define MOT_UAPI_VERSION 1 + +/* Trigger inputs and End Stop functions */ +enum { + MOT_INP_FUNC_NONE = 0, /* No-op, used to clear previous functions */ + MOT_INP_FUNC_STOP, /* Stop immediately */ + MOT_INP_FUNC_STOP_POS, /* Stop immediately if moving forward */ + MOT_INP_FUNC_STOP_NEG, /* Stop immediately if moving backward */ + MOT_INP_FUNC_DECEL, /* Stop by deceleration curve */ + MOT_INP_FUNC_DECEL_POS, + MOT_INP_FUNC_DECEL_NEG, + MOT_INP_FUNC_START, /* Start motion with preset profile */ + MOT_INP_FUNC_SIGNAL, /* Only produce a signal (EPOLLIN) */ + MOT_INP_FUNC_NUM_FUNCS +}; + +/* Config trigger input edge */ +#define MOT_EDGE_RISING 0 +#define MOT_EDGE_FALLING 1 + +/* Start/Stop conditions */ +enum { + MOT_WHEN_IMMEDIATE = 0, + MOT_WHEN_INT_TRIGGER, /* On internal trigger input */ + MOT_WHEN_EXT_TRIGGER, /* On external trigger input */ + MOT_WHEN_NEXT, /* After preceding (current) motion ends */ + MOT_WHEN_NUM_WHENS +}; + +/* Event types */ +enum { + MOT_EVENT_NONE = 0, + MOT_EVENT_TARGET, /* Target position reached */ + MOT_EVENT_STOP, /* Endstop triggered */ + MOT_EVENT_INPUT, /* (Virtual-) input event */ + MOT_EVENT_STALL, /* Motor stalled */ + MOT_EVENT_ERROR, /* Other motor drive error */ + MOT_EVENT_NUM_EVENTS +}; + +#define MOT_DIRECTION_LEFT 0 +#define MOT_DIRECTION_RIGHT 1 + +/* Convention of signed position, speed and acceleration: + * movement of one channel is unidimensional, meaning position can be above or + * below the origin (positive or negative respecively). Consequently, given + * a positive position, a positive speed represents a movement further away + * from the origin (position 0), while a negative speed value represents a + * movement towards the origin. The opposite is valid when starting from a + * negative position value. + * Analogous to what speed does to position, is what acceletation does to speed: + * Given positive speed, positive acceleration increments the speed, and given + * "negative" speed, negative acceleration decrements the speed (increments its + * absolute value). + * For movement profiles, the convention is that profile (acceleration-, speed-) + * values are strictly positive. The direction of movement is solely determined + * by the relative position (i.e. "positive" or "negative" displacement). + */ +typedef __u32 channel_mask_t; +typedef __s32 pos_raw_t; +typedef __s32 speed_raw_t; +typedef __s32 accel_raw_t; +typedef __u32 torque_raw_t; +typedef __s64 mot_time_t; /* Try to mimic ktime_t, unit is nanoseconds. */ + +#define MOT_FEATURE_SPEED BIT(0) +#define MOT_FEATURE_ACCEL BIT(1) +#define MOT_FEATURE_ENCODER BIT(2) +#define MOT_FEATURE_PROFILE BIT(3) +#define MOT_FEATURE_VECTOR BIT(4) + +enum motion_device_type { + MOT_TYPE_DC_MOTOR, + MOT_TYPE_AC_MOTOR, + MOT_TYPE_STEPPER, + MOT_TYPE_BLDC, + MOT_TYPE_SRM, + MOT_TYPE_LINEAR, + MOT_TYPE_NUM_TYPES +}; + +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; +}; + +struct mot_speed_duration { + __u32 channel; + speed_raw_t speed; + mot_time_t duration; + pos_raw_t distance; + __u32 reserved[3]; +}; + +struct mot_status { + __u32 channel; + pos_raw_t position; + speed_raw_t speed; + __u32 reserved; +}; + +struct mot_input { + __u32 index; + __u8 external; + __u8 edge; + __u8 reserved[2]; + __u32 function; + channel_mask_t chmask; +}; + +/** + * struct mot_profile - Describe an acceleration profile + * @index: The index into the table of profiles to change + * @tvmax: Minimum time to stay at maximum velocity + * @tvzero: Minimum time to stay at zero velocity + * @na: Number of acceleration values + * @nv: Number of velocity values + * @acc: List of acceleration values. All values are absolute machine units. + * @vel: List of velocity values. All values are absolure machine units. + * + * 3 different types of acceleration curves are supported: + * 1. Trapezoidal - comprised of 3 velocity values and 2 acceleration values. + * Motion starts at start velocity (vel[0]) and accelerates with acc[0] + * linearly up to maximum velocity vel[1]. Maximum velocity is maintained + * for at least tvmax, before decelerating with acc[1] down to stop + * velocity vel[2]. After that velocity drops to zero and stays there for + * at least tvzero. + * + * 2. Dual slope - comprised of 4 velocity values and 4 acceleration values. + * Similar to trapezoidal profile above, but adding an intermediate + * velocity vel[1]. acc[0] is the first acceleration slope between + * vel[0] and vel[1]. acc[1] is the second acceleration slope between + * vel[1] and vel[2] (maximum velocity). acc[2] is the first deceleration + * slope between vel[2] and vel[1], and acc[3] is the final deceleration + * slope between vel[1] and vel[3]. + * + * 3. S-curve profile - Most advanced profile, often also called 8-point + * profile, comprised of 5 velocity values and 6 acceleration values. + */ +struct mot_profile { + __u32 index; + mot_time_t tvmax; + mot_time_t tvzero; + __u8 na; + __u8 nv; + __u8 reserved[2]; + accel_raw_t acc[MOT_MAX_ACCELPTS]; + speed_raw_t vel[MOT_MAX_SPEEDPTS]; +}; + +struct mot_start { + __u32 channel; + __u8 direction; + __u8 index; + __u8 when; + __u8 reserved1; + mot_time_t duration; + pos_raw_t distance; + __u32 reserved2; +}; + +struct mot_stop { + channel_mask_t chmask; + __u8 when; + __u8 reserved[3]; +}; + +struct mot_event { + __u32 channel; + __u8 event; + __u8 reserved1[3]; + pos_raw_t position; + speed_raw_t speed; + mot_time_t timestamp; + __u32 input_index; + __u32 reserved2; +}; + +/* API capabilities interrogation ioctls */ +#define MOT_IOCTL_APIVER _IO('M', 0x80) +#define MOT_IOCTL_GET_CAPA _IOR('M', 0x81, struct mot_capabilities) + +/* Basic motion control */ +#define MOT_IOCTL_GET_STATUS _IOWR('M', 0x82, struct mot_status) +#define MOT_IOCTL_BASIC_RUN _IOW('M', 0x83, struct mot_speed_duration) +#define MOT_IOCTL_BASIC_STOP _IOW('M', 0x84, __u32) + +/* Feedback control */ +#define MOT_IOCTL_CONFIG_INPUT _IOW('W', 0x85, struct mot_input) + +/* Profile control */ +#define MOT_IOCTL_SET_PROFILE _IOW('M', 0x86, struct mot_profile) +#define MOT_IOCTL_GET_PROFILE _IOWR('M', 0x87, struct mot_profile) +#define MOT_IOCTL_START _IOW('M', 0x88, struct mot_start) +#define MOT_IOCTL_STOP _IOW('M', 0x89, struct mot_stop) + +#endif /* _UAPI_LINUX_MOTION_H */
The Linux Motion Control subsystem (LMC) is a new driver subsystem for peripheral devices that control mechanical motion in some form or another. This could be different kinds of motors (stepper, DC, AC, SRM, BLDC...) or even linear actuators. The subsystem presents a unified UAPI for those devices, based on char devices with ioctl's. It can make use of regular gpio's to function as trigger inputs, like end-stops, fixed position- or motion start triggers and also generate events not only to user-space but also to the IIO subsystem in the form of IIO triggers. Signed-off-by: David Jander <david@protonic.nl> --- MAINTAINERS | 8 + drivers/Kconfig | 2 + drivers/Makefile | 2 + drivers/motion/Kconfig | 19 + drivers/motion/Makefile | 3 + drivers/motion/motion-core.c | 823 ++++++++++++++++++++++++++++++++ drivers/motion/motion-core.h | 172 +++++++ drivers/motion/motion-helpers.c | 590 +++++++++++++++++++++++ drivers/motion/motion-helpers.h | 23 + include/uapi/linux/motion.h | 229 +++++++++ 10 files changed, 1871 insertions(+) create mode 100644 drivers/motion/Kconfig create mode 100644 drivers/motion/Makefile create mode 100644 drivers/motion/motion-core.c create mode 100644 drivers/motion/motion-core.h create mode 100644 drivers/motion/motion-helpers.c create mode 100644 drivers/motion/motion-helpers.h create mode 100644 include/uapi/linux/motion.h