| Message ID | 20230308094024.14115-2-clamor95@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add optional power supply for INA2XX | expand |
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
ср, 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.
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
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.
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. >
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.
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.
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
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.
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?
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
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.
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>; }; };
Add supply property. Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com> --- Documentation/devicetree/bindings/hwmon/ti,ina2xx.yaml | 4 ++++ 1 file changed, 4 insertions(+)