| Message ID | 1468486828-29293-1-git-send-email-shawnguo@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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 */
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(-)