Date:	Mon, 14 Apr 2014 08:00:40 +0100
From:	Jonathan Cameron <jic23@...nel.org>
To:	Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
CC:	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [Patch v3 5/6] iio: hid-sensors: Added device rotation support



On April 14, 2014 2:48:13 AM GMT+01:00, Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com> wrote:
>
>On 04/12/2014 10:21 AM, Jonathan Cameron wrote:
>> On 09/04/14 01:56, Srinivas Pandruvada wrote:
>>> Added usage id processing for device rotation. This uses IIO
>>> interfaces for triggered buffer to present data to user
>>> mode.This uses HID sensor framework for registering callback
>>> events from the sensor hub.
>>> Data is exported to user space in the form of quaternion rotation
>>> format.
>>>
>>> Signed-off-by: Srinivas Pandruvada
><srinivas.pandruvada@...ux.intel.com>
>> Very nice.  Happy to take this the moment we have a go ahead on
>> the devm_kmemdup and perhaps those minor tweaks I asked for in the
>> multi value handling patch.
>>
>> Actually, may seem a random question, but what the heck are the scale
>> units you can read for the quaternion?  Firstly it's an integer, so
>> would only make the value bigger, and secondly we are dealing with
>> a quaternion, which is inherently scale free (when used for rotation 
>> anyway).
>> I suppose these units might be meant to transform to a unit
>quaternion
>> (though this can be easily established form the quaternion itself).
>>
>Good point. For quaternion there is no scale exported in channel spec.
>I 
>have in switch case, which was left over, I will remove it. The offset 
>field mostly will be 0, but as you pointed out below they are unit
>exponent.
>
>
>> Looking quickly at the other HID drivers, I am a little confused as
>> to what is going on in general. Could you talk me through this stuff?
>> I have a vague recollection we went through this before, but can't
>> recall the result of those discussions.
>>
>> At first glance, it looks like scale is being used to indicate the
>> base units (via magic numbers) and offset to contain exponents
>> to be applied also as magic numbers.  Neither of these is anywhere
>> near our ABI which is going to cause issues for any 'standard'
>> userspace library..
>>
>We had this discussion long time back. Initially introduced separate 
>attributes for units ans scale but suggestion was to use offset and
>scale.
>
>Currently these drivers are used only by Android kernel, which has 
>corresponding user space to interpret.
>But now since after Win8, they became available in many devices.
>I have received questions on this from user space developers now, so I 
>have to either document them or make complaint to others. I am working 
>on it. I have many units to convert to standard format. I will submit a
>
>change for this.

Good to know you have this in hand.

Thanks

Jonathan
>
>Thanks,
>Srinivas
>
>> Thanks.
>>
>> Jonathan
>>> ---
>>>   drivers/iio/orientation/Kconfig               |  12 +
>>>   drivers/iio/orientation/Makefile              |   1 +
>>>   drivers/iio/orientation/hid-sensor-rotation.c | 359 
>>> ++++++++++++++++++++++++++
>>>   include/linux/hid-sensor-ids.h                |   1 +
>>>   4 files changed, 373 insertions(+)
>>>   create mode 100644 drivers/iio/orientation/hid-sensor-rotation.c
>>>
>>> diff --git a/drivers/iio/orientation/Kconfig 
>>> b/drivers/iio/orientation/Kconfig
>>> index 58c62c8..e3aa1e5 100644
>>> --- a/drivers/iio/orientation/Kconfig
>>> +++ b/drivers/iio/orientation/Kconfig
>>> @@ -16,4 +16,16 @@ config HID_SENSOR_INCLINOMETER_3D
>>>         Say yes here to build support for the HID SENSOR
>>>         Inclinometer 3D.
>>>
>>> +config HID_SENSOR_DEVICE_ROTATION
>>> +    depends on HID_SENSOR_HUB
>>> +    select IIO_BUFFER
>>> +    select IIO_TRIGGERED_BUFFER
>>> +    select HID_SENSOR_IIO_COMMON
>>> +    select HID_SENSOR_IIO_TRIGGER
>>> +    tristate "HID Device Rotation"
>>> +    help
>>> +      Say yes here to build support for the HID SENSOR
>>> +      device rotation. The output of a device rotation sensor
>>> +      is presented using quaternion format.
>>> +
>>>   endmenu
>>> diff --git a/drivers/iio/orientation/Makefile 
>>> b/drivers/iio/orientation/Makefile
>>> index 2c97572..4734dab 100644
>>> --- a/drivers/iio/orientation/Makefile
>>> +++ b/drivers/iio/orientation/Makefile
>>> @@ -4,3 +4,4 @@
>>>
>>>   # When adding new entries keep the list in alphabetical order
>>>   obj-$(CONFIG_HID_SENSOR_INCLINOMETER_3D) += hid-sensor-incl-3d.o
>>> +obj-$(CONFIG_HID_SENSOR_DEVICE_ROTATION) += hid-sensor-rotation.o
>>> diff --git a/drivers/iio/orientation/hid-sensor-rotation.c 
>>> b/drivers/iio/orientation/hid-sensor-rotation.c
>>> new file mode 100644
>>> index 0000000..5c7d558
>>> --- /dev/null
>>> +++ b/drivers/iio/orientation/hid-sensor-rotation.c
>>> @@ -0,0 +1,359 @@
>>> +/*
>>> + * HID Sensors Driver
>>> + * Copyright (c) 2014, Intel Corporation.
>>> + *
>>> + * This program is free software; you can redistribute it and/or 
>>> modify it
>>> + * under the terms and conditions of the GNU General Public
>License,
>>> + * version 2, as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope it will be useful, but 
>>> WITHOUT
>>> + * ANY WARRANTY; without even the implied warranty of 
>>> MERCHANTABILITY or
>>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public 
>>> License for
>>> + * more details.
>>> + */
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/module.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/irq.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/hid-sensor-hub.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 "../common/hid-sensors/hid-sensor-trigger.h"
>>> +
>>> +struct dev_rot_state {
>>> +    struct hid_sensor_hub_callbacks callbacks;
>>> +    struct hid_sensor_common common_attributes;
>>> +    struct hid_sensor_hub_attribute_info quaternion;
>>> +    u32 sampled_vals[4];
>>> +};
>>> +
>>> +/* Channel definitions */
>>> +static const struct iio_chan_spec dev_rot_channels[] = {
>>> +    {
>>> +        .type = IIO_ROT,
>>> +        .modified = 1,
>>> +        .channel2 = IIO_MOD_QUATERNION,
>>> +        .info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>>> +        .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OFFSET) |
>>> +                    BIT(IIO_CHAN_INFO_SAMP_FREQ) |
>>> +                    BIT(IIO_CHAN_INFO_HYSTERESIS) |
>>> +                    BIT(IIO_CHAN_INFO_RAW),
>>> +    }
>>> +};
>>> +
>>> +/* Adjust channel real bits based on report descriptor */
>>> +static void dev_rot_adjust_channel_bit_mask(struct iio_chan_spec
>*chan,
>>> +                        int size)
>>> +{
>>> +    chan->scan_type.sign = 's';
>>> +    /* Real storage bits will change based on the report desc. */
>>> +    chan->scan_type.realbits = size * 8;
>>> +    /* Maximum size of a sample to capture is u32 */
>>> +    chan->scan_type.storagebits = sizeof(u32) * 8;
>>> +    chan->scan_type.repeat = 4;
>>> +}
>>> +
>>> +/* Channel read_raw handler */
>>> +static int dev_rot_read_raw(struct iio_dev *indio_dev,
>>> +                struct iio_chan_spec const *chan,
>>> +                int size, int *vals, int *val_len,
>>> +                long mask)
>>> +{
>>> +    struct dev_rot_state *rot_state = iio_priv(indio_dev);
>>> +    int ret_type;
>>> +    int i;
>>> +
>>> +    vals[0] = 0;
>>> +    vals[1] = 0;
>>> +
>>> +    switch (mask) {
>>> +    case IIO_CHAN_INFO_RAW:
>>> +        if (size >= 4) {
>>> +            for (i = 0; i < 4; ++i)
>>> +                vals[i] = rot_state->sampled_vals[i];
>>> +            ret_type = IIO_VAL_INT_MULTIPLE;
>>> +            *val_len =  4;
>>> +        } else
>>> +            ret_type = -EINVAL;
>>> +        break;
>>> +    case IIO_CHAN_INFO_SCALE:
>>> +        vals[0] = rot_state->quaternion.units;
>>> +        ret_type = IIO_VAL_INT;
>>> +        break;
>>> +    case IIO_CHAN_INFO_OFFSET:
>>> +        vals[1] = hid_sensor_convert_exponent(
>>> +            rot_state->quaternion.unit_expo);
>>> +        ret_type = IIO_VAL_INT;
>>> +        break;
>>> +    case IIO_CHAN_INFO_SAMP_FREQ:
>>> +        ret_type = hid_sensor_read_samp_freq_value(
>>> +            &rot_state->common_attributes, &vals[0], &vals[1]);
>>> +        break;
>>> +    case IIO_CHAN_INFO_HYSTERESIS:
>>> +        ret_type = hid_sensor_read_raw_hyst_value(
>>> +            &rot_state->common_attributes, &vals[0], &vals[1]);
>>> +        break;
>>> +    default:
>>> +        ret_type = -EINVAL;
>>> +        break;
>>> +    }
>>> +
>>> +    return ret_type;
>>> +}
>>> +
>>> +/* Channel write_raw handler */
>>> +static int dev_rot_write_raw(struct iio_dev *indio_dev,
>>> +                   struct iio_chan_spec const *chan,
>>> +                   int val,
>>> +                   int val2,
>>> +                   long mask)
>>> +{
>>> +    struct dev_rot_state *rot_state = iio_priv(indio_dev);
>>> +    int ret;
>>> +
>>> +    switch (mask) {
>>> +    case IIO_CHAN_INFO_SAMP_FREQ:
>>> +        ret = hid_sensor_write_samp_freq_value(
>>> +                &rot_state->common_attributes, val, val2);
>>> +        break;
>>> +    case IIO_CHAN_INFO_HYSTERESIS:
>>> +        ret = hid_sensor_write_raw_hyst_value(
>>> +                &rot_state->common_attributes, val, val2);
>>> +        break;
>>> +    default:
>>> +        ret = -EINVAL;
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>> +static const struct iio_info dev_rot_info = {
>>> +    .driver_module = THIS_MODULE,
>>> +    .read_raw_multi = &dev_rot_read_raw,
>>> +    .write_raw = &dev_rot_write_raw,
>>> +};
>>> +
>>> +/* Function to push data to buffer */
>>> +static void hid_sensor_push_data(struct iio_dev *indio_dev, u8 
>>> *data, int len)
>>> +{
>>> +    dev_dbg(&indio_dev->dev, "hid_sensor_push_data >>\n");
>>> +    iio_push_to_buffers(indio_dev, (u8 *)data);
>>> +    dev_dbg(&indio_dev->dev, "hid_sensor_push_data <<\n");
>>> +
>>> +}
>>> +
>>> +/* Callback handler to send event after all samples are received
>and 
>>> captured */
>>> +static int dev_rot_proc_event(struct hid_sensor_hub_device *hsdev,
>>> +                unsigned usage_id,
>>> +                void *priv)
>>> +{
>>> +    struct iio_dev *indio_dev = platform_get_drvdata(priv);
>>> +    struct dev_rot_state *rot_state = iio_priv(indio_dev);
>>> +
>>> +    dev_dbg(&indio_dev->dev, "dev_rot_proc_event [%d]\n",
>>> +                rot_state->common_attributes.data_ready);
>>> +
>>> +    if (rot_state->common_attributes.data_ready)
>>> +        hid_sensor_push_data(indio_dev,
>>> +                (u8 *)rot_state->sampled_vals,
>>> +                sizeof(rot_state->sampled_vals));
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/* Capture samples in local storage */
>>> +static int dev_rot_capture_sample(struct hid_sensor_hub_device
>*hsdev,
>>> +                unsigned usage_id,
>>> +                size_t raw_len, char *raw_data,
>>> +                void *priv)
>>> +{
>>> +    struct iio_dev *indio_dev = platform_get_drvdata(priv);
>>> +    struct dev_rot_state *rot_state = iio_priv(indio_dev);
>>> +
>>> +    if (usage_id == HID_USAGE_SENSOR_ORIENT_QUATERNION) {
>>> +        memcpy(rot_state->sampled_vals, raw_data,
>>> +                    sizeof(rot_state->sampled_vals));
>>> +        dev_dbg(&indio_dev->dev, "Recd Quat len:%zu::%zu\n",
>raw_len,
>>> +                    sizeof(rot_state->sampled_vals));
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/* Parse report which is specific to an usage id*/
>>> +static int dev_rot_parse_report(struct platform_device *pdev,
>>> +                struct hid_sensor_hub_device *hsdev,
>>> +                struct iio_chan_spec *channels,
>>> +                unsigned usage_id,
>>> +                struct dev_rot_state *st)
>>> +{
>>> +    int ret;
>>> +
>>> +    ret = sensor_hub_input_get_attribute_info(hsdev,
>>> +                HID_INPUT_REPORT,
>>> +                usage_id,
>>> +                HID_USAGE_SENSOR_ORIENT_QUATERNION,
>>> +                &st->quaternion);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    dev_rot_adjust_channel_bit_mask(&channels[0],
>>> +        st->quaternion.size / 4);
>>> +
>>> +    dev_dbg(&pdev->dev, "dev_rot %x:%x\n", st->quaternion.index,
>>> +        st->quaternion.report_id);
>>> +
>>> +    dev_dbg(&pdev->dev, "dev_rot: attrib size %d\n",
>>> +                st->quaternion.size);
>>> +
>>> +    /* Set Sensitivity field ids, when there is no individual 
>>> modifier */
>>> +    if (st->common_attributes.sensitivity.index < 0) {
>>> +        sensor_hub_input_get_attribute_info(hsdev,
>>> +            HID_FEATURE_REPORT, usage_id,
>>> +            HID_USAGE_SENSOR_DATA_MOD_CHANGE_SENSITIVITY_ABS |
>>> +            HID_USAGE_SENSOR_DATA_ORIENTATION,
>>> +            &st->common_attributes.sensitivity);
>>> +        dev_dbg(&pdev->dev, "Sensitivity index:report %d:%d\n",
>>> +            st->common_attributes.sensitivity.index,
>>> +            st->common_attributes.sensitivity.report_id);
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +/* Function to initialize the processing for usage id */
>>> +static int hid_dev_rot_probe(struct platform_device *pdev)
>>> +{
>>> +    int ret;
>>> +    static char *name = "dev_rotation";
>>> +    struct iio_dev *indio_dev;
>>> +    struct dev_rot_state *rot_state;
>>> +    struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
>>> +    struct iio_chan_spec *channels;
>>> +
>>> +    indio_dev = devm_iio_device_alloc(&pdev->dev,
>>> +                      sizeof(struct dev_rot_state));
>>> +    if (indio_dev == NULL)
>>> +        return -ENOMEM;
>>> +
>>> +    platform_set_drvdata(pdev, indio_dev);
>>> +
>>> +    rot_state = iio_priv(indio_dev);
>>> +    rot_state->common_attributes.hsdev = hsdev;
>>> +    rot_state->common_attributes.pdev = pdev;
>>> +
>>> +    ret = hid_sensor_parse_common_attributes(hsdev,
>>> +                HID_USAGE_SENSOR_DEVICE_ORIENTATION,
>>> +                &rot_state->common_attributes);
>>> +    if (ret) {
>>> +        dev_err(&pdev->dev, "failed to setup common attributes\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    channels = devm_kmemdup(&pdev->dev, dev_rot_channels, 
>>> sizeof(dev_rot_channels),
>>> +               GFP_KERNEL);
>>> +    if (!channels) {
>>> +        dev_err(&pdev->dev, "failed to duplicate channels\n");
>>> +        return -ENOMEM;
>>> +    }
>>> +
>>> +    ret = dev_rot_parse_report(pdev, hsdev, channels,
>>> +            HID_USAGE_SENSOR_DEVICE_ORIENTATION, rot_state);
>>> +    if (ret) {
>>> +        dev_err(&pdev->dev, "failed to setup attributes\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    indio_dev->channels = channels;
>>> +    indio_dev->num_channels = ARRAY_SIZE(dev_rot_channels);
>>> +    indio_dev->dev.parent = &pdev->dev;
>>> +    indio_dev->info = &dev_rot_info;
>>> +    indio_dev->name = name;
>>> +    indio_dev->modes = INDIO_DIRECT_MODE;
>>> +
>>> +    ret = iio_triggered_buffer_setup(indio_dev, 
>>> &iio_pollfunc_store_time,
>>> +        NULL, NULL);
>>> +    if (ret) {
>>> +        dev_err(&pdev->dev, "failed to initialize trigger
>buffer\n");
>>> +        return ret;
>>> +    }
>>> +    rot_state->common_attributes.data_ready = false;
>>> +    ret = hid_sensor_setup_trigger(indio_dev, name,
>>> +                    &rot_state->common_attributes);
>>> +    if (ret) {
>>> +        dev_err(&pdev->dev, "trigger setup failed\n");
>>> +        goto error_unreg_buffer_funcs;
>>> +    }
>>> +
>>> +    ret = iio_device_register(indio_dev);
>>> +    if (ret) {
>>> +        dev_err(&pdev->dev, "device register failed\n");
>>> +        goto error_remove_trigger;
>>> +    }
>>> +
>>> +    rot_state->callbacks.send_event = dev_rot_proc_event;
>>> +    rot_state->callbacks.capture_sample = dev_rot_capture_sample;
>>> +    rot_state->callbacks.pdev = pdev;
>>> +    ret = sensor_hub_register_callback(hsdev,
>>> +                    HID_USAGE_SENSOR_DEVICE_ORIENTATION,
>>> +                    &rot_state->callbacks);
>>> +    if (ret) {
>>> +        dev_err(&pdev->dev, "callback reg failed\n");
>>> +        goto error_iio_unreg;
>>> +    }
>>> +
>>> +    return 0;
>>> +
>>> +error_iio_unreg:
>>> +    iio_device_unregister(indio_dev);
>>> +error_remove_trigger:
>>> + hid_sensor_remove_trigger(&rot_state->common_attributes);
>>> +error_unreg_buffer_funcs:
>>> +    iio_triggered_buffer_cleanup(indio_dev);
>>> +    return ret;
>>> +}
>>> +
>>> +/* Function to deinitialize the processing for usage id */
>>> +static int hid_dev_rot_remove(struct platform_device *pdev)
>>> +{
>>> +    struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
>>> +    struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>>> +    struct dev_rot_state *rot_state = iio_priv(indio_dev);
>>> +
>>> +    sensor_hub_remove_callback(hsdev, 
>>> HID_USAGE_SENSOR_DEVICE_ORIENTATION);
>>> +    iio_device_unregister(indio_dev);
>>> + hid_sensor_remove_trigger(&rot_state->common_attributes);
>>> +    iio_triggered_buffer_cleanup(indio_dev);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static struct platform_device_id hid_dev_rot_ids[] = {
>>> +    {
>>> +        /* Format: HID-SENSOR-usage_id_in_hex_lowercase */
>>> +        .name = "HID-SENSOR-20008a",
>>> +    },
>>> +    { /* sentinel */ }
>>> +};
>>> +MODULE_DEVICE_TABLE(platform, hid_dev_rot_ids);
>>> +
>>> +static struct platform_driver hid_dev_rot_platform_driver = {
>>> +    .id_table = hid_dev_rot_ids,
>>> +    .driver = {
>>> +        .name    = KBUILD_MODNAME,
>>> +        .owner    = THIS_MODULE,
>>> +    },
>>> +    .probe        = hid_dev_rot_probe,
>>> +    .remove        = hid_dev_rot_remove,
>>> +};
>>> +module_platform_driver(hid_dev_rot_platform_driver);
>>> +
>>> +MODULE_DESCRIPTION("HID Sensor Device Rotation");
>>> +MODULE_AUTHOR("Srinivas Pandruvada 
>>> <srinivas.pandruvada@...ux.intel.com>");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/include/linux/hid-sensor-ids.h 
>>> b/include/linux/hid-sensor-ids.h
>>> index 14ead9e..109f0e6 100644
>>> --- a/include/linux/hid-sensor-ids.h
>>> +++ b/include/linux/hid-sensor-ids.h
>>> @@ -76,6 +76,7 @@
>>>   #define HID_USAGE_SENSOR_ORIENT_TILT_Y                0x200480
>>>   #define HID_USAGE_SENSOR_ORIENT_TILT_Z                0x200481
>>>
>>> +#define HID_USAGE_SENSOR_DEVICE_ORIENTATION            0x20008A
>>>   #define HID_USAGE_SENSOR_ORIENT_ROTATION_MATRIX 0x200482
>>>   #define HID_USAGE_SENSOR_ORIENT_QUATERNION            0x200483
>>>   #define HID_USAGE_SENSOR_ORIENT_MAGN_FLUX            0x200484
>>>
>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>in
>> the body of a message to majordomo@...r.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists