[PATCH] drm/edid: move DDC_SEGMENT_ADDR into drm_edid.h

[PATCH] drm/edid: move DDC_SEGMENT_ADDR into drm_edid.h

[Shawn Guo]

The same definition of DDC_SEGMENT_ADDR is currently defined in two
places, drm_edid.c and inno_hdmi.h.  Let's consolidate the definition
into drm_edid.h in the same way that DDC_ADDR is defined.

Signed-off-by: Shawn Guo <shawnguo@kernel.org>
---
 drivers/gpu/drm/drm_edid.c           | 1 -
 drivers/gpu/drm/rockchip/inno_hdmi.h | 2 --
 include/drm/drm_edid.h               | 1 +
 3 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 7df26d4b7ad8..9d6735b68152 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1191,7 +1191,6 @@ bool drm_edid_is_valid(struct edid *edid)
 }
 EXPORT_SYMBOL(drm_edid_is_valid);
 
-#define DDC_SEGMENT_ADDR 0x30
 /**
  * drm_do_probe_ddc_edid() - get EDID information via I2C
  * @data: I2C device adapter
diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.h b/drivers/gpu/drm/rockchip/inno_hdmi.h
index aa7c415f8cc1..b27b30934c7e 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.h
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.h
@@ -16,8 +16,6 @@
 #ifndef __INNO_HDMI_H__
 #define __INNO_HDMI_H__
 
-#define DDC_SEGMENT_ADDR		0x30
-
 enum PWR_MODE {
 	NORMAL,
 	LOWER_PWR,
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 919933d1beb4..7ee78d5c9252 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -26,6 +26,7 @@
 #include <linux/types.h>
 
 #define EDID_LENGTH 128
+#define DDC_SEGMENT_ADDR 0x30
 #define DDC_ADDR 0x50
 #define DDC_ADDR2 0x52 /* E-DDC 1.2 - where DisplayID can hide */
 
-- 
1.9.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/edid: move DDC_SEGMENT_ADDR into drm_edid.h

[Daniel Vetter]

On Thu, Jul 14, 2016 at 05:00:28PM +0800, Shawn Guo wrote:
> The same definition of DDC_SEGMENT_ADDR is currently defined in two
> places, drm_edid.c and inno_hdmi.h.  Let's consolidate the definition
> into drm_edid.h in the same way that DDC_ADDR is defined.
> 
> Signed-off-by: Shawn Guo <shawnguo@kernel.org>

What really should be done here is nuke the fake i2c adapter in
inno_hdmi.c and instead just directly read the edid from the hdim IP.
Using drm_get_edid for something that's not backed by a real i2c bus is
bonghits.
-Daniel

> ---
>  drivers/gpu/drm/drm_edid.c           | 1 -
>  drivers/gpu/drm/rockchip/inno_hdmi.h | 2 --
>  include/drm/drm_edid.h               | 1 +
>  3 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 7df26d4b7ad8..9d6735b68152 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1191,7 +1191,6 @@ bool drm_edid_is_valid(struct edid *edid)
>  }
>  EXPORT_SYMBOL(drm_edid_is_valid);
>  
> -#define DDC_SEGMENT_ADDR 0x30
>  /**
>   * drm_do_probe_ddc_edid() - get EDID information via I2C
>   * @data: I2C device adapter
> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.h b/drivers/gpu/drm/rockchip/inno_hdmi.h
> index aa7c415f8cc1..b27b30934c7e 100644
> --- a/drivers/gpu/drm/rockchip/inno_hdmi.h
> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.h
> @@ -16,8 +16,6 @@
>  #ifndef __INNO_HDMI_H__
>  #define __INNO_HDMI_H__
>  
> -#define DDC_SEGMENT_ADDR		0x30
> -
>  enum PWR_MODE {
>  	NORMAL,
>  	LOWER_PWR,
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 919933d1beb4..7ee78d5c9252 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -26,6 +26,7 @@
>  #include <linux/types.h>
>  
>  #define EDID_LENGTH 128
> +#define DDC_SEGMENT_ADDR 0x30
>  #define DDC_ADDR 0x50
>  #define DDC_ADDR2 0x52 /* E-DDC 1.2 - where DisplayID can hide */
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/edid: move DDC_SEGMENT_ADDR into drm_edid.h

[Shawn Guo]

On Thu, Jul 14, 2016 at 02:45:41PM +0200, Daniel Vetter wrote:
> On Thu, Jul 14, 2016 at 05:00:28PM +0800, Shawn Guo wrote:
> > The same definition of DDC_SEGMENT_ADDR is currently defined in two
> > places, drm_edid.c and inno_hdmi.h.  Let's consolidate the definition
> > into drm_edid.h in the same way that DDC_ADDR is defined.
> > 
> > Signed-off-by: Shawn Guo <shawnguo@kernel.org>
> 
> What really should be done here is nuke the fake i2c adapter in
> inno_hdmi.c and instead just directly read the edid from the hdim IP.
> Using drm_get_edid for something that's not backed by a real i2c bus is
> bonghits.

This patch just makes some change literally.  I do not understand how
the IP block works at all.  I thought the HDMI IP talks to monitors
using I2C protocol implemented inside the IP block.  I added Rockchip
folks to see if we can get any clarifications from them.

Shawn
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/edid: move DDC_SEGMENT_ADDR into drm_edid.h

[Daniel Vetter]

On Thu, Jul 14, 2016 at 09:21:03PM +0800, Shawn Guo wrote:
> On Thu, Jul 14, 2016 at 02:45:41PM +0200, Daniel Vetter wrote:
> > On Thu, Jul 14, 2016 at 05:00:28PM +0800, Shawn Guo wrote:
> > > The same definition of DDC_SEGMENT_ADDR is currently defined in two
> > > places, drm_edid.c and inno_hdmi.h.  Let's consolidate the definition
> > > into drm_edid.h in the same way that DDC_ADDR is defined.
> > > 
> > > Signed-off-by: Shawn Guo <shawnguo@kernel.org>
> > 
> > What really should be done here is nuke the fake i2c adapter in
> > inno_hdmi.c and instead just directly read the edid from the hdim IP.
> > Using drm_get_edid for something that's not backed by a real i2c bus is
> > bonghits.
> 
> This patch just makes some change literally.  I do not understand how
> the IP block works at all.  I thought the HDMI IP talks to monitors
> using I2C protocol implemented inside the IP block.  I added Rockchip
> folks to see if we can get any clarifications from them.

btw drm_edid.c gained a special variant for this case a while ago:
drm_do_get_edid(). With that you can retain all the robustness/retrying
logic, while providing your own special function to read an entire edid
block. I think that's the function which should be used here.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/edid: move DDC_SEGMENT_ADDR into drm_edid.h

[Shawn Guo]

On Fri, Jul 15, 2016 at 09:24:36AM +0200, Daniel Vetter wrote:
> On Thu, Jul 14, 2016 at 09:21:03PM +0800, Shawn Guo wrote:
> > On Thu, Jul 14, 2016 at 02:45:41PM +0200, Daniel Vetter wrote:
> > > On Thu, Jul 14, 2016 at 05:00:28PM +0800, Shawn Guo wrote:
> > > > The same definition of DDC_SEGMENT_ADDR is currently defined in two
> > > > places, drm_edid.c and inno_hdmi.h.  Let's consolidate the definition
> > > > into drm_edid.h in the same way that DDC_ADDR is defined.
> > > > 
> > > > Signed-off-by: Shawn Guo <shawnguo@kernel.org>
> > > 
> > > What really should be done here is nuke the fake i2c adapter in
> > > inno_hdmi.c and instead just directly read the edid from the hdim IP.
> > > Using drm_get_edid for something that's not backed by a real i2c bus is
> > > bonghits.
> > 
> > This patch just makes some change literally.  I do not understand how
> > the IP block works at all.  I thought the HDMI IP talks to monitors
> > using I2C protocol implemented inside the IP block.  I added Rockchip
> > folks to see if we can get any clarifications from them.
> 
> btw drm_edid.c gained a special variant for this case a while ago:
> drm_do_get_edid(). With that you can retain all the robustness/retrying
> logic, while providing your own special function to read an entire edid
> block. I think that's the function which should be used here.

Thanks much for the info, Daniel.  It's now clear where I should go for
my WIP HDMI driver.

Shawn
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel