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

[Lars-Peter Clausen <lars@metafoo.de>]

On 3/8/23 03:19, Svyatoslav Ryhel wrote:
>
> 8 березня 2023 р. 13:13:18 GMT+02:00, Guenter Roeck <linux@roeck-us.net> написав(-ла):
>> On 3/8/23 02:35, Svyatoslav Ryhel wrote:
>>> ср, 8 бер. 2023 р. о 12:25 Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> пише:
>>>> 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@gmail.com>
>>>>> ---
>>>>>    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.