| Message ID | 20220630042357.3308128-1-bryan.odonoghue@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dt-bindings: pm8941-misc: Fix usb_id and usb_vbus definitions | expand |
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
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
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
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(-)