diff mbox series

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

Message ID dc8d0be6a9756bf65371e2e1e0a8062df74f0e5f.1638958050.git.mchehab+huawei@kernel.org (mailing list archive)
State New, archived
Headers show
Series media: si2157: rework firmware load logic | expand

Commit Message

Mauro Carvalho Chehab Dec. 8, 2021, 10:13 a.m. UTC 
Split the firmware load code from si2157_init, in order to
help to add further changes at the way firmware is handled on
this device.

No functional changes.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---

To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover.
See [PATCH 0/3] at: https://lore.kernel.org/all/cover.1638958050.git.mchehab+huawei@kernel.org/

 drivers/media/tuners/si2157.c | 98 ++++++++++++++++++++---------------
 1 file changed, 56 insertions(+), 42 deletions(-)

Comments

Robert Schlabbach Dec. 8, 2021, 4:40 p.m. UTC | #1 
> @@ -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.

> -	/* 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;
}

Best Regards,
-Robert Schlabbach
Mauro Carvalho Chehab Dec. 8, 2021, 5:03 p.m. UTC | #2 
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
diff mbox series

Patch

diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
index 75ddf7ed1faf..481a5db7fb69 100644
--- a/drivers/media/tuners/si2157.c
+++ b/drivers/media/tuners/si2157.c
@@ -76,16 +76,63 @@  static int si2157_cmd_execute(struct i2c_client *client, struct si2157_cmd *cmd)
 	return ret;
 }
 
-static int si2157_init(struct dvb_frontend *fe)
+static int si2157_load_firmware(struct dvb_frontend *fe,
+				const char *fw_name)
 {
 	struct i2c_client *client = fe->tuner_priv;
-	struct si2157_dev *dev = i2c_get_clientdata(client);
-	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
-	int ret, len, remaining;
-	struct si2157_cmd cmd;
 	const struct firmware *fw;
-	const char *fw_name;
+	int ret, len, remaining;
+	struct si2157_cmd cmd;
+
+	/* request the firmware, this will block and timeout */
+	ret = request_firmware(&fw, fw_name, &client->dev);
+	if (ret)
+		return ret;
+
+	/* firmware should be n chunks of 17 bytes */
+	if (fw->size % 17 != 0) {
+		dev_err(&client->dev, "firmware file '%s' is invalid\n",
+			fw_name);
+		ret = -EINVAL;
+		goto err_release_firmware;
+	}
+
+	dev_info(&client->dev, "downloading firmware from file '%s'\n",
+		 fw_name);
+
+	for (remaining = fw->size; remaining > 0; remaining -= 17) {
+		len = fw->data[fw->size - remaining];
+		if (len > SI2157_ARGLEN) {
+			dev_err(&client->dev, "Bad firmware length\n");
+			ret = -EINVAL;
+			goto err_release_firmware;
+		}
+		memcpy(cmd.args, &fw->data[(fw->size - remaining) + 1], len);
+		cmd.wlen = len;
+		cmd.rlen = 1;
+		ret = si2157_cmd_execute(client, &cmd);
+		if (ret) {
+			dev_err(&client->dev, "firmware download failed %d\n",
+					ret);
+			goto err_release_firmware;
+		}
+	}
+
+err_release_firmware:
+	release_firmware(fw);
+
+	return ret;
+}
+
+static int si2157_init(struct dvb_frontend *fe)
+{
+	struct dtv_frontend_properties *c = &fe->dtv_property_cache;
+	struct i2c_client *client = fe->tuner_priv;
+	struct si2157_dev *dev = i2c_get_clientdata(client);
 	unsigned int chip_id, xtal_trim;
+	struct si2157_cmd cmd;
+	const char *fw_name;
+	int ret;
 
 	dev_dbg(&client->dev, "\n");
 
@@ -181,45 +228,13 @@  static int si2157_init(struct dvb_frontend *fe)
 	if (fw_name == NULL)
 		goto skip_fw_download;
 
-	/* 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",
-				fw_name);
-		goto err;
-	}
-
-	/* firmware should be n chunks of 17 bytes */
-	if (fw->size % 17 != 0) {
-		dev_err(&client->dev, "firmware file '%s' is invalid\n",
-				fw_name);
-		ret = -EINVAL;
-		goto err_release_firmware;
-	}
-
-	dev_info(&client->dev, "downloading firmware from file '%s'\n",
 			fw_name);
-
-	for (remaining = fw->size; remaining > 0; remaining -= 17) {
-		len = fw->data[fw->size - remaining];
-		if (len > SI2157_ARGLEN) {
-			dev_err(&client->dev, "Bad firmware length\n");
-			ret = -EINVAL;
-			goto err_release_firmware;
-		}
-		memcpy(cmd.args, &fw->data[(fw->size - remaining) + 1], len);
-		cmd.wlen = len;
-		cmd.rlen = 1;
-		ret = si2157_cmd_execute(client, &cmd);
-		if (ret) {
-			dev_err(&client->dev, "firmware download failed %d\n",
-					ret);
-			goto err_release_firmware;
-		}
+		goto err;
 	}
 
-	release_firmware(fw);
-
 skip_fw_download:
 	/* reboot the tuner with new firmware? */
 	memcpy(cmd.args, "\x01\x01", 2);
@@ -270,8 +285,7 @@  static int si2157_init(struct dvb_frontend *fe)
 
 	dev->active = true;
 	return 0;
-err_release_firmware:
-	release_firmware(fw);
+
 err:
 	dev_dbg(&client->dev, "failed=%d\n", ret);
 	return ret;