Add optional power supply for INA2XX

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

Posted by Svyatoslav Ryhel 2 years, 6 months ago

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;
+	}
+
 	ret = ina2xx_init(data);
 	if (ret < 0) {
 		dev_err(dev, "error configuring the device: %d\n", ret);
-- 
2.37.2

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

Posted by Krzysztof Kozlowski 2 years, 6 months ago

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.

Best regards,
Krzysztof

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

Posted by Svyatoslav Ryhel 2 years, 6 months ago

ср, 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?

> Best regards,
> Krzysztof
>

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

Posted by Guenter Roeck 2 years, 6 months ago

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

>> Best regards,
>> Krzysztof
>>

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

Posted by Svyatoslav Ryhel 2 years, 6 months ago


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!

Best regards,
Svyatoslav R.

>>> Best regards,
>>> Krzysztof
>>> 
>

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

Posted by Lars-Peter Clausen 2 years, 6 months ago

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.

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

Posted by Krzysztof Kozlowski 2 years, 6 months ago

On 08/03/2023 11: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?

Unbind device. Then bind. Then unbind. Then check regulator status
(/sys/kernel/debug). Do you have the answer now?

Best regards,
Krzysztof