| Message ID | 20241002-zinitix-no-keycodes-v1-1-e84029601491@trvn.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Input: zinitix - Don't fail if linux,keycodes prop is absent | expand |
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
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
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
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 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.
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.
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);