# Re: \[PATCH\] Input: ili210x - Make ili251x\_firmware\_to\_buffer more generic

- From: Marek Vasut \<marex@denx.de\>
- Date: 2021-10-18 12:00:21 
- List: linux-input
- Page: https://ratatoskr.run/linux-input/2021/10/8933500
- Thread: https://ratatoskr.run/linux-input/2021/10/8933496/t.md

```text
[...]

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

[...]
```