Re: [PATCH v2 0/2] patches for Innomedia INNEX GENESIS/ATARI adapter

[Tomasz Kramkowski]

On Mon, Mar 06, 2017 at 10:04:50AM +0100, Benjamin Tissoires wrote:
> Well, the HID spec is not very clear, that the least we can agree on :)
> 
> But Microsoft's interpretation is rather clear in the
> multitouch/touchpad/pen specification:
> https://msdn.microsoft.com/en-us/library/windows/hardware/dn672278(v=vs.85).aspx
> paragraph "Required HID usages for pen digitizers":
> 
> "It should be noted that the host will recognize the values outside the
> logical range as signifying the implementation of this protocol only if
> the report descriptor specifically includes the bit signifying the fact
> that X and Y support NULL states. Otherwise, values outside the logical
> range are simply moved to the nearest boundary value."
> 
> And the 2 report descriptors written after are correct concerning the
> NULL state bit.
> 
> For Microsoft (in the pointer delivery protocol):
> - NULL state -> ignore out of range values
> - No NULL State -> clamp at the nearest boundary.
> 
> Following this would solve both issues If I understand correctly. Your
> controller would be clamped to [-1..1] (No Null State), and the ones
> that need to be ignored (like the ones from Denilson will be thanks to
> the NULL state bit set.

The clamping behaviour is the best of both worlds, it still matches how
I interpret the spec and provides a concrete definition of what should
happen when an out of bounds value is reported with the "No Null
Position | Null State" bit unset.

However, currently we just let the value pass through unchanged. So I
propose another patch on top of this one (at the bottom of the email,
done against for-4.12/hid-core-null-state-handling). The original change
which ignores the out of range value does a dbg_hid, I'm not sure if
that's necessary for the clamping scenario. I'll leave a few days for
any comments and if testing goes well (I don't see why not) I'll post it
on here as a patch. I'm not sure if that would be a v3 or a new patch.

> Maybe we can follow this to say we are mimicking Microsoft's driver and
> hope for the best?

Their approach to the ambiguity takes the safest bet and compatibility
with them might not be a bad idea anyway so I agree with following their
interpretation. I will note a link to that website in my commit message.

-- 
Tomasz Kramkowski | GPG: 40B037BA0A5B8680 | Web: https://the-tk.com/

---
 drivers/hid/hid-input.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index cf8256aac2bd..cf38ff79cfe9 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1157,12 +1157,15 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 	 * don't specify logical min and max.
 	 */
 	if ((field->flags & HID_MAIN_ITEM_VARIABLE) &&
-	    (field->flags & HID_MAIN_ITEM_NULL_STATE) &&
 	    (field->logical_minimum < field->logical_maximum) &&
 	    (value < field->logical_minimum ||
 	     value > field->logical_maximum)) {
-		dbg_hid("Ignoring out-of-range value %x\n", value);
-		return;
+		if (field->flags & HID_MAIN_ITEM_NULL_STATE) {
+			dbg_hid("Ignoring out-of-range value %x\n", value);
+			return;
+		}
+		value = value < field->logical_minimum ?
+			field->logical_minimum : field->logical_maximum;
 	}
 
 	/*
-- 
2.12.0