Re: [PATCH v3 5/5] Input: exc3000 - add firmware update support

[Lucas Stach <l.stach@pengutronix.de>]

Hi Dmitry,

Am Sonntag, dem 07.03.2021 um 21:47 -0800 schrieb Dmitry Torokhov:
> Hi Lucas,
> 
> On Mon, Jan 25, 2021 at 07:25:27PM +0100, Lucas Stach wrote:
> > This change allows the device firmware to be updated by putting a firmware
> > file in /lib/firmware and providing the name of the file via the update_fw
> > sysfs property. The driver will then flash the firmware image into the
> > controller internal storage and restart the controller to activate the new
> > firmware.
> > 
> > The implementation was done by looking at the the messages passed between
> > the controller and proprietary vendor update tool. Not every detail of the
> > protocol is totally well understood, so the implementation still has some
> > "monkey see, monkey do" parts, as far as they have been found to be required
> > for the update to succeed.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  .../ABI/testing/sysfs-driver-input-exc3000    |  20 ++
> >  drivers/input/touchscreen/exc3000.c           | 240 +++++++++++++++++-
> >  2 files changed, 258 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-driver-input-exc3000 b/Documentation/ABI/testing/sysfs-driver-input-exc3000
> > index 704434b277b0..123a00ccee8b 100644
> > --- a/Documentation/ABI/testing/sysfs-driver-input-exc3000
> > +++ b/Documentation/ABI/testing/sysfs-driver-input-exc3000
> > @@ -24,3 +24,23 @@ Description:	Reports the type identification provided by the touchscreen, for ex
> >  		Access: Read
> >  
> > 
> > 
> > 
> >  		Valid values: Represented as string
> > +
> > +What:		/sys/bus/i2c/devices/xxx/update_fw
> > +Date:		Jan 2021
> > +Contact:	linux-input@vger.kernel.org
> > +Description:	Allows to specify a firlename of a firmware file located in /lib/firmware/ that will be
> > +		used to update the application firmware on the touchscreen controller. For example
> > +		"eeti/eeti_27_0_EDipper_0735.fw"
> 
> I believe the current idiomatic way is to have statically defined
> firmware name (it can still encode vid/pid/model info etc) and do not
> re-implement variable firmware name in every driver.
> 
> I think if this really is required we need to add this feature of
> overriding default firmware name to firmware loader maybe?

One issue I see with the driver provided firmware name is that the
model name and revision can be changed by the flashed firmware, with
the EXC3000 being a i2c device ,we also don't have any stable VID/PID
to use as a key. Which is an issue for the initial firmware flashing.
In that case one would need to know whats currently on the device to be
able to place a firmware file with the correct name.

Also I don't really see how that simplifies the driver code, as all
this is doing is using the passed in string as a file name to fetch the
firmware update file from.

To be clear: I'm not totally opposed to using a driver provided
firmware name, I just see that it complicates some things that are not
an issue with the patch as-is today and would like to understand the
reason for pushing for a driver provided name, before deciding one way
or the other.

Regards,
Lucas