Re: [PATCH v1 2/2] hwmon: ina2xx: add optional regulator support

From: Lars-Peter Clausen
Date: Wed Mar 08 2023 - 06:59:27 EST


On 3/8/23 03:19, Svyatoslav Ryhel wrote:

8 березня 2023 р. 13:13:18 GMT+02:00, Guenter Roeck <linux@xxxxxxxxxxxx> написав(-ла):
On 3/8/23 02:35, Svyatoslav Ryhel wrote:
ср, 8 бер. 2023 р. о 12:25 Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxx> пише:
On 08/03/2023 10:40, Svyatoslav Ryhel wrote:
Some devices may need a specific supply provided
for this sensor to work properly, like p895 does.

Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
---
drivers/hwmon/ina2xx.c | 12 ++++++++++++
1 file changed, 12 insertions(+)

diff --git a/drivers/hwmon/ina2xx.c b/drivers/hwmon/ina2xx.c
index 00fc70305a89..4a3e2b1bbe8b 100644
--- a/drivers/hwmon/ina2xx.c
+++ b/drivers/hwmon/ina2xx.c
@@ -119,6 +119,7 @@ struct ina2xx_data {
long power_lsb_uW;
struct mutex config_lock;
struct regmap *regmap;
+ struct regulator *vdd_supply;

const struct attribute_group *groups[INA2XX_MAX_ATTRIBUTE_GROUPS];
};
@@ -656,6 +657,17 @@ static int ina2xx_probe(struct i2c_client *client)
return PTR_ERR(data->regmap);
}

+ data->vdd_supply = devm_regulator_get_optional(dev, "vdd");
+ if (IS_ERR(data->vdd_supply))
+ return dev_err_probe(dev, PTR_ERR(data->vdd_supply),
+ "failed to get vdd regulator\n");
+
+ ret = regulator_enable(data->vdd_supply);
+ if (ret < 0) {
+ dev_err(dev, "failed to enable vdd power supply\n");
+ return ret;
And where is disable? On each error path, removal etc.

error path ok, should I create a remove function just to disable the regulator?

Use devm_add_action_or_reset().

Guenter

That is good suggestion. Thanks!

There is a new function devm_regulator_get_enable(). It will both request the regulator and enable in and then automatically disable and free it when the device is removed.

The API can be slightly confusing in this regard. In your case you also want to use the non-optional API. The optional API is for cases where you need to know whether a regulator is connected or not. If you do not need to know use the non-optional API, if no regulator is specified the regulator framework will just a return a noop regulator as a placeholder. The optional version will actually return an error if no regulator is specified, so with your patch existing users of this driver will break.