Hi Paul,

> > --- a/drivers/staging/dwc2/core.c
> > +++ b/drivers/staging/dwc2/core.c
...
> > +   /* Enable common interrupts without disturbing host mode interrupts */
> > +   intmsk = readl(hsotg->regs + GINTMSK);
> > +   intmsk |= GINTSTS_DISCONNINT | GINTSTS_MODEMIS | GINTSTS_OTGINT |
> > +             GINTSTS_CONIDSTSCHNG | GINTSTS_WKUPINT | GINTSTS_USBSUSP |
> >               GINTSTS_SESSREQINT;
> 
> This should use the interrupt list that you just added in patch 8/10, right?

Well, the list is not necessarily the same.

For the common interrupts, the GINTSTS_RESTOREDONE interrupt is in
GINTMSK_COMMON, but it is never enabled. There is only some
dummy handling for this interrupt, so I'll submit a separate patch to
remove that dummy handling (just like for I2CINT). This should make
GINTMSK_COMMON identical to the above list.

However, for host-mode interrupts, there are a number of interrupts that
are not enabled in dwc2_enable_host_interrupts, but later on when
needed. They still need to be in GINTMSK_HOST, since they must be
handled by dwc2_handle_hcd_intr and disabled by
dwc2_disable_host_interrupts (both of which use GINTMSK_HOST).

Given that dwc2_enable_host_interrupts cannot simply use GINTMSK_HOST,
it makes sense to not use GINTMSK_COMMON in
dwc2_enable_common_interrupts either.

However, I did ponder about using these masks in the enable functions,
since it helps robustness if you cannot accidentally enable an interrupt
that is never disabled. How about the following:

 - in dwc2_enable_common_interrupts we just enable GINTMSK_COMMON
 - in dwc2_enable_host_interrupts we enable
   GINTMSK_HOST & ~(GINTSTS_SOF|GINTSTS_PTXFEMP|GINTSTS_NPTXFEMP)
   (e.g., writing a list of interrupts to enable later, instead of
   only specifying the interrupts to enable now). The status of
   GINTSTS_RXFLVL would need to be handled separately, as it is now, of
   course.

How does that look?

Gr.

Matthijs
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to