| Message ID | 20211105033558.1573552-6-bryan.odonoghue@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Add pm8150b TPCM driver | expand |
On Fri, 05 Nov 2021 03:35:56 +0000, Bryan O'Donoghue wrote: > Add a YAML description for the pm8150b-tcpm driver. The pm8150b-tcpm > encapsulates a type-c block and a pdphy block into one block presented to > the tcpm Linux API. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > .../bindings/usb/qcom,pmic-tcpm.yaml | 68 +++++++++++++++++++ > 1 file changed, 68 insertions(+) > create mode 100644 Documentation/devicetree/bindings/usb/qcom,pmic-tcpm.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/usb/qcom,pmic-tcpm.example.dt.yaml: pmic-tcpm: '#address-cells', '#size-cells', 'connector' do not match any of the regexes: 'pinctrl-[0-9]+' From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/usb/qcom,pmic-tcpm.yaml doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1551213 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
On Fri, Nov 05, 2021 at 03:35:56AM +0000, Bryan O'Donoghue wrote: > Add a YAML description for the pm8150b-tcpm driver. The pm8150b-tcpm > encapsulates a type-c block and a pdphy block into one block presented to > the tcpm Linux API. All Linux details that don't belong in binding description and design. > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > --- > .../bindings/usb/qcom,pmic-tcpm.yaml | 68 +++++++++++++++++++ > 1 file changed, 68 insertions(+) > create mode 100644 Documentation/devicetree/bindings/usb/qcom,pmic-tcpm.yaml > > diff --git a/Documentation/devicetree/bindings/usb/qcom,pmic-tcpm.yaml b/Documentation/devicetree/bindings/usb/qcom,pmic-tcpm.yaml > new file mode 100644 > index 0000000000000..29ab7e2d678e1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/qcom,pmic-tcpm.yaml > @@ -0,0 +1,68 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: "http://devicetree.org/schemas/usb/qcom,pmic-tcpm.yaml#" > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > + > +title: Qualcomm PMIC TCPM Driver > + > +maintainers: > + - Bryan O'Donoghue <bryan.odonoghue@linaro.org> > + > +description: | > + Qualcomm PMIC Type-C Port Manager Driver > + > +properties: > + compatible: > + enum: > + - qcom,pm8150b-tcpm > + > + pmic_tcpm_typec: Don't use '_' in property names and custom properties need a vendor prefix. > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + A phandle to the typec port hardware driver. > + > + pmic_tcpm_pdphy: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: > + A phandle to the type-c pdphy hardware driver. What is this binding a child of? Looks like the h/w is all part of a PMIC, so it should be part of the PMIC binding and probably merged with one of the nodes these phandles point to. > + > +required: > + - compatible > + - pmic_tcpm_typec > + - pmic_tcpm_pdphy > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/usb/pd.h> > + #include <dt-bindings/usb/typec/tcpm/qcom,pmic-usb-typec.h> > + #include <dt-bindings/usb/typec/tcpm/qcom,pmic-usb-pdphy.h> > + > + pm8150b_tcpm: pmic-tcpm { > + compatible = "qcom,pm8150b-tcpm"; > + > + #address-cells = <1>; > + #size-cells = <0>; > + > + pmic_tcpm_typec = <&pm8150b_typec>; > + pmic_tcpm_pdphy = <&pm8150b_pdphy>; > + > + connector { > + compatible = "usb-c-connector"; > + > + power-role = "source"; > + data-role = "dual"; > + self-powered; > + > + source-pdos = <PDO_FIXED(5000, 3000, > + PDO_FIXED_DUAL_ROLE | > + PDO_FIXED_USB_COMM | > + PDO_FIXED_DATA_SWAP)>; > + > + }; > + }; > + > +... > -- > 2.33.0 > >
On 08/11/2021 17:13, Rob Herring wrote: > Looks like the h/w is all part of a > PMIC, so it should be part of the PMIC binding and probably merged with > one of the nodes these phandles point to. Not sure I really follow you here. The existing PMIC dts arch/arm64/boot/dts/qcom/pm8150b.dtsi has: pm8150b_gpios: gpio@c000 { compatible = "qcom,pm8150b-gpio"; } Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml and pm8150b_adc_tm: adc-tm@3500 { compatible = "qcom,spmi-adc-tm5"; }; Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml to which I'm adding : pm8150b_typec: typec@1500 { compatible = "qcom,pm8150b-typec"; }; Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml pm8150b_pdphy: pdphy@1700 { compatible = "qcom,pm8150b-pdphy"; }; Documentation/devicetree/bindings/usb/qcom,pmic-pdphy.yaml
On Mon, Nov 8, 2021 at 12:58 PM Bryan O'Donoghue <bryan.odonoghue@linaro.org> wrote: > > On 08/11/2021 17:13, Rob Herring wrote: > > Looks like the h/w is all part of a > > PMIC, so it should be part of the PMIC binding and probably merged with > > one of the nodes these phandles point to. > > Not sure I really follow you here. > > The existing PMIC dts arch/arm64/boot/dts/qcom/pm8150b.dtsi has: > > pm8150b_gpios: gpio@c000 { > compatible = "qcom,pm8150b-gpio"; > } > Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml > > and > > pm8150b_adc_tm: adc-tm@3500 { > compatible = "qcom,spmi-adc-tm5"; > }; > Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml > > to which I'm adding : > > pm8150b_typec: typec@1500 { > compatible = "qcom,pm8150b-typec"; > }; > > Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml > > pm8150b_pdphy: pdphy@1700 { > compatible = "qcom,pm8150b-pdphy"; > }; > > Documentation/devicetree/bindings/usb/qcom,pmic-pdphy.yaml From what I gather, there is not a 3rd h/w device this binding describes, but it is just a collection of all the data you happen to want for your driver. That's assuming a specific structure for a specific OS. Why can't most of this binding be part of "qcom,pm8150b-typec" instead of making up some virtual device? Rob
On 08/11/2021 19:22, Rob Herring wrote: > On Mon, Nov 8, 2021 at 12:58 PM Bryan O'Donoghue > <bryan.odonoghue@linaro.org> wrote: >> >> On 08/11/2021 17:13, Rob Herring wrote: >>> Looks like the h/w is all part of a >>> PMIC, so it should be part of the PMIC binding and probably merged with >>> one of the nodes these phandles point to. >> >> Not sure I really follow you here. >> >> The existing PMIC dts arch/arm64/boot/dts/qcom/pm8150b.dtsi has: >> >> pm8150b_gpios: gpio@c000 { >> compatible = "qcom,pm8150b-gpio"; >> } >> Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml >> >> and >> >> pm8150b_adc_tm: adc-tm@3500 { >> compatible = "qcom,spmi-adc-tm5"; >> }; >> Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml >> >> to which I'm adding : >> >> pm8150b_typec: typec@1500 { >> compatible = "qcom,pm8150b-typec"; >> }; >> >> Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml >> >> pm8150b_pdphy: pdphy@1700 { >> compatible = "qcom,pm8150b-pdphy"; >> }; >> >> Documentation/devicetree/bindings/usb/qcom,pmic-pdphy.yaml > > From what I gather, there is not a 3rd h/w device this binding > describes, but it is just a collection of all the data you happen to > want for your driver. The TCPM "virtual" driver presents as a device to the TCPM API and then uses phandle to talk to the PDPHY and typec devices yes. That's assuming a specific structure for a > specific OS. Why can't most of this binding be part of > "qcom,pm8150b-typec" instead of making up some virtual device? I thought it was a better model to have the TCPM be a separate device with the pdphy and typec blocks as their own devices. #1 Because the address space spans over more than just the pdphy and typec device, there's a charger block in between #2 Because the pdphy and typec have separate IRQ lines and register spaces --- bod
On Mon, Nov 08, 2021 at 07:36:27PM +0000, Bryan O'Donoghue wrote: > On 08/11/2021 19:22, Rob Herring wrote: > > On Mon, Nov 8, 2021 at 12:58 PM Bryan O'Donoghue > > <bryan.odonoghue@linaro.org> wrote: > > > > > > On 08/11/2021 17:13, Rob Herring wrote: > > > > Looks like the h/w is all part of a > > > > PMIC, so it should be part of the PMIC binding and probably merged with > > > > one of the nodes these phandles point to. > > > > > > Not sure I really follow you here. > > > > > > The existing PMIC dts arch/arm64/boot/dts/qcom/pm8150b.dtsi has: > > > > > > pm8150b_gpios: gpio@c000 { > > > compatible = "qcom,pm8150b-gpio"; > > > } > > > Documentation/devicetree/bindings/pinctrl/qcom,pmic-gpio.yaml > > > > > > and > > > > > > pm8150b_adc_tm: adc-tm@3500 { > > > compatible = "qcom,spmi-adc-tm5"; > > > }; > > > Documentation/devicetree/bindings/thermal/qcom-spmi-adc-tm5.yaml > > > > > > to which I'm adding : > > > > > > pm8150b_typec: typec@1500 { > > > compatible = "qcom,pm8150b-typec"; > > > }; > > > > > > Documentation/devicetree/bindings/usb/qcom,pmic-typec.yaml > > > > > > pm8150b_pdphy: pdphy@1700 { > > > compatible = "qcom,pm8150b-pdphy"; > > > }; > > > > > > Documentation/devicetree/bindings/usb/qcom,pmic-pdphy.yaml > > > > From what I gather, there is not a 3rd h/w device this binding > > describes, but it is just a collection of all the data you happen to > > want for your driver. > > The TCPM "virtual" driver presents as a device to the TCPM API and then uses > phandle to talk to the PDPHY and typec devices yes. That's nice, but it doesn't belong in DT. > That's assuming a specific structure for a > > specific OS. Why can't most of this binding be part of > > "qcom,pm8150b-typec" instead of making up some virtual device? > > I thought it was a better model to have the TCPM be a separate device with > the pdphy and typec blocks as their own devices. > > #1 Because the address space spans over more than just the pdphy and typec > device, there's a charger block in between > > #2 Because the pdphy and typec have separate IRQ lines and register spaces I didn't say combine them. That would be once again structuring your h/w description to match your driver architecture. Bind your driver to "qcom,pm8150b-typec" and then it can retrieve the "qcom,pm8150b-pdphy" node which doesn't have a driver. There's no rule that nodes and drivers are 1:1, or that a driver can't access DT data in another node. Your other option is instantiate your own device from the virtual driver's initcall based on presence of the 2 nodes above. Rob
On 12/11/2021 22:25, Rob Herring wrote: > Your other option is instantiate your own device from the virtual > driver's initcall based on presence of the 2 nodes above. This sounds a little bit more like what I'd like to do, I'll investigate it. --- bod
diff --git a/Documentation/devicetree/bindings/usb/qcom,pmic-tcpm.yaml b/Documentation/devicetree/bindings/usb/qcom,pmic-tcpm.yaml new file mode 100644 index 0000000000000..29ab7e2d678e1 --- /dev/null +++ b/Documentation/devicetree/bindings/usb/qcom,pmic-tcpm.yaml @@ -0,0 +1,68 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: "http://devicetree.org/schemas/usb/qcom,pmic-tcpm.yaml#" +$schema: "http://devicetree.org/meta-schemas/core.yaml#" + +title: Qualcomm PMIC TCPM Driver + +maintainers: + - Bryan O'Donoghue <bryan.odonoghue@linaro.org> + +description: | + Qualcomm PMIC Type-C Port Manager Driver + +properties: + compatible: + enum: + - qcom,pm8150b-tcpm + + pmic_tcpm_typec: + $ref: /schemas/types.yaml#/definitions/phandle + description: + A phandle to the typec port hardware driver. + + pmic_tcpm_pdphy: + $ref: /schemas/types.yaml#/definitions/phandle + description: + A phandle to the type-c pdphy hardware driver. + +required: + - compatible + - pmic_tcpm_typec + - pmic_tcpm_pdphy + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/usb/pd.h> + #include <dt-bindings/usb/typec/tcpm/qcom,pmic-usb-typec.h> + #include <dt-bindings/usb/typec/tcpm/qcom,pmic-usb-pdphy.h> + + pm8150b_tcpm: pmic-tcpm { + compatible = "qcom,pm8150b-tcpm"; + + #address-cells = <1>; + #size-cells = <0>; + + pmic_tcpm_typec = <&pm8150b_typec>; + pmic_tcpm_pdphy = <&pm8150b_pdphy>; + + connector { + compatible = "usb-c-connector"; + + power-role = "source"; + data-role = "dual"; + self-powered; + + source-pdos = <PDO_FIXED(5000, 3000, + PDO_FIXED_DUAL_ROLE | + PDO_FIXED_USB_COMM | + PDO_FIXED_DATA_SWAP)>; + + }; + }; + +...
Add a YAML description for the pm8150b-tcpm driver. The pm8150b-tcpm encapsulates a type-c block and a pdphy block into one block presented to the tcpm Linux API. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- .../bindings/usb/qcom,pmic-tcpm.yaml | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 Documentation/devicetree/bindings/usb/qcom,pmic-tcpm.yaml