On July 30, 2016 5:42:41 AM PDT, Arend van Spriel [off-list ref] wrote:

+ Luis (again) ;-)

On 29-07-16 08:13, Daniel Wagner wrote:

quoted

On 07/28/2016 09:01 PM, Bjorn Andersson wrote:

quoted

On Thu 28 Jul 11:33 PDT 2016, Dmitry Torokhov wrote:

quoted

On Thu, Jul 28, 2016 at 09:55:11AM +0200, Daniel Wagner wrote:

quoted

From: Daniel Wagner <redacted>

[..]

quoted

Do not quite like it... I'd rather asynchronous request give out a
firmware status pointer that could be used later on.

Excellent. Why not get rid of the callback function as well and have
fw_loading_wait() return result (0 = firmware available, < 0 = fail).
Just to confirm, you are proposing a new API function next to
request_firmware_nowait(), right?

Yes, that would be a new API call. Maybe we could replace old API with the new at some point.

quoted

quoted

quoted

    pcu->fw_st = request_firmware_async(IMS_PCU_FIRMWARE_NAME,
-                       pcu,
-                       ims_pcu_process_async_firmware);

   +			    pcu);

quoted

quoted

quoted

    if (IS_ERR(pcu->fw_st))
        return PTR_ERR(pcu->fw_st);

    ....

    err = fw_loading_wait(pcu->fw_st);

if (err)
	return err;

fw = fwstat_get_firmware(pcu->fw_st);

Or whatever consistent prefix it is going to be.

quoted

quoted

quoted

In the remoteproc case (patch 6) this would clean up the code,

rather

quoted

quoted

than replacing the completion API 1 to 1. I like it!

IIRC most drivers do it the same way. So request_firmware_async()

indeed

quoted

would be good thing to have. Let me try that.

While the idea behind this series is a good one I am wondering about
the
need for these drivers to use the asynchronous API. The historic reason
might be to avoid timeout caused by user-mode helper, but that may no
longer apply and these drivers could be better off using
request_firmware_direct().

Actually systems using this driver rely on usermode helper to provide necessary delay and load the firmware from storage once root partition is mounted. Converting to request_firmware_direct() would break them.


Thanks.

-- 
Dmitry