[v1] Input: zinitix - Don't fail if linux,keycodes prop is absent

[PATCH] Input: zinitix - Don't fail if linux,keycodes prop is absent

Posted by Nikita Travkin 1 month, 3 weeks ago

When initially adding the touchkey support, a mistake was made in the
property parsing code. The possible negative errno from
device_property_count_u32() was never checked, which was an oversight
left from converting to it from the of_property as part of the review
fixes.

Re-add the correct handling of the absent property, in which case zero
touchkeys should be assumed, which would disable the feature.

Reported-by: Jakob Hauser <jahau@rocketmail.com>
Tested-by: Jakob Hauser <jahau@rocketmail.com>
Fixes: 075d9b22c8fe ("Input: zinitix - add touchkey support")
Signed-off-by: Nikita Travkin <nikita@trvn.ru>
---
 drivers/input/touchscreen/zinitix.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/input/touchscreen/zinitix.c b/drivers/input/touchscreen/zinitix.c
index 52b3950460e2..1f726653940c 100644
--- a/drivers/input/touchscreen/zinitix.c
+++ b/drivers/input/touchscreen/zinitix.c
@@ -645,19 +645,30 @@ static int zinitix_ts_probe(struct i2c_client *client)
 		return error;
 	}
 
-	bt541->num_keycodes = device_property_count_u32(&client->dev, "linux,keycodes");
-	if (bt541->num_keycodes > ARRAY_SIZE(bt541->keycodes)) {
-		dev_err(&client->dev, "too many keys defined (%d)\n", bt541->num_keycodes);
-		return -EINVAL;
+	error = device_property_count_u32(&client->dev, "linux,keycodes");
+	if (error == -EINVAL || error == -ENODATA) {
+		bt541->num_keycodes = 0;
+	} else if (error < 0) {
+		dev_err(&client->dev, "Failed to count \"linux,keycodes\" property: %d\n", error);
+		return error;
+	} else {
+		bt541->num_keycodes = error;
 	}
 
-	error = device_property_read_u32_array(&client->dev, "linux,keycodes",
-					       bt541->keycodes,
-					       bt541->num_keycodes);
-	if (error) {
-		dev_err(&client->dev,
-			"Unable to parse \"linux,keycodes\" property: %d\n", error);
-		return error;
+	if (bt541->num_keycodes > 0) {
+		if (bt541->num_keycodes > ARRAY_SIZE(bt541->keycodes)) {
+			dev_err(&client->dev, "too many keys defined (%d)\n", bt541->num_keycodes);
+			return -EINVAL;
+		}
+
+		error = device_property_read_u32_array(&client->dev, "linux,keycodes",
+						       bt541->keycodes,
+						       bt541->num_keycodes);
+		if (error) {
+			dev_err(&client->dev,
+				"Unable to parse \"linux,keycodes\" property: %d\n", error);
+			return error;
+		}
 	}
 
 	error = zinitix_init_input_dev(bt541);

---
base-commit: fe21733536749bb1b31c9c84e0b8d2ab8d82ce13
change-id: 20241002-zinitix-no-keycodes-f0fe1bdaccb2

Best regards,
-- 
Nikita Travkin <nikita@trvn.ru>

Re: [PATCH] Input: zinitix - Don't fail if linux,keycodes prop is absent

Posted by Dmitry Torokhov 1 month, 3 weeks ago

Hi Nikita,

On Wed, Oct 02, 2024 at 06:01:48PM +0500, Nikita Travkin wrote:
> When initially adding the touchkey support, a mistake was made in the
> property parsing code. The possible negative errno from
> device_property_count_u32() was never checked, which was an oversight
> left from converting to it from the of_property as part of the review
> fixes.
> 
> Re-add the correct handling of the absent property, in which case zero
> touchkeys should be assumed, which would disable the feature.
> 
> Reported-by: Jakob Hauser <jahau@rocketmail.com>
> Tested-by: Jakob Hauser <jahau@rocketmail.com>
> Fixes: 075d9b22c8fe ("Input: zinitix - add touchkey support")
> Signed-off-by: Nikita Travkin <nikita@trvn.ru>
> ---
>  drivers/input/touchscreen/zinitix.c | 33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/zinitix.c b/drivers/input/touchscreen/zinitix.c
> index 52b3950460e2..1f726653940c 100644
> --- a/drivers/input/touchscreen/zinitix.c
> +++ b/drivers/input/touchscreen/zinitix.c
> @@ -645,19 +645,30 @@ static int zinitix_ts_probe(struct i2c_client *client)
>  		return error;
>  	}
>  
> -	bt541->num_keycodes = device_property_count_u32(&client->dev, "linux,keycodes");
> -	if (bt541->num_keycodes > ARRAY_SIZE(bt541->keycodes)) {
> -		dev_err(&client->dev, "too many keys defined (%d)\n", bt541->num_keycodes);
> -		return -EINVAL;
> +	error = device_property_count_u32(&client->dev, "linux,keycodes");
> +	if (error == -EINVAL || error == -ENODATA) {
> +		bt541->num_keycodes = 0;
> +	} else if (error < 0) {
> +		dev_err(&client->dev, "Failed to count \"linux,keycodes\" property: %d\n", error);
> +		return error;
> +	} else {
> +		bt541->num_keycodes = error;
>  	}
>  
> -	error = device_property_read_u32_array(&client->dev, "linux,keycodes",
> -					       bt541->keycodes,
> -					       bt541->num_keycodes);
> -	if (error) {
> -		dev_err(&client->dev,
> -			"Unable to parse \"linux,keycodes\" property: %d\n", error);
> -		return error;
> +	if (bt541->num_keycodes > 0) {

I think this check is not needed and "if" can be folded into "else"
above. But anyways, do you mind if I rewrite it as follows:

	...

	n_keycodes = device_property_count_u32(&client->dev, "linux,keycodes");
	if (n_keycodes < 0) {
		error = n_keycodes;
		if (error != -EINVAL && error != -ENODATA) {
			dev_err(&client->dev,
				"Failed to count \"linux,keycodes\" property: %d\n",
				error);
			return error;
		}
	} else if (n_keycodes > 0) {
		if (n_keycodes > ARRAY_SIZE(bt541->keycodes)) {
			dev_err(&client->dev,
				"too many keys defined (%d)\n", n_keycodes);
			return -EINVAL;
		}

		error = device_property_read_u32_array(&client->dev,
						       "linux,keycodes",
						       bt541->keycodes,
						       n_keycodes);
		if (error) {
			dev_err(&client->dev,
				"Unable to parse \"linux,keycodes\" property: %d\n",
				error);
			return error;
		}

		bt541->num_keycodes = n_keycodes;
	}


Or maybe to avoid checking for specific error codes we should do:

	if (device_property_present(&client->dev, "linux,keycodes")) {
		bt541->num_keycodes = device_property_count_u32(&client->dev,
								"linux,keycodes");
		if (bt541->num_keycodes < 0) {
			error = bt541->num_keycodes;
			dev_err(&client->dev, ...);
			return error;
		}

		...
	}


Thanks.

-- 
Dmitry

Re: [PATCH] Input: zinitix - Don't fail if linux,keycodes prop is absent

Posted by Nikita Travkin 1 month, 3 weeks ago

Dmitry Torokhov писал(а) 03.10.2024 16:43:
> Hi Nikita,
> 
> On Wed, Oct 02, 2024 at 06:01:48PM +0500, Nikita Travkin wrote:
>> When initially adding the touchkey support, a mistake was made in the
>> property parsing code. The possible negative errno from
>> device_property_count_u32() was never checked, which was an oversight
>> left from converting to it from the of_property as part of the review
>> fixes.
>> 
>> Re-add the correct handling of the absent property, in which case zero
>> touchkeys should be assumed, which would disable the feature.
>> 
>> Reported-by: Jakob Hauser <jahau@rocketmail.com>
>> Tested-by: Jakob Hauser <jahau@rocketmail.com>
>> Fixes: 075d9b22c8fe ("Input: zinitix - add touchkey support")
>> Signed-off-by: Nikita Travkin <nikita@trvn.ru>
>> ---
>>  drivers/input/touchscreen/zinitix.c | 33 ++++++++++++++++++++++-----------
>>  1 file changed, 22 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/input/touchscreen/zinitix.c b/drivers/input/touchscreen/zinitix.c
>> index 52b3950460e2..1f726653940c 100644
>> --- a/drivers/input/touchscreen/zinitix.c
>> +++ b/drivers/input/touchscreen/zinitix.c
>> @@ -645,19 +645,30 @@ static int zinitix_ts_probe(struct i2c_client *client)
>>  		return error;
>>  	}
>>  
>> -	bt541->num_keycodes = device_property_count_u32(&client->dev, "linux,keycodes");
>> -	if (bt541->num_keycodes > ARRAY_SIZE(bt541->keycodes)) {
>> -		dev_err(&client->dev, "too many keys defined (%d)\n", bt541->num_keycodes);
>> -		return -EINVAL;
>> +	error = device_property_count_u32(&client->dev, "linux,keycodes");
>> +	if (error == -EINVAL || error == -ENODATA) {
>> +		bt541->num_keycodes = 0;
>> +	} else if (error < 0) {
>> +		dev_err(&client->dev, "Failed to count \"linux,keycodes\" property: %d\n", error);
>> +		return error;
>> +	} else {
>> +		bt541->num_keycodes = error;
>>  	}
>>  
>> -	error = device_property_read_u32_array(&client->dev, "linux,keycodes",
>> -					       bt541->keycodes,
>> -					       bt541->num_keycodes);
>> -	if (error) {
>> -		dev_err(&client->dev,
>> -			"Unable to parse \"linux,keycodes\" property: %d\n", error);
>> -		return error;
>> +	if (bt541->num_keycodes > 0) {
> 
> I think this check is not needed and "if" can be folded into "else"
> above. But anyways, do you mind if I rewrite it as follows:
> 
> 	...
> 
> 	n_keycodes = device_property_count_u32(&client->dev, "linux,keycodes");
> 	if (n_keycodes < 0) {
> 		error = n_keycodes;
> 		if (error != -EINVAL && error != -ENODATA) {
> 			dev_err(&client->dev,
> 				"Failed to count \"linux,keycodes\" property: %d\n",
> 				error);
> 			return error;
> 		}
> 	} else if (n_keycodes > 0) {
> 		if (n_keycodes > ARRAY_SIZE(bt541->keycodes)) {
> 			dev_err(&client->dev,
> 				"too many keys defined (%d)\n", n_keycodes);
> 			return -EINVAL;
> 		}
> 
> 		error = device_property_read_u32_array(&client->dev,
> 						       "linux,keycodes",
> 						       bt541->keycodes,
> 						       n_keycodes);
> 		if (error) {
> 			dev_err(&client->dev,
> 				"Unable to parse \"linux,keycodes\" property: %d\n",
> 				error);
> 			return error;
> 		}
> 
> 		bt541->num_keycodes = n_keycodes;
> 	}
> 
> 
> Or maybe to avoid checking for specific error codes we should do:
> 
> 	if (device_property_present(&client->dev, "linux,keycodes")) {
> 		bt541->num_keycodes = device_property_count_u32(&client->dev,
> 								"linux,keycodes");
> 		if (bt541->num_keycodes < 0) {
> 			error = bt541->num_keycodes;
> 			dev_err(&client->dev, ...);
> 			return error;
> 		}
> 
> 		...
> 	}
> 

Oh, yeah, I didn't think of that but explicitly checking the presence
makes the code easier to read. I think both options are fine but I'd
prefer the (imo) easier to read second one. Should I submit a v2 or
you're planning to fast-track it?

Thank you for looking at this!
Nikita

> 
> Thanks.

Re: [PATCH] Input: zinitix - Don't fail if linux,keycodes prop is absent

Posted by Dmitry Torokhov 1 month, 3 weeks ago

On Thu, Oct 03, 2024 at 06:08:57PM +0500, Nikita Travkin wrote:
> Dmitry Torokhov писал(а) 03.10.2024 16:43:
> > Hi Nikita,
> > 
> > On Wed, Oct 02, 2024 at 06:01:48PM +0500, Nikita Travkin wrote:
> >> When initially adding the touchkey support, a mistake was made in the
> >> property parsing code. The possible negative errno from
> >> device_property_count_u32() was never checked, which was an oversight
> >> left from converting to it from the of_property as part of the review
> >> fixes.
> >> 
> >> Re-add the correct handling of the absent property, in which case zero
> >> touchkeys should be assumed, which would disable the feature.
> >> 
> >> Reported-by: Jakob Hauser <jahau@rocketmail.com>
> >> Tested-by: Jakob Hauser <jahau@rocketmail.com>
> >> Fixes: 075d9b22c8fe ("Input: zinitix - add touchkey support")
> >> Signed-off-by: Nikita Travkin <nikita@trvn.ru>
> >> ---
> >>  drivers/input/touchscreen/zinitix.c | 33 ++++++++++++++++++++++-----------
> >>  1 file changed, 22 insertions(+), 11 deletions(-)
> >> 
> >> diff --git a/drivers/input/touchscreen/zinitix.c b/drivers/input/touchscreen/zinitix.c
> >> index 52b3950460e2..1f726653940c 100644
> >> --- a/drivers/input/touchscreen/zinitix.c
> >> +++ b/drivers/input/touchscreen/zinitix.c
> >> @@ -645,19 +645,30 @@ static int zinitix_ts_probe(struct i2c_client *client)
> >>  		return error;
> >>  	}
> >>  
> >> -	bt541->num_keycodes = device_property_count_u32(&client->dev, "linux,keycodes");
> >> -	if (bt541->num_keycodes > ARRAY_SIZE(bt541->keycodes)) {
> >> -		dev_err(&client->dev, "too many keys defined (%d)\n", bt541->num_keycodes);
> >> -		return -EINVAL;
> >> +	error = device_property_count_u32(&client->dev, "linux,keycodes");
> >> +	if (error == -EINVAL || error == -ENODATA) {
> >> +		bt541->num_keycodes = 0;
> >> +	} else if (error < 0) {
> >> +		dev_err(&client->dev, "Failed to count \"linux,keycodes\" property: %d\n", error);
> >> +		return error;
> >> +	} else {
> >> +		bt541->num_keycodes = error;
> >>  	}
> >>  
> >> -	error = device_property_read_u32_array(&client->dev, "linux,keycodes",
> >> -					       bt541->keycodes,
> >> -					       bt541->num_keycodes);
> >> -	if (error) {
> >> -		dev_err(&client->dev,
> >> -			"Unable to parse \"linux,keycodes\" property: %d\n", error);
> >> -		return error;
> >> +	if (bt541->num_keycodes > 0) {
> > 
> > I think this check is not needed and "if" can be folded into "else"
> > above. But anyways, do you mind if I rewrite it as follows:
> > 
> > 	...
> > 
> > 	n_keycodes = device_property_count_u32(&client->dev, "linux,keycodes");
> > 	if (n_keycodes < 0) {
> > 		error = n_keycodes;
> > 		if (error != -EINVAL && error != -ENODATA) {
> > 			dev_err(&client->dev,
> > 				"Failed to count \"linux,keycodes\" property: %d\n",
> > 				error);
> > 			return error;
> > 		}
> > 	} else if (n_keycodes > 0) {
> > 		if (n_keycodes > ARRAY_SIZE(bt541->keycodes)) {
> > 			dev_err(&client->dev,
> > 				"too many keys defined (%d)\n", n_keycodes);
> > 			return -EINVAL;
> > 		}
> > 
> > 		error = device_property_read_u32_array(&client->dev,
> > 						       "linux,keycodes",
> > 						       bt541->keycodes,
> > 						       n_keycodes);
> > 		if (error) {
> > 			dev_err(&client->dev,
> > 				"Unable to parse \"linux,keycodes\" property: %d\n",
> > 				error);
> > 			return error;
> > 		}
> > 
> > 		bt541->num_keycodes = n_keycodes;
> > 	}
> > 
> > 
> > Or maybe to avoid checking for specific error codes we should do:
> > 
> > 	if (device_property_present(&client->dev, "linux,keycodes")) {
> > 		bt541->num_keycodes = device_property_count_u32(&client->dev,
> > 								"linux,keycodes");
> > 		if (bt541->num_keycodes < 0) {
> > 			error = bt541->num_keycodes;
> > 			dev_err(&client->dev, ...);
> > 			return error;
> > 		}
> > 
> > 		...
> > 	}
> > 
> 
> Oh, yeah, I didn't think of that but explicitly checking the presence
> makes the code easier to read. I think both options are fine but I'd
> prefer the (imo) easier to read second one. Should I submit a v2 or
> you're planning to fast-track it?

Please submit v2 since you have the hardware to do a quick test.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: zinitix - Don't fail if linux,keycodes prop is absent

Posted by Linus Walleij 1 month, 3 weeks ago

On Wed, Oct 2, 2024 at 3:02 PM Nikita Travkin <nikita@trvn.ru> wrote:

> When initially adding the touchkey support, a mistake was made in the
> property parsing code. The possible negative errno from
> device_property_count_u32() was never checked, which was an oversight
> left from converting to it from the of_property as part of the review
> fixes.
>
> Re-add the correct handling of the absent property, in which case zero
> touchkeys should be assumed, which would disable the feature.
>
> Reported-by: Jakob Hauser <jahau@rocketmail.com>
> Tested-by: Jakob Hauser <jahau@rocketmail.com>
> Fixes: 075d9b22c8fe ("Input: zinitix - add touchkey support")
> Signed-off-by: Nikita Travkin <nikita@trvn.ru>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

BTW: Nikita have you noticed and weird offsets in your Zinitix
touchscreens? Mine seem to be off and I need to put my
fingers a bit below the actual target on the screen, consistently.
I was thinking maybe calibration support is necessary.

Yours,
Linus Walleij

Re: [PATCH] Input: zinitix - Don't fail if linux,keycodes prop is absent

Posted by Nikita Travkin 1 month, 3 weeks ago

Linus Walleij писал(а) 02.10.2024 19:11:
> On Wed, Oct 2, 2024 at 3:02 PM Nikita Travkin <nikita@trvn.ru> wrote:
> 
>> When initially adding the touchkey support, a mistake was made in the
>> property parsing code. The possible negative errno from
>> device_property_count_u32() was never checked, which was an oversight
>> left from converting to it from the of_property as part of the review
>> fixes.
>>
>> Re-add the correct handling of the absent property, in which case zero
>> touchkeys should be assumed, which would disable the feature.
>>
>> Reported-by: Jakob Hauser <jahau@rocketmail.com>
>> Tested-by: Jakob Hauser <jahau@rocketmail.com>
>> Fixes: 075d9b22c8fe ("Input: zinitix - add touchkey support")
>> Signed-off-by: Nikita Travkin <nikita@trvn.ru>
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> BTW: Nikita have you noticed and weird offsets in your Zinitix
> touchscreens? Mine seem to be off and I need to put my
> fingers a bit below the actual target on the screen, consistently.
> I was thinking maybe calibration support is necessary.

I for sure noticed this in the context of touchkeys: On the device I
have, if you don't enable the touchkeys, the controller assigns
the lines connected to them to the touch grid, which offsets
the real touchscreen by two lines. Effectively this means that
touch surface is stretched a bit below the screen, and i.e. touching
at the very bottom will produce a touch event a bit above
the actual touch point. Enabling touchkeys reassigns those lines
and then the display is working correctly.

This was the prime reason why I've even made the tkey series in
the first place :D

> 
> Yours,
> Linus Walleij

Re: [PATCH] Input: zinitix - Don't fail if linux,keycodes prop is absent

Posted by Linus Walleij 1 month, 3 weeks ago

On Wed, Oct 2, 2024 at 4:48 PM Nikita Travkin <nikita@trvn.ru> wrote:

> > BTW: Nikita have you noticed and weird offsets in your Zinitix
> > touchscreens? Mine seem to be off and I need to put my
> > fingers a bit below the actual target on the screen, consistently.
> > I was thinking maybe calibration support is necessary.
>
> I for sure noticed this in the context of touchkeys: On the device I
> have, if you don't enable the touchkeys, the controller assigns
> the lines connected to them to the touch grid, which offsets
> the real touchscreen by two lines. Effectively this means that
> touch surface is stretched a bit below the screen, and i.e. touching
> at the very bottom will produce a touch event a bit above
> the actual touch point. Enabling touchkeys reassigns those lines
> and then the display is working correctly.
>
> This was the prime reason why I've even made the tkey series in
> the first place :D

It's embarrassing that I was so focused on just testing the touchkey
support to not notice that it actually fixes this issue for me too :D

Excellent, two problems solved.

Yours,
Linus Walleij