<div dir="ltr">Hi Mark,<div>Firstly thanks for reviewing.<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Oct 28, 2013 at 12:20 PM, Mark Rutland <span dir="ltr"><<a href="mailto:mark.rutland@arm.com" target="_blank">mark.rutland@arm.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi,<br>
<div class="im"><br>
On Mon, Oct 28, 2013 at 06:24:22AM +0000, Shirish S wrote:<br>
> This patch adds dt support to hdmiphy config settings<br>
> as it is board specific and depends on the signal pattern<br>
> of board.<br>
><br>
> Signed-off-by: Shirish S <<a href="mailto:s.shirish@samsung.com">s.shirish@samsung.com</a>><br>
> ---<br>
> .../devicetree/bindings/video/exynos_hdmi.txt | 29 ++++++++<br>
> arch/arm/boot/dts/exynos5250-arndale.dts | 6 +-<br>
> drivers/gpu/drm/exynos/exynos_hdmi.c | 70 ++++++++++++++++++--<br>
> 3 files changed, 98 insertions(+), 7 deletions(-)<br>
><br>
> diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt<br>
> index 323983b..770f92d 100644<br>
> --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt<br>
> +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt<br>
> @@ -13,6 +13,27 @@ Required properties:<br>
> b) pin number within the gpio controller.<br>
> c) optional flags and pull up/down.<br>
><br>
> +- hdmiphy-confs: following information about the hdmiphy conf settings.<br>
<br>
</div>Judging by the other patches, this is a node, not a property.<br>
<div class="im"><br></div></blockquote><div style>Yes its a node. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">
> + a) "nr-confs" specifies the number of pixel clocks supported.<br>
<br>
</div>Why is this needed? Someone will get it wrong eventually and it can be figured<br>
out currently by counting the child nodes, testing if they have the appropriate<br>
properties.<br>
<div class="im"><br></div></blockquote><div style>Actually i need to get the array size also from dt, hence this is approach i have taken </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">
> + b) "confX: confX" specifies the phy configuration settings,<br>
<br>
</div>This is confusing. What is X?<br>
<br></blockquote><div style>I am trying to generalize, here X means any numerical, and the programmer needs to </div><div style>make sure conf0:conf0, wherein X is 0.I shall provide the values permitted for X in my next patch set.</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
The label is irrelevant -- none of this patch looks for phandles pointing at<br>
configurations, nor is the precise name of the label important.<br>
<br>
This is a node, not a property.<br>
<div class="im"><br></div></blockquote><div style>Ideally every conf<numerical value> a combination of pixel clock and new values for data and clock level.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">
> + "clock-frequency" specifies the pixel clock<br>
<br>
</div>Is this a frequency to configure the pixel clock with, or the pre-determined<br>
frequency of a clock that we will select?<br>
<div class="im"><br></div></blockquote><div style>No, as the explanation suggests its the pixel clock itself. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">
> + "con-de-emphasis-level" specifies the configuration<br>
> + of Data De-emphasis levels.<br>
<br>
</div>Please explain _why_ we need this configuration.<br>
<br></blockquote><div style>Our chipset to undergo HDMI compliance test and noticed that the HDMI Compliance Test id 7-10 was failing</div><div style>for eye diagram test. Hence on further analysis, it was found that altering the data de-emphasis levels and clock </div>
<div style>level are required to pass the test.And also these values may vary for variuos board revisons, this is the purpose of this whole patch set.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Also, "con" is not a good abbreviation of "configuration", "config" would be<br>
preferable.<br>
<div class="im"><br></div></blockquote><div style>Agreed, will update the same in next patch set. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">
> + 0x145D0040h[3:0] permitted values:<br>
> + 0000 means 760 mVdiff && 1111 means 1400 mVdiff<br>
<br>
</div>I assume the 'h' suffix is a redundant description that the constant is<br>
hexadecimal. Please drop it.<br>
<br></blockquote><div>Agreed, will update the same in next patch set. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
What is 0x145D0040? The address of the register, or its value?<br>
<br></blockquote><div style>Its the address of the hdmiphy register for data level configuration.</div><div style> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
The description is confusing, 0x145D0040h[3:0] is always 0[3:0].<br>
<br></blockquote><div style>This description is extracted from the one specified in manual, in my first patch set the reviewers had asked me </div><div style>to provide the explaination for every bit, which i have provided. </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Why does this need to be the exact value programmed into the register rather<br>
than separate values the driver can compose?<br>
<div class="im"><br></div></blockquote><div style>As mentioned above the value is must for clearing the test 7-10, and also its derived by a trial and error method</div><div style>with the HDMI analyser. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">
> + 0x145D0040h[7:4] permitted values:<br>
> + 000 0dB<br>
> + 0001 -0.25dB<br>
> + 0010 -0.7dB<br>
> + 0011 -1.15dB<br>
> + 1111 -7.45dB<br>
<br>
</div>Again, this seems very odd. Why this format?<br>
<div class="im"><br></div></blockquote><div style>This binary translation of what the bits actually mean.In the final result it was found that at 0.7dB there is no noise in the output.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">
> + "con-clock-level" specifies the configuration for<br>
> + the corresponding clock level.<br>
<br>
</div>To repeat my comment on "con-de-emphasis-level", "con" is not a good<br>
abbreviation for "configuration".<br>
<div class="im"><br></div></blockquote><div>Agreed, will update the same in next patch set. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">
> + 0x145D005Ch [1:0] permitted values:<br>
> + 00 means 0 mVdiff && 11 means 60 mVdiff<br>
> + 0x145D005Ch [7:3] permitted values:<br>
> + 00000 is 790 mVdiff<br>
> + 11111 is 1430 mVdiff<br>
<br>
</div>Please explain why we must use this exact format rather than one which may be<br>
understood more easily.<br>
<div class="im"><br></div></blockquote><div style>I hope by now have made this point clear! </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">
> Example:<br>
><br>
> hdmi {<br>
> @@ -20,4 +41,12 @@ Example:<br>
> reg = <0x14530000 0x100000>;<br>
> interrupts = <0 95 0>;<br>
> hpd-gpio = <&gpx3 7 1>;<br>
> + hdmiphy-confs {<br>
> + nr-confs = <1>;<br>
> + conf0: conf0 {<br>
> + clock-frequency = <25200000>;<br>
> + conf-de-emphasis-level = /bits/ 8 <0x26>;<br>
> + conf-clock-level = /bits/ 8 < 0x66>;<br>
<br>
</div>Why are these 8-bit? That wasn't described in the binding at all so far.<br>
<div class="im"><br></div></blockquote><div style>These are 8 bit by design(as mentioned in the manual) and were not part of device tree to be explained before. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">
> + };<br>
> + }<br>
> };<br>
> diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts b/arch/arm/boot/dts/exynos5250-arndale.dts<br>
> index c23f16b..436b75a 100644<br>
> --- a/arch/arm/boot/dts/exynos5250-arndale.dts<br>
> +++ b/arch/arm/boot/dts/exynos5250-arndale.dts<br>
> @@ -424,6 +424,9 @@<br>
><br>
> hdmi {<br>
> hpd-gpio = <&gpx3 7 2>;<br>
> + vdd_osc-supply = <&ldo10_reg>;<br>
> + vdd_pll-supply = <&ldo8_reg>;<br>
> + vdd-supply = <&ldo8_reg>;<br>
> hdmiphy-confs {<br>
> nr-confs = <13>;<br>
> conf0: conf0 {<br>
> @@ -492,9 +495,6 @@<br>
> conf-clock-level = /bits/ 8 < 0x66>;<br>
> };<br>
> };<br>
> - vdd_osc-supply = <&ldo10_reg>;<br>
> - vdd_pll-supply = <&ldo8_reg>;<br>
> - vdd-supply = <&ldo8_reg>;<br>
> };<br>
><br>
> mmc_reg: voltage-regulator {<br>
<br>
</div>Why is this moving? It in no way relates to the rest of the patch, and wasn't<br>
described in the commit message.<br>
<br></blockquote><div style>Oops that was supposed to be part of previous pach,will update it. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
[...]<br>
<div class="im"><br>
> +static int drm_hdmi_dt_parse_phy_conf(struct platform_device *pdev,<br>
> + struct hdmi_context *hdata)<br>
> +{<br>
> + struct device *dev = &pdev->dev;<br>
> + struct device_node *dev_np = dev->of_node;<br>
> + struct device_node *phy_conf, *cfg_np;<br>
> + int i = 0;<br>
> +<br>
> + phy_conf = of_find_node_by_name(dev_np, "hdmiphy-confs");<br>
> + if (phy_conf == NULL) {<br>
> + DRM_ERROR("Did not find hdmiphy_conf node\n");<br>
> + return -ENODEV;<br>
> + }<br>
> +<br>
> + of_property_read_u32(phy_conf, "nr-confs", &hdata->nr_confs);<br>
<br>
</div>This can fail, returning an error code. Either check it, or remove this<br>
property entirely and do this based on the number of children which have the<br>
appropriate properties.<br>
<div class="im"><br></div></blockquote><div style>Ok will add a check. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">
> + hdata->confs = kzalloc((hdata->nr_confs * sizeof<br>
> + (struct hdmiphy_config)), GFP_KERNEL);<br>
<br>
</div>Why the brackets on the first argument of kzalloc? They're superfluous.<br>
<br>
What if kzalloc fails?<br>
<div class="im"><br></div></blockquote><div style>Ok, will put a check. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">
> +<br>
> + /* Initialize with default config */<br>
> + hdata->confs = hdmiphy_v14_configs;<br>
> +<br>
> + for_each_child_of_node(phy_conf, cfg_np) {<br>
<br>
</div>What if nr-confs is smaller than the number of child nodes?<br>
<div class="im"><br></div></blockquote><div style>it basically is the array size and it should be 1 + child nodes as mentioned in the descriptions, i</div><div style>assume that the programmer takes care of it.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">
> + if (!of_find_property(cfg_np, "clock-frequency", NULL))<br>
> + continue;<br>
> +<br>
> + if (of_property_read_u32_array(cfg_np, "clock-frequency",<br>
> + (u32 *)&hdata->confs[i].<br>
> + pixel_clock, 1)) {<br>
<br>
</div>Don't split &hdata->confs[i].pixel_clock over two lines.<br>
<br>
Why is the cast necessary?<br>
<br>
Why not just of_property_read_u32? This only ever has a single value, and while<br>
there's no of_property_read_u8, of_property_read_u32 exists.<br>
<div class="im"><br></div></blockquote><div style>Ok, agreed. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">
> + DRM_ERROR("Failed to get pixel clock\n");<br>
> + return -EINVAL;<br>
> + }<br>
> +<br>
> + /* Overwrite the data de-emphasis and data level */<br>
> + if (of_property_read_u8_array(cfg_np, "conf-de-emphasis-level",<br>
> + (u8 *)&hdata->confs[i].conf[16], 1)) {<br>
<br>
</div>Why is this cast necessary?<br>
<br>
I don't see why this must be an 8 bit property.<br>
<div class="im"><br></div></blockquote><div style>As mentioned earlier its by design 8 bits. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">
> + DRM_ERROR("Failed to get conf\n");<br>
> + return -EINVAL;<br>
> + }<br>
> + /* Overwrite the clock level diff */<br>
> + if (of_property_read_u8_array(cfg_np, "conf-clock-level",<br>
> + (u8 *)&hdata->confs[i].conf[23], 1)) {<br>
<br>
</div>Why the cast?<br>
<br></blockquote><div style>Same explaination as above. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Thanks,<br>
Mark.<br>
</blockquote></div>Thanks,</div></div><div class="gmail_extra" style>Shirish S</div></div>