linux-kernel - Re: [PATCH RFC v4 02/13] regulator: add getter for active monitors

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]

Message-ID: <4a71b08e-0c0d-a378-7a3a-0cd12912d4d4@gmail.com>
Date:   Mon, 26 Jun 2023 16:43:49 +0300
From:   Matti Vaittinen <mazziesaccount@...il.com>
To:     Benjamin Bara <bbara93@...il.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>
Cc:     support.opensource@...semi.com,
        DLG-Adam.Ward.opensource@...renesas.com,
        Martin Fuzzey <martin.fuzzey@...wbird.group>,
        linux-kernel@...r.kernel.org,
        Benjamin Bara <benjamin.bara@...data.com>
Subject: Re: [PATCH RFC v4 02/13] regulator: add getter for active monitors

On 6/20/23 23:02, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@...data.com>
> 
> Add an op to get all active monitors of a regulator. This is useful to
> find out if any monitor is turned on, of which the device-tree is not
> aware of (e.g. by bootloader or OTP).
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@...data.com>

Just a couple of minor remarks. Feel free to change those if you end up 
respinning.

Reviewed-by: Matti Vaittinen <mazziesaccount@...il.com>

> ---
>   include/linux/regulator/driver.h | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
> index d3b4a3d4514a..9a9163cae769 100644
> --- a/include/linux/regulator/driver.h
> +++ b/include/linux/regulator/driver.h
> @@ -25,6 +25,13 @@ struct regulator_config;
>   struct regulator_init_data;
>   struct regulator_enable_gpio;
>   
> +#define REGULATOR_MONITOR_NONE 0x0
> +#define REGULATOR_MONITOR_OVER_CURRENT 0x1
> +#define REGULATOR_MONITOR_OVER_VOLTAGE 0x2
> +#define REGULATOR_MONITOR_UNDER_VOLTAGE 0x4
> +#define REGULATOR_MONITOR_OVER_TEMPERATURE 0x8
> +#define REGULATOR_MONITOR_ALL 0xF

Not a big thing but maybe use BIT() to underline this is a bitmask?

> +
>   enum regulator_status {
>   	REGULATOR_STATUS_OFF,
>   	REGULATOR_STATUS_ON,
> @@ -112,6 +119,8 @@ enum regulator_detection_severity {
>    * @set_thermal_protection: Support enabling of and setting limits for over
>    *	temperature situation detection.Detection can be configured for same
>    *	severities as over current protection. Units of degree Kelvin.
> + * @get_active_protections: Get all enabled monitors of a regulator. OR'ed val
> + *	of REGULATOR_MONITOR_*.

I think it wouldn't hurt to have doc stating in which case populating 
this call-back is needed? I haven't read rest of the patches yet but I 
guess this callback is going to be used internally by the regulator core 
and maybe it is not obvious for driver author that this is needed by 
core to be able to support automatic protection handling.

>    *
>    * @set_active_discharge: Set active discharge enable/disable of regulators.
>    *
> @@ -183,6 +192,7 @@ struct regulator_ops {
>   					    int severity, bool enable);
>   	int (*set_thermal_protection)(struct regulator_dev *, int lim,
>   				      int severity, bool enable);
> +	int (*get_active_protections)(struct regulator_dev *dev, unsigned int *state);
>   	int (*set_active_discharge)(struct regulator_dev *, bool enable);
>   
>   	/* enable/disable regulator */
> 

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~