[PATCH] Input: ili210x - Make ili251x_firmware_to_buffer more generic

[PATCH] Input: ili210x - Make ili251x_firmware_to_buffer more generic

[Marek Vasut]

Wrap request_ihex_firmware() and release_firmware() into function
ili251x_firmware_to_buffer(), since the ihex firmware is only used
within that function and it is not required outside of it.

This requires passing the firmware filename, but instead of adding yet
another parameter, add the firmware filename into struct ili2xxx_chip,
so other chips with different firmware filenames can also be updated
when their update support is in place. Rename the firmware parsing
function to ili210x_firmware_to_buffer as well.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Joe Hung <joe_hung@ilitek.com>
Cc: Luca Hsu <luca_hsu@ilitek.com>
---
 drivers/input/touchscreen/ili210x.c | 39 ++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
index 867c13d3cb17..a6b647100250 100644
--- a/drivers/input/touchscreen/ili210x.c
+++ b/drivers/input/touchscreen/ili210x.c
@@ -49,6 +49,7 @@ struct ili2xxx_chip {
 				 unsigned int *x, unsigned int *y,
 				 unsigned int *z);
 	bool (*continue_polling)(const u8 *data, bool touch);
+	char *firmware_filename;
 	unsigned int max_touches;
 	unsigned int resolution;
 	bool has_calibrate_reg;
@@ -288,6 +289,7 @@ static const struct ili2xxx_chip ili251x_chip = {
 	.get_touch_data		= ili251x_read_touch_data,
 	.parse_touch_data	= ili251x_touchdata_to_coords,
 	.continue_polling	= ili251x_check_continue_polling,
+	.firmware_filename	= ILI251X_FW_FILENAME,
 	.max_touches		= 10,
 	.has_calibrate_reg	= true,
 	.has_firmware_proto	= true,
@@ -557,15 +559,25 @@ static ssize_t ili210x_calibrate(struct device *dev,
 }
 static DEVICE_ATTR(calibrate, S_IWUSR, NULL, ili210x_calibrate);
 
-static int ili251x_firmware_to_buffer(const struct firmware *fw,
+static int ili210x_firmware_to_buffer(struct device *dev,
 				      u8 **buf, u16 *ac_end, u16 *df_end)
 {
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ili210x *priv = i2c_get_clientdata(client);
 	const struct ihex_binrec *rec;
 	u32 fw_addr, fw_last_addr = 0;
+	const struct firmware *fw;
 	u16 fw_len;
 	u8 *fw_buf;
 	int error;
 
+	error = request_ihex_firmware(&fw, priv->chip->firmware_filename, dev);
+	if (error) {
+		dev_err(dev, "Failed to request firmware %s, error=%d\n",
+			priv->chip->firmware_filename, error);
+		return error;
+	}
+
 	/*
 	 * The firmware ihex blob can never be bigger than 64 kiB, so make this
 	 * simple -- allocate a 64 kiB buffer, iterate over the ihex blob records
@@ -573,8 +585,10 @@ static int ili251x_firmware_to_buffer(const struct firmware *fw,
 	 * do all operations on this linear buffer.
 	 */
 	fw_buf = kzalloc(SZ_64K, GFP_KERNEL);
-	if (!fw_buf)
-		return -ENOMEM;
+	if (!fw_buf) {
+		error = -ENOMEM;
+		goto err_mem;
+	}
 
 	rec = (const struct ihex_binrec *)fw->data;
 	while (rec) {
@@ -599,10 +613,13 @@ static int ili251x_firmware_to_buffer(const struct firmware *fw,
 	/* DF end address is the last address in the firmware blob */
 	*df_end = fw_addr + fw_len;
 	*buf = fw_buf;
+	release_firmware(fw);
 	return 0;
 
 err_big:
 	kfree(fw_buf);
+err_mem:
+	release_firmware(fw);
 	return error;
 }
 
@@ -759,22 +776,13 @@ static ssize_t ili210x_firmware_update_store(struct device *dev,
 					     const char *buf, size_t count)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	const char *fwname = ILI251X_FW_FILENAME;
-	const struct firmware *fw;
+	struct ili210x *priv = i2c_get_clientdata(client);
 	u16 ac_end, df_end;
 	u8 *fwbuf;
 	int error;
 	int i;
 
-	error = request_ihex_firmware(&fw, fwname, dev);
-	if (error) {
-		dev_err(dev, "Failed to request firmware %s, error=%d\n",
-			fwname, error);
-		return error;
-	}
-
-	error = ili251x_firmware_to_buffer(fw, &fwbuf, &ac_end, &df_end);
-	release_firmware(fw);
+	error = ili210x_firmware_to_buffer(dev, &fwbuf, &ac_end, &df_end);
 	if (error)
 		return error;
 
@@ -787,7 +795,8 @@ static ssize_t ili210x_firmware_update_store(struct device *dev,
 	 */
 	disable_irq(client->irq);
 
-	dev_dbg(dev, "Firmware update started, firmware=%s\n", fwname);
+	dev_dbg(dev, "Firmware update started, firmware=%s\n",
+		priv->chip->firmware_filename);
 
 	ili251x_hardware_reset(dev);
 
-- 
2.33.0

Re: [PATCH] Input: ili210x - Make ili251x_firmware_to_buffer more generic

[Dmitry Torokhov]

On Sun, Oct 17, 2021 at 07:24:35PM +0200, Marek Vasut wrote:
> Wrap request_ihex_firmware() and release_firmware() into function
> ili251x_firmware_to_buffer(), since the ihex firmware is only used
> within that function and it is not required outside of it.
> 
> This requires passing the firmware filename, but instead of adding yet
> another parameter, add the firmware filename into struct ili2xxx_chip,
> so other chips with different firmware filenames can also be updated
> when their update support is in place. Rename the firmware parsing
> function to ili210x_firmware_to_buffer as well.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Joe Hung <joe_hung@ilitek.com>
> Cc: Luca Hsu <luca_hsu@ilitek.com>
> ---
>  drivers/input/touchscreen/ili210x.c | 39 ++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
> index 867c13d3cb17..a6b647100250 100644
> --- a/drivers/input/touchscreen/ili210x.c
> +++ b/drivers/input/touchscreen/ili210x.c
> @@ -49,6 +49,7 @@ struct ili2xxx_chip {
>  				 unsigned int *x, unsigned int *y,
>  				 unsigned int *z);
>  	bool (*continue_polling)(const u8 *data, bool touch);
> +	char *firmware_filename;
>  	unsigned int max_touches;
>  	unsigned int resolution;
>  	bool has_calibrate_reg;
> @@ -288,6 +289,7 @@ static const struct ili2xxx_chip ili251x_chip = {
>  	.get_touch_data		= ili251x_read_touch_data,
>  	.parse_touch_data	= ili251x_touchdata_to_coords,
>  	.continue_polling	= ili251x_check_continue_polling,
> +	.firmware_filename	= ILI251X_FW_FILENAME,
>  	.max_touches		= 10,
>  	.has_calibrate_reg	= true,
>  	.has_firmware_proto	= true,
> @@ -557,15 +559,25 @@ static ssize_t ili210x_calibrate(struct device *dev,
>  }
>  static DEVICE_ATTR(calibrate, S_IWUSR, NULL, ili210x_calibrate);
>  
> -static int ili251x_firmware_to_buffer(const struct firmware *fw,
> +static int ili210x_firmware_to_buffer(struct device *dev,
>  				      u8 **buf, u16 *ac_end, u16 *df_end)
>  {
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct ili210x *priv = i2c_get_clientdata(client);
>  	const struct ihex_binrec *rec;
>  	u32 fw_addr, fw_last_addr = 0;
> +	const struct firmware *fw;
>  	u16 fw_len;
>  	u8 *fw_buf;
>  	int error;
>  
> +	error = request_ihex_firmware(&fw, priv->chip->firmware_filename, dev);
> +	if (error) {
> +		dev_err(dev, "Failed to request firmware %s, error=%d\n",
> +			priv->chip->firmware_filename, error);
> +		return error;
> +	}
> +
>  	/*
>  	 * The firmware ihex blob can never be bigger than 64 kiB, so make this
>  	 * simple -- allocate a 64 kiB buffer, iterate over the ihex blob records
> @@ -573,8 +585,10 @@ static int ili251x_firmware_to_buffer(const struct firmware *fw,
>  	 * do all operations on this linear buffer.
>  	 */
>  	fw_buf = kzalloc(SZ_64K, GFP_KERNEL);
> -	if (!fw_buf)
> -		return -ENOMEM;
> +	if (!fw_buf) {
> +		error = -ENOMEM;
> +		goto err_mem;
> +	}
>  
>  	rec = (const struct ihex_binrec *)fw->data;
>  	while (rec) {
> @@ -599,10 +613,13 @@ static int ili251x_firmware_to_buffer(const struct firmware *fw,
>  	/* DF end address is the last address in the firmware blob */
>  	*df_end = fw_addr + fw_len;
>  	*buf = fw_buf;
> +	release_firmware(fw);
>  	return 0;
>  
>  err_big:
>  	kfree(fw_buf);
> +err_mem:
> +	release_firmware(fw);

So with that we are essentially back to the original version where we
have to release firmware in both branches. If we keep loading firmware
in this function, maybe we could do:

	...
out:
	if (retval)
		kfree(fw_buf);
	release_firmware(fw);
	return retval;

?

>  	return error;
>  }
>  
> @@ -759,22 +776,13 @@ static ssize_t ili210x_firmware_update_store(struct device *dev,
>  					     const char *buf, size_t count)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
> -	const char *fwname = ILI251X_FW_FILENAME;
> -	const struct firmware *fw;
> +	struct ili210x *priv = i2c_get_clientdata(client);
>  	u16 ac_end, df_end;
>  	u8 *fwbuf;
>  	int error;
>  	int i;
>  
> -	error = request_ihex_firmware(&fw, fwname, dev);
> -	if (error) {
> -		dev_err(dev, "Failed to request firmware %s, error=%d\n",
> -			fwname, error);
> -		return error;
> -	}
> -
> -	error = ili251x_firmware_to_buffer(fw, &fwbuf, &ac_end, &df_end);
> -	release_firmware(fw);

You do not like releasing firmware before checking if we could parse it
successfully?

> +	error = ili210x_firmware_to_buffer(dev, &fwbuf, &ac_end, &df_end);
>  	if (error)
>  		return error;
>  

Thanks.

-- 
Dmitry

Re: [PATCH] Input: ili210x - Make ili251x_firmware_to_buffer more generic

[Marek Vasut]

[...]

>> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c

[...]

>> @@ -599,10 +613,13 @@ static int ili251x_firmware_to_buffer(const struct firmware *fw,
>>   	/* DF end address is the last address in the firmware blob */
>>   	*df_end = fw_addr + fw_len;
>>   	*buf = fw_buf;
>> +	release_firmware(fw);
>>   	return 0;
>>   
>>   err_big:
>>   	kfree(fw_buf);
>> +err_mem:
>> +	release_firmware(fw);
> 
> So with that we are essentially back to the original version where we
> have to release firmware in both branches. If we keep loading firmware
> in this function, maybe we could do:

It seems to me the usual fail path with undoing what the function did is 
more obvious, but I can send a V2 with this below.

> 	...
> out:
> 	if (retval)
> 		kfree(fw_buf);
> 	release_firmware(fw);
> 	return retval;
> 
> ?
> 
>>   	return error;
>>   }
>>   
>> @@ -759,22 +776,13 @@ static ssize_t ili210x_firmware_update_store(struct device *dev,
>>   					     const char *buf, size_t count)
>>   {
>>   	struct i2c_client *client = to_i2c_client(dev);
>> -	const char *fwname = ILI251X_FW_FILENAME;
>> -	const struct firmware *fw;
>> +	struct ili210x *priv = i2c_get_clientdata(client);
>>   	u16 ac_end, df_end;
>>   	u8 *fwbuf;
>>   	int error;
>>   	int i;
>>   
>> -	error = request_ihex_firmware(&fw, fwname, dev);
>> -	if (error) {
>> -		dev_err(dev, "Failed to request firmware %s, error=%d\n",
>> -			fwname, error);
>> -		return error;
>> -	}
>> -
>> -	error = ili251x_firmware_to_buffer(fw, &fwbuf, &ac_end, &df_end);
>> -	release_firmware(fw);
> 
> You do not like releasing firmware before checking if we could parse it
> successfully?

This patch doesn't change that behavior, it only wraps the 
request/release of firmware into the ili210x_firmware_to_buffer() 
function, since that ihex firmware is not used anywhere else.

>> +	error = ili210x_firmware_to_buffer(dev, &fwbuf, &ac_end, &df_end);
>>   	if (error)
>>   		return error;
>>   

[...]