Re: [PATCH v2 11/13] regulator: hi6421v600-regulator: move it from staging

[Mark Brown]

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