diff mbox series

[v3,5/7] dt-bindings: usb: Add Qualcomm PMIC TCPM YAML schema

Message ID 20211105033558.1573552-6-bryan.odonoghue@linaro.org (mailing list archive)
State Not Applicable
Headers show
Series Add pm8150b TPCM driver | expand

Commit Message

Bryan O'Donoghue Nov. 5, 2021, 3:35 a.m. UTC
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

Comments

Rob Herring (Arm) Nov. 5, 2021, 2:41 p.m. UTC | #1
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.
Rob Herring (Arm) Nov. 8, 2021, 5:13 p.m. UTC | #2
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
> 
>
Bryan O'Donoghue Nov. 8, 2021, 7 p.m. UTC | #3
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
Rob Herring (Arm) Nov. 8, 2021, 7:22 p.m. UTC | #4
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
Bryan O'Donoghue Nov. 8, 2021, 7:36 p.m. UTC | #5
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
Rob Herring (Arm) Nov. 12, 2021, 10:25 p.m. UTC | #6
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
Bryan O'Donoghue Nov. 12, 2021, 11:05 p.m. UTC | #7
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 mbox series

Patch

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)>;
+
+        };
+    };
+
+...