linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Input: ili210x - Make ili251x_firmware_to_buffer more generic
@ 2021-10-17 17:24 Marek Vasut
  2021-10-18  4:57 ` Dmitry Torokhov
  0 siblings, 1 reply; 3+ messages in thread
From: Marek Vasut @ 2021-10-17 17:24 UTC (permalink / raw)
  To: linux-input; +Cc: Marek Vasut, Dmitry Torokhov, Joe Hung, Luca Hsu

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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] Input: ili210x - Make ili251x_firmware_to_buffer more generic
  2021-10-17 17:24 [PATCH] Input: ili210x - Make ili251x_firmware_to_buffer more generic Marek Vasut
@ 2021-10-18  4:57 ` Dmitry Torokhov
  2021-10-18 12:00   ` Marek Vasut
  0 siblings, 1 reply; 3+ messages in thread
From: Dmitry Torokhov @ 2021-10-18  4:57 UTC (permalink / raw)
  To: Marek Vasut; +Cc: linux-input, Joe Hung, Luca Hsu

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] Input: ili210x - Make ili251x_firmware_to_buffer more generic
  2021-10-18  4:57 ` Dmitry Torokhov
@ 2021-10-18 12:00   ` Marek Vasut
  0 siblings, 0 replies; 3+ messages in thread
From: Marek Vasut @ 2021-10-18 12:00 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: linux-input, Joe Hung, Luca Hsu

[...]

>> 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;
>>   

[...]

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-10-18 12:00 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-17 17:24 [PATCH] Input: ili210x - Make ili251x_firmware_to_buffer more generic Marek Vasut
2021-10-18  4:57 ` Dmitry Torokhov
2021-10-18 12:00   ` Marek Vasut

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).