+ linux-samsung-soc mailing list.
On Wed, Dec 4, 2013 at 10:05 AM, Shirish S <shirish@xxxxxxxxxxxx> wrote:
> Hi Tomasz,
> Thanks for the reivew, please see my replies inline.
>
> On Fri, Nov 29, 2013 at 10:56 PM, Tomasz Figa <t.figa@xxxxxxxxxxx> 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@xxxxxxxxxxx>
>>> ---
>>> .../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@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.
>>> + "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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
