| Message ID | cd3a382dc39e72229a73149cb91e80cf69e2b07d.1638958947.git.mchehab+huawei@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] media: si2157: optionally load firmare for SI2146_A10 and | expand |
> + 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
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
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
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],
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(-)