Re: [PATCH 4/8] regulator: hi6421v600-regulator: move it from staging
On Tue, Nov 17, 2020 at 09:38:30AM +0100, Mauro Carvalho Chehab wrote: > Mark Brown escreveu: > > This also appears to be missing a DT binding document, binding > > documentation is required for anything with DT support. > The DT binding is documented on patch 3/8, together with MFD. > As there's just one compatible for MFD and regulators altogether, > I opted to have just one DT binding document for both. I should've been copied on that patch then so the bindings can be reviewed. > > > +static DEFINE_MUTEX(enable_mutex); > > Drivers shouldn't be declaring global variables, if this is required it > > should be allocated in driver data. > I'll try to see a better place for this, but in this case, the > mutex should be global anyway, as the access to the SPMI bus > should be serialized. Surely the bus should be dealing with basic I/O serialisation? > > It looks like it would be less code overall to just implement a regmap > > for the MFD, even if it were only used in this driver - almost > > everything in here is just a carbon copy of standard helpers that the > > core provides for regmap devices. Doing it in the MFD would be more > > idiomatic for everything though. > I tried to use regmap for this driver while porting it from Linaro's > OOT to upstream, bkut it turns that adding support for it is not trivial. Could you expand on "not trivial" please? What did you try and what difficulties did you encounter? > I suspect that, just like I2C has their own set of regmap functions > (regmap_init_i2c, devm_regmap_init_i2c), if we want to properly support > regmap, we need first to add a SPMI variant to it, as the locking > should be bus-aware. drivers/base/regmap/regmap-spmi.c has been there since 2013, or if for some reason this device is doing something non-standard for the bus then the reg_read() and reg_write() operations can be used. > > > + ret = of_property_read_u32(np, "vsel-reg", >vsel_reg); > > There is no way this stuff should be being parsed out of DT, we should > > know the register map for the device and what regulators it has based > > purely off knowing what device we have. Baking the register map into > > the ABI is bad practice, it prevents us from improving our support for > > the hardware without going and updating people's DTs. > Well, I can move the existing registers out of DT, but, as I don't > have any datasheet for this device, it means that I can implement > there only a subset of the available LDOs. So, from the 36 LDOs that > this chip support, we only know the registers for 8 of them: > ldo3, ldo4, ldo9, ldo15, ldo16, ldo17, ldo33 and ldo34. People will be free to submit patches adding additional functionality to the driver if they need it. > > > + /* > > > + * Not all regulators work on idle mode > > > + */ > > > + ret = of_property_read_u32(np, "idle-mode-mask", >eco_mode_mask); > > There's no conditional code to not register the mode operations if the > > mode information is not available (and again this should be done based > > on knowing the properties of the device, this is unlikely to be system > > dependent). > This is the same case as before: as we don't have any datasheets, > I only know what's there at the DT for just one device (Hikey 970). > Again, we could hardcode those assuming Hikey 970 settings, but, > if some other device using the same chip will ever be added, and > they use some different configuration then we may face some > backward-compatibility issues. I can't follow the logic here, sorry. If we have no idea how to configure something how would offering operations to configure that thing be useful? > > > + constraint->valid_modes_mask = REGULATOR_MODE_NORMAL; > > This is absolutely not OK, a regulator driver should *not* be modifying > > the constraints that the machine has set. If it is safe to change modes > > on a platform and the system integrator wants to do that then they will > > set the constraints appropriately, there is no way the regulator driver > > can tell what is appropriate on a given system. The fact that the > > driver is including machine.h at all ought to have been an indicator > > that there's an abstraction problem here. > Ok. Where such constraints should be instead? at the Hikey 970 DT > file? They should be configured whereever all the other constraints are configured by the platform, if that is DT then they should also be configured in DT. signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/8] regulator: hi6421v600-regulator: move it from staging
On Tue, Nov 17, 2020 at 09:08:22AM +0100, Mauro Carvalho Chehab wrote: > Mark Brown escreveu: > > 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. > The setup for MFD/regulator is different than almost all other > regulator drivers currently upstreamed[1]. It really shouldn't be doing anything unusual. > 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. This is totally fine and very common for drivers in general, a combination of probe deferral and fw_devlink exists to sort this stuff out. > 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. I can't think of a regulator driver that doesn't have an entirly serialized probe routine, that's not the issue. The issue is that almost nothing that the probe routine is doing is done by other regulator drivers. signature.asc Description: PGP signature ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/8] regulator: hi6421v600-regulator: move it from staging
Em Mon, 16 Nov 2020 18:38:33 +
Mark Brown escreveu:
> On Mon, Nov 16, 2020 at 01:59:30PM +0100, Mauro Carvalho Chehab wrote:
>
> > 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.
>
> This also appears to be missing a DT binding document, binding
> documentation is required for anything with DT support.
The DT binding is documented on patch 3/8, together with MFD.
As there's just one compatible for MFD and regulators altogether,
I opted to have just one DT binding document for both.
>
> > +config REGULATOR_HI6421V600
> > + tristate "HiSilicon Hi6421v600 PMIC voltage regulator support"
> > + depends on MFD_HI6421_SPMI && OF
> > + depends on REGULATOR
>
> This is inside an if REGULATOR block, as with all the other regulator
> drivers it doesn't need a dependency on REGULATOR.
I'll drop it. This was needed while this was inside staging.
>
> > --- /dev/null
> > +++ b/drivers/regulator/hi6421v600-regulator.c
> > @@ -0,0 +1,478 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Device driver for regulators in Hisi IC
>
> Please make the entire comment a C++ one so things look more
> intentional.
Ok.
>
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
>
> Are we sure about *all* these includes?
I'll double check and do some cleanups.
>
> > +#define rdev_dbg(rdev, fmt, arg...)\
> > +pr_debug("%s: %s: " fmt, (rdev)->desc->name, __func__, ##arg)
>
> If it is useful to copy this then put it in a header rather than
> cut'n'pasting it. I'm not sure I see a pressing need for most of the
> trace here, it looks to be duplicating a lot of things the core does for
> you.
I'll cleanup the debug messages, and place this on a header.
Due to SPMI bus, the standard debug macros don't work fine on
this file.
>
> > +static DEFINE_MUTEX(enable_mutex);
>
> Drivers shouldn't be declaring global variables, if this is required it
> should be allocated in driver data.
I'll try to see a better place for this, but in this case, the
mutex should be global anyway, as the access to the SPMI bus
should be serialized.
I suspect that a change like that will likely require touching
the SPMI bus core, but I can't foresee the side effects to the
Qualcomm SPMI drivers that would likely have their own ways
to serialize access to the bus.
>
> > +/*
> > + * helper function to ensure when it returns it is at least 'delay_us'
> > + * microseconds after 'since'.
> > + */
> > +
> > +static int hi6421_spmi_regulator_is_enabled(struct regulator_dev *rdev)
>
> The helper function appears to have been removed?
I'll drop the comment.
>
> > +{
> > + struct hi6421v600_regulator *sreg = rdev_get_drvdata(rdev);
> > + struct hi6421_spmi_pmic *pmic = sreg->pmic;
> > + u32 reg_val;
> > +
> > + reg_val = hi6421_spmi_pmic_read(pmic, rdev->desc->enable_reg);
> > +
> > + rdev_dbg(rdev,
> > +"enable_reg=0x%x, val= 0x%x, enable_state=%d\n",
> > +rdev->desc->enable_reg,
> > +reg_val, (reg_val & rdev->desc->enable_mask));
> > +
> > + return ((reg_val & rdev->desc->enable_mask) != 0);
> > +}
>
> It looks like it would be less code overall to just implement a regmap
> for the MFD, even if it were only used in this driver - almost
> everything in here is just a carbon copy of standard helpers that the
> core provides for regmap devices. Doing it in the MFD would be more
> idiomatic for everything though.
I tried to use regmap for this driver while porting it from Linaro's
OOT to upstream, but it turns that adding support for it is not trivial.
I suspect that, just like I2C has their own set of regmap functions
(regmap_init_i2c, devm_regmap_init_i2c), if we want to properly support
regmap, we need first to add a SPMI variant to it, as the locking
should be bus-aware.
>
> > +static int hi6421_spmi_regulator_enable(struct regulator_dev *rdev)
> > +{
> > + struct hi6421v600_regulator *sreg = rdev_get_drvdata(rdev);
> > + struct hi6421_spmi_pmic *pmic = sreg->pmic;
> > +
> > + /* cannot enable more than one regulator at one time */
> > + mutex_lock(_mutex);
> > + usleep_range(HISI_REGS_ENA_PROTECT_TIME,
> > +HISI_REGS_ENA_PROTECT_TIME + 1000);
> > +
> > + /* set enable register */
> > + rdev_dbg(rdev,
> > +"off_on_delay=%d us, enable_reg=0x%x, enable_mask=0x%x\n",
> > +rdev->desc->off_on_delay, rdev->desc->enable_reg,
> > +rdev->desc->enable_mask);
> > +
> > + hi6421_spmi_pmic_rmw(pmic,
Re: [PATCH 4/8] regulator: hi6421v600-regulator: move it from staging
Hi Mark,
Em Mon, 16 Nov 2020 18:38:33 +
Mark Brown 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
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/8] regulator: hi6421v600-regulator: move it from staging
On Mon, Nov 16, 2020 at 01:59:30PM +0100, Mauro Carvalho Chehab wrote:
> 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.
This also appears to be missing a DT binding document, binding
documentation is required for anything with DT support.
> +config REGULATOR_HI6421V600
> + tristate "HiSilicon Hi6421v600 PMIC voltage regulator support"
> + depends on MFD_HI6421_SPMI && OF
> + depends on REGULATOR
This is inside an if REGULATOR block, as with all the other regulator
drivers it doesn't need a dependency on REGULATOR.
> --- /dev/null
> +++ b/drivers/regulator/hi6421v600-regulator.c
> @@ -0,0 +1,478 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Device driver for regulators in Hisi IC
Please make the entire comment a C++ one so things look more
intentional.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Are we sure about *all* these includes?
> +#define rdev_dbg(rdev, fmt, arg...) \
> + pr_debug("%s: %s: " fmt, (rdev)->desc->name, __func__, ##arg)
If it is useful to copy this then put it in a header rather than
cut'n'pasting it. I'm not sure I see a pressing need for most of the
trace here, it looks to be duplicating a lot of things the core does for
you.
> +static DEFINE_MUTEX(enable_mutex);
Drivers shouldn't be declaring global variables, if this is required it
should be allocated in driver data.
> +/*
> + * helper function to ensure when it returns it is at least 'delay_us'
> + * microseconds after 'since'.
> + */
> +
> +static int hi6421_spmi_regulator_is_enabled(struct regulator_dev *rdev)
The helper function appears to have been removed?
> +{
> + struct hi6421v600_regulator *sreg = rdev_get_drvdata(rdev);
> + struct hi6421_spmi_pmic *pmic = sreg->pmic;
> + u32 reg_val;
> +
> + reg_val = hi6421_spmi_pmic_read(pmic, rdev->desc->enable_reg);
> +
> + rdev_dbg(rdev,
> + "enable_reg=0x%x, val= 0x%x, enable_state=%d\n",
> + rdev->desc->enable_reg,
> + reg_val, (reg_val & rdev->desc->enable_mask));
> +
> + return ((reg_val & rdev->desc->enable_mask) != 0);
> +}
It looks like it would be less code overall to just implement a regmap
for the MFD, even if it were only used in this driver - almost
everything in here is just a carbon copy of standard helpers that the
core provides for regmap devices. Doing it in the MFD would be more
idiomatic for everything though.
> +static int hi6421_spmi_regulator_enable(struct regulator_dev *rdev)
> +{
> + struct hi6421v600_regulator *sreg = rdev_get_drvdata(rdev);
> + struct hi6421_spmi_pmic *pmic = sreg->pmic;
> +
> + /* cannot enable more than one regulator at one time */
> + mutex_lock(_mutex);
> + usleep_range(HISI_REGS_ENA_PROTECT_TIME,
> + HISI_REGS_ENA_PROTECT_TIME + 1000);
> +
> + /* set enable register */
> + rdev_dbg(rdev,
> + "off_on_delay=%d us, enable_reg=0x%x, enable_mask=0x%x\n",
> + rdev->desc->off_on_delay, rdev->desc->enable_reg,
> + rdev->desc->enable_mask);
> +
> + hi6421_spmi_pmic_rmw(pmic, rdev->desc->enable_reg,
> + rdev->desc->enable_mask,
> + rdev->desc->enable_mask);
This is the one operation which is doing anything unusual and which I'd
expect to be open coded in the driver - obviously this is a pretty
simplistic implementation but it will work though as indicated above it
shouldn't be using a global, the mutex should be in driver data. I
guess you could use the mutex to protect a timestamp and use that to
figure out if a delay is actually needed?
> +static int hi6421_spmi_dt_parse(struct platform_device *pdev,
> + struct hi6421v600_regulator *sreg,
> + struct regulator_desc *rdesc)
> +{
> + struct device *dev = >dev;
> + struct device_node *np = dev->of_node;
> + unsigned int *v_table;
> + int ret;
> +
> + ret = of_property_read_u32(np, "reg", >enable_reg);
> + if (ret) {
> + dev_err(dev, "missing reg property\n");
> + return ret;
> + }
> +
> + ret = of_property_read_u32(np, "vsel-reg", >vsel_reg);
There is no way this stuff should be being parsed out of DT, we should
know the register map for the device and what regulators it has based
purely off knowing what device we have. Baking the register map into
the ABI is bad practice, it prevents us from improving our support for
the hardware without going and updating people's DTs.
> + /*
> + * Not all regulators
[PATCH 4/8] regulator: hi6421v600-regulator: move it from staging
This driver is ready for mainstream. Move it out of staging.
Signed-off-by: Mauro Carvalho Chehab
---
MAINTAINERS | 7 +-
drivers/regulator/Kconfig | 9 +
drivers/regulator/Makefile| 1 +
drivers/regulator/hi6421v600-regulator.c | 478 ++
drivers/staging/Kconfig | 2 -
drivers/staging/Makefile | 1 -
drivers/staging/hikey9xx/Kconfig | 11 -
drivers/staging/hikey9xx/Makefile | 3 -
drivers/staging/hikey9xx/TODO | 5 -
.../staging/hikey9xx/hi6421v600-regulator.c | 478 --
10 files changed, 489 insertions(+), 506 deletions(-)
create mode 100644 drivers/regulator/hi6421v600-regulator.c
delete mode 100644 drivers/staging/hikey9xx/Kconfig
delete mode 100644 drivers/staging/hikey9xx/Makefile
delete mode 100644 drivers/staging/hikey9xx/TODO
delete mode 100644 drivers/staging/hikey9xx/hi6421v600-regulator.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 450c7cbc6725..aa68aee9e684 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8000,12 +8000,7 @@ L: linux-ker...@vger.kernel.org
S: Maintained
F: Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml
F: drivers/mfd/hi6421-spmi-pmic.c
-
-HISILICON STAGING DRIVERS FOR HIKEY 960/970
-M: Mauro Carvalho Chehab
-L: de...@driverdev.osuosl.org
-S: Maintained
-F: drivers/staging/hikey9xx/
+F: drivers/regulator/hi6421v600-regulator.c
HISILICON TRUE RANDOM NUMBER GENERATOR V2 SUPPORT
M: Zaibo Xu
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 020a00d6696b..08d302c87fa0 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -394,6 +394,15 @@ config REGULATOR_HI655X
This driver provides support for the voltage regulators of the
Hisilicon Hi655x PMIC device.
+config REGULATOR_HI6421V600
+ tristate "HiSilicon Hi6421v600 PMIC voltage regulator support"
+ depends on MFD_HI6421_SPMI && OF
+ depends on REGULATOR
+ help
+ This driver provides support for the voltage regulators on
+ HiSilicon Hi6421v600 PMU / Codec IC.
+ This is used on Kirin 3670 boards, like HiKey 970.
+
config REGULATOR_ISL9305
tristate "Intersil ISL9305 regulator"
depends on I2C
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 6ebae516258e..45d1883de54b 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_REGULATOR_FAN53880) += fan53880.o
obj-$(CONFIG_REGULATOR_GPIO) += gpio-regulator.o
obj-$(CONFIG_REGULATOR_HI6421) += hi6421-regulator.o
obj-$(CONFIG_REGULATOR_HI6421V530) += hi6421v530-regulator.o
+obj-$(CONFIG_REGULATOR_HI6421V600) += hi6421v600-regulator.o
obj-$(CONFIG_REGULATOR_HI655X) += hi655x-regulator.o
obj-$(CONFIG_REGULATOR_ISL6271A) += isl6271a-regulator.o
obj-$(CONFIG_REGULATOR_ISL9305) += isl9305.o
diff --git a/drivers/regulator/hi6421v600-regulator.c
b/drivers/regulator/hi6421v600-regulator.c
new file mode 100644
index ..614b03c9ddfb
--- /dev/null
+++ b/drivers/regulator/hi6421v600-regulator.c
@@ -0,0 +1,478 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Device driver for regulators in Hisi IC
+ *
+ * Copyright (c) 2013 Linaro Ltd.
+ * Copyright (c) 2011 Hisilicon.
+ *
+ * Guodong Xu
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define rdev_dbg(rdev, fmt, arg...)\
+pr_debug("%s: %s: " fmt, (rdev)->desc->name, __func__, ##arg)
+
+struct hi6421v600_regulator {
+ struct regulator_desc rdesc;
+ struct hi6421_spmi_pmic *pmic;
+ u32 eco_mode_mask, eco_uA;
+};
+
+static DEFINE_MUTEX(enable_mutex);
+
+/*
+ * helper function to ensure when it returns it is at least 'delay_us'
+ * microseconds after 'since'.
+ */
+
+static int hi6421_spmi_regulator_is_enabled(struct regulator_dev *rdev)
+{
+ struct hi6421v600_regulator *sreg = rdev_get_drvdata(rdev);
+ struct hi6421_spmi_pmic *pmic = sreg->pmic;
+ u32 reg_val;
+
+ reg_val = hi6421_spmi_pmic_read(pmic, rdev->desc->enable_reg);
+
+ rdev_dbg(rdev,
+"enable_reg=0x%x, val= 0x%x, enable_state=%d\n",
+rdev->desc->enable_reg,
+reg_val, (reg_val
