2023-03-08 09:40:44

by Svyatoslav Ryhel

[permalink] [raw]
Subject: [PATCH v1 0/2] Add optional power supply for INA2XX

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



2023-03-08 09:40:54

by Svyatoslav Ryhel

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

Some devices may need a specific supply provided
for this sensor to work properly, like p895 does.

Signed-off-by: Svyatoslav Ryhel <[email protected]>
---
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


2023-03-08 09:40:58

by Svyatoslav Ryhel

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

Add supply property.

Signed-off-by: Svyatoslav Ryhel <[email protected]>
---
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


2023-03-08 10:26:10

by Krzysztof Kozlowski

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

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 <[email protected]>
> ---
> 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


2023-03-08 10:27:50

by Krzysztof Kozlowski

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

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


2023-03-08 10:34:05

by Svyatoslav Ryhel

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

ср, 8 бер. 2023 р. о 12:27 Krzysztof Kozlowski
<[email protected]> пише:
>
> 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.

2023-03-08 10:36:41

by Svyatoslav Ryhel

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

ср, 8 бер. 2023 р. о 12:25 Krzysztof Kozlowski
<[email protected]> пише:
>
> 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 <[email protected]>
> > ---
> > 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
>

2023-03-08 10:37:07

by Krzysztof Kozlowski

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

On 08/03/2023 11:32, Svyatoslav Ryhel wrote:
> ср, 8 бер. 2023 р. о 12:27 Krzysztof Kozlowski
> <[email protected]> пише:
>>
>> 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


2023-03-08 10:37:48

by Krzysztof Kozlowski

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

On 08/03/2023 11:35, Svyatoslav Ryhel wrote:
> ср, 8 бер. 2023 р. о 12:25 Krzysztof Kozlowski
> <[email protected]> пише:
>>
>> 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 <[email protected]>
>>> ---
>>> 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


2023-03-08 11:13:28

by Guenter Roeck

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

On 3/8/23 02:35, Svyatoslav Ryhel wrote:
> ср, 8 бер. 2023 р. о 12:25 Krzysztof Kozlowski
> <[email protected]> пише:
>>
>> 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 <[email protected]>
>>> ---
>>> 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
>>


2023-03-08 11:20:03

by Svyatoslav Ryhel

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



8 березня 2023 р. 13:13:18 GMT+02:00, Guenter Roeck <[email protected]> написав(-ла):
>On 3/8/23 02:35, Svyatoslav Ryhel wrote:
>> ср, 8 бер. 2023 р. о 12:25 Krzysztof Kozlowski
>> <[email protected]> пише:
>>>
>>> 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 <[email protected]>
>>>> ---
>>>> 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
>>>
>

2023-03-08 11:36:37

by Guenter Roeck

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

On 3/8/23 02:32, Svyatoslav Ryhel wrote:
> ср, 8 бер. 2023 р. о 12:27 Krzysztof Kozlowski
> <[email protected]> пише:
>>
>> 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.


2023-03-08 11:55:21

by Svyatoslav Ryhel

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



8 березня 2023 р. 13:35:46 GMT+02:00, Guenter Roeck <[email protected]> написав(-ла):
>On 3/8/23 02:32, Svyatoslav Ryhel wrote:
>> ср, 8 бер. 2023 р. о 12:27 Krzysztof Kozlowski
>> <[email protected]> пише:
>>>
>>> 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.
>

2023-03-08 11:59:27

by Lars-Peter Clausen

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

On 3/8/23 03:19, Svyatoslav Ryhel wrote:
>
> 8 березня 2023 р. 13:13:18 GMT+02:00, Guenter Roeck <[email protected]> написав(-ла):
>> On 3/8/23 02:35, Svyatoslav Ryhel wrote:
>>> ср, 8 бер. 2023 р. о 12:25 Krzysztof Kozlowski
>>> <[email protected]> пише:
>>>> 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 <[email protected]>
>>>>> ---
>>>>> 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.



2023-03-08 12:54:53

by Mark Brown

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

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.


Attachments:
(No filename) (231.00 B)
signature.asc (488.00 B)
Download all attachments

2023-03-08 12:58:29

by Svyatoslav Ryhel

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



8 березня 2023 р. 14:54:34 GMT+02:00, Mark Brown <[email protected]> написав(-ла):
>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.

2023-03-08 13:16:46

by Krzysztof Kozlowski

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

On 08/03/2023 13:58, Svyatoslav Ryhel wrote:
>
>
> 8 березня 2023 р. 14:54:34 GMT+02:00, Mark Brown <[email protected]> написав(-ла):
>> 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


2023-03-08 13:47:35

by Mark Brown

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

On Wed, Mar 08, 2023 at 02:58:20PM +0200, Svyatoslav Ryhel wrote:
> 8 березня 2023 р. 14:54:34 GMT+02:00, Mark Brown <[email protected]> написав(-ла):
> >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.


Attachments:
(No filename) (809.00 B)
signature.asc (488.00 B)
Download all attachments

2023-03-08 14:04:46

by Svyatoslav Ryhel

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



8 березня 2023 р. 15:46:52 GMT+02:00, Mark Brown <[email protected]> написав(-ла):
>On Wed, Mar 08, 2023 at 02:58:20PM +0200, Svyatoslav Ryhel wrote:
>> 8 березня 2023 р. 14:54:34 GMT+02:00, Mark Brown <[email protected]> написав(-ла):
>> >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?

2023-03-08 14:38:07

by Krzysztof Kozlowski

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

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


2023-03-08 14:38:53

by Mark Brown

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

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

> >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.


Attachments:
(No filename) (858.00 B)
signature.asc (488.00 B)
Download all attachments