[PATCH v2 0/4] media: si2157: rework firmware load logic

[PATCH v2 0/4] media: si2157: rework firmware load logic

[Mauro Carvalho Chehab]

As pointed bt Robert, if there's already a firmware for si2157 at
/lib/firmware, it is preferred to use it, instead of the default firmware.

Yet, when this is missed, the driver may still work. That's the case,
for instance, of the devices explicitly flagged with dont_load_firmware
(currently, Terratec Cinergy TC2).

So, rework the firmware logic in a way that it will emit a warning for
devices that are using the eeprom firmware and may not work well
with it.

Compile-tested only.

Robert,

Could you please test it on your devices?

--

v2:
 - added a patch to allow supporting different firmwares depending on
   the device's reported ROM ID.
   
Mauro Carvalho Chehab (3):
  media: si2157: move firmware load to a separate function
  media: si2157: rework the firmware load logic
  media: si2157: use a different namespace for firmware

Robert Schlabbach (1):
  media: si2157: Add optional firmware download

 drivers/media/tuners/si2157.c      | 248 +++++++++++++++++------------
 drivers/media/tuners/si2157_priv.h |  14 ++
 2 files changed, 163 insertions(+), 99 deletions(-)

-- 
2.33.1

[PATCH v2 1/4] media: si2157: move firmware load to a separate function

[Mauro Carvalho Chehab]

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>
---

See [PATCH v2 0/4] at: https://lore.kernel.org/all/cover.1639049865.git.mchehab+huawei@kernel.org/

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

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;
-- 
2.33.1

[PATCH v2 2/4] media: si2157: Add optional firmware download

[Mauro Carvalho Chehab]

From: Robert Schlabbach <robert_s@gmx.net>

The Si2157 (A30) is functional with the ROM firmware 3.0.5, but can also
be patched at runtime, e.g. to firmware 3.1.3. However, although a
firmware filename for its firmware patch exists, that has only been used
for the Si2177 (A30) so far (which indeed takes the binary identical
firmware patch).

Add support for downloading firmware patches into the Si2157 (A30), but
make it optional, so that initialization can succeed with and without a
firmware patch being available. Keep the use of request_firmware() for
this purpose rather than firmware_request_nowarn(), so that the warning
in the log makes users aware that it is possible to provide a firmware
for this tuner.

The firmware patch is probably also optional for other (if not all)
tuners supported by the driver, but since I do not have the others
available to test, they are kept mandatory for now to avoid regressions.

Signed-off-by: Robert Schlabbach <robert_s@gmx.net>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---

See [PATCH v2 0/4] at: https://lore.kernel.org/all/cover.1639049865.git.mchehab+huawei@kernel.org/

 drivers/media/tuners/si2157.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
index 481a5db7fb69..ed28672c060d 100644
--- a/drivers/media/tuners/si2157.c
+++ b/drivers/media/tuners/si2157.c
@@ -130,6 +130,7 @@ static int si2157_init(struct dvb_frontend *fe)
 	struct i2c_client *client = fe->tuner_priv;
 	struct si2157_dev *dev = i2c_get_clientdata(client);
 	unsigned int chip_id, xtal_trim;
+	unsigned int fw_required;
 	struct si2157_cmd cmd;
 	const char *fw_name;
 	int ret;
@@ -198,6 +199,10 @@ static int si2157_init(struct dvb_frontend *fe)
 	#define SI2146_A10 ('A' << 24 | 46 << 16 | '1' << 8 | '0' << 0)
 	#define SI2141_A10 ('A' << 24 | 41 << 16 | '1' << 8 | '0' << 0)
 
+	/* assume firmware is required, unless verified not to be */
+	/* only the SI2157_A30 has been verified not to yet */
+	fw_required = true;
+
 	switch (chip_id) {
 	case SI2158_A20:
 	case SI2148_A20:
@@ -206,10 +211,13 @@ static int si2157_init(struct dvb_frontend *fe)
 	case SI2141_A10:
 		fw_name = SI2141_A10_FIRMWARE;
 		break;
-	case SI2177_A30:
-		fw_name = SI2157_A30_FIRMWARE;
-		break;
 	case SI2157_A30:
+		fw_name = SI2157_A30_FIRMWARE;
+		fw_required = false;
+		break;
+	case SI2177_A30:
+		fw_name = SI2157_A30_FIRMWARE;
+		break;
 	case SI2147_A30:
 	case SI2146_A10:
 		fw_name = NULL;
@@ -230,6 +238,9 @@ static int si2157_init(struct dvb_frontend *fe)
 
 	ret = si2157_load_firmware(fe, fw_name);
 	if (ret) {
+		if (!fw_required)
+			goto skip_fw_download;
+
 		dev_err(&client->dev, "firmware file '%s' not found\n",
 			fw_name);
 		goto err;
-- 
2.33.1

[PATCH v2 3/4] media: si2157: rework the firmware load logic

[Mauro Carvalho Chehab]

Loading a firmware file should not be mandatory, as devices
could work with an eeprom firmware, if available.

Yet, using the eeprom firmware could lead into unpredictable
results, so the best is to warn about that.

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

See [PATCH v2 0/4] at: https://lore.kernel.org/all/cover.1639049865.git.mchehab+huawei@kernel.org/

 drivers/media/tuners/si2157.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
index ed28672c060d..5f4ae8593864 100644
--- a/drivers/media/tuners/si2157.c
+++ b/drivers/media/tuners/si2157.c
@@ -129,8 +129,9 @@ 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);
+	bool warn_firmware_not_loaded = false;
 	unsigned int chip_id, xtal_trim;
-	unsigned int fw_required;
+	bool fw_required = true;
 	struct si2157_cmd cmd;
 	const char *fw_name;
 	int ret;
@@ -199,10 +200,6 @@ static int si2157_init(struct dvb_frontend *fe)
 	#define SI2146_A10 ('A' << 24 | 46 << 16 | '1' << 8 | '0' << 0)
 	#define SI2141_A10 ('A' << 24 | 41 << 16 | '1' << 8 | '0' << 0)
 
-	/* assume firmware is required, unless verified not to be */
-	/* only the SI2157_A30 has been verified not to yet */
-	fw_required = true;
-
 	switch (chip_id) {
 	case SI2158_A20:
 	case SI2148_A20:
@@ -212,9 +209,8 @@ static int si2157_init(struct dvb_frontend *fe)
 		fw_name = SI2141_A10_FIRMWARE;
 		break;
 	case SI2157_A30:
-		fw_name = SI2157_A30_FIRMWARE;
 		fw_required = false;
-		break;
+		fallthrough;
 	case SI2177_A30:
 		fw_name = SI2157_A30_FIRMWARE;
 		break;
@@ -237,12 +233,11 @@ static int si2157_init(struct dvb_frontend *fe)
 		goto skip_fw_download;
 
 	ret = si2157_load_firmware(fe, fw_name);
-	if (ret) {
-		if (!fw_required)
-			goto skip_fw_download;
-
-		dev_err(&client->dev, "firmware file '%s' not found\n",
-			fw_name);
+	if (fw_required && ret == -ENOENT)
+		warn_firmware_not_loaded = true;
+	else if (ret < 0) {
+		dev_err(&client->dev, "error %d when loading firmware file '%s'\n",
+			ret, fw_name);
 		goto err;
 	}
 
@@ -263,6 +258,11 @@ static int si2157_init(struct dvb_frontend *fe)
 	if (ret)
 		goto err;
 
+	if (warn_firmware_not_loaded) {
+		dev_warn(&client->dev, "firmware file '%s' not found. Using firmware from eeprom.\n",
+			 fw_name);
+		warn_firmware_not_loaded = false;
+	}
 	dev_info(&client->dev, "firmware version: %c.%c.%d\n",
 			cmd.args[6], cmd.args[7], cmd.args[8]);
 
@@ -298,6 +298,11 @@ static int si2157_init(struct dvb_frontend *fe)
 	return 0;
 
 err:
+	if (warn_firmware_not_loaded)
+		dev_err(&client->dev,
+			"firmware file '%s' not found. Can't continue without a firmware.\n",
+			fw_name);
+
 	dev_dbg(&client->dev, "failed=%d\n", ret);
 	return ret;
 }
-- 
2.33.1

[PATCH v2 4/4] media: si2157: use a different namespace for firmware

[Mauro Carvalho Chehab]

Each chip at the si21xx TER family seems to have a different firmware,
with seems to actually be a patch against the ROM code.

Rework the code in orde to use different firmware files depending
at the chip ID and rom ID.

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

See [PATCH v2 0/4] at: https://lore.kernel.org/all/cover.1639049865.git.mchehab+huawei@kernel.org/

 drivers/media/tuners/si2157.c      | 172 ++++++++++++++++-------------
 drivers/media/tuners/si2157_priv.h |  14 +++
 2 files changed, 110 insertions(+), 76 deletions(-)

diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
index 5f4ae8593864..f28eab17e82f 100644
--- a/drivers/media/tuners/si2157.c
+++ b/drivers/media/tuners/si2157.c
@@ -76,6 +76,36 @@ static int si2157_cmd_execute(struct i2c_client *client, struct si2157_cmd *cmd)
 	return ret;
 }
 
+enum si2157_chip_type {
+	SI2141 = 41,
+	SI2146 = 46,
+	SI2147 = 47,
+	SI2148 = 48,
+	SI2157 = 57,
+	SI2158 = 58,
+	SI2177 = 77,
+};
+
+struct si2157_firmware {
+	enum si2157_chip_type chip_id;
+	unsigned char rom_id;
+	bool required;
+	const char *fw_name, *fw_alt_name;
+};
+
+static const struct si2157_firmware si2157_fw[] = {
+	{ SI2141, false, 0x60, SI2141_60_FIRMWARE, SI2141_A10_FIRMWARE },
+	{ SI2141, false, 0x61, SI2141_61_FIRMWARE, SI2141_A10_FIRMWARE },
+	{ SI2146, false, 0x11, SI2146_11_FIRMWARE, NULL },
+	{ SI2147, false, 0x50, SI2147_50_FIRMWARE, NULL },
+	{ SI2148, true,  0x32, SI2148_32_FIRMWARE, SI2158_A20_FIRMWARE },
+	{ SI2148, true,  0x33, SI2148_33_FIRMWARE, SI2158_A20_FIRMWARE },
+	{ SI2157, false, 0x50, SI2157_50_FIRMWARE, SI2157_A30_FIRMWARE },
+	{ SI2158, false, 0x50, SI2158_50_FIRMWARE, SI2158_A20_FIRMWARE },
+	{ SI2158, false, 0x51, SI2158_51_FIRMWARE, SI2158_A20_FIRMWARE },
+	{ SI2177, false, 0x50, SI2177_50_FIRMWARE, SI2157_A30_FIRMWARE },
+};
+
 static int si2157_load_firmware(struct dvb_frontend *fe,
 				const char *fw_name)
 {
@@ -124,16 +154,63 @@ static int si2157_load_firmware(struct dvb_frontend *fe,
 	return ret;
 }
 
+static int si2157_find_and_load_firmware(struct dvb_frontend *fe)
+{
+	struct i2c_client *client = fe->tuner_priv;
+	unsigned char chip_id, rom_id;
+	struct si2157_cmd cmd;
+	int ret, i;
+
+	/* query chip revision */
+	memcpy(cmd.args, "\x02", 1);
+	cmd.wlen = 1;
+	cmd.rlen = 13;
+	ret = si2157_cmd_execute(client, &cmd);
+	if (ret)
+		return ret;
+
+	chip_id = cmd.args[2];
+	rom_id = cmd.args[12];
+
+	for (i = 0; i < ARRAY_SIZE(si2157_fw); i++) {
+		if (si2157_fw[i].chip_id != chip_id)
+			continue;
+		if (si2157_fw[i].rom_id == rom_id)
+			break;
+	}
+
+	if (i >= ARRAY_SIZE(si2157_fw)) {
+		dev_err(&client->dev,
+			"unknown chip version Si21%d-%c%c%c ROM 0x%02x\n",
+			chip_id, cmd.args[1], cmd.args[3], cmd.args[4], rom_id);
+		return -EINVAL;
+	}
+
+	dev_info(&client->dev,
+		 "found a 'Silicon Labs Si21%d-%c%c%c ROM 0x%02x'\n",
+		 chip_id, cmd.args[1], cmd.args[3], cmd.args[4], rom_id);
+
+	ret = si2157_load_firmware(fe, si2157_fw[i].fw_name);
+
+	/* Try alternate name, if any */
+	if (ret == -ENOENT && si2157_fw[i].fw_alt_name)
+		ret = si2157_load_firmware(fe, si2157_fw[i].fw_alt_name);
+
+	if (ret == -ENOENT && si2157_fw[i].required)
+	    dev_err(&client->dev, "Can't continue without a firmware.\n");
+	else if (ret < 0) {
+		dev_err(&client->dev, "error %d when loading firmware\n", ret);
+	}
+	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);
-	bool warn_firmware_not_loaded = false;
-	unsigned int chip_id, xtal_trim;
-	bool fw_required = true;
+	unsigned int xtal_trim;
 	struct si2157_cmd cmd;
-	const char *fw_name;
 	int ret;
 
 	dev_dbg(&client->dev, "\n");
@@ -176,72 +253,15 @@ static int si2157_init(struct dvb_frontend *fe)
 			goto err;
 	}
 
+	/* Try to load the firmware */
 	if (dev->dont_load_firmware) {
 		dev_info(&client->dev, "device is buggy, skipping firmware download\n");
-		goto skip_fw_download;
+	} else {
+		ret = si2157_find_and_load_firmware(fe);
+		if (ret < 0)
+			goto err;
 	}
 
-	/* query chip revision */
-	memcpy(cmd.args, "\x02", 1);
-	cmd.wlen = 1;
-	cmd.rlen = 13;
-	ret = si2157_cmd_execute(client, &cmd);
-	if (ret)
-		goto err;
-
-	chip_id = cmd.args[1] << 24 | cmd.args[2] << 16 | cmd.args[3] << 8 |
-			cmd.args[4] << 0;
-
-	#define SI2177_A30 ('A' << 24 | 77 << 16 | '3' << 8 | '0' << 0)
-	#define SI2158_A20 ('A' << 24 | 58 << 16 | '2' << 8 | '0' << 0)
-	#define SI2148_A20 ('A' << 24 | 48 << 16 | '2' << 8 | '0' << 0)
-	#define SI2157_A30 ('A' << 24 | 57 << 16 | '3' << 8 | '0' << 0)
-	#define SI2147_A30 ('A' << 24 | 47 << 16 | '3' << 8 | '0' << 0)
-	#define SI2146_A10 ('A' << 24 | 46 << 16 | '1' << 8 | '0' << 0)
-	#define SI2141_A10 ('A' << 24 | 41 << 16 | '1' << 8 | '0' << 0)
-
-	switch (chip_id) {
-	case SI2158_A20:
-	case SI2148_A20:
-		fw_name = SI2158_A20_FIRMWARE;
-		break;
-	case SI2141_A10:
-		fw_name = SI2141_A10_FIRMWARE;
-		break;
-	case SI2157_A30:
-		fw_required = false;
-		fallthrough;
-	case SI2177_A30:
-		fw_name = SI2157_A30_FIRMWARE;
-		break;
-	case SI2147_A30:
-	case SI2146_A10:
-		fw_name = NULL;
-		break;
-	default:
-		dev_err(&client->dev, "unknown chip version Si21%d-%c%c%c\n",
-				cmd.args[2], cmd.args[1],
-				cmd.args[3], cmd.args[4]);
-		ret = -EINVAL;
-		goto err;
-	}
-
-	dev_info(&client->dev, "found a 'Silicon Labs Si21%d-%c%c%c'\n",
-			cmd.args[2], cmd.args[1], cmd.args[3], cmd.args[4]);
-
-	if (fw_name == NULL)
-		goto skip_fw_download;
-
-	ret = si2157_load_firmware(fe, fw_name);
-	if (fw_required && ret == -ENOENT)
-		warn_firmware_not_loaded = true;
-	else if (ret < 0) {
-		dev_err(&client->dev, "error %d when loading firmware file '%s'\n",
-			ret, fw_name);
-		goto err;
-	}
-
-skip_fw_download:
 	/* reboot the tuner with new firmware? */
 	memcpy(cmd.args, "\x01\x01", 2);
 	cmd.wlen = 2;
@@ -258,11 +278,6 @@ static int si2157_init(struct dvb_frontend *fe)
 	if (ret)
 		goto err;
 
-	if (warn_firmware_not_loaded) {
-		dev_warn(&client->dev, "firmware file '%s' not found. Using firmware from eeprom.\n",
-			 fw_name);
-		warn_firmware_not_loaded = false;
-	}
 	dev_info(&client->dev, "firmware version: %c.%c.%d\n",
 			cmd.args[6], cmd.args[7], cmd.args[8]);
 
@@ -298,11 +313,6 @@ static int si2157_init(struct dvb_frontend *fe)
 	return 0;
 
 err:
-	if (warn_firmware_not_loaded)
-		dev_err(&client->dev,
-			"firmware file '%s' not found. Can't continue without a firmware.\n",
-			fw_name);
-
 	dev_dbg(&client->dev, "failed=%d\n", ret);
 	return ret;
 }
@@ -968,3 +978,13 @@ MODULE_LICENSE("GPL");
 MODULE_FIRMWARE(SI2158_A20_FIRMWARE);
 MODULE_FIRMWARE(SI2141_A10_FIRMWARE);
 MODULE_FIRMWARE(SI2157_A30_FIRMWARE);
+MODULE_FIRMWARE(SI2141_60_FIRMWARE);
+MODULE_FIRMWARE(SI2141_61_FIRMWARE);
+MODULE_FIRMWARE(SI2146_11_FIRMWARE);
+MODULE_FIRMWARE(SI2147_50_FIRMWARE);
+MODULE_FIRMWARE(SI2148_32_FIRMWARE);
+MODULE_FIRMWARE(SI2148_33_FIRMWARE);
+MODULE_FIRMWARE(SI2157_50_FIRMWARE);
+MODULE_FIRMWARE(SI2158_50_FIRMWARE);
+MODULE_FIRMWARE(SI2158_51_FIRMWARE);
+MODULE_FIRMWARE(SI2177_50_FIRMWARE);
diff --git a/drivers/media/tuners/si2157_priv.h b/drivers/media/tuners/si2157_priv.h
index ef4796098931..a99e04589b6a 100644
--- a/drivers/media/tuners/si2157_priv.h
+++ b/drivers/media/tuners/si2157_priv.h
@@ -54,7 +54,21 @@ struct si2157_cmd {
 	unsigned rlen;
 };
 
+/* Old firmware namespace */
 #define SI2158_A20_FIRMWARE "dvb-tuner-si2158-a20-01.fw"
 #define SI2141_A10_FIRMWARE "dvb-tuner-si2141-a10-01.fw"
 #define SI2157_A30_FIRMWARE "dvb-tuner-si2157-a30-01.fw"
+
+/* New firmware namespace */
+#define SI2141_60_FIRMWARE "dvb_driver_si2141_0_ab23.fw"
+#define SI2141_61_FIRMWARE "dvb_driver_si2141_1_1b12.fw"
+#define SI2146_11_FIRMWARE "dvb_driver_si2146_1_1b3.fw"
+#define SI2147_50_FIRMWARE "dvb_driver_si2147_3_1b3.fw"
+#define SI2148_32_FIRMWARE "dvb_driver_si2148_0_eb15.fw"
+#define SI2148_33_FIRMWARE "dvb_driver_si2148_2_1b11.fw"
+#define SI2157_50_FIRMWARE "dvb_driver_si2157_3_1b3.fw"
+#define SI2158_50_FIRMWARE "dvb_driver_si2178b_0_ab15.fw"
+#define SI2158_51_FIRMWARE "dvb_driver_si2158b_4_1b3.fw"
+#define SI2177_50_FIRMWARE "dvb_driver_si2177_3_1b3.fw"
+
 #endif
-- 
2.33.1

Re: [PATCH v2 4/4] media: si2157: use a different namespace for firmware

[Robert Schlabbach]

Just wondering: You keep stacking new commits on top, rather than fixing
issues found in the earlier commits. Do you plan to squash these into
a single commit in the end? Because I would not feel well submitting the
"bad"/untested in-between commits into the code...

> Rework the code in orde to use different firmware files depending
> at the chip ID and rom ID.

"order" (r missing) and "depending on" not "at".

> +enum si2157_chip_type {

SiLabs calls this "part", and I would stick to that name to avoid
confusion. We should not invent new names for things that the know
the original name of.

> +struct si2157_firmware {

My original idea was that this struct array would be the one-stop shop
where the driver would obtain ALL its information about the tuners it
supports from, i.e. it would be used to:

- provide a list of the supported parts, with all the constants
  needed for identification
- provide the firmware filenames
- provide further information, e.g. "quirks"

In fact, you already use it for all that, but do not reflect that in
the name. Maybe "struct si2157_tuner_info" and then name the list
"si2157_supported_tuners[]"?

As to the #defines for the part numbers, I actually see not much use
in that, if each define will only be used in exactly one place - the
table. Why not have the table be the "single source of truth",
containing _all_ identifiers in one place, rather than needlessly
scattering them over the source code?

> + enum si2157_chip_type chip_id;
> + unsigned char rom_id;
> + bool required;
> + const char *fw_name, *fw_alt_name;

Before this patch, the driver determined whether it supported the
tuner by comparing more/other fields from the PART_INFO response.
I suppose it is ok to expand the support, but the newly introduced
comparison of the rom_id could theoretically remove support for
tuners that were previously supported by this driver. Is this an
acceptable risk?

> + const char *fw_name, *fw_alt_name;

This might not suffice. Theoretically, there could be newer firmware
patches in the future which we would want to support. Would we then
throw out support for the older patches?

I would suggest, e.g.:

#define MAX_SUPPORTED_FIRMWARE_VERSIONS 4

const char *fw_name[MAX_SUPPORTED_FIRMWARE_VERSIONS];

and then sort the filenames from newest to oldest, and put the
"legacy" names at the bottom.

> +static int si2157_find_and_load_firmware(struct dvb_frontend *fe)

Hmm, I would prefer to cut the functionality in a different way:

- query the part information (it's not "chip revision" - Antti did
  not have the SiLabs source code available and had to guess what
  this was called) and determine whether this part is supported by
  the driver (and if we do not want to take the risk mentioned
  above, maybe even pick one when there is no matching rom_id,
  but the other values from the part information match?). I think
  this would be compact enough to remain in the init() function.

- have a download_and_start_firmware() function that is passed the
  matched struct si2157_tuner_info * and iterates over the contained
  firmware filenames, and if it finds an available one, downloads
  that into the tuner. Either way, the EXIT_BOOTLOADER command
  (commented in the code as "reboot the tuner with new firmware?",
  because, again, Antti could not know what the command was
  called) should be part of THIS function, and not be in the init()
  function. That is also the way it is cut in the SiLabs sources,
  and it makes sense to me.

Best Regards,
-Robert Schlabbach

Re: [PATCH v2 4/4] media: si2157: use a different namespace for firmware

[Mauro Carvalho Chehab]

Em Thu, 9 Dec 2021 22:30:03 +0100
Robert Schlabbach <Robert.Schlabbach@gmx.net> escreveu:

> Just wondering: You keep stacking new commits on top, rather than fixing
> issues found in the earlier commits. Do you plan to squash these into
> a single commit in the end? Because I would not feel well submitting the
> "bad"/untested in-between commits into the code...
> 
> > Rework the code in orde to use different firmware files depending
> > at the chip ID and rom ID.  
> 
> "order" (r missing) and "depending on" not "at".

ok.

> 
> > +enum si2157_chip_type {  
> 
> SiLabs calls this "part", and I would stick to that name to avoid
> confusion. We should not invent new names for things that the know
> the original name of.

ok.

> 
> > +struct si2157_firmware {  
> 
> My original idea was that this struct array would be the one-stop shop
> where the driver would obtain ALL its information about the tuners it
> supports from, i.e. it would be used to:
> 
> - provide a list of the supported parts, with all the constants
>   needed for identification
> - provide the firmware filenames
> - provide further information, e.g. "quirks"
> 
> In fact, you already use it for all that, but do not reflect that in
> the name. Maybe "struct si2157_tuner_info" and then name the list
> "si2157_supported_tuners[]"?

Works for me.

> As to the #defines for the part numbers, I actually see not much use
> in that, if each define will only be used in exactly one place - the
> table. Why not have the table be the "single source of truth",
> containing _all_ identifiers in one place, rather than needlessly
> scattering them over the source code?

Do you mean the enum (currently named as si2157_chip_type)?

If so, even if they're meant to be used just once, it still makes
sense to have them on an enum. See, Kernel developers hate to use
random meaningful numbers inside the code, as this obscures the
code. On other words, it is a lot more intuitive for anyone reviewing
the code to see:

	SI2141, SI2177, ...

than random numbers
	41, 77, ...

Also, assuming that this would be used just once is a mistake, as
the part enum could later be used to replace those defines:

	#define SI2157_CHIPTYPE_SI2157 0
	#define SI2157_CHIPTYPE_SI2146 1
	#define SI2157_CHIPTYPE_SI2141 2
	#define SI2157_CHIPTYPE_SI2177 3

> > + enum si2157_chip_type chip_id;
> > + unsigned char rom_id;
> > + bool required;
> > + const char *fw_name, *fw_alt_name;  
> 
> Before this patch, the driver determined whether it supported the
> tuner by comparing more/other fields from the PART_INFO response.
> I suppose it is ok to expand the support, but the newly introduced
> comparison of the rom_id could theoretically remove support for
> tuners that were previously supported by this driver. Is this an
> acceptable risk?

Good point. Before submitting the code, I had a logic that would
select the first ROM ID if it won't be able to match a rom. I
ended removing on the submitted version, but maybe I could do
something different. I'll address it at the next version.

> 
> > + const char *fw_name, *fw_alt_name;  
> 
> This might not suffice. Theoretically, there could be newer firmware
> patches in the future which we would want to support. Would we then
> throw out support for the older patches?
> 
> I would suggest, e.g.:
> 
> #define MAX_SUPPORTED_FIRMWARE_VERSIONS 4
> 
> const char *fw_name[MAX_SUPPORTED_FIRMWARE_VERSIONS];
> 
> and then sort the filenames from newest to oldest, and put the
> "legacy" names at the bottom.

Yeah, I'm not comfortable with that either, as it can be a maintenance
nightmare.

There's a better way to solve it: instead of placing the firmware
revision name, we could simply name the firmware files as:

	/* New firmware namespace */
	#define SI2141_60_FIRMWARE "dvb_driver_si2141_rom60.fw"
	#define SI2141_61_FIRMWARE "dvb_driver_si2141_rom61.fw"
	#define SI2146_11_FIRMWARE "dvb_driver_si2146_rom11.fw"
	#define SI2147_50_FIRMWARE "dvb_driver_si2147_rom50.fw"
	#define SI2148_32_FIRMWARE "dvb_driver_si2148_rom32.fw"
	#define SI2148_33_FIRMWARE "dvb_driver_si2148_rom33.fw"
	#define SI2157_50_FIRMWARE "dvb_driver_si2157_rom50.fw"
	#define SI2158_50_FIRMWARE "dvb_driver_si2178_rom50.fw"
	#define SI2158_51_FIRMWARE "dvb_driver_si2158_rom51.fw"
	#define SI2177_50_FIRMWARE "dvb_driver_si2177_rom50.fw"

This will still allow having one firmware per part/rom ID, and will
allow to use upgraded firmware files.

> > +static int si2157_find_and_load_firmware(struct dvb_frontend *fe)  
> 
> Hmm, I would prefer to cut the functionality in a different way:

I opted to split this way in order to simplify error handling logic.

With the current split:

- si2157_load_firmware() has the entire logic required for loading a
  firmware, and will de-allocate any data allocated by request_firmware;
- si2157_find_and_load_firmware() will contain all the logic needed to
  identify what firmware should be loaded, and will call 
  si2157_load_firmware() for both new and old firmware names.

> - query the part information (it's not "chip revision" - Antti did
>   not have the SiLabs source code available and had to guess what
>   this was called) and determine whether this part is supported by
>   the driver (and if we do not want to take the risk mentioned
>   above, maybe even pick one when there is no matching rom_id,
>   but the other values from the part information match?). I think
>   this would be compact enough to remain in the init() function.

Yes, but this would mean that the entire code would be under an if
block

	if (dev_dont_load_firmware) {
		dev_info(&client->dev, "device is buggy, skipping firmware download\n");
	} else {
		...
	}

Placing it on a separate function avoids the indentation, simplifying
the logic.

> - have a download_and_start_firmware() function that is passed the
>   matched struct si2157_tuner_info * and iterates over the contained
>   firmware filenames, and if it finds an available one, downloads
>   that into the tuner. Either way, the EXIT_BOOTLOADER command
>   (commented in the code as "reboot the tuner with new firmware?",
>   because, again, Antti could not know what the command was
>   called) should be part of THIS function, and not be in the init()
>   function. That is also the way it is cut in the SiLabs sources,
>   and it makes sense to me.

The "reboot the tuner with new firmware" logic could eventually be
moved to si2157_find_and_load_firmware(), but not sure if it is
worth, as calling it puts the device on a know condition. Moving it
to the load firmware function would mean that such code will never be
called if "dev->dont_load_firmware".

As moving it brings no real benefit and could cause regressions, I prefer
to keep it as-is, except if someone reports a bug due to that.

Thanks,
Mauro