| Message ID | 20220111175017.223966-4-krzysztof.kozlowski@canonical.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | leds/power/regulator/mfd: dt-bindings: maxim,max77693: convert to dtschema | expand |
On Tue, 11 Jan 2022 18:50:16 +0100, Krzysztof Kozlowski wrote: > Convert the regulator bindings of Maxim MAX77693 MUIC to DT schema format. > The existing bindings were defined in ../bindings/mfd/max77693.txt. > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> > --- > .../bindings/regulator/maxim,max77693.yaml | 60 +++++++++++++++++++ > 1 file changed, 60 insertions(+) > create mode 100644 Documentation/devicetree/bindings/regulator/maxim,max77693.yaml > Reviewed-by: Rob Herring <robh@kernel.org>
On Tue, Jan 11, 2022 at 06:50:16PM +0100, Krzysztof Kozlowski wrote: > + properties: > + regulator-name: true > + regulator-always-on: true > + regulator-boot-on: true Why are these specific generic regulator properties enumerated?
On 14/02/2022 17:41, Mark Brown wrote: > On Tue, Jan 11, 2022 at 06:50:16PM +0100, Krzysztof Kozlowski wrote: > >> + properties: >> + regulator-name: true >> + regulator-always-on: true >> + regulator-boot-on: true > > Why are these specific generic regulator properties enumerated? additionalProperties=false is used, so all properties, also ones from regulator.yaml, have to be mentioned. Why this approach was used? Because the hardware here is very limited, so no other properties are expected. No other features are supported. Best regards, Krzysztof
On Mon, Feb 14, 2022 at 05:45:40PM +0100, Krzysztof Kozlowski wrote: > On 14/02/2022 17:41, Mark Brown wrote: > >> + properties: > >> + regulator-name: true > >> + regulator-always-on: true > >> + regulator-boot-on: true > > Why are these specific generic regulator properties enumerated? > additionalProperties=false is used, so all properties, also ones from > regulator.yaml, have to be mentioned. > Why this approach was used? Because the hardware here is very limited, > so no other properties are expected. No other features are supported. That's not going to scale well if we add any new features in the core, and doesn't include things like coupling which could be applied to any regulator.
On 14/02/2022 17:51, Mark Brown wrote: > On Mon, Feb 14, 2022 at 05:45:40PM +0100, Krzysztof Kozlowski wrote: >> On 14/02/2022 17:41, Mark Brown wrote: >>>> + properties: >>>> + regulator-name: true >>>> + regulator-always-on: true >>>> + regulator-boot-on: true > >>> Why are these specific generic regulator properties enumerated? > >> additionalProperties=false is used, so all properties, also ones from >> regulator.yaml, have to be mentioned. > >> Why this approach was used? Because the hardware here is very limited, >> so no other properties are expected. No other features are supported. > > That's not going to scale well if we add any new features in the core, > and doesn't include things like coupling which could be applied to any > regulator. Relaxing constraints - removing required nodes and using uneavluatedProperties=false - is still possible. Unlike the other way if we ever wanted to restrict too flexible bindings. You mantioned new features - this approach does not change that. If you add new properties to common schema, you already alter bindings. Just because we use common part, it does not change the fact that it is a bindings change. Adding new features in common schema is the same binding change as adding new feature in the specific binding, except more work. I guess you though that work in scaling, so yes, this scales worse. The benefit is that this really restricts usage of regulator to what is supported, so allows to detect wrongly configured DTS. Once coupling (or any other feature) is supported, each of such restricted regulator bindings should be independently revised, instead of adding this new feature to everything. Best regards, Krzysztof
On Mon, Feb 14, 2022 at 06:01:17PM +0100, Krzysztof Kozlowski wrote: > You mantioned new features - this approach does not change that. If you > add new properties to common schema, you already alter bindings. Just > because we use common part, it does not change the fact that it is a > bindings change. Adding new features in common schema is the same > binding change as adding new feature in the specific binding, except > more work. > I guess you though that work in scaling, so yes, this scales worse. The > benefit is that this really restricts usage of regulator to what is > supported, so allows to detect wrongly configured DTS. We should have a way of specifying generic properties that doesn't require us to go through every single user of a binding and updating them all, then auditing by hand any new users to make sure they didn't forget one of the generic properties. This is just error prone and miserable, especially when most of the checking is done by hand rather than automated. > Once coupling (or any other feature) is supported, each of such > restricted regulator bindings should be independently revised, instead > of adding this new feature to everything. Coupling is already supported - it doesn't require anything on the part of the driver, it's about defining the relationships between supplies rather than anything the driver or device does.
On 14/02/2022 18:07, Mark Brown wrote: > On Mon, Feb 14, 2022 at 06:01:17PM +0100, Krzysztof Kozlowski wrote: > >> You mantioned new features - this approach does not change that. If you >> add new properties to common schema, you already alter bindings. Just >> because we use common part, it does not change the fact that it is a >> bindings change. Adding new features in common schema is the same >> binding change as adding new feature in the specific binding, except >> more work. > >> I guess you though that work in scaling, so yes, this scales worse. The >> benefit is that this really restricts usage of regulator to what is >> supported, so allows to detect wrongly configured DTS. > > We should have a way of specifying generic properties that doesn't > require us to go through every single user of a binding and updating > them all, then auditing by hand any new users to make sure they didn't > forget one of the generic properties. This is just error prone and > miserable, especially when most of the checking is done by hand rather > than automated. I see. The hardware really does not support most of core regulator features, so if we switch to your proposal (unevaluatedProperties:false), the DTS could contain something which is good from the core regulator point of view, but does not fit at all this hardware. A disallow/deny-list could solve it... but it also does not scale. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/regulator/maxim,max77693.yaml b/Documentation/devicetree/bindings/regulator/maxim,max77693.yaml new file mode 100644 index 000000000000..20d8559bdc2b --- /dev/null +++ b/Documentation/devicetree/bindings/regulator/maxim,max77693.yaml @@ -0,0 +1,60 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/regulator/maxim,max77693.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Maxim MAX77693 MicroUSB and Companion Power Management IC regulators + +maintainers: + - Chanwoo Choi <cw00.choi@samsung.com> + - Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> + +description: | + This is a part of device tree bindings for Maxim MAX77693 MicroUSB Integrated + Circuit (MUIC). + + See also Documentation/devicetree/bindings/mfd/maxim,max77693.yaml for + additional information and example. + +properties: + CHARGER: + type: object + $ref: regulator.yaml# + additionalProperties: false + description: | + Current regulator. + + properties: + regulator-name: true + regulator-always-on: true + regulator-boot-on: true + regulator-min-microamp: + minimum: 60000 + regulator-max-microamp: + maximum: 2580000 + + required: + - regulator-name + +patternProperties: + "^ESAFEOUT[12]$": + type: object + $ref: regulator.yaml# + additionalProperties: false + description: | + Safeout LDO regulator. + + properties: + regulator-name: true + regulator-always-on: true + regulator-boot-on: true + regulator-min-microvolt: + minimum: 3300000 + regulator-max-microvolt: + maximum: 4950000 + + required: + - regulator-name + +additionalProperties: false
Convert the regulator bindings of Maxim MAX77693 MUIC to DT schema format. The existing bindings were defined in ../bindings/mfd/max77693.txt. Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> --- .../bindings/regulator/maxim,max77693.yaml | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 Documentation/devicetree/bindings/regulator/maxim,max77693.yaml