Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Message-ID: <961b0ad4-baac-4ca4-bc2a-7dae6f129e6f@quicinc.com>
Date:   Mon, 18 Sep 2023 13:12:42 +0530
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v11 10/13] usb: dwc3: qcom: Add multiport suspend/resume
 support for wrapper
Content-Language: en-US
To:     Konrad Dybcio <konrad.dybcio@linaro.org>,
        Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Andy Gross <agross@kernel.org>,
        "Bjorn Andersson" <andersson@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
        Felipe Balbi <balbi@kernel.org>,
        Wesley Cheng <quic_wcheng@quicinc.com>,
        Johan Hovold <johan@kernel.org>,
        Mathias Nyman <mathias.nyman@intel.com>
CC:     <linux-usb@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-msm@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <quic_pkondeti@quicinc.com>, <quic_ppratap@quicinc.com>,
        <quic_jackp@quicinc.com>, <ahalaney@redhat.com>,
        <quic_shazhuss@quicinc.com>
References: <20230828133033.11988-1-quic_kriskura@quicinc.com>
 <20230828133033.11988-11-quic_kriskura@quicinc.com>
 <825bc60b-2067-43e2-8b43-9d38b7cebf02@linaro.org>
From:   Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
In-Reply-To: <825bc60b-2067-43e2-8b43-9d38b7cebf02@linaro.org>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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,