On Thu, 03 May 2018, Javier Arteaga wrote:
> Hi Lee,
>
> On Thu, May 03, 2018 at 09:32:04AM +0100, Lee Jones wrote:
> > > That makes sense. Though then I have only two other choices:
> > >
> > > a) add the mfd driver + at least one child _in the same commit_,
> > > b) add the child drivers before the mfd parent within the series.
> > >
> > > (a) feels wrong. And if I do (b) the child drivers won't be functional
> > > either until the parent (that implements the regmap) is introduced.
> > >
> > > How to proceed?
> >
> > a) is the correct thing to do.
> >
> > Or, why don't you add them all?
>
> Ah, if that's OK, then I'll squash the series into one patch.
Only the MFD parts. Any patches which touch other subsystems must be
kept separate unless there are *build time* dependencies.
> > > OK, so if I'm following you here, replace this:
> > >
> > > static const struct acpi_device_id upboard_acpi_match[] = {
> > > { "AANT0F01", (kernel_ulong_t)&upboard_up2_data },
> > > { }
> > > };
> > > MODULE_DEVICE_TABLE(acpi, upboard_acpi_match);
> > >
> > > with this:
> > >
> > > static const struct acpi_device_id upboard_acpi_match[] = {
> > > { "AANT0F01" },
> > > { }
> > > };
> > > MODULE_DEVICE_TABLE(acpi, upboard_acpi_match);
> > >
> > > and then on probe, choose the right regmap config and cells for that
> > > device inside a switch() on the ACPI ID.
> >
> > That's how drivers currently do it, yes.
> >
> > > Did I get that right? If so - what is the benefit?
> >
> > Cleanliness. If we allow the mixing of different platform
> > initialisation methods (i.e. passing pointers from one into the other
> > for 'matching'), things could get very messy, very quickly.
> >
> > It's best to keep them completely separate.
>
> OK then. Will do for v2.
>
> > > I see your point. I was going by the reasoning that really_probe()
> > > already avoids printing log warnings on -ENODEV/-ENXIO (yes, literally
> > > an error code, but meaning "driver rejected a device match" AFAIK).
> > > Following that logic, it should be desirable to be just as quiet from
> > > our driver too.
> >
> > Silently failing is a valid use-case. It would depend on what the
> > failure is. In the use-case to which you refer a mis-match is not
> > 'really' an error, they basically say "this is not the correct driver,
> > continue to look for the right one". The mis-matches will also be
> > many and the output of the log, useless. However in your case, an
> > -ENODEV here would been that someone is trying to boot the kernel on a
> > board which should probably be supported, but support is missing.
>
> Hmm. Following from your comment:
>
> - the first check ("is FPGA firmware explicitly marked as custom?") is a
> valid use of dev_dbg() ; return -ENODEV, as this driver surely isn't
> meant to handle arbitrary firmware, but
No, it's an error.
"Custom firmware not supported" it's something your user will always
want to know. Why are you hiding this from a user?
> - the next check ("is FPGA firmware an official version that we don't
> support yet?") should be promoted to dev_warn, as it falls under
> "should probably be supported, but support is missing".
Please see my laymans description of what 'warn' means. Warnings are
issued if execution can continue. If a device is unsupported, how can
it continue.
The rule of thumb i.e. 99.9% of the time, is, if you cannot continue
and you are returning an error code, either fail silently (unusual
corner-case) or print an error.
I don't understand why you are hiding these messages from your users?
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
