Em Thu, 9 Dec 2021 09:17:17 +0100
Mauro Carvalho Chehab <mchehab+huawei@xxxxxxxxxx> escreveu:
> Em Thu, 9 Dec 2021 00:01:55 +0100
> Robert Schlabbach <robert_s@xxxxxxx> 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
