31.07.2020 10:57, Jiada Wang пишет:
> From: Nick Dyer <[email protected]>
>
> 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 <[email protected]>
> Acked-by: Benson Leung <[email protected]>
> Acked-by: Yufeng Shen <[email protected]>
> (cherry picked from ndyer/linux/for-upstream commit
> 76ebb7cee971cb42dfb0a3a9224403b8b09abcf1)
> [gdavis: Forward port and fix conflicts.]
> Signed-off-by: George G. Davis <[email protected]>
> Signed-off-by: Jiada Wang <[email protected]>
> ---
> 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;
>
