| Message ID | 1434034458-1968-1-git-send-email-simon@mungewell.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
On Thu, 2015-06-11 at 08:54 -0600, Simon Wood wrote: > [resent as I screwed up addressing... sorry] > > This patch is a RFC/POC for the idea of using the iio-subsystem as > the > 'proper' way to communicate the state of motion enabled controllers > to > the Linux internals. > > I have started with the hid-sony driver, with support for the > SixAxis's > 3 Accelerometers. Proper trigger/buffer support will follow shortly, > along > with support for the DS4 and PSMove controllers (which have a much > more > extensive set of motion hardware). > > Once the sensor data is available (over iio) I envision that it will > be > considerably easier to write motion tracking, flight control and AHRS > software in a consistant manner. Given that the software reading from the sensors will need to be root, how will you send the data from those devices to the applications? > Hopefully this will become the standard way of connecting > controllers, > motion controls and head mounted displays. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/06/15 15:54, Simon Wood wrote: > [resent as I screwed up addressing... sorry] > > This patch is a RFC/POC for the idea of using the iio-subsystem as the > 'proper' way to communicate the state of motion enabled controllers to > the Linux internals. Sounds good to me. > > I have started with the hid-sony driver, with support for the SixAxis's > 3 Accelerometers. Proper trigger/buffer support will follow shortly, along > with support for the DS4 and PSMove controllers (which have a much more > extensive set of motion hardware). Good stuff. > > Once the sensor data is available (over iio) I envision that it will be > considerably easier to write motion tracking, flight control and AHRS > software in a consistant manner. Cool :) Clearly as Bastien has raised, depending on use case some generic userspace support to expose these to non root users might be needed. > > Hopefully this will become the standard way of connecting controllers, > motion controls and head mounted displays. A few bits and bobs inline. Jonathan > --- > drivers/hid/hid-sony.c | 175 +++++++++++++++++++++++++++++++++++++++++++++++++ Hmm. This driver is already a substantial mash up of a number of different types of driver (as far as linux is concerned), with input, battery and led drivers. Might be worth considering a more formal MFD approach as it'll break the driver up into a number of sub components that will then sit in the various subsystems in a cleaner fashion. Just a thought! > 1 file changed, 175 insertions(+) > > diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c > index 6ca96ce..79afae6 100644 > --- a/drivers/hid/hid-sony.c > +++ b/drivers/hid/hid-sony.c > @@ -36,6 +36,7 @@ > #include <linux/list.h> > #include <linux/idr.h> > #include <linux/input/mt.h> > +#include <linux/iio/iio.h> > > #include "hid-ids.h" > > @@ -54,6 +55,7 @@ > DUALSHOCK4_CONTROLLER) > #define SONY_BATTERY_SUPPORT (SIXAXIS_CONTROLLER | DUALSHOCK4_CONTROLLER) > #define SONY_FF_SUPPORT (SIXAXIS_CONTROLLER | DUALSHOCK4_CONTROLLER) > +#define SONY_IIO_SUPPORT SIXAXIS_CONTROLLER > > #define MAX_LEDS 4 > > @@ -835,6 +837,23 @@ struct sony_sc { > __u8 led_delay_on[MAX_LEDS]; > __u8 led_delay_off[MAX_LEDS]; > __u8 led_count; > + > +#if IS_BUILTIN(CONFIG_IIO) || \ > + (IS_MODULE(CONFIG_IIO) && IS_MODULE(CONFIG_HID_SONY)) > + struct iio_dev *indio_dev; > + __u16 last_acc[3]; > + __u16 last_gyro[3]; > +}; > + > +enum sony_iio_axis { > + AXIS_X, > + AXIS_Y, > + AXIS_Z, > +}; > + > +struct sony_iio { > + struct sony_sc *sc; > +#endif > }; > > static __u8 *sixaxis_fixup(struct hid_device *hdev, __u8 *rdesc, > @@ -1047,6 +1066,12 @@ static int sony_raw_event(struct hid_device *hdev, struct hid_report *report, > swap(rd[45], rd[46]); > swap(rd[47], rd[48]); > > +#if IS_BUILTIN(CONFIG_IIO) || \ > + (IS_MODULE(CONFIG_IIO) && IS_MODULE(CONFIG_HID_SONY)) > + sc->last_acc[0] = (rd[42] << 8) + rd[41]; > + sc->last_acc[1] = (rd[44] << 8) + rd[43]; > + sc->last_acc[2] = (rd[46] << 8) + rd[45]; > +#endif > sixaxis_parse_report(sc, rd, size); > } else if (((sc->quirks & DUALSHOCK4_CONTROLLER_USB) && rd[0] == 0x01 && > size == 64) || ((sc->quirks & DUALSHOCK4_CONTROLLER_BT) > @@ -1769,6 +1794,136 @@ static void sony_battery_remove(struct sony_sc *sc) > sc->battery_desc.name = NULL; > } > > +#if IS_BUILTIN(CONFIG_IIO) || \ > + (IS_MODULE(CONFIG_IIO) && IS_MODULE(CONFIG_HID_SONY)) > + > +static int sony_iio_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, > + long mask) > +{ > + struct sony_iio *data = iio_priv(indio_dev); I'd be tempted to have the sc as the local variable here as there is nothing else in sony_iio at the moment and it'll save you a few characters on every reference to it below. > + int ret; > + u32 temp; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + switch (chan->type) { > + case IIO_ACCEL: > + *val = data->sc->last_acc[chan->scan_index]; You can use scan_index for this I suppose, but that's really meant for location in the buffer. We have iio_chan->address for this sort of lookup index. Now if they happen to be the same it doesn't really matter! Not sure whether it is better to use the last reading obtained, or wait for a new one to show up with a completion etc. > + return IIO_VAL_INT; > + case IIO_ANGL_VEL: > + *val = data->sc->last_gyro[chan->scan_index]; > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > + case IIO_CHAN_INFO_SCALE: > + switch (chan->type) { If the scale really is 1 then don't export it. Note the units would have to be in m/s^2 which seems unlikely though. I'm guessing this is a placeholder.. > + case IIO_ACCEL: > + *val = 1; > + return IIO_VAL_INT; > + case IIO_ANGL_VEL: > + *val = 1; > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > + case IIO_CHAN_INFO_OFFSET: > + switch (chan->type) { > + case IIO_ACCEL: Again, some unlikely values given after application of offset and scale the raw values are supposed to be in m/s^2 or radians/s > + *val = 512; > + return IIO_VAL_INT; > + case IIO_ANGL_VEL: > + *val = 512; > + return IIO_VAL_INT; > + default: > + return -EINVAL; > + } > + } > + > + return -EINVAL; > +} > + > +#define SONY_ACC_CHANNEL(_axis) { \ > + .type = IIO_ACCEL, \ > + .modified = 1, \ > + .channel2 = IIO_MOD_##_axis, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_OFFSET), \ > + .scan_index = AXIS_##_axis, \ > +} > + > +#define SONY_GYRO_CHANNEL(_axis) { \ > + .type = IIO_ANGL_VEL, \ > + .modified = 1, \ > + .channel2 = IIO_MOD_##_axis, \ > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ > + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_SCALE) | \ > + BIT(IIO_CHAN_INFO_OFFSET), \ > + .scan_index = AXIS_##_axis, \ > +} > + > +static const struct iio_chan_spec sony_sixaxis_channels[] = { > + SONY_ACC_CHANNEL(X), > + SONY_ACC_CHANNEL(Y), > + SONY_ACC_CHANNEL(Z), No gyro channels yet? Just to note, if the gyro frequency etc is different from the accelerometer (pretty common) then you'll want to register two IIO devices rather than just the one so that the control and buffers are separate. > +}; > + > +static const struct iio_info sony_iio_info = { > + .read_raw = &sony_iio_read_raw, > + .driver_module = THIS_MODULE, > +}; > + > +static int sony_iio_probe(struct sony_sc *sc) > +{ > + struct hid_device *hdev = sc->hdev; > + struct iio_dev *indio_dev; > + struct sony_iio *data; > + int ret; > + > + indio_dev = iio_device_alloc(sizeof(*data)); > + if (!indio_dev) > + return -ENOMEM; > + > + sc->indio_dev = indio_dev; > + data = iio_priv(indio_dev); > + data->sc = sc; > + > + indio_dev->dev.parent = &hdev->dev; > + indio_dev->name = dev_name(&hdev->dev); > + indio_dev->modes = INDIO_DIRECT_MODE; > + indio_dev->info = &sony_iio_info; > + indio_dev->channels = sony_sixaxis_channels; > + indio_dev->num_channels = 3; I'd use array_size for this to avoid accidental bugs if the number of elements changes in future. > + > + ret = iio_device_register(indio_dev); > + if (ret < 0) { > + hid_err(hdev, "Unable to register iio device\n"); > + goto error_iio_probe; > + } > + return 0; > + > +error_iio_probe: > + kfree(indio_dev); > + sc->indio_dev = NULL; > + return ret; > +} > + > +static void sony_iio_remove(struct sony_sc *sc) > +{ > + if (!sc->indio_dev) > + return; > + > + iio_device_unregister(sc->indio_dev); > + kfree(sc->indio_dev); > + sc->indio_dev = NULL; > +} > +#endif > + > /* > * If a controller is plugged in via USB while already connected via Bluetooth > * it will show up as two devices. A global list of connected controllers and > @@ -2073,6 +2228,15 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) > } > } > > +#if IS_BUILTIN(CONFIG_IIO) || \ > + (IS_MODULE(CONFIG_IIO) && IS_MODULE(CONFIG_HID_SONY)) > + if (sc->quirks & SONY_IIO_SUPPORT) { > + ret = sony_iio_probe(sc); > + if (ret < 0) > + goto err_stop; > + } > +#endif > + > if (sc->quirks & SONY_FF_SUPPORT) { > ret = sony_init_ff(sc); > if (ret < 0) > @@ -2087,6 +2251,11 @@ err_stop: > sony_leds_remove(sc); > if (sc->quirks & SONY_BATTERY_SUPPORT) > sony_battery_remove(sc); > +#if IS_BUILTIN(CONFIG_IIO) || \ > + (IS_MODULE(CONFIG_IIO) && IS_MODULE(CONFIG_HID_SONY)) > + if (sc->quirks & SONY_IIO_SUPPORT) > + sony_iio_remove(sc); > +#endif > sony_cancel_work_sync(sc); > kfree(sc->output_report_dmabuf); > sony_remove_dev_list(sc); > @@ -2107,6 +2276,12 @@ static void sony_remove(struct hid_device *hdev) > sony_battery_remove(sc); > } > > +#if IS_BUILTIN(CONFIG_IIO) || \ > + (IS_MODULE(CONFIG_IIO) && IS_MODULE(CONFIG_HID_SONY)) > + if (sc->quirks & SONY_IIO_SUPPORT) > + sony_iio_remove(sc); > +#endif > + > sony_cancel_work_sync(sc); > > kfree(sc->output_report_dmabuf); > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thank you for your comments, I'm just getting started with IIO so it's all good stuff... >> +++++++++++++++++++++++++++++++++++++++++++++++++ > Hmm. This driver is already a substantial mash up of a number of different > types of driver (as far as linux is concerned), with input, battery and > led > drivers. Might be worth considering a more formal MFD approach as it'll > break the driver up into a number of sub components that will then sit > in the various subsystems in a cleaner fashion. > Just a thought! Might be, but that sounds like more work ;-) If pushing some ideas around prompts that, it can't be a bad thing.... right? >> + case IIO_CHAN_INFO_SCALE: >> + switch (chan->type) { > If the scale really is 1 then don't export it. Note the units would > have to be in m/s^2 which seems unlikely though. I'm guessing this > is a placeholder.. Yes place holder, different controllers (SixAxis, DS4 and PSMove) have different values. In the far future I'd hope to use the per-device calibration stored on the PSMove. https://github.com/nitsch/moveonpc/wiki/Calibration-data >> +static const struct iio_chan_spec sony_sixaxis_channels[] = { >> + SONY_ACC_CHANNEL(X), >> + SONY_ACC_CHANNEL(Y), >> + SONY_ACC_CHANNEL(Z), > No gyro channels yet? > Just to note, if the gyro frequency etc is different from the > accelerometer > (pretty common) then you'll want to register two IIO devices rather than > just the one so that the control and buffers are separate. I have a vague memory that the SixAxis has a 1-ch gyro but this is not showing on hidraw (might be 'hidden' behind MultiTouch ID/Bug). The DS4 has Accel/Gyros, and the PSMove has Accel/Gyro/Mag. I didn't expose the mags over input/joystick axis as I didn't want to corrupt stream the PSMoveAPI (it needs to be re-ordered) and it's unlikely anyone would actually use via joystick. The PSMove's report actually contains 2 frames assumed to be 1/2 sample rate apart for the Accel/Gyro, but only one Mag reading. I have further advanced the patch to include reading via buffer, but I'm having trigger 'conceptual' problems getting my head around the HID device issuing an interrupt when a input report is received. Looking at iio_dummy_event and iios_sysfs for inspiration.... On the assumption that there will be multiple devices (either same type or with different HID drivers) all trying to issue triggers, we'd need to be a little careful. Is there a 'short-cut' we can use if a HID device is only required to trigger itself (and not other iio devices)? ie. not need true interrupt system. Simon. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 14/06/15 18:25, simon@mungewell.org wrote: > Thank you for your comments, I'm just getting started with IIO so it's all > good stuff... > Srinivas, cc'd you as a few queries came up about the hid-sensors driver. (it's got me thoroughly confused ;( >>> +++++++++++++++++++++++++++++++++++++++++++++++++ >> Hmm. This driver is already a substantial mash up of a number of different >> types of driver (as far as linux is concerned), with input, battery and >> led >> drivers. Might be worth considering a more formal MFD approach as it'll >> break the driver up into a number of sub components that will then sit >> in the various subsystems in a cleaner fashion. >> Just a thought! > > Might be, but that sounds like more work ;-) If pushing some ideas around > prompts that, it can't be a bad thing.... right? > >>> + case IIO_CHAN_INFO_SCALE: >>> + switch (chan->type) { >> If the scale really is 1 then don't export it. Note the units would >> have to be in m/s^2 which seems unlikely though. I'm guessing this >> is a placeholder.. > > Yes place holder, different controllers (SixAxis, DS4 and PSMove) have > different values. In the far future I'd hope to use the per-device > calibration stored on the PSMove. > > https://github.com/nitsch/moveonpc/wiki/Calibration-data > >>> +static const struct iio_chan_spec sony_sixaxis_channels[] = { >>> + SONY_ACC_CHANNEL(X), >>> + SONY_ACC_CHANNEL(Y), >>> + SONY_ACC_CHANNEL(Z), >> No gyro channels yet? >> Just to note, if the gyro frequency etc is different from the >> accelerometer >> (pretty common) then you'll want to register two IIO devices rather than >> just the one so that the control and buffers are separate. > > I have a vague memory that the SixAxis has a 1-ch gyro but this is not > showing on hidraw (might be 'hidden' behind MultiTouch ID/Bug). > > The DS4 has Accel/Gyros, and the PSMove has Accel/Gyro/Mag. I didn't > expose the mags over input/joystick axis as I didn't want to corrupt > stream the PSMoveAPI (it needs to be re-ordered) and it's unlikely anyone > would actually use via joystick. > > The PSMove's report actually contains 2 frames assumed to be 1/2 sample > rate apart for the Accel/Gyro, but only one Mag reading. > > > > I have further advanced the patch to include reading via buffer, but I'm > having trigger 'conceptual' problems getting my head around the HID device > issuing an interrupt when a input report is received. Looking at > iio_dummy_event and iios_sysfs for inspiration.... You can skip the triggers. It's not obligatory, you can push directly into a buffer. Triggers are nice for 'data ready' type signals (which is closest to what we have here) if you might want to hang other sensors off the timing (so read on demand ADCs etc. They aren't actually 'required' as such. Looking at the hid drivers I'm not entirely sure they are actually doing anything. (been a while since I looked at these in detail!). Srinivas, perhaps you could help out with what's going on there? I can't find an iio_trigger_poll call so the trigger is neither used by the hid sensors drivers, not usable by anything else as far as I can see.. Looks like it is used purely to get the device into the correct power state etc. If so that stuff should be in the buffer preenable / postdisable not the trigger stuff. I get the feeling that this might all be a work around for a perceived need for a trigger (particularly in userspace app examples?) as it predates the am355x driver where we absolutely said they weren't required and the userspace demo changes that were made to support that. > > On the assumption that there will be multiple devices (either same type or > with different HID drivers) all trying to issue triggers, we'd need to be > a little careful. > > Is there a 'short-cut' we can use if a HID device is only required to > trigger itself (and not other iio devices)? ie. not need true interrupt > system. Yes, you can use the 'validate_trigger' and 'validate_device' callbacks to reject either other devices trying to use the trigger, or your device trying to use a different one. Lots of drivers do this in the first place (as you point out it is a short cut to get things working) then if anyone cares relax the constraint later by making the true interrupt stuff work. > > Simon. > > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, 2015-06-14 at 18:57 +0100, Jonathan Cameron wrote: > On 14/06/15 18:25, simon@mungewell.org wrote: > > Thank you for your comments, I'm just getting started with IIO so it's all > > good stuff... > > > Srinivas, cc'd you as a few queries came up about the hid-sensors driver. > (it's got me thoroughly confused ;( > >>> +++++++++++++++++++++++++++++++++++++++++++++++++ > >> Hmm. This driver is already a substantial mash up of a number of different > >> types of driver (as far as linux is concerned), with input, battery and > >> led > >> drivers. Might be worth considering a more formal MFD approach as it'll > >> break the driver up into a number of sub components that will then sit > >> in the various subsystems in a cleaner fashion. > >> Just a thought! > > > > Might be, but that sounds like more work ;-) If pushing some ideas around > > prompts that, it can't be a bad thing.... right? > > > >>> + case IIO_CHAN_INFO_SCALE: > >>> + switch (chan->type) { > >> If the scale really is 1 then don't export it. Note the units would > >> have to be in m/s^2 which seems unlikely though. I'm guessing this > >> is a placeholder.. > > > > Yes place holder, different controllers (SixAxis, DS4 and PSMove) have > > different values. In the far future I'd hope to use the per-device > > calibration stored on the PSMove. > > > > https://github.com/nitsch/moveonpc/wiki/Calibration-data > > > >>> +static const struct iio_chan_spec sony_sixaxis_channels[] = { > >>> + SONY_ACC_CHANNEL(X), > >>> + SONY_ACC_CHANNEL(Y), > >>> + SONY_ACC_CHANNEL(Z), > >> No gyro channels yet? > >> Just to note, if the gyro frequency etc is different from the > >> accelerometer > >> (pretty common) then you'll want to register two IIO devices rather than > >> just the one so that the control and buffers are separate. > > > > I have a vague memory that the SixAxis has a 1-ch gyro but this is not > > showing on hidraw (might be 'hidden' behind MultiTouch ID/Bug). > > > > The DS4 has Accel/Gyros, and the PSMove has Accel/Gyro/Mag. I didn't > > expose the mags over input/joystick axis as I didn't want to corrupt > > stream the PSMoveAPI (it needs to be re-ordered) and it's unlikely anyone > > would actually use via joystick. > > > > The PSMove's report actually contains 2 frames assumed to be 1/2 sample > > rate apart for the Accel/Gyro, but only one Mag reading. > > > > > > > > I have further advanced the patch to include reading via buffer, but I'm > > having trigger 'conceptual' problems getting my head around the HID device > > issuing an interrupt when a input report is received. Looking at > > iio_dummy_event and iios_sysfs for inspiration.... > You can skip the triggers. It's not obligatory, you can push directly into a buffer. > Triggers are nice for 'data ready' type signals (which is closest to what we > have here) if you might want to hang other sensors off the timing (so read > on demand ADCs etc. They aren't actually 'required' as such. > > Looking at the hid drivers I'm not entirely sure they are actually doing anything. > (been a while since I looked at these in detail!). Srinivas, perhaps you could help > out with what's going on there? > > I can't find an iio_trigger_poll call so the trigger is neither used by the > hid sensors drivers, not usable by anything else as far as I can see.. > Looks like it is used purely to get the device into the correct power state > etc. If so that stuff should be in the buffer preenable / postdisable not > the trigger stuff. > > I get the feeling that this might all be a work around for a perceived need > for a trigger (particularly in userspace app examples?) as it predates > the am355x driver where we absolutely said they weren't required and the > userspace demo changes that were made to support that. > > The user space was developed using the iio user space demo. We could have done with buffer pre/post enable. But triggers provided path for enhancements. We are currently looking how camera buffer sample and accelerometer data can be tied together by using trigger buffers. Even one sensor data ready can trigger read on a companion senor on the hub. Thanks, Srinivas > > On the assumption that there will be multiple devices (either same type or > > with different HID drivers) all trying to issue triggers, we'd need to be > > a little careful. > > > > Is there a 'short-cut' we can use if a HID device is only required to > > trigger itself (and not other iio devices)? ie. not need true interrupt > > system. > Yes, you can use the 'validate_trigger' and 'validate_device' callbacks to > reject either other devices trying to use the trigger, or your device trying > to use a different one. Lots of drivers do this in the first place (as you > point out it is a short cut to get things working) then if anyone cares > relax the constraint later by making the true interrupt stuff work. > > > > > > Simon. > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> I have further advanced the patch to include reading via buffer, but I'm >> having trigger 'conceptual' problems getting my head around the HID >> device >> issuing an interrupt when a input report is received. Looking at >> iio_dummy_event and iios_sysfs for inspiration.... > You can skip the triggers. It's not obligatory, you can push directly > into a buffer. > Triggers are nice for 'data ready' type signals (which is closest to what > we > have here) if you might want to hang other sensors off the timing (so read > on demand ADCs etc. They aren't actually 'required' as such. After a bit of poking around I got the triggers working as well, I can self trigger and even trigger another (iio-dummy) device. However I couldn't trigger two devices at the same time from a single trigger, is this normal for IIO? Next is a little code clean-up and I'll post what I have for some more comments, Simon. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18/06/15 07:15, simon@mungewell.org wrote: > >>> I have further advanced the patch to include reading via buffer, but I'm >>> having trigger 'conceptual' problems getting my head around the HID >>> device >>> issuing an interrupt when a input report is received. Looking at >>> iio_dummy_event and iios_sysfs for inspiration.... >> You can skip the triggers. It's not obligatory, you can push directly >> into a buffer. >> Triggers are nice for 'data ready' type signals (which is closest to what >> we >> have here) if you might want to hang other sensors off the timing (so read >> on demand ADCs etc. They aren't actually 'required' as such. > > After a bit of poking around I got the triggers working as well, I can > self trigger and even trigger another (iio-dummy) device. > > However I couldn't trigger two devices at the same time from a single > trigger, is this normal for IIO? You should be able to. Defaults limit it to 2 per trigger, in Kconfig it is "Maximum number of consumers per trigger". Any chance that has the value 1 for your build? Otherwise, if you can do a bit of debugging to find out where things get stuck, that would be great. Can you bring up the buffers with the trigger attached or is it failing before that? After that it would just be a case of chasing in your trigger driver to see if it gets reenabled correctly after the first trigger. > > > Next is a little code clean-up and I'll post what I have for some more > comments, Cool. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in
On 15/06/15 19:14, Srinivas Pandruvada wrote: > On Sun, 2015-06-14 at 18:57 +0100, Jonathan Cameron wrote: >> On 14/06/15 18:25, simon@mungewell.org wrote: >>> Thank you for your comments, I'm just getting started with IIO so it's all >>> good stuff... >>> >> Srinivas, cc'd you as a few queries came up about the hid-sensors driver. >> (it's got me thoroughly confused ;( >>>>> +++++++++++++++++++++++++++++++++++++++++++++++++ >>>> Hmm. This driver is already a substantial mash up of a number of different >>>> types of driver (as far as linux is concerned), with input, battery and >>>> led >>>> drivers. Might be worth considering a more formal MFD approach as it'll >>>> break the driver up into a number of sub components that will then sit >>>> in the various subsystems in a cleaner fashion. >>>> Just a thought! >>> >>> Might be, but that sounds like more work ;-) If pushing some ideas around >>> prompts that, it can't be a bad thing.... right? >>> >>>>> + case IIO_CHAN_INFO_SCALE: >>>>> + switch (chan->type) { >>>> If the scale really is 1 then don't export it. Note the units would >>>> have to be in m/s^2 which seems unlikely though. I'm guessing this >>>> is a placeholder.. >>> >>> Yes place holder, different controllers (SixAxis, DS4 and PSMove) have >>> different values. In the far future I'd hope to use the per-device >>> calibration stored on the PSMove. >>> >>> https://github.com/nitsch/moveonpc/wiki/Calibration-data >>> >>>>> +static const struct iio_chan_spec sony_sixaxis_channels[] = { >>>>> + SONY_ACC_CHANNEL(X), >>>>> + SONY_ACC_CHANNEL(Y), >>>>> + SONY_ACC_CHANNEL(Z), >>>> No gyro channels yet? >>>> Just to note, if the gyro frequency etc is different from the >>>> accelerometer >>>> (pretty common) then you'll want to register two IIO devices rather than >>>> just the one so that the control and buffers are separate. >>> >>> I have a vague memory that the SixAxis has a 1-ch gyro but this is not >>> showing on hidraw (might be 'hidden' behind MultiTouch ID/Bug). >>> >>> The DS4 has Accel/Gyros, and the PSMove has Accel/Gyro/Mag. I didn't >>> expose the mags over input/joystick axis as I didn't want to corrupt >>> stream the PSMoveAPI (it needs to be re-ordered) and it's unlikely anyone >>> would actually use via joystick. >>> >>> The PSMove's report actually contains 2 frames assumed to be 1/2 sample >>> rate apart for the Accel/Gyro, but only one Mag reading. >>> >>> >>> >>> I have further advanced the patch to include reading via buffer, but I'm >>> having trigger 'conceptual' problems getting my head around the HID device >>> issuing an interrupt when a input report is received. Looking at >>> iio_dummy_event and iios_sysfs for inspiration.... >> You can skip the triggers. It's not obligatory, you can push directly into a buffer. >> Triggers are nice for 'data ready' type signals (which is closest to what we >> have here) if you might want to hang other sensors off the timing (so read >> on demand ADCs etc. They aren't actually 'required' as such. >> >> Looking at the hid drivers I'm not entirely sure they are actually doing anything. >> (been a while since I looked at these in detail!). Srinivas, perhaps you could help >> out with what's going on there? >> >> I can't find an iio_trigger_poll call so the trigger is neither used by the >> hid sensors drivers, not usable by anything else as far as I can see.. >> Looks like it is used purely to get the device into the correct power state >> etc. If so that stuff should be in the buffer preenable / postdisable not >> the trigger stuff. >> >> I get the feeling that this might all be a work around for a perceived need >> for a trigger (particularly in userspace app examples?) as it predates >> the am355x driver where we absolutely said they weren't required and the >> userspace demo changes that were made to support that. >>> > The user space was developed using the iio user space demo. We could > have done with buffer pre/post enable. But triggers provided path for > enhancements. Fair enough in principle, I was just getting lost on whether any actual triggers ever occurred. That would require iio_trigger_poll to be called somewhere. If this isn't happening and the trigger is just used as a kind of internal placeholder then that is fine as long as there are validate functions so no other drivers think they can use the triggers. In particular the trigger validate_device callback needs to prevent this. > We are currently looking how camera buffer sample and > accelerometer data can be tied together by using trigger buffers. Even > one sensor data ready can trigger read on a companion senor on the hub. > > Thanks, > Srinivas > >>> On the assumption that there will be multiple devices (either same type or >>> with different HID drivers) all trying to issue triggers, we'd need to be >>> a little careful. >>> >>> Is there a 'short-cut' we can use if a HID device is only required to >>> trigger itself (and not other iio devices)? ie. not need true interrupt >>> system. >> Yes, you can use the 'validate_trigger' and 'validate_device' callbacks to >> reject either other devices trying to use the trigger, or your device trying >> to use a different one. Lots of drivers do this in the first place (as you >> point out it is a short cut to get things working) then if anyone cares >> relax the constraint later by making the true interrupt stuff work. >> >> >>> >>> Simon. >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >> > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c index 6ca96ce..79afae6 100644 --- a/drivers/hid/hid-sony.c +++ b/drivers/hid/hid-sony.c @@ -36,6 +36,7 @@ #include <linux/list.h> #include <linux/idr.h> #include <linux/input/mt.h> +#include <linux/iio/iio.h> #include "hid-ids.h" @@ -54,6 +55,7 @@ DUALSHOCK4_CONTROLLER) #define SONY_BATTERY_SUPPORT (SIXAXIS_CONTROLLER | DUALSHOCK4_CONTROLLER) #define SONY_FF_SUPPORT (SIXAXIS_CONTROLLER | DUALSHOCK4_CONTROLLER) +#define SONY_IIO_SUPPORT SIXAXIS_CONTROLLER #define MAX_LEDS 4 @@ -835,6 +837,23 @@ struct sony_sc { __u8 led_delay_on[MAX_LEDS]; __u8 led_delay_off[MAX_LEDS]; __u8 led_count; + +#if IS_BUILTIN(CONFIG_IIO) || \ + (IS_MODULE(CONFIG_IIO) && IS_MODULE(CONFIG_HID_SONY)) + struct iio_dev *indio_dev; + __u16 last_acc[3]; + __u16 last_gyro[3]; +}; + +enum sony_iio_axis { + AXIS_X, + AXIS_Y, + AXIS_Z, +}; + +struct sony_iio { + struct sony_sc *sc; +#endif }; static __u8 *sixaxis_fixup(struct hid_device *hdev, __u8 *rdesc, @@ -1047,6 +1066,12 @@ static int sony_raw_event(struct hid_device *hdev, struct hid_report *report, swap(rd[45], rd[46]); swap(rd[47], rd[48]); +#if IS_BUILTIN(CONFIG_IIO) || \ + (IS_MODULE(CONFIG_IIO) && IS_MODULE(CONFIG_HID_SONY)) + sc->last_acc[0] = (rd[42] << 8) + rd[41]; + sc->last_acc[1] = (rd[44] << 8) + rd[43]; + sc->last_acc[2] = (rd[46] << 8) + rd[45]; +#endif sixaxis_parse_report(sc, rd, size); } else if (((sc->quirks & DUALSHOCK4_CONTROLLER_USB) && rd[0] == 0x01 && size == 64) || ((sc->quirks & DUALSHOCK4_CONTROLLER_BT) @@ -1769,6 +1794,136 @@ static void sony_battery_remove(struct sony_sc *sc) sc->battery_desc.name = NULL; } +#if IS_BUILTIN(CONFIG_IIO) || \ + (IS_MODULE(CONFIG_IIO) && IS_MODULE(CONFIG_HID_SONY)) + +static int sony_iio_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, + long mask) +{ + struct sony_iio *data = iio_priv(indio_dev); + int ret; + u32 temp; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + switch (chan->type) { + case IIO_ACCEL: + *val = data->sc->last_acc[chan->scan_index]; + return IIO_VAL_INT; + case IIO_ANGL_VEL: + *val = data->sc->last_gyro[chan->scan_index]; + return IIO_VAL_INT; + default: + return -EINVAL; + } + case IIO_CHAN_INFO_SCALE: + switch (chan->type) { + case IIO_ACCEL: + *val = 1; + return IIO_VAL_INT; + case IIO_ANGL_VEL: + *val = 1; + return IIO_VAL_INT; + default: + return -EINVAL; + } + case IIO_CHAN_INFO_OFFSET: + switch (chan->type) { + case IIO_ACCEL: + *val = 512; + return IIO_VAL_INT; + case IIO_ANGL_VEL: + *val = 512; + return IIO_VAL_INT; + default: + return -EINVAL; + } + } + + return -EINVAL; +} + +#define SONY_ACC_CHANNEL(_axis) { \ + .type = IIO_ACCEL, \ + .modified = 1, \ + .channel2 = IIO_MOD_##_axis, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_OFFSET), \ + .scan_index = AXIS_##_axis, \ +} + +#define SONY_GYRO_CHANNEL(_axis) { \ + .type = IIO_ANGL_VEL, \ + .modified = 1, \ + .channel2 = IIO_MOD_##_axis, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_SCALE) | \ + BIT(IIO_CHAN_INFO_OFFSET), \ + .scan_index = AXIS_##_axis, \ +} + +static const struct iio_chan_spec sony_sixaxis_channels[] = { + SONY_ACC_CHANNEL(X), + SONY_ACC_CHANNEL(Y), + SONY_ACC_CHANNEL(Z), +}; + +static const struct iio_info sony_iio_info = { + .read_raw = &sony_iio_read_raw, + .driver_module = THIS_MODULE, +}; + +static int sony_iio_probe(struct sony_sc *sc) +{ + struct hid_device *hdev = sc->hdev; + struct iio_dev *indio_dev; + struct sony_iio *data; + int ret; + + indio_dev = iio_device_alloc(sizeof(*data)); + if (!indio_dev) + return -ENOMEM; + + sc->indio_dev = indio_dev; + data = iio_priv(indio_dev); + data->sc = sc; + + indio_dev->dev.parent = &hdev->dev; + indio_dev->name = dev_name(&hdev->dev); + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->info = &sony_iio_info; + indio_dev->channels = sony_sixaxis_channels; + indio_dev->num_channels = 3; + + ret = iio_device_register(indio_dev); + if (ret < 0) { + hid_err(hdev, "Unable to register iio device\n"); + goto error_iio_probe; + } + return 0; + +error_iio_probe: + kfree(indio_dev); + sc->indio_dev = NULL; + return ret; +} + +static void sony_iio_remove(struct sony_sc *sc) +{ + if (!sc->indio_dev) + return; + + iio_device_unregister(sc->indio_dev); + kfree(sc->indio_dev); + sc->indio_dev = NULL; +} +#endif + /* * If a controller is plugged in via USB while already connected via Bluetooth * it will show up as two devices. A global list of connected controllers and @@ -2073,6 +2228,15 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id) } } +#if IS_BUILTIN(CONFIG_IIO) || \ + (IS_MODULE(CONFIG_IIO) && IS_MODULE(CONFIG_HID_SONY)) + if (sc->quirks & SONY_IIO_SUPPORT) { + ret = sony_iio_probe(sc); + if (ret < 0) + goto err_stop; + } +#endif + if (sc->quirks & SONY_FF_SUPPORT) { ret = sony_init_ff(sc); if (ret < 0) @@ -2087,6 +2251,11 @@ err_stop: sony_leds_remove(sc); if (sc->quirks & SONY_BATTERY_SUPPORT) sony_battery_remove(sc); +#if IS_BUILTIN(CONFIG_IIO) || \ + (IS_MODULE(CONFIG_IIO) && IS_MODULE(CONFIG_HID_SONY)) + if (sc->quirks & SONY_IIO_SUPPORT) + sony_iio_remove(sc); +#endif sony_cancel_work_sync(sc); kfree(sc->output_report_dmabuf); sony_remove_dev_list(sc); @@ -2107,6 +2276,12 @@ static void sony_remove(struct hid_device *hdev) sony_battery_remove(sc); } +#if IS_BUILTIN(CONFIG_IIO) || \ + (IS_MODULE(CONFIG_IIO) && IS_MODULE(CONFIG_HID_SONY)) + if (sc->quirks & SONY_IIO_SUPPORT) + sony_iio_remove(sc); +#endif + sony_cancel_work_sync(sc); kfree(sc->output_report_dmabuf);