[PATCH 0/3] Add dt support for exynos hdmiphy settings

[PATCH 0/3] Add dt support for exynos hdmiphy settings

[Shirish S]

For various revisions of a chipset if the signal pattern is changed for every
revision, then the phy setting need to be updated correspondingly by measuring
the signal.
For getting correct signals the clock level and data de-emphasis 
levels needs to be adjusted.
Since only these 2 values matter,we can move the same to dt, 
wherein we can have different dt files for every revision. 

This is an initial patchset towards achieving the same 
for exynos 5250 and can be later extended to future chipsets.

V2: replaced moving of entire phy config structure with only
required and justifiable conf registers.

Shirish S (3):
  ARM: dts: smdk5250: Add hdmi phy settings
  ARM: dts: arndale: Add hdmi phy settings
  drm: exynos: hdmi: Add dt support for hdmiphy settings

 .../devicetree/bindings/video/exynos_hdmi.txt      |   29 ++++++++
 arch/arm/boot/dts/exynos5250-arndale.dts           |   68 +++++++++++++++++++
 arch/arm/boot/dts/exynos5250-smdk5250.dts          |   68 +++++++++++++++++++
 drivers/gpu/drm/exynos/exynos_hdmi.c               |   70 ++++++++++++++++++--
 4 files changed, 231 insertions(+), 4 deletions(-)

-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] ARM: dts: smdk5250: Add hdmi phy settings

[Shirish S]

This patch moves the hdmi phy setting to smdk5250
dts,as its more of a per board configuration and
also shall be easier for supporting future chipsets.

Signed-off-by: Shirish S <s.shirish-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 arch/arm/boot/dts/exynos5250-smdk5250.dts |   68 +++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts b/arch/arm/boot/dts/exynos5250-smdk5250.dts
index 49f18c2..d6d0801 100644
--- a/arch/arm/boot/dts/exynos5250-smdk5250.dts
+++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts
@@ -220,6 +220,74 @@
 
 	hdmi {
 		hpd-gpio = <&gpx3 7 0>;
+		hdmiphy-confs {
+			nr-confs = <13>;
+			conf0: conf0 {
+				clock-frequency = <25200000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf1: conf1 {
+				clock-frequency = <27000000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf2: conf2 {
+				clock-frequency = <27027000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf3: conf3 {
+				clock-frequency = <36000000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf4: conf4 {
+				clock-frequency = <40000000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf5: conf5 {
+				clock-frequency = <65000000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf6: conf6 {
+				clock-frequency = <74176000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf7: conf7 {
+				clock-frequency = <74250000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf8: conf8 {
+				clock-frequency = <83500000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf9: conf9 {
+				clock-frequency = <106500000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf10: conf10 {
+				clock-frequency = <108000000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf11: conf11 {
+				clock-frequency = <146250000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf12: conf12 {
+				clock-frequency = <148500000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+		};
 	};
 
 	codec@11000000 {
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] ARM: dts: arndale: Add hdmi phy settings

[Shirish S]

This patch moves the hdmi phy setting to arndale dts,
as its more of a per board configuration and also
shall be easier for supporting future chipsets.

Signed-off-by: Shirish S <s.shirish-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 arch/arm/boot/dts/exynos5250-arndale.dts |   68 ++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts b/arch/arm/boot/dts/exynos5250-arndale.dts
index abc7272..c23f16b 100644
--- a/arch/arm/boot/dts/exynos5250-arndale.dts
+++ b/arch/arm/boot/dts/exynos5250-arndale.dts
@@ -424,6 +424,74 @@
 
 	hdmi {
 		hpd-gpio = <&gpx3 7 2>;
+		hdmiphy-confs {
+			nr-confs = <13>;
+			conf0: conf0 {
+				clock-frequency = <25200000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf1: conf1 {
+				clock-frequency = <27000000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf2: conf2 {
+				clock-frequency = <27027000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf3: conf3 {
+				clock-frequency = <36000000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf4: conf4 {
+				clock-frequency = <40000000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf5: conf5 {
+				clock-frequency = <65000000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf6: conf6 {
+				clock-frequency = <74176000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf7: conf7 {
+				clock-frequency = <74250000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf8: conf8 {
+				clock-frequency = <83500000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf9: conf9 {
+				clock-frequency = <106500000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf10: conf10 {
+				clock-frequency = <108000000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf11: conf11 {
+				clock-frequency = <146250000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+			conf12: conf12 {
+				clock-frequency = <148500000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+		};
 		vdd_osc-supply = <&ldo10_reg>;
 		vdd_pll-supply = <&ldo8_reg>;
 		vdd-supply = <&ldo8_reg>;
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Shirish S]

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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 .../devicetree/bindings/video/exynos_hdmi.txt      |   29 ++++++++
 arch/arm/boot/dts/exynos5250-arndale.dts           |    6 +-
 drivers/gpu/drm/exynos/exynos_hdmi.c               |   70 ++++++++++++++++++--
 3 files changed, 98 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
index 323983b..770f92d 100644
--- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
+++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
@@ -13,6 +13,27 @@ Required properties:
 	b) pin number within the gpio controller.
 	c) optional flags and pull up/down.
 
+- hdmiphy-confs: following information about the hdmiphy conf settings.
+        a) "nr-confs" specifies the number of pixel clocks supported.
+	b) "confX: confX" specifies the phy configuration settings,
+		"clock-frequency" specifies the pixel clock
+		"con-de-emphasis-level" specifies the configuration
+		of Data De-emphasis levels.
+			0x145D0040h[3:0] permitted values:
+				0000 means 760 mVdiff && 1111 means 1400 mVdiff
+			0x145D0040h[7:4] permitted values:
+				000 	0dB
+				0001	-0.25dB
+				0010	-0.7dB
+				0011	-1.15dB
+				1111	-7.45dB
+		"con-clock-level" specifies the configuration for
+		the corresponding clock level.
+			0x145D005Ch [1:0] permitted values:
+				00 means 0 mVdiff && 11 means 60 mVdiff
+			0x145D005Ch [7:3] permitted values:
+				00000 is 790 mVdiff
+				11111 is 1430 mVdiff
 Example:
 
 	hdmi {
@@ -20,4 +41,12 @@ Example:
 		reg = <0x14530000 0x100000>;
 		interrupts = <0 95 0>;
 		hpd-gpio = <&gpx3 7 1>;
+		hdmiphy-confs {
+			nr-confs = <1>;
+			conf0: conf0 {
+				clock-frequency = <25200000>;
+				conf-de-emphasis-level =  /bits/ 8 <0x26>;
+				conf-clock-level =  /bits/ 8 < 0x66>;
+			};
+		}
 	};
diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts b/arch/arm/boot/dts/exynos5250-arndale.dts
index c23f16b..436b75a 100644
--- a/arch/arm/boot/dts/exynos5250-arndale.dts
+++ b/arch/arm/boot/dts/exynos5250-arndale.dts
@@ -424,6 +424,9 @@
 
 	hdmi {
 		hpd-gpio = <&gpx3 7 2>;
+		vdd_osc-supply = <&ldo10_reg>;
+		vdd_pll-supply = <&ldo8_reg>;
+		vdd-supply = <&ldo8_reg>;
 		hdmiphy-confs {
 			nr-confs = <13>;
 			conf0: conf0 {
@@ -492,9 +495,6 @@
 				conf-clock-level =  /bits/ 8 < 0x66>;
 			};
 		};
-		vdd_osc-supply = <&ldo10_reg>;
-		vdd_pll-supply = <&ldo8_reg>;
-		vdd-supply = <&ldo8_reg>;
 	};
 
 	mmc_reg: voltage-regulator {
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index a0e10ae..3125e67 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -200,6 +200,9 @@ struct hdmi_context {
 
 	struct hdmi_resources		res;
 
+	struct hdmiphy_config		*confs;
+	int				nr_confs;
+
 	int				hpd_gpio;
 
 	enum hdmi_type			type;
@@ -259,7 +262,7 @@ static const struct hdmiphy_config hdmiphy_v13_configs[] = {
 	},
 };
 
-static const struct hdmiphy_config hdmiphy_v14_configs[] = {
+static struct hdmiphy_config hdmiphy_v14_configs[] = {
 	{
 		.pixel_clock = 25200000,
 		.conf = {
@@ -778,8 +781,8 @@ static int hdmi_find_phy_conf(struct hdmi_context *hdata, u32 pixel_clock)
 		confs = hdmiphy_v13_configs;
 		count = ARRAY_SIZE(hdmiphy_v13_configs);
 	} else if (hdata->type == HDMI_TYPE14) {
-		confs = hdmiphy_v14_configs;
-		count = ARRAY_SIZE(hdmiphy_v14_configs);
+		confs = hdata->confs;
+		count = hdata->nr_confs;
 	} else
 		return -EINVAL;
 
@@ -1366,7 +1369,7 @@ static void hdmiphy_conf_apply(struct hdmi_context *hdata)
 	if (hdata->type == HDMI_TYPE13)
 		hdmiphy_data = hdmiphy_v13_configs[i].conf;
 	else
-		hdmiphy_data = hdmiphy_v14_configs[i].conf;
+		hdmiphy_data = hdata->confs[i].conf;
 
 	memcpy(buffer, hdmiphy_data, 32);
 	ret = i2c_master_send(hdata->hdmiphy_port, buffer, 32);
@@ -1858,6 +1861,56 @@ void hdmi_attach_hdmiphy_client(struct i2c_client *hdmiphy)
 		hdmi_hdmiphy = hdmiphy;
 }
 
+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 = 0;
+
+	phy_conf = of_find_node_by_name(dev_np, "hdmiphy-confs");
+	if (phy_conf == NULL) {
+		DRM_ERROR("Did not find hdmiphy_conf node\n");
+		return -ENODEV;
+	}
+
+	of_property_read_u32(phy_conf, "nr-confs", &hdata->nr_confs);
+	hdata->confs = kzalloc((hdata->nr_confs * sizeof
+					(struct hdmiphy_config)), GFP_KERNEL);
+
+	/* Initialize with default config */
+	hdata->confs = hdmiphy_v14_configs;
+
+	for_each_child_of_node(phy_conf, cfg_np) {
+		if (!of_find_property(cfg_np, "clock-frequency", NULL))
+			continue;
+
+		if (of_property_read_u32_array(cfg_np, "clock-frequency",
+					(u32 *)&hdata->confs[i].
+							pixel_clock, 1)) {
+				DRM_ERROR("Failed to get pixel clock\n");
+				return -EINVAL;
+		}
+
+		/* Overwrite the data de-emphasis and data level */
+		if (of_property_read_u8_array(cfg_np, "conf-de-emphasis-level",
+					(u8 *)&hdata->confs[i].conf[16], 1)) {
+				DRM_ERROR("Failed to get conf\n");
+				return -EINVAL;
+		}
+		/* Overwrite the clock level diff */
+		if (of_property_read_u8_array(cfg_np, "conf-clock-level",
+					(u8 *)&hdata->confs[i].conf[23], 1)) {
+				DRM_ERROR("Failed to get conf\n");
+				return -EINVAL;
+		}
+		i++;
+	}
+	return 0;
+
+}
+
 static struct s5p_hdmi_platform_data *drm_hdmi_dt_parse_pdata
 					(struct device *dev)
 {
@@ -1986,6 +2039,15 @@ static int hdmi_probe(struct platform_device *pdev)
 		goto err_hdmiphy;
 	}
 
+	/* get hdmiphy confs */
+	if (hdata->type == HDMI_TYPE14) {
+		ret = drm_hdmi_dt_parse_phy_conf(pdev, hdata);
+		if (ret) {
+			DRM_ERROR("failed to get confs\n");
+			goto err_hdmiphy;
+		}
+	}
+
 	/* Attach HDMI Driver to common hdmi. */
 	exynos_hdmi_drv_attach(drm_hdmi_ctx);
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mark Rutland]

Hi,

On Mon, Oct 28, 2013 at 06:24:22AM +0000, 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  .../devicetree/bindings/video/exynos_hdmi.txt      |   29 ++++++++
>  arch/arm/boot/dts/exynos5250-arndale.dts           |    6 +-
>  drivers/gpu/drm/exynos/exynos_hdmi.c               |   70 ++++++++++++++++++--
>  3 files changed, 98 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> index 323983b..770f92d 100644
> --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> @@ -13,6 +13,27 @@ Required properties:
>  	b) pin number within the gpio controller.
>  	c) optional flags and pull up/down.
>  
> +- hdmiphy-confs: following information about the hdmiphy conf settings.

Judging by the other patches, this is a node, not a property.

> +        a) "nr-confs" specifies the number of pixel clocks supported.

Why is this needed? Someone will get it wrong eventually and it can be figured
out currently by counting the child nodes, testing if they have the appropriate
properties.

> +	b) "confX: confX" specifies the phy configuration settings,

This is confusing. What is X?

The label is irrelevant -- none of this patch looks for phandles pointing at
configurations, nor is the precise name of the label important.

This is a node, not a property.

> +		"clock-frequency" specifies the pixel clock

Is this a frequency to configure the pixel clock with, or the pre-determined
frequency of a clock that we will select?

> +		"con-de-emphasis-level" specifies the configuration
> +		of Data De-emphasis levels.

Please explain _why_ we need this configuration.

Also, "con" is not a good abbreviation of "configuration", "config" would be
preferable.

> +			0x145D0040h[3:0] permitted values:
> +				0000 means 760 mVdiff && 1111 means 1400 mVdiff

I assume the 'h' suffix is a redundant description that the constant is
hexadecimal. Please drop it.

What is 0x145D0040? The address of the register, or its value?

The description is confusing, 0x145D0040h[3:0] is always 0[3:0].

Why does this need to be the exact value programmed into the register rather
than separate values the driver can compose?

> +			0x145D0040h[7:4] permitted values:
> +				000 	0dB
> +				0001	-0.25dB
> +				0010	-0.7dB
> +				0011	-1.15dB
> +				1111	-7.45dB

Again, this seems very odd. Why this format?

> +		"con-clock-level" specifies the configuration for
> +		the corresponding clock level.

To repeat my comment on "con-de-emphasis-level", "con" is not a good
abbreviation for "configuration".

> +			0x145D005Ch [1:0] permitted values:
> +				00 means 0 mVdiff && 11 means 60 mVdiff
> +			0x145D005Ch [7:3] permitted values:
> +				00000 is 790 mVdiff
> +				11111 is 1430 mVdiff

Please explain why we must use this exact format rather than one which may be
understood more easily.

>  Example:
>  
>  	hdmi {
> @@ -20,4 +41,12 @@ Example:
>  		reg = <0x14530000 0x100000>;
>  		interrupts = <0 95 0>;
>  		hpd-gpio = <&gpx3 7 1>;
> +		hdmiphy-confs {
> +			nr-confs = <1>;
> +			conf0: conf0 {
> +				clock-frequency = <25200000>;
> +				conf-de-emphasis-level =  /bits/ 8 <0x26>;
> +				conf-clock-level =  /bits/ 8 < 0x66>;

Why are these 8-bit? That wasn't described in the binding at all so far.

> +			};
> +		}
>  	};
> diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts b/arch/arm/boot/dts/exynos5250-arndale.dts
> index c23f16b..436b75a 100644
> --- a/arch/arm/boot/dts/exynos5250-arndale.dts
> +++ b/arch/arm/boot/dts/exynos5250-arndale.dts
> @@ -424,6 +424,9 @@
>  
>  	hdmi {
>  		hpd-gpio = <&gpx3 7 2>;
> +		vdd_osc-supply = <&ldo10_reg>;
> +		vdd_pll-supply = <&ldo8_reg>;
> +		vdd-supply = <&ldo8_reg>;
>  		hdmiphy-confs {
>  			nr-confs = <13>;
>  			conf0: conf0 {
> @@ -492,9 +495,6 @@
>  				conf-clock-level =  /bits/ 8 < 0x66>;
>  			};
>  		};
> -		vdd_osc-supply = <&ldo10_reg>;
> -		vdd_pll-supply = <&ldo8_reg>;
> -		vdd-supply = <&ldo8_reg>;
>  	};
>  
>  	mmc_reg: voltage-regulator {

Why is this moving? It in no way relates to the rest of the patch, and wasn't
described in the commit message.

[...]

> +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 = 0;
> +
> +	phy_conf = of_find_node_by_name(dev_np, "hdmiphy-confs");
> +	if (phy_conf == NULL) {
> +		DRM_ERROR("Did not find hdmiphy_conf node\n");
> +		return -ENODEV;
> +	}
> +
> +	of_property_read_u32(phy_conf, "nr-confs", &hdata->nr_confs);

This can fail, returning an error code. Either check it, or remove this
property entirely and do this based on the number of children which have the
appropriate properties.

> +	hdata->confs = kzalloc((hdata->nr_confs * sizeof
> +					(struct hdmiphy_config)), GFP_KERNEL);

Why the brackets on the first argument of kzalloc? They're superfluous.

What if kzalloc fails?

> +
> +	/* Initialize with default config */
> +	hdata->confs = hdmiphy_v14_configs;
> +
> +	for_each_child_of_node(phy_conf, cfg_np) {

What if nr-confs is smaller than the number of child nodes?

> +		if (!of_find_property(cfg_np, "clock-frequency", NULL))
> +			continue;
> +
> +		if (of_property_read_u32_array(cfg_np, "clock-frequency",
> +					(u32 *)&hdata->confs[i].
> +							pixel_clock, 1)) {

Don't split &hdata->confs[i].pixel_clock over two lines.

Why is the cast necessary?

Why not just of_property_read_u32? This only ever has a single value, and while
there's no of_property_read_u8, of_property_read_u32 exists.

> +				DRM_ERROR("Failed to get pixel clock\n");
> +				return -EINVAL;
> +		}
> +
> +		/* Overwrite the data de-emphasis and data level */
> +		if (of_property_read_u8_array(cfg_np, "conf-de-emphasis-level",
> +					(u8 *)&hdata->confs[i].conf[16], 1)) {

Why is this cast necessary?

I don't see why this must be an 8 bit property.

> +				DRM_ERROR("Failed to get conf\n");
> +				return -EINVAL;
> +		}
> +		/* Overwrite the clock level diff */
> +		if (of_property_read_u8_array(cfg_np, "conf-clock-level",
> +					(u8 *)&hdata->confs[i].conf[23], 1)) {

Why the cast?

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Shirish S]

[-- Attachment #1.1: Type: text/plain, Size: 10304 bytes --]

Hi Mark,
Firstly thanks for reviewing.


On Mon, Oct 28, 2013 at 12:20 PM, Mark Rutland <mark.rutland@arm.com> wrote:

> Hi,
>
> On Mon, Oct 28, 2013 at 06:24:22AM +0000, 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@samsung.com>
> > ---
> >  .../devicetree/bindings/video/exynos_hdmi.txt      |   29 ++++++++
> >  arch/arm/boot/dts/exynos5250-arndale.dts           |    6 +-
> >  drivers/gpu/drm/exynos/exynos_hdmi.c               |   70
> ++++++++++++++++++--
> >  3 files changed, 98 insertions(+), 7 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> > index 323983b..770f92d 100644
> > --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> > +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
> > @@ -13,6 +13,27 @@ Required properties:
> >       b) pin number within the gpio controller.
> >       c) optional flags and pull up/down.
> >
> > +- hdmiphy-confs: following information about the hdmiphy conf settings.
>
> Judging by the other patches, this is a node, not a property.
>
> Yes its a node.

> > +        a) "nr-confs" specifies the number of pixel clocks supported.
>
> Why is this needed? Someone will get it wrong eventually and it can be
> figured
> out currently by counting the child nodes, testing if they have the
> appropriate
> properties.
>
> Actually i need to get the array size also from dt, hence this is approach
i have taken

> > +     b) "confX: confX" specifies the phy configuration settings,
>
> This is confusing. What is X?
>
> I am trying to generalize, here X means any numerical, and the programmer
needs to
make sure conf0:conf0, wherein X is 0.I shall provide the values permitted
for X in my next patch set.

> The label is irrelevant -- none of this patch looks for phandles pointing
> at
> configurations, nor is the precise name of the label important.
>
> This is a node, not a property.
>
> Ideally every conf<numerical value>  a combination of pixel clock and new
values for data and clock level.

> > +             "clock-frequency" specifies the pixel clock
>
> Is this a frequency to configure the pixel clock with, or the
> pre-determined
> frequency of a clock that we will select?
>
> No, as the explanation suggests its the pixel clock itself.

> > +             "con-de-emphasis-level" specifies the configuration
> > +             of Data De-emphasis levels.
>
> Please explain _why_ we need this configuration.
>
> Our chipset to undergo HDMI compliance test and noticed that the HDMI
Compliance Test id 7-10 was failing
for eye diagram test. Hence on further analysis, it was found that altering
the data de-emphasis levels and clock
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.

> Also, "con" is not a good abbreviation of "configuration", "config" would
> be
> preferable.
>
> Agreed, will update the same in next patch set.

> > +                     0x145D0040h[3:0] permitted values:
> > +                             0000 means 760 mVdiff && 1111 means 1400
> mVdiff
>
> I assume the 'h' suffix is a redundant description that the constant is
> hexadecimal. Please drop it.
>
> Agreed, will update the same in next patch set.

> What is 0x145D0040? The address of the register, or its value?
>
> Its the address of the hdmiphy register for data level configuration.


> The description is confusing, 0x145D0040h[3:0] is always 0[3:0].
>
> This description is extracted from the one specified in manual, in my
first patch set the reviewers had asked me
to provide the explaination for every bit, which i have provided.

> Why does this need to be the exact value programmed into the register
> rather
> than separate values the driver can compose?
>
> As mentioned above the value is must for clearing the test 7-10, and also
its derived by a trial and error method
with the HDMI analyser.

> > +                     0x145D0040h[7:4] permitted values:
> > +                             000     0dB
> > +                             0001    -0.25dB
> > +                             0010    -0.7dB
> > +                             0011    -1.15dB
> > +                             1111    -7.45dB
>
> Again, this seems very odd. Why this format?
>
> 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.

> > +             "con-clock-level" specifies the configuration for
> > +             the corresponding clock level.
>
> To repeat my comment on "con-de-emphasis-level", "con" is not a good
> abbreviation for "configuration".
>
> Agreed, will update the same in next patch set.

> > +                     0x145D005Ch [1:0] permitted values:
> > +                             00 means 0 mVdiff && 11 means 60 mVdiff
> > +                     0x145D005Ch [7:3] permitted values:
> > +                             00000 is 790 mVdiff
> > +                             11111 is 1430 mVdiff
>
> Please explain why we must use this exact format rather than one which may
> be
> understood more easily.
>
> I hope by now have made this point clear!

> >  Example:
> >
> >       hdmi {
> > @@ -20,4 +41,12 @@ Example:
> >               reg = <0x14530000 0x100000>;
> >               interrupts = <0 95 0>;
> >               hpd-gpio = <&gpx3 7 1>;
> > +             hdmiphy-confs {
> > +                     nr-confs = <1>;
> > +                     conf0: conf0 {
> > +                             clock-frequency = <25200000>;
> > +                             conf-de-emphasis-level =  /bits/ 8 <0x26>;
> > +                             conf-clock-level =  /bits/ 8 < 0x66>;
>
> Why are these 8-bit? That wasn't described in the binding at all so far.
>
> These are 8 bit by design(as mentioned in the manual) and were not part of
device tree to be explained before.

> > +                     };
> > +             }
> >       };
> > diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts
> b/arch/arm/boot/dts/exynos5250-arndale.dts
> > index c23f16b..436b75a 100644
> > --- a/arch/arm/boot/dts/exynos5250-arndale.dts
> > +++ b/arch/arm/boot/dts/exynos5250-arndale.dts
> > @@ -424,6 +424,9 @@
> >
> >       hdmi {
> >               hpd-gpio = <&gpx3 7 2>;
> > +             vdd_osc-supply = <&ldo10_reg>;
> > +             vdd_pll-supply = <&ldo8_reg>;
> > +             vdd-supply = <&ldo8_reg>;
> >               hdmiphy-confs {
> >                       nr-confs = <13>;
> >                       conf0: conf0 {
> > @@ -492,9 +495,6 @@
> >                               conf-clock-level =  /bits/ 8 < 0x66>;
> >                       };
> >               };
> > -             vdd_osc-supply = <&ldo10_reg>;
> > -             vdd_pll-supply = <&ldo8_reg>;
> > -             vdd-supply = <&ldo8_reg>;
> >       };
> >
> >       mmc_reg: voltage-regulator {
>
> Why is this moving? It in no way relates to the rest of the patch, and
> wasn't
> described in the commit message.
>
> Oops that was supposed to be part of previous pach,will update it.

> [...]
>
> > +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 = 0;
> > +
> > +     phy_conf = of_find_node_by_name(dev_np, "hdmiphy-confs");
> > +     if (phy_conf == NULL) {
> > +             DRM_ERROR("Did not find hdmiphy_conf node\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     of_property_read_u32(phy_conf, "nr-confs", &hdata->nr_confs);
>
> This can fail, returning an error code. Either check it, or remove this
> property entirely and do this based on the number of children which have
> the
> appropriate properties.
>
> Ok will add a check.

> > +     hdata->confs = kzalloc((hdata->nr_confs * sizeof
> > +                                     (struct hdmiphy_config)),
> GFP_KERNEL);
>
> Why the brackets on the first argument of kzalloc? They're superfluous.
>
> What if kzalloc fails?
>
> Ok, will put a check.

> > +
> > +     /* Initialize with default config */
> > +     hdata->confs = hdmiphy_v14_configs;
> > +
> > +     for_each_child_of_node(phy_conf, cfg_np) {
>
> What if nr-confs is smaller than the number of child nodes?
>
> it basically is the array size and it should be 1 + child nodes as
mentioned in the descriptions, i
assume that the programmer takes care of it.

> > +             if (!of_find_property(cfg_np, "clock-frequency", NULL))
> > +                     continue;
> > +
> > +             if (of_property_read_u32_array(cfg_np, "clock-frequency",
> > +                                     (u32 *)&hdata->confs[i].
> > +                                                     pixel_clock, 1)) {
>
> Don't split &hdata->confs[i].pixel_clock over two lines.
>
> Why is the cast necessary?
>
> Why not just of_property_read_u32? This only ever has a single value, and
> while
> there's no of_property_read_u8, of_property_read_u32 exists.
>
> Ok, agreed.

> > +                             DRM_ERROR("Failed to get pixel clock\n");
> > +                             return -EINVAL;
> > +             }
> > +
> > +             /* Overwrite the data de-emphasis and data level */
> > +             if (of_property_read_u8_array(cfg_np,
> "conf-de-emphasis-level",
> > +                                     (u8 *)&hdata->confs[i].conf[16],
> 1)) {
>
> Why is this cast necessary?
>
> I don't see why this must be an 8 bit property.
>
> As mentioned earlier its by design 8 bits.

> > +                             DRM_ERROR("Failed to get conf\n");
> > +                             return -EINVAL;
> > +             }
> > +             /* Overwrite the clock level diff */
> > +             if (of_property_read_u8_array(cfg_np, "conf-clock-level",
> > +                                     (u8 *)&hdata->confs[i].conf[23],
> 1)) {
>
> Why the cast?
>
> Same explaination as above.

> Thanks,
> Mark.
>
Thanks,
Shirish S

[-- Attachment #1.2: Type: text/html, Size: 17233 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Shirish S]

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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 .../devicetree/bindings/video/exynos_hdmi.txt      |   32 +++++++++
 drivers/gpu/drm/exynos/exynos_hdmi.c               |   76 ++++++++++++++++++--
 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..c3b546a 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.
+        a) "nr-configs" specifies the number of pixel clocks supported.
+	b) "config<N>: config<N>" specifies the phy configuration settings,
+		wher 'N' denotes the number of iteration.
+		"pixel-clock" specifies the pixel clock
+		"conifig-de-emphasis-level" specifies the 8 bit configuration
+		of Data De-emphasis levels,below shown is example for
+		data de-emphasis register at address 0x145D0040.
+			0x145D0040 [3:0] permitted values:
+				0000 means 760 mVdiff && 1111 means 1400 mVdiff
+			0x145D0040 [7:4] permitted values:
+				0000 	0dB
+				0001	-0.25dB
+				0010	-0.7dB
+				0011	-1.15dB
+				1111	-7.45dB
+		"config-clock-level" specifies the 8 bit configuration for
+		the corresponding clock level, for example if 0x145D005C
+		is the address of clock level register.
+			0x145D005C [1:0] permitted values:
+				00 means 0 mVdiff && 11 means 60 mVdiff
+			0x145D005C [7:3] permitted values:
+				00000 is 790 mVdiff
+				11111 is 1430 mVdiff
 Example:
 
 	hdmi {
@@ -20,4 +44,12 @@ Example:
 		reg = <0x14530000 0x100000>;
 		interrupts = <0 95 0>;
 		hpd-gpio = <&gpx3 7 1>;
+		hdmiphy-configs {
+			nr-configs = <1>;
+			config0: config0 {
+				pixel-clock = <25200000>;
+				config-de-emphasis-level =  /bits/ 8 <0x26>;
+				config-clock-level =  /bits/ 8 < 0x66>;
+			};
+		}
 	};
diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
index a0e10ae..7b94a5d 100644
--- a/drivers/gpu/drm/exynos/exynos_hdmi.c
+++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
@@ -200,6 +200,9 @@ struct hdmi_context {
 
 	struct hdmi_resources		res;
 
+	struct hdmiphy_config		*confs;
+	int				nr_confs;
+
 	int				hpd_gpio;
 
 	enum hdmi_type			type;
@@ -259,7 +262,7 @@ static const struct hdmiphy_config hdmiphy_v13_configs[] = {
 	},
 };
 
-static const struct hdmiphy_config hdmiphy_v14_configs[] = {
+static struct hdmiphy_config hdmiphy_v14_configs[] = {
 	{
 		.pixel_clock = 25200000,
 		.conf = {
@@ -778,8 +781,8 @@ static int hdmi_find_phy_conf(struct hdmi_context *hdata, u32 pixel_clock)
 		confs = hdmiphy_v13_configs;
 		count = ARRAY_SIZE(hdmiphy_v13_configs);
 	} else if (hdata->type == HDMI_TYPE14) {
-		confs = hdmiphy_v14_configs;
-		count = ARRAY_SIZE(hdmiphy_v14_configs);
+		confs = hdata->confs;
+		count = hdata->nr_confs;
 	} else
 		return -EINVAL;
 
@@ -1366,7 +1369,7 @@ static void hdmiphy_conf_apply(struct hdmi_context *hdata)
 	if (hdata->type == HDMI_TYPE13)
 		hdmiphy_data = hdmiphy_v13_configs[i].conf;
 	else
-		hdmiphy_data = hdmiphy_v14_configs[i].conf;
+		hdmiphy_data = hdata->confs[i].conf;
 
 	memcpy(buffer, hdmiphy_data, 32);
 	ret = i2c_master_send(hdata->hdmiphy_port, buffer, 32);
@@ -1858,6 +1861,62 @@ void hdmi_attach_hdmiphy_client(struct i2c_client *hdmiphy)
 		hdmi_hdmiphy = hdmiphy;
 }
 
+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 = 0;
+
+	/* Initialize with default config */
+	hdata->confs = hdmiphy_v14_configs;
+
+	phy_conf = of_find_node_by_name(dev_np, "hdmiphy-configs");
+	if (phy_conf == NULL) {
+		DRM_ERROR("Did not find hdmiphy-configs node\n");
+		return -ENODEV;
+	}
+
+	if (of_property_read_u32(phy_conf, "nr-configs", &hdata->nr_confs)) {
+		DRM_ERROR("Failed to get the number of configurations");
+		return -EINVAL;
+	}
+
+	if (hdata->nr_confs < ARRAY_SIZE(hdmiphy_v14_configs)) {
+		DRM_ERROR("mismatch in the number of configs\n");
+		return -EINVAL;
+	}
+
+	for_each_child_of_node(phy_conf, cfg_np) {
+		if (!of_find_property(cfg_np, "pixel-clock", NULL))
+			continue;
+
+		if (of_property_read_u32_array(cfg_np, "pixel-clock",
+					&hdata->confs[i].pixel_clock, 1)) {
+				DRM_ERROR("Failed to get pixel clock\n");
+				return -EINVAL;
+		}
+
+		/* Overwrite 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;
+		}
+		/* Overwrite 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;
+		}
+		i++;
+	}
+	return 0;
+
+}
+
 static struct s5p_hdmi_platform_data *drm_hdmi_dt_parse_pdata
 					(struct device *dev)
 {
@@ -1986,6 +2045,15 @@ static int hdmi_probe(struct platform_device *pdev)
 		goto err_hdmiphy;
 	}
 
+	/* get hdmiphy confs */
+	if (hdata->type == HDMI_TYPE14) {
+		ret = drm_hdmi_dt_parse_phy_conf(pdev, hdata);
+		if (ret) {
+			DRM_ERROR("failed to get user defined config,will use
+			default configs, eye diagram tests may fail\n");
+		}
+	}
+
 	/* Attach HDMI Driver to common hdmi. */
 	exynos_hdmi_drv_attach(drm_hdmi_ctx);
 
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mark Rutland]

On Mon, Oct 28, 2013 at 10:15:00AM +0000, Shirish S wrote:
> Hi Mark,
> Firstly thanks for reviewing.

Hi,

Please could you refrain from replying in HTML and use plaintext, it's rather
difficult to respond sensibly.

> 
> 
> On Mon, Oct 28, 2013 at 12:20 PM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> 
>     Hi,
> 
>     On Mon, Oct 28, 2013 at 06:24:22AM +0000, 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>     > ---
>     >  .../devicetree/bindings/video/exynos_hdmi.txt      |   29 ++++++++
>     >  arch/arm/boot/dts/exynos5250-arndale.dts           |    6 +-
>     >  drivers/gpu/drm/exynos/exynos_hdmi.c               |   70
>     ++++++++++++++++++--
>     >  3 files changed, 98 insertions(+), 7 deletions(-)
>     >
>     > diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/
>     Documentation/devicetree/bindings/video/exynos_hdmi.txt
>     > index 323983b..770f92d 100644
>     > --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>     > +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>     > @@ -13,6 +13,27 @@ Required properties:
>     >       b) pin number within the gpio controller.
>     >       c) optional flags and pull up/down.
>     >
>     > +- hdmiphy-confs: following information about the hdmiphy conf settings.
> 
>     Judging by the other patches, this is a node, not a property.
> 
> 
> Yes its a node. 

My point was that the documentation should reflect this.

> 
>     > +        a) "nr-confs" specifies the number of pixel clocks supported.
> 
>     Why is this needed? Someone will get it wrong eventually and it can be
>     figured
>     out currently by counting the child nodes, testing if they have the
>     appropriate
>     properties.
> 
> 
> Actually i need to get the array size also from dt, hence this is approach i
> have taken 

While this approach works now for perfect DTs, as I pointed out it is encoding
redundant information and could be broken by future changes. Please fix this.

> 
>     > +     b) "confX: confX" specifies the phy configuration settings,
> 
>     This is confusing. What is X?
> 
> 
> I am trying to generalize, here X means any numerical, and the programmer needs
> to 
> make sure conf0:conf0, wherein X is 0.I shall provide the values permitted for
> X in my next patch set.

Please be explicit with your definitions. Experience shows that people will get
this wrong.

> 
>     The label is irrelevant -- none of this patch looks for phandles pointing
>     at
>     configurations, nor is the precise name of the label important.
> 
>     This is a node, not a property.
> 
> 
> Ideally every conf<numerical value>  a combination of pixel clock and new
> values for data and clock level.

This answers neither of my concerns.

> 
>     > +             "clock-frequency" specifies the pixel clock
> 
>     Is this a frequency to configure the pixel clock with, or the
>     pre-determined
>     frequency of a clock that we will select?
> 
> 
> No, as the explanation suggests its the pixel clock itself. 

That doesn't answer my question. Is this the frequency that the pixel clock is
fixed at in hardware, or is this a value to configure the pixel clock with?

> 
>     > +             "con-de-emphasis-level" specifies the configuration
>     > +             of Data De-emphasis levels.
> 
>     Please explain _why_ we need this configuration.
> 
> 
> Our chipset to undergo HDMI compliance test and noticed that the HDMI
> Compliance Test id 7-10 was failing
> for eye diagram test. Hence on further analysis, it was found that altering the
> data de-emphasis levels and clock 
> 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.

Ok, this sounds reasonable for the DT. Could you please add a description to
the binding documenting precisely what this property does and why we need it.

> 
>     Also, "con" is not a good abbreviation of "configuration", "config" would
>     be
>     preferable.
> 
> 
> Agreed, will update the same in next patch set. 

Thanks.

> 
>     > +                     0x145D0040h[3:0] permitted values:
>     > +                             0000 means 760 mVdiff && 1111 means 1400
>     mVdiff
> 
>     I assume the 'h' suffix is a redundant description that the constant is
>     hexadecimal. Please drop it.
> 
> 
> Agreed, will update the same in next patch set.  

Thanks.

> 
>     What is 0x145D0040? The address of the register, or its value?
> 
> 
> Its the address of the hdmiphy register for data level configuration.

Is the register named in any way? The address is completely opaque, especially
as we have no documentation linked in the binding document.

>  
> 
>     The description is confusing, 0x145D0040h[3:0] is always 0[3:0].
> 
> 
> This description is extracted from the one specified in manual, in my first
> patch set the reviewers had asked me 
> to provide the explaination for every bit, which i have provided. 

My comment was invalid given your description that this is an offset, this
would be far clearer with a name for the register.

> 
>     Why does this need to be the exact value programmed into the register
>     rather
>     than separate values the driver can compose?
> 
> 
> As mentioned above the value is must for clearing the test 7-10, and also its
> derived by a trial and error method
> with the HDMI analyser. 

While we need the values, the _representation_ in DT does not have to match the
registers precisely. If you can provide a strong argument for having the opaque
values, then I will consider it ok.

> 
>     > +                     0x145D0040h[7:4] permitted values:
>     > +                             000     0dB
>     > +                             0001    -0.25dB
>     > +                             0010    -0.7dB
>     > +                             0011    -1.15dB
>     > +                             1111    -7.45dB
> 
>     Again, this seems very odd. Why this format?
> 
> 
> 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.

My argument here was that the DT representation need not be identical to the
raw values programmed into the hardware.

> 
>     > +             "con-clock-level" specifies the configuration for
>     > +             the corresponding clock level.
> 
>     To repeat my comment on "con-de-emphasis-level", "con" is not a good
>     abbreviation for "configuration".
> 
> 
> Agreed, will update the same in next patch set.   

Thanks.

> 
>     > +                     0x145D005Ch [1:0] permitted values:
>     > +                             00 means 0 mVdiff && 11 means 60 mVdiff
>     > +                     0x145D005Ch [7:3] permitted values:
>     > +                             00000 is 790 mVdiff
>     > +                             11111 is 1430 mVdiff
> 
>     Please explain why we must use this exact format rather than one which may
>     be
>     understood more easily.
> 
> 
> I hope by now have made this point clear! 

So far I am unsure. However a reposting with the updates you've promised may
help.

One thing I worry about are the other bits in these registers. Are hey used?
Are we doing non-destructive modifications to them?

> 
>     >  Example:
>     >
>     >       hdmi {
>     > @@ -20,4 +41,12 @@ Example:
>     >               reg = <0x14530000 0x100000>;
>     >               interrupts = <0 95 0>;
>     >               hpd-gpio = <&gpx3 7 1>;
>     > +             hdmiphy-confs {
>     > +                     nr-confs = <1>;
>     > +                     conf0: conf0 {
>     > +                             clock-frequency = <25200000>;
>     > +                             conf-de-emphasis-level =  /bits/ 8 <0x26>;
>     > +                             conf-clock-level =  /bits/ 8 < 0x66>;
> 
>     Why are these 8-bit? That wasn't described in the binding at all so far.
> 
> 
> These are 8 bit by design(as mentioned in the manual) and were not part of
> device tree to be explained before. 

The representation in the device tree is a required element of the binding. The
fact that a register has a certain width does not imply that its DT
representation must be, and this _will_ confuse people. Case in point is my
confusion here.

> 
>     > +                     };
>     > +             }
>     >       };
>     > diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts b/arch/arm/boot/dts
>     /exynos5250-arndale.dts
>     > index c23f16b..436b75a 100644
>     > --- a/arch/arm/boot/dts/exynos5250-arndale.dts
>     > +++ b/arch/arm/boot/dts/exynos5250-arndale.dts
>     > @@ -424,6 +424,9 @@
>     >
>     >       hdmi {
>     >               hpd-gpio = <&gpx3 7 2>;
>     > +             vdd_osc-supply = <&ldo10_reg>;
>     > +             vdd_pll-supply = <&ldo8_reg>;
>     > +             vdd-supply = <&ldo8_reg>;
>     >               hdmiphy-confs {
>     >                       nr-confs = <13>;
>     >                       conf0: conf0 {
>     > @@ -492,9 +495,6 @@
>     >                               conf-clock-level =  /bits/ 8 < 0x66>;
>     >                       };
>     >               };
>     > -             vdd_osc-supply = <&ldo10_reg>;
>     > -             vdd_pll-supply = <&ldo8_reg>;
>     > -             vdd-supply = <&ldo8_reg>;
>     >       };
>     >
>     >       mmc_reg: voltage-regulator {
> 
>     Why is this moving? It in no way relates to the rest of the patch, and
>     wasn't
>     described in the commit message.
> 
> 
> Oops that was supposed to be part of previous pach,will update it. 

Ok.

> 
>     [...]
> 
>     > +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 = 0;
>     > +
>     > +     phy_conf = of_find_node_by_name(dev_np, "hdmiphy-confs");
>     > +     if (phy_conf == NULL) {
>     > +             DRM_ERROR("Did not find hdmiphy_conf node\n");
>     > +             return -ENODEV;
>     > +     }
>     > +
>     > +     of_property_read_u32(phy_conf, "nr-confs", &hdata->nr_confs);
> 
>     This can fail, returning an error code. Either check it, or remove this
>     property entirely and do this based on the number of children which have
>     the
>     appropriate properties.
> 
> 
> Ok will add a check. 

Cheers.

> 
>     > +     hdata->confs = kzalloc((hdata->nr_confs * sizeof
>     > +                                     (struct hdmiphy_config)),
>     GFP_KERNEL);
> 
>     Why the brackets on the first argument of kzalloc? They're superfluous.
> 
>     What if kzalloc fails?
> 
> 
> Ok, will put a check. 

Thanks.

> 
>     > +
>     > +     /* Initialize with default config */
>     > +     hdata->confs = hdmiphy_v14_configs;
>     > +
>     > +     for_each_child_of_node(phy_conf, cfg_np) {
> 
>     What if nr-confs is smaller than the number of child nodes?
> 
> 
> it basically is the array size and it should be 1 + child nodes as mentioned in
> the descriptions, i
> assume that the programmer takes care of it.

Experience has shown people _will_ get this wrong, and it's entirely possible
to make do without nr-confs. I would very much like to see nr-confs disappear.

> 
>     > +             if (!of_find_property(cfg_np, "clock-frequency", NULL))
>     > +                     continue;
>     > +
>     > +             if (of_property_read_u32_array(cfg_np, "clock-frequency",
>     > +                                     (u32 *)&hdata->confs[i].
>     > +                                                     pixel_clock, 1)) {
> 
>     Don't split &hdata->confs[i].pixel_clock over two lines.
> 
>     Why is the cast necessary?

Please explain the cast.

> 
>     Why not just of_property_read_u32? This only ever has a single value, and
>     while
>     there's no of_property_read_u8, of_property_read_u32 exists.
> 
> 
> Ok, agreed. 

Thanks.

> 
>     > +                             DRM_ERROR("Failed to get pixel clock\n");
>     > +                             return -EINVAL;
>     > +             }
>     > +
>     > +             /* Overwrite the data de-emphasis and data level */
>     > +             if (of_property_read_u8_array(cfg_np,
>     "conf-de-emphasis-level",
>     > +                                     (u8 *)&hdata->confs[i].conf[16],
>     1)) {
> 
>     Why is this cast necessary?

Please explain the cast.

> 
>     I don't see why this must be an 8 bit property.
> 
> 
> As mentioned earlier its by design 8 bits. 

The hardware width doesn't necessitate the representation. This should be
documented if required, or just changed to be a u32 cell so as to confuse fewer
people.

> 
>     > +                             DRM_ERROR("Failed to get conf\n");
>     > +                             return -EINVAL;
>     > +             }
>     > +             /* Overwrite the clock level diff */
>     > +             if (of_property_read_u8_array(cfg_np, "conf-clock-level",
>     > +                                     (u8 *)&hdata->confs[i].conf[23],
>     1)) {
> 
>     Why the cast?
> 
> 
> Same explaination as above. 

There was no explanation above. Please explain the cast.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Shirish S]

On Tue, Oct 29, 2013 at 4:08 AM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> On Mon, Oct 28, 2013 at 10:15:00AM +0000, Shirish S wrote:
>> Hi Mark,
>> Firstly thanks for reviewing.
>
> Hi,
>
> Please could you refrain from replying in HTML and use plaintext, it's rather
> difficult to respond sensibly.
>
Sorry for your troubles,hope with this reply its fine.
>>
>>
>> On Mon, Oct 28, 2013 at 12:20 PM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
>>
>>     Hi,
>>
>>     On Mon, Oct 28, 2013 at 06:24:22AM +0000, 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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>>     > ---
>>     >  .../devicetree/bindings/video/exynos_hdmi.txt      |   29 ++++++++
>>     >  arch/arm/boot/dts/exynos5250-arndale.dts           |    6 +-
>>     >  drivers/gpu/drm/exynos/exynos_hdmi.c               |   70
>>     ++++++++++++++++++--
>>     >  3 files changed, 98 insertions(+), 7 deletions(-)
>>     >
>>     > diff --git a/Documentation/devicetree/bindings/video/exynos_hdmi.txt b/
>>     Documentation/devicetree/bindings/video/exynos_hdmi.txt
>>     > index 323983b..770f92d 100644
>>     > --- a/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>>     > +++ b/Documentation/devicetree/bindings/video/exynos_hdmi.txt
>>     > @@ -13,6 +13,27 @@ Required properties:
>>     >       b) pin number within the gpio controller.
>>     >       c) optional flags and pull up/down.
>>     >
>>     > +- hdmiphy-confs: following information about the hdmiphy conf settings.
>>
>>     Judging by the other patches, this is a node, not a property.
>>
>>
>> Yes its a node.
>
> My point was that the documentation should reflect this.
>
Its property and not a node, i had misunderstood the concepts.
>>
>>     > +        a) "nr-confs" specifies the number of pixel clocks supported.
>>
>>     Why is this needed? Someone will get it wrong eventually and it can be
>>     figured
>>     out currently by counting the child nodes, testing if they have the
>>     appropriate
>>     properties.
>>
>>
>> Actually i need to get the array size also from dt, hence this is approach i
>> have taken
>
> While this approach works now for perfect DTs, as I pointed out it is encoding
> redundant information and could be broken by future changes. Please fix this.
>
Hope you have had a look at my next patch set (v4), this property is
now more or less optional,
i mean if the user enters wrong number of configs it shall not be
considered and driver shall use the predefined
table provided in the driver.
>>
>>     > +     b) "confX: confX" specifies the phy configuration settings,
>>
>>     This is confusing. What is X?
>>
>>
>> I am trying to generalize, here X means any numerical, and the programmer needs
>> to
>> make sure conf0:conf0, wherein X is 0.I shall provide the values permitted for
>> X in my next patch set.
>
> Please be explicit with your definitions. Experience shows that people will get
> this wrong.
>
Is the way i have documented in patch set v4 still wrong, or its clear?
>>
>>     The label is irrelevant -- none of this patch looks for phandles pointing
>>     at
>>     configurations, nor is the precise name of the label important.
>>
>>     This is a node, not a property.
>>
>>
>> Ideally every conf<numerical value>  a combination of pixel clock and new
>> values for data and clock level.
>
> This answers neither of my concerns.
>
The fact that its a property and not a node,and contains pairs of
pixel clock and config values should answer your concerns.
Please note: in this case(exynos5250) the config values seem to same
for all the pixel clocks, however in the next chipset (exynos5420) the
values vary across pixel clocks, that is the reason why i need to keep
these config values attached to pixel clocks.
>>
>>     > +             "clock-frequency" specifies the pixel clock
>>
>>     Is this a frequency to configure the pixel clock with, or the
>>     pre-determined
>>     frequency of a clock that we will select?
>>
>>
>> No, as the explanation suggests its the pixel clock itself.
>
> That doesn't answer my question. Is this the frequency that the pixel clock is
> fixed at in hardware, or is this a value to configure the pixel clock with?
>
This the frequency that the pixel clock is fixed at in hardware also
note that i have changed the name from clock-frequency to pixel-clock.
>>
>>     > +             "con-de-emphasis-level" specifies the configuration
>>     > +             of Data De-emphasis levels.
>>
>>     Please explain _why_ we need this configuration.
>>
>>
>> Our chipset to undergo HDMI compliance test and noticed that the HDMI
>> Compliance Test id 7-10 was failing
>> for eye diagram test. Hence on further analysis, it was found that altering the
>> data de-emphasis levels and clock
>> 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.
>
> Ok, this sounds reasonable for the DT. Could you please add a description to
> the binding documenting precisely what this property does and why we need it.
>
Ok, i have added it in patch set v4.
>>
>>     Also, "con" is not a good abbreviation of "configuration", "config" would
>>     be
>>     preferable.
>>
>>
>> Agreed, will update the same in next patch set.
>
> Thanks.
>
>>
>>     > +                     0x145D0040h[3:0] permitted values:
>>     > +                             0000 means 760 mVdiff && 1111 means 1400
>>     mVdiff
>>
>>     I assume the 'h' suffix is a redundant description that the constant is
>>     hexadecimal. Please drop it.
>>
>>
>> Agreed, will update the same in next patch set.
>
> Thanks.
>
>>
>>     What is 0x145D0040? The address of the register, or its value?
>>
>>
>> Its the address of the hdmiphy register for data level configuration.
>
> Is the register named in any way? The address is completely opaque, especially
> as we have no documentation linked in the binding document.
>
To configure hdmi phy we use dedicated i2c, the address of hdmiphy is 0x70.
And in this patch i am trying to address the offset of 16 and 23,
there is no name as such.
However i shall give a name, but note its not in the manual.
>>
>>
>>     The description is confusing, 0x145D0040h[3:0] is always 0[3:0].
>>
>>
>> This description is extracted from the one specified in manual, in my first
>> patch set the reviewers had asked me
>> to provide the explaination for every bit, which i have provided.
>
> My comment was invalid given your description that this is an offset, this
> would be far clearer with a name for the register.
>
>>
>>     Why does this need to be the exact value programmed into the register
>>     rather
>>     than separate values the driver can compose?
>>
>>
>> As mentioned above the value is must for clearing the test 7-10, and also its
>> derived by a trial and error method
>> with the HDMI analyser.
>
> While we need the values, the _representation_ in DT does not have to match the
> registers precisely. If you can provide a strong argument for having the opaque
> values, then I will consider it ok.
>
As you can see the documentation describes every bit of these
registers, we have arrived at the mentioned values by trial and error
method. I have described what these opaque values means in the dt
files.Kindly have a look in patch set v4.
>>
>>     > +                     0x145D0040h[7:4] permitted values:
>>     > +                             000     0dB
>>     > +                             0001    -0.25dB
>>     > +                             0010    -0.7dB
>>     > +                             0011    -1.15dB
>>     > +                             1111    -7.45dB
>>
>>     Again, this seems very odd. Why this format?
>>
>>
>> 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.
>
> My argument here was that the DT representation need not be identical to the
> raw values programmed into the hardware.
>
I am not clear, that is the whole reason why we are planning to move to dt.
Instead of duplicating the hdmiphy config structures(which again are
raw values) in the driver, we want to associate these raw values in
the revision specific dt files.
>>
>>     > +             "con-clock-level" specifies the configuration for
>>     > +             the corresponding clock level.
>>
>>     To repeat my comment on "con-de-emphasis-level", "con" is not a good
>>     abbreviation for "configuration".
>>
>>
>> Agreed, will update the same in next patch set.
>
> Thanks.
>
>>
>>     > +                     0x145D005Ch [1:0] permitted values:
>>     > +                             00 means 0 mVdiff && 11 means 60 mVdiff
>>     > +                     0x145D005Ch [7:3] permitted values:
>>     > +                             00000 is 790 mVdiff
>>     > +                             11111 is 1430 mVdiff
>>
>>     Please explain why we must use this exact format rather than one which may
>>     be
>>     understood more easily.
>>
>>
>> I hope by now have made this point clear!
>
> So far I am unsure. However a reposting with the updates you've promised may
> help.
>
> One thing I worry about are the other bits in these registers. Are hey used?
> Are we doing non-destructive modifications to them?
>
The other remaining bits are non writable, and hence we are not doing
any destructive modifications.
>>
>>     >  Example:
>>     >
>>     >       hdmi {
>>     > @@ -20,4 +41,12 @@ Example:
>>     >               reg = <0x14530000 0x100000>;
>>     >               interrupts = <0 95 0>;
>>     >               hpd-gpio = <&gpx3 7 1>;
>>     > +             hdmiphy-confs {
>>     > +                     nr-confs = <1>;
>>     > +                     conf0: conf0 {
>>     > +                             clock-frequency = <25200000>;
>>     > +                             conf-de-emphasis-level =  /bits/ 8 <0x26>;
>>     > +                             conf-clock-level =  /bits/ 8 < 0x66>;
>>
>>     Why are these 8-bit? That wasn't described in the binding at all so far.
>>
>>
>> These are 8 bit by design(as mentioned in the manual) and were not part of
>> device tree to be explained before.
>
> The representation in the device tree is a required element of the binding. The
> fact that a register has a certain width does not imply that its DT
> representation must be, and this _will_ confuse people. Case in point is my
> confusion here.
>
As these values are directly copied into the 8 bit arrays of configs,
i think its better if we go ahead with
the cast, also as dt facilitates us with the /bits/8, to avoid
multiple levels of typecasting in the driver.
>>
>>     > +                     };
>>     > +             }
>>     >       };
>>     > diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts b/arch/arm/boot/dts
>>     /exynos5250-arndale.dts
>>     > index c23f16b..436b75a 100644
>>     > --- a/arch/arm/boot/dts/exynos5250-arndale.dts
>>     > +++ b/arch/arm/boot/dts/exynos5250-arndale.dts
>>     > @@ -424,6 +424,9 @@
>>     >
>>     >       hdmi {
>>     >               hpd-gpio = <&gpx3 7 2>;
>>     > +             vdd_osc-supply = <&ldo10_reg>;
>>     > +             vdd_pll-supply = <&ldo8_reg>;
>>     > +             vdd-supply = <&ldo8_reg>;
>>     >               hdmiphy-confs {
>>     >                       nr-confs = <13>;
>>     >                       conf0: conf0 {
>>     > @@ -492,9 +495,6 @@
>>     >                               conf-clock-level =  /bits/ 8 < 0x66>;
>>     >                       };
>>     >               };
>>     > -             vdd_osc-supply = <&ldo10_reg>;
>>     > -             vdd_pll-supply = <&ldo8_reg>;
>>     > -             vdd-supply = <&ldo8_reg>;
>>     >       };
>>     >
>>     >       mmc_reg: voltage-regulator {
>>
>>     Why is this moving? It in no way relates to the rest of the patch, and
>>     wasn't
>>     described in the commit message.
>>
>>
>> Oops that was supposed to be part of previous pach,will update it.
>
> Ok.
>
>>
>>     [...]
>>
>>     > +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 = 0;
>>     > +
>>     > +     phy_conf = of_find_node_by_name(dev_np, "hdmiphy-confs");
>>     > +     if (phy_conf == NULL) {
>>     > +             DRM_ERROR("Did not find hdmiphy_conf node\n");
>>     > +             return -ENODEV;
>>     > +     }
>>     > +
>>     > +     of_property_read_u32(phy_conf, "nr-confs", &hdata->nr_confs);
>>
>>     This can fail, returning an error code. Either check it, or remove this
>>     property entirely and do this based on the number of children which have
>>     the
>>     appropriate properties.
>>
>>
>> Ok will add a check.
>
> Cheers.
>
>>
>>     > +     hdata->confs = kzalloc((hdata->nr_confs * sizeof
>>     > +                                     (struct hdmiphy_config)),
>>     GFP_KERNEL);
>>
>>     Why the brackets on the first argument of kzalloc? They're superfluous.
>>
>>     What if kzalloc fails?
>>
>>
>> Ok, will put a check.
>
> Thanks.
>
>>
>>     > +
>>     > +     /* Initialize with default config */
>>     > +     hdata->confs = hdmiphy_v14_configs;
>>     > +
>>     > +     for_each_child_of_node(phy_conf, cfg_np) {
>>
>>     What if nr-confs is smaller than the number of child nodes?
>>
>>
>> it basically is the array size and it should be 1 + child nodes as mentioned in
>> the descriptions, i
>> assume that the programmer takes care of it.
>
> Experience has shown people _will_ get this wrong, and it's entirely possible
> to make do without nr-confs. I would very much like to see nr-confs disappear.
>
>>
>>     > +             if (!of_find_property(cfg_np, "clock-frequency", NULL))
>>     > +                     continue;
>>     > +
>>     > +             if (of_property_read_u32_array(cfg_np, "clock-frequency",
>>     > +                                     (u32 *)&hdata->confs[i].
>>     > +                                                     pixel_clock, 1)) {
>>
>>     Don't split &hdata->confs[i].pixel_clock over two lines.
>>
>>     Why is the cast necessary?
>
> Please explain the cast.
>
have removed it.
>>
>>     Why not just of_property_read_u32? This only ever has a single value, and
>>     while
>>     there's no of_property_read_u8, of_property_read_u32 exists.
>>
>>
>> Ok, agreed.
>
> Thanks.
>
>>
>>     > +                             DRM_ERROR("Failed to get pixel clock\n");
>>     > +                             return -EINVAL;
>>     > +             }
>>     > +
>>     > +             /* Overwrite the data de-emphasis and data level */
>>     > +             if (of_property_read_u8_array(cfg_np,
>>     "conf-de-emphasis-level",
>>     > +                                     (u8 *)&hdata->confs[i].conf[16],
>>     1)) {
>>
>>     Why is this cast necessary?
>
> Please explain the cast.
>
have removed the cast.
>>
>>     I don't see why this must be an 8 bit property.
>>
>>
>> As mentioned earlier its by design 8 bits.
>
> The hardware width doesn't necessitate the representation. This should be
> documented if required, or just changed to be a u32 cell so as to confuse fewer
> people.
>
>>
>>     > +                             DRM_ERROR("Failed to get conf\n");
>>     > +                             return -EINVAL;
>>     > +             }
>>     > +             /* Overwrite the clock level diff */
>>     > +             if (of_property_read_u8_array(cfg_np, "conf-clock-level",
>>     > +                                     (u8 *)&hdata->confs[i].conf[23],
>>     1)) {
>>
>>     Why the cast?
>>
>>
>> Same explaination as above.
>
> There was no explanation above. Please explain the cast.
>
> Thanks,
> Mark.
Thanks & Regards,
Shirish S
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html