[PATCH] dt-bindings: pm8941-misc: Fix usb_id and usb_vbus definitions

[PATCH] dt-bindings: pm8941-misc: Fix usb_id and usb_vbus definitions

[Bryan O'Donoghue]

dts validation is throwing an error for me on 8916 and 8939 with
extcon@1300. In this case we have usb_vbus but not usb_id.

Looking at the pm8941-misc driver we can have usb_id, usb_vbus or both at
the same time.

Expand the definition with anyOf to capture the three different valid
modes.

Fixes: 4fcdd677c4ea ("bindings: pm8941-misc: Add support for VBUS detection")
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 .../devicetree/bindings/extcon/qcom,pm8941-misc.yaml | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml
index 6a9c96f0352ac..1bc412a4ac5e6 100644
--- a/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml
+++ b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml
@@ -27,10 +27,14 @@ properties:
 
   interrupt-names:
     minItems: 1
-    items:
-      - const: usb_id
-      - const: usb_vbus
-
+    anyOf:
+      - items:
+          - const: usb_id
+          - const: usb_vbus
+      - items:
+          - const: usb_id
+      - items:
+          - const: usb_vbus
 required:
   - compatible
   - reg
-- 
2.36.1

Re: [PATCH] dt-bindings: pm8941-misc: Fix usb_id and usb_vbus definitions

[Krzysztof Kozlowski]

On 30/06/2022 06:23, Bryan O'Donoghue wrote:
> dts validation is throwing an error for me on 8916 and 8939 with
> extcon@1300. In this case we have usb_vbus but not usb_id.
> 
> Looking at the pm8941-misc driver we can have usb_id, usb_vbus or both at
> the same time.

Implementation is not the best reason to change bindings. Implementation
can change, bindings should not.

> 
> Expand the definition with anyOf to capture the three different valid
> modes.
> 
> Fixes: 4fcdd677c4ea ("bindings: pm8941-misc: Add support for VBUS detection")
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  .../devicetree/bindings/extcon/qcom,pm8941-misc.yaml | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml
> index 6a9c96f0352ac..1bc412a4ac5e6 100644
> --- a/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml
> +++ b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml
> @@ -27,10 +27,14 @@ properties:
>  
>    interrupt-names:
>      minItems: 1
> -    items:
> -      - const: usb_id
> -      - const: usb_vbus
> -
> +    anyOf:
> +      - items:
> +          - const: usb_id
> +          - const: usb_vbus
> +      - items:
> +          - const: usb_id

I don't think you can have ID connected and VBUS disconnected, therefore
is it even possible to have missing VBUS interrupt?

> +      - items:
> +          - const: usb_vbus
>  required:
>    - compatible
>    - reg


Best regards,
Krzysztof

Re: [PATCH] dt-bindings: pm8941-misc: Fix usb_id and usb_vbus definitions

[Bryan O'Donoghue]

On 30/06/2022 19:47, Krzysztof Kozlowski wrote:
> On 30/06/2022 06:23, Bryan O'Donoghue wrote:
>> dts validation is throwing an error for me on 8916 and 8939 with
>> extcon@1300. In this case we have usb_vbus but not usb_id.
>>
>> Looking at the pm8941-misc driver we can have usb_id, usb_vbus or both at
>> the same time.
> 
> Implementation is not the best reason to change bindings. Implementation
> can change, bindings should not.
> 
>>
>> Expand the definition with anyOf to capture the three different valid
>> modes.
>>
>> Fixes: 4fcdd677c4ea ("bindings: pm8941-misc: Add support for VBUS detection")
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> ---
>>   .../devicetree/bindings/extcon/qcom,pm8941-misc.yaml | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml
>> index 6a9c96f0352ac..1bc412a4ac5e6 100644
>> --- a/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml
>> +++ b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml
>> @@ -27,10 +27,14 @@ properties:
>>   
>>     interrupt-names:
>>       minItems: 1
>> -    items:
>> -      - const: usb_id
>> -      - const: usb_vbus
>> -
>> +    anyOf:
>> +      - items:
>> +          - const: usb_id
>> +          - const: usb_vbus
>> +      - items:
>> +          - const: usb_id
> 
> I don't think you can have ID connected and VBUS disconnected, therefore
> is it even possible to have missing VBUS interrupt?

So the driver code does support that configuration

info->id_irq = platform_get_irq_byname(pdev, "usb_id");
if (info->id_irq > 0) {
         ret = devm_request_threaded_irq(dev, info->id_irq, NULL,
                                 qcom_usb_irq_handler,
                                 IRQF_TRIGGER_RISING |
                                 IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
                                 pdev->name, info);
         if (ret < 0) {
                 dev_err(dev, "failed to request handler for ID IRQ\n");
                 return ret;
         }
}

info->vbus_irq = platform_get_irq_byname(pdev, "usb_vbus");
if (info->vbus_irq > 0) {
         ret = devm_request_threaded_irq(dev, info->vbus_irq, NULL,
                                 qcom_usb_irq_handler,
                                 IRQF_TRIGGER_RISING |
                                 IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
                                 pdev->name, info);
         if (ret < 0) {
                 dev_err(dev, "failed to request handler for VBUS IRQ\n");
                 return ret;
         }
}

Looking at what we have in upstream we declare the usb_vbus interrupt 
but no platform that I can see declares a usb_id interrupt.

In practice the USB host driver drivers/usb/chipidea/core.c gets an 
extcon from a GPIO instead of from the pm8941 block.

arch/arm64/boot/dts/qcom/msm8916-huawei-g7.dts
arch/arm64/boot/dts/qcom/msm8916-asus-z00l.dts

On the T2a platform we use an external USB type-c controller which owns 
both vbus and usb_id/role but, in that case we don't want to switch on 
this driver at all...

Yep, I agree with you. I don't see a valid use-case for this driver 
without usb_vbus.

I'll tweak the bindings to reflect.

---
bod

Re: [PATCH] dt-bindings: pm8941-misc: Fix usb_id and usb_vbus definitions

[Bryan O'Donoghue]

On 30/06/2022 21:09, Bryan O'Donoghue wrote:
> On 30/06/2022 19:47, Krzysztof Kozlowski wrote:
>> On 30/06/2022 06:23, Bryan O'Donoghue wrote:
>>> dts validation is throwing an error for me on 8916 and 8939 with
>>> extcon@1300. In this case we have usb_vbus but not usb_id.
>>>
>>> Looking at the pm8941-misc driver we can have usb_id, usb_vbus or 
>>> both at
>>> the same time.
>>
>> Implementation is not the best reason to change bindings. Implementation
>> can change, bindings should not.
>>
>>>
>>> Expand the definition with anyOf to capture the three different valid
>>> modes.
>>>
>>> Fixes: 4fcdd677c4ea ("bindings: pm8941-misc: Add support for VBUS 
>>> detection")
>>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>>> ---
>>>   .../devicetree/bindings/extcon/qcom,pm8941-misc.yaml | 12 ++++++++----
>>>   1 file changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml 
>>> b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml
>>> index 6a9c96f0352ac..1bc412a4ac5e6 100644
>>> --- a/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml
>>> +++ b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml
>>> @@ -27,10 +27,14 @@ properties:
>>>     interrupt-names:
>>>       minItems: 1
>>> -    items:
>>> -      - const: usb_id
>>> -      - const: usb_vbus
>>> -
>>> +    anyOf:
>>> +      - items:
>>> +          - const: usb_id
>>> +          - const: usb_vbus
>>> +      - items:
>>> +          - const: usb_id
>>
>> I don't think you can have ID connected and VBUS disconnected, therefore
>> is it even possible to have missing VBUS interrupt?
> 
> So the driver code does support that configuration
> 
> info->id_irq = platform_get_irq_byname(pdev, "usb_id");
> if (info->id_irq > 0) {
>          ret = devm_request_threaded_irq(dev, info->id_irq, NULL,
>                                  qcom_usb_irq_handler,
>                                  IRQF_TRIGGER_RISING |
>                                  IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>                                  pdev->name, info);
>          if (ret < 0) {
>                  dev_err(dev, "failed to request handler for ID IRQ\n");
>                  return ret;
>          }
> }
> 
> info->vbus_irq = platform_get_irq_byname(pdev, "usb_vbus");
> if (info->vbus_irq > 0) {
>          ret = devm_request_threaded_irq(dev, info->vbus_irq, NULL,
>                                  qcom_usb_irq_handler,
>                                  IRQF_TRIGGER_RISING |
>                                  IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>                                  pdev->name, info);
>          if (ret < 0) {
>                  dev_err(dev, "failed to request handler for VBUS IRQ\n");
>                  return ret;
>          }
> }
> 
> Looking at what we have in upstream we declare the usb_vbus interrupt 
> but no platform that I can see declares a usb_id interrupt.
> 
> In practice the USB host driver drivers/usb/chipidea/core.c gets an 
> extcon from a GPIO instead of from the pm8941 block.
> 
> arch/arm64/boot/dts/qcom/msm8916-huawei-g7.dts
> arch/arm64/boot/dts/qcom/msm8916-asus-z00l.dts
> 
> On the T2a platform we use an external USB type-c controller which owns 
> both vbus and usb_id/role but, in that case we don't want to switch on 
> this driver at all...
> 
> Yep, I agree with you. I don't see a valid use-case for this driver 
> without usb_vbus.
> 
> I'll tweak the bindings to reflect.
> 
> ---
> bod

Ah but wait.

Equally it would be possible to have an external port manager like the 
i2c type-c port manager we have which would control vbus but, the 
hardware could route usb_id straight to the pm8941 block.

Hmm, no I do think it is possible to have a valid use of this driver - 
with vbus owned by an external IC but usb_id routed here instead of to a 
GPIO..

---
bod

Re: [PATCH] dt-bindings: pm8941-misc: Fix usb_id and usb_vbus definitions

[Stephan Gerhold]

On Thu, Jun 30, 2022 at 08:47:32PM +0200, Krzysztof Kozlowski wrote:
> On 30/06/2022 06:23, Bryan O'Donoghue wrote:
> > diff --git a/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml
> > index 6a9c96f0352ac..1bc412a4ac5e6 100644
> > --- a/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml
> > +++ b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml
> > @@ -27,10 +27,14 @@ properties:
> >  
> >    interrupt-names:
> >      minItems: 1
> > -    items:
> > -      - const: usb_id
> > -      - const: usb_vbus
> > -
> > +    anyOf:
> > +      - items:
> > +          - const: usb_id
> > +          - const: usb_vbus
> > +      - items:
> > +          - const: usb_id
> 
> I don't think you can have ID connected and VBUS disconnected, therefore
> is it even possible to have missing VBUS interrupt?
> 

The driver was originally made for pm8941, which uses exactly the
usb_id-only configuration (see arch/arm/boot/dts/qcom-pm8941.dtsi):

        usb_id: misc@900 {
                compatible = "qcom,pm8941-misc";
                reg = <0x900>;
                interrupts = <0x0 0x9 0 IRQ_TYPE_EDGE_BOTH>;
                interrupt-names = "usb_id";
        };

The "usb_vbus" interrupt is basically already assigned to the charger
node ("usb-valid" in qcom,pm8941-charger), so I'm not sure if it's
possible to add it to the extcon node as well. The charger driver
provides a separate extcon device with the VBUS state so it's not much
of a problem in practice.

Thanks,
Stephan

Re: [PATCH] dt-bindings: pm8941-misc: Fix usb_id and usb_vbus definitions

[Marijn Suijten]

On 2022-06-30 20:47:32, Krzysztof Kozlowski wrote:
> On 30/06/2022 06:23, Bryan O'Donoghue wrote:
> > dts validation is throwing an error for me on 8916 and 8939 with
> > extcon@1300. In this case we have usb_vbus but not usb_id.
> > 
> > Looking at the pm8941-misc driver we can have usb_id, usb_vbus or both at
> > the same time.
> 
> Implementation is not the best reason to change bindings. Implementation
> can change, bindings should not.
> 
> > 
> > Expand the definition with anyOf to capture the three different valid
> > modes.
> > 
> > Fixes: 4fcdd677c4ea ("bindings: pm8941-misc: Add support for VBUS detection")
> > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> > ---
> >  .../devicetree/bindings/extcon/qcom,pm8941-misc.yaml | 12 ++++++++----
> >  1 file changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml
> > index 6a9c96f0352ac..1bc412a4ac5e6 100644
> > --- a/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml
> > +++ b/Documentation/devicetree/bindings/extcon/qcom,pm8941-misc.yaml
> > @@ -27,10 +27,14 @@ properties:
> >  
> >    interrupt-names:
> >      minItems: 1
> > -    items:
> > -      - const: usb_id
> > -      - const: usb_vbus
> > -
> > +    anyOf:
> > +      - items:
> > +          - const: usb_id
> > +          - const: usb_vbus
> > +      - items:
> > +          - const: usb_id
> 
> I don't think you can have ID connected and VBUS disconnected, therefore
> is it even possible to have missing VBUS interrupt?

This is how I've been using it on the pmi8950 (apologies, patches not
yet upstream) because both interrupts are on a different block:

    pmi8950_usb_vbus: extcon-chgpth@1300 {
        compatible = "qcom,pm8941-misc";
        interrupts = <0x2 0x13 0x2 IRQ_TYPE_NONE>;
        interrupt-names = "usb_vbus";
    };

    pmi8950_usb_id: extcon-otg@1100 {
        compatible = "qcom,pm8941-misc";
        interrupts = <0x2 0x11 0x3 IRQ_TYPE_NONE>;
        interrupt-names = "usb_id";
    };

This also results in annoying errors (back in the day, found these on a
5.13 log but surely remember seeing it on 5.18 as well):

    [    1.377491] extcon-pm8941-misc 200f000.spmi:pmic@2:extcon_chgpth@1300: IRQ usb_id not found
    [    1.380399] extcon-pm8941-misc 200f000.spmi:pmic@2:extcon_otg@1100: IRQ usb_vbus not found

Now, given that the offset also seems to be encoded in the interrupt
definition, and it's been functioning without `reg` (required by the
dt-bindings but I see no register mapping in the driver, it reads
interrupt line level from the irqchip directly), it is perhaps possible
to throw them into one DT node without address altogether?  (untested)

    pmi8950_usb_extcon: extcon {
        compatible = "qcom,pm8941-misc";
        interrupts = <0x2 0x11 0x3 IRQ_TYPE_NONE>,
                     <0x2 0x13 0x2 IRQ_TYPE_NONE>;
        interrupt-names = "usb_id", "usb_vbus";
    };

- Marijn