Re: [PATCH v3 3/4] drm/bridge: analogix_dp: add the PSR function support

2016-07-12 Thread Sean Paul 
On Thu, Jul 7, 2016 at 7:26 PM, Yakir Yang  wrote:
> Sean,
>
> Thanks for your review.
>
>
> On 07/02/2016 03:46 AM, Sean Paul wrote:
>>
>> On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang  wrote:
>>>
>>> The full name of PSR is Panel Self Refresh, panel device could refresh
>>> itself with the hardware framebuffer in panel, this would make lots of
>>> sense to save the power consumption.
>>>
>>> This patch have exported two symbols for platform driver to implement
>>> the PSR function in hardware side:
>>> - analogix_dp_active_psr()
>>> - analogix_dp_inactive_psr()
>>>
>>> Signed-off-by: Yakir Yang 
>>> ---
>>> Changes in v3:
>>> - split analogix_dp_enable_psr(), make it more clearly
>>>  analogix_dp_detect_sink_psr()
>>>  analogix_dp_enable_sink_psr()
>>> - remove some nosie register setting comments
>>>
>>> Changes in v2:
>>> - introduce in v2, splite the common Analogix DP changes out
>>>
>>>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 64
>>> ++
>>>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  4 ++
>>>   drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  | 54
>>> ++
>>>   drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h  | 28 ++
>>>   include/drm/bridge/analogix_dp.h   |  3 +
>>>   5 files changed, 153 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> index 32715da..b557097 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> @@ -97,6 +97,66 @@ static int analogix_dp_detect_hpd(struct
>>> analogix_dp_device *dp)
>>>  return 0;
>>>   }
>>>
>>> +int analogix_dp_active_psr(struct device *dev)
>>> +{
>>> +   struct analogix_dp_device *dp = dev_get_drvdata(dev);
>>> +
>>> +   if (!dp->psr_support)
>>> +   return -EINVAL;
>>> +
>>> +   analogix_dp_send_psr_spd(dp, EDP_VSC_PSR_STATE_ACTIVE |
>>> +EDP_VSC_PSR_CRC_VALUES_VALID);
>>> +   return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(analogix_dp_active_psr);
>>> +
>>> +int analogix_dp_inactive_psr(struct device *dev)
>>> +{
>>> +   struct analogix_dp_device *dp = dev_get_drvdata(dev);
>>> +
>>> +   if (!dp->psr_support)
>>> +   return -EINVAL;
>>> +
>>> +   analogix_dp_send_psr_spd(dp, 0);
>>> +   return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(analogix_dp_inactive_psr);
>>> +
>>> +static bool analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
>>> +{
>>> +   unsigned char psr_version;
>>> +
>>> +   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_SUPPORT,
>>> _version);
>>> +   dev_info(dp->dev, "Panel PSR version : %x\n", psr_version);
>>> +
>>
>> This info message is likely to be spammy since it's printed everytime
>> the panel toggle on. Perhaps downgrade to debug level.
>
>
> Okay, done.
>
>>> +   return (psr_version & DP_PSR_IS_SUPPORTED) ? true : false;
>>> +}
>>> +
>>> +static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
>>
>> Return type is int, but the function never fails and you don't check
>> the return value when calling it. Seems like this should be void.
>
>
> Done.
>
>>> +{
>>> +   unsigned char psr_en;
>>> +
>>> +   /* Disable psr function */
>>> +   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
>>> +   psr_en &= ~DP_PSR_ENABLE;
>>> +   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
>>> +
>>> +   /* Main-Link transmitter remains active during PSR active states
>>> */
>>> +   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
>>> +   psr_en = DP_PSR_MAIN_LINK_ACTIVE | DP_PSR_CRC_VERIFICATION;
>>
>> Why read psr_en if you're just going to overwrite it? Perhaps you meant |=
>> here.
>>
>
> Yes, it's my mistaken, no need to read the DP_PSR_EN_CFG, just configure it
> directly is enough.
>
>>> +   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
>>> +
>>> +   /* Enable psr function */
>>> +   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
>>> +   psr_en = DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE |
>>> +DP_PSR_CRC_VERIFICATION;
>>
>> Again, no need to read if you're just overwriting.
>
>
> Yes, ditto
>
>
>>> +   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
>>> +
>>> +   analogix_dp_enable_psr_crc(dp);
>>> +
>>> +   return 0;
>>> +}
>>> +
>>>   static unsigned char analogix_dp_calc_edid_check_sum(unsigned char
>>> *edid_data)
>>>   {
>>>  int i;
>>> @@ -921,6 +981,10 @@ static void analogix_dp_commit(struct
>>> analogix_dp_device *dp)
>>>
>>>  /* Enable video */
>>>  analogix_dp_start_video(dp);
>>> +
>>> +   dp->psr_support = analogix_dp_detect_sink_psr(dp);
>>> +   if (dp->psr_support)
>>> +   analogix_dp_enable_sink_psr(dp);
>>>   }
>>>

Re: [PATCH v3 3/4] drm/bridge: analogix_dp: add the PSR function support

2016-07-12 Thread Sean Paul 
On Thu, Jul 7, 2016 at 7:26 PM, Yakir Yang  wrote:
> Sean,
>
> Thanks for your review.
>
>
> On 07/02/2016 03:46 AM, Sean Paul wrote:
>>
>> On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang  wrote:
>>>
>>> The full name of PSR is Panel Self Refresh, panel device could refresh
>>> itself with the hardware framebuffer in panel, this would make lots of
>>> sense to save the power consumption.
>>>
>>> This patch have exported two symbols for platform driver to implement
>>> the PSR function in hardware side:
>>> - analogix_dp_active_psr()
>>> - analogix_dp_inactive_psr()
>>>
>>> Signed-off-by: Yakir Yang 
>>> ---
>>> Changes in v3:
>>> - split analogix_dp_enable_psr(), make it more clearly
>>>  analogix_dp_detect_sink_psr()
>>>  analogix_dp_enable_sink_psr()
>>> - remove some nosie register setting comments
>>>
>>> Changes in v2:
>>> - introduce in v2, splite the common Analogix DP changes out
>>>
>>>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 64
>>> ++
>>>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  4 ++
>>>   drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  | 54
>>> ++
>>>   drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h  | 28 ++
>>>   include/drm/bridge/analogix_dp.h   |  3 +
>>>   5 files changed, 153 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> index 32715da..b557097 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> @@ -97,6 +97,66 @@ static int analogix_dp_detect_hpd(struct
>>> analogix_dp_device *dp)
>>>  return 0;
>>>   }
>>>
>>> +int analogix_dp_active_psr(struct device *dev)
>>> +{
>>> +   struct analogix_dp_device *dp = dev_get_drvdata(dev);
>>> +
>>> +   if (!dp->psr_support)
>>> +   return -EINVAL;
>>> +
>>> +   analogix_dp_send_psr_spd(dp, EDP_VSC_PSR_STATE_ACTIVE |
>>> +EDP_VSC_PSR_CRC_VALUES_VALID);
>>> +   return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(analogix_dp_active_psr);
>>> +
>>> +int analogix_dp_inactive_psr(struct device *dev)
>>> +{
>>> +   struct analogix_dp_device *dp = dev_get_drvdata(dev);
>>> +
>>> +   if (!dp->psr_support)
>>> +   return -EINVAL;
>>> +
>>> +   analogix_dp_send_psr_spd(dp, 0);
>>> +   return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(analogix_dp_inactive_psr);
>>> +
>>> +static bool analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
>>> +{
>>> +   unsigned char psr_version;
>>> +
>>> +   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_SUPPORT,
>>> _version);
>>> +   dev_info(dp->dev, "Panel PSR version : %x\n", psr_version);
>>> +
>>
>> This info message is likely to be spammy since it's printed everytime
>> the panel toggle on. Perhaps downgrade to debug level.
>
>
> Okay, done.
>
>>> +   return (psr_version & DP_PSR_IS_SUPPORTED) ? true : false;
>>> +}
>>> +
>>> +static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
>>
>> Return type is int, but the function never fails and you don't check
>> the return value when calling it. Seems like this should be void.
>
>
> Done.
>
>>> +{
>>> +   unsigned char psr_en;
>>> +
>>> +   /* Disable psr function */
>>> +   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
>>> +   psr_en &= ~DP_PSR_ENABLE;
>>> +   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
>>> +
>>> +   /* Main-Link transmitter remains active during PSR active states
>>> */
>>> +   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
>>> +   psr_en = DP_PSR_MAIN_LINK_ACTIVE | DP_PSR_CRC_VERIFICATION;
>>
>> Why read psr_en if you're just going to overwrite it? Perhaps you meant |=
>> here.
>>
>
> Yes, it's my mistaken, no need to read the DP_PSR_EN_CFG, just configure it
> directly is enough.
>
>>> +   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
>>> +
>>> +   /* Enable psr function */
>>> +   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
>>> +   psr_en = DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE |
>>> +DP_PSR_CRC_VERIFICATION;
>>
>> Again, no need to read if you're just overwriting.
>
>
> Yes, ditto
>
>
>>> +   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
>>> +
>>> +   analogix_dp_enable_psr_crc(dp);
>>> +
>>> +   return 0;
>>> +}
>>> +
>>>   static unsigned char analogix_dp_calc_edid_check_sum(unsigned char
>>> *edid_data)
>>>   {
>>>  int i;
>>> @@ -921,6 +981,10 @@ static void analogix_dp_commit(struct
>>> analogix_dp_device *dp)
>>>
>>>  /* Enable video */
>>>  analogix_dp_start_video(dp);
>>> +
>>> +   dp->psr_support = analogix_dp_detect_sink_psr(dp);
>>> +   if (dp->psr_support)
>>> +   analogix_dp_enable_sink_psr(dp);
>>>   }
>>>
>>>   int analogix_dp_get_modes(struct drm_connector

Re: [PATCH v3 3/4] drm/bridge: analogix_dp: add the PSR function support

2016-07-07 Thread Yakir Yang 



On 07/08/2016 10:26 AM, Yakir Yang wrote:

Sean,

Thanks for your review.

On 07/02/2016 03:46 AM, Sean Paul wrote:

On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang  wrote:

The full name of PSR is Panel Self Refresh, panel device could refresh
itself with the hardware framebuffer in panel, this would make lots of
sense to save the power consumption.

This patch have exported two symbols for platform driver to implement
the PSR function in hardware side:
- analogix_dp_active_psr()
- analogix_dp_inactive_psr()

Signed-off-by: Yakir Yang 
---
Changes in v3:
- split analogix_dp_enable_psr(), make it more clearly
 analogix_dp_detect_sink_psr()
 analogix_dp_enable_sink_psr()
- remove some nosie register setting comments

Changes in v2:
- introduce in v2, splite the common Analogix DP changes out

  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 64 
++

  drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  4 ++
  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  | 54 
++

  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h  | 28 ++
  include/drm/bridge/analogix_dp.h   |  3 +
  5 files changed, 153 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c

index 32715da..b557097 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -97,6 +97,66 @@ static int analogix_dp_detect_hpd(struct 
analogix_dp_device *dp)

 return 0;
  }

+int analogix_dp_active_psr(struct device *dev)
+{
+   struct analogix_dp_device *dp = dev_get_drvdata(dev);
+
+   if (!dp->psr_support)
+   return -EINVAL;
+
+   analogix_dp_send_psr_spd(dp, EDP_VSC_PSR_STATE_ACTIVE |
+ EDP_VSC_PSR_CRC_VALUES_VALID);
+   return 0;
+}
+EXPORT_SYMBOL_GPL(analogix_dp_active_psr);
+
+int analogix_dp_inactive_psr(struct device *dev)
+{
+   struct analogix_dp_device *dp = dev_get_drvdata(dev);
+
+   if (!dp->psr_support)
+   return -EINVAL;
+
+   analogix_dp_send_psr_spd(dp, 0);
+   return 0;
+}
+EXPORT_SYMBOL_GPL(analogix_dp_inactive_psr);
+
+static bool analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
+{
+   unsigned char psr_version;
+
+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_SUPPORT, 
_version);

+   dev_info(dp->dev, "Panel PSR version : %x\n", psr_version);
+

This info message is likely to be spammy since it's printed everytime
the panel toggle on. Perhaps downgrade to debug level.


Okay, done.


+   return (psr_version & DP_PSR_IS_SUPPORTED) ? true : false;
+}
+
+static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)

Return type is int, but the function never fails and you don't check
the return value when calling it. Seems like this should be void.


Done.


+{
+   unsigned char psr_en;
+
+   /* Disable psr function */
+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
+   psr_en &= ~DP_PSR_ENABLE;
+   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
+
+   /* Main-Link transmitter remains active during PSR active 
states */

+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
+   psr_en = DP_PSR_MAIN_LINK_ACTIVE | DP_PSR_CRC_VERIFICATION;
Why read psr_en if you're just going to overwrite it? Perhaps you 
meant |= here.




Yes, it's my mistaken, no need to read the DP_PSR_EN_CFG, just 
configure it directly is enough.



+ analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
+
+   /* Enable psr function */
+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
+   psr_en = DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE |
+DP_PSR_CRC_VERIFICATION;

Again, no need to read if you're just overwriting.


Yes, ditto


+ analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
+
+   analogix_dp_enable_psr_crc(dp);
+
+   return 0;
+}
+
  static unsigned char analogix_dp_calc_edid_check_sum(unsigned char 
*edid_data)

  {
 int i;
@@ -921,6 +981,10 @@ static void analogix_dp_commit(struct 
analogix_dp_device *dp)


 /* Enable video */
 analogix_dp_start_video(dp);
+
+   dp->psr_support = analogix_dp_detect_sink_psr(dp);
+   if (dp->psr_support)
+   analogix_dp_enable_sink_psr(dp);
  }

  int analogix_dp_get_modes(struct drm_connector *connector)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h

index b456380..6ca5dde 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
@@ -177,6 +177,7 @@ struct analogix_dp_device {
 int hpd_gpio;
 boolforce_hpd;
 unsigned char   edid[EDID_BLOCK_LENGTH * 2];
+   boolpsr_support;

Re: [PATCH v3 3/4] drm/bridge: analogix_dp: add the PSR function support

2016-07-07 Thread Yakir Yang 



On 07/08/2016 10:26 AM, Yakir Yang wrote:

Sean,

Thanks for your review.

On 07/02/2016 03:46 AM, Sean Paul wrote:

On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang  wrote:

The full name of PSR is Panel Self Refresh, panel device could refresh
itself with the hardware framebuffer in panel, this would make lots of
sense to save the power consumption.

This patch have exported two symbols for platform driver to implement
the PSR function in hardware side:
- analogix_dp_active_psr()
- analogix_dp_inactive_psr()

Signed-off-by: Yakir Yang 
---
Changes in v3:
- split analogix_dp_enable_psr(), make it more clearly
 analogix_dp_detect_sink_psr()
 analogix_dp_enable_sink_psr()
- remove some nosie register setting comments

Changes in v2:
- introduce in v2, splite the common Analogix DP changes out

  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 64 
++

  drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  4 ++
  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  | 54 
++

  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h  | 28 ++
  include/drm/bridge/analogix_dp.h   |  3 +
  5 files changed, 153 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c

index 32715da..b557097 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -97,6 +97,66 @@ static int analogix_dp_detect_hpd(struct 
analogix_dp_device *dp)

 return 0;
  }

+int analogix_dp_active_psr(struct device *dev)
+{
+   struct analogix_dp_device *dp = dev_get_drvdata(dev);
+
+   if (!dp->psr_support)
+   return -EINVAL;
+
+   analogix_dp_send_psr_spd(dp, EDP_VSC_PSR_STATE_ACTIVE |
+ EDP_VSC_PSR_CRC_VALUES_VALID);
+   return 0;
+}
+EXPORT_SYMBOL_GPL(analogix_dp_active_psr);
+
+int analogix_dp_inactive_psr(struct device *dev)
+{
+   struct analogix_dp_device *dp = dev_get_drvdata(dev);
+
+   if (!dp->psr_support)
+   return -EINVAL;
+
+   analogix_dp_send_psr_spd(dp, 0);
+   return 0;
+}
+EXPORT_SYMBOL_GPL(analogix_dp_inactive_psr);
+
+static bool analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
+{
+   unsigned char psr_version;
+
+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_SUPPORT, 
_version);

+   dev_info(dp->dev, "Panel PSR version : %x\n", psr_version);
+

This info message is likely to be spammy since it's printed everytime
the panel toggle on. Perhaps downgrade to debug level.


Okay, done.


+   return (psr_version & DP_PSR_IS_SUPPORTED) ? true : false;
+}
+
+static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)

Return type is int, but the function never fails and you don't check
the return value when calling it. Seems like this should be void.


Done.


+{
+   unsigned char psr_en;
+
+   /* Disable psr function */
+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
+   psr_en &= ~DP_PSR_ENABLE;
+   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
+
+   /* Main-Link transmitter remains active during PSR active 
states */

+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
+   psr_en = DP_PSR_MAIN_LINK_ACTIVE | DP_PSR_CRC_VERIFICATION;
Why read psr_en if you're just going to overwrite it? Perhaps you 
meant |= here.




Yes, it's my mistaken, no need to read the DP_PSR_EN_CFG, just 
configure it directly is enough.



+ analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
+
+   /* Enable psr function */
+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
+   psr_en = DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE |
+DP_PSR_CRC_VERIFICATION;

Again, no need to read if you're just overwriting.


Yes, ditto


+ analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
+
+   analogix_dp_enable_psr_crc(dp);
+
+   return 0;
+}
+
  static unsigned char analogix_dp_calc_edid_check_sum(unsigned char 
*edid_data)

  {
 int i;
@@ -921,6 +981,10 @@ static void analogix_dp_commit(struct 
analogix_dp_device *dp)


 /* Enable video */
 analogix_dp_start_video(dp);
+
+   dp->psr_support = analogix_dp_detect_sink_psr(dp);
+   if (dp->psr_support)
+   analogix_dp_enable_sink_psr(dp);
  }

  int analogix_dp_get_modes(struct drm_connector *connector)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h

index b456380..6ca5dde 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
@@ -177,6 +177,7 @@ struct analogix_dp_device {
 int hpd_gpio;
 boolforce_hpd;
 unsigned char   edid[EDID_BLOCK_LENGTH * 2];
+   boolpsr_support;

 struct analogix_dp_plat_data

Re: [PATCH v3 3/4] drm/bridge: analogix_dp: add the PSR function support

2016-07-07 Thread Yakir Yang 

Sean,

Thanks for your review.

On 07/02/2016 03:46 AM, Sean Paul wrote:

On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang  wrote:

The full name of PSR is Panel Self Refresh, panel device could refresh
itself with the hardware framebuffer in panel, this would make lots of
sense to save the power consumption.

This patch have exported two symbols for platform driver to implement
the PSR function in hardware side:
- analogix_dp_active_psr()
- analogix_dp_inactive_psr()

Signed-off-by: Yakir Yang 
---
Changes in v3:
- split analogix_dp_enable_psr(), make it more clearly
 analogix_dp_detect_sink_psr()
 analogix_dp_enable_sink_psr()
- remove some nosie register setting comments

Changes in v2:
- introduce in v2, splite the common Analogix DP changes out

  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 64 ++
  drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  4 ++
  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  | 54 ++
  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h  | 28 ++
  include/drm/bridge/analogix_dp.h   |  3 +
  5 files changed, 153 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 32715da..b557097 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -97,6 +97,66 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device 
*dp)
 return 0;
  }

+int analogix_dp_active_psr(struct device *dev)
+{
+   struct analogix_dp_device *dp = dev_get_drvdata(dev);
+
+   if (!dp->psr_support)
+   return -EINVAL;
+
+   analogix_dp_send_psr_spd(dp, EDP_VSC_PSR_STATE_ACTIVE |
+EDP_VSC_PSR_CRC_VALUES_VALID);
+   return 0;
+}
+EXPORT_SYMBOL_GPL(analogix_dp_active_psr);
+
+int analogix_dp_inactive_psr(struct device *dev)
+{
+   struct analogix_dp_device *dp = dev_get_drvdata(dev);
+
+   if (!dp->psr_support)
+   return -EINVAL;
+
+   analogix_dp_send_psr_spd(dp, 0);
+   return 0;
+}
+EXPORT_SYMBOL_GPL(analogix_dp_inactive_psr);
+
+static bool analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
+{
+   unsigned char psr_version;
+
+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_SUPPORT, _version);
+   dev_info(dp->dev, "Panel PSR version : %x\n", psr_version);
+

This info message is likely to be spammy since it's printed everytime
the panel toggle on. Perhaps downgrade to debug level.


Okay, done.


+   return (psr_version & DP_PSR_IS_SUPPORTED) ? true : false;
+}
+
+static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)

Return type is int, but the function never fails and you don't check
the return value when calling it. Seems like this should be void.


Done.


+{
+   unsigned char psr_en;
+
+   /* Disable psr function */
+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
+   psr_en &= ~DP_PSR_ENABLE;
+   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
+
+   /* Main-Link transmitter remains active during PSR active states */
+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
+   psr_en = DP_PSR_MAIN_LINK_ACTIVE | DP_PSR_CRC_VERIFICATION;

Why read psr_en if you're just going to overwrite it? Perhaps you meant |= here.



Yes, it's my mistaken, no need to read the DP_PSR_EN_CFG, just configure 
it directly is enough.



+   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
+
+   /* Enable psr function */
+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
+   psr_en = DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE |
+DP_PSR_CRC_VERIFICATION;

Again, no need to read if you're just overwriting.


Yes, ditto


+   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
+
+   analogix_dp_enable_psr_crc(dp);
+
+   return 0;
+}
+
  static unsigned char analogix_dp_calc_edid_check_sum(unsigned char *edid_data)
  {
 int i;
@@ -921,6 +981,10 @@ static void analogix_dp_commit(struct analogix_dp_device 
*dp)

 /* Enable video */
 analogix_dp_start_video(dp);
+
+   dp->psr_support = analogix_dp_detect_sink_psr(dp);
+   if (dp->psr_support)
+   analogix_dp_enable_sink_psr(dp);
  }

  int analogix_dp_get_modes(struct drm_connector *connector)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
index b456380..6ca5dde 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
@@ -177,6 +177,7 @@ struct analogix_dp_device {
 int hpd_gpio;
 boolforce_hpd;
 unsigned char   edid[EDID_BLOCK_LENGTH * 2];
+   boolpsr_support;

 struct

Re: [PATCH v3 3/4] drm/bridge: analogix_dp: add the PSR function support

2016-07-07 Thread Yakir Yang 

Sean,

Thanks for your review.

On 07/02/2016 03:46 AM, Sean Paul wrote:

On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang  wrote:

The full name of PSR is Panel Self Refresh, panel device could refresh
itself with the hardware framebuffer in panel, this would make lots of
sense to save the power consumption.

This patch have exported two symbols for platform driver to implement
the PSR function in hardware side:
- analogix_dp_active_psr()
- analogix_dp_inactive_psr()

Signed-off-by: Yakir Yang 
---
Changes in v3:
- split analogix_dp_enable_psr(), make it more clearly
 analogix_dp_detect_sink_psr()
 analogix_dp_enable_sink_psr()
- remove some nosie register setting comments

Changes in v2:
- introduce in v2, splite the common Analogix DP changes out

  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 64 ++
  drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  4 ++
  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  | 54 ++
  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h  | 28 ++
  include/drm/bridge/analogix_dp.h   |  3 +
  5 files changed, 153 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 32715da..b557097 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -97,6 +97,66 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device 
*dp)
 return 0;
  }

+int analogix_dp_active_psr(struct device *dev)
+{
+   struct analogix_dp_device *dp = dev_get_drvdata(dev);
+
+   if (!dp->psr_support)
+   return -EINVAL;
+
+   analogix_dp_send_psr_spd(dp, EDP_VSC_PSR_STATE_ACTIVE |
+EDP_VSC_PSR_CRC_VALUES_VALID);
+   return 0;
+}
+EXPORT_SYMBOL_GPL(analogix_dp_active_psr);
+
+int analogix_dp_inactive_psr(struct device *dev)
+{
+   struct analogix_dp_device *dp = dev_get_drvdata(dev);
+
+   if (!dp->psr_support)
+   return -EINVAL;
+
+   analogix_dp_send_psr_spd(dp, 0);
+   return 0;
+}
+EXPORT_SYMBOL_GPL(analogix_dp_inactive_psr);
+
+static bool analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
+{
+   unsigned char psr_version;
+
+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_SUPPORT, _version);
+   dev_info(dp->dev, "Panel PSR version : %x\n", psr_version);
+

This info message is likely to be spammy since it's printed everytime
the panel toggle on. Perhaps downgrade to debug level.


Okay, done.


+   return (psr_version & DP_PSR_IS_SUPPORTED) ? true : false;
+}
+
+static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)

Return type is int, but the function never fails and you don't check
the return value when calling it. Seems like this should be void.


Done.


+{
+   unsigned char psr_en;
+
+   /* Disable psr function */
+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
+   psr_en &= ~DP_PSR_ENABLE;
+   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
+
+   /* Main-Link transmitter remains active during PSR active states */
+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
+   psr_en = DP_PSR_MAIN_LINK_ACTIVE | DP_PSR_CRC_VERIFICATION;

Why read psr_en if you're just going to overwrite it? Perhaps you meant |= here.



Yes, it's my mistaken, no need to read the DP_PSR_EN_CFG, just configure 
it directly is enough.



+   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
+
+   /* Enable psr function */
+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
+   psr_en = DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE |
+DP_PSR_CRC_VERIFICATION;

Again, no need to read if you're just overwriting.


Yes, ditto


+   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
+
+   analogix_dp_enable_psr_crc(dp);
+
+   return 0;
+}
+
  static unsigned char analogix_dp_calc_edid_check_sum(unsigned char *edid_data)
  {
 int i;
@@ -921,6 +981,10 @@ static void analogix_dp_commit(struct analogix_dp_device 
*dp)

 /* Enable video */
 analogix_dp_start_video(dp);
+
+   dp->psr_support = analogix_dp_detect_sink_psr(dp);
+   if (dp->psr_support)
+   analogix_dp_enable_sink_psr(dp);
  }

  int analogix_dp_get_modes(struct drm_connector *connector)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
index b456380..6ca5dde 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
@@ -177,6 +177,7 @@ struct analogix_dp_device {
 int hpd_gpio;
 boolforce_hpd;
 unsigned char   edid[EDID_BLOCK_LENGTH * 2];
+   boolpsr_support;

 struct analogix_dp_plat_data *plat_data;
  };
@@ -278,4

Re: [PATCH v3 3/4] drm/bridge: analogix_dp: add the PSR function support

2016-07-01 Thread Sean Paul 
On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang  wrote:
> The full name of PSR is Panel Self Refresh, panel device could refresh
> itself with the hardware framebuffer in panel, this would make lots of
> sense to save the power consumption.
>
> This patch have exported two symbols for platform driver to implement
> the PSR function in hardware side:
> - analogix_dp_active_psr()
> - analogix_dp_inactive_psr()
>
> Signed-off-by: Yakir Yang 
> ---
> Changes in v3:
> - split analogix_dp_enable_psr(), make it more clearly
> analogix_dp_detect_sink_psr()
> analogix_dp_enable_sink_psr()
> - remove some nosie register setting comments
>
> Changes in v2:
> - introduce in v2, splite the common Analogix DP changes out
>
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 64 
> ++
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  4 ++
>  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  | 54 ++
>  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h  | 28 ++
>  include/drm/bridge/analogix_dp.h   |  3 +
>  5 files changed, 153 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 32715da..b557097 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -97,6 +97,66 @@ static int analogix_dp_detect_hpd(struct 
> analogix_dp_device *dp)
> return 0;
>  }
>
> +int analogix_dp_active_psr(struct device *dev)
> +{
> +   struct analogix_dp_device *dp = dev_get_drvdata(dev);
> +
> +   if (!dp->psr_support)
> +   return -EINVAL;
> +
> +   analogix_dp_send_psr_spd(dp, EDP_VSC_PSR_STATE_ACTIVE |
> +EDP_VSC_PSR_CRC_VALUES_VALID);
> +   return 0;
> +}
> +EXPORT_SYMBOL_GPL(analogix_dp_active_psr);
> +
> +int analogix_dp_inactive_psr(struct device *dev)
> +{
> +   struct analogix_dp_device *dp = dev_get_drvdata(dev);
> +
> +   if (!dp->psr_support)
> +   return -EINVAL;
> +
> +   analogix_dp_send_psr_spd(dp, 0);
> +   return 0;
> +}
> +EXPORT_SYMBOL_GPL(analogix_dp_inactive_psr);
> +
> +static bool analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
> +{
> +   unsigned char psr_version;
> +
> +   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_SUPPORT, _version);
> +   dev_info(dp->dev, "Panel PSR version : %x\n", psr_version);
> +

This info message is likely to be spammy since it's printed everytime
the panel toggle on. Perhaps downgrade to debug level.

> +   return (psr_version & DP_PSR_IS_SUPPORTED) ? true : false;
> +}
> +
> +static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)

Return type is int, but the function never fails and you don't check
the return value when calling it. Seems like this should be void.

> +{
> +   unsigned char psr_en;
> +
> +   /* Disable psr function */
> +   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
> +   psr_en &= ~DP_PSR_ENABLE;
> +   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
> +
> +   /* Main-Link transmitter remains active during PSR active states */
> +   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
> +   psr_en = DP_PSR_MAIN_LINK_ACTIVE | DP_PSR_CRC_VERIFICATION;

Why read psr_en if you're just going to overwrite it? Perhaps you meant |= here.

> +   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
> +
> +   /* Enable psr function */
> +   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
> +   psr_en = DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE |
> +DP_PSR_CRC_VERIFICATION;

Again, no need to read if you're just overwriting.

> +   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
> +
> +   analogix_dp_enable_psr_crc(dp);
> +
> +   return 0;
> +}
> +
>  static unsigned char analogix_dp_calc_edid_check_sum(unsigned char 
> *edid_data)
>  {
> int i;
> @@ -921,6 +981,10 @@ static void analogix_dp_commit(struct analogix_dp_device 
> *dp)
>
> /* Enable video */
> analogix_dp_start_video(dp);
> +
> +   dp->psr_support = analogix_dp_detect_sink_psr(dp);
> +   if (dp->psr_support)
> +   analogix_dp_enable_sink_psr(dp);
>  }
>
>  int analogix_dp_get_modes(struct drm_connector *connector)
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h 
> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> index b456380..6ca5dde 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> @@ -177,6 +177,7 @@ struct analogix_dp_device {
> int hpd_gpio;
> boolforce_hpd;
> unsigned char   edid[EDID_BLOCK_LENGTH * 2];
> +   boolpsr_support;
>
>

Re: [PATCH v3 3/4] drm/bridge: analogix_dp: add the PSR function support

2016-07-01 Thread Sean Paul 
On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang  wrote:
> The full name of PSR is Panel Self Refresh, panel device could refresh
> itself with the hardware framebuffer in panel, this would make lots of
> sense to save the power consumption.
>
> This patch have exported two symbols for platform driver to implement
> the PSR function in hardware side:
> - analogix_dp_active_psr()
> - analogix_dp_inactive_psr()
>
> Signed-off-by: Yakir Yang 
> ---
> Changes in v3:
> - split analogix_dp_enable_psr(), make it more clearly
> analogix_dp_detect_sink_psr()
> analogix_dp_enable_sink_psr()
> - remove some nosie register setting comments
>
> Changes in v2:
> - introduce in v2, splite the common Analogix DP changes out
>
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 64 
> ++
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  4 ++
>  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  | 54 ++
>  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h  | 28 ++
>  include/drm/bridge/analogix_dp.h   |  3 +
>  5 files changed, 153 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 32715da..b557097 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -97,6 +97,66 @@ static int analogix_dp_detect_hpd(struct 
> analogix_dp_device *dp)
> return 0;
>  }
>
> +int analogix_dp_active_psr(struct device *dev)
> +{
> +   struct analogix_dp_device *dp = dev_get_drvdata(dev);
> +
> +   if (!dp->psr_support)
> +   return -EINVAL;
> +
> +   analogix_dp_send_psr_spd(dp, EDP_VSC_PSR_STATE_ACTIVE |
> +EDP_VSC_PSR_CRC_VALUES_VALID);
> +   return 0;
> +}
> +EXPORT_SYMBOL_GPL(analogix_dp_active_psr);
> +
> +int analogix_dp_inactive_psr(struct device *dev)
> +{
> +   struct analogix_dp_device *dp = dev_get_drvdata(dev);
> +
> +   if (!dp->psr_support)
> +   return -EINVAL;
> +
> +   analogix_dp_send_psr_spd(dp, 0);
> +   return 0;
> +}
> +EXPORT_SYMBOL_GPL(analogix_dp_inactive_psr);
> +
> +static bool analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
> +{
> +   unsigned char psr_version;
> +
> +   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_SUPPORT, _version);
> +   dev_info(dp->dev, "Panel PSR version : %x\n", psr_version);
> +

This info message is likely to be spammy since it's printed everytime
the panel toggle on. Perhaps downgrade to debug level.

> +   return (psr_version & DP_PSR_IS_SUPPORTED) ? true : false;
> +}
> +
> +static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)

Return type is int, but the function never fails and you don't check
the return value when calling it. Seems like this should be void.

> +{
> +   unsigned char psr_en;
> +
> +   /* Disable psr function */
> +   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
> +   psr_en &= ~DP_PSR_ENABLE;
> +   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
> +
> +   /* Main-Link transmitter remains active during PSR active states */
> +   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
> +   psr_en = DP_PSR_MAIN_LINK_ACTIVE | DP_PSR_CRC_VERIFICATION;

Why read psr_en if you're just going to overwrite it? Perhaps you meant |= here.

> +   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
> +
> +   /* Enable psr function */
> +   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
> +   psr_en = DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE |
> +DP_PSR_CRC_VERIFICATION;

Again, no need to read if you're just overwriting.

> +   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
> +
> +   analogix_dp_enable_psr_crc(dp);
> +
> +   return 0;
> +}
> +
>  static unsigned char analogix_dp_calc_edid_check_sum(unsigned char 
> *edid_data)
>  {
> int i;
> @@ -921,6 +981,10 @@ static void analogix_dp_commit(struct analogix_dp_device 
> *dp)
>
> /* Enable video */
> analogix_dp_start_video(dp);
> +
> +   dp->psr_support = analogix_dp_detect_sink_psr(dp);
> +   if (dp->psr_support)
> +   analogix_dp_enable_sink_psr(dp);
>  }
>
>  int analogix_dp_get_modes(struct drm_connector *connector)
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h 
> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> index b456380..6ca5dde 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> @@ -177,6 +177,7 @@ struct analogix_dp_device {
> int hpd_gpio;
> boolforce_hpd;
> unsigned char   edid[EDID_BLOCK_LENGTH * 2];
> +   boolpsr_support;
>
> struct analogix_dp_plat_data *plat_data;
>  };

[PATCH v3 3/4] drm/bridge: analogix_dp: add the PSR function support

2016-07-01 Thread Yakir Yang 
The full name of PSR is Panel Self Refresh, panel device could refresh
itself with the hardware framebuffer in panel, this would make lots of
sense to save the power consumption.

This patch have exported two symbols for platform driver to implement
the PSR function in hardware side:
- analogix_dp_active_psr()
- analogix_dp_inactive_psr()

Signed-off-by: Yakir Yang 
---
Changes in v3:
- split analogix_dp_enable_psr(), make it more clearly
analogix_dp_detect_sink_psr()
analogix_dp_enable_sink_psr()
- remove some nosie register setting comments

Changes in v2:
- introduce in v2, splite the common Analogix DP changes out

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 64 ++
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  4 ++
 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  | 54 ++
 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h  | 28 ++
 include/drm/bridge/analogix_dp.h   |  3 +
 5 files changed, 153 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 32715da..b557097 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -97,6 +97,66 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device 
*dp)
return 0;
 }
 
+int analogix_dp_active_psr(struct device *dev)
+{
+   struct analogix_dp_device *dp = dev_get_drvdata(dev);
+
+   if (!dp->psr_support)
+   return -EINVAL;
+
+   analogix_dp_send_psr_spd(dp, EDP_VSC_PSR_STATE_ACTIVE |
+EDP_VSC_PSR_CRC_VALUES_VALID);
+   return 0;
+}
+EXPORT_SYMBOL_GPL(analogix_dp_active_psr);
+
+int analogix_dp_inactive_psr(struct device *dev)
+{
+   struct analogix_dp_device *dp = dev_get_drvdata(dev);
+
+   if (!dp->psr_support)
+   return -EINVAL;
+
+   analogix_dp_send_psr_spd(dp, 0);
+   return 0;
+}
+EXPORT_SYMBOL_GPL(analogix_dp_inactive_psr);
+
+static bool analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
+{
+   unsigned char psr_version;
+
+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_SUPPORT, _version);
+   dev_info(dp->dev, "Panel PSR version : %x\n", psr_version);
+
+   return (psr_version & DP_PSR_IS_SUPPORTED) ? true : false;
+}
+
+static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
+{
+   unsigned char psr_en;
+
+   /* Disable psr function */
+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
+   psr_en &= ~DP_PSR_ENABLE;
+   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
+
+   /* Main-Link transmitter remains active during PSR active states */
+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
+   psr_en = DP_PSR_MAIN_LINK_ACTIVE | DP_PSR_CRC_VERIFICATION;
+   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
+
+   /* Enable psr function */
+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
+   psr_en = DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE |
+DP_PSR_CRC_VERIFICATION;
+   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
+
+   analogix_dp_enable_psr_crc(dp);
+
+   return 0;
+}
+
 static unsigned char analogix_dp_calc_edid_check_sum(unsigned char *edid_data)
 {
int i;
@@ -921,6 +981,10 @@ static void analogix_dp_commit(struct analogix_dp_device 
*dp)
 
/* Enable video */
analogix_dp_start_video(dp);
+
+   dp->psr_support = analogix_dp_detect_sink_psr(dp);
+   if (dp->psr_support)
+   analogix_dp_enable_sink_psr(dp);
 }
 
 int analogix_dp_get_modes(struct drm_connector *connector)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
index b456380..6ca5dde 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
@@ -177,6 +177,7 @@ struct analogix_dp_device {
int hpd_gpio;
boolforce_hpd;
unsigned char   edid[EDID_BLOCK_LENGTH * 2];
+   boolpsr_support;
 
struct analogix_dp_plat_data *plat_data;
 };
@@ -278,4 +279,7 @@ int analogix_dp_is_video_stream_on(struct 
analogix_dp_device *dp);
 void analogix_dp_config_video_slave_mode(struct analogix_dp_device *dp);
 void analogix_dp_enable_scrambling(struct analogix_dp_device *dp);
 void analogix_dp_disable_scrambling(struct analogix_dp_device *dp);
+void analogix_dp_enable_psr_crc(struct analogix_dp_device *dp);
+void analogix_dp_send_psr_spd(struct analogix_dp_device *dp, int db1);
+
 #endif /* _ANALOGIX_DP_CORE_H */
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
index 48030f0..e8372c7 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c

[PATCH v3 3/4] drm/bridge: analogix_dp: add the PSR function support

2016-07-01 Thread Yakir Yang 
The full name of PSR is Panel Self Refresh, panel device could refresh
itself with the hardware framebuffer in panel, this would make lots of
sense to save the power consumption.

This patch have exported two symbols for platform driver to implement
the PSR function in hardware side:
- analogix_dp_active_psr()
- analogix_dp_inactive_psr()

Signed-off-by: Yakir Yang 
---
Changes in v3:
- split analogix_dp_enable_psr(), make it more clearly
analogix_dp_detect_sink_psr()
analogix_dp_enable_sink_psr()
- remove some nosie register setting comments

Changes in v2:
- introduce in v2, splite the common Analogix DP changes out

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 64 ++
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  4 ++
 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  | 54 ++
 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h  | 28 ++
 include/drm/bridge/analogix_dp.h   |  3 +
 5 files changed, 153 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 32715da..b557097 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -97,6 +97,66 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device 
*dp)
return 0;
 }
 
+int analogix_dp_active_psr(struct device *dev)
+{
+   struct analogix_dp_device *dp = dev_get_drvdata(dev);
+
+   if (!dp->psr_support)
+   return -EINVAL;
+
+   analogix_dp_send_psr_spd(dp, EDP_VSC_PSR_STATE_ACTIVE |
+EDP_VSC_PSR_CRC_VALUES_VALID);
+   return 0;
+}
+EXPORT_SYMBOL_GPL(analogix_dp_active_psr);
+
+int analogix_dp_inactive_psr(struct device *dev)
+{
+   struct analogix_dp_device *dp = dev_get_drvdata(dev);
+
+   if (!dp->psr_support)
+   return -EINVAL;
+
+   analogix_dp_send_psr_spd(dp, 0);
+   return 0;
+}
+EXPORT_SYMBOL_GPL(analogix_dp_inactive_psr);
+
+static bool analogix_dp_detect_sink_psr(struct analogix_dp_device *dp)
+{
+   unsigned char psr_version;
+
+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_SUPPORT, _version);
+   dev_info(dp->dev, "Panel PSR version : %x\n", psr_version);
+
+   return (psr_version & DP_PSR_IS_SUPPORTED) ? true : false;
+}
+
+static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp)
+{
+   unsigned char psr_en;
+
+   /* Disable psr function */
+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
+   psr_en &= ~DP_PSR_ENABLE;
+   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
+
+   /* Main-Link transmitter remains active during PSR active states */
+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
+   psr_en = DP_PSR_MAIN_LINK_ACTIVE | DP_PSR_CRC_VERIFICATION;
+   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
+
+   /* Enable psr function */
+   analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, _en);
+   psr_en = DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE |
+DP_PSR_CRC_VERIFICATION;
+   analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en);
+
+   analogix_dp_enable_psr_crc(dp);
+
+   return 0;
+}
+
 static unsigned char analogix_dp_calc_edid_check_sum(unsigned char *edid_data)
 {
int i;
@@ -921,6 +981,10 @@ static void analogix_dp_commit(struct analogix_dp_device 
*dp)
 
/* Enable video */
analogix_dp_start_video(dp);
+
+   dp->psr_support = analogix_dp_detect_sink_psr(dp);
+   if (dp->psr_support)
+   analogix_dp_enable_sink_psr(dp);
 }
 
 int analogix_dp_get_modes(struct drm_connector *connector)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
index b456380..6ca5dde 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
@@ -177,6 +177,7 @@ struct analogix_dp_device {
int hpd_gpio;
boolforce_hpd;
unsigned char   edid[EDID_BLOCK_LENGTH * 2];
+   boolpsr_support;
 
struct analogix_dp_plat_data *plat_data;
 };
@@ -278,4 +279,7 @@ int analogix_dp_is_video_stream_on(struct 
analogix_dp_device *dp);
 void analogix_dp_config_video_slave_mode(struct analogix_dp_device *dp);
 void analogix_dp_enable_scrambling(struct analogix_dp_device *dp);
 void analogix_dp_disable_scrambling(struct analogix_dp_device *dp);
+void analogix_dp_enable_psr_crc(struct analogix_dp_device *dp);
+void analogix_dp_send_psr_spd(struct analogix_dp_device *dp, int db1);
+
 #endif /* _ANALOGIX_DP_CORE_H */
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
index 48030f0..e8372c7 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
+++