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

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

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