[v3,2/3] drm/bridge: analogix_dp: use jiffies to simulate timeout loop
diff mbox

Message ID 1473414290-335-1-git-send-email-ykk@rock-chips.com
State New
Headers show

Commit Message

Yakir Yang Sept. 9, 2016, 9:44 a.m. UTC
Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Changes in v3:
- Suggested by Sean

Changes in v2: None

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  3 ++-
 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  | 18 +++++++++---------
 2 files changed, 11 insertions(+), 10 deletions(-)

Comments

Sean Paul Sept. 12, 2016, 1:51 p.m. UTC | #1
On Fri, Sep 9, 2016 at 5:44 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> ---
> Changes in v3:
> - Suggested by Sean
>
> Changes in v2: None
>
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  3 ++-
>  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  | 18 +++++++++---------
>  2 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> index a15f076..d564e90 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> @@ -16,10 +16,11 @@
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_dp_helper.h>
>
> -#define DP_TIMEOUT_LOOP_COUNT 100
>  #define MAX_CR_LOOP 5
>  #define MAX_EQ_LOOP 5
>
> +#define DP_TIMEOUT_LOOP_MS                     msecs_to_jiffies(1)

The name suggests the units here are ms, but you're storing jiffies.
Do the msecs_to_jiffies conversion down below.

> +
>  /* DP_MAX_LANE_COUNT */
>  #define DPCD_ENHANCED_FRAME_CAP(x)             (((x) >> 7) & 0x1)
>  #define DPCD_MAX_LANE_COUNT(x)                 ((x) & 0x1f)
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index a4d17b8..15a4cf0 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> @@ -335,7 +335,7 @@ void analogix_dp_set_analog_power_down(struct analogix_dp_device *dp,
>  void analogix_dp_init_analog_func(struct analogix_dp_device *dp)
>  {
>         u32 reg;
> -       int timeout_loop = 0;
> +       unsigned long timeout;
>
>         analogix_dp_set_analog_power_down(dp, POWER_ALL, 0);
>
> @@ -350,9 +350,9 @@ void analogix_dp_init_analog_func(struct analogix_dp_device *dp)
>         if (analogix_dp_get_pll_lock_status(dp) == PLL_UNLOCKED) {
>                 analogix_dp_set_pll_power_down(dp, 0);
>
> +               timeout = jiffies + DP_TIMEOUT_LOOP_MS;

timeout = jiffies + msecs_to_jiffies(DP_TIMEOUT_LOOP_MS);

>                 while (analogix_dp_get_pll_lock_status(dp) == PLL_UNLOCKED) {
> -                       timeout_loop++;
> -                       if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
> +                       if (time_after(jiffies, timeout)) {
>                                 dev_err(dp->dev, "failed to get pll lock status\n");
>                                 return;
>                         }
> @@ -501,7 +501,7 @@ int analogix_dp_start_aux_transaction(struct analogix_dp_device *dp)
>  {
>         int reg;
>         int retval = 0;
> -       int timeout_loop = 0;
> +       unsigned long timeout;
>
>         /* Enable AUX CH operation */
>         reg = readl(dp->reg_base + ANALOGIX_DP_AUX_CH_CTL_2);
> @@ -509,10 +509,10 @@ int analogix_dp_start_aux_transaction(struct analogix_dp_device *dp)
>         writel(reg, dp->reg_base + ANALOGIX_DP_AUX_CH_CTL_2);
>
>         /* Is AUX CH command reply received? */
> +       timeout = jiffies + DP_TIMEOUT_LOOP_MS;
>         reg = readl(dp->reg_base + ANALOGIX_DP_INT_STA);
>         while (!(reg & RPLY_RECEIV)) {
> -               timeout_loop++;
> -               if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
> +               if (time_after(jiffies, timeout)) {
>                         dev_err(dp->dev, "AUX CH command reply failed!\n");
>                         return -ETIMEDOUT;
>                 }
> @@ -1055,7 +1055,7 @@ ssize_t analogix_dp_transfer(struct analogix_dp_device *dp,
>  {
>         u32 reg;
>         u8 *buffer = msg->buffer;
> -       int timeout_loop = 0;
> +       unsigned long timeout;
>         unsigned int i;
>         int num_transferred = 0;
>
> @@ -1123,10 +1123,10 @@ ssize_t analogix_dp_transfer(struct analogix_dp_device *dp,
>
>         /* Is AUX CH command reply received? */
>         /* TODO: Wait for an interrupt instead of looping? */
> +       timeout = jiffies + DP_TIMEOUT_LOOP_MS;
>         reg = readl(dp->reg_base + ANALOGIX_DP_INT_STA);
>         while (!(reg & RPLY_RECEIV)) {
> -               timeout_loop++;
> -               if (timeout_loop > DP_TIMEOUT_LOOP_COUNT) {
> +               if (time_after(jiffies, timeout)) {
>                         dev_err(dp->dev, "AUX CH command reply failed!\n");
>                         return -ETIMEDOUT;
>                 }
> --
> 1.9.1
>
>
Yakir Yang Sept. 20, 2016, 2:18 a.m. UTC | #2
Hi Sean,


On 09/12/2016 09:51 PM, Sean Paul wrote:
> On Fri, Sep 9, 2016 at 5:44 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> ---
>> Changes in v3:
>> - Suggested by Sean
>>
>> Changes in v2: None
>>
>>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  3 ++-
>>   drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  | 18 +++++++++---------
>>   2 files changed, 11 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>> index a15f076..d564e90 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>> @@ -16,10 +16,11 @@
>>   #include <drm/drm_crtc.h>
>>   #include <drm/drm_dp_helper.h>
>>
>> -#define DP_TIMEOUT_LOOP_COUNT 100
>>   #define MAX_CR_LOOP 5
>>   #define MAX_EQ_LOOP 5
>>
>> +#define DP_TIMEOUT_LOOP_MS                     msecs_to_jiffies(1)
> The name suggests the units here are ms, but you're storing jiffies.
> Do the msecs_to_jiffies conversion down below.

I suddenly realized that 'analogix_dp_core.c' also used the 
'DP_TIMEOUT_LOOP_COUNT' macros, and 'analogix_dp_core.c' have four kinds 
of timeout,
   - DP_TIMEOUT_LOOP_COUNT * 1us
   - DP_TIMEOUT_LOOP_COUNT * 10us
   - DP_TIMEOUT_LOOP_COUNT * 100us
   - DP_TIMEOUT_LOOP_COUNT * 1000us

I may guess it's not necessary to replace the 'DP_TIMEOUT_LOOP_COUNT' 
now  :-)

- Yakir

>
>> +
>>   /* DP_MAX_LANE_COUNT */
>>   #define DPCD_ENHANCED_FRAME_CAP(x)             (((x) >> 7) & 0x1)
>>   #define DPCD_MAX_LANE_COUNT(x)                 ((x) & 0x1f)
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>> index a4d17b8..15a4cf0 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>> @@ -335,7 +335,7 @@ void analogix_dp_set_analog_power_down(struct analogix_dp_device *dp,
>>   void analogix_dp_init_analog_func(struct analogix_dp_device *dp)
>>   {
>>          u32 reg;
>> -       int timeout_loop = 0;
>> +       unsigned long timeout;
>>
>>          analogix_dp_set_analog_power_down(dp, POWER_ALL, 0);
>>
>> @@ -350,9 +350,9 @@ void analogix_dp_init_analog_func(struct analogix_dp_device *dp)
>>          if (analogix_dp_get_pll_lock_status(dp) == PLL_UNLOCKED) {
>>                  analogix_dp_set_pll_power_down(dp, 0);
>>
>> +               timeout = jiffies + DP_TIMEOUT_LOOP_MS;
> timeout = jiffies + msecs_to_jiffies(DP_TIMEOUT_LOOP_MS);
>
>>                  while (analogix_dp_get_pll_lock_status(dp) == PLL_UNLOCKED) {
>> -                       timeout_loop++;
>> -                       if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
>> +                       if (time_after(jiffies, timeout)) {
>>                                  dev_err(dp->dev, "failed to get pll lock status\n");
>>                                  return;
>>                          }
>> @@ -501,7 +501,7 @@ int analogix_dp_start_aux_transaction(struct analogix_dp_device *dp)
>>   {
>>          int reg;
>>          int retval = 0;
>> -       int timeout_loop = 0;
>> +       unsigned long timeout;
>>
>>          /* Enable AUX CH operation */
>>          reg = readl(dp->reg_base + ANALOGIX_DP_AUX_CH_CTL_2);
>> @@ -509,10 +509,10 @@ int analogix_dp_start_aux_transaction(struct analogix_dp_device *dp)
>>          writel(reg, dp->reg_base + ANALOGIX_DP_AUX_CH_CTL_2);
>>
>>          /* Is AUX CH command reply received? */
>> +       timeout = jiffies + DP_TIMEOUT_LOOP_MS;
>>          reg = readl(dp->reg_base + ANALOGIX_DP_INT_STA);
>>          while (!(reg & RPLY_RECEIV)) {
>> -               timeout_loop++;
>> -               if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
>> +               if (time_after(jiffies, timeout)) {
>>                          dev_err(dp->dev, "AUX CH command reply failed!\n");
>>                          return -ETIMEDOUT;
>>                  }
>> @@ -1055,7 +1055,7 @@ ssize_t analogix_dp_transfer(struct analogix_dp_device *dp,
>>   {
>>          u32 reg;
>>          u8 *buffer = msg->buffer;
>> -       int timeout_loop = 0;
>> +       unsigned long timeout;
>>          unsigned int i;
>>          int num_transferred = 0;
>>
>> @@ -1123,10 +1123,10 @@ ssize_t analogix_dp_transfer(struct analogix_dp_device *dp,
>>
>>          /* Is AUX CH command reply received? */
>>          /* TODO: Wait for an interrupt instead of looping? */
>> +       timeout = jiffies + DP_TIMEOUT_LOOP_MS;
>>          reg = readl(dp->reg_base + ANALOGIX_DP_INT_STA);
>>          while (!(reg & RPLY_RECEIV)) {
>> -               timeout_loop++;
>> -               if (timeout_loop > DP_TIMEOUT_LOOP_COUNT) {
>> +               if (time_after(jiffies, timeout)) {
>>                          dev_err(dp->dev, "AUX CH command reply failed!\n");
>>                          return -ETIMEDOUT;
>>                  }
>> --
>> 1.9.1
>>
>>
>
>

Patch
diff mbox

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
index a15f076..d564e90 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
@@ -16,10 +16,11 @@ 
 #include <drm/drm_crtc.h>
 #include <drm/drm_dp_helper.h>
 
-#define DP_TIMEOUT_LOOP_COUNT 100
 #define MAX_CR_LOOP 5
 #define MAX_EQ_LOOP 5
 
+#define DP_TIMEOUT_LOOP_MS			msecs_to_jiffies(1)
+
 /* DP_MAX_LANE_COUNT */
 #define DPCD_ENHANCED_FRAME_CAP(x)		(((x) >> 7) & 0x1)
 #define DPCD_MAX_LANE_COUNT(x)			((x) & 0x1f)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
index a4d17b8..15a4cf0 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
@@ -335,7 +335,7 @@  void analogix_dp_set_analog_power_down(struct analogix_dp_device *dp,
 void analogix_dp_init_analog_func(struct analogix_dp_device *dp)
 {
 	u32 reg;
-	int timeout_loop = 0;
+	unsigned long timeout;
 
 	analogix_dp_set_analog_power_down(dp, POWER_ALL, 0);
 
@@ -350,9 +350,9 @@  void analogix_dp_init_analog_func(struct analogix_dp_device *dp)
 	if (analogix_dp_get_pll_lock_status(dp) == PLL_UNLOCKED) {
 		analogix_dp_set_pll_power_down(dp, 0);
 
+		timeout = jiffies + DP_TIMEOUT_LOOP_MS;
 		while (analogix_dp_get_pll_lock_status(dp) == PLL_UNLOCKED) {
-			timeout_loop++;
-			if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
+			if (time_after(jiffies, timeout)) {
 				dev_err(dp->dev, "failed to get pll lock status\n");
 				return;
 			}
@@ -501,7 +501,7 @@  int analogix_dp_start_aux_transaction(struct analogix_dp_device *dp)
 {
 	int reg;
 	int retval = 0;
-	int timeout_loop = 0;
+	unsigned long timeout;
 
 	/* Enable AUX CH operation */
 	reg = readl(dp->reg_base + ANALOGIX_DP_AUX_CH_CTL_2);
@@ -509,10 +509,10 @@  int analogix_dp_start_aux_transaction(struct analogix_dp_device *dp)
 	writel(reg, dp->reg_base + ANALOGIX_DP_AUX_CH_CTL_2);
 
 	/* Is AUX CH command reply received? */
+	timeout = jiffies + DP_TIMEOUT_LOOP_MS;
 	reg = readl(dp->reg_base + ANALOGIX_DP_INT_STA);
 	while (!(reg & RPLY_RECEIV)) {
-		timeout_loop++;
-		if (DP_TIMEOUT_LOOP_COUNT < timeout_loop) {
+		if (time_after(jiffies, timeout)) {
 			dev_err(dp->dev, "AUX CH command reply failed!\n");
 			return -ETIMEDOUT;
 		}
@@ -1055,7 +1055,7 @@  ssize_t analogix_dp_transfer(struct analogix_dp_device *dp,
 {
 	u32 reg;
 	u8 *buffer = msg->buffer;
-	int timeout_loop = 0;
+	unsigned long timeout;
 	unsigned int i;
 	int num_transferred = 0;
 
@@ -1123,10 +1123,10 @@  ssize_t analogix_dp_transfer(struct analogix_dp_device *dp,
 
 	/* Is AUX CH command reply received? */
 	/* TODO: Wait for an interrupt instead of looping? */
+	timeout = jiffies + DP_TIMEOUT_LOOP_MS;
 	reg = readl(dp->reg_base + ANALOGIX_DP_INT_STA);
 	while (!(reg & RPLY_RECEIV)) {
-		timeout_loop++;
-		if (timeout_loop > DP_TIMEOUT_LOOP_COUNT) {
+		if (time_after(jiffies, timeout)) {
 			dev_err(dp->dev, "AUX CH command reply failed!\n");
 			return -ETIMEDOUT;
 		}