Handling Controllers with Acc/Gyro/Mag via HID system

Handling Controllers with Acc/Gyro/Mag via HID system

[simon]

Hi all,
I'm in the process of fixing the HID descriptor for the PS3 Move
Controller. which has a particularly convoluted layout for it's
Accelormeters, Gyros and Magnetometers involving 2 sets of data per output
report.

https://github.com/nitsch/moveonpc/wiki/Input-report

The plan is/was to massage the HID descriptor so the first set of
Accels/Gyro/Mag appear in a set a axis, as a simple way of getting to this
data.

The Magnetometers are only 12 bits, and if 'we' want the HID system to be
able to process these as an axis the data needs to be shuffled. Previously
for the Sony SixAxis Accels was done in 'sony_raw_event'.

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-sony.c?id=refs/tags/v4.1-rc7#n1035


My reshuffle would be:
---
                /* Need to fix order and shared nibble of 12bit Temp/Mags */
                /* 8e 4. .. .. .. .. -> Temp = 0x08E4 */
                /* .. .1 51 .. .. .. -> MagX = 0x0151 */
                /* .. .. .. 02 1. .. -> MagZ = 0x0021 */
                /* .. .. .. .. .2 98 -> MagY = 0x0298 */

                __u8 tmp;
                tmp = rd[38];
                rd[38] = (((rd[37] & 0xF0) >> 4) | ((rd[39] & 0x0F) << 4));
                rd[37] = (((tmp & 0xF0) >> 4) | ((rd[37] & 0x0F) << 4));
                rd[39] = (((rd[39] & 0xF0) >> 4) | ((tmp & 0x0F) << 4));

                tmp = rd[41];
                rd[41] = (((rd[40] & 0xF0) >> 4) | ((rd[42] & 0x0F) << 4));
                rd[40] = (((tmp & 0xF0) >> 4) | ((rd[40] & 0x0F) << 4));
                rd[42] = (((rd[42] & 0xF0) >> 4) | ((tmp & 0x0F) << 4));
---

This works, however I have noticed that this affects/reshuffles the output
of '/dev/hidraw0', which would 'corrupt' the data going into the existing
user-mode drivers (such as psmoveapi).

Is there a 'better' place/way to reshuffle this data, or is this just how
it needs to be?

Given that more controllers are coming with 'motion' controls, is there a
framework in place for unifying support?

Cheers,
Simon.

Re: Handling Controllers with Acc/Gyro/Mag via HID system

[Frank Praznik]

On 6/8/2015 11:41, simon@mungewell.org wrote:
> Hi all,
> I'm in the process of fixing the HID descriptor for the PS3 Move
> Controller. which has a particularly convoluted layout for it's
> Accelormeters, Gyros and Magnetometers involving 2 sets of data per output
> report.
>
> https://github.com/nitsch/moveonpc/wiki/Input-report
>
> The plan is/was to massage the HID descriptor so the first set of
> Accels/Gyro/Mag appear in a set a axis, as a simple way of getting to this
> data.
>
> The Magnetometers are only 12 bits, and if 'we' want the HID system to be
> able to process these as an axis the data needs to be shuffled. Previously
> for the Sony SixAxis Accels was done in 'sony_raw_event'.
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/hid/hid-sony.c?id=refs/tags/v4.1-rc7#n1035
>
>
> My reshuffle would be:
> ---
>                  /* Need to fix order and shared nibble of 12bit Temp/Mags */
>                  /* 8e 4. .. .. .. .. -> Temp = 0x08E4 */
>                  /* .. .1 51 .. .. .. -> MagX = 0x0151 */
>                  /* .. .. .. 02 1. .. -> MagZ = 0x0021 */
>                  /* .. .. .. .. .2 98 -> MagY = 0x0298 */
>
>                  __u8 tmp;
>                  tmp = rd[38];
>                  rd[38] = (((rd[37] & 0xF0) >> 4) | ((rd[39] & 0x0F) << 4));
>                  rd[37] = (((tmp & 0xF0) >> 4) | ((rd[37] & 0x0F) << 4));
>                  rd[39] = (((rd[39] & 0xF0) >> 4) | ((tmp & 0x0F) << 4));
>
>                  tmp = rd[41];
>                  rd[41] = (((rd[40] & 0xF0) >> 4) | ((rd[42] & 0x0F) << 4));
>                  rd[40] = (((tmp & 0xF0) >> 4) | ((rd[40] & 0x0F) << 4));
>                  rd[42] = (((rd[42] & 0xF0) >> 4) | ((tmp & 0x0F) << 4));
> ---
>
> This works, however I have noticed that this affects/reshuffles the output
> of '/dev/hidraw0', which would 'corrupt' the data going into the existing
> user-mode drivers (such as psmoveapi).
>
> Is there a 'better' place/way to reshuffle this data, or is this just how
> it needs to be?
>
> Given that more controllers are coming with 'motion' controls, is there a
> framework in place for unifying support?
>
> Cheers,
> Simon.
>

Hmm, perhaps remove the mappings for the motion controls in the HID 
descriptor and add the motion axis flags in the input device creation 
callback as is currently done with the touchpad axes on the DS4.  Then 
you can parse the motion data and fire off motion events manually in the 
raw event function (again, same as the touch events on the DS4) and the 
raw data in the packet will pass through unchanged for userspace drivers.

Regards,
Frank

[RFC] HID: hid-sony: Add basic iio-subsystem reading of SixAxis Accelerometers

[Simon Wood]

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.

Hopefully this will become the standard way of connecting controllers,
motion controls and head mounted displays.
---
 drivers/hid/hid-sony.c | 175 +++++++++++++++++++++++++++++++++++++++++++++++++
 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);
+	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);
-- 
2.1.4

[RFC] HID: hid-sony: Add basic iio-subsystem reading of SixAxis Accelerometers

[Simon Wood]

[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.

Hopefully this will become the standard way of connecting controllers,
motion controls and head mounted displays.
---
 drivers/hid/hid-sony.c | 175 +++++++++++++++++++++++++++++++++++++++++++++++++
 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);
+	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);
-- 
2.1.4

[RFC] HID: hid-sony: Add basic iio-subsystem reading of SixAxis Accelerometers

[Simon Wood]

[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.

Hopefully this will become the standard way of connecting controllers,
motion controls and head mounted displays.
---
 drivers/hid/hid-sony.c | 175 +++++++++++++++++++++++++++++++++++++++++++++++++
 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);
+	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);
-- 
2.1.4

Re: [RFC] HID: hid-sony: Add basic iio-subsystem reading of SixAxis Accelerometers

[Bastien Nocera]

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.

Re: [RFC] HID: hid-sony: Add basic iio-subsystem reading of SixAxis Accelerometers

[Jonathan Cameron]

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);
> 

Re: [RFC] HID: hid-sony: Add basic iio-subsystem reading of SixAxis Accelerometers

[simon]

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.

Re: [RFC] HID: hid-sony: Add basic iio-subsystem reading of SixAxis Accelerometers

[Jonathan Cameron]

On 14/06/15 18:25, simon-wM4F9T/ekXmXDw4h08c5KA@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC] HID: hid-sony: Add basic iio-subsystem reading of SixAxis Accelerometers

[Jonathan Cameron]

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
> 

Re: [RFC] HID: hid-sony: Add basic iio-subsystem reading of SixAxis Accelerometers

[Srinivas Pandruvada]

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
> > 
> 

Re: [RFC] HID: hid-sony: Add basic iio-subsystem reading of SixAxis Accelerometers

[Jonathan Cameron]

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-wM4F9T/ekXmXDw4h08c5KA@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC] HID: hid-sony: Add basic iio-subsystem reading of SixAxis Accelerometers

[Jonathan Cameron]

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-iio" in

Re: [RFC] HID: hid-sony: Add basic iio-subsystem reading of SixAxis Accelerometers

[simon]

>> 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.

Re: [RFC] HID: hid-sony: Add basic iio-subsystem reading of SixAxis Accelerometers

[Jonathan Cameron]

On 18/06/15 07:15, simon-wM4F9T/ekXmXDw4h08c5KA@public.gmane.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.

Re: [RFC] HID: hid-sony: Add basic iio-subsystem reading of SixAxis Accelerometers

[Jonathan Cameron]

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-iio" in