Re: [PATCH] Input: elants_i2c: Append hw_version to FW file

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

On Wed, Mar 11, 2015 at 10:09:18AM -0700, Charles Mooney wrote:
> On Tue, Mar 10, 2015 at 6:41 PM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Hi Charlie,
> >
> > On Tue, Mar 10, 2015 at 12:57:03PM -0700, Charlie Mooney wrote:
> >> Currently the elants_i2c driver simply requests a static filename
> >> /lib/firmware/elants_i2c.bin when it gets firmware updates.  This is
> >> a problem if you have two Elan touchscreens using the same driver.
> >> If both touchscreens have different firmwares, you would need to move
> >> the files around in your filesystem when you're updating them so that
> >> they don't get updated with the other's FW.  If you have a read-only
> >> filesystem then this is impossible, even.
> >>
> >> This patch changes the elants_i2c driver to automatically append the
> >> four-hex-digit hw_version of the device onto the name of the FW file
> >> it's requesting for update.  Since different touchscreens should have
> >> a different hw_version's this means the user needs to append the hw
> >> version of the touchscreen he or she intends to update onto the end
> >> of the firmware filename and then the driver will do the rest.
> >>
> >> The firmware filenames it looks for now are of the form:
> >>
> >>   elants_i2c_${HW_VERSION}.bin
> >>
> >>   eg:
> >>
> >>   elants_i2c_2a44.bin
> >>
> >> Signed-off-by: Charlie Mooney <charliemooney@chromium.org>
> >> ---
> >>  drivers/input/touchscreen/elants_i2c.c | 13 ++++++++++---
> >>  1 file changed, 10 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/input/touchscreen/elants_i2c.c b/drivers/input/touchscreen/elants_i2c.c
> >> index 926c58e..d34ba57 100644
> >> --- a/drivers/input/touchscreen/elants_i2c.c
> >> +++ b/drivers/input/touchscreen/elants_i2c.c
> >> @@ -98,7 +98,10 @@
> >>  #define MAX_FW_UPDATE_RETRIES        30
> >>
> >>  #define ELAN_FW_PAGESIZE     132
> >> -#define ELAN_FW_FILENAME     "elants_i2c.bin"
> >> +#define      ELAN_FW_BASE_FILENAME   "elants_i2c"
> >> +#define ELAN_FW_EXTENSION    "bin"
> >> +#define ELAN_FW_FILENAME_MAX_LEN     (ARRAY_SIZE(ELAN_FW_BASE_FILENAME) + \
> >> +                                      ARRAY_SIZE(ELAN_FW_EXTENSION) + 5)
> >
> > Looked at this again and I don't really like how we need to maintain the
> > MAX_LEN and make sure it is in sync with the format string. How about
> > the patch below instead?
> >
> > Thanks.
> >
> > --
> > Dmitry
> >
> >
> > Input: elants_i2c - append hw_version to FW file
> >
> > From: Charlie Mooney <charliemooney@chromium.org>
> >
> > Currently the elants_i2c driver simply requests a static filename
> > /lib/firmware/elants_i2c.bin when it gets firmware updates.  This is a
> > problem if you have two Elan touchscreens using the same driver.  If both
> > touchscreens have different firmwares, you would need to move the files
> > around in your filesystem when you're updating them so that they don't get
> > updated with the other's FW.  If you have a read-only filesystem then this
> > is impossible, even.
> >
> > This patch changes the elants_i2c driver to automatically append the
> > four-hex-digit hw_version of the device onto the name of the FW file it's
> > requesting for update.  Since different touchscreens should have a
> > different hw_version's this means the user needs to append the hw version
> > of the touchscreen he or she intends to update onto the end of the firmware
> > filename and then the driver will do the rest.
> >
> > The firmware filenames it looks for now are of the form:
> >
> >   elants_i2c_${HW_VERSION}.bin
> >
> >   eg:
> >
> >   elants_i2c_2a44.bin
> >
> > Signed-off-by: Charlie Mooney <charliemooney@chromium.org>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/input/touchscreen/elants_i2c.c |   14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/elants_i2c.c b/drivers/input/touchscreen/elants_i2c.c
> > index 926c58e..43b3c9c 100644
> > --- a/drivers/input/touchscreen/elants_i2c.c
> > +++ b/drivers/input/touchscreen/elants_i2c.c
> > @@ -98,7 +98,6 @@
> >  #define MAX_FW_UPDATE_RETRIES  30
> >
> >  #define ELAN_FW_PAGESIZE       132
> > -#define ELAN_FW_FILENAME       "elants_i2c.bin"
> >
> >  /* calibration timeout definition */
> >  #define ELAN_CALI_TIMEOUT_MSEC 10000
> > @@ -697,12 +696,19 @@ static int elants_i2c_fw_update(struct elants_data *ts)
> >  {
> >         struct i2c_client *client = ts->client;
> >         const struct firmware *fw;
> > +       char *fw_name;
> >         int error;
> >
> > -       error = request_firmware(&fw, ELAN_FW_FILENAME, &client->dev);
> > +       fw_name = kasprintf(GFP_KERNEL, "elants_i2c_%4x.bin", ts->hw_version);
> > +       if (!fw_name)
> > +               return -ENOMEM;
> > +
> > +       dev_info(&client->dev, "requesting fw name = %s\n", fw_name);
> > +       error = request_firmware(&fw, fw_name, &client->dev);
> > +       kfree(fw_name);
> >         if (error) {
> > -               dev_err(&client->dev, "failed to request firmware %s: %d\n",
> > -                       ELAN_FW_FILENAME, error);
> > +               dev_err(&client->dev, "failed to request firmware: %d\n",
> > +                       error);
> >                 return error;
> >         }
> >
> 
> I agree, that does indeed look better.  kasprintf() is a smart choice.

Thanks, applied the 2nd version then.

-- 
Dmitry