Re: [PATCH 2/4] usb: typec: tipd: add function to request firmware

[Javier Carrasco]

On 12.12.23 15:15, Heikki Krogerus wrote:
> Hi,
> 
> On Fri, Dec 08, 2023 at 07:58:52PM +0100, Javier Carrasco wrote:
>> Hi Heikki,
>>
>> On 08.12.23 15:55, Heikki Krogerus wrote:
>>
>>>> +	ret = request_firmware(fw, firmware_name, tps->dev);
>>>> +	if (ret) {
>>>> +		dev_err(tps->dev, "failed to retrieve \"%s\"\n", firmware_name);
>>>> +		/* probe deferring in case the file system is not ready */
>>>> +		return (ret == -ENOENT) ? -EPROBE_DEFER : ret;
>>>
>>> It's more likely that the firmware really isn't available, and it will
>>> never be available in this case. I think there is only one place in
>>> kernel where failing request_firmware() can lead to deferred probe
>>> (drivers/tee/optee/smc_abi.c) and there the code can actually see the
>>> system state - that's actually the condition.
>>>
>>> So just return dev_err_probe() here:
>>>
>>> 	ret = request_firmware(fw, firmware_name, tps->dev);
>>> 	if (ret)
>>>                 return dev_err_probe(tps->dev, ret, "failed to retrieve \"%s\"", firmware_name);
>>>
>> Thank you for your feedback.
>>
>> This solution arose from a real use case: in the system I am using to
>> test the tps65987d, the filesystem is not ready when the probe function
>> is called. If I just return on -ENOENT, the device will never get the
>> update.
> 
> Just like all the other devices that require firmware. This driver is
> no different from the others, and it is also not the only one that
> needs the firmware only in special cases. Just make the firmware part
> of your ramdisk, or build the driver as a module.

I wonder why then there is no general solution that does not force the
driver to be built as a module. If there is none, the documentation
should mention that somehow (sorry if it does, I missed it). Actually a
solution like the one implemented in the driver you mentioned could be
used by any driver that can wait to be updated when the system is
running.

> Are these firmwares available linux-firmware (or are the going to be)?
> https://git.kernel.org/?p=linux/kernel/git/firmware/linux-firmware.git
> 
> thanks,
> 
The firmware (at least for the tps6598x) can be tailored with a TI
specific tool and it depends on the use case, so I suppose making it
public does not make much sense.

Best regards,
Javier Carrasco