| Message ID | 20220401202643.877609-7-caleb.connolly@linaro.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | power: supply: introduce the Qualcomm smb2 | expand |
Hi, On Fri, Apr 01, 2022 at 09:26:43PM +0100, Caleb Connolly wrote: > Add devicetree bindings for the Qualcomm PMI8998/PM660 SMB2 charger > drivers. > > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > --- > .../bindings/power/supply/qcom,smb2.yaml | 68 +++++++++++++++++++ > 1 file changed, 68 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml > > diff --git a/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml > new file mode 100644 > index 000000000000..1bea1fef78b8 > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml > @@ -0,0 +1,68 @@ > +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/power/supply/qcom,smb2.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm PMI8998/PM660 Switch-Mode Battery Charger "2" > + > +maintainers: > + - Caleb Connolly <caleb.connolly@linaro.org> > + > +properties: > + compatible: > + enum: > + - qcom,pmi8998-smb2 > + > + reg: > + maxItems: 1 > + > + interrupts: > + items: > + - description: usb plugin > + > + interrupt-names: > + items: > + - const: usb-plugin > + > + io-channels: > + items: > + - description: USB in current in uA > + - description: USB in voltage in uV > + > + io-channel-names: > + items: > + - const: usbin_i > + - const: usbin_v Is there a good reason to use usbin_ instead of usb_in_? -- Sebastian > + > +required: > + - compatible > + - reg > + - interrupts > + - interrupt-names > + - io-channels > + - io-channel-names > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + pmic { > + #address-cells = <1>; > + #size-cells = <0>; > + #interrupt-cells = <4>; > + > + smb2@1000 { > + compatible = "qcom,pmi8998-smb2"; > + reg = <0x1000>; > + > + interrupts = <0x2 0x13 0x4 IRQ_TYPE_EDGE_BOTH>; > + interrupt-names = "usb-plugin"; > + > + io-channels = <&pmi8998_rradc 3>, > + <&pmi8998_rradc 4>; > + io-channel-names = "usbin_i", > + "usbin_v"; > + }; > + }; > -- > 2.35.1 >
On 01/04/2022 22:26, Caleb Connolly wrote: > Add devicetree bindings for the Qualcomm PMI8998/PM660 SMB2 charger > drivers. > > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > --- > .../bindings/power/supply/qcom,smb2.yaml | 68 +++++++++++++++++++ > 1 file changed, 68 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml > > diff --git a/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml > new file mode 100644 > index 000000000000..1bea1fef78b8 > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml > @@ -0,0 +1,68 @@ > +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/power/supply/qcom,smb2.yaml# Hi, Are you sure "smb2" is a real Qualcomm versioning? IOW, is there going to be smb3 in the future? If not, better to just name the file according to model, so like compatible and like other existing schemas from Qualcomm. > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm PMI8998/PM660 Switch-Mode Battery Charger "2" > + > +maintainers: > + - Caleb Connolly <caleb.connolly@linaro.org> > + > +properties: > + compatible: > + enum: > + - qcom,pmi8998-smb2 > + > + reg: > + maxItems: 1 > + > + interrupts: > + items: > + - description: usb plugin Just maxItems:1 (description is obvious and matches names). > + > + interrupt-names: > + items: > + - const: usb-plugin > + > + io-channels: > + items: > + - description: USB in current in uA > + - description: USB in voltage in uV > + > + io-channel-names: > + items: > + - const: usbin_i > + - const: usbin_v > + What about monitored-battery? How do you configure the battery characteristics? > +required: > + - compatible > + - reg > + - interrupts > + - interrupt-names > + - io-channels > + - io-channel-names > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + pmic { > + #address-cells = <1>; > + #size-cells = <0>; > + #interrupt-cells = <4>; > + > + smb2@1000 { Generic node name please, so "charger". Best regards, Krzysztof
On 02/04/2022 00:03, Sebastian Reichel wrote: > Hi, > > On Fri, Apr 01, 2022 at 09:26:43PM +0100, Caleb Connolly wrote: >> Add devicetree bindings for the Qualcomm PMI8998/PM660 SMB2 charger >> drivers. >> >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> >> --- >> .../bindings/power/supply/qcom,smb2.yaml | 68 +++++++++++++++++++ >> 1 file changed, 68 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml >> >> diff --git a/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml >> new file mode 100644 >> index 000000000000..1bea1fef78b8 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml >> @@ -0,0 +1,68 @@ >> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/power/supply/qcom,smb2.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Qualcomm PMI8998/PM660 Switch-Mode Battery Charger "2" >> + >> +maintainers: >> + - Caleb Connolly <caleb.connolly@linaro.org> >> + >> +properties: >> + compatible: >> + enum: >> + - qcom,pmi8998-smb2 >> + >> + reg: >> + maxItems: 1 >> + >> + interrupts: >> + items: >> + - description: usb plugin >> + >> + interrupt-names: >> + items: >> + - const: usb-plugin >> + >> + io-channels: >> + items: >> + - description: USB in current in uA >> + - description: USB in voltage in uV >> + >> + io-channel-names: >> + items: >> + - const: usbin_i >> + - const: usbin_v > > Is there a good reason to use usbin_ instead of usb_in_? These are the channel names exposed by the RRADC driver, so they would have to be changed there. > > -- Sebastian > >> + >> +required: >> + - compatible >> + - reg >> + - interrupts >> + - interrupt-names >> + - io-channels >> + - io-channel-names >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/interrupt-controller/irq.h> >> + pmic { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + #interrupt-cells = <4>; >> + >> + smb2@1000 { >> + compatible = "qcom,pmi8998-smb2"; >> + reg = <0x1000>; >> + >> + interrupts = <0x2 0x13 0x4 IRQ_TYPE_EDGE_BOTH>; >> + interrupt-names = "usb-plugin"; >> + >> + io-channels = <&pmi8998_rradc 3>, >> + <&pmi8998_rradc 4>; >> + io-channel-names = "usbin_i", >> + "usbin_v"; >> + }; >> + }; >> -- >> 2.35.1 >>
On 02/04/2022 15:22, Krzysztof Kozlowski wrote: > On 01/04/2022 22:26, Caleb Connolly wrote: >> Add devicetree bindings for the Qualcomm PMI8998/PM660 SMB2 charger >> drivers. >> >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> >> --- >> .../bindings/power/supply/qcom,smb2.yaml | 68 +++++++++++++++++++ >> 1 file changed, 68 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml >> >> diff --git a/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml >> new file mode 100644 >> index 000000000000..1bea1fef78b8 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml >> @@ -0,0 +1,68 @@ >> +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/power/supply/qcom,smb2.yaml# > > Hi, Hi Krzysztof, > > Are you sure "smb2" is a real Qualcomm versioning? IOW, is there going > to be smb3 in the future? If not, better to just name the file according > to model, so like compatible and like other existing schemas from Qualcomm. Qualcomm versioning is a complete mystery to me
On Fri, Apr 01, 2022 at 09:26:43PM +0100, Caleb Connolly wrote: > Add devicetree bindings for the Qualcomm PMI8998/PM660 SMB2 charger > drivers. > > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > --- > .../bindings/power/supply/qcom,smb2.yaml | 68 +++++++++++++++++++ > 1 file changed, 68 insertions(+) > create mode 100644 Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml > > diff --git a/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml > new file mode 100644 > index 000000000000..1bea1fef78b8 > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml > @@ -0,0 +1,68 @@ > +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/power/supply/qcom,smb2.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm PMI8998/PM660 Switch-Mode Battery Charger "2" > + > +maintainers: > + - Caleb Connolly <caleb.connolly@linaro.org> > + > +properties: > + compatible: > + enum: > + - qcom,pmi8998-smb2 Since there's only 1 entry, please use const. > + > + reg: > + maxItems: 1 > + > + interrupts: > + items: > + - description: usb plugin > + > + interrupt-names: > + items: > + - const: usb-plugin > + > + io-channels: > + items: > + - description: USB in current in uA > + - description: USB in voltage in uV > + > + io-channel-names: > + items: > + - const: usbin_i > + - const: usbin_v > + > +required: > + - compatible > + - reg > + - interrupts > + - interrupt-names > + - io-channels > + - io-channel-names > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> New line here. Looks nice. > + pmic { > + #address-cells = <1>; > + #size-cells = <0>; > + #interrupt-cells = <4>; > + > + smb2@1000 { > + compatible = "qcom,pmi8998-smb2"; > + reg = <0x1000>; > + > + interrupts = <0x2 0x13 0x4 IRQ_TYPE_EDGE_BOTH>; > + interrupt-names = "usb-plugin"; > + In-between new lines may not be required. And DTs use tabs instead of 2 spaces, we can follow that here also. > + io-channels = <&pmi8998_rradc 3>, > + <&pmi8998_rradc 4>; > + io-channel-names = "usbin_i", > + "usbin_v"; Channel-names can be written in one line. > + }; > + }; > -- > 2.35.1 >
On 03/04/2022 09:14, Kuldeep Singh wrote: > On Fri, Apr 01, 2022 at 09:26:43PM +0100, Caleb Connolly wrote: >> Add devicetree bindings for the Qualcomm PMI8998/PM660 SMB2 charger >> drivers. >> (...) > >> + pmic { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + #interrupt-cells = <4>; >> + >> + smb2@1000 { >> + compatible = "qcom,pmi8998-smb2"; >> + reg = <0x1000>; >> + >> + interrupts = <0x2 0x13 0x4 IRQ_TYPE_EDGE_BOTH>; >> + interrupt-names = "usb-plugin"; >> + > > In-between new lines may not be required. > And DTs use tabs instead of 2 spaces, we can follow that here also. The DT examples in bindings use spaces. Either two (like YAML) or four (for easier reading). > >> + io-channels = <&pmi8998_rradc 3>, >> + <&pmi8998_rradc 4>; >> + io-channel-names = "usbin_i", >> + "usbin_v"; > > Channel-names can be written in one line. They match the format of io-channels, so this is quite readable. Best regards, Krzysztof
On Sun, Apr 03, 2022 at 09:56:25AM +0200, Krzysztof Kozlowski wrote: > On 03/04/2022 09:14, Kuldeep Singh wrote: > > On Fri, Apr 01, 2022 at 09:26:43PM +0100, Caleb Connolly wrote: > >> Add devicetree bindings for the Qualcomm PMI8998/PM660 SMB2 charger > >> drivers. > >> > > (...) > > > > >> + pmic { > >> + #address-cells = <1>; > >> + #size-cells = <0>; > >> + #interrupt-cells = <4>; > >> + > >> + smb2@1000 { > >> + compatible = "qcom,pmi8998-smb2"; > >> + reg = <0x1000>; > >> + > >> + interrupts = <0x2 0x13 0x4 IRQ_TYPE_EDGE_BOTH>; > >> + interrupt-names = "usb-plugin"; > >> + > > > > In-between new lines may not be required. > > And DTs use tabs instead of 2 spaces, we can follow that here also. > > The DT examples in bindings use spaces. Either two (like YAML) or four > (for easier reading). ok, since example snippet is taken from DT that's why I said four spaces(tab) as it will be closest to actual env. > > > > >> + io-channels = <&pmi8998_rradc 3>, > >> + <&pmi8998_rradc 4>; > >> + io-channel-names = "usbin_i", > >> + "usbin_v"; > > > > Channel-names can be written in one line. > > They match the format of io-channels, so this is quite readable. io-channels doesn't exceed max characters in line(i.e 75) even after being clubbed. Won't be better if kept in one line? This might be personal perspective but I thought it's worth mentioning.
On 03/04/2022 15:31, Kuldeep Singh wrote: > On Sun, Apr 03, 2022 at 09:56:25AM +0200, Krzysztof Kozlowski wrote: >> On 03/04/2022 09:14, Kuldeep Singh wrote: >>> On Fri, Apr 01, 2022 at 09:26:43PM +0100, Caleb Connolly wrote: >>>> Add devicetree bindings for the Qualcomm PMI8998/PM660 SMB2 charger >>>> drivers. >>>> >> >> (...) >> >>> >>>> + pmic { >>>> + #address-cells = <1>; >>>> + #size-cells = <0>; >>>> + #interrupt-cells = <4>; >>>> + >>>> + smb2@1000 { >>>> + compatible = "qcom,pmi8998-smb2"; >>>> + reg = <0x1000>; >>>> + >>>> + interrupts = <0x2 0x13 0x4 IRQ_TYPE_EDGE_BOTH>; >>>> + interrupt-names = "usb-plugin"; >>>> + >>> >>> In-between new lines may not be required. >>> And DTs use tabs instead of 2 spaces, we can follow that here also. >> >> The DT examples in bindings use spaces. Either two (like YAML) or four >> (for easier reading). > > ok, since example snippet is taken from DT that's why I said four > spaces(tab) as it will be closest to actual env. You said "use tabs", which is 8 spaces in Linux. So to clarify - we do not use tabs here, so do not use tabs. >>>> + io-channels = <&pmi8998_rradc 3>, >>>> + <&pmi8998_rradc 4>; >>>> + io-channel-names = "usbin_i", >>>> + "usbin_v"; >>> >>> Channel-names can be written in one line. >> >> They match the format of io-channels, so this is quite readable. > > io-channels doesn't exceed max characters in line(i.e 75) even after > being clubbed. Won't be better if kept in one line? > This might be personal perspective but I thought it's worth mentioning. I find current code readable. The other option would be fine as well, kind of does not matter to me much. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml new file mode 100644 index 000000000000..1bea1fef78b8 --- /dev/null +++ b/Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml @@ -0,0 +1,68 @@ +# SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/power/supply/qcom,smb2.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Qualcomm PMI8998/PM660 Switch-Mode Battery Charger "2" + +maintainers: + - Caleb Connolly <caleb.connolly@linaro.org> + +properties: + compatible: + enum: + - qcom,pmi8998-smb2 + + reg: + maxItems: 1 + + interrupts: + items: + - description: usb plugin + + interrupt-names: + items: + - const: usb-plugin + + io-channels: + items: + - description: USB in current in uA + - description: USB in voltage in uV + + io-channel-names: + items: + - const: usbin_i + - const: usbin_v + +required: + - compatible + - reg + - interrupts + - interrupt-names + - io-channels + - io-channel-names + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + pmic { + #address-cells = <1>; + #size-cells = <0>; + #interrupt-cells = <4>; + + smb2@1000 { + compatible = "qcom,pmi8998-smb2"; + reg = <0x1000>; + + interrupts = <0x2 0x13 0x4 IRQ_TYPE_EDGE_BOTH>; + interrupt-names = "usb-plugin"; + + io-channels = <&pmi8998_rradc 3>, + <&pmi8998_rradc 4>; + io-channel-names = "usbin_i", + "usbin_v"; + }; + };
Add devicetree bindings for the Qualcomm PMI8998/PM660 SMB2 charger drivers. Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> --- .../bindings/power/supply/qcom,smb2.yaml | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/supply/qcom,smb2.yaml