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

[Javier Arteaga <javier@xxxxxxxxxx>]

Hi Lee,

On Tue, May 01, 2018 at 10:16:29AM +0100, Lee Jones wrote:
> > +static int upboard_read(void *context, unsigned int reg, unsigned int *val)
> > +{
> > +	const struct upboard * const upboard = context;
> 
> This is more traditionally called ddata.

Renamed. Thanks!

> > +	int i;
> > +
> > +	gpiod_set_value(upboard->clear_gpio, 0);
> > +	gpiod_set_value(upboard->clear_gpio, 1);
> 
> What does flipping the Clear GPIO actually do?
> 
> Comments please.

This pulse signals the start of a read/write sequence.
I'll document that too.

> > +static int upboard_write(void *context, unsigned int reg, unsigned int val)
> > +{
> > +	const struct upboard * const upboard = context;
> 
> This is more traditionally called ddata.

Renamed.

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

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.

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

> > +static const struct upboard_data upboard_up2_data = {
> > +	.regmapconf = &upboard_up2_regmap_config,
> > +	.cells = upboard_up2_mfd_cells,
> > +	.ncells = ARRAY_SIZE(upboard_up2_mfd_cells),
> 
> Again, empty?

Same reason.

> > +static int upboard_init_gpio(struct upboard *upboard)
> 
> Better to pass pdev and use dev_set_drvdata() to obtain your ddata.

OK.

> > +static int upboard_check_supported(struct upboard *upboard)
> > +{
> > +	const unsigned int AAEON_MANUFACTURER_ID = 0x01;
> > +	const unsigned int SUPPORTED_FW_MAJOR = 0x0;
> 
> Why aren't these defines?

They are now.

> > +	unsigned int platform_id, manufacturer_id;
> > +	unsigned int firmware_id, build, major, minor, patch;
> > +	int ret;
> > +
> > +	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.

> > +	build = (firmware_id >> 12) & 0xf;
> > +	major = (firmware_id >> 8) & 0xf;
> > +	minor = (firmware_id >> 4) & 0xf;
> > +	patch = firmware_id & 0xf;
> 
> Not keen on non-defined magic numbers.  How about:
> 
> UPBOARD_BUILD_SHIFT		12
> (etc... )
> UPBOARD_GET_BUILD_NUM(x)	((x >> UPBOARD_BUILD_SHIFT) & 0x0f)
> (etc...)
> 
> build = UPBOARD_GET_BUILD_NUM(firmware_id);

Sure. I'll throw in some #defines.

> Also, why aren't these uint8_t?

Right, will do.

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

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

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

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

> Also, you need to check the return value and inform your user(s) of
> any failure.

Okay.

> > +static struct platform_driver upboard_driver = {
> > +	.probe = upboard_probe,
> 
> Nothing to do in .remove?

After discussion in another thread, next iteration will maybe return the
FPGA pins to Hi-Z upon remove.

> > +	.driver = {
> > +		.name = "upboard",
> > +		.acpi_match_table = upboard_acpi_match,
> > +	},
> > +};
> > +
> 
> Nit: remove this line.

Removed.

> 
> > +module_platform_driver(upboard_driver);
> > +
> > +MODULE_AUTHOR("Javier Arteaga <javier@xxxxxxxxxx>");
> > +MODULE_DESCRIPTION("UP Board platform controller driver");
> > +MODULE_LICENSE("GPL");
> 
> Version mismatch with the header comment.

Fixed for next series (reads "GPL v2").

> > +/**
> > + * 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?

Thanks for your time!
--
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