[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <961b0ad4-baac-4ca4-bc2a-7dae6f129e6f@quicinc.com>
Date: Mon, 18 Sep 2023 13:12:42 +0530
From: Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
To: Konrad Dybcio <konrad.dybcio@...aro.org>,
Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Philipp Zabel <p.zabel@...gutronix.de>,
Andy Gross <agross@...nel.org>,
"Bjorn Andersson" <andersson@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@...aro.org>,
Felipe Balbi <balbi@...nel.org>,
Wesley Cheng <quic_wcheng@...cinc.com>,
Johan Hovold <johan@...nel.org>,
Mathias Nyman <mathias.nyman@...el.com>
CC: <linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
<quic_pkondeti@...cinc.com>, <quic_ppratap@...cinc.com>,
<quic_jackp@...cinc.com>, <ahalaney@...hat.com>,
<quic_shazhuss@...cinc.com>
Subject: Re: [PATCH v11 10/13] usb: dwc3: qcom: Add multiport suspend/resume
support for wrapper
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,
Powered by blists - more mailing lists