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!
