| Message ID | 20200731075714.10608-1-jiada_wang@mentor.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] Input: atmel_mxt_ts - allow specification of firmware file name | expand |
31.07.2020 10:57, Jiada Wang пишет: > From: Nick Dyer <nick.dyer@itdev.co.uk> > > On platforms which have multiple device instances using this driver, the > firmware may be different on each device. This patch makes the user give > the name of the firmware file when flashing. > > This also prevents accidental triggering of the firmware load process. > > Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk> > Acked-by: Benson Leung <bleung@chromium.org> > Acked-by: Yufeng Shen <miletus@chromium.org> > (cherry picked from ndyer/linux/for-upstream commit 76ebb7cee971cb42dfb0a3a9224403b8b09abcf1) > [gdavis: Forward port and fix conflicts.] > Signed-off-by: George G. Davis <george_davis@mentor.com> > Signed-off-by: Jiada Wang <jiada_wang@mentor.com> > --- > drivers/input/touchscreen/atmel_mxt_ts.c | 42 ++++++++++++++++++++---- > 1 file changed, 36 insertions(+), 6 deletions(-) > > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c > index a2189739e30f..024dee7a3571 100644 > --- a/drivers/input/touchscreen/atmel_mxt_ts.c > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c > @@ -30,8 +30,6 @@ > #include <media/videobuf2-v4l2.h> > #include <media/videobuf2-vmalloc.h> > > -/* Firmware files */ > -#define MXT_FW_NAME "maxtouch.fw" > #define MXT_CFG_NAME "maxtouch.cfg" > #define MXT_CFG_MAGIC "OBP_RAW V1" > > @@ -308,6 +306,7 @@ struct mxt_data { > struct t7_config t7_cfg; > struct mxt_dbg dbg; > struct gpio_desc *reset_gpio; > + char *fw_name; Hello, Jiada! Have you decided to abandon the patch which allowed to specify firmware name in a device tree? > /* Cached parameters from object table */ > u16 T5_address; > @@ -2766,7 +2765,7 @@ static int mxt_check_firmware_format(struct device *dev, > return -EINVAL; > } > > -static int mxt_load_fw(struct device *dev, const char *fn) > +static int mxt_load_fw(struct device *dev) > { > struct mxt_data *data = dev_get_drvdata(dev); > const struct firmware *fw = NULL; > @@ -2776,9 +2775,9 @@ static int mxt_load_fw(struct device *dev, const char *fn) > unsigned int frame = 0; > int ret; > > - ret = request_firmware(&fw, fn, dev); > + ret = request_firmware(&fw, data->fw_name, dev); > if (ret) { > - dev_err(dev, "Unable to open firmware %s\n", fn); > + dev_err(dev, "Unable to open firmware %s\n", data->fw_name); > return ret; > } > > @@ -2887,6 +2886,33 @@ static int mxt_load_fw(struct device *dev, const char *fn) > return ret; > } > > +static int mxt_update_file_name(struct device *dev, char **file_name, > + const char *buf, size_t count) > +{ > + char *file_name_tmp; > + > + /* Simple sanity check */ > + if (count > 64) { > + dev_warn(dev, "File name too long\n"); > + return -EINVAL; > + } > + > + file_name_tmp = krealloc(*file_name, count + 1, GFP_KERNEL); > + if (!file_name_tmp) > + return -ENOMEM; The allocated string is never release, this is not good. Wouldn't it be nicer to make data->fw_name a fixed-size string? > + *file_name = file_name_tmp; > + memcpy(*file_name, buf, count); > + > + /* Echo into the sysfs entry may append newline at the end of buf */ > + if (buf[count - 1] == '\n') > + (*file_name)[count - 1] = '\0'; > + else > + (*file_name)[count] = '\0'; What about to use strscpy? > + return 0; > +} > + > static ssize_t mxt_update_fw_store(struct device *dev, > struct device_attribute *attr, > const char *buf, size_t count) > @@ -2894,7 +2920,11 @@ static ssize_t mxt_update_fw_store(struct device *dev, > struct mxt_data *data = dev_get_drvdata(dev); > int error; > > - error = mxt_load_fw(dev, MXT_FW_NAME); > + error = mxt_update_file_name(dev, &data->fw_name, buf, count); > + if (error) > + return error; This change breaks old behavior, I'm not sure that it's acceptable. The default name should remain to be "maxtouch.fw", IMO. > + error = mxt_load_fw(dev); > if (error) { > dev_err(dev, "The firmware update failed(%d)\n", error); > count = error; >
Hi Dmitry On 2020/08/10 5:11, Dmitry Osipenko wrote: > 31.07.2020 10:57, Jiada Wang пишет: >> From: Nick Dyer <nick.dyer@itdev.co.uk> >> >> On platforms which have multiple device instances using this driver, the >> firmware may be different on each device. This patch makes the user give >> the name of the firmware file when flashing. >> >> This also prevents accidental triggering of the firmware load process. >> >> Signed-off-by: Nick Dyer <nick.dyer@itdev.co.uk> >> Acked-by: Benson Leung <bleung@chromium.org> >> Acked-by: Yufeng Shen <miletus@chromium.org> >> (cherry picked from ndyer/linux/for-upstream commit 76ebb7cee971cb42dfb0a3a9224403b8b09abcf1) >> [gdavis: Forward port and fix conflicts.] >> Signed-off-by: George G. Davis <george_davis@mentor.com> >> Signed-off-by: Jiada Wang <jiada_wang@mentor.com> >> --- >> drivers/input/touchscreen/atmel_mxt_ts.c | 42 ++++++++++++++++++++---- >> 1 file changed, 36 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c >> index a2189739e30f..024dee7a3571 100644 >> --- a/drivers/input/touchscreen/atmel_mxt_ts.c >> +++ b/drivers/input/touchscreen/atmel_mxt_ts.c >> @@ -30,8 +30,6 @@ >> #include <media/videobuf2-v4l2.h> >> #include <media/videobuf2-vmalloc.h> >> >> -/* Firmware files */ >> -#define MXT_FW_NAME "maxtouch.fw" >> #define MXT_CFG_NAME "maxtouch.cfg" >> #define MXT_CFG_MAGIC "OBP_RAW V1" >> >> @@ -308,6 +306,7 @@ struct mxt_data { >> struct t7_config t7_cfg; >> struct mxt_dbg dbg; >> struct gpio_desc *reset_gpio; >> + char *fw_name; > > Hello, Jiada! > > Have you decided to abandon the patch which allowed to specify firmware > name in a device tree? > In v11 patch-set, there is a patch allows user to specify config file name via device-tree and sysfs, I will drop device-tree part, since Dmitry Torokhov commented he would like to change the behavior of firmware loading during boot time, but I will keep sysfs interface part, which is similar to this patch but for configuration file. >> /* Cached parameters from object table */ >> u16 T5_address; >> @@ -2766,7 +2765,7 @@ static int mxt_check_firmware_format(struct device *dev, >> return -EINVAL; >> } >> >> -static int mxt_load_fw(struct device *dev, const char *fn) >> +static int mxt_load_fw(struct device *dev) >> { >> struct mxt_data *data = dev_get_drvdata(dev); >> const struct firmware *fw = NULL; >> @@ -2776,9 +2775,9 @@ static int mxt_load_fw(struct device *dev, const char *fn) >> unsigned int frame = 0; >> int ret; >> >> - ret = request_firmware(&fw, fn, dev); >> + ret = request_firmware(&fw, data->fw_name, dev); >> if (ret) { >> - dev_err(dev, "Unable to open firmware %s\n", fn); >> + dev_err(dev, "Unable to open firmware %s\n", data->fw_name); >> return ret; >> } >> >> @@ -2887,6 +2886,33 @@ static int mxt_load_fw(struct device *dev, const char *fn) >> return ret; >> } >> >> +static int mxt_update_file_name(struct device *dev, char **file_name, >> + const char *buf, size_t count) >> +{ >> + char *file_name_tmp; >> + >> + /* Simple sanity check */ >> + if (count > 64) { >> + dev_warn(dev, "File name too long\n"); >> + return -EINVAL; >> + } >> + >> + file_name_tmp = krealloc(*file_name, count + 1, GFP_KERNEL); >> + if (!file_name_tmp) >> + return -ENOMEM; > > The allocated string is never release, this is not good. > > Wouldn't it be nicer to make data->fw_name a fixed-size string? OK, I will change to a fixed-size string > >> + *file_name = file_name_tmp; >> + memcpy(*file_name, buf, count); >> + >> + /* Echo into the sysfs entry may append newline at the end of buf */ >> + if (buf[count - 1] == '\n') >> + (*file_name)[count - 1] = '\0'; >> + else >> + (*file_name)[count] = '\0'; > > What about to use strscpy? > Thanks for the advice, I will replace with strscpy >> + return 0; >> +} >> + >> static ssize_t mxt_update_fw_store(struct device *dev, >> struct device_attribute *attr, >> const char *buf, size_t count) >> @@ -2894,7 +2920,11 @@ static ssize_t mxt_update_fw_store(struct device *dev, >> struct mxt_data *data = dev_get_drvdata(dev); >> int error; >> >> - error = mxt_load_fw(dev, MXT_FW_NAME); >> + error = mxt_update_file_name(dev, &data->fw_name, buf, count); >> + if (error) >> + return error; > > This change breaks old behavior, I'm not sure that it's acceptable. The > default name should remain to be "maxtouch.fw", IMO. it makes sense, I will update to write default "maxtouch.fw", in case user doesn't specify a file name in sysfs write operation Thanks, Jiada > >> + error = mxt_load_fw(dev); >> if (error) { >> dev_err(dev, "The firmware update failed(%d)\n", error); >> count = error; >> >
12.08.2020 13:11, Wang, Jiada пишет: >> Have you decided to abandon the patch which allowed to specify firmware >> name in a device tree? >> > In v11 patch-set, there is a patch allows user to specify config file > name via device-tree and sysfs, I will drop device-tree part, since > Dmitry Torokhov commented he would like to change the behavior of > firmware loading during boot time, > but I will keep sysfs interface part, which is similar to this patch but > for configuration file. Okay! BTW, looking forward to the "I2C retrying" patch, which is needed for the mxt1386 controller. The Acer A500 device-tree has been merged into upstream, but some pieces are still missing in the kernel which are needed to get that device fully functional, the mxt1386 controller support is one of those pieces.
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c index a2189739e30f..024dee7a3571 100644 --- a/drivers/input/touchscreen/atmel_mxt_ts.c +++ b/drivers/input/touchscreen/atmel_mxt_ts.c @@ -30,8 +30,6 @@ #include <media/videobuf2-v4l2.h> #include <media/videobuf2-vmalloc.h> -/* Firmware files */ -#define MXT_FW_NAME "maxtouch.fw" #define MXT_CFG_NAME "maxtouch.cfg" #define MXT_CFG_MAGIC "OBP_RAW V1" @@ -308,6 +306,7 @@ struct mxt_data { struct t7_config t7_cfg; struct mxt_dbg dbg; struct gpio_desc *reset_gpio; + char *fw_name; /* Cached parameters from object table */ u16 T5_address; @@ -2766,7 +2765,7 @@ static int mxt_check_firmware_format(struct device *dev, return -EINVAL; } -static int mxt_load_fw(struct device *dev, const char *fn) +static int mxt_load_fw(struct device *dev) { struct mxt_data *data = dev_get_drvdata(dev); const struct firmware *fw = NULL; @@ -2776,9 +2775,9 @@ static int mxt_load_fw(struct device *dev, const char *fn) unsigned int frame = 0; int ret; - ret = request_firmware(&fw, fn, dev); + ret = request_firmware(&fw, data->fw_name, dev); if (ret) { - dev_err(dev, "Unable to open firmware %s\n", fn); + dev_err(dev, "Unable to open firmware %s\n", data->fw_name); return ret; } @@ -2887,6 +2886,33 @@ static int mxt_load_fw(struct device *dev, const char *fn) return ret; } +static int mxt_update_file_name(struct device *dev, char **file_name, + const char *buf, size_t count) +{ + char *file_name_tmp; + + /* Simple sanity check */ + if (count > 64) { + dev_warn(dev, "File name too long\n"); + return -EINVAL; + } + + file_name_tmp = krealloc(*file_name, count + 1, GFP_KERNEL); + if (!file_name_tmp) + return -ENOMEM; + + *file_name = file_name_tmp; + memcpy(*file_name, buf, count); + + /* Echo into the sysfs entry may append newline at the end of buf */ + if (buf[count - 1] == '\n') + (*file_name)[count - 1] = '\0'; + else + (*file_name)[count] = '\0'; + + return 0; +} + static ssize_t mxt_update_fw_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) @@ -2894,7 +2920,11 @@ static ssize_t mxt_update_fw_store(struct device *dev, struct mxt_data *data = dev_get_drvdata(dev); int error; - error = mxt_load_fw(dev, MXT_FW_NAME); + error = mxt_update_file_name(dev, &data->fw_name, buf, count); + if (error) + return error; + + error = mxt_load_fw(dev); if (error) { dev_err(dev, "The firmware update failed(%d)\n", error); count = error;