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

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

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