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