Re: [PATCH v2 1/2] drm/bridge: analogix_dp: Remove duplicated code v2

[Sean Paul]

On Wed, Sep 7, 2016 at 11:48 PM, Yakir Yang  wrote:
> From: Tomeu Vizoso 
>
> Remove code for reading the EDID and DPCD fields and use the helpers
> instead.
>
> Besides the obvious code reduction, other helpers are being added to the
> core that could be used in this driver and will be good to be able to
> use them instead of duplicating them.
>
> Signed-off-by: Tomeu Vizoso 
> Cc: Javier Martinez Canillas 
> Cc: Mika Kahola 
> Cc: Yakir Yang 
> Cc: Daniel Vetter 
>
> Reviewed-by: Sean Paul 
> Reviewed-by: Yakir Yang 
> Tested-by: Javier Martinez Canillas 
> Tested-by: Sean Paul 
> ---
> Changes in v2:
> - A bunch of good fixes from Sean and Yakir
> - Moved the transfer function to analogix_dp_reg.c
> - Removed reference to the EDID from the dp struct
> - Rebase on Sean's next tree
> git://people.freedesktop.org/~seanpaul/dogwood
>


This patch has already been merged to my 'base' branch, I suppose
since no one has picked it up yet, I'll pull it in my rockchip
for-next.

Thanks,

Sean




>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 263 
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  41 +-
>  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  | 451 
> ++---
>  3 files changed, 204 insertions(+), 551 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index efac8ab..5fe3982 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -31,6 +31,7 @@
>  #include 
>
>  #include "analogix_dp_core.h"
> +#include "analogix_dp_reg.h"
>
>  #define to_dp(nm)  container_of(nm, struct analogix_dp_device, nm)
>
> @@ -174,150 +175,21 @@ static void analogix_dp_enable_sink_psr(struct 
> analogix_dp_device *dp)
> analogix_dp_enable_psr_crc(dp);
>  }
>
> -static unsigned char analogix_dp_calc_edid_check_sum(unsigned char 
> *edid_data)
> -{
> -   int i;
> -   unsigned char sum = 0;
> -
> -   for (i = 0; i < EDID_BLOCK_LENGTH; i++)
> -   sum = sum + edid_data[i];
> -
> -   return sum;
> -}
> -
> -static int analogix_dp_read_edid(struct analogix_dp_device *dp)
> -{
> -   unsigned char *edid = dp->edid;
> -   unsigned int extend_block = 0;
> -   unsigned char sum;
> -   unsigned char test_vector;
> -   int retval;
> -
> -   /*
> -* EDID device address is 0x50.
> -* However, if necessary, you must have set upper address
> -* into E-EDID in I2C device, 0x30.
> -*/
> -
> -   /* Read Extension Flag, Number of 128-byte EDID extension blocks */
> -   retval = analogix_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
> -   EDID_EXTENSION_FLAG,
> -   _block);
> -   if (retval)
> -   return retval;
> -
> -   if (extend_block > 0) {
> -   dev_dbg(dp->dev, "EDID data includes a single extension!\n");
> -
> -   /* Read EDID data */
> -   retval = analogix_dp_read_bytes_from_i2c(dp,
> -   I2C_EDID_DEVICE_ADDR,
> -   EDID_HEADER_PATTERN,
> -   EDID_BLOCK_LENGTH,
> -   [EDID_HEADER_PATTERN]);
> -   if (retval != 0) {
> -   dev_err(dp->dev, "EDID Read failed!\n");
> -   return -EIO;
> -   }
> -   sum = analogix_dp_calc_edid_check_sum(edid);
> -   if (sum != 0) {
> -   dev_err(dp->dev, "EDID bad checksum!\n");
> -   return -EIO;
> -   }
> -
> -   /* Read additional EDID data */
> -   retval = analogix_dp_read_bytes_from_i2c(dp,
> -   I2C_EDID_DEVICE_ADDR,
> -   EDID_BLOCK_LENGTH,
> -   EDID_BLOCK_LENGTH,
> -   [EDID_BLOCK_LENGTH]);
> -   if (retval != 0) {
> -   dev_err(dp->dev, "EDID Read failed!\n");
> -   return -EIO;
> -   }
> -   sum = 
> analogix_dp_calc_edid_check_sum([EDID_BLOCK_LENGTH]);
> -   if (sum != 0) {
> -   dev_err(dp->dev, "EDID bad checksum!\n");
> -   return -EIO;
> -   }
> -
> -   analogix_dp_read_byte_from_dpcd(dp, DP_TEST_REQUEST,
> -   _vector);
> -   if (test_vector 

Re: [PATCH v2 1/2] drm/bridge: analogix_dp: Remove duplicated code v2

[Sean Paul]

On Wed, Sep 7, 2016 at 11:48 PM, Yakir Yang  wrote:
> From: Tomeu Vizoso 
>
> Remove code for reading the EDID and DPCD fields and use the helpers
> instead.
>
> Besides the obvious code reduction, other helpers are being added to the
> core that could be used in this driver and will be good to be able to
> use them instead of duplicating them.
>
> Signed-off-by: Tomeu Vizoso 
> Cc: Javier Martinez Canillas 
> Cc: Mika Kahola 
> Cc: Yakir Yang 
> Cc: Daniel Vetter 
>
> Reviewed-by: Sean Paul 
> Reviewed-by: Yakir Yang 
> Tested-by: Javier Martinez Canillas 
> Tested-by: Sean Paul 
> ---
> Changes in v2:
> - A bunch of good fixes from Sean and Yakir
> - Moved the transfer function to analogix_dp_reg.c
> - Removed reference to the EDID from the dp struct
> - Rebase on Sean's next tree
> git://people.freedesktop.org/~seanpaul/dogwood
>


This patch has already been merged to my 'base' branch, I suppose
since no one has picked it up yet, I'll pull it in my rockchip
for-next.

Thanks,

Sean




>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 263 
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  41 +-
>  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  | 451 
> ++---
>  3 files changed, 204 insertions(+), 551 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index efac8ab..5fe3982 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -31,6 +31,7 @@
>  #include 
>
>  #include "analogix_dp_core.h"
> +#include "analogix_dp_reg.h"
>
>  #define to_dp(nm)  container_of(nm, struct analogix_dp_device, nm)
>
> @@ -174,150 +175,21 @@ static void analogix_dp_enable_sink_psr(struct 
> analogix_dp_device *dp)
> analogix_dp_enable_psr_crc(dp);
>  }
>
> -static unsigned char analogix_dp_calc_edid_check_sum(unsigned char 
> *edid_data)
> -{
> -   int i;
> -   unsigned char sum = 0;
> -
> -   for (i = 0; i < EDID_BLOCK_LENGTH; i++)
> -   sum = sum + edid_data[i];
> -
> -   return sum;
> -}
> -
> -static int analogix_dp_read_edid(struct analogix_dp_device *dp)
> -{
> -   unsigned char *edid = dp->edid;
> -   unsigned int extend_block = 0;
> -   unsigned char sum;
> -   unsigned char test_vector;
> -   int retval;
> -
> -   /*
> -* EDID device address is 0x50.
> -* However, if necessary, you must have set upper address
> -* into E-EDID in I2C device, 0x30.
> -*/
> -
> -   /* Read Extension Flag, Number of 128-byte EDID extension blocks */
> -   retval = analogix_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
> -   EDID_EXTENSION_FLAG,
> -   _block);
> -   if (retval)
> -   return retval;
> -
> -   if (extend_block > 0) {
> -   dev_dbg(dp->dev, "EDID data includes a single extension!\n");
> -
> -   /* Read EDID data */
> -   retval = analogix_dp_read_bytes_from_i2c(dp,
> -   I2C_EDID_DEVICE_ADDR,
> -   EDID_HEADER_PATTERN,
> -   EDID_BLOCK_LENGTH,
> -   [EDID_HEADER_PATTERN]);
> -   if (retval != 0) {
> -   dev_err(dp->dev, "EDID Read failed!\n");
> -   return -EIO;
> -   }
> -   sum = analogix_dp_calc_edid_check_sum(edid);
> -   if (sum != 0) {
> -   dev_err(dp->dev, "EDID bad checksum!\n");
> -   return -EIO;
> -   }
> -
> -   /* Read additional EDID data */
> -   retval = analogix_dp_read_bytes_from_i2c(dp,
> -   I2C_EDID_DEVICE_ADDR,
> -   EDID_BLOCK_LENGTH,
> -   EDID_BLOCK_LENGTH,
> -   [EDID_BLOCK_LENGTH]);
> -   if (retval != 0) {
> -   dev_err(dp->dev, "EDID Read failed!\n");
> -   return -EIO;
> -   }
> -   sum = 
> analogix_dp_calc_edid_check_sum([EDID_BLOCK_LENGTH]);
> -   if (sum != 0) {
> -   dev_err(dp->dev, "EDID bad checksum!\n");
> -   return -EIO;
> -   }
> -
> -   analogix_dp_read_byte_from_dpcd(dp, DP_TEST_REQUEST,
> -   _vector);
> -   if (test_vector & DP_TEST_LINK_EDID_READ) {
> -   analogix_dp_write_byte_to_dpcd(dp,
> -   DP_TEST_EDID_CHECKSUM,
> -   edid[EDID_BLOCK_LENGTH + EDID_CHECKSUM]);
> -   

[PATCH v2 1/2] drm/bridge: analogix_dp: Remove duplicated code v2

[Yakir Yang]

From: Tomeu Vizoso 

Remove code for reading the EDID and DPCD fields and use the helpers
instead.

Besides the obvious code reduction, other helpers are being added to the
core that could be used in this driver and will be good to be able to
use them instead of duplicating them.

Signed-off-by: Tomeu Vizoso 
Cc: Javier Martinez Canillas 
Cc: Mika Kahola 
Cc: Yakir Yang 
Cc: Daniel Vetter 

Reviewed-by: Sean Paul 
Reviewed-by: Yakir Yang 
Tested-by: Javier Martinez Canillas 
Tested-by: Sean Paul 
---
Changes in v2:
- A bunch of good fixes from Sean and Yakir
- Moved the transfer function to analogix_dp_reg.c
- Removed reference to the EDID from the dp struct
- Rebase on Sean's next tree
git://people.freedesktop.org/~seanpaul/dogwood

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 263 
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  41 +-
 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  | 451 ++---
 3 files changed, 204 insertions(+), 551 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index efac8ab..5fe3982 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -31,6 +31,7 @@
 #include 
 
 #include "analogix_dp_core.h"
+#include "analogix_dp_reg.h"
 
 #define to_dp(nm)  container_of(nm, struct analogix_dp_device, nm)
 
@@ -174,150 +175,21 @@ static void analogix_dp_enable_sink_psr(struct 
analogix_dp_device *dp)
analogix_dp_enable_psr_crc(dp);
 }
 
-static unsigned char analogix_dp_calc_edid_check_sum(unsigned char *edid_data)
-{
-   int i;
-   unsigned char sum = 0;
-
-   for (i = 0; i < EDID_BLOCK_LENGTH; i++)
-   sum = sum + edid_data[i];
-
-   return sum;
-}
-
-static int analogix_dp_read_edid(struct analogix_dp_device *dp)
-{
-   unsigned char *edid = dp->edid;
-   unsigned int extend_block = 0;
-   unsigned char sum;
-   unsigned char test_vector;
-   int retval;
-
-   /*
-* EDID device address is 0x50.
-* However, if necessary, you must have set upper address
-* into E-EDID in I2C device, 0x30.
-*/
-
-   /* Read Extension Flag, Number of 128-byte EDID extension blocks */
-   retval = analogix_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
-   EDID_EXTENSION_FLAG,
-   _block);
-   if (retval)
-   return retval;
-
-   if (extend_block > 0) {
-   dev_dbg(dp->dev, "EDID data includes a single extension!\n");
-
-   /* Read EDID data */
-   retval = analogix_dp_read_bytes_from_i2c(dp,
-   I2C_EDID_DEVICE_ADDR,
-   EDID_HEADER_PATTERN,
-   EDID_BLOCK_LENGTH,
-   [EDID_HEADER_PATTERN]);
-   if (retval != 0) {
-   dev_err(dp->dev, "EDID Read failed!\n");
-   return -EIO;
-   }
-   sum = analogix_dp_calc_edid_check_sum(edid);
-   if (sum != 0) {
-   dev_err(dp->dev, "EDID bad checksum!\n");
-   return -EIO;
-   }
-
-   /* Read additional EDID data */
-   retval = analogix_dp_read_bytes_from_i2c(dp,
-   I2C_EDID_DEVICE_ADDR,
-   EDID_BLOCK_LENGTH,
-   EDID_BLOCK_LENGTH,
-   [EDID_BLOCK_LENGTH]);
-   if (retval != 0) {
-   dev_err(dp->dev, "EDID Read failed!\n");
-   return -EIO;
-   }
-   sum = analogix_dp_calc_edid_check_sum([EDID_BLOCK_LENGTH]);
-   if (sum != 0) {
-   dev_err(dp->dev, "EDID bad checksum!\n");
-   return -EIO;
-   }
-
-   analogix_dp_read_byte_from_dpcd(dp, DP_TEST_REQUEST,
-   _vector);
-   if (test_vector & DP_TEST_LINK_EDID_READ) {
-   analogix_dp_write_byte_to_dpcd(dp,
-   DP_TEST_EDID_CHECKSUM,
-   edid[EDID_BLOCK_LENGTH + EDID_CHECKSUM]);
-   analogix_dp_write_byte_to_dpcd(dp,
-   DP_TEST_RESPONSE,
-   DP_TEST_EDID_CHECKSUM_WRITE);
-   }
-   } else {
-   dev_info(dp->dev, "EDID data 

[PATCH v2 1/2] drm/bridge: analogix_dp: Remove duplicated code v2

[Yakir Yang]

From: Tomeu Vizoso 

Remove code for reading the EDID and DPCD fields and use the helpers
instead.

Besides the obvious code reduction, other helpers are being added to the
core that could be used in this driver and will be good to be able to
use them instead of duplicating them.

Signed-off-by: Tomeu Vizoso 
Cc: Javier Martinez Canillas 
Cc: Mika Kahola 
Cc: Yakir Yang 
Cc: Daniel Vetter 

Reviewed-by: Sean Paul 
Reviewed-by: Yakir Yang 
Tested-by: Javier Martinez Canillas 
Tested-by: Sean Paul 
---
Changes in v2:
- A bunch of good fixes from Sean and Yakir
- Moved the transfer function to analogix_dp_reg.c
- Removed reference to the EDID from the dp struct
- Rebase on Sean's next tree
git://people.freedesktop.org/~seanpaul/dogwood

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 263 
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |  41 +-
 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  | 451 ++---
 3 files changed, 204 insertions(+), 551 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index efac8ab..5fe3982 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -31,6 +31,7 @@
 #include 
 
 #include "analogix_dp_core.h"
+#include "analogix_dp_reg.h"
 
 #define to_dp(nm)  container_of(nm, struct analogix_dp_device, nm)
 
@@ -174,150 +175,21 @@ static void analogix_dp_enable_sink_psr(struct 
analogix_dp_device *dp)
analogix_dp_enable_psr_crc(dp);
 }
 
-static unsigned char analogix_dp_calc_edid_check_sum(unsigned char *edid_data)
-{
-   int i;
-   unsigned char sum = 0;
-
-   for (i = 0; i < EDID_BLOCK_LENGTH; i++)
-   sum = sum + edid_data[i];
-
-   return sum;
-}
-
-static int analogix_dp_read_edid(struct analogix_dp_device *dp)
-{
-   unsigned char *edid = dp->edid;
-   unsigned int extend_block = 0;
-   unsigned char sum;
-   unsigned char test_vector;
-   int retval;
-
-   /*
-* EDID device address is 0x50.
-* However, if necessary, you must have set upper address
-* into E-EDID in I2C device, 0x30.
-*/
-
-   /* Read Extension Flag, Number of 128-byte EDID extension blocks */
-   retval = analogix_dp_read_byte_from_i2c(dp, I2C_EDID_DEVICE_ADDR,
-   EDID_EXTENSION_FLAG,
-   _block);
-   if (retval)
-   return retval;
-
-   if (extend_block > 0) {
-   dev_dbg(dp->dev, "EDID data includes a single extension!\n");
-
-   /* Read EDID data */
-   retval = analogix_dp_read_bytes_from_i2c(dp,
-   I2C_EDID_DEVICE_ADDR,
-   EDID_HEADER_PATTERN,
-   EDID_BLOCK_LENGTH,
-   [EDID_HEADER_PATTERN]);
-   if (retval != 0) {
-   dev_err(dp->dev, "EDID Read failed!\n");
-   return -EIO;
-   }
-   sum = analogix_dp_calc_edid_check_sum(edid);
-   if (sum != 0) {
-   dev_err(dp->dev, "EDID bad checksum!\n");
-   return -EIO;
-   }
-
-   /* Read additional EDID data */
-   retval = analogix_dp_read_bytes_from_i2c(dp,
-   I2C_EDID_DEVICE_ADDR,
-   EDID_BLOCK_LENGTH,
-   EDID_BLOCK_LENGTH,
-   [EDID_BLOCK_LENGTH]);
-   if (retval != 0) {
-   dev_err(dp->dev, "EDID Read failed!\n");
-   return -EIO;
-   }
-   sum = analogix_dp_calc_edid_check_sum([EDID_BLOCK_LENGTH]);
-   if (sum != 0) {
-   dev_err(dp->dev, "EDID bad checksum!\n");
-   return -EIO;
-   }
-
-   analogix_dp_read_byte_from_dpcd(dp, DP_TEST_REQUEST,
-   _vector);
-   if (test_vector & DP_TEST_LINK_EDID_READ) {
-   analogix_dp_write_byte_to_dpcd(dp,
-   DP_TEST_EDID_CHECKSUM,
-   edid[EDID_BLOCK_LENGTH + EDID_CHECKSUM]);
-   analogix_dp_write_byte_to_dpcd(dp,
-   DP_TEST_RESPONSE,
-   DP_TEST_EDID_CHECKSUM_WRITE);
-   }
-   } else {
-   dev_info(dp->dev, "EDID data does not include any 
extensions.\n");
-
-   /* Read EDID data */
-   retval = analogix_dp_read_bytes_from_i2c(dp,
-   I2C_EDID_DEVICE_ADDR, EDID_HEADER_PATTERN,
-