linux-kernel - Re: [PATCH v5 2/4] regulator: max77686: Store opmode non-shifted

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, 27 Oct 2014 14:37:18 +0100
From:	Javier Martinez Canillas <javier.martinez@...labora.co.uk>
To:	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>, linux-kernel@...r.kernel.org,
	Ben Dooks <ben-linux@...ff.org>,
	Kukjin Kim <kgene.kim@...sung.com>,
	Russell King <linux@....linux.org.uk>,
	linux-arm-kernel@...ts.infradead.org,
	linux-samsung-soc@...r.kernel.org, devicetree@...r.kernel.org
CC:	Kyungmin Park <kyungmin.park@...sung.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
	Chanwoo Choi <cw00.choi@...sung.com>
Subject: Re: [PATCH v5 2/4] regulator: max77686: Store opmode non-shifted

Hello Krzysztof,

On 10/27/2014 01:11 PM, Krzysztof Kozlowski wrote:
> Introduce simple helper for calculating the shift for OPMODE field in
> registers. This allows storing the current value of opmode in
> non-shifted form and simplifies a little set_suspend_disable and enable
> functions. Additionally this will allow adding support LDOs to the
> existing set_suspend_disable function.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@...sung.com>
> Suggested-by: Javier Martinez Canillas <javier.martinez@...labora.co.uk>
> ---
>  drivers/regulator/max77686.c | 49 ++++++++++++++++++++++++++++++--------------
>  1 file changed, 34 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/regulator/max77686.c b/drivers/regulator/max77686.c

The patch looks good to me.

Reviewed-by: Javier Martinez Canillas <javier.martinez@...labora.co.uk>

>  
>  static int max77686_set_ramp_delay(struct regulator_dev *rdev, int ramp_delay)
> @@ -495,7 +513,8 @@ static int max77686_pmic_probe(struct platform_device *pdev)
>  		config.init_data = pdata->regulators[i].initdata;
>  		config.of_node = pdata->regulators[i].of_node;
>  
> -		max77686->opmode[i] = regulators[i].enable_mask;
> +		max77686->opmode[i] = regulators[i].enable_mask >>
> +						max77686_get_opmode_shift(i);

I don't think it has to be done in your patch but as a follow-up it would be
good to change this to:

		max77686->opmode[i] = MAX77686_NORMAL;

since that is really what this code is trying to do AFAIU. This just happens
to work because the value of MAX77686_OPMODE_MASK (0x3) is also "Output ON in
Normal Mode" but the code is not correct IMHO.

Or even better, the regulator mode should be read from the hw registers instead
of setting always to normal. That is what the max77802 driver does for example.

Best regards,
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/