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

[Marek Vasut <marex@xxxxxxx>]

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@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.
>
> 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.