* [PATCH 0/2] typec: tipd: Add support for polling
@ 2022-04-14 8:31 Aswath Govindraju
2022-04-14 8:31 ` [PATCH 1/2] dt-bindings: usb: tps6598x: Make the interrupts property optional Aswath Govindraju
2022-04-14 8:31 ` [PATCH 2/2] usb: typec: tipd: Add support for polling interrupts status when interrupt line is not connected Aswath Govindraju
0 siblings, 2 replies; 11+ messages in thread
From: Aswath Govindraju @ 2022-04-14 8:31 UTC (permalink / raw)
Cc: Vignesh Raghavendra, Roger Quadros, Kishon Vijay Abraham I,
Aswath Govindraju, Greg Kroah-Hartman, Rob Herring,
Krzysztof Kozlowski, Heikki Krogerus, Sven Peter,
Alyssa Rosenzweig, Hector Martin, Saranya Gopal,
Bryan O'Donoghue, linux-usb, devicetree, linux-kernel
The following series of patches add support for polling in the tipd
driver. The driver switches to polling by default, when interrupts
property is not populated.
Link to RFC patch posted earlier,
- https://patchwork.kernel.org/project/linux-usb/patch/20220412145059.4717-1-a-govindraju@ti.com/
Changes since RFC patch,
- Added patch to make the required changes in dt-bindings to make
interrupts optional
- Changed to using (client->irq) to decide whether interrupts or
polling should be used instead of switching to polling based on
the return value while requesting irq line.
Aswath Govindraju (2):
dt-bindings: usb: tps6598x: Make the interrupts property optional
usb: typec: tipd: Add support for polling interrupts status when
interrupt line is not connected
.../devicetree/bindings/usb/ti,tps6598x.yaml | 4 +-
drivers/usb/typec/tipd/core.c | 99 ++++++++++++++++---
2 files changed, 90 insertions(+), 13 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/2] dt-bindings: usb: tps6598x: Make the interrupts property optional 2022-04-14 8:31 [PATCH 0/2] typec: tipd: Add support for polling Aswath Govindraju @ 2022-04-14 8:31 ` Aswath Govindraju 2022-04-14 18:10 ` Roger Quadros 2022-04-14 8:31 ` [PATCH 2/2] usb: typec: tipd: Add support for polling interrupts status when interrupt line is not connected Aswath Govindraju 1 sibling, 1 reply; 11+ messages in thread From: Aswath Govindraju @ 2022-04-14 8:31 UTC (permalink / raw) Cc: Vignesh Raghavendra, Roger Quadros, Kishon Vijay Abraham I, Aswath Govindraju, Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski, Heikki Krogerus, Sven Peter, Alyssa Rosenzweig, Hector Martin, Martin Kepplinger, Bryan O'Donoghue, linux-usb, devicetree, linux-kernel Support for polling has been added in the driver, which will be used by default if interrupts property is not populated. Therefore, remove interrupts and interrupt-names from the required properties and add a note under interrupts property describing the above support in driver. Suggested-by: Roger Quadros <rogerq@kernel.org> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com> --- Documentation/devicetree/bindings/usb/ti,tps6598x.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml index a4c53b1f1af3..1c4b8c6233e5 100644 --- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml +++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml @@ -25,6 +25,8 @@ properties: interrupts: maxItems: 1 + description: + If interrupts are not populated then by default polling will be used. interrupt-names: items: @@ -33,8 +35,6 @@ properties: required: - compatible - reg - - interrupts - - interrupt-names additionalProperties: true -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] dt-bindings: usb: tps6598x: Make the interrupts property optional 2022-04-14 8:31 ` [PATCH 1/2] dt-bindings: usb: tps6598x: Make the interrupts property optional Aswath Govindraju @ 2022-04-14 18:10 ` Roger Quadros 2022-04-18 5:19 ` Aswath Govindraju 0 siblings, 1 reply; 11+ messages in thread From: Roger Quadros @ 2022-04-14 18:10 UTC (permalink / raw) To: Aswath Govindraju Cc: Vignesh Raghavendra, Kishon Vijay Abraham I, Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski, Heikki Krogerus, Sven Peter, Alyssa Rosenzweig, Hector Martin, Martin Kepplinger, Bryan O'Donoghue, linux-usb, devicetree, linux-kernel Hi, On 14/04/2022 11:31, Aswath Govindraju wrote: > Support for polling has been added in the driver, which will be used by > default if interrupts property is not populated. Therefore, remove > interrupts and interrupt-names from the required properties and add a note > under interrupts property describing the above support in driver. > > Suggested-by: Roger Quadros <rogerq@kernel.org> I did not suggest to make interrupts optional by default. What I suggested was that if a DT property exists to explicitly indicate polling mode then interrupts are not required. > Signed-off-by: Aswath Govindraju <a-govindraju@ti.com> > --- > Documentation/devicetree/bindings/usb/ti,tps6598x.yaml | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml > index a4c53b1f1af3..1c4b8c6233e5 100644 > --- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml > +++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml > @@ -25,6 +25,8 @@ properties: > > interrupts: > maxItems: 1 > + description: > + If interrupts are not populated then by default polling will be used. > > interrupt-names: > items: > @@ -33,8 +35,6 @@ properties: > required: > - compatible > - reg > - - interrupts > - - interrupt-names > > additionalProperties: true > cheers, -roger ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] dt-bindings: usb: tps6598x: Make the interrupts property optional 2022-04-14 18:10 ` Roger Quadros @ 2022-04-18 5:19 ` Aswath Govindraju 2022-04-20 19:16 ` Roger Quadros 0 siblings, 1 reply; 11+ messages in thread From: Aswath Govindraju @ 2022-04-18 5:19 UTC (permalink / raw) To: Roger Quadros Cc: Vignesh Raghavendra, Kishon Vijay Abraham I, Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski, Heikki Krogerus, Sven Peter, Alyssa Rosenzweig, Hector Martin, Martin Kepplinger, Bryan O'Donoghue, linux-usb, devicetree, linux-kernel Hi Roger, On 14/04/22 23:40, Roger Quadros wrote: > Hi, > > On 14/04/2022 11:31, Aswath Govindraju wrote: >> Support for polling has been added in the driver, which will be used by >> default if interrupts property is not populated. Therefore, remove >> interrupts and interrupt-names from the required properties and add a note >> under interrupts property describing the above support in driver. >> >> Suggested-by: Roger Quadros <rogerq@kernel.org> > > I did not suggest to make interrupts optional by default. > > What I suggested was that if a DT property exists to explicitly > indicate polling mode then interrupts are not required. > ohh okay, got it. However, may I know if adding a dt property to indicate polling for aiding the driver, is the correct approach to model it? In terms of modelling hardware, as interrupts are not connected we are not populating the interrupts property. Shouldn't that be all. If we are adding a property explicitly to indicate polling that can be used by driver, wouldn't that be a software aid being added in the device tree? Thanks, Aswath >> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com> >> --- >> Documentation/devicetree/bindings/usb/ti,tps6598x.yaml | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml >> index a4c53b1f1af3..1c4b8c6233e5 100644 >> --- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml >> +++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml >> @@ -25,6 +25,8 @@ properties: >> >> interrupts: >> maxItems: 1 >> + description: >> + If interrupts are not populated then by default polling will be used. >> >> interrupt-names: >> items: >> @@ -33,8 +35,6 @@ properties: >> required: >> - compatible >> - reg >> - - interrupts >> - - interrupt-names >> >> additionalProperties: true >> > > cheers, > -roger ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] dt-bindings: usb: tps6598x: Make the interrupts property optional 2022-04-18 5:19 ` Aswath Govindraju @ 2022-04-20 19:16 ` Roger Quadros 2022-04-22 5:07 ` Aswath Govindraju 0 siblings, 1 reply; 11+ messages in thread From: Roger Quadros @ 2022-04-20 19:16 UTC (permalink / raw) To: Aswath Govindraju Cc: Vignesh Raghavendra, Kishon Vijay Abraham I, Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski, Heikki Krogerus, Sven Peter, Alyssa Rosenzweig, Hector Martin, Martin Kepplinger, Bryan O'Donoghue, linux-usb, devicetree, linux-kernel Hi, On 18/04/2022 08:19, Aswath Govindraju wrote: > Hi Roger, > > On 14/04/22 23:40, Roger Quadros wrote: >> Hi, >> >> On 14/04/2022 11:31, Aswath Govindraju wrote: >>> Support for polling has been added in the driver, which will be used by >>> default if interrupts property is not populated. Therefore, remove >>> interrupts and interrupt-names from the required properties and add a note >>> under interrupts property describing the above support in driver. >>> >>> Suggested-by: Roger Quadros <rogerq@kernel.org> >> >> I did not suggest to make interrupts optional by default. >> >> What I suggested was that if a DT property exists to explicitly >> indicate polling mode then interrupts are not required. >> > > ohh okay, got it. However, may I know if adding a dt property to > indicate polling for aiding the driver, is the correct approach to model it? > > In terms of modelling hardware, as interrupts are not connected we are > not populating the interrupts property. Shouldn't that be all. If we are > adding a property explicitly to indicate polling that can be used by > driver, wouldn't that be a software aid being added in the device tree? The hardware (tps6598x chip) has an interrupt pin and is expected to be used in normal case. Some buggy boards might have forgot to connect it. We are adding polling mode only for these buggy boards. ;) So polling mode is an exception. cheers, -roger > > Thanks, > Aswath > >>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com> >>> --- >>> Documentation/devicetree/bindings/usb/ti,tps6598x.yaml | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml >>> index a4c53b1f1af3..1c4b8c6233e5 100644 >>> --- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml >>> +++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml >>> @@ -25,6 +25,8 @@ properties: >>> >>> interrupts: >>> maxItems: 1 >>> + description: >>> + If interrupts are not populated then by default polling will be used. >>> >>> interrupt-names: >>> items: >>> @@ -33,8 +35,6 @@ properties: >>> required: >>> - compatible >>> - reg >>> - - interrupts >>> - - interrupt-names >>> >>> additionalProperties: true >>> >> >> cheers, >> -roger ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] dt-bindings: usb: tps6598x: Make the interrupts property optional 2022-04-20 19:16 ` Roger Quadros @ 2022-04-22 5:07 ` Aswath Govindraju 2022-04-26 6:42 ` Roger Quadros 0 siblings, 1 reply; 11+ messages in thread From: Aswath Govindraju @ 2022-04-22 5:07 UTC (permalink / raw) To: Roger Quadros Cc: Vignesh Raghavendra, Kishon Vijay Abraham I, Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski, Heikki Krogerus, Sven Peter, Alyssa Rosenzweig, Hector Martin, Martin Kepplinger, Bryan O'Donoghue, linux-usb, devicetree, linux-kernel Hi Roger, On 21/04/22 00:46, Roger Quadros wrote: > Hi, > > On 18/04/2022 08:19, Aswath Govindraju wrote: >> Hi Roger, >> >> On 14/04/22 23:40, Roger Quadros wrote: >>> Hi, >>> >>> On 14/04/2022 11:31, Aswath Govindraju wrote: >>>> Support for polling has been added in the driver, which will be used by >>>> default if interrupts property is not populated. Therefore, remove >>>> interrupts and interrupt-names from the required properties and add a note >>>> under interrupts property describing the above support in driver. >>>> >>>> Suggested-by: Roger Quadros <rogerq@kernel.org> >>> >>> I did not suggest to make interrupts optional by default. >>> >>> What I suggested was that if a DT property exists to explicitly >>> indicate polling mode then interrupts are not required. >>> >> >> ohh okay, got it. However, may I know if adding a dt property to >> indicate polling for aiding the driver, is the correct approach to model it? >> >> In terms of modelling hardware, as interrupts are not connected we are >> not populating the interrupts property. Shouldn't that be all. If we are >> adding a property explicitly to indicate polling that can be used by >> driver, wouldn't that be a software aid being added in the device tree? > > The hardware (tps6598x chip) has an interrupt pin and is expected to be used > in normal case. > > Some buggy boards might have forgot to connect it. We are adding polling mode only for these buggy boards. ;) > So polling mode is an exception. > Yes as you mentioned the interrupt line is expected to connected but there could be cases where there are not enough pins on the SoC and polling is used intentionally. In these cases this would be a feature rather than a bug. Also, I feel like not adding interrupts property in the dt nodes will indicate polling. My question is why are we adding an extra property (which is being used only as an aid in the driver) when this feature can be modeled by making interrupts property optional. Thanks, Aswath > cheers, > -roger > >> >> Thanks, >> Aswath >> >>>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com> >>>> --- >>>> Documentation/devicetree/bindings/usb/ti,tps6598x.yaml | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml >>>> index a4c53b1f1af3..1c4b8c6233e5 100644 >>>> --- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml >>>> +++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml >>>> @@ -25,6 +25,8 @@ properties: >>>> >>>> interrupts: >>>> maxItems: 1 >>>> + description: >>>> + If interrupts are not populated then by default polling will be used. >>>> >>>> interrupt-names: >>>> items: >>>> @@ -33,8 +35,6 @@ properties: >>>> required: >>>> - compatible >>>> - reg >>>> - - interrupts >>>> - - interrupt-names >>>> >>>> additionalProperties: true >>>> >>> >>> cheers, >>> -roger -- Thanks, Aswath ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] dt-bindings: usb: tps6598x: Make the interrupts property optional 2022-04-22 5:07 ` Aswath Govindraju @ 2022-04-26 6:42 ` Roger Quadros 2022-04-26 6:57 ` Heikki Krogerus 0 siblings, 1 reply; 11+ messages in thread From: Roger Quadros @ 2022-04-26 6:42 UTC (permalink / raw) To: Aswath Govindraju, heikki.krogerus Cc: Vignesh Raghavendra, Kishon Vijay Abraham I, Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski, Heikki Krogerus, Sven Peter, Alyssa Rosenzweig, Hector Martin, Martin Kepplinger, Bryan O'Donoghue, linux-usb, devicetree, linux-kernel Hi, On 22/04/2022 08:07, Aswath Govindraju wrote: > Hi Roger, > > On 21/04/22 00:46, Roger Quadros wrote: >> Hi, >> >> On 18/04/2022 08:19, Aswath Govindraju wrote: >>> Hi Roger, >>> >>> On 14/04/22 23:40, Roger Quadros wrote: >>>> Hi, >>>> >>>> On 14/04/2022 11:31, Aswath Govindraju wrote: >>>>> Support for polling has been added in the driver, which will be used by >>>>> default if interrupts property is not populated. Therefore, remove >>>>> interrupts and interrupt-names from the required properties and add a note >>>>> under interrupts property describing the above support in driver. >>>>> >>>>> Suggested-by: Roger Quadros <rogerq@kernel.org> >>>> >>>> I did not suggest to make interrupts optional by default. >>>> >>>> What I suggested was that if a DT property exists to explicitly >>>> indicate polling mode then interrupts are not required. >>>> >>> >>> ohh okay, got it. However, may I know if adding a dt property to >>> indicate polling for aiding the driver, is the correct approach to model it? >>> >>> In terms of modelling hardware, as interrupts are not connected we are >>> not populating the interrupts property. Shouldn't that be all. If we are >>> adding a property explicitly to indicate polling that can be used by >>> driver, wouldn't that be a software aid being added in the device tree? >> >> The hardware (tps6598x chip) has an interrupt pin and is expected to be used >> in normal case. >> >> Some buggy boards might have forgot to connect it. We are adding polling mode only for these buggy boards. ;) >> So polling mode is an exception. >> > > Yes as you mentioned the interrupt line is expected to connected but > there could be cases where there are not enough pins on the SoC and > polling is used intentionally. In these cases this would be a feature > rather than a bug. I do not agree that this is a feature but a board defect. You can always use a GPIO expander to add more GPIOs than the SoC can provide. Type-C events are asynchronous and polling is a waste of CPU time. What will you do if system suspends and you need to wake up on Type-C status change? So polling mode is just an exception for the defective boards or could be used for debugging. > > Also, I feel like not adding interrupts property in the dt nodes will > indicate polling. My question is why are we adding an extra property > (which is being used only as an aid in the driver) when this feature can > be modeled by making interrupts property optional. Because interrupt property was not originally optional for this driver. I would like to hear what Heikki has to say about this. Any thoughts Heikki? cheers, -roger > > Thanks, > Aswath > >> cheers, >> -roger >> >>> >>> Thanks, >>> Aswath >>> >>>>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com> >>>>> --- >>>>> Documentation/devicetree/bindings/usb/ti,tps6598x.yaml | 4 ++-- >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml >>>>> index a4c53b1f1af3..1c4b8c6233e5 100644 >>>>> --- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml >>>>> +++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml >>>>> @@ -25,6 +25,8 @@ properties: >>>>> >>>>> interrupts: >>>>> maxItems: 1 >>>>> + description: >>>>> + If interrupts are not populated then by default polling will be used. >>>>> >>>>> interrupt-names: >>>>> items: >>>>> @@ -33,8 +35,6 @@ properties: >>>>> required: >>>>> - compatible >>>>> - reg >>>>> - - interrupts >>>>> - - interrupt-names >>>>> >>>>> additionalProperties: true >>>>> >>>> >>>> cheers, >>>> -roger > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] dt-bindings: usb: tps6598x: Make the interrupts property optional 2022-04-26 6:42 ` Roger Quadros @ 2022-04-26 6:57 ` Heikki Krogerus 2022-05-02 5:30 ` Aswath Govindraju 0 siblings, 1 reply; 11+ messages in thread From: Heikki Krogerus @ 2022-04-26 6:57 UTC (permalink / raw) To: Roger Quadros Cc: Aswath Govindraju, Vignesh Raghavendra, Kishon Vijay Abraham I, Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski, Sven Peter, Alyssa Rosenzweig, Hector Martin, Martin Kepplinger, Bryan O'Donoghue, linux-usb, devicetree, linux-kernel On Tue, Apr 26, 2022 at 09:42:31AM +0300, Roger Quadros wrote: > Hi, > > On 22/04/2022 08:07, Aswath Govindraju wrote: > > Hi Roger, > > > > On 21/04/22 00:46, Roger Quadros wrote: > >> Hi, > >> > >> On 18/04/2022 08:19, Aswath Govindraju wrote: > >>> Hi Roger, > >>> > >>> On 14/04/22 23:40, Roger Quadros wrote: > >>>> Hi, > >>>> > >>>> On 14/04/2022 11:31, Aswath Govindraju wrote: > >>>>> Support for polling has been added in the driver, which will be used by > >>>>> default if interrupts property is not populated. Therefore, remove > >>>>> interrupts and interrupt-names from the required properties and add a note > >>>>> under interrupts property describing the above support in driver. > >>>>> > >>>>> Suggested-by: Roger Quadros <rogerq@kernel.org> > >>>> > >>>> I did not suggest to make interrupts optional by default. > >>>> > >>>> What I suggested was that if a DT property exists to explicitly > >>>> indicate polling mode then interrupts are not required. > >>>> > >>> > >>> ohh okay, got it. However, may I know if adding a dt property to > >>> indicate polling for aiding the driver, is the correct approach to model it? > >>> > >>> In terms of modelling hardware, as interrupts are not connected we are > >>> not populating the interrupts property. Shouldn't that be all. If we are > >>> adding a property explicitly to indicate polling that can be used by > >>> driver, wouldn't that be a software aid being added in the device tree? > >> > >> The hardware (tps6598x chip) has an interrupt pin and is expected to be used > >> in normal case. > >> > >> Some buggy boards might have forgot to connect it. We are adding polling mode only for these buggy boards. ;) > >> So polling mode is an exception. > >> > > > > Yes as you mentioned the interrupt line is expected to connected but > > there could be cases where there are not enough pins on the SoC and > > polling is used intentionally. In these cases this would be a feature > > rather than a bug. > > I do not agree that this is a feature but a board defect. You can always use > a GPIO expander to add more GPIOs than the SoC can provide. > > Type-C events are asynchronous and polling is a waste of CPU time. > What will you do if system suspends and you need to wake up on Type-C > status change? > So polling mode is just an exception for the defective boards or could > be used for debugging. > > > > > Also, I feel like not adding interrupts property in the dt nodes will > > indicate polling. My question is why are we adding an extra property > > (which is being used only as an aid in the driver) when this feature can > > be modeled by making interrupts property optional. > > Because interrupt property was not originally optional for this driver. > > I would like to hear what Heikki has to say about this. > > Any thoughts Heikki? I think the question is generic. How should DT describe the connection/lack of connection? Rob should comment on this. thanks, > cheers, > -roger > > > > > Thanks, > > Aswath > > > >> cheers, > >> -roger > >> > >>> > >>> Thanks, > >>> Aswath > >>> > >>>>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com> > >>>>> --- > >>>>> Documentation/devicetree/bindings/usb/ti,tps6598x.yaml | 4 ++-- > >>>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>>> > >>>>> diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml > >>>>> index a4c53b1f1af3..1c4b8c6233e5 100644 > >>>>> --- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml > >>>>> +++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml > >>>>> @@ -25,6 +25,8 @@ properties: > >>>>> > >>>>> interrupts: > >>>>> maxItems: 1 > >>>>> + description: > >>>>> + If interrupts are not populated then by default polling will be used. > >>>>> > >>>>> interrupt-names: > >>>>> items: > >>>>> @@ -33,8 +35,6 @@ properties: > >>>>> required: > >>>>> - compatible > >>>>> - reg > >>>>> - - interrupts > >>>>> - - interrupt-names > >>>>> > >>>>> additionalProperties: true > >>>>> > >>>> > >>>> cheers, > >>>> -roger > > > > -- heikki ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] dt-bindings: usb: tps6598x: Make the interrupts property optional 2022-04-26 6:57 ` Heikki Krogerus @ 2022-05-02 5:30 ` Aswath Govindraju 2022-06-09 5:33 ` Aswath Govindraju 0 siblings, 1 reply; 11+ messages in thread From: Aswath Govindraju @ 2022-05-02 5:30 UTC (permalink / raw) To: Heikki Krogerus, Roger Quadros, Rob Herring Cc: Vignesh Raghavendra, Kishon Vijay Abraham I, Greg Kroah-Hartman, Krzysztof Kozlowski, Sven Peter, Alyssa Rosenzweig, Hector Martin, Martin Kepplinger, Bryan O'Donoghue, linux-usb, devicetree, linux-kernel Hi Rob, On 26/04/22 12:27, Heikki Krogerus wrote: > On Tue, Apr 26, 2022 at 09:42:31AM +0300, Roger Quadros wrote: >> Hi, >> >> On 22/04/2022 08:07, Aswath Govindraju wrote: >>> Hi Roger, >>> >>> On 21/04/22 00:46, Roger Quadros wrote: >>>> Hi, >>>> >>>> On 18/04/2022 08:19, Aswath Govindraju wrote: >>>>> Hi Roger, >>>>> >>>>> On 14/04/22 23:40, Roger Quadros wrote: >>>>>> Hi, >>>>>> >>>>>> On 14/04/2022 11:31, Aswath Govindraju wrote: >>>>>>> Support for polling has been added in the driver, which will be used by >>>>>>> default if interrupts property is not populated. Therefore, remove >>>>>>> interrupts and interrupt-names from the required properties and add a note >>>>>>> under interrupts property describing the above support in driver. >>>>>>> >>>>>>> Suggested-by: Roger Quadros <rogerq@kernel.org> >>>>>> >>>>>> I did not suggest to make interrupts optional by default. >>>>>> >>>>>> What I suggested was that if a DT property exists to explicitly >>>>>> indicate polling mode then interrupts are not required. >>>>>> >>>>> >>>>> ohh okay, got it. However, may I know if adding a dt property to >>>>> indicate polling for aiding the driver, is the correct approach to model it? >>>>> >>>>> In terms of modelling hardware, as interrupts are not connected we are >>>>> not populating the interrupts property. Shouldn't that be all. If we are >>>>> adding a property explicitly to indicate polling that can be used by >>>>> driver, wouldn't that be a software aid being added in the device tree? >>>> >>>> The hardware (tps6598x chip) has an interrupt pin and is expected to be used >>>> in normal case. >>>> >>>> Some buggy boards might have forgot to connect it. We are adding polling mode only for these buggy boards. ;) >>>> So polling mode is an exception. >>>> >>> >>> Yes as you mentioned the interrupt line is expected to connected but >>> there could be cases where there are not enough pins on the SoC and >>> polling is used intentionally. In these cases this would be a feature >>> rather than a bug. >> >> I do not agree that this is a feature but a board defect. You can always use >> a GPIO expander to add more GPIOs than the SoC can provide. >> >> Type-C events are asynchronous and polling is a waste of CPU time. >> What will you do if system suspends and you need to wake up on Type-C >> status change? >> So polling mode is just an exception for the defective boards or could >> be used for debugging. >> >>> >>> Also, I feel like not adding interrupts property in the dt nodes will >>> indicate polling. My question is why are we adding an extra property >>> (which is being used only as an aid in the driver) when this feature can >>> be modeled by making interrupts property optional. >> >> Because interrupt property was not originally optional for this driver. >> >> I would like to hear what Heikki has to say about this. >> >> Any thoughts Heikki? > > I think the question is generic. How should DT describe the > connection/lack of connection? Rob should comment on this. > A gentle ping regarding this. Thanks, Aswath > thanks, > > >> cheers, >> -roger >> >>> >>> Thanks, >>> Aswath >>> >>>> cheers, >>>> -roger >>>> >>>>> >>>>> Thanks, >>>>> Aswath >>>>> >>>>>>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com> >>>>>>> --- >>>>>>> Documentation/devicetree/bindings/usb/ti,tps6598x.yaml | 4 ++-- >>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>>>> >>>>>>> diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml >>>>>>> index a4c53b1f1af3..1c4b8c6233e5 100644 >>>>>>> --- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml >>>>>>> +++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml >>>>>>> @@ -25,6 +25,8 @@ properties: >>>>>>> >>>>>>> interrupts: >>>>>>> maxItems: 1 >>>>>>> + description: >>>>>>> + If interrupts are not populated then by default polling will be used. >>>>>>> >>>>>>> interrupt-names: >>>>>>> items: >>>>>>> @@ -33,8 +35,6 @@ properties: >>>>>>> required: >>>>>>> - compatible >>>>>>> - reg >>>>>>> - - interrupts >>>>>>> - - interrupt-names >>>>>>> >>>>>>> additionalProperties: true >>>>>>> >>>>>> >>>>>> cheers, >>>>>> -roger >>> >>> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] dt-bindings: usb: tps6598x: Make the interrupts property optional 2022-05-02 5:30 ` Aswath Govindraju @ 2022-06-09 5:33 ` Aswath Govindraju 0 siblings, 0 replies; 11+ messages in thread From: Aswath Govindraju @ 2022-06-09 5:33 UTC (permalink / raw) To: Heikki Krogerus, Roger Quadros, Rob Herring Cc: Vignesh Raghavendra, Kishon Vijay Abraham I, Greg Kroah-Hartman, Krzysztof Kozlowski, Sven Peter, Alyssa Rosenzweig, Hector Martin, Martin Kepplinger, Bryan O'Donoghue, linux-usb, devicetree, linux-kernel Hi Rob, On 02/05/22 11:00, Aswath Govindraju wrote: > Hi Rob, > > On 26/04/22 12:27, Heikki Krogerus wrote: >> On Tue, Apr 26, 2022 at 09:42:31AM +0300, Roger Quadros wrote: >>> Hi, >>> >>> On 22/04/2022 08:07, Aswath Govindraju wrote: >>>> Hi Roger, >>>> >>>> On 21/04/22 00:46, Roger Quadros wrote: >>>>> Hi, >>>>> >>>>> On 18/04/2022 08:19, Aswath Govindraju wrote: >>>>>> Hi Roger, >>>>>> >>>>>> On 14/04/22 23:40, Roger Quadros wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On 14/04/2022 11:31, Aswath Govindraju wrote: >>>>>>>> Support for polling has been added in the driver, which will be used by >>>>>>>> default if interrupts property is not populated. Therefore, remove >>>>>>>> interrupts and interrupt-names from the required properties and add a note >>>>>>>> under interrupts property describing the above support in driver. >>>>>>>> >>>>>>>> Suggested-by: Roger Quadros <rogerq@kernel.org> >>>>>>> >>>>>>> I did not suggest to make interrupts optional by default. >>>>>>> >>>>>>> What I suggested was that if a DT property exists to explicitly >>>>>>> indicate polling mode then interrupts are not required. >>>>>>> >>>>>> >>>>>> ohh okay, got it. However, may I know if adding a dt property to >>>>>> indicate polling for aiding the driver, is the correct approach to model it? >>>>>> >>>>>> In terms of modelling hardware, as interrupts are not connected we are >>>>>> not populating the interrupts property. Shouldn't that be all. If we are >>>>>> adding a property explicitly to indicate polling that can be used by >>>>>> driver, wouldn't that be a software aid being added in the device tree? >>>>> >>>>> The hardware (tps6598x chip) has an interrupt pin and is expected to be used >>>>> in normal case. >>>>> >>>>> Some buggy boards might have forgot to connect it. We are adding polling mode only for these buggy boards. ;) >>>>> So polling mode is an exception. >>>>> >>>> >>>> Yes as you mentioned the interrupt line is expected to connected but >>>> there could be cases where there are not enough pins on the SoC and >>>> polling is used intentionally. In these cases this would be a feature >>>> rather than a bug. >>> >>> I do not agree that this is a feature but a board defect. You can always use >>> a GPIO expander to add more GPIOs than the SoC can provide. >>> >>> Type-C events are asynchronous and polling is a waste of CPU time. >>> What will you do if system suspends and you need to wake up on Type-C >>> status change? >>> So polling mode is just an exception for the defective boards or could >>> be used for debugging. >>> >>>> >>>> Also, I feel like not adding interrupts property in the dt nodes will >>>> indicate polling. My question is why are we adding an extra property >>>> (which is being used only as an aid in the driver) when this feature can >>>> be modeled by making interrupts property optional. >>> >>> Because interrupt property was not originally optional for this driver. >>> >>> I would like to hear what Heikki has to say about this. >>> >>> Any thoughts Heikki? >> >> I think the question is generic. How should DT describe the >> connection/lack of connection? Rob should comment on this. >> > > A gentle ping regarding this. > A gentle ping regarding the above question. Thanks, Aswath > Thanks, > Aswath > >> thanks, >> >> >>> cheers, >>> -roger >>> >>>> >>>> Thanks, >>>> Aswath >>>> >>>>> cheers, >>>>> -roger >>>>> >>>>>> >>>>>> Thanks, >>>>>> Aswath >>>>>> >>>>>>>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com> >>>>>>>> --- >>>>>>>> Documentation/devicetree/bindings/usb/ti,tps6598x.yaml | 4 ++-- >>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>>>>>> >>>>>>>> diff --git a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml >>>>>>>> index a4c53b1f1af3..1c4b8c6233e5 100644 >>>>>>>> --- a/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml >>>>>>>> +++ b/Documentation/devicetree/bindings/usb/ti,tps6598x.yaml >>>>>>>> @@ -25,6 +25,8 @@ properties: >>>>>>>> >>>>>>>> interrupts: >>>>>>>> maxItems: 1 >>>>>>>> + description: >>>>>>>> + If interrupts are not populated then by default polling will be used. >>>>>>>> >>>>>>>> interrupt-names: >>>>>>>> items: >>>>>>>> @@ -33,8 +35,6 @@ properties: >>>>>>>> required: >>>>>>>> - compatible >>>>>>>> - reg >>>>>>>> - - interrupts >>>>>>>> - - interrupt-names >>>>>>>> >>>>>>>> additionalProperties: true >>>>>>>> >>>>>>> >>>>>>> cheers, >>>>>>> -roger >>>> >>>> >> -- Thanks, Aswath ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] usb: typec: tipd: Add support for polling interrupts status when interrupt line is not connected 2022-04-14 8:31 [PATCH 0/2] typec: tipd: Add support for polling Aswath Govindraju 2022-04-14 8:31 ` [PATCH 1/2] dt-bindings: usb: tps6598x: Make the interrupts property optional Aswath Govindraju @ 2022-04-14 8:31 ` Aswath Govindraju 1 sibling, 0 replies; 11+ messages in thread From: Aswath Govindraju @ 2022-04-14 8:31 UTC (permalink / raw) Cc: Vignesh Raghavendra, Roger Quadros, Kishon Vijay Abraham I, Aswath Govindraju, Greg Kroah-Hartman, Rob Herring, Krzysztof Kozlowski, Heikki Krogerus, Sven Peter, Hector Martin, Alyssa Rosenzweig, Jens Axboe, Bryan O'Donoghue, linux-usb, devicetree, linux-kernel In some cases the interrupt line from the pd controller may not be connected. In these cases, poll the status of various events. Suggested-by: Roger Quadros <rogerq@kernel.org> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com> --- drivers/usb/typec/tipd/core.c | 99 +++++++++++++++++++++++++++++++---- 1 file changed, 88 insertions(+), 11 deletions(-) diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c index 16b4560216ba..72fd586ac080 100644 --- a/drivers/usb/typec/tipd/core.c +++ b/drivers/usb/typec/tipd/core.c @@ -15,6 +15,8 @@ #include <linux/interrupt.h> #include <linux/usb/typec.h> #include <linux/usb/role.h> +#include <linux/workqueue.h> +#include <linux/devm-helpers.h> #include "tps6598x.h" #include "trace.h" @@ -93,6 +95,8 @@ struct tps6598x { struct power_supply *psy; struct power_supply_desc psy_desc; enum power_supply_usb_type usb_type; + + struct delayed_work wq_poll; }; static enum power_supply_property tps6598x_psy_props[] = { @@ -473,9 +477,8 @@ static void tps6598x_handle_plug_event(struct tps6598x *tps, u32 status) } } -static irqreturn_t cd321x_interrupt(int irq, void *data) +static int cd321x_handle_interrupt_status(struct tps6598x *tps) { - struct tps6598x *tps = data; u64 event; u32 status; int ret; @@ -513,14 +516,45 @@ static irqreturn_t cd321x_interrupt(int irq, void *data) err_unlock: mutex_unlock(&tps->lock); + if (ret) + return ret; + if (event) - return IRQ_HANDLED; - return IRQ_NONE; + return 0; + return 1; } -static irqreturn_t tps6598x_interrupt(int irq, void *data) +static irqreturn_t cd321x_interrupt(int irq, void *data) { struct tps6598x *tps = data; + int ret; + + ret = cd321x_handle_interrupt_status(tps); + if (ret) + return IRQ_NONE; + return IRQ_HANDLED; +} + +/* Time interval for Polling */ +#define POLL_INTERVAL 500 /* msecs */ +static void cd321x_poll_work(struct work_struct *work) +{ + struct tps6598x *tps = container_of(to_delayed_work(work), + struct tps6598x, wq_poll); + int ret; + + ret = cd321x_handle_interrupt_status(tps); + /* + * If there is an error while reading the interrupt registers + * then stop polling else, schedule another poll work item + */ + if (!(ret < 0)) + queue_delayed_work(system_power_efficient_wq, + &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL)); +} + +static int tps6598x_handle_interrupt_status(struct tps6598x *tps) +{ u64 event1; u64 event2; u32 status; @@ -561,9 +595,39 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data) err_unlock: mutex_unlock(&tps->lock); + if (ret) + return ret; + if (event1 | event2) - return IRQ_HANDLED; - return IRQ_NONE; + return 0; + return 1; +} + +static irqreturn_t tps6598x_interrupt(int irq, void *data) +{ + struct tps6598x *tps = data; + int ret; + + ret = tps6598x_handle_interrupt_status(tps); + if (ret) + return IRQ_NONE; + return IRQ_HANDLED; +} + +static void tps6598x_poll_work(struct work_struct *work) +{ + struct tps6598x *tps = container_of(to_delayed_work(work), + struct tps6598x, wq_poll); + int ret; + + ret = tps6598x_handle_interrupt_status(tps); + /* + * If there is an error while reading the interrupt registers + * then stop polling else, schedule another poll work item + */ + if (!(ret < 0)) + queue_delayed_work(system_power_efficient_wq, + &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL)); } static int tps6598x_check_mode(struct tps6598x *tps) @@ -704,6 +768,7 @@ static int devm_tps6598_psy_register(struct tps6598x *tps) static int tps6598x_probe(struct i2c_client *client) { irq_handler_t irq_handler = tps6598x_interrupt; + work_func_t work_poll_handler = tps6598x_poll_work; struct device_node *np = client->dev.of_node; struct typec_capability typec_cap = { }; struct tps6598x *tps; @@ -748,6 +813,7 @@ static int tps6598x_probe(struct i2c_client *client) APPLE_CD_REG_INT_PLUG_EVENT; irq_handler = cd321x_interrupt; + work_poll_handler = cd321x_poll_work; } else { /* Enable power status, data status and plug event interrupts */ mask1 = TPS_REG_INT_POWER_STATUS_UPDATE | @@ -842,10 +908,21 @@ static int tps6598x_probe(struct i2c_client *client) dev_err(&client->dev, "failed to register partner\n"); } - ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, - irq_handler, - IRQF_SHARED | IRQF_ONESHOT, - dev_name(&client->dev), tps); + if (client->irq) { + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL, + irq_handler, + IRQF_SHARED | IRQF_ONESHOT, + dev_name(&client->dev), tps); + } else { + dev_warn(&client->dev, "Unable to find the interrupt, switching to polling\n"); + ret = devm_delayed_work_autocancel(tps->dev, &tps->wq_poll, work_poll_handler); + if (ret) + dev_err(&client->dev, "error while initializing workqueue\n"); + else + queue_delayed_work(system_power_efficient_wq, &tps->wq_poll, + msecs_to_jiffies(POLL_INTERVAL)); + } + if (ret) { tps6598x_disconnect(tps, 0); typec_unregister_port(tps->port); -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-06-09 5:34 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-14 8:31 [PATCH 0/2] typec: tipd: Add support for polling Aswath Govindraju 2022-04-14 8:31 ` [PATCH 1/2] dt-bindings: usb: tps6598x: Make the interrupts property optional Aswath Govindraju 2022-04-14 18:10 ` Roger Quadros 2022-04-18 5:19 ` Aswath Govindraju 2022-04-20 19:16 ` Roger Quadros 2022-04-22 5:07 ` Aswath Govindraju 2022-04-26 6:42 ` Roger Quadros 2022-04-26 6:57 ` Heikki Krogerus 2022-05-02 5:30 ` Aswath Govindraju 2022-06-09 5:33 ` Aswath Govindraju 2022-04-14 8:31 ` [PATCH 2/2] usb: typec: tipd: Add support for polling interrupts status when interrupt line is not connected Aswath Govindraju
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).