Re: [RFC PATCH 1/3] mfd: upboard: Add UP2 platform controller driver

[Lee Jones <lee.jones@xxxxxxxxxx>]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html