[PATCH v1 0/2] Add optional power supply for INA2XX

[PATCH v1 0/2] Add optional power supply for INA2XX

[Svyatoslav Ryhel]

On some devices (like LG Optimus Vu) ina2xx hwmon may require
power supply for correct work, hance add one.

Svyatoslav Ryhel (2):
  dt-bindings: hwmon: ina2xx: add supply property
  hwmon: ina2xx: add optional regulator support

 .../devicetree/bindings/hwmon/ti,ina2xx.yaml         |  4 ++++
 drivers/hwmon/ina2xx.c                               | 12 ++++++++++++
 2 files changed, 16 insertions(+)

-- 
2.37.2

[PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property

[Svyatoslav Ryhel]

Add supply property.

Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
---
 Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
index 47af97bb4ced..0f494131fa05 100644
--- a/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
+++ b/Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml
@@ -57,6 +57,8 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     enum: [1, 2, 4, 8]
 
+  vdd-supply: true
+
 required:
   - compatible
   - reg
@@ -73,5 +75,7 @@ examples:
             compatible = "ti,ina220";
             reg = <0x44>;
             shunt-resistor = <1000>;
+
+            vdd-supply = <&vdd_3v0_sen>;
         };
     };
-- 
2.37.2

Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property

[Krzysztof Kozlowski]

On 08/03/2023 10:40, Svyatoslav Ryhel wrote:
> Add supply property.

You have entire commit msg to explain and give background, but instead
there is just sentence duplicating subject. And since you did not
explain anything, we have questions... like: INA238 does not have VDD,
so this does not look correct.


Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property

[Svyatoslav Ryhel]

ср, 8 бер. 2023 р. о 12:27 Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> пише:
>
> On 08/03/2023 10:40, Svyatoslav Ryhel wrote:
> > Add supply property.
>
> You have entire commit msg to explain and give background, but instead
> there is just sentence duplicating subject. And since you did not
> explain anything, we have questions... like: INA238 does not have VDD,
> so this does not look correct.
>

This is why a regulator is not mandatory. If ina238 does not have vdd
then one who needs ina238 may omit this prop. How about looking from
this perspective?

>
> Best regards,
> Krzysztof
>

Best regards,
Svyatoslav R.

Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property

[Krzysztof Kozlowski]

On 08/03/2023 11:32, Svyatoslav Ryhel wrote:
> ср, 8 бер. 2023 р. о 12:27 Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> пише:
>>
>> On 08/03/2023 10:40, Svyatoslav Ryhel wrote:
>>> Add supply property.
>>
>> You have entire commit msg to explain and give background, but instead
>> there is just sentence duplicating subject. And since you did not
>> explain anything, we have questions... like: INA238 does not have VDD,
>> so this does not look correct.
>>
> 
> This is why a regulator is not mandatory. If ina238 does not have vdd
> then one who needs ina238 may omit this prop. How about looking from
> this perspective?

Not required means "optional", not "not existing" or "totally invalid".

Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property

[Guenter Roeck]

On 3/8/23 02:32, Svyatoslav Ryhel wrote:
> ср, 8 бер. 2023 р. о 12:27 Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> пише:
>>
>> On 08/03/2023 10:40, Svyatoslav Ryhel wrote:
>>> Add supply property.
>>
>> You have entire commit msg to explain and give background, but instead
>> there is just sentence duplicating subject. And since you did not
>> explain anything, we have questions... like: INA238 does not have VDD,
>> so this does not look correct.
>>
> 
> This is why a regulator is not mandatory. If ina238 does not have vdd
> then one who needs ina238 may omit this prop. How about looking from
> this perspective?
> 

If a chip does not have VDD, the driver should not even try to get it
for that chip. INS238 is supported by a different driver, so that does
not require special code, but it needs to be spelled out in the devicetree
bindings. Devicetree has a means to specify if a property is valid for
a given device.

Having said this, as it turns out, _none_ of the chips supported by
the ina2xx and the ina238 drivers have VDD. All of them have Vs, and
all but INA219 have Vbus. The bindings don't even explain which one
of those is supposed to refer to "VDD".

Also, regulator_get_optional() returns -ENODEV if CONFIG_REGULATOR
is not enabled, so it isn't entirely optional. We can not suddenly fail
to load the driver on systems with CONFIG_REGULATOR=n, so some conditional
code will unfortunately be necessary.

Guenter

>>
>> Best regards,
>> Krzysztof
>>
> 
> Best regards,
> Svyatoslav R.

Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property

[Svyatoslav Ryhel]

8 березня 2023 р. 13:35:46 GMT+02:00, Guenter Roeck <linux@roeck-us.net> написав(-ла):
>On 3/8/23 02:32, Svyatoslav Ryhel wrote:
>> ср, 8 бер. 2023 р. о 12:27 Krzysztof Kozlowski
>> <krzysztof.kozlowski@linaro.org> пише:
>>> 
>>> On 08/03/2023 10:40, Svyatoslav Ryhel wrote:
>>>> Add supply property.
>>> 
>>> You have entire commit msg to explain and give background, but instead
>>> there is just sentence duplicating subject. And since you did not
>>> explain anything, we have questions... like: INA238 does not have VDD,
>>> so this does not look correct.
>>> 
>> 
>> This is why a regulator is not mandatory. If ina238 does not have vdd
>> then one who needs ina238 may omit this prop. How about looking from
>> this perspective?
>> 
>
>If a chip does not have VDD, the driver should not even try to get it
>for that chip. INS238 is supported by a different driver, so that does
>not require special code, but it needs to be spelled out in the devicetree
>bindings. Devicetree has a means to specify if a property is valid for
>a given device.
>
>Having said this, as it turns out, _none_ of the chips supported by
>the ina2xx and the ina238 drivers have VDD. All of them have Vs, and
>all but INA219 have Vbus. The bindings don't even explain which one
>of those is supposed to refer to "VDD".
>

So you refer to vdd as to a real name. Now I understand this misunderstand. Regulator I am interested in is Vs. Since you confirmed that Vs is supported by all ina2xx there should be no contraversions further.

>Also, regulator_get_optional() returns -ENODEV if CONFIG_REGULATOR
>is not enabled, so it isn't entirely optional. We can not suddenly fail
>to load the driver on systems with CONFIG_REGULATOR=n, so some conditional
>code will unfortunately be necessary.
>
>Guenter
>

Hm, then Lars-Peter Clausen suggestion should be applicable in this case.

>>> 
>>> Best regards,
>>> Krzysztof
>>> 
>> 
>> Best regards,
>> Svyatoslav R.
>

Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 231 bytes --]

On Wed, Mar 08, 2023 at 11:40:23AM +0200, Svyatoslav Ryhel wrote:
> Add supply property.

> +  vdd-supply: true
> +
>  required:
>    - compatible
>    - reg

Unless the device can work without power the supply should be required.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property

[Svyatoslav Ryhel]

8 березня 2023 р. 14:54:34 GMT+02:00, Mark Brown <broonie@kernel.org> написав(-ла):
>On Wed, Mar 08, 2023 at 11:40:23AM +0200, Svyatoslav Ryhel wrote:
>> Add supply property.
>
>> +  vdd-supply: true
>> +
>>  required:
>>    - compatible
>>    - reg
>
>Unless the device can work without power the supply should be required.

Device can work without supply defined on most devices, but in my case power is gated with gpio and devices will not work without fixed regulator.

Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property

[Krzysztof Kozlowski]

On 08/03/2023 13:58, Svyatoslav Ryhel wrote:
> 
> 
> 8 березня 2023 р. 14:54:34 GMT+02:00, Mark Brown <broonie@kernel.org> написав(-ла):
>> On Wed, Mar 08, 2023 at 11:40:23AM +0200, Svyatoslav Ryhel wrote:
>>> Add supply property.
>>
>>> +  vdd-supply: true
>>> +
>>>  required:
>>>    - compatible
>>>    - reg
>>
>> Unless the device can work without power the supply should be required.
> 
> Device can work without supply defined on most devices,

Are you sure they can work without any power? INA231 does not say VS
supply is optional. Datasheet says:
"The INA231 is typically powered by a separate supply that can range
from 2.7 V to 5.5 V."

Although it uses word "typically" which could suggest other design, but
are you sure it can work without it? From where it gets the power?


>  but in my case power is gated with gpio and devices will not work
without fixed regulator.

BTW, wrap your lines to match mailing list style.

Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 828 bytes --]

On Wed, Mar 08, 2023 at 02:58:20PM +0200, Svyatoslav Ryhel wrote:
> 8 березня 2023 р. 14:54:34 GMT+02:00, Mark Brown <broonie@kernel.org> написав(-ла):
> >On Wed, Mar 08, 2023 at 11:40:23AM +0200, Svyatoslav Ryhel wrote:
> >> Add supply property.

> >> +  vdd-supply: true
> >> +
> >>  required:
> >>    - compatible
> >>    - reg

> >Unless the device can work without power the supply should be required.

> Device can work without supply defined on most devices, but in my case power is gated with gpio and devices will not work without fixed regulator.

If there are devices that work without any source of power at all that
would be very surprising.  It doesn't matter if a particular system has
a non-controllable regulator, the binding should still make it mandatory
to describe that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property

[Svyatoslav Ryhel]

8 березня 2023 р. 15:46:52 GMT+02:00, Mark Brown <broonie@kernel.org> написав(-ла):
>On Wed, Mar 08, 2023 at 02:58:20PM +0200, Svyatoslav Ryhel wrote:
>> 8 березня 2023 р. 14:54:34 GMT+02:00, Mark Brown <broonie@kernel.org> написав(-ла):
>> >On Wed, Mar 08, 2023 at 11:40:23AM +0200, Svyatoslav Ryhel wrote:
>> >> Add supply property.
>
>> >> +  vdd-supply: true
>> >> +
>> >>  required:
>> >>    - compatible
>> >>    - reg
>
>> >Unless the device can work without power the supply should be required.
>
>> Device can work without supply defined on most devices, but in my case power is gated with gpio and devices will not work without fixed regulator.
>
>If there are devices that work without any source of power at all that
>would be very surprising.  It doesn't matter if a particular system has
>a non-controllable regulator, the binding should still make it mandatory
>to describe that.

Then question is WHY and WHO passed driver without power supply system implemented? Why it pops only now?

Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property

[Krzysztof Kozlowski]

On 08/03/2023 15:01, Svyatoslav Ryhel wrote:
>>>>> +  vdd-supply: true
>>>>> +
>>>>>  required:
>>>>>    - compatible
>>>>>    - reg
>>
>>>> Unless the device can work without power the supply should be required.
>>
>>> Device can work without supply defined on most devices, but in my case power is gated with gpio and devices will not work without fixed regulator.
>>
>> If there are devices that work without any source of power at all that
>> would be very surprising.  It doesn't matter if a particular system has
>> a non-controllable regulator, the binding should still make it mandatory
>> to describe that.
> 
> Then question is WHY and WHO passed driver without power supply system implemented? Why it pops only now?

Why do you mention driver? We talk about bindings and device. What the
driver does, matters less here.

Best regards,
Krzysztof

Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

On Wed, Mar 08, 2023 at 04:01:44PM +0200, Svyatoslav Ryhel wrote:
> 8 березня 2023 р. 15:46:52 GMT+02:00, Mark Brown <broonie@kernel.org> написав(-ла):

> >If there are devices that work without any source of power at all that
> >would be very surprising.  It doesn't matter if a particular system has
> >a non-controllable regulator, the binding should still make it mandatory
> >to describe that.

> Then question is WHY and WHO passed driver without power supply system implemented? Why it pops only now?

You are defining a supply property.  When you define a supply property
that supply property should be mandatory if it's physically mandatory
for the device.

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Svyatoslav Ryhel]

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

[Krzysztof Kozlowski]

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

[Svyatoslav Ryhel]

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

[Krzysztof Kozlowski]

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

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

[Guenter Roeck]

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

[Svyatoslav Ryhel]

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

[Lars-Peter Clausen]

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.