Re: [PATCH v9 06/10] usb: dwc3: qcom: Add support to read IRQ's related to multiport

[Johan Hovold]

On Sun, Jul 16, 2023 at 12:31:05AM +0530, Krishna Kurapati PSSNV wrote:
> On 7/14/2023 4:10 PM, Krishna Kurapati PSSNV wrote:
> > On 7/14/2023 2:35 PM, Johan Hovold wrote:

> >> I haven't had time to look at your latest replies yet, but as I already
> >> said when reviewing v9, it seems you should be using a common helper for
> >> non-mp and mp.

> >   The gist of my mail was to see if I can defer qcom probe when dwc3 
> > probe fails/or doesn't happen on of_plat_pop (which is logical) so that 
> > we can move setup_irq to after dwc3_register_core so that we know 
> > whether we are MP capable or not. This would help us move all IRQ 
> > reading into one function.

>   I see it is difficult to write a common helper. To do so, we need to 
> know whether the device is MP capable or not in advance. And since it is 
> not possible to know it before of_plat_pop is done, I see only few ways 
> to do it:
> 
> 1. Based on qcom node compatible string, I can read whether the device 
> is MP capable or not and get IRQ's accordingly.

See, it's not impossible. You can also determine whether you have a
multiport controller from looking at the interrupt names which are
indexed and distinct for MP.

> 2. Read the port_info in advance but it needs me to go through some DT 
> props and try getting this info. Or read xhci regs like we are doing in 
> core (which is not good). Also since some Dt props can be missing, is it 
> difficult to get the MP capability info before of_plat_pop is done.

That seem unnecessary currently, but long term we probably need to fix
the design of this driver and defer some setup using callbacks that are
called when the core driver probes. Perhaps now is the time to add such
functionality.

> 3. Remove IRQ handling completely. Just because the device has IRQ's 
> present, I don't see a point in adding them to bindings, and because we 
> added them to bindings, we are making a patch to read them (and since 
> this is a little challenging, the whole of multiport series is blocked 
> although I don't need wakeup support on these interrupts right away).

Again, no. The devicetree binding should describe the hardware
capabilities and that has nothing to do with whether you need this for
you current project or not.

> Can't we let the rest of the patches go through and let interrupt 
> handling for 2nd, 3rd and 4rth ports be taken care later ? I am asking 
> this because I want the rest of the patches which are in good shape now 
> (after fixing the nits mentioned) to get merged atleast. I will make 
> sure to add interrupt handling later in a different series once this is 
> merged once I send v10.

As I've explained in earlier mails, I don't think that is acceptable as
you'd be dumping your technical debt on the community which will be left
to clean up your mess.

> Or if there is a simpler way to do it, I would be happy to take any 
> suggestions and complete this missing part in this series itself.

Using the 'compatible' or 'interrupt-names' properties seems like the
easiest way to determine whether you have an MP controller or not.

Johan