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

[Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>]

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,