* [PATCH v1 0/2] Add optional power supply for INA2XX @ 2023-03-08 9:40 Svyatoslav Ryhel 2023-03-08 9:40 ` [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property Svyatoslav Ryhel 2023-03-08 9:40 ` [PATCH v1 2/2] hwmon: ina2xx: add optional regulator support Svyatoslav Ryhel 0 siblings, 2 replies; 21+ messages in thread From: Svyatoslav Ryhel @ 2023-03-08 9:40 UTC (permalink / raw) To: Guenter Roeck, Jean Delvare, Rob Herring, Krzysztof Kozlowski, Liam Girdwood, Mark Brown Cc: linux-hwmon, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property 2023-03-08 9:40 [PATCH v1 0/2] Add optional power supply for INA2XX Svyatoslav Ryhel @ 2023-03-08 9:40 ` Svyatoslav Ryhel 2023-03-08 10:27 ` Krzysztof Kozlowski 2023-03-08 12:54 ` Mark Brown 2023-03-08 9:40 ` [PATCH v1 2/2] hwmon: ina2xx: add optional regulator support Svyatoslav Ryhel 1 sibling, 2 replies; 21+ messages in thread From: Svyatoslav Ryhel @ 2023-03-08 9:40 UTC (permalink / raw) To: Guenter Roeck, Jean Delvare, Rob Herring, Krzysztof Kozlowski, Liam Girdwood, Mark Brown Cc: linux-hwmon, devicetree, linux-kernel 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property 2023-03-08 9:40 ` [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property Svyatoslav Ryhel @ 2023-03-08 10:27 ` Krzysztof Kozlowski 2023-03-08 10:32 ` Svyatoslav Ryhel 2023-03-08 12:54 ` Mark Brown 1 sibling, 1 reply; 21+ messages in thread From: Krzysztof Kozlowski @ 2023-03-08 10:27 UTC (permalink / raw) To: Svyatoslav Ryhel, Guenter Roeck, Jean Delvare, Rob Herring, Krzysztof Kozlowski, Liam Girdwood, Mark Brown Cc: linux-hwmon, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property 2023-03-08 10:27 ` Krzysztof Kozlowski @ 2023-03-08 10:32 ` Svyatoslav Ryhel 2023-03-08 10:36 ` Krzysztof Kozlowski 2023-03-08 11:35 ` Guenter Roeck 0 siblings, 2 replies; 21+ messages in thread From: Svyatoslav Ryhel @ 2023-03-08 10:32 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Guenter Roeck, Jean Delvare, Rob Herring, Krzysztof Kozlowski, Liam Girdwood, Mark Brown, linux-hwmon, devicetree, linux-kernel ср, 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property 2023-03-08 10:32 ` Svyatoslav Ryhel @ 2023-03-08 10:36 ` Krzysztof Kozlowski 2023-03-08 11:35 ` Guenter Roeck 1 sibling, 0 replies; 21+ messages in thread From: Krzysztof Kozlowski @ 2023-03-08 10:36 UTC (permalink / raw) To: Svyatoslav Ryhel Cc: Guenter Roeck, Jean Delvare, Rob Herring, Krzysztof Kozlowski, Liam Girdwood, Mark Brown, linux-hwmon, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property 2023-03-08 10:32 ` Svyatoslav Ryhel 2023-03-08 10:36 ` Krzysztof Kozlowski @ 2023-03-08 11:35 ` Guenter Roeck 2023-03-08 11:54 ` Svyatoslav Ryhel 1 sibling, 1 reply; 21+ messages in thread From: Guenter Roeck @ 2023-03-08 11:35 UTC (permalink / raw) To: Svyatoslav Ryhel, Krzysztof Kozlowski Cc: Jean Delvare, Rob Herring, Krzysztof Kozlowski, Liam Girdwood, Mark Brown, linux-hwmon, devicetree, linux-kernel 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property 2023-03-08 11:35 ` Guenter Roeck @ 2023-03-08 11:54 ` Svyatoslav Ryhel 0 siblings, 0 replies; 21+ messages in thread From: Svyatoslav Ryhel @ 2023-03-08 11:54 UTC (permalink / raw) To: Guenter Roeck, Krzysztof Kozlowski Cc: Jean Delvare, Rob Herring, Krzysztof Kozlowski, Liam Girdwood, Mark Brown, linux-hwmon, devicetree, linux-kernel 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. > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property 2023-03-08 9:40 ` [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property Svyatoslav Ryhel 2023-03-08 10:27 ` Krzysztof Kozlowski @ 2023-03-08 12:54 ` Mark Brown 2023-03-08 12:58 ` Svyatoslav Ryhel 1 sibling, 1 reply; 21+ messages in thread From: Mark Brown @ 2023-03-08 12:54 UTC (permalink / raw) To: Svyatoslav Ryhel Cc: Guenter Roeck, Jean Delvare, Rob Herring, Krzysztof Kozlowski, Liam Girdwood, linux-hwmon, devicetree, linux-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property 2023-03-08 12:54 ` Mark Brown @ 2023-03-08 12:58 ` Svyatoslav Ryhel 2023-03-08 13:13 ` Krzysztof Kozlowski 2023-03-08 13:46 ` Mark Brown 0 siblings, 2 replies; 21+ messages in thread From: Svyatoslav Ryhel @ 2023-03-08 12:58 UTC (permalink / raw) To: Mark Brown Cc: Guenter Roeck, Jean Delvare, Rob Herring, Krzysztof Kozlowski, Liam Girdwood, linux-hwmon, devicetree, linux-kernel 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property 2023-03-08 12:58 ` Svyatoslav Ryhel @ 2023-03-08 13:13 ` Krzysztof Kozlowski 2023-03-08 13:46 ` Mark Brown 1 sibling, 0 replies; 21+ messages in thread From: Krzysztof Kozlowski @ 2023-03-08 13:13 UTC (permalink / raw) To: Svyatoslav Ryhel, Mark Brown Cc: Guenter Roeck, Jean Delvare, Rob Herring, Krzysztof Kozlowski, Liam Girdwood, linux-hwmon, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property 2023-03-08 12:58 ` Svyatoslav Ryhel 2023-03-08 13:13 ` Krzysztof Kozlowski @ 2023-03-08 13:46 ` Mark Brown 2023-03-08 14:01 ` Svyatoslav Ryhel 1 sibling, 1 reply; 21+ messages in thread From: Mark Brown @ 2023-03-08 13:46 UTC (permalink / raw) To: Svyatoslav Ryhel Cc: Guenter Roeck, Jean Delvare, Rob Herring, Krzysztof Kozlowski, Liam Girdwood, linux-hwmon, devicetree, linux-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property 2023-03-08 13:46 ` Mark Brown @ 2023-03-08 14:01 ` Svyatoslav Ryhel 2023-03-08 14:37 ` Krzysztof Kozlowski 2023-03-08 14:38 ` Mark Brown 0 siblings, 2 replies; 21+ messages in thread From: Svyatoslav Ryhel @ 2023-03-08 14:01 UTC (permalink / raw) To: Mark Brown Cc: Guenter Roeck, Jean Delvare, Rob Herring, Krzysztof Kozlowski, Liam Girdwood, linux-hwmon, devicetree, linux-kernel 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? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property 2023-03-08 14:01 ` Svyatoslav Ryhel @ 2023-03-08 14:37 ` Krzysztof Kozlowski 2023-03-08 14:38 ` Mark Brown 1 sibling, 0 replies; 21+ messages in thread From: Krzysztof Kozlowski @ 2023-03-08 14:37 UTC (permalink / raw) To: Svyatoslav Ryhel, Mark Brown Cc: Guenter Roeck, Jean Delvare, Rob Herring, Krzysztof Kozlowski, Liam Girdwood, linux-hwmon, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property 2023-03-08 14:01 ` Svyatoslav Ryhel 2023-03-08 14:37 ` Krzysztof Kozlowski @ 2023-03-08 14:38 ` Mark Brown 1 sibling, 0 replies; 21+ messages in thread From: Mark Brown @ 2023-03-08 14:38 UTC (permalink / raw) To: Svyatoslav Ryhel Cc: Guenter Roeck, Jean Delvare, Rob Herring, Krzysztof Kozlowski, Liam Girdwood, linux-hwmon, devicetree, linux-kernel [-- 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 --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v1 2/2] hwmon: ina2xx: add optional regulator support 2023-03-08 9:40 [PATCH v1 0/2] Add optional power supply for INA2XX Svyatoslav Ryhel 2023-03-08 9:40 ` [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property Svyatoslav Ryhel @ 2023-03-08 9:40 ` Svyatoslav Ryhel 2023-03-08 10:25 ` Krzysztof Kozlowski 1 sibling, 1 reply; 21+ messages in thread From: Svyatoslav Ryhel @ 2023-03-08 9:40 UTC (permalink / raw) To: Guenter Roeck, Jean Delvare, Rob Herring, Krzysztof Kozlowski, Liam Girdwood, Mark Brown Cc: linux-hwmon, devicetree, linux-kernel 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v1 2/2] hwmon: ina2xx: add optional regulator support 2023-03-08 9:40 ` [PATCH v1 2/2] hwmon: ina2xx: add optional regulator support Svyatoslav Ryhel @ 2023-03-08 10:25 ` Krzysztof Kozlowski 2023-03-08 10:35 ` Svyatoslav Ryhel 0 siblings, 1 reply; 21+ messages in thread From: Krzysztof Kozlowski @ 2023-03-08 10:25 UTC (permalink / raw) To: Svyatoslav Ryhel, Guenter Roeck, Jean Delvare, Rob Herring, Krzysztof Kozlowski, Liam Girdwood, Mark Brown Cc: linux-hwmon, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 2/2] hwmon: ina2xx: add optional regulator support 2023-03-08 10:25 ` Krzysztof Kozlowski @ 2023-03-08 10:35 ` Svyatoslav Ryhel 2023-03-08 10:37 ` Krzysztof Kozlowski 2023-03-08 11:13 ` Guenter Roeck 0 siblings, 2 replies; 21+ messages in thread From: Svyatoslav Ryhel @ 2023-03-08 10:35 UTC (permalink / raw) To: Krzysztof Kozlowski Cc: Guenter Roeck, Jean Delvare, Rob Herring, Krzysztof Kozlowski, Liam Girdwood, Mark Brown, linux-hwmon, devicetree, linux-kernel ср, 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 > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 2/2] hwmon: ina2xx: add optional regulator support 2023-03-08 10:35 ` Svyatoslav Ryhel @ 2023-03-08 10:37 ` Krzysztof Kozlowski 2023-03-08 11:13 ` Guenter Roeck 1 sibling, 0 replies; 21+ messages in thread From: Krzysztof Kozlowski @ 2023-03-08 10:37 UTC (permalink / raw) To: Svyatoslav Ryhel Cc: Guenter Roeck, Jean Delvare, Rob Herring, Krzysztof Kozlowski, Liam Girdwood, Mark Brown, linux-hwmon, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 2/2] hwmon: ina2xx: add optional regulator support 2023-03-08 10:35 ` Svyatoslav Ryhel 2023-03-08 10:37 ` Krzysztof Kozlowski @ 2023-03-08 11:13 ` Guenter Roeck 2023-03-08 11:19 ` Svyatoslav Ryhel 1 sibling, 1 reply; 21+ messages in thread From: Guenter Roeck @ 2023-03-08 11:13 UTC (permalink / raw) To: Svyatoslav Ryhel, Krzysztof Kozlowski Cc: Jean Delvare, Rob Herring, Krzysztof Kozlowski, Liam Girdwood, Mark Brown, linux-hwmon, devicetree, linux-kernel 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 >> ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 2/2] hwmon: ina2xx: add optional regulator support 2023-03-08 11:13 ` Guenter Roeck @ 2023-03-08 11:19 ` Svyatoslav Ryhel 2023-03-08 11:32 ` Lars-Peter Clausen 0 siblings, 1 reply; 21+ messages in thread From: Svyatoslav Ryhel @ 2023-03-08 11:19 UTC (permalink / raw) To: Guenter Roeck, Krzysztof Kozlowski Cc: Jean Delvare, Rob Herring, Krzysztof Kozlowski, Liam Girdwood, Mark Brown, linux-hwmon, devicetree, linux-kernel 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 >>> > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v1 2/2] hwmon: ina2xx: add optional regulator support 2023-03-08 11:19 ` Svyatoslav Ryhel @ 2023-03-08 11:32 ` Lars-Peter Clausen 0 siblings, 0 replies; 21+ messages in thread From: Lars-Peter Clausen @ 2023-03-08 11:32 UTC (permalink / raw) To: Svyatoslav Ryhel, Guenter Roeck, Krzysztof Kozlowski Cc: Jean Delvare, Rob Herring, Krzysztof Kozlowski, Liam Girdwood, Mark Brown, linux-hwmon, devicetree, linux-kernel 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2023-03-08 14:38 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-03-08 9:40 [PATCH v1 0/2] Add optional power supply for INA2XX Svyatoslav Ryhel 2023-03-08 9:40 ` [PATCH v1 1/2] dt-bindings: hwmon: ina2xx: add supply property Svyatoslav Ryhel 2023-03-08 10:27 ` Krzysztof Kozlowski 2023-03-08 10:32 ` Svyatoslav Ryhel 2023-03-08 10:36 ` Krzysztof Kozlowski 2023-03-08 11:35 ` Guenter Roeck 2023-03-08 11:54 ` Svyatoslav Ryhel 2023-03-08 12:54 ` Mark Brown 2023-03-08 12:58 ` Svyatoslav Ryhel 2023-03-08 13:13 ` Krzysztof Kozlowski 2023-03-08 13:46 ` Mark Brown 2023-03-08 14:01 ` Svyatoslav Ryhel 2023-03-08 14:37 ` Krzysztof Kozlowski 2023-03-08 14:38 ` Mark Brown 2023-03-08 9:40 ` [PATCH v1 2/2] hwmon: ina2xx: add optional regulator support Svyatoslav Ryhel 2023-03-08 10:25 ` Krzysztof Kozlowski 2023-03-08 10:35 ` Svyatoslav Ryhel 2023-03-08 10:37 ` Krzysztof Kozlowski 2023-03-08 11:13 ` Guenter Roeck 2023-03-08 11:19 ` Svyatoslav Ryhel 2023-03-08 11:32 ` Lars-Peter Clausen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).