[3/3] media: si2157: rework the firmware load logic

Loading a firmware file should not be mandatory, as devices
could work with an eeprom firmware, if available.

Yet, using the eeprom firmware could lead into unpredictable
results, so the best is to warn about that.

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

To avoid mailbombing on a large number of people, only mailing lists were C/C on the cover.
See [PATCH 0/3] at: https://lore.kernel.org/all/cover.1638958050.git.mchehab+huawei@kernel.org/

 drivers/media/tuners/si2157.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

> Loading a firmware file should not be mandatory, as devices
> could work with an eeprom firmware, if available.
>
> Yet, using the eeprom firmware could lead into unpredictable
> results, so the best is to warn about that.

As there is no proof of an EEPROM being involved in any of
that, but strong evidence that all these devices actually have
an embedded firmware ROM, I propose changing that to "ROM"
instead.

> + bool warn_firmware_not_loaded = false;
> unsigned int chip_id, xtal_trim;
> - unsigned int fw_required;
> + bool fw_required = true;

To me, this is getting too ugly. All these per-model special
flags set somewhere in the code.

I propose removing BOTH these flags. Review of the SiLabs code
revealed:

1. ALL of the tuner models this driver supports have a firmware
   patch from SiLabs available.

2. NONE of them seems to require it. At least all the SiLabs
   drivers allow disabling the download.

So my proposal is:

1. Add firmware download support to all tuner models (this
   means adding some new firmware file names)

2. When a firmware file is not available, log an info (not
   warning) message and proceed.

None of the above boolean flags are needed then. The
dont_load_firmware flag from the config should of course be
kept as it is.

> + dev_warn(&client->dev, "firmware file '%s' not found. Using firmware from eeprom.\n",

Aside from using dev_info instead and changing the text to
"ROM firmware will be used.", this would also be a duplicate
message, as firmware_request() also logs a message when a
requested firmware file is not found.

So I propose also changing the firmware_request() call to
request_firmware_nowarn() instead to suppress the message
from the firmware loader.

Best Regards,
-Robert Schlabbach

Em Wed, 8 Dec 2021 23:37:33 +0100
Robert Schlabbach <robert_s@gmx.net> escreveu:

> > Loading a firmware file should not be mandatory, as devices
> > could work with an eeprom firmware, if available.
> >
> > Yet, using the eeprom firmware could lead into unpredictable
> > results, so the best is to warn about that.
> 
> As there is no proof of an EEPROM being involved in any of
> that, but strong evidence that all these devices actually have
> an embedded firmware ROM, I propose changing that to "ROM"
> instead.

Agreed. Will do such changes on patch 4, to be added to this series.

> > + bool warn_firmware_not_loaded = false;
> > unsigned int chip_id, xtal_trim;
> > - unsigned int fw_required;
> > + bool fw_required = true;
> 
> To me, this is getting too ugly. All these per-model special
> flags set somewhere in the code.
> 
> I propose removing BOTH these flags. Review of the SiLabs code
> revealed:
> 
> 1. ALL of the tuner models this driver supports have a firmware
>    patch from SiLabs available.

OK.

> 2. NONE of them seems to require it. At least all the SiLabs
>    drivers allow disabling the download.

Not true. if you check the code for si2148, it doesn't have
an option to skip firmware load.

The same is also true for other currently unsupported models.

> So my proposal is:
> 
> 1. Add firmware download support to all tuner models (this
>    means adding some new firmware file names)

Ok.

> 2. When a firmware file is not available, log an info (not
>    warning) message and proceed.

I guess this shouldn't be allowed for si2148 devices.

> None of the above boolean flags are needed then. The
> dont_load_firmware flag from the config should of course be
> kept as it is.
> 
> > + dev_warn(&client->dev, "firmware file '%s' not found. Using firmware from eeprom.\n",
> 
> Aside from using dev_info instead and changing the text to
> "ROM firmware will be used.", this would also be a duplicate
> message, as firmware_request() also logs a message when a
> requested firmware file is not found.
> 
> So I propose also changing the firmware_request() call to
> request_firmware_nowarn() instead to suppress the message
> from the firmware loader.

I can't see a request_firmware_nowarn() function, but year, the
printed messages can be simplified.

Thanks,
Mauro

> Not true. if you check the code for si2148, it doesn't have
> an option to skip firmware load.

You're right. I thought I had checked all code, but I must have
missed that one.

Or I was distracted by the fact that for Si2148 with romid 0x33,
a "dummy patch" is used, which according to the code comment
skips the firmware download and boots from NVM only. So I suppose
that version does not actually need the firmware...?!?

> I can't see a request_firmware_nowarn() function

Sorry, it's:

EXPORT_SYMBOL_GPL(firmware_request_nowarn);

They swapped the words around vs. the original function, for
whatever reason. Anyway, please use "firmware_request_nowarn()"
which does not log any message when the file is not found, so
that only the message logged from the si2157 shows up in the
kernel log.

Best Regards,
-Robert Schlabbach

Em Thu, 9 Dec 2021 20:37:29 +0100
Robert Schlabbach <robert_s@gmx.net> escreveu:

> > Not true. if you check the code for si2148, it doesn't have
> > an option to skip firmware load.  
> 
> You're right. I thought I had checked all code, but I must have
> missed that one.
> 
> Or I was distracted by the fact that for Si2148 with romid 0x33,
> a "dummy patch" is used, which according to the code comment
> skips the firmware download and boots from NVM only. So I suppose
> that version does not actually need the firmware...?!?

That's a good question. It sounds funny to have a "dummy patch"
loaded that would "skip firmware download", as the same would happen
without a firmware patch :-)

Hard to know for sure, but maybe the comment there was just outdated.
E. g. on a previous release it would have the code below such comment
also commented, but a new patch was then added, but someone forgot
to remove the comments.

> > I can't see a request_firmware_nowarn() function  
> 
> Sorry, it's:
> 
> EXPORT_SYMBOL_GPL(firmware_request_nowarn);

Ah ;-)

> They swapped the words around vs. the original function, for
> whatever reason. Anyway, please use "firmware_request_nowarn()"
> which does not log any message when the file is not found, so
> that only the message logged from the si2157 shows up in the
> kernel log.

Yeah, makes sense, especially since we'll be trying to load two
firmware files, at least for some of the tuners.

Thanks,
Mauro