On Thu, Feb 02, 2023, Thinh Nguyen wrote:
> On Thu, Feb 02, 2023, Linyu Yuan wrote:
> >
> > hi Thinh,
> >
> >
> > do you prefer below change ? will it be good for all cases ?
> >
> >
> > +static void dwc3_gadget_update_link_state(struct dwc3 *dwc,
> > + const struct dwc3_event_devt *event)
> > +{
> > + switch (event->type) {
> > + case DWC3_DEVICE_EVENT_HIBER_REQ:
> > + case DWC3_DEVICE_EVENT_LINK_STATUS_CHANGE:
> > + case DWC3_DEVICE_EVENT_SUSPEND:
> > + break;
> > + default:
> > + dwc->link_state = event->event_info & DWC3_LINK_STATE_MASK;
> > + break;
> > + }
> > +}
> > +
> > static void dwc3_gadget_interrupt(struct dwc3 *dwc,
> > const struct dwc3_event_devt *event)
> > {
> > + dwc3_gadget_update_link_state(dwc3, event);
> > +
> > switch (event->type)
> >
> >
>
> This would break the check in dwc3_gadget_suspend_interrupt(). However,
> I'm actually not sure why we had that check in the beginning. I suppose
> certain setup may trigger suspend event multiple time consecutively?
>
Actually, looking at it again, you're skipping updating the events
listed above and not for every event. So that should work. For some
reason, I thought you want to update the link_state for every new event.
However, this would complicate the driver. Now we have to remember when
to set the link_state and when not to, and when to check the previous
link_state. On top of that, dwc->link_state may not reflect the current
state of the controller either, which makes it more confusing.
Perhaps we should not update dwc->link_state outside of link state
change event, and just track whether we called gadget_driver->suspend()
to call the correspond resume() on wakeup? It can be tracked with
dwc->gadget_driver_is_suspended.
Thanks,
Thinh
