| Message ID | 20230217025200.203833-1-marex@denx.de |
|---|---|
| State | Accepted |
| Commit | e1141b0c625e495006c814edc3ffc58ef9ee86b5 |
| Headers | show |
| Series | [v2] Input: ili210x - Probe even if no resolution information | expand |
Hi Marek, On Fri, Feb 17, 2023 at 03:52:00AM +0100, Marek Vasut wrote: > Probe the touch controller driver even if resolution information is not > available. This can happen e.g. in case the touch controller suffered a > failed firmware update and is stuck in bootloader mode. > > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > Cc: Joe Hung <joe_hung@ilitek.com> > Cc: Luca Hsu <luca_hsu@ilitek.com> > --- > V2: Add dev_warn() in case resolution is invalid > --- > drivers/input/touchscreen/ili210x.c | 28 +++++++++++++++++++--------- > 1 file changed, 19 insertions(+), 9 deletions(-) > > diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c > index 4897fafa4204d..d64b6d77d2e08 100644 > --- a/drivers/input/touchscreen/ili210x.c > +++ b/drivers/input/touchscreen/ili210x.c > @@ -370,22 +370,33 @@ static int ili251x_firmware_update_resolution(struct device *dev) > > /* The firmware update blob might have changed the resolution. */ > error = priv->chip->read_reg(client, REG_PANEL_INFO, &rs, sizeof(rs)); > - if (error) > - return error; > + if (!error) { > + resx = le16_to_cpup((__le16 *)rs); > + resy = le16_to_cpup((__le16 *)(rs + 2)); > > - resx = le16_to_cpup((__le16 *)rs); > - resy = le16_to_cpup((__le16 *)(rs + 2)); > + /* The value reported by the firmware is invalid. */ > + if (!resx || resx == 0xffff || !resy || resy == 0xffff) > + error = -EINVAL; > + } > > - /* The value reported by the firmware is invalid. */ > - if (!resx || resx == 0xffff || !resy || resy == 0xffff) > - return -EINVAL; > + /* > + * In case of error, the firmware might be stuck in bootloader mode, > + * e.g. after a failed firmware update. Set maximum resolution, but > + * do not fail to probe, so the user can re-trigger the firmware > + * update and recover the touch controller. > + */ > + if (error) { > + dev_warn(dev, "Invalid resolution reported by controller.\n"); > + resx = 16384; > + resy = 16384; > + } > > input_abs_set_max(priv->input, ABS_X, resx - 1); > input_abs_set_max(priv->input, ABS_Y, resy - 1); > input_abs_set_max(priv->input, ABS_MT_POSITION_X, resx - 1); > input_abs_set_max(priv->input, ABS_MT_POSITION_Y, resy - 1); > > - return 0; > + return error; I think this will make ili251x_firmware_update_cached_state() continue failing when it reports invalid coordinates. Was this intended? > } > > static ssize_t ili251x_firmware_update_firmware_version(struct device *dev) > @@ -980,7 +991,6 @@ static int ili210x_i2c_probe(struct i2c_client *client) > if (error) { > dev_err(dev, "Unable to cache firmware information, err: %d\n", > error); > - return error; > } > touchscreen_parse_properties(input, true, &priv->prop); > > -- > 2.39.1 > Thanks.
On 2/21/23 20:40, Dmitry Torokhov wrote: > Hi Marek, > > On Fri, Feb 17, 2023 at 03:52:00AM +0100, Marek Vasut wrote: >> Probe the touch controller driver even if resolution information is not >> available. This can happen e.g. in case the touch controller suffered a >> failed firmware update and is stuck in bootloader mode. >> >> Signed-off-by: Marek Vasut <marex@denx.de> >> --- >> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> >> Cc: Joe Hung <joe_hung@ilitek.com> >> Cc: Luca Hsu <luca_hsu@ilitek.com> >> --- >> V2: Add dev_warn() in case resolution is invalid >> --- >> drivers/input/touchscreen/ili210x.c | 28 +++++++++++++++++++--------- >> 1 file changed, 19 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c >> index 4897fafa4204d..d64b6d77d2e08 100644 >> --- a/drivers/input/touchscreen/ili210x.c >> +++ b/drivers/input/touchscreen/ili210x.c >> @@ -370,22 +370,33 @@ static int ili251x_firmware_update_resolution(struct device *dev) >> >> /* The firmware update blob might have changed the resolution. */ >> error = priv->chip->read_reg(client, REG_PANEL_INFO, &rs, sizeof(rs)); >> - if (error) >> - return error; >> + if (!error) { >> + resx = le16_to_cpup((__le16 *)rs); >> + resy = le16_to_cpup((__le16 *)(rs + 2)); >> >> - resx = le16_to_cpup((__le16 *)rs); >> - resy = le16_to_cpup((__le16 *)(rs + 2)); >> + /* The value reported by the firmware is invalid. */ >> + if (!resx || resx == 0xffff || !resy || resy == 0xffff) >> + error = -EINVAL; >> + } >> >> - /* The value reported by the firmware is invalid. */ >> - if (!resx || resx == 0xffff || !resy || resy == 0xffff) >> - return -EINVAL; >> + /* >> + * In case of error, the firmware might be stuck in bootloader mode, >> + * e.g. after a failed firmware update. Set maximum resolution, but >> + * do not fail to probe, so the user can re-trigger the firmware >> + * update and recover the touch controller. >> + */ >> + if (error) { >> + dev_warn(dev, "Invalid resolution reported by controller.\n"); >> + resx = 16384; >> + resy = 16384; >> + } >> >> input_abs_set_max(priv->input, ABS_X, resx - 1); >> input_abs_set_max(priv->input, ABS_Y, resy - 1); >> input_abs_set_max(priv->input, ABS_MT_POSITION_X, resx - 1); >> input_abs_set_max(priv->input, ABS_MT_POSITION_Y, resy - 1); >> >> - return 0; >> + return error; > > I think this will make ili251x_firmware_update_cached_state() continue > failing when it reports invalid coordinates. Was this intended? This is actually correct, ili251x_firmware_update_cached_state() will fail, but ili210x_i2c_probe() won't stop there anymore, see the second hunk of this patch. The driver will instantiate the controller, so user can load correct firmware into it and recover the hardware.
On Tue, Feb 21, 2023 at 09:12:29PM +0100, Marek Vasut wrote: > On 2/21/23 20:40, Dmitry Torokhov wrote: > > Hi Marek, > > > > On Fri, Feb 17, 2023 at 03:52:00AM +0100, Marek Vasut wrote: > > > Probe the touch controller driver even if resolution information is not > > > available. This can happen e.g. in case the touch controller suffered a > > > failed firmware update and is stuck in bootloader mode. > > > > > > Signed-off-by: Marek Vasut <marex@denx.de> > > > --- > > > Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> > > > Cc: Joe Hung <joe_hung@ilitek.com> > > > Cc: Luca Hsu <luca_hsu@ilitek.com> > > > --- > > > V2: Add dev_warn() in case resolution is invalid > > > --- > > > drivers/input/touchscreen/ili210x.c | 28 +++++++++++++++++++--------- > > > 1 file changed, 19 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c > > > index 4897fafa4204d..d64b6d77d2e08 100644 > > > --- a/drivers/input/touchscreen/ili210x.c > > > +++ b/drivers/input/touchscreen/ili210x.c > > > @@ -370,22 +370,33 @@ static int ili251x_firmware_update_resolution(struct device *dev) > > > /* The firmware update blob might have changed the resolution. */ > > > error = priv->chip->read_reg(client, REG_PANEL_INFO, &rs, sizeof(rs)); > > > - if (error) > > > - return error; > > > + if (!error) { > > > + resx = le16_to_cpup((__le16 *)rs); > > > + resy = le16_to_cpup((__le16 *)(rs + 2)); > > > - resx = le16_to_cpup((__le16 *)rs); > > > - resy = le16_to_cpup((__le16 *)(rs + 2)); > > > + /* The value reported by the firmware is invalid. */ > > > + if (!resx || resx == 0xffff || !resy || resy == 0xffff) > > > + error = -EINVAL; > > > + } > > > - /* The value reported by the firmware is invalid. */ > > > - if (!resx || resx == 0xffff || !resy || resy == 0xffff) > > > - return -EINVAL; > > > + /* > > > + * In case of error, the firmware might be stuck in bootloader mode, > > > + * e.g. after a failed firmware update. Set maximum resolution, but > > > + * do not fail to probe, so the user can re-trigger the firmware > > > + * update and recover the touch controller. > > > + */ > > > + if (error) { > > > + dev_warn(dev, "Invalid resolution reported by controller.\n"); > > > + resx = 16384; > > > + resy = 16384; > > > + } > > > input_abs_set_max(priv->input, ABS_X, resx - 1); > > > input_abs_set_max(priv->input, ABS_Y, resy - 1); > > > input_abs_set_max(priv->input, ABS_MT_POSITION_X, resx - 1); > > > input_abs_set_max(priv->input, ABS_MT_POSITION_Y, resy - 1); > > > - return 0; > > > + return error; > > > > I think this will make ili251x_firmware_update_cached_state() continue > > failing when it reports invalid coordinates. Was this intended? > > This is actually correct, ili251x_firmware_update_cached_state() will fail, > but ili210x_i2c_probe() won't stop there anymore, see the second hunk of > this patch. The driver will instantiate the controller, so user can load > correct firmware into it and recover the hardware. I was concerned about call from ili210x_firmware_update_store() which will continue returning error. Thanks.
On 2/21/23 22:14, Dmitry Torokhov wrote: > On Tue, Feb 21, 2023 at 09:12:29PM +0100, Marek Vasut wrote: >> On 2/21/23 20:40, Dmitry Torokhov wrote: >>> Hi Marek, >>> >>> On Fri, Feb 17, 2023 at 03:52:00AM +0100, Marek Vasut wrote: >>>> Probe the touch controller driver even if resolution information is not >>>> available. This can happen e.g. in case the touch controller suffered a >>>> failed firmware update and is stuck in bootloader mode. >>>> >>>> Signed-off-by: Marek Vasut <marex@denx.de> >>>> --- >>>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> >>>> Cc: Joe Hung <joe_hung@ilitek.com> >>>> Cc: Luca Hsu <luca_hsu@ilitek.com> >>>> --- >>>> V2: Add dev_warn() in case resolution is invalid >>>> --- >>>> drivers/input/touchscreen/ili210x.c | 28 +++++++++++++++++++--------- >>>> 1 file changed, 19 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c >>>> index 4897fafa4204d..d64b6d77d2e08 100644 >>>> --- a/drivers/input/touchscreen/ili210x.c >>>> +++ b/drivers/input/touchscreen/ili210x.c >>>> @@ -370,22 +370,33 @@ static int ili251x_firmware_update_resolution(struct device *dev) >>>> /* The firmware update blob might have changed the resolution. */ >>>> error = priv->chip->read_reg(client, REG_PANEL_INFO, &rs, sizeof(rs)); >>>> - if (error) >>>> - return error; >>>> + if (!error) { >>>> + resx = le16_to_cpup((__le16 *)rs); >>>> + resy = le16_to_cpup((__le16 *)(rs + 2)); >>>> - resx = le16_to_cpup((__le16 *)rs); >>>> - resy = le16_to_cpup((__le16 *)(rs + 2)); >>>> + /* The value reported by the firmware is invalid. */ >>>> + if (!resx || resx == 0xffff || !resy || resy == 0xffff) >>>> + error = -EINVAL; >>>> + } >>>> - /* The value reported by the firmware is invalid. */ >>>> - if (!resx || resx == 0xffff || !resy || resy == 0xffff) >>>> - return -EINVAL; >>>> + /* >>>> + * In case of error, the firmware might be stuck in bootloader mode, >>>> + * e.g. after a failed firmware update. Set maximum resolution, but >>>> + * do not fail to probe, so the user can re-trigger the firmware >>>> + * update and recover the touch controller. >>>> + */ >>>> + if (error) { >>>> + dev_warn(dev, "Invalid resolution reported by controller.\n"); >>>> + resx = 16384; >>>> + resy = 16384; >>>> + } >>>> input_abs_set_max(priv->input, ABS_X, resx - 1); >>>> input_abs_set_max(priv->input, ABS_Y, resy - 1); >>>> input_abs_set_max(priv->input, ABS_MT_POSITION_X, resx - 1); >>>> input_abs_set_max(priv->input, ABS_MT_POSITION_Y, resy - 1); >>>> - return 0; >>>> + return error; >>> >>> I think this will make ili251x_firmware_update_cached_state() continue >>> failing when it reports invalid coordinates. Was this intended? >> >> This is actually correct, ili251x_firmware_update_cached_state() will fail, >> but ili210x_i2c_probe() won't stop there anymore, see the second hunk of >> this patch. The driver will instantiate the controller, so user can load >> correct firmware into it and recover the hardware. > > I was concerned about call from ili210x_firmware_update_store() which > will continue returning error. It hopefully won't, because at that point the controller would be running new and working firmware, which would report the correct values. If the firmware is broken, then yes, it would fail.
On 2/22/23 00:14, Marek Vasut wrote: > On 2/21/23 22:14, Dmitry Torokhov wrote: >> On Tue, Feb 21, 2023 at 09:12:29PM +0100, Marek Vasut wrote: >>> On 2/21/23 20:40, Dmitry Torokhov wrote: >>>> Hi Marek, >>>> >>>> On Fri, Feb 17, 2023 at 03:52:00AM +0100, Marek Vasut wrote: >>>>> Probe the touch controller driver even if resolution information is >>>>> not >>>>> available. This can happen e.g. in case the touch controller >>>>> suffered a >>>>> failed firmware update and is stuck in bootloader mode. >>>>> >>>>> Signed-off-by: Marek Vasut <marex@denx.de> >>>>> --- >>>>> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> >>>>> Cc: Joe Hung <joe_hung@ilitek.com> >>>>> Cc: Luca Hsu <luca_hsu@ilitek.com> >>>>> --- >>>>> V2: Add dev_warn() in case resolution is invalid >>>>> --- >>>>> drivers/input/touchscreen/ili210x.c | 28 >>>>> +++++++++++++++++++--------- >>>>> 1 file changed, 19 insertions(+), 9 deletions(-) >>>>> >>>>> diff --git a/drivers/input/touchscreen/ili210x.c >>>>> b/drivers/input/touchscreen/ili210x.c >>>>> index 4897fafa4204d..d64b6d77d2e08 100644 >>>>> --- a/drivers/input/touchscreen/ili210x.c >>>>> +++ b/drivers/input/touchscreen/ili210x.c >>>>> @@ -370,22 +370,33 @@ static int >>>>> ili251x_firmware_update_resolution(struct device *dev) >>>>> /* The firmware update blob might have changed the >>>>> resolution. */ >>>>> error = priv->chip->read_reg(client, REG_PANEL_INFO, &rs, >>>>> sizeof(rs)); >>>>> - if (error) >>>>> - return error; >>>>> + if (!error) { >>>>> + resx = le16_to_cpup((__le16 *)rs); >>>>> + resy = le16_to_cpup((__le16 *)(rs + 2)); >>>>> - resx = le16_to_cpup((__le16 *)rs); >>>>> - resy = le16_to_cpup((__le16 *)(rs + 2)); >>>>> + /* The value reported by the firmware is invalid. */ >>>>> + if (!resx || resx == 0xffff || !resy || resy == 0xffff) >>>>> + error = -EINVAL; >>>>> + } >>>>> - /* The value reported by the firmware is invalid. */ >>>>> - if (!resx || resx == 0xffff || !resy || resy == 0xffff) >>>>> - return -EINVAL; >>>>> + /* >>>>> + * In case of error, the firmware might be stuck in bootloader >>>>> mode, >>>>> + * e.g. after a failed firmware update. Set maximum >>>>> resolution, but >>>>> + * do not fail to probe, so the user can re-trigger the firmware >>>>> + * update and recover the touch controller. >>>>> + */ >>>>> + if (error) { >>>>> + dev_warn(dev, "Invalid resolution reported by >>>>> controller.\n"); >>>>> + resx = 16384; >>>>> + resy = 16384; >>>>> + } >>>>> input_abs_set_max(priv->input, ABS_X, resx - 1); >>>>> input_abs_set_max(priv->input, ABS_Y, resy - 1); >>>>> input_abs_set_max(priv->input, ABS_MT_POSITION_X, resx - 1); >>>>> input_abs_set_max(priv->input, ABS_MT_POSITION_Y, resy - 1); >>>>> - return 0; >>>>> + return error; >>>> >>>> I think this will make ili251x_firmware_update_cached_state() continue >>>> failing when it reports invalid coordinates. Was this intended? >>> >>> This is actually correct, ili251x_firmware_update_cached_state() will >>> fail, >>> but ili210x_i2c_probe() won't stop there anymore, see the second hunk of >>> this patch. The driver will instantiate the controller, so user can load >>> correct firmware into it and recover the hardware. >> >> I was concerned about call from ili210x_firmware_update_store() which >> will continue returning error. > > It hopefully won't, because at that point the controller would be > running new and working firmware, which would report the correct values. > If the firmware is broken, then yes, it would fail. Hi, any news on this ? I still have to track this downstream to recover those devices from broken firmware update, it would be really useful to have this also upstream.
On Fri, Feb 17, 2023 at 03:52:00AM +0100, Marek Vasut wrote: > Probe the touch controller driver even if resolution information is not > available. This can happen e.g. in case the touch controller suffered a > failed firmware update and is stuck in bootloader mode. > > Signed-off-by: Marek Vasut <marex@denx.de> Applied, thank you.
On 5/6/23 20:46, Dmitry Torokhov wrote: > On Fri, Feb 17, 2023 at 03:52:00AM +0100, Marek Vasut wrote: >> Probe the touch controller driver even if resolution information is not >> available. This can happen e.g. in case the touch controller suffered a >> failed firmware update and is stuck in bootloader mode. >> >> Signed-off-by: Marek Vasut <marex@denx.de> > > Applied, thank you. Much appreciated, thanks !
diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c index 4897fafa4204d..d64b6d77d2e08 100644 --- a/drivers/input/touchscreen/ili210x.c +++ b/drivers/input/touchscreen/ili210x.c @@ -370,22 +370,33 @@ static int ili251x_firmware_update_resolution(struct device *dev) /* The firmware update blob might have changed the resolution. */ error = priv->chip->read_reg(client, REG_PANEL_INFO, &rs, sizeof(rs)); - if (error) - return error; + if (!error) { + resx = le16_to_cpup((__le16 *)rs); + resy = le16_to_cpup((__le16 *)(rs + 2)); - resx = le16_to_cpup((__le16 *)rs); - resy = le16_to_cpup((__le16 *)(rs + 2)); + /* The value reported by the firmware is invalid. */ + if (!resx || resx == 0xffff || !resy || resy == 0xffff) + error = -EINVAL; + } - /* The value reported by the firmware is invalid. */ - if (!resx || resx == 0xffff || !resy || resy == 0xffff) - return -EINVAL; + /* + * In case of error, the firmware might be stuck in bootloader mode, + * e.g. after a failed firmware update. Set maximum resolution, but + * do not fail to probe, so the user can re-trigger the firmware + * update and recover the touch controller. + */ + if (error) { + dev_warn(dev, "Invalid resolution reported by controller.\n"); + resx = 16384; + resy = 16384; + } input_abs_set_max(priv->input, ABS_X, resx - 1); input_abs_set_max(priv->input, ABS_Y, resy - 1); input_abs_set_max(priv->input, ABS_MT_POSITION_X, resx - 1); input_abs_set_max(priv->input, ABS_MT_POSITION_Y, resy - 1); - return 0; + return error; } static ssize_t ili251x_firmware_update_firmware_version(struct device *dev) @@ -980,7 +991,6 @@ static int ili210x_i2c_probe(struct i2c_client *client) if (error) { dev_err(dev, "Unable to cache firmware information, err: %d\n", error); - return error; } touchscreen_parse_properties(input, true, &priv->prop);
Probe the touch controller driver even if resolution information is not available. This can happen e.g. in case the touch controller suffered a failed firmware update and is stuck in bootloader mode. Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com> Cc: Joe Hung <joe_hung@ilitek.com> Cc: Luca Hsu <luca_hsu@ilitek.com> --- V2: Add dev_warn() in case resolution is invalid --- drivers/input/touchscreen/ili210x.c | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-)