Re: [PATCH 1/3] media: si2157: move firmware load to a separate function

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

Em Wed, 8 Dec 2021 17:40:56 +0100
Robert Schlabbach <robert_s@gmx.net> escreveu:

> > @@ -181,45 +228,13 @@  static int si2157_init(struct dvb_frontend *fe)
> >  	if (fw_name == NULL)
> >  		goto skip_fw_download;  
> 
> You can invert the condition now and put the si2157_load_firmware() call
> inside the if () block, and do away with the goto.

I know, but this would also require to move the dont_load_firmware check,
which also does the goto.

I did that on a first version of the patch, but ended reverting,
as I can't be 100% certain devices with dont_load_firmware:

	if ((le16_to_cpu(d->udev->descriptor.idVendor) == USB_VID_DEXATEK &&
	     le16_to_cpu(d->udev->descriptor.idProduct) == 0x0100) ||
	    (le16_to_cpu(d->udev->descriptor.idVendor) == USB_VID_TERRATEC &&
	     le16_to_cpu(d->udev->descriptor.idProduct) == USB_PID_TERRATEC_CINERGY_TC2_STICK))
		si2157_config.dont_load_firmware = true;

Would keep work and report the hardware type/review. 

> > -	/* request the firmware, this will block and timeout */
> > -	ret = request_firmware(&fw, fw_name, &client->dev);
> > +	ret = si2157_load_firmware(fe, fw_name);
> >  	if (ret) {
> >  		dev_err(&client->dev, "firmware file '%s' not found\n",  
> 
> This will produce duplicate error messages, because the called function
> will already output some error messages. You should move this line to
> the extracted function as well, and reduce the code in the init function
> to:
> 
> if (fw_name != null) {
>         ret = si2157_load_firmware(fe, fw_name);
>         if (ret)
>                 goto err;
> }

True, but I guess patch 3 fixes it.

On patch 1, my goal were just to place everything on a single routine
without any real changes.  Patch 3 does the optimization/cleanup.

Thanks,
Mauro