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
