Em Wed, 8 Dec 2021 17:40:56 +0100
Robert Schlabbach <robert_s@xxxxxxx> 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
