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

[Lee Jones <lee.jones@xxxxxxxxxx>]

On Wed, 02 May 2018, Javier Arteaga wrote:
> On Tue, May 01, 2018 at 06:06:50PM +0100, Lee Jones wrote:
> > > Hmm... We do populate the struct mfd_cell array, although the way this
> > > patch series is structured, we leave it empty while we're only adding
> > > the MFD driver, and then fill it in later commits. I can instead
> > > introduce the cell array along with the first cell if you think that's
> > > more logical.
> > 
> > I do not accept drivers which do not provide a purpose, and without
> > child devices, this driver wouldn't actually do anything.
> 
> 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?

> > > That aside we *do* mix platform data and ACPI data but I'm not sure how
> > > I can avoid it:
> > > 
> > > - we have an ACPI ID for matching, and
> > > - board ACPI data has SoC GPIO references for the upboard-pinctrl,
> > > 
> > > but
> > > 
> > > - board ACPI data doesn't indicate number/color of LEDs, and
> > > - with this driver structure we have to pass a regmap to the child
> > >   drivers, and it looks like that's done via pdata anyway.
> > 
> > Right, so you define all that you know about the platform statically,
> > then use the ACPI provided ID to match with a switch or similar.  What
> > I don't like to see is pointers to MFD cells inside ACPI driver data
> > (or whatever the correct nomenclature is).
> 
> 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.

> > > > > +	ret = regmap_read(upboard->regmap, UPBOARD_REG_PLATFORM_ID, &platform_id);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	manufacturer_id = platform_id & 0xff;
> > > > > +	if (manufacturer_id != AAEON_MANUFACTURER_ID) {
> > > > > +		dev_dbg(upboard->dev,
> > > > 
> > > > If you are returning an error, this should be dev_err().
> > > 
> > > It's not really an error. In vendor-specific applications, the FPGA
> > > firmware could be customized for some purpose other than platform
> > > control, and then this driver simply doesn't apply.
> > > 
> > > Hence dev_dbg and -ENODEV to silently end the probe.
> > 
> > -ENODEV is an error.
> > 
> [...]
> > > Similar to above - why consider it an error if this driver can't handle
> > > a non-backwards-compatible future configuration? Some other driver may
> > > be able to handle that.
> > 
> > If you are returning because something is not supported or provides
> > the wrong information (-ENODEV or EINVAL) for instance, it's an
> > error.  The 'E' stands for Error.
> > 
> > You have dbg for debugging information, info for useful information,
> > warn for when there is an issue, but you can carry on and err for when
> > the driver can no longer continue.
> > 
> > An unsupported device is the latter.
> 
> 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.

> I won't insist any further though - if you're unconvinced these will
> become dev_err's :)

I'm convinced.

[...]

> > > > 	if (ret) {
> > > > 		if (ret != -EPROBE_DEFER)
> > > > 			dev_err(&pdev->dev, "failed to init controller GPIOs: %d", ret);
> > > > 		return ret;
> > > > 	}
> > > 
> > > I assumed (but didn't verify) that the compiler would sort that out.
> > > Anyway it still looks awkward so I'll rewrite.
> > 
> > How could it?  Deferring probe is a run-time ordering thing.
> 
> *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?

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