Em Mon, 18 Jan 2021 13:54:40 +0000
Mark Brown <broo...@kernel.org> escreveu:
> On Mon, Jan 18, 2021 at 02:28:12PM +0100, Mauro Carvalho Chehab wrote:
>
> > index f385146d2bd1..3b23ad56b31a 100644
> > --- a/Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml
> > +++ b/Documentation/devicetree/bindings/mfd/hisilicon,hi6421-spmi-pmic.yaml
> > @@ -60,6 +60,8 @@ required:
> > - reg
> > - regulators
> >
> > +additionalProperties: false
> > +
> > examples:
> > - |
> > /* pmic properties */
>
> Why is this part of this patch?
I'll place on a separate one.
>
> > +// SPDX-License-Identifier: GPL-2.0
> > +//
> > +// Device driver for regulators in Hisi IC
> > +//
> > +// Copyright (c) 2013 Linaro Ltd.
> > +// Copyright (c) 2011 Hisilicon.
> > +//
>
> This looks like it needs an update.
Ok.
>
> > +// 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.
>
> This boilerplate can be removed.
Ok.
>
> > +static int hi6421_spmi_regulator_enable(struct regulator_dev *rdev)
> > +{
> > + struct hi6421_spmi_reg_info *sreg = rdev_get_drvdata(rdev);
> > + struct hi6421_spmi_pmic *pmic = sreg->pmic;
> > + int ret;
> > +
> > + /* cannot enable more than one regulator at one time */
> > + mutex_lock(&sreg->enable_mutex);
> > + usleep_range(HISI_REGS_ENA_PROTECT_TIME,
> > + HISI_REGS_ENA_PROTECT_TIME + 1000);
> > +
> > + /* set enable register */
> > + ret = hi6421_spmi_pmic_rmw(pmic, rdev->desc->enable_reg,
> > + rdev->desc->enable_mask,
> > + rdev->desc->enable_mask);
>
> If for some reason the PMIC is sufficiently fragile to need a delay
> between enables it's not clear why the driver is doing it before
> enabling rather than after, presumably there's issues with the regulator
> ramping up and stabalising its output
I don't have any datasheets or documentation from this device, except for
the Linaro's official driver for Kernel 4.9 (from where this driver
was originally ported), and a high-level documentation for this
hardware at 96boards site, which doesn't cover any details.
-
My understanding is that the issue here seems to be different.
This is the original code (which can be seen on changeset 42f24d9d446a,
already upstream):
struct timeval last_enabled;
static void ensured_time_after(struct timeval since, u32 delay_us)
{
struct timeval now;
u64 elapsed_ns64, delay_ns64;
u32 actual_us32;
delay_ns64 = delay_us * NSEC_PER_USEC;
do_gettimeofday(&now);
elapsed_ns64 = timeval_to_ns(&now) - timeval_to_ns(&since);
if (delay_ns64 > elapsed_ns64) {
actual_us32 = ((u32)(delay_ns64 - elapsed_ns64) /
NSEC_PER_USEC);
if (actual_us32 >= 1000) {
mdelay(actual_us32 / 1000); /*lint !e647 */
udelay(actual_us32 % 1000);
} else if (actual_us32 > 0) {
udelay(actual_us32);
}
}
return;
}
static int hisi_regulator_enable(struct regulator_dev *dev)
{
struct hisi_regulator *sreg = rdev_get_drvdata(dev);
struct hisi_pmic *pmic = rdev_to_pmic(dev);
/* keep a distance of off_on_delay from last time disabled */
ensured_time_after(sreg->last_off_time, sreg->off_on_delay);
BRAND_DEBUG("<[%s]: off_on_delay=%dus>\n", __func__,
sreg->off_on_delay);
/* cannot enable more than one regulator at one time */
mutex_lock(&enable_mutex);
ensured_time_after(last_enabled, HISI_REGS_ENA_PROTECT_TIME);
/* set enable register */
hisi_pmic_rmw(pmic, sreg->register_info.ctrl_reg,
sreg->register_info.enable_mask,
sreg->register_info.enable_mask);
BRAND_DEBUG("<[%s]: ctrl_reg=0x%x,enable_mask=0x%x>\n",
__func__, sreg->register_info.ctrl_reg,\
sreg->register_info.enable_mask);
do_gettimeofday(&last_enabled);
mutex_unlock(&enable_mutex);
return 0;
}
What I did here was to play safe. So, I removed the static var and the
call to gettimeofday() and replaced by just a delay.
As, on this device, the power lines don't change too often: only
the USB power supply changes over time during its reset sequence.
The other power lines are powered during the boot and remain powered
if the hardware is detected.
> > + /* set enable register to 0 */
> > + return hi6421_spmi_pmic_rmw(pmic, rdev->desc->enable_reg,
> > + rdev->desc->enable_mask, 0);
>
> I'm not sure all these comments are adding anything.
I'll drop on a next version.
>
> > + if (unlikely(selector >= rdev->desc->n_voltages))
> > + return -EINVAL;
>
> This should not be a hot path that needs an unlikely() annotation.
I'll drop unlikely().
> > +static unsigned int
> > +hi6421_spmi_regulator_get_optimum_mode(struct regulator_dev *rdev,
> > + int input_uV, int output_uV,
> > + int load_uA)
> > +{
> > + struct hi6421_spmi_reg_info *sreg = rdev_get_drvdata(rdev);
> > +
> > + if (load_uA || ((unsigned int)load_uA > sreg->eco_uA))
> > + return REGULATOR_MODE_NORMAL;
>
> This means that for *any* load at all we select NORMAL - I'm not
> convinced this is intentional?
Indeed this seems to be a bug. Thanks for noticing it!
I'll fix that.
Thanks,
Mauro
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
