Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4a71b08e-0c0d-a378-7a3a-0cd12912d4d4@gmail.com>
Date:   Mon, 26 Jun 2023 16:43:49 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Content-Language: en-US, en-GB
To:     Benjamin Bara <bbara93@gmail.com>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>
Cc:     support.opensource@diasemi.com,
        DLG-Adam.Ward.opensource@dm.renesas.com,
        Martin Fuzzey <martin.fuzzey@flowbird.group>,
        linux-kernel@vger.kernel.org,
        Benjamin Bara <benjamin.bara@skidata.com>
References: <20230419-dynamic-vmon-v4-0-4d3734e62ada@skidata.com>
 <20230419-dynamic-vmon-v4-2-4d3734e62ada@skidata.com>
From:   Matti Vaittinen <mazziesaccount@gmail.com>
Subject: Re: [PATCH RFC v4 02/13] regulator: add getter for active monitors
In-Reply-To: <20230419-dynamic-vmon-v4-2-4d3734e62ada@skidata.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 6/20/23 23:02, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.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@skidata.com>

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

Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.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! ~~