[PATCH 4/4] drm: exynos: hdmi: Add dt support for hdmiphy settings

Shirish S shirish at chromium.org
Tue Dec 3 20:35:29 PST 2013


Hi Tomasz,
Thanks for the reivew, please see my replies inline.

On Fri, Nov 29, 2013 at 10:56 PM, Tomasz Figa <t.figa at samsung.com> wrote:
> Hi Shirish,
>
> Please see my comments inline.
>
> On Monday 25 of November 2013 14:24:39 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 at samsung.com>
>> ---
>>  .../devicetree/bindings/video/exynos_hdmi.txt      |   31 ++++++++
>>  drivers/gpu/drm/exynos/exynos_hdmi.c               |   77 +++++++++++++++++++-
>>  2 files changed, 104 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>> index 323983b..6eeb333 100644
>> --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>> +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>> @@ -13,6 +13,30 @@ Required properties:
>>       b) pin number within the gpio controller.
>>       c) optional flags and pull up/down.
>>
>> +- hdmiphy-configs: following information about the hdmiphy config settings.
>
> Is this node required or optional? If it's required, then it breaks
> compatibility with already existing DTBs, which is not desirable.
>
Yes its an Optional-but-recommended node, and i have mentioned the same
in this document in next patch set(v9).
>> +     a) "config<N>: config<N>" specifies the phy configuration settings,
>> +             where 'N' denotes the number of configuration, since every
>> +             pixel clock can have its unique configuration.
>
> Node names should not have any semantic meaning for parsing code. I know
> that there are already existing bindings which rely on presence of
> particularly named nodes, but that's not right and new bindings should
> not follow that.
>
I referred Documentation/devicetree/bindings/pinctrl/samsung-pinctrl.txt
for the implementation, am not clear with what you want me to do here, however
the requirement seems similar as pinctrl, can u kindly suggest any
existing newer
implementations to refer.
> Also what do you need the label of each config node for?
>
Each label here is a different pixel clock and corresponding phy setting, and
it may vary from one pixel clock to other hence i need one for each config node.
> Generally from parsing perspective you shouldn't really care about node
> names. All you seem to do in the driver is iterating over all specified
> nodes and matching them with internal driver data using pixel clock
> frequency.
>
True, that is what i intended to do.I think for the requirement
at hand, this should be fine.
>> +             "pixel-clock" specifies the pixel clock
>
> Vendor-specific properties should have vendor prefix, so this one should
> be called "samsung,pixel-clock".
>
Agreed, updated in the next patch set(v9).
>> +             "conifig-de-emphasis-level" provides fine control of TMDS data
>
> Typo: s/conifig/config
>
> Also it should be called "samsung,de-emphasis-level".
>
Agreed, updated in the next patch set(v9).
>> +              pre emphasis, below shown is example for
>> +             data de-emphasis register at address 0x145D0040.
>> +                     hdmiphy at 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 at 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.
>> +             "config-clock-level" provides fine control of TMDS data
>
> "samsung,clock-level"
>
Agreed, updated in the next patch set(v9).
>> +             amplitude for each channel,
>> +             for example if 0x145D005C is the address of clock level
> [snip]
>> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
>> index 32ce9a6..5f599e3 100644
>> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
>> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> [snip]
>> +static int drm_hdmi_dt_parse_phy_conf(struct platform_device *pdev,
>> +                                             struct hdmi_context *hdata)
>> +{
>> +     struct device *dev = &pdev->dev;
>> +     struct device_node *dev_np = dev->of_node;
>> +     struct device_node *phy_conf, *cfg_np;
>> +     int i, pixel_clock = 0;
>> +
>> +     /* Initialize with default config */
>> +     hdata->confs = hdmiphy_v14_configs;
>> +     hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
>> +
>> +     phy_conf = of_find_node_by_name(dev_np, "hdmiphy-configs");
>
> of_find_node_by_name() does not do what you need here. Please refer to
> its implementation to learn why.
>
> What you need here is of_get_child_by_name().
>
Agreed, updated in the next patch set(v9).
>> +     if (phy_conf == NULL) {
>> +             hdata->nr_confs = ARRAY_SIZE(hdmiphy_v14_configs);
>> +             DRM_ERROR("Did not find hdmiphy-configs node\n");
>> +             return -ENODEV;
>> +     }
>> +
>> +     for_each_child_of_node(phy_conf, cfg_np) {
>> +             if (!of_find_property(cfg_np, "pixel-clock", NULL))
>> +                     continue;
>
> This check is not needed. You can simply check the return value of
> of_property_read_u32() below (as you already do anyway).
>
Agreed, updated in the next patch set(v9).
>> +
>> +             if (of_property_read_u32(cfg_np, "pixel-clock",
>> +                                                     &pixel_clock, 1)) {
>> +                             DRM_ERROR("Failed to get pixel clock\n");
>> +                             return -EINVAL;
>> +             }
>> +
>> +             for (i = 0; i < ARRAY_SIZE(hdmiphy_v14_configs); i++) {
>
> The code would be much cleaner if you simply used the loop to find the
> config you need and then do the rest outside of the loop.
>
As you can see below, i need to update 2 values in the phy array,
which are 16 and 23 indexed values,
for me to move this out of the for loop would need to add a 3
dimensional array and run this
for loop again. Can we consider the below to be ok for the requirement at hand?
>> +                     if (hdata->confs[i].pixel_clock == pixel_clock)
>> +                             /* Update the data de-emphasis and data level */
>> +                             if (of_property_read_u8_array(cfg_np,
>> +                                      "config-de-emphasis-level",
>> +                                     &hdata->confs[i].conf[16], 1)) {
>> +                                     DRM_ERROR("Failed to get conf\n");
>> +                                     return -EINVAL;
>> +                             }
>> +                             if (of_property_read_u8_array(cfg_np,
>> +                                      "config-de-emphasis-level",
>> +                                     &hdata->confs[i].conf[16], 1)) {
>> +                                     DRM_ERROR("Failed to get conf\n");
>> +                                     return -EINVAL;
>> +                             }
>
> Why do you parse this property twice?
>
My bad, have updated in the next patch set.
>> +                             /* Update the clock level diff */
>> +                             if (of_property_read_u8_array(cfg_np,
>> +                                      "config-clock-level",
>> +                                     &hdata->confs[i].conf[23], 1)) {
>> +                                     DRM_ERROR("Failed to get conf\n");
>> +                                     return -EINVAL;
>> +                             }
>> +             }
>> +     }
>> +     return 0;
>> +
>> +}
>> +
>>  static struct s5p_hdmi_platform_data *drm_hdmi_dt_parse_pdata
>>                                       (struct device *dev)
>>  {
>> @@ -2024,6 +2084,15 @@ static int hdmi_probe(struct platform_device *pdev)
>>               goto err_hdmiphy;
>>       }
>>
>> +     /* get hdmiphy confs */
>> +     if (hdata->type == HDMI_TYPE14) {
>
> Why is this used only for HDMI_TYPE14?
>
Have extended it to both HDMI_TYPE13 and 14.However i dont have tested
values for TYPE13,
hence this would be dummy for that version.

> Best regards,
> Tomasz
>
Thanks & Regards,
Shirish S


More information about the dri-devel mailing list