Re: [PATCH v2] Input: ili210x - Probe even if no resolution information

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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.

-- 
Dmitry