Re: [PATCH v11 10/13] usb: dwc3: qcom: Add multiport suspend/resume support for wrapper

[Krishna Kurapati PSSNV]

On 9/15/2023 7:18 PM, Konrad Dybcio wrote:
> >   
> > -#define PWR_EVNT_IRQ_STAT_REG			0x58
> > +#define PWR_EVNT_IRQ1_STAT_REG			0x58
> > +#define PWR_EVNT_IRQ2_STAT_REG			0x1dc
> > +#define PWR_EVNT_IRQ3_STAT_REG			0x228
> > +#define PWR_EVNT_IRQ4_STAT_REG			0x238
> > +
> >   #define PWR_EVNT_LPM_IN_L2_MASK			BIT(4)
> >   #define PWR_EVNT_LPM_OUT_L2_MASK		BIT(5)
> >   
> > @@ -107,6 +111,19 @@ struct dwc3_qcom {
> >   	int			num_ports;
> >   };
> >   
> > +/*
> > + * SA8295 has 4 power event IRQ STAT registers to be checked
> > + * during suspend resume.
> > + */
> But this driver supports much more than just SA8295?
Yes. Other than SA8295, all single port controllers and SA8195(2 port 
controller), have these reigsters.

The rational behind adding this array was that depending on num_ports, 
any controller can access its required pwr_event_irq_stat register and 
loop in the suspend/resume code would take care of it. Perhaps I can 
change the comments to indicate that the array would be used by all 
controllers and not just SA8295.

> > +#define NUM_PWR_EVENT_STAT_REGS	4
> > +
> > +static u32 pwr_evnt_irq_stat_reg_offset[NUM_PWR_EVENT_STAT_REGS] = {
> > +	PWR_EVNT_IRQ1_STAT_REG,
> > +	PWR_EVNT_IRQ2_STAT_REG,
> > +	PWR_EVNT_IRQ3_STAT_REG,
> > +	PWR_EVNT_IRQ4_STAT_REG,
> > +};
> > +
> >   static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
> >   {
> >   	u32 reg;
> > @@ -440,15 +457,19 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
> >   
> >   static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
> >   {
> > +	u8 num_ports;
> Maybe I'm picky, but I'm not sure defining a variable for
> a single use of an object with a rather short name
> (qcom->num_ports) is justified, here and below..

Sure, will replace num_ports with (qcom->num_ports) and remove the extra 
variable.

Regards,
Krishna,