On 4/27/2020 9:59 AM, Vinod Koul wrote:
> On 23-04-20, 10:26, Wesley Cheng wrote:
>
>> +static int qcom_snps_hsphy_suspend(struct qcom_snps_hsphy *hsphy)
>> +{
>> + if (hsphy->suspended)
>> + return 0;
>> +
>> + dev_dbg(&hsphy->phy->dev, "Suspend QCOM SNPS PHY, mode = %d \n", hsphy->mode);
>> +
>> + if (hsphy->mode == PHY_MODE_USB_HOST) {
>> + /* Enable auto-resume to meet remote wakeup timing */
>> + qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_HS_PHY_CTRL2,
>> + USB2_AUTO_RESUME, USB2_AUTO_RESUME);
>> + usleep_range(500, 1000);
>> + qcom_snps_hsphy_write_mask(hsphy->base, USB2_PHY_USB_PHY_HS_PHY_CTRL2,
>> + 0, USB2_AUTO_RESUME);
>
> Kernel has a coding guideline where we try to "stick" to 80 char limit
> and is sometimes okay like debug logs. Above is not okay. Please fix it
> and run ./scripts/checkpatch.pl --strict on your patch and fix all
> errors. Warning and checks at your discretion using common sense. When
> in doubt do ask :)
>
Hi Vinod,
Thanks for the input. I do run the checkpatch script before sending
patches, and addressing the errors. However, seems this was tagged as a
warning instead. Will keep in mind to address as many warnings as I can
as well.
>> + }
>> +
>> + clk_disable_unprepare(hsphy->cfg_ahb_clk);
>> + hsphy->suspended = true;
>
> why do you need to track this?
>
More for debug purposes in case the RPM state becomes out of sync with
the expected PHY state. We've seen some situations during PM
suspend/resume testing where our RPM routines aren't executed, as PM
will disable RPM briefly. It would be nice to be able to catch these
situations after collecting our crash dumps.
>> +
>> + return 0;
>> +}
>> +
>> +static int qcom_snps_hsphy_resume(struct qcom_snps_hsphy *hsphy)
>> +{
>> + int ret = 0;
>
> superfluous init..
>
Agreed.
>> static int qcom_snps_hsphy_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> @@ -251,6 +333,14 @@ static int qcom_snps_hsphy_probe(struct platform_device *pdev)
>> return ret;
>> }
>>
>> + pm_runtime_set_active(dev);
>> + pm_runtime_enable(dev);
>
> would it not make sense to enable this after pjy in initialized?
>
Not sure we want to put this in the phy_init() callback, as we can't
guarantee how the driver registering the PHY will use it. It'll put the
requirement of having to call phy_exit() for every phy_init() sequence,
in order to avoid unbalanced disable_depth warnings from the RPM driver.
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
