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

[Javier Arteaga <javier@xxxxxxxxxx>]

On Fri, May 04, 2018 at 09:09:18AM +0100, Lee Jones wrote:
> 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.

Got it.

> > > > 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?

*Theoretically* another driver (e.g. out-of-tree support for that custom
firmware) may be able to handle that firmware just fine, at which point
an error line from this driver in dmesg is just misleading spam.

But while typing it out, I can see is all hypothetical and hand-wavy.
While having a re-flashed board that silently fails to bind would
certainly confuse users. Let's make it a dev_err() then.

> > - the next check ("is FPGA firmware an offical 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.

True, I should have said dev_err().

> I don't understand why you are hiding these messages from your users?

Not anymore. Thanks for bearing with me :)
--
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