Hi Mark,
Em Mon, 16 Nov 2020 18:38:33 +0000
Mark Brown <broonie@xxxxxxxxxx> escreveu:
> > This driver is ready for mainstream. Move it out of staging.
>
> There's quite a few issues here, to be honest I'm disappointed some of
> them weren't caught during staging review, this needs fairly substantial
> work and there's signs that this also applies to at least the MFD
> portion.
>
...
> > +static int hi6421_spmi_regulator_probe_ldo(struct platform_device *pdev,
> > + struct device_node *np,
> > + struct hi6421_spmi_pmic *pmic)
> > +{
>
> This probe code looks very different to other regulator drivers, this
> alone should have been a warning that the driver needs some substantial
> refactoring here. As indicated information about what regulators are
> present on devices and their properties should be in the driver not the
> DT, the driver should just be able to register them unconditionally and
> use of_match and regulators_node to allow the core to find any
> constraints that are provided by the platform.
Let me reply to this before handling the other issues you pointed, as
this one is related to some design decisions I had to make for this driver
to properly work upstream.
FYI, all documentation I have about this board is at:
https://www.96boards.org/documentation/consumer/hikey/hikey970/
-
The setup for MFD/regulator is different than almost all other
regulator drivers currently upstreamed[1].
The PM part of Hikey970 is based on a master/slave SPMI bus. Each
bus can have up to 16 PM chips connected into it.
It means that, for the regulator driver to work, two drivers
should be probed first: the SPMI bus controller driver
(hisi-spmi-controller.c) and the SPMI bus client driver, which is
at the MFD driver(hi6421-spmi-pmic.c).
Only after having both probed, the regulator driver can be
probed.
Also, as all the communication between the PM chip
and the SoC happens via a single serial bus, there's no
sense on probing the regulators in parallel.
That's mainly the reason why I opted to serialize the probe
inside hi6421v600-regulator.c.
The relevant changeset that ensures that everything is
probed the right way is this one:
75937f8f961e ("staging: regulator: hi6421v600-regulator: change the binding logic")
Without such change, the driver doesn't work upstream, as the
regulator driver ends by being probed before the bus client
driver (MFD).
There's a second reason, though: when letting regulator probe to
happen in parallel, the LDOs got probed on a random order, which
makes more difficult to debug the driver, as the LDO numbering
may not be following the LDO name, making harder to debug the
drivers that depend on regulator support.
Thanks,
Mauro
[1] The only other drivers for SPMI bus are from some Qualcomm
based boards - those seem to be using a different setup.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
