[PATCH RFC] media: si2157: optionally load firmare for SI2146_A10 and

[PATCH RFC] media: si2157: optionally load firmare for SI2146_A10 and

[Mauro Carvalho Chehab]

Cc: Antti Palosaari <crope@iki.fi>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Robert Schlabbach <robert_s@gmx.net>
Cc: Olli Salonen <olli.salonen@iki.fi>
Cc: linux-kernel@vger.kernel.org
Cc: linux-media@vger.kernel.org

While the eeprom firmware files for such devices are know to work,
if there is a firmware file, use it instead, as a newer version
could have solved some tuning issues.

Compile-tested only.

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

At least on my eyes, it makes sense to also allow to optionally load A10
and A30 firmware files for SI2146_A10 and SI147_A30.

Yet, before applying this one, someone needs to report that those devices
will keep working with the loaded firmware files.

 drivers/media/tuners/si2157.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/tuners/si2157.c b/drivers/media/tuners/si2157.c
index 5f4ae8593864..f970bedfb179 100644
--- a/drivers/media/tuners/si2157.c
+++ b/drivers/media/tuners/si2157.c
@@ -205,19 +205,19 @@ static int si2157_init(struct dvb_frontend *fe)
 	case SI2148_A20:
 		fw_name = SI2158_A20_FIRMWARE;
 		break;
+	case SI2146_A10:
+		fw_required = false;
+		fallthrough;
 	case SI2141_A10:
 		fw_name = SI2141_A10_FIRMWARE;
 		break;
+	case SI2147_A30:
 	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],
-- 
2.33.1

Re: [PATCH RFC] media: si2157: optionally load firmare for SI2146_A10 and

[Robert Schlabbach]

> + case SI2146_A10:
> + fw_required = false;
> + fallthrough;
> case SI2141_A10:
> fw_name = SI2141_A10_FIRMWARE;
> break;

I don't think this form of firmware name aliasing is
a good idea. The SiLabs code has a dedicated source
file for the ROM patch for each tuner model, even if
some are binary identical.

And in this particular case, there are not even
binary identical firmware patches available for these
two tuners, so they definitely should NOT share the
same firmware filename.

So I propose having a clean 1:1 model <-> firmware
filename mapping. For si2157/si2177 and si2148/si2158
it's already too late, but we should not expand this
error even further.

More broadly, the SiLabs code actually matches the
applicable firmware patch to the rom_id returned by
the tuner. So if we wanted to do a real cleanup,
I would propose having a const struct table, e.g.

const struct {
    unsigned char part;
    unsigned char chiprev;
    unsigned char pmajor;
    unsigned char pminor;
    unsigned char rom_id;
    const char *  firmware_name
} supported_models[] = {
    { /*Si21*/41, 'A', 1, 0, 0x60, "dvb-tuner-si2141-a10-00.fw" },
    { /*Si21*/41, 'A', 1, 0, 0x61, "dvb-tuner-si2141-a10-01.fw" },
    { /*Si21*/57, 'A', 3, 0, 0x50, "dvb-tuner-si2157-a30-01.fw" },
(etc)
};

Best Regards,
-Robert Schlabbach

Re: [PATCH RFC] media: si2157: optionally load firmare for SI2146_A10 and

[Mauro Carvalho Chehab]

Em Thu, 9 Dec 2021 00:01:55 +0100
Robert Schlabbach <robert_s@gmx.net> escreveu:

> > + case SI2146_A10:
> > + fw_required = false;
> > + fallthrough;
> > case SI2141_A10:
> > fw_name = SI2141_A10_FIRMWARE;
> > break;  
> 
> I don't think this form of firmware name aliasing is
> a good idea. The SiLabs code has a dedicated source
> file for the ROM patch for each tuner model, even if
> some are binary identical.
> 
> And in this particular case, there are not even
> binary identical firmware patches available for these
> two tuners, so they definitely should NOT share the
> same firmware filename.

Ok.

> So I propose having a clean 1:1 model <-> firmware
> filename mapping.

Makes sense.

> For si2157/si2177 and si2148/si2158
> it's already too late, but we should not expand this
> error even further.

It is not too late. It is just a matter of adding a secondary
firmware name for those devices. if the primary (new) name
is not found, the driver would try the old name for those
firmwares. As this is the current 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"

We would need to have a different namespace for the newer firmware
file model. On a quick look at the opensourced drivers, those seem to
be the firmware structs over there:

	$ git grep 'firmware_struct.*=.\s*{' TER|perl -ne 'print "$1\n" if m/struct.*(Si[^\[]+)/'
	Si2124_FW_2_1b5
	Si2141_FW_0_Ab23
	Si2141_FW_1_1b12
	Si2144_FW_2_1b5
	Si2147_FW_3_1b3
	Si2148_FW_2_1b11
	Si2151_FW_0_Ab23
	Si2151_FW_1_1b11
	Si2157_FW_3_1b3
	Si2158B_FW_0_Ab15
	Si2158B_FW_4_1b3
	Si2177_FW_3_1b3
	Si2178B_FW_0_Ab15
	Si2178B_FW_4_1b3

If the idea is to be as close as possible to how the original firmware are named,
we could do, e. g. something like this:

	$ git grep 'firmware_struct.*=.\s*{' TER|perl -ne 'tr /A-Z/a-z/; print "dvb_driver_si$1_$2.fw\n" if m/struct.*si(\w+)_fw_([^\[]+)/'
	dvb_driver_si2124_2_1b5.fw
	dvb_driver_si2141_0_ab23.fw
	dvb_driver_si2141_1_1b12.fw
	dvb_driver_si2144_2_1b5.fw
	dvb_driver_si2147_3_1b3.fw
	dvb_driver_si2148_2_1b11.fw
	dvb_driver_si2151_0_ab23.fw
	dvb_driver_si2151_1_1b11.fw
	dvb_driver_si2157_3_1b3.fw
	dvb_driver_si2158b_0_ab15.fw
	dvb_driver_si2158b_4_1b3.fw
	dvb_driver_si2177_3_1b3.fw
	dvb_driver_si2178b_0_ab15.fw
	dvb_driver_si2178b_4_1b3.fw

On other words, for si2157, for instance, the driver would first try
to load:
	dvb_driver_si2157_3_1b3.fw
if it fails, it would try:
	dvb-tuner-si2157-a30-01.fw

This is backward compatible and should be flexible enough to support
different firmware for different tuners.

There are some issues, though. This would require to have all those
firmware files generated from the opensourced sources and stored somewhere,
assuming that the license would allow that.

Also, as the firmware files will probably be different, tests with
the different supported models will be required to be sure that the
code is compatible with them (as the API might have changed on
some of those).

> More broadly, the SiLabs code actually matches the
> applicable firmware patch to the rom_id returned by
> the tuner. So if we wanted to do a real cleanup,
> I would propose having a const struct table, e.g.
> 
> const struct {
>     unsigned char part;
>     unsigned char chiprev;
>     unsigned char pmajor;
>     unsigned char pminor;
>     unsigned char rom_id;
>     const char *  firmware_name
> } supported_models[] = {
>     { /*Si21*/41, 'A', 1, 0, 0x60, "dvb-tuner-si2141-a10-00.fw" },
>     { /*Si21*/41, 'A', 1, 0, 0x61, "dvb-tuner-si2141-a10-01.fw" },
>     { /*Si21*/57, 'A', 3, 0, 0x50, "dvb-tuner-si2157-a30-01.fw" },
> (etc)
> };

The struct itself sounds OK to me, with some adjustments:

1. Coding style nit:  firmware name should be, instead:

	const char *firmware_name

2. It would also need a:

	const char *firmware_alt_name

   to store the old firmware namespace, e. g.:
	SI2158_A20_FIRMWARE, SI2141_A10_FIRMWARE and SI2157_A30_FIRMWARE.

3. instead of placing a number (41, 57, ...) it should use defines
   or enums.

Thanks,
Mauro

Re: [PATCH RFC] media: si2157: optionally load firmare for SI2146_A10 and

[Mauro Carvalho Chehab]

Em Thu, 9 Dec 2021 09:17:17 +0100
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> Em Thu, 9 Dec 2021 00:01:55 +0100
> Robert Schlabbach <robert_s@gmx.net> escreveu:
> 
> > > + case SI2146_A10:
> > > + fw_required = false;
> > > + fallthrough;
> > > case SI2141_A10:
> > > fw_name = SI2141_A10_FIRMWARE;
> > > break;    
> > 
> > I don't think this form of firmware name aliasing is
> > a good idea. The SiLabs code has a dedicated source
> > file for the ROM patch for each tuner model, even if
> > some are binary identical.
> > 
> > And in this particular case, there are not even
> > binary identical firmware patches available for these
> > two tuners, so they definitely should NOT share the
> > same firmware filename.  
> 
> Ok.
> 
> > So I propose having a clean 1:1 model <-> firmware
> > filename mapping.  
> 
> Makes sense.
> 
> > For si2157/si2177 and si2148/si2158
> > it's already too late, but we should not expand this
> > error even further.  
> 
> It is not too late. It is just a matter of adding a secondary
> firmware name for those devices. if the primary (new) name
> is not found, the driver would try the old name for those
> firmwares. As this is the current 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"
> 
> We would need to have a different namespace for the newer firmware
> file model. On a quick look at the opensourced drivers, those seem to
> be the firmware structs over there:
> 
> 	$ git grep 'firmware_struct.*=.\s*{' TER|perl -ne 'print "$1\n" if m/struct.*(Si[^\[]+)/'
> 	Si2124_FW_2_1b5
> 	Si2141_FW_0_Ab23
> 	Si2141_FW_1_1b12
> 	Si2144_FW_2_1b5
> 	Si2147_FW_3_1b3
> 	Si2148_FW_2_1b11
> 	Si2151_FW_0_Ab23
> 	Si2151_FW_1_1b11
> 	Si2157_FW_3_1b3
> 	Si2158B_FW_0_Ab15
> 	Si2158B_FW_4_1b3
> 	Si2177_FW_3_1b3
> 	Si2178B_FW_0_Ab15
> 	Si2178B_FW_4_1b3
> 
> If the idea is to be as close as possible to how the original firmware are named,
> we could do, e. g. something like this:
> 
> 	$ git grep 'firmware_struct.*=.\s*{' TER|perl -ne 'tr /A-Z/a-z/; print "dvb_driver_si$1_$2.fw\n" if m/struct.*si(\w+)_fw_([^\[]+)/'
> 	dvb_driver_si2124_2_1b5.fw
> 	dvb_driver_si2141_0_ab23.fw
> 	dvb_driver_si2141_1_1b12.fw
> 	dvb_driver_si2144_2_1b5.fw
> 	dvb_driver_si2147_3_1b3.fw
> 	dvb_driver_si2148_2_1b11.fw
> 	dvb_driver_si2151_0_ab23.fw
> 	dvb_driver_si2151_1_1b11.fw
> 	dvb_driver_si2157_3_1b3.fw
> 	dvb_driver_si2158b_0_ab15.fw
> 	dvb_driver_si2158b_4_1b3.fw
> 	dvb_driver_si2177_3_1b3.fw
> 	dvb_driver_si2178b_0_ab15.fw
> 	dvb_driver_si2178b_4_1b3.fw
> 
> On other words, for si2157, for instance, the driver would first try
> to load:
> 	dvb_driver_si2157_3_1b3.fw
> if it fails, it would try:
> 	dvb-tuner-si2157-a30-01.fw
> 
> This is backward compatible and should be flexible enough to support
> different firmware for different tuners.
> 
> There are some issues, though. This would require to have all those
> firmware files generated from the opensourced sources and stored somewhere,
> assuming that the license would allow that.
> 
> Also, as the firmware files will probably be different, tests with
> the different supported models will be required to be sure that the
> code is compatible with them (as the API might have changed on
> some of those).
> 
> > More broadly, the SiLabs code actually matches the
> > applicable firmware patch to the rom_id returned by
> > the tuner. So if we wanted to do a real cleanup,
> > I would propose having a const struct table, e.g.
> > 
> > const struct {
> >     unsigned char part;
> >     unsigned char chiprev;
> >     unsigned char pmajor;
> >     unsigned char pminor;
> >     unsigned char rom_id;
> >     const char *  firmware_name
> > } supported_models[] = {
> >     { /*Si21*/41, 'A', 1, 0, 0x60, "dvb-tuner-si2141-a10-00.fw" },
> >     { /*Si21*/41, 'A', 1, 0, 0x61, "dvb-tuner-si2141-a10-01.fw" },
> >     { /*Si21*/57, 'A', 3, 0, 0x50, "dvb-tuner-si2157-a30-01.fw" },
> > (etc)
> > };  
> 
> The struct itself sounds OK to me, with some adjustments:
> 
> 1. Coding style nit:  firmware name should be, instead:
> 
> 	const char *firmware_name
> 
> 2. It would also need a:
> 
> 	const char *firmware_alt_name
> 
>    to store the old firmware namespace, e. g.:
> 	SI2158_A20_FIRMWARE, SI2141_A10_FIRMWARE and SI2157_A30_FIRMWARE.
> 
> 3. instead of placing a number (41, 57, ...) it should use defines
>    or enums.
> 
> Thanks,
> Mauro

I double-checked at the code: the A10/A20/A30 is actually not used,
just the ROM version.

So, I guess the enclosed data structs should be enough for the load
firmware code.

Regards,
Mauro

-

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 type;
	unsigned char rom_id;
	bool required;
	const char *fw_name, *fw_alt_name;
};

#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_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"

static const struct si2157_firmware fw[] = {
	{ SI2141, true,  0x60, SI2141_60_FIRMWARE, SI2141_A10_FIRMWARE },
	{ SI2141, true,  0x61, SI2141_61_FIRMWARE, SI2141_A10_FIRMWARE },
	{ SI2146, false, 0x11, SI2146_11_FIRMWARE, NULL },
	{ SI2147, false, 0x50, SI2147_50_FIRMWARE, NULL },
	{ SI2148, true,  0x33, SI2148_33_FIRMWARE, SI2158_A20_FIRMWARE },
	{ SI2157, false, 0x50, SI2157_50_FIRMWARE, SI2157_A30_FIRMWARE },
	{ SI2158, true,  0x50, SI2158_50_FIRMWARE, SI2158_A20_FIRMWARE },
	{ SI2158, true,  0x51, SI2158_51_FIRMWARE, SI2158_A20_FIRMWARE },
	{ SI2177, true,  0x50, SI2177_50_FIRMWARE, SI2157_A30_FIRMWARE },
};

Thanks,
Mauro