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!
