* [PATCH] iio: hid-sensors: Fix power and report state
@ 2013-10-08 21:32 Srinivas Pandruvada
2013-10-14 22:23 ` Jonathan Cameron
0 siblings, 1 reply; 11+ messages in thread
From: Srinivas Pandruvada @ 2013-10-08 21:32 UTC (permalink / raw)
To: linux-iio; +Cc: jic23, Srinivas Pandruvada
In the original HID sensor specifications all Named array enums stated
to be 0-based. But the most commercial devices implemented as 1-based,
because of the implementation by one of the major OS vendor. To fix this
we added a quirk, which required module to be recompiled. Instead now
added a module parameter, so that it can be switched at runtime.
By default it will be 1-based to be compatible with majority of devices.
This is true for both power and report state usage id for hid sensors.
Also added defines for power on values for D0 to D4 and using it for
clarity.
Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
drivers/iio/common/hid-sensors/Kconfig | 9 --------
.../iio/common/hid-sensors/hid-sensor-trigger.c | 26 +++++++++++++++++-----
include/linux/hid-sensor-ids.h | 12 ++++++++++
3 files changed, 33 insertions(+), 14 deletions(-)
diff --git a/drivers/iio/common/hid-sensors/Kconfig b/drivers/iio/common/hid-sensors/Kconfig
index 1178121..39188b7 100644
--- a/drivers/iio/common/hid-sensors/Kconfig
+++ b/drivers/iio/common/hid-sensors/Kconfig
@@ -25,13 +25,4 @@ config HID_SENSOR_IIO_TRIGGER
If this driver is compiled as a module, it will be named
hid-sensor-trigger.
-config HID_SENSOR_ENUM_BASE_QUIRKS
- bool "ENUM base quirks for HID Sensor IIO drivers"
- depends on HID_SENSOR_IIO_COMMON
- help
- Say yes here to build support for sensor hub FW using
- enumeration, which is using 1 as base instead of 0.
- Since logical minimum is still set 0 instead of 1,
- there is no easy way to differentiate.
-
endmenu
diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
index b6e77e0..dbc9141 100644
--- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
+++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
@@ -28,21 +28,37 @@
#include <linux/iio/sysfs.h>
#include "hid-sensor-trigger.h"
+#define HID_SENSOR_NAMED_ARRAY_BASE 1
+static int hid_sensor_named_array_base = HID_SENSOR_NAMED_ARRAY_BASE;
+module_param(hid_sensor_named_array_base, int, 0644);
+MODULE_PARM_DESC(hid_sensor_named_array_base,
+ "HID SENSOR NAMED ARRAY BASE (0 or 1).");
+
static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
bool state)
{
struct hid_sensor_common *st = iio_trigger_get_drvdata(trig);
int state_val;
+ int report_state;
if (state) {
if (sensor_hub_device_open(st->hsdev))
return -EIO;
- } else
+ state_val =
+ HID_USAGE_SENSOR_PROP_POWER_STATE_D0_FULL_POWER_ENUM;
+ report_state =
+ HID_USAGE_SENSOR_PROP_REPORTING_STATE_ALL_EVENTS_ENUM;
+ } else {
sensor_hub_device_close(st->hsdev);
-
- state_val = state ? 1 : 0;
- if (IS_ENABLED(CONFIG_HID_SENSOR_ENUM_BASE_QUIRKS))
+ state_val =
+ HID_USAGE_SENSOR_PROP_POWER_STATE_D4_POWER_OFF_ENUM;
+ report_state =
+ HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVENTS_ENUM;
+ }
+ if (hid_sensor_named_array_base) {
++state_val;
+ ++report_state;
+ }
st->data_ready = state;
sensor_hub_set_feature(st->hsdev, st->power_state.report_id,
st->power_state.index,
@@ -50,7 +66,7 @@ static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
sensor_hub_set_feature(st->hsdev, st->report_state.report_id,
st->report_state.index,
- (s32)state_val);
+ (s32)report_state);
return 0;
}
diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
index 4f945d3..8323775 100644
--- a/include/linux/hid-sensor-ids.h
+++ b/include/linux/hid-sensor-ids.h
@@ -117,4 +117,16 @@
#define HID_USAGE_SENSOR_PROP_REPORT_STATE 0x200316
#define HID_USAGE_SENSOR_PROY_POWER_STATE 0x200319
+/* Power state enumerations */
+#define HID_USAGE_SENSOR_PROP_POWER_STATE_UNDEFINED_ENUM 0x00
+#define HID_USAGE_SENSOR_PROP_POWER_STATE_D0_FULL_POWER_ENUM 0x01
+#define HID_USAGE_SENSOR_PROP_POWER_STATE_D1_LOW_POWER_ENUM 0x02
+#define HID_USAGE_SENSOR_PROP_POWER_STATE_D2_STANDBY_WITH_WAKE_ENUM 0x03
+#define HID_USAGE_SENSOR_PROP_POWER_STATE_D3_SLEEP_WITH_WAKE_ENUM 0x04
+#define HID_USAGE_SENSOR_PROP_POWER_STATE_D4_POWER_OFF_ENUM 0x05
+
+/* Report State enumerations */
+#define HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVENTS_ENUM 0x00
+#define HID_USAGE_SENSOR_PROP_REPORTING_STATE_ALL_EVENTS_ENUM 0x01
+
#endif
--
1.8.3.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH] iio: hid-sensors: Fix power and report state
2013-10-08 21:32 [PATCH] iio: hid-sensors: Fix power and report state Srinivas Pandruvada
@ 2013-10-14 22:23 ` Jonathan Cameron
2013-10-14 22:27 ` Srinivas Pandruvada
0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Cameron @ 2013-10-14 22:23 UTC (permalink / raw)
To: Srinivas Pandruvada, linux-iio
On 10/08/13 22:32, Srinivas Pandruvada wrote:
> In the original HID sensor specifications all Named array enums stated
> to be 0-based. But the most commercial devices implemented as 1-based,
> because of the implementation by one of the major OS vendor. To fix this
> we added a quirk, which required module to be recompiled. Instead now
> added a module parameter, so that it can be switched at runtime.
> By default it will be 1-based to be compatible with majority of devices.
> This is true for both power and report state usage id for hid sensors.
> Also added defines for power on values for D0 to D4 and using it for
> clarity.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
This looks fine but raises a few questions...
What other options do we have for controlling this? Are hid sensors
chips identifiable? If so can we have a list of chips where it is 0 in
the driver. The module parameter might still be needed to deal with new
devices but would be nice to have most work out of the box.
Got to love it when the quirk becomes the default ;)
> ---
> drivers/iio/common/hid-sensors/Kconfig | 9 --------
> .../iio/common/hid-sensors/hid-sensor-trigger.c | 26 +++++++++++++++++-----
> include/linux/hid-sensor-ids.h | 12 ++++++++++
> 3 files changed, 33 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/iio/common/hid-sensors/Kconfig b/drivers/iio/common/hid-sensors/Kconfig
> index 1178121..39188b7 100644
> --- a/drivers/iio/common/hid-sensors/Kconfig
> +++ b/drivers/iio/common/hid-sensors/Kconfig
> @@ -25,13 +25,4 @@ config HID_SENSOR_IIO_TRIGGER
> If this driver is compiled as a module, it will be named
> hid-sensor-trigger.
>
> -config HID_SENSOR_ENUM_BASE_QUIRKS
> - bool "ENUM base quirks for HID Sensor IIO drivers"
> - depends on HID_SENSOR_IIO_COMMON
> - help
> - Say yes here to build support for sensor hub FW using
> - enumeration, which is using 1 as base instead of 0.
> - Since logical minimum is still set 0 instead of 1,
> - there is no easy way to differentiate.
> -
> endmenu
> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> index b6e77e0..dbc9141 100644
> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
> @@ -28,21 +28,37 @@
> #include <linux/iio/sysfs.h>
> #include "hid-sensor-trigger.h"
>
> +#define HID_SENSOR_NAMED_ARRAY_BASE 1
> +static int hid_sensor_named_array_base = HID_SENSOR_NAMED_ARRAY_BASE;
> +module_param(hid_sensor_named_array_base, int, 0644);
> +MODULE_PARM_DESC(hid_sensor_named_array_base,
> + "HID SENSOR NAMED ARRAY BASE (0 or 1).");
> +
> static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
> bool state)
> {
> struct hid_sensor_common *st = iio_trigger_get_drvdata(trig);
> int state_val;
> + int report_state;
>
> if (state) {
> if (sensor_hub_device_open(st->hsdev))
> return -EIO;
> - } else
> + state_val =
> + HID_USAGE_SENSOR_PROP_POWER_STATE_D0_FULL_POWER_ENUM;
> + report_state =
> + HID_USAGE_SENSOR_PROP_REPORTING_STATE_ALL_EVENTS_ENUM;
> + } else {
> sensor_hub_device_close(st->hsdev);
> -
> - state_val = state ? 1 : 0;
> - if (IS_ENABLED(CONFIG_HID_SENSOR_ENUM_BASE_QUIRKS))
> + state_val =
> + HID_USAGE_SENSOR_PROP_POWER_STATE_D4_POWER_OFF_ENUM;
> + report_state =
> + HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVENTS_ENUM;
> + }
> + if (hid_sensor_named_array_base) {
> ++state_val;
> + ++report_state;
> + }
> st->data_ready = state;
> sensor_hub_set_feature(st->hsdev, st->power_state.report_id,
> st->power_state.index,
> @@ -50,7 +66,7 @@ static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
>
> sensor_hub_set_feature(st->hsdev, st->report_state.report_id,
> st->report_state.index,
> - (s32)state_val);
> + (s32)report_state);
>
> return 0;
> }
> diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
> index 4f945d3..8323775 100644
> --- a/include/linux/hid-sensor-ids.h
> +++ b/include/linux/hid-sensor-ids.h
> @@ -117,4 +117,16 @@
> #define HID_USAGE_SENSOR_PROP_REPORT_STATE 0x200316
> #define HID_USAGE_SENSOR_PROY_POWER_STATE 0x200319
>
> +/* Power state enumerations */
> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_UNDEFINED_ENUM 0x00
> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D0_FULL_POWER_ENUM 0x01
> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D1_LOW_POWER_ENUM 0x02
> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D2_STANDBY_WITH_WAKE_ENUM 0x03
> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D3_SLEEP_WITH_WAKE_ENUM 0x04
> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D4_POWER_OFF_ENUM 0x05
> +
> +/* Report State enumerations */
> +#define HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVENTS_ENUM 0x00
> +#define HID_USAGE_SENSOR_PROP_REPORTING_STATE_ALL_EVENTS_ENUM 0x01
> +
> #endif
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] iio: hid-sensors: Fix power and report state
2013-10-14 22:23 ` Jonathan Cameron
@ 2013-10-14 22:27 ` Srinivas Pandruvada
2013-10-15 18:58 ` Jonathan Cameron
0 siblings, 1 reply; 11+ messages in thread
From: Srinivas Pandruvada @ 2013-10-14 22:27 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio
On 10/14/2013 03:23 PM, Jonathan Cameron wrote:
> On 10/08/13 22:32, Srinivas Pandruvada wrote:
>> In the original HID sensor specifications all Named array enums stated
>> to be 0-based. But the most commercial devices implemented as 1-based,
>> because of the implementation by one of the major OS vendor. To fix this
>> we added a quirk, which required module to be recompiled. Instead now
>> added a module parameter, so that it can be switched at runtime.
>> By default it will be 1-based to be compatible with majority of devices.
>> This is true for both power and report state usage id for hid sensors.
>> Also added defines for power on values for D0 to D4 and using it for
>> clarity.
>>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> This looks fine but raises a few questions...
>
> What other options do we have for controlling this? Are hid sensors
> chips identifiable? If so can we have a list of chips where it is 0 in
> the driver. The module parameter might still be needed to deal with new
> devices but would be nice to have most work out of the box.
We could have quirk based on vendor id/pid. But the problem is that once
they update FW to be compliant with WIN8, it will not work.
So there is no way to distinguish. Since sensor hub is present in most
of WIN8 convertible devices, the quirk became a new normal.
> Got to love it when the quirk becomes the default ;)
Unfortunately they don't go back to specification update after such change.
>> ---
>> drivers/iio/common/hid-sensors/Kconfig | 9 --------
>> .../iio/common/hid-sensors/hid-sensor-trigger.c | 26 +++++++++++++++++-----
>> include/linux/hid-sensor-ids.h | 12 ++++++++++
>> 3 files changed, 33 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/iio/common/hid-sensors/Kconfig b/drivers/iio/common/hid-sensors/Kconfig
>> index 1178121..39188b7 100644
>> --- a/drivers/iio/common/hid-sensors/Kconfig
>> +++ b/drivers/iio/common/hid-sensors/Kconfig
>> @@ -25,13 +25,4 @@ config HID_SENSOR_IIO_TRIGGER
>> If this driver is compiled as a module, it will be named
>> hid-sensor-trigger.
>>
>> -config HID_SENSOR_ENUM_BASE_QUIRKS
>> - bool "ENUM base quirks for HID Sensor IIO drivers"
>> - depends on HID_SENSOR_IIO_COMMON
>> - help
>> - Say yes here to build support for sensor hub FW using
>> - enumeration, which is using 1 as base instead of 0.
>> - Since logical minimum is still set 0 instead of 1,
>> - there is no easy way to differentiate.
>> -
>> endmenu
>> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>> index b6e77e0..dbc9141 100644
>> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>> @@ -28,21 +28,37 @@
>> #include <linux/iio/sysfs.h>
>> #include "hid-sensor-trigger.h"
>>
>> +#define HID_SENSOR_NAMED_ARRAY_BASE 1
>> +static int hid_sensor_named_array_base = HID_SENSOR_NAMED_ARRAY_BASE;
>> +module_param(hid_sensor_named_array_base, int, 0644);
>> +MODULE_PARM_DESC(hid_sensor_named_array_base,
>> + "HID SENSOR NAMED ARRAY BASE (0 or 1).");
>> +
>> static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
>> bool state)
>> {
>> struct hid_sensor_common *st = iio_trigger_get_drvdata(trig);
>> int state_val;
>> + int report_state;
>>
>> if (state) {
>> if (sensor_hub_device_open(st->hsdev))
>> return -EIO;
>> - } else
>> + state_val =
>> + HID_USAGE_SENSOR_PROP_POWER_STATE_D0_FULL_POWER_ENUM;
>> + report_state =
>> + HID_USAGE_SENSOR_PROP_REPORTING_STATE_ALL_EVENTS_ENUM;
>> + } else {
>> sensor_hub_device_close(st->hsdev);
>> -
>> - state_val = state ? 1 : 0;
>> - if (IS_ENABLED(CONFIG_HID_SENSOR_ENUM_BASE_QUIRKS))
>> + state_val =
>> + HID_USAGE_SENSOR_PROP_POWER_STATE_D4_POWER_OFF_ENUM;
>> + report_state =
>> + HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVENTS_ENUM;
>> + }
>> + if (hid_sensor_named_array_base) {
>> ++state_val;
>> + ++report_state;
>> + }
>> st->data_ready = state;
>> sensor_hub_set_feature(st->hsdev, st->power_state.report_id,
>> st->power_state.index,
>> @@ -50,7 +66,7 @@ static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
>>
>> sensor_hub_set_feature(st->hsdev, st->report_state.report_id,
>> st->report_state.index,
>> - (s32)state_val);
>> + (s32)report_state);
>>
>> return 0;
>> }
>> diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
>> index 4f945d3..8323775 100644
>> --- a/include/linux/hid-sensor-ids.h
>> +++ b/include/linux/hid-sensor-ids.h
>> @@ -117,4 +117,16 @@
>> #define HID_USAGE_SENSOR_PROP_REPORT_STATE 0x200316
>> #define HID_USAGE_SENSOR_PROY_POWER_STATE 0x200319
>>
>> +/* Power state enumerations */
>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_UNDEFINED_ENUM 0x00
>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D0_FULL_POWER_ENUM 0x01
>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D1_LOW_POWER_ENUM 0x02
>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D2_STANDBY_WITH_WAKE_ENUM 0x03
>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D3_SLEEP_WITH_WAKE_ENUM 0x04
>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D4_POWER_OFF_ENUM 0x05
>> +
>> +/* Report State enumerations */
>> +#define HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVENTS_ENUM 0x00
>> +#define HID_USAGE_SENSOR_PROP_REPORTING_STATE_ALL_EVENTS_ENUM 0x01
>> +
>> #endif
>>
Thanks,
Srinivas
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] iio: hid-sensors: Fix power and report state
2013-10-14 22:27 ` Srinivas Pandruvada
@ 2013-10-15 18:58 ` Jonathan Cameron
2013-10-15 18:34 ` Srinivas Pandruvada
0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Cameron @ 2013-10-15 18:58 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: linux-iio
On 10/14/13 23:27, Srinivas Pandruvada wrote:
> On 10/14/2013 03:23 PM, Jonathan Cameron wrote:
>> On 10/08/13 22:32, Srinivas Pandruvada wrote:
>>> In the original HID sensor specifications all Named array enums stated
>>> to be 0-based. But the most commercial devices implemented as 1-based,
>>> because of the implementation by one of the major OS vendor. To fix this
>>> we added a quirk, which required module to be recompiled. Instead now
>>> added a module parameter, so that it can be switched at runtime.
>>> By default it will be 1-based to be compatible with majority of devices.
>>> This is true for both power and report state usage id for hid sensors.
>>> Also added defines for power on values for D0 to D4 and using it for
>>> clarity.
>>>
>>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>> This looks fine but raises a few questions...
>>
>> What other options do we have for controlling this? Are hid sensors
>> chips identifiable? If so can we have a list of chips where it is 0 in
>> the driver. The module parameter might still be needed to deal with new
>> devices but would be nice to have most work out of the box.
> We could have quirk based on vendor id/pid. But the problem is that once they update FW to be compliant with WIN8, it
> will not work.
> So there is no way to distinguish. Since sensor hub is present in most of WIN8 convertible devices, the quirk became a
> new normal.
Yuck. You have my sympathies!
>> Got to love it when the quirk becomes the default ;)
> Unfortunately they don't go back to specification update after such change.
Just to check, is this quirk only hid-sensors related? Named arrays are
as far as I can see a part of HID in general.
>>> ---
>>> drivers/iio/common/hid-sensors/Kconfig | 9 --------
>>> .../iio/common/hid-sensors/hid-sensor-trigger.c | 26 +++++++++++++++++-----
>>> include/linux/hid-sensor-ids.h | 12 ++++++++++
>>> 3 files changed, 33 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/iio/common/hid-sensors/Kconfig b/drivers/iio/common/hid-sensors/Kconfig
>>> index 1178121..39188b7 100644
>>> --- a/drivers/iio/common/hid-sensors/Kconfig
>>> +++ b/drivers/iio/common/hid-sensors/Kconfig
>>> @@ -25,13 +25,4 @@ config HID_SENSOR_IIO_TRIGGER
>>> If this driver is compiled as a module, it will be named
>>> hid-sensor-trigger.
>>> -config HID_SENSOR_ENUM_BASE_QUIRKS
>>> - bool "ENUM base quirks for HID Sensor IIO drivers"
>>> - depends on HID_SENSOR_IIO_COMMON
>>> - help
>>> - Say yes here to build support for sensor hub FW using
>>> - enumeration, which is using 1 as base instead of 0.
>>> - Since logical minimum is still set 0 instead of 1,
>>> - there is no easy way to differentiate.
>>> -
>>> endmenu
>>> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>> index b6e77e0..dbc9141 100644
>>> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>> @@ -28,21 +28,37 @@
>>> #include <linux/iio/sysfs.h>
>>> #include "hid-sensor-trigger.h"
>>> +#define HID_SENSOR_NAMED_ARRAY_BASE 1
>>> +static int hid_sensor_named_array_base = HID_SENSOR_NAMED_ARRAY_BASE;
>>> +module_param(hid_sensor_named_array_base, int, 0644);
>>> +MODULE_PARM_DESC(hid_sensor_named_array_base,
>>> + "HID SENSOR NAMED ARRAY BASE (0 or 1).");
>>> +
>>> static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
>>> bool state)
>>> {
>>> struct hid_sensor_common *st = iio_trigger_get_drvdata(trig);
>>> int state_val;
>>> + int report_state;
>>> if (state) {
>>> if (sensor_hub_device_open(st->hsdev))
>>> return -EIO;
>>> - } else
>>> + state_val =
>>> + HID_USAGE_SENSOR_PROP_POWER_STATE_D0_FULL_POWER_ENUM;
>>> + report_state =
>>> + HID_USAGE_SENSOR_PROP_REPORTING_STATE_ALL_EVENTS_ENUM;
>>> + } else {
>>> sensor_hub_device_close(st->hsdev);
>>> -
>>> - state_val = state ? 1 : 0;
>>> - if (IS_ENABLED(CONFIG_HID_SENSOR_ENUM_BASE_QUIRKS))
>>> + state_val =
>>> + HID_USAGE_SENSOR_PROP_POWER_STATE_D4_POWER_OFF_ENUM;
>>> + report_state =
>>> + HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVENTS_ENUM;
>>> + }
>>> + if (hid_sensor_named_array_base) {
>>> ++state_val;
>>> + ++report_state;
>>> + }
>>> st->data_ready = state;
>>> sensor_hub_set_feature(st->hsdev, st->power_state.report_id,
>>> st->power_state.index,
>>> @@ -50,7 +66,7 @@ static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
>>> sensor_hub_set_feature(st->hsdev, st->report_state.report_id,
>>> st->report_state.index,
>>> - (s32)state_val);
>>> + (s32)report_state);
>>> return 0;
>>> }
>>> diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
>>> index 4f945d3..8323775 100644
>>> --- a/include/linux/hid-sensor-ids.h
>>> +++ b/include/linux/hid-sensor-ids.h
>>> @@ -117,4 +117,16 @@
>>> #define HID_USAGE_SENSOR_PROP_REPORT_STATE 0x200316
>>> #define HID_USAGE_SENSOR_PROY_POWER_STATE 0x200319
>>> +/* Power state enumerations */
>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_UNDEFINED_ENUM 0x00
>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D0_FULL_POWER_ENUM 0x01
>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D1_LOW_POWER_ENUM 0x02
>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D2_STANDBY_WITH_WAKE_ENUM 0x03
>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D3_SLEEP_WITH_WAKE_ENUM 0x04
>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D4_POWER_OFF_ENUM 0x05
>>> +
>>> +/* Report State enumerations */
>>> +#define HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVENTS_ENUM 0x00
>>> +#define HID_USAGE_SENSOR_PROP_REPORTING_STATE_ALL_EVENTS_ENUM 0x01
>>> +
>>> #endif
>>>
> Thanks,
> Srinivas
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] iio: hid-sensors: Fix power and report state
2013-10-15 18:58 ` Jonathan Cameron
@ 2013-10-15 18:34 ` Srinivas Pandruvada
2013-10-15 19:42 ` Jonathan Cameron
0 siblings, 1 reply; 11+ messages in thread
From: Srinivas Pandruvada @ 2013-10-15 18:34 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio
On 10/15/2013 11:58 AM, Jonathan Cameron wrote:
> On 10/14/13 23:27, Srinivas Pandruvada wrote:
>> On 10/14/2013 03:23 PM, Jonathan Cameron wrote:
>>> On 10/08/13 22:32, Srinivas Pandruvada wrote:
>>>> In the original HID sensor specifications all Named array enums stated
>>>> to be 0-based. But the most commercial devices implemented as 1-based,
>>>> because of the implementation by one of the major OS vendor. To fix this
>>>> we added a quirk, which required module to be recompiled. Instead now
>>>> added a module parameter, so that it can be switched at runtime.
>>>> By default it will be 1-based to be compatible with majority of devices.
>>>> This is true for both power and report state usage id for hid sensors.
>>>> Also added defines for power on values for D0 to D4 and using it for
>>>> clarity.
>>>>
>>>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>>> This looks fine but raises a few questions...
>>>
>>> What other options do we have for controlling this? Are hid sensors
>>> chips identifiable? If so can we have a list of chips where it is 0 in
>>> the driver. The module parameter might still be needed to deal with new
>>> devices but would be nice to have most work out of the box.
>> We could have quirk based on vendor id/pid. But the problem is that once they update FW to be compliant with WIN8, it
>> will not work.
>> So there is no way to distinguish. Since sensor hub is present in most of WIN8 convertible devices, the quirk became a
>> new normal.
> Yuck. You have my sympathies!
>
>>> Got to love it when the quirk becomes the default ;)
>> Unfortunately they don't go back to specification update after such change.
> Just to check, is this quirk only hid-sensors related? Named arrays are
> as far as I can see a part of HID in general.
Currently I have information about HID sensors implementation only as
they are in new in the WIN8. I didn't hear any complaint about any other
HID devices.
Thanks,
Srinivas
>
>>>> ---
>>>> drivers/iio/common/hid-sensors/Kconfig | 9 --------
>>>> .../iio/common/hid-sensors/hid-sensor-trigger.c | 26 +++++++++++++++++-----
>>>> include/linux/hid-sensor-ids.h | 12 ++++++++++
>>>> 3 files changed, 33 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/common/hid-sensors/Kconfig b/drivers/iio/common/hid-sensors/Kconfig
>>>> index 1178121..39188b7 100644
>>>> --- a/drivers/iio/common/hid-sensors/Kconfig
>>>> +++ b/drivers/iio/common/hid-sensors/Kconfig
>>>> @@ -25,13 +25,4 @@ config HID_SENSOR_IIO_TRIGGER
>>>> If this driver is compiled as a module, it will be named
>>>> hid-sensor-trigger.
>>>> -config HID_SENSOR_ENUM_BASE_QUIRKS
>>>> - bool "ENUM base quirks for HID Sensor IIO drivers"
>>>> - depends on HID_SENSOR_IIO_COMMON
>>>> - help
>>>> - Say yes here to build support for sensor hub FW using
>>>> - enumeration, which is using 1 as base instead of 0.
>>>> - Since logical minimum is still set 0 instead of 1,
>>>> - there is no easy way to differentiate.
>>>> -
>>>> endmenu
>>>> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>>> index b6e77e0..dbc9141 100644
>>>> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>>> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>>> @@ -28,21 +28,37 @@
>>>> #include <linux/iio/sysfs.h>
>>>> #include "hid-sensor-trigger.h"
>>>> +#define HID_SENSOR_NAMED_ARRAY_BASE 1
>>>> +static int hid_sensor_named_array_base = HID_SENSOR_NAMED_ARRAY_BASE;
>>>> +module_param(hid_sensor_named_array_base, int, 0644);
>>>> +MODULE_PARM_DESC(hid_sensor_named_array_base,
>>>> + "HID SENSOR NAMED ARRAY BASE (0 or 1).");
>>>> +
>>>> static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
>>>> bool state)
>>>> {
>>>> struct hid_sensor_common *st = iio_trigger_get_drvdata(trig);
>>>> int state_val;
>>>> + int report_state;
>>>> if (state) {
>>>> if (sensor_hub_device_open(st->hsdev))
>>>> return -EIO;
>>>> - } else
>>>> + state_val =
>>>> + HID_USAGE_SENSOR_PROP_POWER_STATE_D0_FULL_POWER_ENUM;
>>>> + report_state =
>>>> + HID_USAGE_SENSOR_PROP_REPORTING_STATE_ALL_EVENTS_ENUM;
>>>> + } else {
>>>> sensor_hub_device_close(st->hsdev);
>>>> -
>>>> - state_val = state ? 1 : 0;
>>>> - if (IS_ENABLED(CONFIG_HID_SENSOR_ENUM_BASE_QUIRKS))
>>>> + state_val =
>>>> + HID_USAGE_SENSOR_PROP_POWER_STATE_D4_POWER_OFF_ENUM;
>>>> + report_state =
>>>> + HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVENTS_ENUM;
>>>> + }
>>>> + if (hid_sensor_named_array_base) {
>>>> ++state_val;
>>>> + ++report_state;
>>>> + }
>>>> st->data_ready = state;
>>>> sensor_hub_set_feature(st->hsdev, st->power_state.report_id,
>>>> st->power_state.index,
>>>> @@ -50,7 +66,7 @@ static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
>>>> sensor_hub_set_feature(st->hsdev, st->report_state.report_id,
>>>> st->report_state.index,
>>>> - (s32)state_val);
>>>> + (s32)report_state);
>>>> return 0;
>>>> }
>>>> diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
>>>> index 4f945d3..8323775 100644
>>>> --- a/include/linux/hid-sensor-ids.h
>>>> +++ b/include/linux/hid-sensor-ids.h
>>>> @@ -117,4 +117,16 @@
>>>> #define HID_USAGE_SENSOR_PROP_REPORT_STATE 0x200316
>>>> #define HID_USAGE_SENSOR_PROY_POWER_STATE 0x200319
>>>> +/* Power state enumerations */
>>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_UNDEFINED_ENUM 0x00
>>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D0_FULL_POWER_ENUM 0x01
>>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D1_LOW_POWER_ENUM 0x02
>>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D2_STANDBY_WITH_WAKE_ENUM 0x03
>>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D3_SLEEP_WITH_WAKE_ENUM 0x04
>>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D4_POWER_OFF_ENUM 0x05
>>>> +
>>>> +/* Report State enumerations */
>>>> +#define HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVENTS_ENUM 0x00
>>>> +#define HID_USAGE_SENSOR_PROP_REPORTING_STATE_ALL_EVENTS_ENUM 0x01
>>>> +
>>>> #endif
>>>>
>> Thanks,
>> Srinivas
>>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] iio: hid-sensors: Fix power and report state
2013-10-15 18:34 ` Srinivas Pandruvada
@ 2013-10-15 19:42 ` Jonathan Cameron
2013-11-06 0:14 ` Srinivas Pandruvada
2013-11-21 9:24 ` Jiri Kosina
0 siblings, 2 replies; 11+ messages in thread
From: Jonathan Cameron @ 2013-10-15 19:42 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: linux-iio, Jiri Kosina
On 10/15/13 19:34, Srinivas Pandruvada wrote:
> On 10/15/2013 11:58 AM, Jonathan Cameron wrote:
>> On 10/14/13 23:27, Srinivas Pandruvada wrote:
>>> On 10/14/2013 03:23 PM, Jonathan Cameron wrote:
>>>> On 10/08/13 22:32, Srinivas Pandruvada wrote:
>>>>> In the original HID sensor specifications all Named array enums stated
>>>>> to be 0-based. But the most commercial devices implemented as 1-based,
>>>>> because of the implementation by one of the major OS vendor. To fix this
>>>>> we added a quirk, which required module to be recompiled. Instead now
>>>>> added a module parameter, so that it can be switched at runtime.
>>>>> By default it will be 1-based to be compatible with majority of devices.
>>>>> This is true for both power and report state usage id for hid sensors.
>>>>> Also added defines for power on values for D0 to D4 and using it for
>>>>> clarity.
>>>>>
>>>>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>>>> This looks fine but raises a few questions...
>>>>
>>>> What other options do we have for controlling this? Are hid sensors
>>>> chips identifiable? If so can we have a list of chips where it is 0 in
>>>> the driver. The module parameter might still be needed to deal with new
>>>> devices but would be nice to have most work out of the box.
>>> We could have quirk based on vendor id/pid. But the problem is that once they update FW to be compliant with WIN8, it
>>> will not work.
>>> So there is no way to distinguish. Since sensor hub is present in most of WIN8 convertible devices, the quirk became a
>>> new normal.
>> Yuck. You have my sympathies!
>>
>>>> Got to love it when the quirk becomes the default ;)
>>> Unfortunately they don't go back to specification update after such change.
>> Just to check, is this quirk only hid-sensors related? Named arrays are
>> as far as I can see a part of HID in general.
> Currently I have information about HID sensors implementation only as they are in new in the WIN8. I didn't hear any
> complaint about any other HID devices.
Jiri, have you seen anything similar to this before.
I am just trying to work out if it should be a Hid quirk or should be
handled only in the hid-sensors module.
Jonathan
>
> Thanks,
> Srinivas
>>
>>>>> ---
>>>>> drivers/iio/common/hid-sensors/Kconfig | 9 --------
>>>>> .../iio/common/hid-sensors/hid-sensor-trigger.c | 26 +++++++++++++++++-----
>>>>> include/linux/hid-sensor-ids.h | 12 ++++++++++
>>>>> 3 files changed, 33 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iio/common/hid-sensors/Kconfig b/drivers/iio/common/hid-sensors/Kconfig
>>>>> index 1178121..39188b7 100644
>>>>> --- a/drivers/iio/common/hid-sensors/Kconfig
>>>>> +++ b/drivers/iio/common/hid-sensors/Kconfig
>>>>> @@ -25,13 +25,4 @@ config HID_SENSOR_IIO_TRIGGER
>>>>> If this driver is compiled as a module, it will be named
>>>>> hid-sensor-trigger.
>>>>> -config HID_SENSOR_ENUM_BASE_QUIRKS
>>>>> - bool "ENUM base quirks for HID Sensor IIO drivers"
>>>>> - depends on HID_SENSOR_IIO_COMMON
>>>>> - help
>>>>> - Say yes here to build support for sensor hub FW using
>>>>> - enumeration, which is using 1 as base instead of 0.
>>>>> - Since logical minimum is still set 0 instead of 1,
>>>>> - there is no easy way to differentiate.
>>>>> -
>>>>> endmenu
>>>>> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>>>> index b6e77e0..dbc9141 100644
>>>>> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>>>> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>>>> @@ -28,21 +28,37 @@
>>>>> #include <linux/iio/sysfs.h>
>>>>> #include "hid-sensor-trigger.h"
>>>>> +#define HID_SENSOR_NAMED_ARRAY_BASE 1
>>>>> +static int hid_sensor_named_array_base = HID_SENSOR_NAMED_ARRAY_BASE;
>>>>> +module_param(hid_sensor_named_array_base, int, 0644);
>>>>> +MODULE_PARM_DESC(hid_sensor_named_array_base,
>>>>> + "HID SENSOR NAMED ARRAY BASE (0 or 1).");
>>>>> +
>>>>> static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
>>>>> bool state)
>>>>> {
>>>>> struct hid_sensor_common *st = iio_trigger_get_drvdata(trig);
>>>>> int state_val;
>>>>> + int report_state;
>>>>> if (state) {
>>>>> if (sensor_hub_device_open(st->hsdev))
>>>>> return -EIO;
>>>>> - } else
>>>>> + state_val =
>>>>> + HID_USAGE_SENSOR_PROP_POWER_STATE_D0_FULL_POWER_ENUM;
>>>>> + report_state =
>>>>> + HID_USAGE_SENSOR_PROP_REPORTING_STATE_ALL_EVENTS_ENUM;
>>>>> + } else {
>>>>> sensor_hub_device_close(st->hsdev);
>>>>> -
>>>>> - state_val = state ? 1 : 0;
>>>>> - if (IS_ENABLED(CONFIG_HID_SENSOR_ENUM_BASE_QUIRKS))
>>>>> + state_val =
>>>>> + HID_USAGE_SENSOR_PROP_POWER_STATE_D4_POWER_OFF_ENUM;
>>>>> + report_state =
>>>>> + HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVENTS_ENUM;
>>>>> + }
>>>>> + if (hid_sensor_named_array_base) {
>>>>> ++state_val;
>>>>> + ++report_state;
>>>>> + }
>>>>> st->data_ready = state;
>>>>> sensor_hub_set_feature(st->hsdev, st->power_state.report_id,
>>>>> st->power_state.index,
>>>>> @@ -50,7 +66,7 @@ static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
>>>>> sensor_hub_set_feature(st->hsdev, st->report_state.report_id,
>>>>> st->report_state.index,
>>>>> - (s32)state_val);
>>>>> + (s32)report_state);
>>>>> return 0;
>>>>> }
>>>>> diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
>>>>> index 4f945d3..8323775 100644
>>>>> --- a/include/linux/hid-sensor-ids.h
>>>>> +++ b/include/linux/hid-sensor-ids.h
>>>>> @@ -117,4 +117,16 @@
>>>>> #define HID_USAGE_SENSOR_PROP_REPORT_STATE 0x200316
>>>>> #define HID_USAGE_SENSOR_PROY_POWER_STATE 0x200319
>>>>> +/* Power state enumerations */
>>>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_UNDEFINED_ENUM 0x00
>>>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D0_FULL_POWER_ENUM 0x01
>>>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D1_LOW_POWER_ENUM 0x02
>>>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D2_STANDBY_WITH_WAKE_ENUM 0x03
>>>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D3_SLEEP_WITH_WAKE_ENUM 0x04
>>>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D4_POWER_OFF_ENUM 0x05
>>>>> +
>>>>> +/* Report State enumerations */
>>>>> +#define HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVENTS_ENUM 0x00
>>>>> +#define HID_USAGE_SENSOR_PROP_REPORTING_STATE_ALL_EVENTS_ENUM 0x01
>>>>> +
>>>>> #endif
>>>>>
>>> Thanks,
>>> Srinivas
>>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] iio: hid-sensors: Fix power and report state
2013-10-15 19:42 ` Jonathan Cameron
@ 2013-11-06 0:14 ` Srinivas Pandruvada
2013-11-21 9:24 ` Jiri Kosina
1 sibling, 0 replies; 11+ messages in thread
From: Srinivas Pandruvada @ 2013-11-06 0:14 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio, Jiri Kosina
On 10/15/2013 12:42 PM, Jonathan Cameron wrote:
> On 10/15/13 19:34, Srinivas Pandruvada wrote:
>> On 10/15/2013 11:58 AM, Jonathan Cameron wrote:
>>> On 10/14/13 23:27, Srinivas Pandruvada wrote:
>>>> On 10/14/2013 03:23 PM, Jonathan Cameron wrote:
>>>>> On 10/08/13 22:32, Srinivas Pandruvada wrote:
>>>>>> In the original HID sensor specifications all Named array enums stated
>>>>>> to be 0-based. But the most commercial devices implemented as 1-based,
>>>>>> because of the implementation by one of the major OS vendor. To fix this
>>>>>> we added a quirk, which required module to be recompiled. Instead now
>>>>>> added a module parameter, so that it can be switched at runtime.
>>>>>> By default it will be 1-based to be compatible with majority of devices.
>>>>>> This is true for both power and report state usage id for hid sensors.
>>>>>> Also added defines for power on values for D0 to D4 and using it for
>>>>>> clarity.
>>>>>>
>>>>>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>>>>> This looks fine but raises a few questions...
>>>>>
>>>>> What other options do we have for controlling this? Are hid sensors
>>>>> chips identifiable? If so can we have a list of chips where it is 0 in
>>>>> the driver. The module parameter might still be needed to deal with new
>>>>> devices but would be nice to have most work out of the box.
>>>> We could have quirk based on vendor id/pid. But the problem is that once they update FW to be compliant with WIN8, it
>>>> will not work.
>>>> So there is no way to distinguish. Since sensor hub is present in most of WIN8 convertible devices, the quirk became a
>>>> new normal.
>>> Yuck. You have my sympathies!
>>>
>>>>> Got to love it when the quirk becomes the default ;)
>>>> Unfortunately they don't go back to specification update after such change.
>>> Just to check, is this quirk only hid-sensors related? Named arrays are
>>> as far as I can see a part of HID in general.
>> Currently I have information about HID sensors implementation only as they are in new in the WIN8. I didn't hear any
>> complaint about any other HID devices.
> Jiri, have you seen anything similar to this before.
>
> I am just trying to work out if it should be a Hid quirk or should be
> handled only in the hid-sensors module.
Any updated comment on this?
Thanks,
Srinivas
> Jonathan
>> Thanks,
>> Srinivas
>>>>>> ---
>>>>>> drivers/iio/common/hid-sensors/Kconfig | 9 --------
>>>>>> .../iio/common/hid-sensors/hid-sensor-trigger.c | 26 +++++++++++++++++-----
>>>>>> include/linux/hid-sensor-ids.h | 12 ++++++++++
>>>>>> 3 files changed, 33 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/iio/common/hid-sensors/Kconfig b/drivers/iio/common/hid-sensors/Kconfig
>>>>>> index 1178121..39188b7 100644
>>>>>> --- a/drivers/iio/common/hid-sensors/Kconfig
>>>>>> +++ b/drivers/iio/common/hid-sensors/Kconfig
>>>>>> @@ -25,13 +25,4 @@ config HID_SENSOR_IIO_TRIGGER
>>>>>> If this driver is compiled as a module, it will be named
>>>>>> hid-sensor-trigger.
>>>>>> -config HID_SENSOR_ENUM_BASE_QUIRKS
>>>>>> - bool "ENUM base quirks for HID Sensor IIO drivers"
>>>>>> - depends on HID_SENSOR_IIO_COMMON
>>>>>> - help
>>>>>> - Say yes here to build support for sensor hub FW using
>>>>>> - enumeration, which is using 1 as base instead of 0.
>>>>>> - Since logical minimum is still set 0 instead of 1,
>>>>>> - there is no easy way to differentiate.
>>>>>> -
>>>>>> endmenu
>>>>>> diff --git a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>>>>> index b6e77e0..dbc9141 100644
>>>>>> --- a/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>>>>> +++ b/drivers/iio/common/hid-sensors/hid-sensor-trigger.c
>>>>>> @@ -28,21 +28,37 @@
>>>>>> #include <linux/iio/sysfs.h>
>>>>>> #include "hid-sensor-trigger.h"
>>>>>> +#define HID_SENSOR_NAMED_ARRAY_BASE 1
>>>>>> +static int hid_sensor_named_array_base = HID_SENSOR_NAMED_ARRAY_BASE;
>>>>>> +module_param(hid_sensor_named_array_base, int, 0644);
>>>>>> +MODULE_PARM_DESC(hid_sensor_named_array_base,
>>>>>> + "HID SENSOR NAMED ARRAY BASE (0 or 1).");
>>>>>> +
>>>>>> static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
>>>>>> bool state)
>>>>>> {
>>>>>> struct hid_sensor_common *st = iio_trigger_get_drvdata(trig);
>>>>>> int state_val;
>>>>>> + int report_state;
>>>>>> if (state) {
>>>>>> if (sensor_hub_device_open(st->hsdev))
>>>>>> return -EIO;
>>>>>> - } else
>>>>>> + state_val =
>>>>>> + HID_USAGE_SENSOR_PROP_POWER_STATE_D0_FULL_POWER_ENUM;
>>>>>> + report_state =
>>>>>> + HID_USAGE_SENSOR_PROP_REPORTING_STATE_ALL_EVENTS_ENUM;
>>>>>> + } else {
>>>>>> sensor_hub_device_close(st->hsdev);
>>>>>> -
>>>>>> - state_val = state ? 1 : 0;
>>>>>> - if (IS_ENABLED(CONFIG_HID_SENSOR_ENUM_BASE_QUIRKS))
>>>>>> + state_val =
>>>>>> + HID_USAGE_SENSOR_PROP_POWER_STATE_D4_POWER_OFF_ENUM;
>>>>>> + report_state =
>>>>>> + HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVENTS_ENUM;
>>>>>> + }
>>>>>> + if (hid_sensor_named_array_base) {
>>>>>> ++state_val;
>>>>>> + ++report_state;
>>>>>> + }
>>>>>> st->data_ready = state;
>>>>>> sensor_hub_set_feature(st->hsdev, st->power_state.report_id,
>>>>>> st->power_state.index,
>>>>>> @@ -50,7 +66,7 @@ static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
>>>>>> sensor_hub_set_feature(st->hsdev, st->report_state.report_id,
>>>>>> st->report_state.index,
>>>>>> - (s32)state_val);
>>>>>> + (s32)report_state);
>>>>>> return 0;
>>>>>> }
>>>>>> diff --git a/include/linux/hid-sensor-ids.h b/include/linux/hid-sensor-ids.h
>>>>>> index 4f945d3..8323775 100644
>>>>>> --- a/include/linux/hid-sensor-ids.h
>>>>>> +++ b/include/linux/hid-sensor-ids.h
>>>>>> @@ -117,4 +117,16 @@
>>>>>> #define HID_USAGE_SENSOR_PROP_REPORT_STATE 0x200316
>>>>>> #define HID_USAGE_SENSOR_PROY_POWER_STATE 0x200319
>>>>>> +/* Power state enumerations */
>>>>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_UNDEFINED_ENUM 0x00
>>>>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D0_FULL_POWER_ENUM 0x01
>>>>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D1_LOW_POWER_ENUM 0x02
>>>>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D2_STANDBY_WITH_WAKE_ENUM 0x03
>>>>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D3_SLEEP_WITH_WAKE_ENUM 0x04
>>>>>> +#define HID_USAGE_SENSOR_PROP_POWER_STATE_D4_POWER_OFF_ENUM 0x05
>>>>>> +
>>>>>> +/* Report State enumerations */
>>>>>> +#define HID_USAGE_SENSOR_PROP_REPORTING_STATE_NO_EVENTS_ENUM 0x00
>>>>>> +#define HID_USAGE_SENSOR_PROP_REPORTING_STATE_ALL_EVENTS_ENUM 0x01
>>>>>> +
>>>>>> #endif
>>>>>>
>>>> Thanks,
>>>> Srinivas
>>>>
> --
> 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
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] iio: hid-sensors: Fix power and report state
2013-10-15 19:42 ` Jonathan Cameron
2013-11-06 0:14 ` Srinivas Pandruvada
@ 2013-11-21 9:24 ` Jiri Kosina
2013-11-21 20:06 ` Jonathan Cameron
1 sibling, 1 reply; 11+ messages in thread
From: Jiri Kosina @ 2013-11-21 9:24 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Srinivas Pandruvada, linux-iio
On Tue, 15 Oct 2013, Jonathan Cameron wrote:
> >>>>> In the original HID sensor specifications all Named array enums
> >>>>> stated to be 0-based. But the most commercial devices implemented
> >>>>> as 1-based, because of the implementation by one of the major OS
> >>>>> vendor. To fix this we added a quirk, which required module to be
> >>>>> recompiled. Instead now added a module parameter, so that it can
> >>>>> be switched at runtime. By default it will be 1-based to be
> >>>>> compatible with majority of devices. This is true for both power
> >>>>> and report state usage id for hid sensors. Also added defines for
> >>>>> power on values for D0 to D4 and using it for clarity.
> >>>>>
> >>>>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> >>>> This looks fine but raises a few questions...
> >>>>
> >>>> What other options do we have for controlling this? Are hid sensors
> >>>> chips identifiable? If so can we have a list of chips where it is 0 in
> >>>> the driver. The module parameter might still be needed to deal with new
> >>>> devices but would be nice to have most work out of the box.
> >>> We could have quirk based on vendor id/pid. But the problem is that once they update FW to be compliant with WIN8, it
> >>> will not work.
> >>> So there is no way to distinguish. Since sensor hub is present in most of WIN8 convertible devices, the quirk became a
> >>> new normal.
> >> Yuck. You have my sympathies!
> >>
> >>>> Got to love it when the quirk becomes the default ;)
> >>> Unfortunately they don't go back to specification update after such change.
> >> Just to check, is this quirk only hid-sensors related? Named arrays are
> >> as far as I can see a part of HID in general.
> > Currently I have information about HID sensors implementation only as they are in new in the WIN8. I didn't hear any
> > complaint about any other HID devices.
> Jiri, have you seen anything similar to this before.
>
> I am just trying to work out if it should be a Hid quirk or should be
> handled only in the hid-sensors module.
Sorry for super-late response from me, I have been drowned in other
things.
This looks indeed pretty bad. I don't have a strong objection to having
this as a generic HID quirk; the argument being, if this was made de-facto
Win8-mandated standard, odds are that we'd start seeing a lot of devices
with this behavior.
So if you decide to take this way, please send the patch to me, I'll apply
it.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] iio: hid-sensors: Fix power and report state
2013-11-21 9:24 ` Jiri Kosina
@ 2013-11-21 20:06 ` Jonathan Cameron
2013-11-22 1:37 ` Srinivas Pandruvada
0 siblings, 1 reply; 11+ messages in thread
From: Jonathan Cameron @ 2013-11-21 20:06 UTC (permalink / raw)
To: Jiri Kosina; +Cc: Srinivas Pandruvada, linux-iio
Jiri Kosina <jkosina@suse.cz> wrote:
>On Tue, 15 Oct 2013, Jonathan Cameron wrote:
>
>> >>>>> In the original HID sensor specifications all Named array enums
>
>> >>>>> stated to be 0-based. But the most commercial devices
>implemented
>> >>>>> as 1-based, because of the implementation by one of the major
>OS
>> >>>>> vendor. To fix this we added a quirk, which required module to
>be
>> >>>>> recompiled. Instead now added a module parameter, so that it
>can
>> >>>>> be switched at runtime. By default it will be 1-based to be
>> >>>>> compatible with majority of devices. This is true for both
>power
>> >>>>> and report state usage id for hid sensors. Also added defines
>for
>> >>>>> power on values for D0 to D4 and using it for clarity.
>> >>>>>
>> >>>>> Signed-off-by: Srinivas Pandruvada
><srinivas.pandruvada@linux.intel.com>
>> >>>> This looks fine but raises a few questions...
>> >>>>
>> >>>> What other options do we have for controlling this? Are hid
>sensors
>> >>>> chips identifiable? If so can we have a list of chips where it
>is 0 in
>> >>>> the driver. The module parameter might still be needed to deal
>with new
>> >>>> devices but would be nice to have most work out of the box.
>> >>> We could have quirk based on vendor id/pid. But the problem is
>that once they update FW to be compliant with WIN8, it
>> >>> will not work.
>> >>> So there is no way to distinguish. Since sensor hub is present in
>most of WIN8 convertible devices, the quirk became a
>> >>> new normal.
>> >> Yuck. You have my sympathies!
>> >>
>> >>>> Got to love it when the quirk becomes the default ;)
>> >>> Unfortunately they don't go back to specification update after
>such change.
>> >> Just to check, is this quirk only hid-sensors related? Named
>arrays are
>> >> as far as I can see a part of HID in general.
>> > Currently I have information about HID sensors implementation only
>as they are in new in the WIN8. I didn't hear any
>> > complaint about any other HID devices.
>> Jiri, have you seen anything similar to this before.
>>
>> I am just trying to work out if it should be a Hid quirk or should be
>> handled only in the hid-sensors module.
>
>Sorry for super-late response from me, I have been drowned in other
>things.
>
>This looks indeed pretty bad. I don't have a strong objection to having
>
>this as a generic HID quirk; the argument being, if this was made
>de-facto
>Win8-mandated standard, odds are that we'd start seeing a lot of
>devices
>with this behavior.
>
>So if you decide to take this way, please send the patch to me, I'll
>apply
>it.
Thanks for the reply Jiri. That was pretty much what I was expecting.
Srinivas, please rework this as a generic his quirk.
>
>Thanks,
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] iio: hid-sensors: Fix power and report state
2013-11-21 20:06 ` Jonathan Cameron
@ 2013-11-22 1:37 ` Srinivas Pandruvada
2013-11-27 14:59 ` Jiri Kosina
0 siblings, 1 reply; 11+ messages in thread
From: Srinivas Pandruvada @ 2013-11-22 1:37 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: Jiri Kosina, linux-iio
On 11/21/2013 12:06 PM, Jonathan Cameron wrote:
>
> Jiri Kosina <jkosina@suse.cz> wrote:
>> On Tue, 15 Oct 2013, Jonathan Cameron wrote:
>>
>>>>>>>> In the original HID sensor specifications all Named array enums
>>>>>>>> stated to be 0-based. But the most commercial devices
>> implemented
>>>>>>>> as 1-based, because of the implementation by one of the major
>> OS
>>>>>>>> vendor. To fix this we added a quirk, which required module to
>> be
>>>>>>>> recompiled. Instead now added a module parameter, so that it
>> can
>>>>>>>> be switched at runtime. By default it will be 1-based to be
>>>>>>>> compatible with majority of devices. This is true for both
>> power
>>>>>>>> and report state usage id for hid sensors. Also added defines
>> for
>>>>>>>> power on values for D0 to D4 and using it for clarity.
>>>>>>>>
>>>>>>>> Signed-off-by: Srinivas Pandruvada
>> <srinivas.pandruvada@linux.intel.com>
>>>>>>> This looks fine but raises a few questions...
>>>>>>>
>>>>>>> What other options do we have for controlling this? Are hid
>> sensors
>>>>>>> chips identifiable? If so can we have a list of chips where it
>> is 0 in
>>>>>>> the driver. The module parameter might still be needed to deal
>> with new
>>>>>>> devices but would be nice to have most work out of the box.
>>>>>> We could have quirk based on vendor id/pid. But the problem is
>> that once they update FW to be compliant with WIN8, it
>>>>>> will not work.
>>>>>> So there is no way to distinguish. Since sensor hub is present in
>> most of WIN8 convertible devices, the quirk became a
>>>>>> new normal.
>>>>> Yuck. You have my sympathies!
>>>>>
>>>>>>> Got to love it when the quirk becomes the default ;)
>>>>>> Unfortunately they don't go back to specification update after
>> such change.
>>>>> Just to check, is this quirk only hid-sensors related? Named
>> arrays are
>>>>> as far as I can see a part of HID in general.
>>>> Currently I have information about HID sensors implementation only
>> as they are in new in the WIN8. I didn't hear any
>>>> complaint about any other HID devices.
>>> Jiri, have you seen anything similar to this before.
>>>
>>> I am just trying to work out if it should be a Hid quirk or should be
>>> handled only in the hid-sensors module.
>> Sorry for super-late response from me, I have been drowned in other
>> things.
>>
>> This looks indeed pretty bad. I don't have a strong objection to having
>>
>> this as a generic HID quirk; the argument being, if this was made
>> de-facto
>> Win8-mandated standard, odds are that we'd start seeing a lot of
>> devices
>> with this behavior.
>>
>> So if you decide to take this way, please send the patch to me, I'll
>> apply
>> it.
> Thanks for the reply Jiri. That was pretty much what I was expecting.
>
> Srinivas, please rework this as a generic his quirk.
The problem I have is identify NAry data if we implement as a common hid
quirk. If I can identify, then in hid_set_field,
I can increment the requested value by 1 if the hid_device->quirk is set
for enum base quirk.
But from the report descriptor there is no way to tell it is a named
array. For example for the following
report descriptor for setting power state, which by spec is a named
array with selectors.
From report descriptor:
HID_USAGE_SENSOR_PROPERTY_POWER_STATE, // NAry
HID_LOGICAL_MIN_8(0),
HID_LOGICAL_MAX_8(5),
HID_REPORT_SIZE(8),
HID_REPORT_COUNT(1),
HID_COLLECTION(Logical),
HID_USAGE_SENSOR_PROPERTY_POWER_STATE_UNDEFINED,
HID_USAGE_SENSOR_PROPERTY_POWER_STATE_D0_FULL_POWER,
HID_USAGE_SENSOR_PROPERTY_POWER_STATE_D1_LOW_POWER,
HID_USAGE_SENSOR_PROPERTY_POWER_STATE_D2_STANDBY_WITH_WAKE,
HID_USAGE_SENSOR_PROPERTY_POWER_STATE_D3_SLEEP_WITH_WAKE,
HID_USAGE_SENSOR_PROPERTY_POWER_STATE_D4_POWER_OFF,
HID_FEATURE(Data_Arr_Abs),
HID_END_COLLECTION,
The HID core spec allows only collection to be typed as named arrays.
But as
you can see the report descriptor still names this as COLLECTION(Logical).
If it is collection defined as HID_COLLECTION(NAMED_ARRAY), it is very
easy to implement a
quirk at HID level. Unfortunately the logical minimum and maximum also
don't really
show correct values accommodating the enum base.
One way to do this is by adding a quirk field in hid_field structure
also. This way, quirk can be
enabled on a per field basis. Then we can have a common hid quirk at a
device level and also individual field level.
Jiri, Do you have some suggestion?
Thanks,
Srinivas
>> Thanks,
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH] iio: hid-sensors: Fix power and report state
2013-11-22 1:37 ` Srinivas Pandruvada
@ 2013-11-27 14:59 ` Jiri Kosina
0 siblings, 0 replies; 11+ messages in thread
From: Jiri Kosina @ 2013-11-27 14:59 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: Jonathan Cameron, linux-iio
On Thu, 21 Nov 2013, Srinivas Pandruvada wrote:
> One way to do this is by adding a quirk field in hid_field structure also.
> This way, quirk can be
> enabled on a per field basis. Then we can have a common hid quirk at a device
> level and also individual field level.
>
> Jiri, Do you have some suggestion?
I don't really see any better option. Could you please prepare a patch? It
should be doable in a relatively clean way.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-11-27 14:59 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-08 21:32 [PATCH] iio: hid-sensors: Fix power and report state Srinivas Pandruvada
2013-10-14 22:23 ` Jonathan Cameron
2013-10-14 22:27 ` Srinivas Pandruvada
2013-10-15 18:58 ` Jonathan Cameron
2013-10-15 18:34 ` Srinivas Pandruvada
2013-10-15 19:42 ` Jonathan Cameron
2013-11-06 0:14 ` Srinivas Pandruvada
2013-11-21 9:24 ` Jiri Kosina
2013-11-21 20:06 ` Jonathan Cameron
2013-11-22 1:37 ` Srinivas Pandruvada
2013-11-27 14:59 ` Jiri Kosina
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).