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

[Javier Arteaga <javier@xxxxxxxxxx>]

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?

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

Did I get that right? If so - what is the benefit?

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

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

> > > I have one further question about MFD in this patch too - should I keep
> > > passing regmap into the driver via dev_get_drvdata(pdev->dev.parent),
> > > or is explicit platform_data preferable?
> 
> platform_data is preferable.

Thanks.

> > > > +	if (ret && ret != -EPROBE_DEFER)
> > > > +		dev_err(&pdev->dev, "failed to init controller GPIOs: %d", ret);
> > > > +	if (ret)
> > > > +		return ret;
> > > 
> > > You should save some cycles during the !error path here by:
> > > 
> > > 	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.

(You were right about changing that code anyway).

> > > > +	ret = upboard_check_supported(upboard);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	return devm_mfd_add_devices(&pdev->dev, PLATFORM_DEVID_AUTO,
> > > > +				    upboard_data->cells, upboard_data->ncells,
> > > > +				    NULL, 0, NULL);
> > > 
> > > This doesn't do anything.
> > 
> > Not at this point. Again, I can introduce it along with the first cell
> > if that's OK.
> 
> At least one, yes.
> 
> Else you are upstreaming un-functional code, which is not allowed.
> 
> [...]

As above - I can't quite see the right way to do this yet.

> > As above - when to use cell platdata vs. parent drvdata?
> 
> There are few times when accessing a parent's drvdata is preferable.

Thank you!
--
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