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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 18 Jan 2021 13:54:40 +0000
From:   Mark Brown <broonie@...nel.org>
To:     Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Lee Jones <lee.jones@...aro.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mayulong <mayulong1@...wei.com>,
        Rob Herring <robh+dt@...nel.org>, devel@...verdev.osuosl.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 11/13] regulator: hi6421v600-regulator: move it from
 staging

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?

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)