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

[Lee Jones <lee.jones@xxxxxxxxxx>]

[...]

> > > +struct upboard_data {
> > > +	const struct regmap_config *regmapconf;
> > > +	const struct mfd_cell *cells;
> > > +	size_t ncells;
> > 
> > Not sure I like where this is heading ...
> > 
> > Okay, I've now seen what this does.  Please don't mix and match
> > platform data technologies (MFD, ACPI, DT, etc).  In this case you
> > should match on an ACPI ID and select the correct populated static MFD
> > struct in a switch statement.  There are lots of examples of this
> > already in MFD.
> 
> 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 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).

> > > +static const struct mfd_cell upboard_up2_mfd_cells[] = {
> > > +};
> > 
> > Why are you supplying an empty static struct?
> 
> As above. At this point in the series the array achieves nothing yet.

That's a NACK. :)

[...]

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

[...]

> > > +	if (major != SUPPORTED_FW_MAJOR) {
> > 
> > This could get messy after supporting a few different versions.
> 
> Why? Rewriting this check if that ever happens doesn't seem hard.

Depends how many platforms you support I guess, but yes, it's fine for
one or two and can be re-written if anything significant changes.
Just something to bear in mind.

> > > +		dev_dbg(upboard->dev, "unsupported FPGA firmware v%u.%u.%u.%u",
> > > +			major, minor, patch, build);
> > 
> > dev_err()
> 
> 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.

> > > +static int upboard_probe(struct platform_device *pdev)
> > > +{
> > > +	struct upboard *upboard;
> > > +	const struct acpi_device_id *id;
> > > +	const struct upboard_data *upboard_data;
> > > +	int ret;
> > > +
> > > +	id = acpi_match_device(upboard_acpi_match, &pdev->dev);
> > > +	if (!id)
> > > +		return -ENODEV;
> > > +
> > > +	upboard_data = (const struct upboard_data *) id->driver_data;
> > > +
> > > +	upboard = devm_kzalloc(&pdev->dev, sizeof(*upboard), GFP_KERNEL);
> > > +	if (!upboard)
> > > +		return -ENOMEM;
> > > +
> > > +	dev_set_drvdata(&pdev->dev, upboard);
> > 
> > Where is this consumed?
> 
> The child drivers grab this as dev_get_drvdata(pdev->dev.parent).
> I'm actually not sure about the proper way to do that. A previous
> question fell through - quoting below:
> 
> > 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.

> > Andy suggested platform_data allows more flexibility on the parent
> > device side (although I can't see upboard-pinctrl being used other than
> > as a child of the upboard driver).
> > 
> > I went with parent drvdata simply because that's what I found in other
> > MFD drivers and material [1].
> > 
> > Thank you!
> > 
> > [1]: http://events17.linuxfoundation.org/sites/events/files/slides/belloni-mfd-regmap-syscon_0.pdf
> 
> (from <20180426133653.4yhjv6uuaox7vt7m@localhost>)
> 
> > > +	ret = upboard_init_gpio(upboard);
> > > +	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.

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

[...]

> > > +/**
> > > + * struct upboard - internal data shared by the UP MFD and its child drivers
> > 
> > ddata should be used internally by the MFD.
> 
> Yes, this was moved to drivers/mfd/upboard.c now.
> 
> The only thing we were actually interested in passing is the regmap.
> 
> > If you're passing data to the children, you should do it in their
> > pdata.
> 
> As above - when to use cell platdata vs. parent drvdata?

There are few times when accessing a parent's drvdata is preferable.

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