diff mbox

hid-sensor-hub: Force logical minimum to 1 for power and report state

Message ID 20170807045201.3286-1-srinivas.pandruvada@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Srinivas Pandruvada Aug. 7, 2017, 4:52 a.m. UTC 
In the reference HID sensor hub firmware all Named array enums were
0-based. There is no description of the default base of enums in HID
sensor hub specification as logical minimum should have set this base
value.

Every sensor hub implemented enum as 1-based, without explicitly setting
logical minimum to 1, because of the implementation by one of the major
OS vendor. In Linux we used logical minimum to decide the enum base.

Some sensor hub FWs already changed logical minimum from 0 to 1. We hoped
that every other vendor will follow. But that didn't happen and we had to
fix the report header for every sensor hub to change logical minimum to 1
by using .report_fixup() callback. So for every new sensor hub we had to
modify source code by adding this quirk based on the vendor and device id.
This is becoming a maintenance burden.

This patch hardcodes the logical minimum of power and report state
attributes to 1. In this way we can remove the existing quirks and also
we don't have to add more quirks.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/hid/hid-sensor-hub.c                       | 94 ----------------------
 .../iio/common/hid-sensors/hid-sensor-attributes.c |  3 +
 2 files changed, 3 insertions(+), 94 deletions(-)

Comments

Jiri Kosina Aug. 8, 2017, 8:26 a.m. UTC | #1 
On Sun, 6 Aug 2017, Srinivas Pandruvada wrote:

> In the reference HID sensor hub firmware all Named array enums were
> 0-based. There is no description of the default base of enums in HID
> sensor hub specification as logical minimum should have set this base
> value.
> 
> Every sensor hub implemented enum as 1-based, without explicitly setting
> logical minimum to 1, because of the implementation by one of the major
> OS vendor. In Linux we used logical minimum to decide the enum base.
> 
> Some sensor hub FWs already changed logical minimum from 0 to 1. We hoped
> that every other vendor will follow. But that didn't happen and we had to
> fix the report header for every sensor hub to change logical minimum to 1
> by using .report_fixup() callback. So for every new sensor hub we had to
> modify source code by adding this quirk based on the vendor and device id.
> This is becoming a maintenance burden.
> 
> This patch hardcodes the logical minimum of power and report state
> attributes to 1. In this way we can remove the existing quirks and also
> we don't have to add more quirks.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

Jonathan, are you ok with me taking this through hid.git? Thanks,
Jonathan Cameron Aug. 9, 2017, 2:34 p.m. UTC | #2 
On Tue, 8 Aug 2017 10:26:58 +0200 (CEST)
Jiri Kosina <jikos@kernel.org> wrote:

> On Sun, 6 Aug 2017, Srinivas Pandruvada wrote:
> 
> > In the reference HID sensor hub firmware all Named array enums were
> > 0-based. There is no description of the default base of enums in HID
> > sensor hub specification as logical minimum should have set this base
> > value.
> > 
> > Every sensor hub implemented enum as 1-based, without explicitly setting
> > logical minimum to 1, because of the implementation by one of the major
> > OS vendor. In Linux we used logical minimum to decide the enum base.
> > 
> > Some sensor hub FWs already changed logical minimum from 0 to 1. We hoped
> > that every other vendor will follow. But that didn't happen and we had to
> > fix the report header for every sensor hub to change logical minimum to 1
> > by using .report_fixup() callback. So for every new sensor hub we had to
> > modify source code by adding this quirk based on the vendor and device id.
> > This is becoming a maintenance burden.
> > 
> > This patch hardcodes the logical minimum of power and report state
> > attributes to 1. In this way we can remove the existing quirks and also
> > we don't have to add more quirks.
> > 
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>  
> 
> Jonathan, are you ok with me taking this through hid.git? Thanks,
> 
Sure,
Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

--
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
Jiri Kosina Aug. 9, 2017, 8:17 p.m. UTC | #3 
On Wed, 9 Aug 2017, Jonathan Cameron wrote:

> > Jonathan, are you ok with me taking this through hid.git? Thanks,
> > 
> Sure,
> Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

Thanks. Applied to for-4.14/ish.
diff mbox

Patch

diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index 4ef7337..25363fc 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -579,54 +579,6 @@  void sensor_hub_device_close(struct hid_sensor_hub_device *hsdev)
 }
 EXPORT_SYMBOL_GPL(sensor_hub_device_close);
 
-static __u8 *sensor_hub_report_fixup(struct hid_device *hdev, __u8 *rdesc,
-		unsigned int *rsize)
-{
-	int index;
-	struct sensor_hub_data *sd =  hid_get_drvdata(hdev);
-	unsigned char report_block[] = {
-				0x0a,  0x16, 0x03, 0x15, 0x00, 0x25, 0x05};
-	unsigned char power_block[] = {
-				0x0a,  0x19, 0x03, 0x15, 0x00, 0x25, 0x05};
-
-	if (!(sd->quirks & HID_SENSOR_HUB_ENUM_QUIRK)) {
-		hid_dbg(hdev, "No Enum quirks\n");
-		return rdesc;
-	}
-
-	/* Looks for power and report state usage id and force to 1 */
-	for (index = 0; index < *rsize; ++index) {
-		if (((*rsize - index) > sizeof(report_block)) &&
-			!memcmp(&rdesc[index], report_block,
-						sizeof(report_block))) {
-			rdesc[index + 4] = 0x01;
-			index += sizeof(report_block);
-		}
-		if (((*rsize - index) > sizeof(power_block)) &&
-			!memcmp(&rdesc[index], power_block,
-						sizeof(power_block))) {
-			rdesc[index + 4] = 0x01;
-			index += sizeof(power_block);
-		}
-	}
-
-	/* Checks if the report descriptor of Thinkpad Helix 2 has a logical
-	 * minimum for magnetic flux axis greater than the maximum */
-	if (hdev->product == USB_DEVICE_ID_TEXAS_INSTRUMENTS_LENOVO_YOGA &&
-		*rsize == 2558 && rdesc[913] == 0x17 && rdesc[914] == 0x40 &&
-		rdesc[915] == 0x81 && rdesc[916] == 0x08 &&
-		rdesc[917] == 0x00 && rdesc[918] == 0x27 &&
-		rdesc[921] == 0x07 && rdesc[922] == 0x00) {
-		/* Sets negative logical minimum for mag x, y and z */
-		rdesc[914] = rdesc[935] = rdesc[956] = 0xc0;
-		rdesc[915] = rdesc[936] = rdesc[957] = 0x7e;
-		rdesc[916] = rdesc[937] = rdesc[958] = 0xf7;
-		rdesc[917] = rdesc[938] = rdesc[959] = 0xff;
-	}
-
-	return rdesc;
-}
-
 static int sensor_hub_probe(struct hid_device *hdev,
 				const struct hid_device_id *id)
 {
@@ -778,51 +730,6 @@  static void sensor_hub_remove(struct hid_device *hdev)
 }
 
 static const struct hid_device_id sensor_hub_devices[] = {
-	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, USB_VENDOR_ID_INTEL_0,
-			USB_DEVICE_ID_INTEL_HID_SENSOR_0),
-			.driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
-	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, USB_VENDOR_ID_INTEL_1,
-			USB_DEVICE_ID_INTEL_HID_SENSOR_0),
-			.driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
-	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, USB_VENDOR_ID_INTEL_1,
-			USB_DEVICE_ID_INTEL_HID_SENSOR_1),
-			.driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
-	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, USB_VENDOR_ID_MICROSOFT,
-			USB_DEVICE_ID_MS_SURFACE_PRO_2),
-			.driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
-	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, USB_VENDOR_ID_MICROSOFT,
-			USB_DEVICE_ID_MS_TOUCH_COVER_2),
-			.driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
-	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, USB_VENDOR_ID_MICROSOFT,
-			USB_DEVICE_ID_MS_TYPE_COVER_2),
-			.driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
-	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, USB_VENDOR_ID_MICROSOFT,
-			0x07bd), /* Microsoft Surface 3 */
-			.driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
-	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, USB_VENDOR_ID_MICROCHIP,
-			0x0f01), /* MM7150 */
-			.driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
-	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, USB_VENDOR_ID_STM_0,
-			USB_DEVICE_ID_STM_HID_SENSOR),
-			.driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
-	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, USB_VENDOR_ID_STM_0,
-			USB_DEVICE_ID_STM_HID_SENSOR_1),
-			.driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
-	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, USB_VENDOR_ID_TEXAS_INSTRUMENTS,
-			USB_DEVICE_ID_TEXAS_INSTRUMENTS_LENOVO_YOGA),
-			.driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
-	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, USB_VENDOR_ID_ITE,
-			USB_DEVICE_ID_ITE_LENOVO_YOGA),
-			.driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
-	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, USB_VENDOR_ID_ITE,
-			USB_DEVICE_ID_ITE_LENOVO_YOGA2),
-			.driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
-	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, USB_VENDOR_ID_ITE,
-			USB_DEVICE_ID_ITE_LENOVO_YOGA900),
-			.driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
-	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, USB_VENDOR_ID_INTEL_0,
-			0x22D8),
-			.driver_data = HID_SENSOR_HUB_ENUM_QUIRK},
 	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_SENSOR_HUB, HID_ANY_ID,
 		     HID_ANY_ID) },
 	{ }
@@ -835,7 +742,6 @@  static struct hid_driver sensor_hub_driver = {
 	.probe = sensor_hub_probe,
 	.remove = sensor_hub_remove,
 	.raw_event = sensor_hub_raw_event,
-	.report_fixup = sensor_hub_report_fixup,
 #ifdef CONFIG_PM
 	.suspend = sensor_hub_suspend,
 	.resume = sensor_hub_resume,
diff --git a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
index 1c0874c..f8460c7 100644
--- a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
+++ b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
@@ -425,6 +425,9 @@  int hid_sensor_parse_common_attributes(struct hid_sensor_hub_device *hsdev,
 					HID_USAGE_SENSOR_PROY_POWER_STATE,
 					&st->power_state);
 
+	st->power_state.logical_minimum = 1;
+	st->report_state.logical_minimum = 1;
+
 	sensor_hub_input_get_attribute_info(hsdev,
 			HID_FEATURE_REPORT, usage_id,
 			HID_USAGE_SENSOR_PROP_SENSITIVITY_ABS,