[RFC_v2 0/4] HID: hid-sony: Add IIO Suport for Motion Controllers

[RFC_v2 0/4] HID: hid-sony: Add IIO Suport for Motion Controllers

[Simon Wood]

This series of patches is a RFC for the idea of connecting motion capability
controllers to the IIO subsystem, initially targeting the Sony SixAxis
controller. In the future I hope that this can be used for sensor packs used
in VR headset/devices.

The advantage of the IIO subsystem is that the data is presented in SI units,
although it is noted that the current API requires root level access - this is
really a distribution requirement (UDEV rules can make '/dev/iiodevice0'
accessible).

The RFC is in 4 parts, split into logical steps:
[PATCH 1/4] HID: hid-sony: Add basic IIO support for SixAxis Controller
[PATCH 2/4] HID: hid-sony: Add IIO buffer support for SixAxis Controller
[PATCH 3/4] HID: hid-sony: Add IIO trigger support for SixAxis Controller
[PATCH 4/4] HID: hid-sony: Add IIO support for DualShock4 Controller

The SixAxis contains accelerometers, the DS4 contains accelerometers and gyros.

The next stage would be support the PS Move controller, which contains
accelerometers, gyros and magnetometers.

As an example of IIO usage I would point to RTIMULib, which recently showed
a full 9-dof IMU fusion via IIO. (1)


Known Bug:
At present the 3rd patch introduces the issue that once loaded and a device
connects, the module can not be unloaded (even after device disconnects).

It seems that the refcount is being increased, but not decreased.
--
$ cat /sys/module/hid_sony/refcnt 
2
--


(1) https://richardstechnotes.wordpress.com/2015/06/17/rteiioimu-driving-a-9-dof-imu-via-industrial-io-iio/

[RFC_v2 1/4] HID: hid-sony: Add basic IIO support for SixAxis Controller

[Simon Wood]

---
 drivers/hid/hid-sony.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 152 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 6ca96ce..c4686e3 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,22 @@ 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_data[3];
+};
+
+enum sony_iio_axis {
+	AXIS_ACC_X,
+	AXIS_ACC_Y,
+	AXIS_ACC_Z,
+};
+
+struct sony_iio {
+	struct sony_sc *sc;
+#endif
 };
 
 static __u8 *sixaxis_fixup(struct hid_device *hdev, __u8 *rdesc,
@@ -843,7 +861,6 @@ static __u8 *sixaxis_fixup(struct hid_device *hdev, __u8 *rdesc,
 	*rsize = sizeof(sixaxis_rdesc);
 	return sixaxis_rdesc;
 }
-
 static __u8 *ps3remote_fixup(struct hid_device *hdev, __u8 *rdesc,
 			     unsigned int *rsize)
 {
@@ -1047,6 +1064,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_data[AXIS_ACC_X] = (rd[42] << 8) + rd[41];
+		sc->last_data[AXIS_ACC_Y] = (rd[44] << 8) + rd[43];
+		sc->last_data[AXIS_ACC_Z] = (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 +1792,114 @@ 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);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->type) {
+		case IIO_ACCEL:
+			*val = data->sc->last_data[chan->address];
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_ACCEL:
+			*val = 0;	/* 9.80665/117 = 0.084540086 */
+			*val2 = 84540;
+			return IIO_VAL_INT_PLUS_MICRO;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_OFFSET:
+		switch (chan->type) {
+		case IIO_ACCEL:
+			*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_OFFSET),                              \
+	.address = AXIS_ACC_##_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 err;
+	}
+	return 0;
+
+err:
+	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 +2204,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 +2227,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 +2252,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_v2 1/4] HID: hid-sony: Add basic IIO support for SixAxis Controller

[Antonio Ospite]

On Tue, 23 Jun 2015 18:30:27 -0600
Simon Wood <simon-wM4F9T/ekXmXDw4h08c5KA@public.gmane.org> wrote:

> ---
>  drivers/hid/hid-sony.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 152 insertions(+), 1 deletion(-)
> 

Hi Simon, I don't know much about the IIO API, I just have some generic
comments.

> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 6ca96ce..c4686e3 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,22 @@ 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))

This check can be factored out, just define a HAVE_IIO constant when it
passes and check for that. This is mainly for readability, it
avoids having to read the condition over and over.

> +	struct iio_dev *indio_dev;
> +	__u16 last_data[3];
> +};
> +
> +enum sony_iio_axis {
> +	AXIS_ACC_X,
> +	AXIS_ACC_Y,
> +	AXIS_ACC_Z,
> +};
> +
> +struct sony_iio {
> +	struct sony_sc *sc;
> +#endif
>  };
>  
>  static __u8 *sixaxis_fixup(struct hid_device *hdev, __u8 *rdesc,
> @@ -843,7 +861,6 @@ static __u8 *sixaxis_fixup(struct hid_device *hdev, __u8 *rdesc,
>  	*rsize = sizeof(sixaxis_rdesc);
>  	return sixaxis_rdesc;
>  }
> -

unrelated change, which you undo in patch 2 anyway :)

>  static __u8 *ps3remote_fixup(struct hid_device *hdev, __u8 *rdesc,
>  			     unsigned int *rsize)
>  {
> @@ -1047,6 +1064,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_data[AXIS_ACC_X] = (rd[42] << 8) + rd[41];
> +		sc->last_data[AXIS_ACC_Y] = (rd[44] << 8) + rd[43];
> +		sc->last_data[AXIS_ACC_Z] = (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 +1792,114 @@ 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);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +			*val = data->sc->last_data[chan->address];
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +			*val = 0;	/* 9.80665/117 = 0.084540086 */
> +			*val2 = 84540;

I guess 9.80665 is 'g', but is the 117 taken from the accelerometer
datasheet? What is it? And BTW I get 9.80665/117 = 0.083817521

> +			return IIO_VAL_INT_PLUS_MICRO;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_OFFSET:
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +			*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_OFFSET),                              \
> +	.address = AXIS_ACC_##_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));

In general for new code the devm_ variants are preferred, but I am
not sure in this case, maybe others have comments about that?

> +	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 you used the devm_ variant here and in the other patches, the cleanup
below and the sony_iio_remove() function could go away.

> +	if (ret < 0) {
> +		hid_err(hdev, "Unable to register iio device\n");
> +		goto err;
> +	}
> +	return 0;
> +
> +err:
> +	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 +2204,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 +2227,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 +2252,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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [RFC_v2 1/4] HID: hid-sony: Add basic IIO support for SixAxis Controller

[Daniel Baluta]

<snip>

>> +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));
>
> In general for new code the devm_ variants are preferred, but I am
> not sure in this case, maybe others have comments about that?
>
>> +     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;

Use ARRAY_SIZE(sony_sixaxis_channels) here.

>> +
>> +     ret = iio_device_register(indio_dev);
>
> if you used the devm_ variant here and in the other patches, the cleanup
> below and the sony_iio_remove() function could go away.
>
>> +     if (ret < 0) {
>> +             hid_err(hdev, "Unable to register iio device\n");
>> +             goto err;
>> +     }
>> +     return 0;
>> +
>> +err:
>> +     kfree(indio_dev);

Not to mention that the correct way to free an iio_dev is iio_device_free.

>> +     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);
The same here.

>> +     sc->indio_dev = NULL;
>> +}

So, better use the devm_ variant at least for indio_dev allocation.

thanks,
Daniel.

Re: [RFC_v2 1/4] HID: hid-sony: Add basic IIO support for SixAxis Controller

[simon-wM4F9T/ekXmXDw4h08c5KA]

>> +	case IIO_CHAN_INFO_SCALE:
>> +		switch (chan->type) {
>> +		case IIO_ACCEL:
>> +			*val = 0;	/* 9.80665/117 = 0.084540086 */
>> +			*val2 = 84540;
>
> I guess 9.80665 is 'g', but is the 117 taken from the accelerometer
> datasheet? What is it? And BTW I get 9.80665/117 = 0.083817521

The '117' was taken from experimentation, average value accessed across
all accelerometer axis on a stationary device.

It does appear however that I can not use a calculator ;-) (...must have
been a prior value left there).
Simon.

Re: [RFC_v2 1/4] HID: hid-sony: Add basic IIO support for SixAxis Controller

[Jonathan Cameron]

On 24/06/15 10:13, Antonio Ospite wrote:
> On Tue, 23 Jun 2015 18:30:27 -0600
> Simon Wood <simon@mungewell.org> wrote:
> 
>> ---
>>  drivers/hid/hid-sony.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 152 insertions(+), 1 deletion(-)
>>
> 
> Hi Simon, I don't know much about the IIO API, I just have some generic
> comments.
> 
>> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
>> index 6ca96ce..c4686e3 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,22 @@ 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))
> 
> This check can be factored out, just define a HAVE_IIO constant when it
> passes and check for that. This is mainly for readability, it
> avoids having to read the condition over and over.
> 
>> +	struct iio_dev *indio_dev;
>> +	__u16 last_data[3];
>> +};
>> +
>> +enum sony_iio_axis {
>> +	AXIS_ACC_X,
>> +	AXIS_ACC_Y,
>> +	AXIS_ACC_Z,
>> +};
>> +
>> +struct sony_iio {
>> +	struct sony_sc *sc;
>> +#endif
>>  };
>>  
>>  static __u8 *sixaxis_fixup(struct hid_device *hdev, __u8 *rdesc,
>> @@ -843,7 +861,6 @@ static __u8 *sixaxis_fixup(struct hid_device *hdev, __u8 *rdesc,
>>  	*rsize = sizeof(sixaxis_rdesc);
>>  	return sixaxis_rdesc;
>>  }
>> -
> 
> unrelated change, which you undo in patch 2 anyway :)
> 
>>  static __u8 *ps3remote_fixup(struct hid_device *hdev, __u8 *rdesc,
>>  			     unsigned int *rsize)
>>  {
>> @@ -1047,6 +1064,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_data[AXIS_ACC_X] = (rd[42] << 8) + rd[41];
>> +		sc->last_data[AXIS_ACC_Y] = (rd[44] << 8) + rd[43];
>> +		sc->last_data[AXIS_ACC_Z] = (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 +1792,114 @@ 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);
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		switch (chan->type) {
>> +		case IIO_ACCEL:
>> +			*val = data->sc->last_data[chan->address];
>> +			return IIO_VAL_INT;
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +	case IIO_CHAN_INFO_SCALE:
>> +		switch (chan->type) {
>> +		case IIO_ACCEL:
>> +			*val = 0;	/* 9.80665/117 = 0.084540086 */
>> +			*val2 = 84540;
> 
> I guess 9.80665 is 'g', but is the 117 taken from the accelerometer
> datasheet? What is it? And BTW I get 9.80665/117 = 0.083817521
> 
>> +			return IIO_VAL_INT_PLUS_MICRO;
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +	case IIO_CHAN_INFO_OFFSET:
>> +		switch (chan->type) {
>> +		case IIO_ACCEL:
>> +			*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_OFFSET),                              \
>> +	.address = AXIS_ACC_##_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));
> 
> In general for new code the devm_ variants are preferred, but I am
> not sure in this case, maybe others have comments about that?
Absolutely for the allocation.   No ordering issues with this one.
> 
>> +	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 you used the devm_ variant here and in the other patches, the cleanup
> below and the sony_iio_remove() function could go away.
I haven't checked this driver, but rule of thumb for this one is that, if there
is any other related cleanup done without using devm functions then you should
not use the devm_iio_device_register variant as it will mean something has been
cleaned up BEFORE we remove the userspace interfaces that might need it.

> 
>> +	if (ret < 0) {
>> +		hid_err(hdev, "Unable to register iio device\n");
>> +		goto err;
>> +	}
>> +	return 0;
>> +
>> +err:
>> +	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 +2204,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 +2227,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 +2252,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
>>
>> --
>> 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
> 
> 

[RFC_v2 2/4] HID: hid-sony: Add IIO buffer support for SixAxis Controller

[Simon Wood]

---
 drivers/hid/hid-sony.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index c4686e3..b7a7f0d 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -37,6 +37,11 @@
 #include <linux/idr.h>
 #include <linux/input/mt.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/interrupt.h>
 
 #include "hid-ids.h"
 
@@ -852,6 +857,7 @@ enum sony_iio_axis {
 
 struct sony_iio {
 	struct sony_sc *sc;
+	u8 buff[16];		/* 3x 16-bit + padding + timestamp */
 #endif
 };
 
@@ -861,6 +867,7 @@ static __u8 *sixaxis_fixup(struct hid_device *hdev, __u8 *rdesc,
 	*rsize = sizeof(sixaxis_rdesc);
 	return sixaxis_rdesc;
 }
+
 static __u8 *ps3remote_fixup(struct hid_device *hdev, __u8 *rdesc,
 			     unsigned int *rsize)
 {
@@ -1841,12 +1848,20 @@ static int sony_iio_read_raw(struct iio_dev *indio_dev,
 	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |          \
 		BIT(IIO_CHAN_INFO_OFFSET),                              \
 	.address = AXIS_ACC_##_axis,                                    \
+	.scan_index = AXIS_ACC_##_axis,                                 \
+	.scan_type = {                                                  \
+		.sign = 's',                                            \
+		.realbits = 16,                                         \
+		.storagebits = 16,                                      \
+		.shift = 0,                                             \
+	},                                                              \
 }
 
 static const struct iio_chan_spec sony_sixaxis_channels[] = {
 	SONY_ACC_CHANNEL(X),
 	SONY_ACC_CHANNEL(Y),
 	SONY_ACC_CHANNEL(Z),
+	IIO_CHAN_SOFT_TIMESTAMP(3),
 };
 
 static const struct iio_info sony_iio_info = {
@@ -1854,6 +1869,25 @@ static const struct iio_info sony_iio_info = {
 	.driver_module = THIS_MODULE,
 };
 
+static irqreturn_t sony_iio_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct sony_iio *data = iio_priv(indio_dev);
+	int64_t time_ns = iio_get_time_ns();
+	int bit, i = 0;
+
+	for_each_set_bit(bit, indio_dev->active_scan_mask,
+			 indio_dev->masklength) {
+		((u16 *)data->buff)[i++] = data->sc->last_data[bit];
+	}
+
+	iio_push_to_buffers_with_timestamp(indio_dev, data->buff, time_ns);
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
 static int sony_iio_probe(struct sony_sc *sc)
 {
 	struct hid_device *hdev = sc->hdev;
@@ -1871,18 +1905,27 @@ static int sony_iio_probe(struct sony_sc *sc)
 
 	indio_dev->dev.parent = &hdev->dev;
 	indio_dev->name = dev_name(&hdev->dev);
-	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
 	indio_dev->info = &sony_iio_info;
 	indio_dev->channels = sony_sixaxis_channels;
-	indio_dev->num_channels = 3;
+	indio_dev->num_channels = ARRAY_SIZE(sony_sixaxis_channels);
+
+	ret = iio_triggered_buffer_setup(indio_dev, NULL,
+			sony_iio_trigger_handler, NULL);
+	if (ret < 0) {
+		dev_err(&hdev->dev, "unable to setup iio triggered buffer\n");
+		goto err;
+	}
 
 	ret = iio_device_register(indio_dev);
 	if (ret < 0) {
 		hid_err(hdev, "Unable to register iio device\n");
-		goto err;
+		goto err_buffer_cleanup;
 	}
 	return 0;
 
+err_buffer_cleanup:
+	iio_triggered_buffer_cleanup(indio_dev);
 err:
 	kfree(indio_dev);
 	sc->indio_dev = NULL;
@@ -1895,6 +1938,7 @@ static void sony_iio_remove(struct sony_sc *sc)
 		return;
 
 	iio_device_unregister(sc->indio_dev);
+	iio_triggered_buffer_cleanup(sc->indio_dev);
 	kfree(sc->indio_dev);
 	sc->indio_dev = NULL;
 }
-- 
2.1.4

Re: [RFC_v2 2/4] HID: hid-sony: Add IIO buffer support for SixAxis Controller

[Jonathan Cameron]

On 24/06/15 01:30, Simon Wood wrote:
> ---
>  drivers/hid/hid-sony.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index c4686e3..b7a7f0d 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -37,6 +37,11 @@
>  #include <linux/idr.h>
>  #include <linux/input/mt.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
This sysfs one shouldn't have become a dependency as part of adding buffered support...

> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/interrupt.h>
>  
>  #include "hid-ids.h"
>  
> @@ -852,6 +857,7 @@ enum sony_iio_axis {
>  
>  struct sony_iio {
>  	struct sony_sc *sc;
> +	u8 buff[16];		/* 3x 16-bit + padding + timestamp */
Make it u16 buff[8] to cut down on the casts below.
>  #endif
>  };
>  
> @@ -861,6 +867,7 @@ static __u8 *sixaxis_fixup(struct hid_device *hdev, __u8 *rdesc,
>  	*rsize = sizeof(sixaxis_rdesc);
>  	return sixaxis_rdesc;
>  }
> +
Some stray white space cleanups in here that should be in a separate patch.
>  static __u8 *ps3remote_fixup(struct hid_device *hdev, __u8 *rdesc,
>  			     unsigned int *rsize)
>  {
> @@ -1841,12 +1848,20 @@ static int sony_iio_read_raw(struct iio_dev *indio_dev,
>  	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |          \
>  		BIT(IIO_CHAN_INFO_OFFSET),                              \
>  	.address = AXIS_ACC_##_axis,                                    \
> +	.scan_index = AXIS_ACC_##_axis,                                 \
> +	.scan_type = {                                                  \
> +		.sign = 's',                                            \
> +		.realbits = 16,                                         \
> +		.storagebits = 16,                                      \
> +		.shift = 0,                                             \
Don't bother specifying shift=0 as it's the default.
> +	},                                                              \
>  }
>  
>  static const struct iio_chan_spec sony_sixaxis_channels[] = {
>  	SONY_ACC_CHANNEL(X),
>  	SONY_ACC_CHANNEL(Y),
>  	SONY_ACC_CHANNEL(Z),
> +	IIO_CHAN_SOFT_TIMESTAMP(3),
>  };
>  
>  static const struct iio_info sony_iio_info = {
> @@ -1854,6 +1869,25 @@ static const struct iio_info sony_iio_info = {
>  	.driver_module = THIS_MODULE,
>  };
>  
> +static irqreturn_t sony_iio_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct sony_iio *data = iio_priv(indio_dev);
> +	int64_t time_ns = iio_get_time_ns();
> +	int bit, i = 0;
> +
> +	for_each_set_bit(bit, indio_dev->active_scan_mask,
> +			 indio_dev->masklength) {
> +		((u16 *)data->buff)[i++] = data->sc->last_data[bit];
> +	}
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, data->buff, time_ns);
Put the iio_get_time_ns call directly in rather than bothering with the local
variable.
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int sony_iio_probe(struct sony_sc *sc)
>  {
>  	struct hid_device *hdev = sc->hdev;
> @@ -1871,18 +1905,27 @@ static int sony_iio_probe(struct sony_sc *sc)
>  
>  	indio_dev->dev.parent = &hdev->dev;
>  	indio_dev->name = dev_name(&hdev->dev);
> -	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
>  	indio_dev->info = &sony_iio_info;
>  	indio_dev->channels = sony_sixaxis_channels;
> -	indio_dev->num_channels = 3;
> +	indio_dev->num_channels = ARRAY_SIZE(sony_sixaxis_channels);
> +
> +	ret = iio_triggered_buffer_setup(indio_dev, NULL,
> +			sony_iio_trigger_handler, NULL);
> +	if (ret < 0) {
> +		dev_err(&hdev->dev, "unable to setup iio triggered buffer\n");
> +		goto err;
> +	}
>  
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0) {
>  		hid_err(hdev, "Unable to register iio device\n");
> -		goto err;
> +		goto err_buffer_cleanup;
>  	}
>  	return 0;
>  
> +err_buffer_cleanup:
> +	iio_triggered_buffer_cleanup(indio_dev);
>  err:
>  	kfree(indio_dev);
>  	sc->indio_dev = NULL;
> @@ -1895,6 +1938,7 @@ static void sony_iio_remove(struct sony_sc *sc)
>  		return;
>  
>  	iio_device_unregister(sc->indio_dev);
> +	iio_triggered_buffer_cleanup(sc->indio_dev);
>  	kfree(sc->indio_dev);
>  	sc->indio_dev = NULL;
>  }
> 

[RFC_v2 3/4] HID: hid-sony: Add IIO trigger support for SixAxis Controller

[Simon Wood]

---
 drivers/hid/hid-sony.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 69 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index b7a7f0d..ce0526d 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -39,9 +39,11 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 #include <linux/iio/buffer.h>
+#include <linux/iio/trigger.h>
 #include <linux/iio/trigger_consumer.h>
 #include <linux/iio/triggered_buffer.h>
 #include <linux/interrupt.h>
+#include <linux/irq_work.h>
 
 #include "hid-ids.h"
 
@@ -855,9 +857,14 @@ enum sony_iio_axis {
 	AXIS_ACC_Z,
 };
 
+static void sony_iio_trigger_work(struct irq_work *work);
+
 struct sony_iio {
 	struct sony_sc *sc;
+	struct iio_trigger *trig;
+
 	u8 buff[16];		/* 3x 16-bit + padding + timestamp */
+	struct irq_work work;
 #endif
 };
 
@@ -1076,6 +1083,13 @@ static int sony_raw_event(struct hid_device *hdev, struct hid_report *report,
 		sc->last_data[AXIS_ACC_X] = (rd[42] << 8) + rd[41];
 		sc->last_data[AXIS_ACC_Y] = (rd[44] << 8) + rd[43];
 		sc->last_data[AXIS_ACC_Z] = (rd[46] << 8) + rd[45];
+
+		if (sc->indio_dev) {
+			struct sony_iio *data;
+
+			data = iio_priv(sc->indio_dev);
+			sony_iio_trigger_work(&data->work);
+		}
 #endif
 		sixaxis_parse_report(sc, rd, size);
 	} else if (((sc->quirks & DUALSHOCK4_CONTROLLER_USB) && rd[0] == 0x01 &&
@@ -1869,6 +1883,28 @@ static const struct iio_info sony_iio_info = {
 	.driver_module = THIS_MODULE,
 };
 
+static void sony_iio_trigger_work(struct irq_work *work)
+{
+	struct sony_iio *data = container_of(work, struct sony_iio, work);
+
+	iio_trigger_poll(data->trig);
+}
+
+static ssize_t sony_iio_trigger_poll(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct iio_trigger *trig = to_iio_trigger(dev);
+	struct sony_iio *data = iio_trigger_get_drvdata(trig);
+
+	irq_work_queue(&data->work);
+
+	return count;
+}
+
+static const struct iio_trigger_ops sony_iio_trigger_ops = {
+	.owner = THIS_MODULE,
+};
+
 static irqreturn_t sony_iio_trigger_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -1910,11 +1946,29 @@ static int sony_iio_probe(struct sony_sc *sc)
 	indio_dev->channels = sony_sixaxis_channels;
 	indio_dev->num_channels = ARRAY_SIZE(sony_sixaxis_channels);
 
+	data->trig = iio_trigger_alloc("%s-dev%d", indio_dev->name,
+		indio_dev->id);
+	if (!data->trig) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	data->trig->dev.parent = &hdev->dev;
+	data->trig->ops = &sony_iio_trigger_ops;
+	iio_trigger_set_drvdata(data->trig, indio_dev);
+	indio_dev->trig = iio_trigger_get(data->trig);
+
+	init_irq_work(&data->work, sony_iio_trigger_work);
+
+	ret = iio_trigger_register(data->trig);
+	if (ret)
+		goto err_trigger_free;
+
 	ret = iio_triggered_buffer_setup(indio_dev, NULL,
 			sony_iio_trigger_handler, NULL);
 	if (ret < 0) {
 		dev_err(&hdev->dev, "unable to setup iio triggered buffer\n");
-		goto err;
+		goto err_trigger_unregister;
 	}
 
 	ret = iio_device_register(indio_dev);
@@ -1926,6 +1980,11 @@ static int sony_iio_probe(struct sony_sc *sc)
 
 err_buffer_cleanup:
 	iio_triggered_buffer_cleanup(indio_dev);
+err_trigger_unregister:
+	if (data->trig)
+		iio_trigger_unregister(data->trig);
+err_trigger_free:
+	iio_trigger_free(data->trig);
 err:
 	kfree(indio_dev);
 	sc->indio_dev = NULL;
@@ -1934,11 +1993,19 @@ err:
 
 static void sony_iio_remove(struct sony_sc *sc)
 {
+	struct sony_iio *data;
+
 	if (!sc->indio_dev)
 		return;
 
-	iio_device_unregister(sc->indio_dev);
+	data = iio_priv(sc->indio_dev);
+
 	iio_triggered_buffer_cleanup(sc->indio_dev);
+	if (data->trig)
+		iio_trigger_unregister(data->trig);
+	iio_trigger_free(data->trig);
+	iio_device_unregister(sc->indio_dev);
+
 	kfree(sc->indio_dev);
 	sc->indio_dev = NULL;
 }
-- 
2.1.4

Re: [RFC_v2 3/4] HID: hid-sony: Add IIO trigger support for SixAxis Controller

[Jonathan Cameron]

On 24/06/15 01:30, Simon Wood wrote:
Description here please and sign-off etc (though as this is an RFC
perhaps you don't consider it ready to be signed off on?)

Main issue I've spotted in here is in the ordering in remove...

J
> ---
>  drivers/hid/hid-sony.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 69 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index b7a7f0d..ce0526d 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -39,9 +39,11 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  #include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
>  #include <linux/interrupt.h>
> +#include <linux/irq_work.h>
>  
>  #include "hid-ids.h"
>  
> @@ -855,9 +857,14 @@ enum sony_iio_axis {
>  	AXIS_ACC_Z,
>  };
>  
> +static void sony_iio_trigger_work(struct irq_work *work);
> +
>  struct sony_iio {
>  	struct sony_sc *sc;
> +	struct iio_trigger *trig;
> +
>  	u8 buff[16];		/* 3x 16-bit + padding + timestamp */
> +	struct irq_work work;
>  #endif
>  };
>  
> @@ -1076,6 +1083,13 @@ static int sony_raw_event(struct hid_device *hdev, struct hid_report *report,
>  		sc->last_data[AXIS_ACC_X] = (rd[42] << 8) + rd[41];
>  		sc->last_data[AXIS_ACC_Y] = (rd[44] << 8) + rd[43];
>  		sc->last_data[AXIS_ACC_Z] = (rd[46] << 8) + rd[45];
> +
> +		if (sc->indio_dev) {
> +			struct sony_iio *data;
> +
> +			data = iio_priv(sc->indio_dev);
> +			sony_iio_trigger_work(&data->work);
> +		}
>  #endif
>  		sixaxis_parse_report(sc, rd, size);
>  	} else if (((sc->quirks & DUALSHOCK4_CONTROLLER_USB) && rd[0] == 0x01 &&
> @@ -1869,6 +1883,28 @@ static const struct iio_info sony_iio_info = {
>  	.driver_module = THIS_MODULE,
>  };
>  
> +static void sony_iio_trigger_work(struct irq_work *work)
> +{
> +	struct sony_iio *data = container_of(work, struct sony_iio, work);
> +
> +	iio_trigger_poll(data->trig);
> +}
> +
> +static ssize_t sony_iio_trigger_poll(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct iio_trigger *trig = to_iio_trigger(dev);
> +	struct sony_iio *data = iio_trigger_get_drvdata(trig);
> +
> +	irq_work_queue(&data->work);
> +
> +	return count;
> +}
> +
> +static const struct iio_trigger_ops sony_iio_trigger_ops = {
> +	.owner = THIS_MODULE,
> +};
> +
>  static irqreturn_t sony_iio_trigger_handler(int irq, void *p)
>  {
>  	struct iio_poll_func *pf = p;
> @@ -1910,11 +1946,29 @@ static int sony_iio_probe(struct sony_sc *sc)
>  	indio_dev->channels = sony_sixaxis_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(sony_sixaxis_channels);
>  
> +	data->trig = iio_trigger_alloc("%s-dev%d", indio_dev->name,
> +		indio_dev->id);
> +	if (!data->trig) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	data->trig->dev.parent = &hdev->dev;
> +	data->trig->ops = &sony_iio_trigger_ops;
> +	iio_trigger_set_drvdata(data->trig, indio_dev);
> +	indio_dev->trig = iio_trigger_get(data->trig);
> +
> +	init_irq_work(&data->work, sony_iio_trigger_work);
> +
> +	ret = iio_trigger_register(data->trig);
> +	if (ret)
> +		goto err_trigger_free;
> +
>  	ret = iio_triggered_buffer_setup(indio_dev, NULL,
>  			sony_iio_trigger_handler, NULL);
>  	if (ret < 0) {
>  		dev_err(&hdev->dev, "unable to setup iio triggered buffer\n");
> -		goto err;
> +		goto err_trigger_unregister;
>  	}
>  
>  	ret = iio_device_register(indio_dev);
> @@ -1926,6 +1980,11 @@ static int sony_iio_probe(struct sony_sc *sc)
>  
>  err_buffer_cleanup:
>  	iio_triggered_buffer_cleanup(indio_dev);
> +err_trigger_unregister:
> +	if (data->trig)
> +		iio_trigger_unregister(data->trig);
> +err_trigger_free:
> +	iio_trigger_free(data->trig);
>  err:
>  	kfree(indio_dev);
>  	sc->indio_dev = NULL;
> @@ -1934,11 +1993,19 @@ err:
>  
>  static void sony_iio_remove(struct sony_sc *sc)
>  {
> +	struct sony_iio *data;
> +
>  	if (!sc->indio_dev)
>  		return;
>  
> -	iio_device_unregister(sc->indio_dev);
> +	data = iio_priv(sc->indio_dev);
> +
>  	iio_triggered_buffer_cleanup(sc->indio_dev);
> +	if (data->trig)
> +		iio_trigger_unregister(data->trig);
> +	iio_trigger_free(data->trig);
> +	iio_device_unregister(sc->indio_dev);
Nope. Device still wants to be unregistered first.  That removes
the userspace interfaces and at least reduces the chance of a race
condition.
> +
>  	kfree(sc->indio_dev);
>  	sc->indio_dev = NULL;
>  }
> 

Re: [RFC_v2 0/4] HID: hid-sony: Add IIO Suport for Motion Controllers

[Bastien Nocera]

On Tue, 2015-06-23 at 18:30 -0600, Simon Wood wrote:
> 
<snip>
> (1) https://richardstechnotes.wordpress.com/2015/06/17/rteiioimu-driving-a-9-dof-imu-via-industrial-io-iio/
> 

That URL doesn't seem to work anymore.

[RFC_v2 4/4] HID: hid-sony: Add IIO support for DualShock4 Controller

[Simon Wood]

---
 drivers/hid/hid-sony.c | 87 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 79 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index ce0526d..f1c1a16 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -62,7 +62,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 SONY_IIO_SUPPORT (SIXAXIS_CONTROLLER | DUALSHOCK4_CONTROLLER)
 
 #define MAX_LEDS 4
 
@@ -848,13 +848,16 @@ struct sony_sc {
 #if IS_BUILTIN(CONFIG_IIO) || \
 	(IS_MODULE(CONFIG_IIO) && IS_MODULE(CONFIG_HID_SONY))
 	struct iio_dev *indio_dev;
-	__u16 last_data[3];
+	__s16 last_data[6];
 };
 
 enum sony_iio_axis {
 	AXIS_ACC_X,
 	AXIS_ACC_Y,
 	AXIS_ACC_Z,
+	AXIS_GYRO_X,
+	AXIS_GYRO_Y,
+	AXIS_GYRO_Z,
 };
 
 static void sony_iio_trigger_work(struct irq_work *work);
@@ -863,7 +866,7 @@ struct sony_iio {
 	struct sony_sc *sc;
 	struct iio_trigger *trig;
 
-	u8 buff[16];		/* 3x 16-bit + padding + timestamp */
+	u8 buff[24];		/* 6x 16-bit + padding + timestamp */
 	struct irq_work work;
 #endif
 };
@@ -1095,6 +1098,25 @@ static int sony_raw_event(struct hid_device *hdev, struct hid_report *report,
 	} else if (((sc->quirks & DUALSHOCK4_CONTROLLER_USB) && rd[0] == 0x01 &&
 			size == 64) || ((sc->quirks & DUALSHOCK4_CONTROLLER_BT)
 			&& rd[0] == 0x11 && size == 78)) {
+#if IS_BUILTIN(CONFIG_IIO) || \
+	(IS_MODULE(CONFIG_IIO) && IS_MODULE(CONFIG_HID_SONY))
+		int offset = (sc->quirks & DUALSHOCK4_CONTROLLER_USB) ? 13 : 15;
+
+		sc->last_data[AXIS_ACC_X] = (rd[offset+7] << 8) + rd[offset+6];
+		sc->last_data[AXIS_ACC_Y] = (rd[offset+9] << 8) + rd[offset+8];
+		sc->last_data[AXIS_ACC_Z] = (rd[offset+11] << 8) + rd[offset+10];
+
+		sc->last_data[AXIS_GYRO_X] = (rd[offset+1] << 8) + rd[offset];
+		sc->last_data[AXIS_GYRO_Y] = (rd[offset+3] << 8) + rd[offset+2];
+		sc->last_data[AXIS_GYRO_Z] = (rd[offset+5] << 8) + rd[offset+4];
+
+		if (sc->indio_dev) {
+			struct sony_iio *data;
+
+			data = iio_priv(sc->indio_dev);
+			sony_iio_trigger_work(&data->work);
+		}
+#endif
 		dualshock4_parse_report(sc, rd, size);
 	}
 
@@ -1827,6 +1849,7 @@ static int sony_iio_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_RAW:
 		switch (chan->type) {
 		case IIO_ACCEL:
+		case IIO_ANGL_VEL:
 			*val = data->sc->last_data[chan->address];
 			return IIO_VAL_INT;
 		default:
@@ -1835,8 +1858,17 @@ static int sony_iio_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
 		case IIO_ACCEL:
-			*val = 0;	/* 9.80665/117 = 0.084540086 */
-			*val2 = 84540;
+			if (data->sc->quirks & SIXAXIS_CONTROLLER) {
+				*val = 0;	/* 9.80665/117 = 0.084540086 */
+				*val2 = 84540;
+			} else if (data->sc->quirks & DUALSHOCK4_CONTROLLER) {
+				*val = 0;	/* 9.80665/8192 = 0.001197101 */
+				*val2 = 1197;
+			}
+			return IIO_VAL_INT_PLUS_MICRO;
+		case IIO_ANGL_VEL:
+			*val = 0;	/* 0.001 */
+			*val2 = 1000;
 			return IIO_VAL_INT_PLUS_MICRO;
 		default:
 			return -EINVAL;
@@ -1844,7 +1876,13 @@ static int sony_iio_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_OFFSET:
 		switch (chan->type) {
 		case IIO_ACCEL:
-			*val = -512;
+			if (data->sc->quirks & SIXAXIS_CONTROLLER)
+				*val = -512;
+			else if (data->sc->quirks & DUALSHOCK4_CONTROLLER)
+				*val = 0;
+			return IIO_VAL_INT;
+		case IIO_ANGL_VEL:
+			*val = 0;
 			return IIO_VAL_INT;
 		default:
 			return -EINVAL;
@@ -1871,6 +1909,23 @@ static int sony_iio_read_raw(struct iio_dev *indio_dev,
 	},                                                              \
 }
 
+#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_OFFSET),                              \
+	.address = AXIS_GYRO_##_axis,                                   \
+	.scan_index = AXIS_GYRO_##_axis,                                \
+	.scan_type = {                                                  \
+		.sign = 's',                                            \
+		.realbits = 16,                                         \
+		.storagebits = 16,                                      \
+		.shift = 0,                                             \
+	},                                                              \
+}
+
 static const struct iio_chan_spec sony_sixaxis_channels[] = {
 	SONY_ACC_CHANNEL(X),
 	SONY_ACC_CHANNEL(Y),
@@ -1878,6 +1933,16 @@ static const struct iio_chan_spec sony_sixaxis_channels[] = {
 	IIO_CHAN_SOFT_TIMESTAMP(3),
 };
 
+static const struct iio_chan_spec sony_dualshock4_channels[] = {
+	SONY_ACC_CHANNEL(X),
+	SONY_ACC_CHANNEL(Y),
+	SONY_ACC_CHANNEL(Z),
+	SONY_GYRO_CHANNEL(X),
+	SONY_GYRO_CHANNEL(Y),
+	SONY_GYRO_CHANNEL(Z),
+	IIO_CHAN_SOFT_TIMESTAMP(6),
+};
+
 static const struct iio_info sony_iio_info = {
 	.read_raw = &sony_iio_read_raw,
 	.driver_module = THIS_MODULE,
@@ -1943,8 +2008,14 @@ static int sony_iio_probe(struct sony_sc *sc)
 	indio_dev->name = dev_name(&hdev->dev);
 	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
 	indio_dev->info = &sony_iio_info;
-	indio_dev->channels = sony_sixaxis_channels;
-	indio_dev->num_channels = ARRAY_SIZE(sony_sixaxis_channels);
+
+	if (sc->quirks & SIXAXIS_CONTROLLER) {
+		indio_dev->channels = sony_sixaxis_channels;
+		indio_dev->num_channels = ARRAY_SIZE(sony_sixaxis_channels);
+	} else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
+		indio_dev->channels = sony_dualshock4_channels;
+		indio_dev->num_channels = ARRAY_SIZE(sony_dualshock4_channels);
+	}
 
 	data->trig = iio_trigger_alloc("%s-dev%d", indio_dev->name,
 		indio_dev->id);
-- 
2.1.4

Re: [RFC_v2 4/4] HID: hid-sony: Add IIO support for DualShock4 Controller

[Jonathan Cameron]

On 24/06/15 01:30, Simon Wood wrote:
A couple of trivial bits and no sign off.
> ---
>  drivers/hid/hid-sony.c | 87 +++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 79 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index ce0526d..f1c1a16 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -62,7 +62,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 SONY_IIO_SUPPORT (SIXAXIS_CONTROLLER | DUALSHOCK4_CONTROLLER)
>  
>  #define MAX_LEDS 4
>  
> @@ -848,13 +848,16 @@ struct sony_sc {
>  #if IS_BUILTIN(CONFIG_IIO) || \
>  	(IS_MODULE(CONFIG_IIO) && IS_MODULE(CONFIG_HID_SONY))
>  	struct iio_dev *indio_dev;
> -	__u16 last_data[3];
> +	__s16 last_data[6];
>  };
>  
>  enum sony_iio_axis {
>  	AXIS_ACC_X,
>  	AXIS_ACC_Y,
>  	AXIS_ACC_Z,
> +	AXIS_GYRO_X,
> +	AXIS_GYRO_Y,
> +	AXIS_GYRO_Z,
>  };
>  
>  static void sony_iio_trigger_work(struct irq_work *work);
> @@ -863,7 +866,7 @@ struct sony_iio {
>  	struct sony_sc *sc;
>  	struct iio_trigger *trig;
>  
> -	u8 buff[16];		/* 3x 16-bit + padding + timestamp */
> +	u8 buff[24];		/* 6x 16-bit + padding + timestamp */
>  	struct irq_work work;
>  #endif
>  };
> @@ -1095,6 +1098,25 @@ static int sony_raw_event(struct hid_device *hdev, struct hid_report *report,
>  	} else if (((sc->quirks & DUALSHOCK4_CONTROLLER_USB) && rd[0] == 0x01 &&
>  			size == 64) || ((sc->quirks & DUALSHOCK4_CONTROLLER_BT)
>  			&& rd[0] == 0x11 && size == 78)) {
> +#if IS_BUILTIN(CONFIG_IIO) || \
> +	(IS_MODULE(CONFIG_IIO) && IS_MODULE(CONFIG_HID_SONY))
> +		int offset = (sc->quirks & DUALSHOCK4_CONTROLLER_USB) ? 13 : 15;
> +
Run checkpatch over the patches.  Spaces around the +s
> +		sc->last_data[AXIS_ACC_X] = (rd[offset+7] << 8) + rd[offset+6];
> +		sc->last_data[AXIS_ACC_Y] = (rd[offset+9] << 8) + rd[offset+8];
> +		sc->last_data[AXIS_ACC_Z] = (rd[offset+11] << 8) + rd[offset+10];
> +
> +		sc->last_data[AXIS_GYRO_X] = (rd[offset+1] << 8) + rd[offset];
> +		sc->last_data[AXIS_GYRO_Y] = (rd[offset+3] << 8) + rd[offset+2];
> +		sc->last_data[AXIS_GYRO_Z] = (rd[offset+5] << 8) + rd[offset+4];
> +
> +		if (sc->indio_dev) {
> +			struct sony_iio *data;
> +
> +			data = iio_priv(sc->indio_dev);
> +			sony_iio_trigger_work(&data->work);
> +		}
> +#endif
>  		dualshock4_parse_report(sc, rd, size);
>  	}
>  
> @@ -1827,6 +1849,7 @@ static int sony_iio_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_RAW:
>  		switch (chan->type) {
>  		case IIO_ACCEL:
> +		case IIO_ANGL_VEL:
>  			*val = data->sc->last_data[chan->address];
>  			return IIO_VAL_INT;
>  		default:
> @@ -1835,8 +1858,17 @@ static int sony_iio_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_SCALE:
>  		switch (chan->type) {
>  		case IIO_ACCEL:
> -			*val = 0;	/* 9.80665/117 = 0.084540086 */
> -			*val2 = 84540;
> +			if (data->sc->quirks & SIXAXIS_CONTROLLER) {
> +				*val = 0;	/* 9.80665/117 = 0.084540086 */
> +				*val2 = 84540;
> +			} else if (data->sc->quirks & DUALSHOCK4_CONTROLLER) {
> +				*val = 0;	/* 9.80665/8192 = 0.001197101 */
> +				*val2 = 1197;
> +			}
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		case IIO_ANGL_VEL:
> +			*val = 0;	/* 0.001 */
> +			*val2 = 1000;
>  			return IIO_VAL_INT_PLUS_MICRO;
>  		default:
>  			return -EINVAL;
> @@ -1844,7 +1876,13 @@ static int sony_iio_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_OFFSET:
>  		switch (chan->type) {
>  		case IIO_ACCEL:
> -			*val = -512;
> +			if (data->sc->quirks & SIXAXIS_CONTROLLER)
> +				*val = -512;
> +			else if (data->sc->quirks & DUALSHOCK4_CONTROLLER)
> +				*val = 0;
> +			return IIO_VAL_INT;
> +		case IIO_ANGL_VEL:
> +			*val = 0;
>  			return IIO_VAL_INT;
>  		default:
>  			return -EINVAL;
> @@ -1871,6 +1909,23 @@ static int sony_iio_read_raw(struct iio_dev *indio_dev,
>  	},                                                              \
>  }
>  
> +#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_OFFSET),                              \
> +	.address = AXIS_GYRO_##_axis,                                   \
> +	.scan_index = AXIS_GYRO_##_axis,                                \
> +	.scan_type = {                                                  \
> +		.sign = 's',                                            \
> +		.realbits = 16,                                         \
> +		.storagebits = 16,                                      \
> +		.shift = 0,                                             \
Drop the specification of shift as unnecesary.
> +	},                                                              \
> +}
> +
>  static const struct iio_chan_spec sony_sixaxis_channels[] = {
>  	SONY_ACC_CHANNEL(X),
>  	SONY_ACC_CHANNEL(Y),
> @@ -1878,6 +1933,16 @@ static const struct iio_chan_spec sony_sixaxis_channels[] = {
>  	IIO_CHAN_SOFT_TIMESTAMP(3),
>  };
>  
> +static const struct iio_chan_spec sony_dualshock4_channels[] = {
> +	SONY_ACC_CHANNEL(X),
> +	SONY_ACC_CHANNEL(Y),
> +	SONY_ACC_CHANNEL(Z),
> +	SONY_GYRO_CHANNEL(X),
> +	SONY_GYRO_CHANNEL(Y),
> +	SONY_GYRO_CHANNEL(Z),
> +	IIO_CHAN_SOFT_TIMESTAMP(6),
> +};
> +
>  static const struct iio_info sony_iio_info = {
>  	.read_raw = &sony_iio_read_raw,
>  	.driver_module = THIS_MODULE,
> @@ -1943,8 +2008,14 @@ static int sony_iio_probe(struct sony_sc *sc)
>  	indio_dev->name = dev_name(&hdev->dev);
>  	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
>  	indio_dev->info = &sony_iio_info;
> -	indio_dev->channels = sony_sixaxis_channels;
> -	indio_dev->num_channels = ARRAY_SIZE(sony_sixaxis_channels);
> +
> +	if (sc->quirks & SIXAXIS_CONTROLLER) {
> +		indio_dev->channels = sony_sixaxis_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(sony_sixaxis_channels);
> +	} else if (sc->quirks & DUALSHOCK4_CONTROLLER) {
> +		indio_dev->channels = sony_dualshock4_channels;
> +		indio_dev->num_channels = ARRAY_SIZE(sony_dualshock4_channels);
> +	}
>  
>  	data->trig = iio_trigger_alloc("%s-dev%d", indio_dev->name,
>  		indio_dev->id);
> 

Re: [RFC_v2 0/4] HID: hid-sony: Add IIO Suport for Motion Controllers

[Antonio Ospite]

On Tue, 23 Jun 2015 18:30:26 -0600
Simon Wood <simon@mungewell.org> wrote:

[...]
> The SixAxis contains accelerometers, the DS4 contains accelerometers and gyros.
>

Hi Simon,

the SixAxis also have a gyroscope, I don't know how useful/reliable it
is, but it's there.

Ciao,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Re: [RFC_v2 0/4] HID: hid-sony: Add IIO Suport for Motion Controllers

[simon-wM4F9T/ekXmXDw4h08c5KA]

> the SixAxis also have a gyroscope, I don't know how useful/reliable it
> is, but it's there.

I had a recollection that there was a gyro, but didn't see it when looking
at the 'hidraw' stream.

It might be affected by the 'multi-touch axes' bug.
Simon