Message ID | 1415935631-16826-1-git-send-email-andy.yan@rock-chips.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi Andy, Nice work on this patch series. Its getting better and better :). On 14/11/14 03:27, Andy Yan wrote: > hdmi phy clock symbol and transmission termination value > can adjust platform specific to get the best SI ^Is this signal integrity? Are these two disjoint features in separate patches? > > also add mode_valid interface for some platform may not support > all the display mode Sounds like another separate patch to me. :) Also, This series is becoming quite large. With major changes and fixes mixed together. Patch 3 splits imx-drm. Patch 4 moves dw-drm out of imx-drm folder. Patch 7 adds binding Patch 9 converts to drm bridge. Can these be placed together easily? And in the start. i.e. patch 1, 2, 3, 4, Then all fixes etc can come afterwards? It helps when checking histories later as to how a driver was made and how fixes happened. Especially when file moves happen.. Cheers, ZubairLK > > Signed-off-by: Andy Yan <andy.yan@rock-chips.com> > > --- > > Changes in v10: > - split generic dw_hdmi.c improvements from patch#11 (add rk3288 support) > > Changes in v9: None > Changes in v8: None > Changes in v7: None > Changes in v6: None > Changes in v5: None > Changes in v4: None > Changes in v3: None > Changes in v2: None > > drivers/gpu/drm/bridge/dw_hdmi.c | 29 +++++++++++++++++++++++++++-- > drivers/staging/imx-drm/dw_hdmi-imx.c | 10 +++++++++- > include/drm/bridge/dw_hdmi.h | 7 +++++++ > 3 files changed, 43 insertions(+), 3 deletions(-) >
Hi ZubairLK: Thanks for your review. On 2014?11?14? 18:19, Zubair Lutfullah Kakakhel wrote: > Hi Andy, > > Nice work on this patch series. Its getting better and better :). > > On 14/11/14 03:27, Andy Yan wrote: >> hdmi phy clock symbol and transmission termination value >> can adjust platform specific to get the best SI > ^Is this signal integrity? yes , SI is signal integrity, such as eye diagram measurement > > Are these two disjoint features in separate patches? > >> also add mode_valid interface for some platform may not support >> all the display mode > Sounds like another separate patch to me. :) they can seperate > > Also, This series is becoming quite large. With major changes and fixes mixed together. > > Patch 3 splits imx-drm. > Patch 4 moves dw-drm out of imx-drm folder. > Patch 7 adds binding > Patch 9 converts to drm bridge. > > Can these be placed together easily? > And in the start. i.e. patch 1, 2, 3, 4, > > Then all fixes etc can come afterwards? > > It helps when checking histories later as to how a driver was made and how fixes happened. > > Especially when file moves happen.. Do you mean we can rearrange the patch series? put patch 3, 4 ,7, 9 together one bye one than followed by the fixes patches 5 ,6, 8, 11 ? > > Cheers, > ZubairLK > >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com> >> >> --- >> >> Changes in v10: >> - split generic dw_hdmi.c improvements from patch#11 (add rk3288 support) >> >> Changes in v9: None >> Changes in v8: None >> Changes in v7: None >> Changes in v6: None >> Changes in v5: None >> Changes in v4: None >> Changes in v3: None >> Changes in v2: None >> >> drivers/gpu/drm/bridge/dw_hdmi.c | 29 +++++++++++++++++++++++++++-- >> drivers/staging/imx-drm/dw_hdmi-imx.c | 10 +++++++++- >> include/drm/bridge/dw_hdmi.h | 7 +++++++ >> 3 files changed, 43 insertions(+), 3 deletions(-) >> > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip > > >
On 14/11/14 10:53, Andy Yan wrote: > Hi ZubairLK: > Thanks for your review. > On 2014?11?14? 18:19, Zubair Lutfullah Kakakhel wrote: >> Hi Andy, >> >> Nice work on this patch series. Its getting better and better :). >> >> On 14/11/14 03:27, Andy Yan wrote: >>> hdmi phy clock symbol and transmission termination value >>> can adjust platform specific to get the best SI >> ^Is this signal integrity? > yes , SI is signal integrity, such as eye diagram measurement >> >> Are these two disjoint features in separate patches? >> >>> also add mode_valid interface for some platform may not support >>> all the display mode >> Sounds like another separate patch to me. :) > they can seperate >> >> Also, This series is becoming quite large. With major changes and fixes mixed together. >> >> Patch 3 splits imx-drm. >> Patch 4 moves dw-drm out of imx-drm folder. >> Patch 7 adds binding >> Patch 9 converts to drm bridge. >> >> Can these be placed together easily? >> And in the start. i.e. patch 1, 2, 3, 4, >> >> Then all fixes etc can come afterwards? >> >> It helps when checking histories later as to how a driver was made and how fixes happened. >> >> Especially when file moves happen.. > Do you mean we can rearrange the patch series? > put patch 3, 4 ,7, 9 together one bye one > than followed by the fixes patches 5 ,6, 8, 11 ? Yes. Rearrange so that the split imx-drm/imx-hdmi and conversion to drm-bridge is at the start of the series. Then the rest are bug fixes and feature additions. Cheers, ZubairLK
On 2014?11?14? 18:55, Zubair Lutfullah Kakakhel wrote: > > On 14/11/14 10:53, Andy Yan wrote: >> Hi ZubairLK: >> Thanks for your review. >> On 2014?11?14? 18:19, Zubair Lutfullah Kakakhel wrote: >>> Hi Andy, >>> >>> Nice work on this patch series. Its getting better and better :). >>> >>> On 14/11/14 03:27, Andy Yan wrote: >>>> hdmi phy clock symbol and transmission termination value >>>> can adjust platform specific to get the best SI >>> ^Is this signal integrity? >> yes , SI is signal integrity, such as eye diagram measurement >>> Are these two disjoint features in separate patches? >>> >>>> also add mode_valid interface for some platform may not support >>>> all the display mode >>> Sounds like another separate patch to me. :) >> they can seperate >>> Also, This series is becoming quite large. With major changes and fixes mixed together. >>> >>> Patch 3 splits imx-drm. >>> Patch 4 moves dw-drm out of imx-drm folder. >>> Patch 7 adds binding >>> Patch 9 converts to drm bridge. >>> >>> Can these be placed together easily? >>> And in the start. i.e. patch 1, 2, 3, 4, >>> >>> Then all fixes etc can come afterwards? >>> >>> It helps when checking histories later as to how a driver was made and how fixes happened. >>> >>> Especially when file moves happen.. >> Do you mean we can rearrange the patch series? >> put patch 3, 4 ,7, 9 together one bye one >> than followed by the fixes patches 5 ,6, 8, 11 ? > Yes. Rearrange so that the split imx-drm/imx-hdmi and conversion to drm-bridge is at the start of the series. > Then the rest are bug fixes and feature additions. Can I put patch#1(make checkpatch happy) and patch#2 (defer probe) as the first two patch. Daniel from Google chromium think it's better to put the two slightly changes in the front for easy review. > Cheers, > ZubairLK > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
On 14/11/14 11:08, Andy Yan wrote: > > On 2014?11?14? 18:55, Zubair Lutfullah Kakakhel wrote: >> >> On 14/11/14 10:53, Andy Yan wrote: >>> Hi ZubairLK: >>> Thanks for your review. >>> On 2014?11?14? 18:19, Zubair Lutfullah Kakakhel wrote: >>>> Hi Andy, >>>> >>>> Nice work on this patch series. Its getting better and better :). >>>> >>>> On 14/11/14 03:27, Andy Yan wrote: >>>>> hdmi phy clock symbol and transmission termination value >>>>> can adjust platform specific to get the best SI >>>> ^Is this signal integrity? >>> yes , SI is signal integrity, such as eye diagram measurement >>>> Are these two disjoint features in separate patches? >>>> >>>>> also add mode_valid interface for some platform may not support >>>>> all the display mode >>>> Sounds like another separate patch to me. :) >>> they can seperate >>>> Also, This series is becoming quite large. With major changes and fixes mixed together. >>>> >>>> Patch 3 splits imx-drm. >>>> Patch 4 moves dw-drm out of imx-drm folder. >>>> Patch 7 adds binding >>>> Patch 9 converts to drm bridge. >>>> >>>> Can these be placed together easily? >>>> And in the start. i.e. patch 1, 2, 3, 4, >>>> >>>> Then all fixes etc can come afterwards? >>>> >>>> It helps when checking histories later as to how a driver was made and how fixes happened. >>>> >>>> Especially when file moves happen.. >>> Do you mean we can rearrange the patch series? >>> put patch 3, 4 ,7, 9 together one bye one >>> than followed by the fixes patches 5 ,6, 8, 11 ? >> Yes. Rearrange so that the split imx-drm/imx-hdmi and conversion to drm-bridge is at the start of the series. >> Then the rest are bug fixes and feature additions. > Can I put patch#1(make checkpatch happy) and patch#2 (defer probe) as the first two patch. > Daniel from Google chromium think it's better to put the two slightly changes in the front for easy review. Sure. I am not the maintainer. They have to make the final decision. ZubairLK
On Fri, Nov 14, 2014 at 7:13 PM, Zubair Lutfullah Kakakhel <Zubair.Kakakhel@imgtec.com> wrote: > > > On 14/11/14 11:08, Andy Yan wrote: >> >> On 2014?11?14? 18:55, Zubair Lutfullah Kakakhel wrote: >>> >>> On 14/11/14 10:53, Andy Yan wrote: >>>> Hi ZubairLK: >>>> Thanks for your review. >>>> On 2014?11?14? 18:19, Zubair Lutfullah Kakakhel wrote: >>>>> Hi Andy, >>>>> >>>>> Nice work on this patch series. Its getting better and better :). >>>>> >>>>> On 14/11/14 03:27, Andy Yan wrote: >>>>>> hdmi phy clock symbol and transmission termination value >>>>>> can adjust platform specific to get the best SI >>>>> ^Is this signal integrity? >>>> yes , SI is signal integrity, such as eye diagram measurement >>>>> Are these two disjoint features in separate patches? >>>>> >>>>>> also add mode_valid interface for some platform may not support >>>>>> all the display mode >>>>> Sounds like another separate patch to me. :) >>>> they can seperate >>>>> Also, This series is becoming quite large. With major changes and fixes mixed together. >>>>> >>>>> Patch 3 splits imx-drm. >>>>> Patch 4 moves dw-drm out of imx-drm folder. >>>>> Patch 7 adds binding >>>>> Patch 9 converts to drm bridge. >>>>> >>>>> Can these be placed together easily? >>>>> And in the start. i.e. patch 1, 2, 3, 4, >>>>> >>>>> Then all fixes etc can come afterwards? >>>>> >>>>> It helps when checking histories later as to how a driver was made and how fixes happened. >>>>> >>>>> Especially when file moves happen.. >>>> Do you mean we can rearrange the patch series? >>>> put patch 3, 4 ,7, 9 together one bye one >>>> than followed by the fixes patches 5 ,6, 8, 11 ? >>> Yes. Rearrange so that the split imx-drm/imx-hdmi and conversion to drm-bridge is at the start of the series. >>> Then the rest are bug fixes and feature additions. >> Can I put patch#1(make checkpatch happy) and patch#2 (defer probe) as the first two patch. >> Daniel from Google chromium think it's better to put the two slightly changes in the front for easy review. Sorry, I didn't see this conversation before my earlier emails. I was suggesting to Andy to arrange his patches like this: (1) fixes that directly apply to imx-drm as it is today. (2) split out the "generic dw_hdmi" parts from imx-hdmi into dw_hdmi.c (3) convert dw_hdmi.c to a drm_bridge (4) move the dw_hdmi.c bridge implementation to drm/bridge (5) modifications to dw_hdmi.c required to support hdmi on rk3288 (5) add rk3288-hdmi.c to drm/rockchip (at least whenever drm/rockchip lands) The idea being that we can start landing the patches for (1) even while we are still debating / reviewing (2)+ upstream. > Sure. > > I am not the maintainer. They have to make the final decision. imx-drm is still in staging. I do not see anyone listed explicitly under MAINTAINERS for drivers/staging/imx-drm, so by default I guess it falls back to gregkh (drivers/staging/)? The "Commit" field in git log seems to back this up. Although, based on Author, I think we also want the opinions of Philipp Zabel and Russel King. Thanks, -djk > > ZubairLK
On Sat, Nov 15, 2014 at 06:07:50PM +0800, Daniel Kurtz wrote: > On Fri, Nov 14, 2014 at 7:13 PM, Zubair Lutfullah Kakakhel > <Zubair.Kakakhel@imgtec.com> wrote: > > > > > > On 14/11/14 11:08, Andy Yan wrote: > >> > >> On 2014?11?14? 18:55, Zubair Lutfullah Kakakhel wrote: > >>> > >>> On 14/11/14 10:53, Andy Yan wrote: > >>>> Hi ZubairLK: > >>>> Thanks for your review. > >>>> On 2014?11?14? 18:19, Zubair Lutfullah Kakakhel wrote: > >>>>> Hi Andy, > >>>>> > >>>>> Nice work on this patch series. Its getting better and better :). > >>>>> > >>>>> On 14/11/14 03:27, Andy Yan wrote: > >>>>>> hdmi phy clock symbol and transmission termination value > >>>>>> can adjust platform specific to get the best SI > >>>>> ^Is this signal integrity? > >>>> yes , SI is signal integrity, such as eye diagram measurement > >>>>> Are these two disjoint features in separate patches? > >>>>> > >>>>>> also add mode_valid interface for some platform may not support > >>>>>> all the display mode > >>>>> Sounds like another separate patch to me. :) > >>>> they can seperate > >>>>> Also, This series is becoming quite large. With major changes and fixes mixed together. > >>>>> > >>>>> Patch 3 splits imx-drm. > >>>>> Patch 4 moves dw-drm out of imx-drm folder. > >>>>> Patch 7 adds binding > >>>>> Patch 9 converts to drm bridge. > >>>>> > >>>>> Can these be placed together easily? > >>>>> And in the start. i.e. patch 1, 2, 3, 4, > >>>>> > >>>>> Then all fixes etc can come afterwards? > >>>>> > >>>>> It helps when checking histories later as to how a driver was made and how fixes happened. > >>>>> > >>>>> Especially when file moves happen.. > >>>> Do you mean we can rearrange the patch series? > >>>> put patch 3, 4 ,7, 9 together one bye one > >>>> than followed by the fixes patches 5 ,6, 8, 11 ? > >>> Yes. Rearrange so that the split imx-drm/imx-hdmi and conversion to drm-bridge is at the start of the series. > >>> Then the rest are bug fixes and feature additions. > >> Can I put patch#1(make checkpatch happy) and patch#2 (defer probe) as the first two patch. > >> Daniel from Google chromium think it's better to put the two slightly changes in the front for easy review. > > Sorry, I didn't see this conversation before my earlier emails. > > I was suggesting to Andy to arrange his patches like this: > (1) fixes that directly apply to imx-drm as it is today. > (2) split out the "generic dw_hdmi" parts from imx-hdmi into dw_hdmi.c > (3) convert dw_hdmi.c to a drm_bridge > (4) move the dw_hdmi.c bridge implementation to drm/bridge > (5) modifications to dw_hdmi.c required to support hdmi on rk3288 > (5) add rk3288-hdmi.c to drm/rockchip (at least whenever drm/rockchip lands) > > The idea being that we can start landing the patches for (1) even > while we are still debating / reviewing (2)+ upstream. > > > Sure. > > > > I am not the maintainer. They have to make the final decision. > > imx-drm is still in staging. I do not see anyone listed explicitly > under MAINTAINERS for drivers/staging/imx-drm, so by default I guess > it falls back to gregkh (drivers/staging/)? > The "Commit" field in git log seems to back this up. > > Although, based on Author, I think we also want the opinions of > Philipp Zabel and Russel King. Once the wranglings on the patch series are complete, I do intend to test it on the platforms I have - and remember that I do have the ALSA based audio and CEC bits as well, some of which will probably need a little bit of re-work. All in all, I welcome the renaming of this to include a reference to DesignWare - I've always thought it's a mistake that the HDMI interface in iMX6 was not named with a "dw" prefix as the docs contain references to it being a DesignWare IP module.
On Sat, Nov 15, 2014 at 10:12:18AM +0000, Russell King - ARM Linux wrote: > Once the wranglings on the patch series are complete, I do intend to test > it on the platforms I have - and remember that I do have the ALSA based > audio and CEC bits as well, some of which will probably need a little bit > of re-work. > > All in all, I welcome the renaming of this to include a reference to > DesignWare - I've always thought it's a mistake that the HDMI interface > in iMX6 was not named with a "dw" prefix as the docs contain references > to it being a DesignWare IP module. One thing I would ask is that the subsequent submissions do not thread onto the previous submission. It may seem a good idea (people claim that it allows the previous reviews to be trivially found) but these people forget an important side effect from this behaviour - when looking at the message index in a threaded mail reader (like mutt), each reply to a thread moves the subject line by three characters to the right. What this means is that after about five or six iterations of the submission, there is no longer any subject line visible. Moreover, it means that with lesser iterations, it becomes much more difficult to see /any/ of the review thread structure. I would suggest that if you do want to "connect" the subsequent submissions, please use the same reference message for each submission. In other words, rather than: v1 0/2 +-> v1 1/2 +-> v1 2/2 +-> v2 0/2 +-> v2 1/2 +-> v2 2/2 +-> v3 0/2 +-> v3 1/2 +-> v3 2/2 ... This is done instead: v1 0/2 +-> v1 1/2 +-> v1 2/2 +-> v2 0/2 | +-> v2 1/2 | +-> v2 2/2 +-> v3 0/2 | +-> v3 1/2 | +-> v3 2/2 ... which is a compromise between threading the messages together, and keeping stopping the thread pushing the subject line completely off the right hand side of the screen. In this case, I'd suggest a reference of: 1415793593-5075-1-git-send-email-andy.yan@rock-chips.com which is the v8 covering message which started this big thread. Thanks.
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c index e9f0dfe..05e7156 100644 --- a/drivers/gpu/drm/bridge/dw_hdmi.c +++ b/drivers/gpu/drm/bridge/dw_hdmi.c @@ -718,6 +718,7 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi, unsigned char prep, u8 val, msec; const struct mpll_config *mpll_cfg = hdmi->plat_data->mpll_cfg; const struct curr_ctrl *curr_ctr = hdmi->plat_data->cur_ctr; + const struct sym_term *sym_term = hdmi->plat_data->sym_term; if (prep) return -EINVAL; @@ -787,10 +788,17 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi, unsigned char prep, hdmi_phy_i2c_write(hdmi, 0x0000, 0x13); /* PLLPHBYCTRL */ hdmi_phy_i2c_write(hdmi, 0x0006, 0x17); + + for (i = 0; sym_term[i].mpixelclock != (~0UL); i++) + if (hdmi->hdmi_data.video_mode.mpixelclock <= + sym_term[i].mpixelclock) + break; + /* RESISTANCE TERM 133Ohm Cfg */ - hdmi_phy_i2c_write(hdmi, 0x0005, 0x19); /* TXTERM */ + hdmi_phy_i2c_write(hdmi, sym_term[i].term, 0x19); /* TXTERM */ /* PREEMP Cgf 0.00 */ - hdmi_phy_i2c_write(hdmi, 0x800d, 0x09); /* CKSYMTXCTRL */ + hdmi_phy_i2c_write(hdmi, sym_term[i].sym_ctr, 0x09); /* CKSYMTXCTRL */ + /* TX/CK LVL 10 */ hdmi_phy_i2c_write(hdmi, 0x01ad, 0x0E); /* VLEVCTRL */ /* REMOVE CLK TERM */ @@ -1326,6 +1334,20 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector) return 0; } +static enum drm_mode_status +dw_hdmi_connector_mode_valid(struct drm_connector *connector, + struct drm_display_mode *mode) +{ + struct dw_hdmi *hdmi = container_of(connector, + struct dw_hdmi, connector); + enum drm_mode_status mode_status = MODE_OK; + + if (hdmi->plat_data->mode_valid) + mode_status = hdmi->plat_data->mode_valid(connector, mode); + + return mode_status; +} + static struct drm_encoder *dw_hdmi_connector_best_encoder(struct drm_connector *connector) { @@ -1416,6 +1438,7 @@ static struct drm_connector_funcs dw_hdmi_connector_funcs = { static struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = { .get_modes = dw_hdmi_connector_get_modes, + .mode_valid = dw_hdmi_connector_mode_valid, .best_encoder = dw_hdmi_connector_best_encoder, }; @@ -1487,6 +1510,8 @@ static int dw_hdmi_register(struct drm_device *drm, struct dw_hdmi *hdmi) drm_mode_connector_attach_encoder(&hdmi->connector, &hdmi->encoder); + drm_connector_register(&hdmi->connector); + return 0; } diff --git a/drivers/staging/imx-drm/dw_hdmi-imx.c b/drivers/staging/imx-drm/dw_hdmi-imx.c index 4b48ea6..d693051 100644 --- a/drivers/staging/imx-drm/dw_hdmi-imx.c +++ b/drivers/staging/imx-drm/dw_hdmi-imx.c @@ -69,7 +69,13 @@ static const struct curr_ctrl imx_cur_ctr[] = { } }; -static int dw_hdmi_parse_dt(struct imx_hdmi *hdmi) +static const struct sym_term imx_sym_term[] = { + /*pixelclk symbol term*/ + { 148500000, 0x800d, 0x0005 }, + { ~0UL, 0x0000, 0x0000 } +}; + +static int dw_hdmi_imx_parse_dt(struct imx_hdmi *hdmi) { struct device_node *np = hdmi->dev->of_node; @@ -156,6 +162,7 @@ static struct dw_hdmi_plat_data imx6q_hdmi_drv_data = { .encoder_prepare = dw_hdmi_imx_encoder_prepare, .mpll_cfg = imx_mpll_cfg, .cur_ctr = imx_cur_ctr, + .sym_term = imx_sym_term, .dev_type = IMX6Q_HDMI, }; @@ -166,6 +173,7 @@ static struct dw_hdmi_plat_data imx6dl_hdmi_drv_data = { .encoder_prepare = dw_hdmi_imx_encoder_prepare, .mpll_cfg = imx_mpll_cfg, .cur_ctr = imx_cur_ctr, + .sym_term = imx_sym_term, .dev_type = IMX6DL_HDMI, }; diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h index 6683b63..1688ec9 100644 --- a/include/drm/bridge/dw_hdmi.h +++ b/include/drm/bridge/dw_hdmi.h @@ -37,6 +37,12 @@ struct curr_ctrl { u16 curr[RES_MAX]; }; +struct sym_term { + unsigned long mpixelclock; + u16 sym_ctr; /*clock symbol and transmitter control*/ + u16 term; /*transmission termination value*/ +}; + struct dw_hdmi_plat_data { void * (*setup)(struct platform_device *pdev); void (*exit)(void *priv); @@ -47,6 +53,7 @@ struct dw_hdmi_plat_data { struct drm_display_mode *mode); const struct mpll_config *mpll_cfg; const struct curr_ctrl *cur_ctr; + const struct sym_term *sym_term; enum dw_hdmi_devtype dev_type; };
hdmi phy clock symbol and transmission termination value can adjust platform specific to get the best SI also add mode_valid interface for some platform may not support all the display mode Signed-off-by: Andy Yan <andy.yan@rock-chips.com> --- Changes in v10: - split generic dw_hdmi.c improvements from patch#11 (add rk3288 support) Changes in v9: None Changes in v8: None Changes in v7: None Changes in v6: None Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2: None drivers/gpu/drm/bridge/dw_hdmi.c | 29 +++++++++++++++++++++++++++-- drivers/staging/imx-drm/dw_hdmi-imx.c | 10 +++++++++- include/drm/bridge/dw_hdmi.h | 7 +++++++ 3 files changed, 43 insertions(+), 3 deletions(-)