RE: [PATCH 1/3] iio: hid-sensor-rotation: Add relative orientation sensor hid support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thanks Jonathan & Matt, patch have been updated.

BR
Song Hongyan

-----Original Message-----
From: Jonathan Cameron [mailto:jic23@xxxxxxxxxx] 
Sent: Saturday, May 6, 2017 2:30 AM
To: Matt Ranostay <matt.ranostay@xxxxxxxxxxxx>; Song, Hongyan <hongyan.song@xxxxxxxxx>
Cc: linux-input@xxxxxxxxxxxxxxx; open list:IIO SUBSYSTEM AND... <linux-iio@xxxxxxxxxxxxxxx>; jikos@xxxxxxxxxx; Pandruvada, Srinivas <srinivas.pandruvada@xxxxxxxxx>
Subject: Re: [PATCH 1/3] iio: hid-sensor-rotation: Add relative orientation sensor hid support

On 05/05/17 07:18, Matt Ranostay wrote:
> On Fri, May 5, 2017 at 6:39 AM, Song Hongyan <hongyan.song@xxxxxxxxx> wrote:
>> Relative orientation(AG) sensor is a 6dof orientation sensor, it 
>> depends on acceleration and gyroscope sensor data. It gives a 
>> quaternion describing the orientation of the device relative to an 
>> initial orientation. It is a standard HID sensor.
>>
>> More information can be found in:
>> http://www.usb.org/developers/hidpage/HUTRR59_-_Usages_for_Wearables.
>> pdf
>>
>> Relative orientation(AG) sensor and dev rotation sensor have same 
>> channels and share channel usage id. So the most of the code for 
>> relative orientation sensor can be reused.
>>
>> Signed-off-by: Song Hongyan <hongyan.song@xxxxxxxxx>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
>> Reviewed-by: Xu Even <even.xu@xxxxxxxxx>
>> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
>> ---
>>  drivers/iio/orientation/hid-sensor-rotation.c | 22 ++++++++++++++--------
>>  include/linux/hid-sensor-ids.h                |  1 +
>>  2 files changed, 15 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iio/orientation/hid-sensor-rotation.c 
>> b/drivers/iio/orientation/hid-sensor-rotation.c
>> index a97e802c..5f07a5a 100644
>> --- a/drivers/iio/orientation/hid-sensor-rotation.c
>> +++ b/drivers/iio/orientation/hid-sensor-rotation.c
>> @@ -218,7 +218,7 @@ static int dev_rot_parse_report(struct 
>> platform_device *pdev,  static int hid_dev_rot_probe(struct 
>> platform_device *pdev)  {
>>         int ret;
>> -       static char *name = "dev_rotation";
>> +       static char *name;
>>         struct iio_dev *indio_dev;
>>         struct dev_rot_state *rot_state;
>>         struct hid_sensor_hub_device *hsdev = 
>> pdev->dev.platform_data; @@ -234,8 +234,12 @@ static int hid_dev_rot_probe(struct platform_device *pdev)
>>         rot_state->common_attributes.hsdev = hsdev;
>>         rot_state->common_attributes.pdev = pdev;
>>
>> -       ret = hid_sensor_parse_common_attributes(hsdev,
>> -                               HID_USAGE_SENSOR_DEVICE_ORIENTATION,
>> +       if (hsdev->usage == HID_USAGE_SENSOR_DEVICE_ORIENTATION)
>> +               name = "dev_rotation";
>> +       else if (hsdev->usage == HID_USAGE_SENSOR_RELATIVE_ORIENTATION)
>> +               name = "relative_orientation";
> 
> Since you have yet another "else if" in another patch in this series..
> it would be better to have
> 
> switch (hsdev->usage) {
> case HID_USAGE_SENSOR_DEVICE_ORIENTATION:
>     name = "dev_rotation"
>     break;
> case HID_USAGE_SENSOR_RELATIVE_ORIENTATION:
>     name = "relative_orientation";
>     break;
> default:
>     **probably some error out if somehow we get here** }
> 
> 
Seconded.  Otherwise the series looks good to me!

Jonathan
> 
> 
>> +
>> +       ret = hid_sensor_parse_common_attributes(hsdev, hsdev->usage,
>>                                 &rot_state->common_attributes);
>>         if (ret) {
>>                 dev_err(&pdev->dev, "failed to setup common 
>> attributes\n"); @@ -252,8 +256,7 @@ static int 
>> hid_dev_rot_probe(struct platform_device *pdev)
>>
>>         ret = dev_rot_parse_report(pdev, hsdev,
>>                                    (struct iio_chan_spec *)indio_dev->channels,
>> -                                  HID_USAGE_SENSOR_DEVICE_ORIENTATION,
>> -                                  rot_state);
>> +                                       hsdev->usage, rot_state);
>>         if (ret) {
>>                 dev_err(&pdev->dev, "failed to setup attributes\n");
>>                 return ret;
>> @@ -288,8 +291,7 @@ static int hid_dev_rot_probe(struct platform_device *pdev)
>>         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,
>> +       ret = sensor_hub_register_callback(hsdev, hsdev->usage,
>>                                         &rot_state->callbacks);
>>         if (ret) {
>>                 dev_err(&pdev->dev, "callback reg failed\n"); @@ 
>> -314,7 +316,7 @@ static int hid_dev_rot_remove(struct platform_device *pdev)
>>         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);
>> +       sensor_hub_remove_callback(hsdev, hsdev->usage);
>>         iio_device_unregister(indio_dev);
>>         hid_sensor_remove_trigger(&rot_state->common_attributes);
>>         iio_triggered_buffer_cleanup(indio_dev);
>> @@ -327,6 +329,10 @@ static int hid_dev_rot_remove(struct platform_device *pdev)
>>                 /* Format: HID-SENSOR-usage_id_in_hex_lowercase */
>>                 .name = "HID-SENSOR-20008a",
>>         },
>> +       {
>> +               /* Relatve orientation(AG) sensor */
> 
> Slight typo. Should be "Relative"
>> +               .name = "HID-SENSOR-20008e",
>> +       },
>>         { /* sentinel */ }
>>  };
>>  MODULE_DEVICE_TABLE(platform, hid_dev_rot_ids); diff --git 
>> a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h 
>> index 30c7dc4..b61b985 100644
>> --- a/include/linux/hid-sensor-ids.h
>> +++ b/include/linux/hid-sensor-ids.h
>> @@ -82,6 +82,7 @@
>>  #define HID_USAGE_SENSOR_ORIENT_TILT_Z                         0x200481
>>
>>  #define HID_USAGE_SENSOR_DEVICE_ORIENTATION                    0x20008A
>> +#define HID_USAGE_SENSOR_RELATIVE_ORIENTATION                  0x20008E
>>  #define HID_USAGE_SENSOR_ORIENT_ROTATION_MATRIX                        0x200482
>>  #define HID_USAGE_SENSOR_ORIENT_QUATERNION                     0x200483
>>  #define HID_USAGE_SENSOR_ORIENT_MAGN_FLUX                      0x200484
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" 
>> in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo 
>> info at  http://vger.kernel.org/majordomo-info.html

��.n��������+%������w��{.n�����{��(��)��jg��������ݢj����G�������j:+v���w�m������w�������h�����٥




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux