Hi Tomasz,
Thanks for the review comments, please find my replies inline.
On Thu, Dec 19, 2013 at 6:49 PM, Tomasz Figa <t.figa@xxxxxxxxxxx> wrote:
> On Thursday 19 of December 2013 17:42:28 Shirish S wrote:
>> This patch adds dt support to hdmiphy config settings
>> as it is board specific and depends on the signal pattern
>> of board.
>>
>> Signed-off-by: Shirish S <s.shirish@xxxxxxxxxxx>
>> ---
>> .../devicetree/bindings/video/exynos_hdmi.txt | 34 ++++++++
>> drivers/gpu/drm/exynos/exynos_hdmi.c | 89 ++++++++++++++++----
>> 2 files changed, 105 insertions(+), 18 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>> index 323983b..0766e6e 100644
>> --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>> +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>> @@ -13,6 +13,31 @@ Required properties:
>> b) pin number within the gpio controller.
>> c) optional flags and pull up/down.
>>
>> +Optional-but-recommended properties:
>> +- hdmiphy-configs: following information about the hdmiphy config settings.
>> + a) "config<N>: config<N>" specifies the phy configuration settings,
>
> Why do you need this "config<N>: " part? (This is called "label" in DT
> terminology by the way and can be used to reference the node from
> properties of other nodes, by so called "phandle".)
>
The config is required for every pixel clock that the IP supports,
since in the parsing i iterate through all pixel clocks, i have used 'N'.
However, if you proposed approach below is ok, then all of this shall
be removed.
>> + where 'N' denotes the number of configuration, since every
>> + pixel clock can have its unique configuration.
>> + "samsung,pixel-clock" specifies the pixel clock
>> + "samsung,de-emphasis-level" provides fine control of TMDS data
>> + pre emphasis, below shown is example for
>> + data de-emphasis register at address 0x145D0040.
>> + hdmiphy@38[16] for bits[3:0] permitted values are in
>> + the range of 760 mVdiff to 1400 mVdiff at 20mVdiff
>> + increments for every LSB
>> + hdmiphy@38[16] for bits[7:4] permitted values are in
>> + the range of 0dB to -7.45dB at increments of -0.45dB
>> + for every LSB.
>> + "samsung,clock-level" provides fine control of TMDS data
>> + amplitude for each channel,
>> + for example if 0x145D005C is the address of clock level
>> + register then,
>> + hdmiphy@38[23] for bits [1:0] permitted values are in
>> + the range of 0 mVdiff & 60 mVdiff for each channel at
>> + increments 20 mVdiff of amplitude levels for every LSB,
>> + hdmiphy@38[23] for bits [7:3] permitted values are in
>> + the range of 790 and 1430 mV at 20mV increments for
>> + every LSB.
>> Example:
>>
>> hdmi {
>> @@ -20,4 +45,13 @@ Example:
>> reg = <0x14530000 0x100000>;
>> interrupts = <0 95 0>;
>> hpd-gpio = <&gpx3 7 1>;
>> + hdmiphy-configs {
>> + config0: config0 {
>> + samsung,pixel-clock = <25200000>;
>> + samsung,de-emphasis-level = /bits/ 8 <0x26>;
>
> nit: Two spaces before "/bits/".
have corrected in the next patchset.
>
>> + samsung,clock-level = /bits/ 8 < 0x66>;
>
> nit: Two spaces before "/bits/" and incorrect space after "<".
have corrected in the next patchset.
>
> Generally the list of configurations should look like below:
>
> phy-configs {
> #address-cells = <1>;
> #size-cells = <0>;
>
> config@0 {
> reg = <0>;
> /* other properties... */
> };
>
> config@1 {
> reg = <1>;
> /* other properties... */
> };
>
> /* ... */
> };
>
> This is how bus-like structures should be represented in device tree.
> Also, since this is HDMI node, maybe it's enough to call the node simply
> phy-configs. Please rework the patches to use this correct representation.
>
I think there is slight misunderstanding here, the configs that i want
to mention are not physical entities,and
hence dont think would be a better way, i am planning to use the below
method, if you think its the right approach then i shall do the rework
and submit the patch:
phy-configs {
#pixel-clock = <1>;
#de-emphasis-level = <1>;
#clock-level = <1>;
phy-map = <25200000 0x26 0x66>,
<27000000 0x26 0x66>,
/*....so on....*/,
};>> +
>> + /* ... */
>> + }
>> };
> [snip]
>> + for_each_child_of_node(phy_conf, cfg_np) {
>> + if (of_property_read_u32(cfg_np, "samsung,pixel-clock",
>> + &pixel_clock))
>> + continue;
>> +
>> + for (i = 0; i < ARRAY_SIZE(hdata->nr_confs); i++) {
>> + if (hdata->confs[i].pixel_clock == pixel_clock)
>
> Can you have more than one config with the same pixel clock?
>
No, right now i intend to have one config parameters for every
corresponding pixel clock.
> Even if not, the code could be made more readable if the code
> below is moved outside the if and continue keyword is used instead.
>
The parsing mechanism shall be altered according to the new approach
proposed above.
Kindly let me know your thoughts about the approach.
> Best regards,
> Tomasz
>
Thanks & Regards,
Shirish S
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
