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.
> > 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
- 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".
I'll do that. Thanks for your analysis!
> > I won't insist any further though - if you're unconvinced these will
> > become dev_err's :)
>
> I'm convinced.
:)
> > *Scratches head*
> >
> > But your snippet just fixes testing "ret" twice? That's what I meant the
> > compiler could be catching on already.
>
> But the tests for comparison are different?
>
> ret != NULL
> ret != -EPROBE_DEFER
>
> How could the compiler conduct 1 test for different comparisons?
I mean, my old code was:
if (ret && ret != -EPROBE_DEFER)
dev_err(&pdev->dev, "failed to init controller GPIOs: %d", ret);
if (ret)
return ret;
Yours is:
if (ret) {
if (ret != -EPROBE_DEFER)
dev_err(&pdev->dev, "failed to init controller GPIOs: %d", ret);
return ret;
}
Yes we have to make two checks, but yours reads better and does not
attempt to test "ret" twice (*that* bit is what I meant by "assumed the
compiler would sort that out").
We're in agreement here anyway. Sorry that this trivial thing dragged so
long, I wasn't too clear before.
