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

[Javier Arteaga <javier@xxxxxxxxxx>]

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