lkml.org 
[lkml]   [2020]   [Apr]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] phy: qcom-snps: Add runtime suspend and resume handlers
From
Date


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

\
 
 \ /
  Last update: 2020-04-27 22:33    [W:0.070 / U:1.592 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site