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?
> +// 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.
> +// 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.
> +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
> + /* 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.
> + if (unlikely(selector >= rdev->desc->n_voltages))
> + return -EINVAL;
This should not be a hot path that needs an unlikely() annotation.
> +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?
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
