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@xxxxxxx>
> > > ---
> > > Cc: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> > > Cc: Joe Hung <joe_hung@xxxxxxxxxx>
> > > Cc: Luca Hsu <luca_hsu@xxxxxxxxxx>
> > > ---
> > > 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.
--
Dmitry
