[PATCH 0/3] ARM:exynos5:power-domain: Save and restore CLK_TOP_SRC3 via clock framework.

[PATCH 0/3] ARM:exynos5:power-domain: Save and restore CLK_TOP_SRC3 via clock framework.

[Prasanna Kumar]

After Suspend-Resume operation of exynos5, CLK_TOP_SRC3 register modified
while power gating G-scaler and MFC power domains.This is seen only after
suspend and resume.

The solution to this problem is to save CLK_SRC_TOP3 register and restore
it while powergating. But CLK_SRC_TOP3 register cannot accessed directly 
by power domain code.
Please refer below URL to know the background of this issue.
http://www.mail-archive.com/linux-samsung-soc at vger.kernel.org/msg14347.html.

This patch set adds clock framework support for save and restore
clock register (CLK_SRC_TOP3)  for G-scaler and MFC power domains.

This patch set depends on
http://www.mail-archive.com/linux-samsung-soc at vger.kernel.org/msg14648.html

Prasanna Kumar (3):
  ARM: dts: exynos5: Add power domain clocks to pd node of Gscaler and MFC
  ARM:exynos5:dts: Bindings for clock definitions are added.
  ARM: exynos5: Add clock save and restore operation(CLK_SRC_TOP3) using clock framework.

 .../bindings/arm/exynos/power_domain.txt           |   14 ++
 arch/arm/boot/dts/exynos5250.dtsi                  |    2 +
 arch/arm/mach-exynos/pm_domains.c                  |  125 ++++++++++++++++++++
 3 files changed, 141 insertions(+), 0 deletions(-)

-- 
1.7.5.4

[PATCH 1/3] ARM: dts: exynos5: Add power domain clocks to pd node of Gscaler and MFC

[Prasanna Kumar]

Power domain clks are added to Gscaler and MFC pd nodes.
These clocks are saved during power off.

Signed-off-by: Prasanna Kumar <prasanna.ps@samsung.com>
---
 arch/arm/boot/dts/exynos5250.dtsi |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
index 6d0e87c..8043be3 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -558,11 +558,13 @@
 	pd_gsc: gsc-power-domain at 0x10044000 {
 		compatible = "samsung,exynos4210-pd";
 		reg = <0x10044000 0x20>;
+		samsung,exynos-pd-clks ="aclk_266_gscl","aclk_300_gscl";
 	};
 
 	pd_mfc: mfc-power-domain at 0x10044040 {
 		compatible = "samsung,exynos4210-pd";
 		reg = <0x10044040 0x20>;
+		samsung,exynos-pd-clks ="aclk_333";
 	};
 
 	gsc_0:  gsc at 0x13e00000 {
-- 
1.7.5.4

[PATCH 1/3] ARM: dts: exynos5: Add power domain clocks to pd node of Gscaler and MFC

[Thomas Abraham]

On 9 January 2013 04:54, Prasanna Kumar <prasanna.ps@samsung.com> wrote:
> Power domain clks are added to Gscaler and MFC pd nodes.
> These clocks are saved during power off.
>
> Signed-off-by: Prasanna Kumar <prasanna.ps@samsung.com>
> ---
>  arch/arm/boot/dts/exynos5250.dtsi |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/boot/dts/exynos5250.dtsi b/arch/arm/boot/dts/exynos5250.dtsi
> index 6d0e87c..8043be3 100644
> --- a/arch/arm/boot/dts/exynos5250.dtsi
> +++ b/arch/arm/boot/dts/exynos5250.dtsi
> @@ -558,11 +558,13 @@
>         pd_gsc: gsc-power-domain at 0x10044000 {
>                 compatible = "samsung,exynos4210-pd";
>                 reg = <0x10044000 0x20>;
> +               samsung,exynos-pd-clks ="aclk_266_gscl","aclk_300_gscl";

Passing clock names from dts is generally discouraged. These clock
names are linux specific. dts/dtsi files should only describe the
hardware and not have any linux specific entries in them. Common clock
framework can be used here to specify the clock.

>         };
>
>         pd_mfc: mfc-power-domain at 0x10044040 {
>                 compatible = "samsung,exynos4210-pd";
>                 reg = <0x10044040 0x20>;
> +               samsung,exynos-pd-clks ="aclk_333";

same as above comment.

Thanks,
Thomas.

>         };
>
>         gsc_0:  gsc at 0x13e00000 {
> --
> 1.7.5.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] ARM:exynos5:dts: Bindings for clock definitions are added.

[Prasanna Kumar]

The details of "samsung,exynos-pd-clks" property is added.

Signed-off-by: Prasanna Kumar <prasanna.ps@samsung.com>
---
 .../bindings/arm/exynos/power_domain.txt           |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/exynos/power_domain.txt b/Documentation/devicetree/bindings/arm/exynos/power_domain.txt
index 5216b41..dc1e89bd 100644
--- a/Documentation/devicetree/bindings/arm/exynos/power_domain.txt
+++ b/Documentation/devicetree/bindings/arm/exynos/power_domain.txt
@@ -12,6 +12,12 @@ Required Properties:
 Node of a device using power domains must have a samsung,power-domain property
 defined with a phandle to respective power domain.
 
+Optional Properties:
+samsung,exynos-pd-clks: After suspend-resume of system ,the clock sources of
+	few devices forming part of power domain need to be saved as they get
+	modified during power domain On->Off operation.Such clock defintions
+	are added via "samsung,exynos-pd-clks" property as strings.
+
 Example:
 
 	lcd0: power-domain-lcd0 {
@@ -26,3 +32,11 @@ Example of the node using power domain:
 		samsung,power-domain = <&lcd0>;
 		/* ... */
 	};
+
+Example of the node using power domain clock defintions:
+
+	node {
+		/* ... */
+		samsung,exynos-pd-clks = "aclk_333";
+		/* ... */
+	};
-- 
1.7.5.4

[PATCH 3/3] ARM: exynos5: Add clock save and restore

[Prasanna Kumar]

After Suspend-Resume operation, it is observed that CLK_TOP_SRC3 register
gets modified if the G-Scaler/MFC devices are power gated.

The clock for G-Scaler gets set to XXTI which results in the device
running very slow.This issue also seen for MFC.

To solve above issue, the existing clock framework of exynos5 is used
to save and restore clocks while power gating instead of accessing
CLK_SRC_TOP3 register directly.The clock names are read from DT file.

Please refer below URL to know the background of this issue.
http://www.mail-archive.com/linux-samsung-soc at vger.kernel.org/msg14347.html.

Signed-off-by: Prasanna Kumar <prasanna.ps@samsung.com>
---
 arch/arm/mach-exynos/pm_domains.c |  125 +++++++++++++++++++++++++++++++++++++
 1 files changed, 125 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c
index 9f1351d..2f49de9 100644
--- a/arch/arm/mach-exynos/pm_domains.c
+++ b/arch/arm/mach-exynos/pm_domains.c
@@ -21,9 +21,14 @@
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
 #include <linux/sched.h>
+#include <linux/clk.h>
+#include <linux/list.h>
 
 #include <mach/regs-pmu.h>
 #include <plat/devs.h>
+#include <plat/clock.h>
+#include <plat/clock-clksrc.h>
+
 
 /*
  * Exynos specific wrapper around the generic power domain
@@ -33,7 +38,69 @@ struct exynos_pm_domain {
 	char const *name;
 	bool is_off;
 	struct generic_pm_domain pd;
+	struct list_head list_pdclks;
+	struct list_head saved_list_pdclks;
+	int pd_clks;
+};
+
+struct exynos_pd_clk {
+	struct list_head node;
+	struct clk *clk;
 };
+static int exynos_pdclk_save(struct exynos_pm_domain *epd)
+{
+	struct exynos_pd_clk *pdclk;
+	struct exynos_pd_clk *saved_pdclk;
+
+	list_for_each_entry(pdclk, &epd->list_pdclks, node) {
+		if (pdclk) {
+			saved_pdclk = kzalloc(sizeof(struct exynos_pd_clk),
+					GFP_KERNEL);
+			if (!saved_pdclk) {
+				pr_err("%s: failed to allocate memory\n",
+					__func__);
+				return -ENOMEM;
+			}
+			saved_pdclk->clk = clk_get_parent(pdclk->clk);
+			if (IS_ERR(saved_pdclk->clk)) {
+				pr_err(" Cannot get parent for %s\n",
+							pdclk->clk->name);
+				return PTR_ERR(saved_pdclk->clk);
+			}
+			list_add_tail(&saved_pdclk->node,
+						&epd->saved_list_pdclks);
+		}
+	}
+	return 0;
+}
+
+static void exynos_pdclk_restore(struct exynos_pm_domain *epd)
+{
+	struct exynos_pd_clk *pdclk;
+	struct exynos_pd_clk *saved_pdclk;
+	struct list_head *p_clk;
+	struct list_head *p_saved_clk;
+	int ret;
+
+	p_saved_clk = epd->saved_list_pdclks.next;
+	p_clk = epd->list_pdclks.next;
+
+	for ( ; p_saved_clk != &epd->saved_list_pdclks &&
+		p_clk != &epd->list_pdclks;
+		p_clk = p_clk->next, p_saved_clk = p_saved_clk->next) {
+
+		saved_pdclk = list_entry(p_saved_clk,
+						struct exynos_pd_clk, node);
+		pdclk = list_entry(p_clk, struct exynos_pd_clk, node);
+		if (saved_pdclk && pdclk) {
+			ret = clk_set_parent(pdclk->clk, saved_pdclk->clk);
+			if (ret)
+				pr_err("Failed to set %s as parent of %s\n",
+				 saved_pdclk->clk->name, pdclk->clk->name);
+		}
+	}
+	return;
+}
 
 static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on)
 {
@@ -45,6 +112,13 @@ static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on)
 	pd = container_of(domain, struct exynos_pm_domain, pd);
 	base = pd->base;
 
+	if (!power_on) {
+		if (pd->pd_clks > 0)
+			if (exynos_pdclk_save(pd))
+				pr_err("Failed to save pdclks for %s\n",
+								domain->name);
+	}
+
 	pwr = power_on ? S5P_INT_LOCAL_PWR_EN : 0;
 	__raw_writel(pwr, base);
 
@@ -61,6 +135,11 @@ static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on)
 		cpu_relax();
 		usleep_range(80, 100);
 	}
+
+	if (!power_on) {
+		if (pd->pd_clks > 0)
+			exynos_pdclk_restore(pd);
+	}
 	return 0;
 }
 
@@ -157,10 +236,48 @@ static struct notifier_block platform_nb = {
 	.notifier_call = exynos_pm_notifier_call,
 };
 
+static int exynos_read_pdclk_from_dt(struct device_node *dev_node,
+						struct exynos_pm_domain *pd)
+{
+	int i = 0;
+	const char *clk_name;
+	struct exynos_pd_clk *pd_clk;
+	int err = 0;
+
+	INIT_LIST_HEAD(&pd->list_pdclks);
+	INIT_LIST_HEAD(&pd->saved_list_pdclks);
+
+	for (i = 0; i < pd->pd_clks; i++) {
+		pd_clk = kzalloc(sizeof(struct exynos_pd_clk),
+							GFP_KERNEL);
+		if (!pd_clk) {
+			pr_err("%s: failed to allocate memory\n",
+					__func__);
+			return -ENOMEM;
+		}
+		err = of_property_read_string_index(dev_node,
+				"samsung,exynos-pd-clks",
+				i,
+				&clk_name);
+		if (err) {
+			pr_err("failed to read pd_clks\n");
+			return err;
+		}
+		pd_clk->clk = clk_get(NULL, clk_name);
+		if (IS_ERR(pd_clk->clk)) {
+			pr_err("clk_get failed for %s\n", clk_name);
+			return PTR_ERR(pd_clk->clk);
+		}
+		list_add(&pd_clk->node, &pd->list_pdclks);
+	}
+	return 0;
+}
+
 static __init int exynos_pm_dt_parse_domains(void)
 {
 	struct platform_device *pdev;
 	struct device_node *np;
+	int err = 0;
 
 	for_each_compatible_node(np, NULL, "samsung,exynos4210-pd") {
 		struct exynos_pm_domain *pd;
@@ -182,6 +299,14 @@ static __init int exynos_pm_dt_parse_domains(void)
 		pd->pd.power_on = exynos_pd_power_on;
 		pd->pd.of_node = np;
 
+		pd->pd_clks = of_property_count_strings(np,
+						"samsung,exynos-pd-clks");
+		if (pd->pd_clks > 0) {
+			err = exynos_read_pdclk_from_dt(np, pd);
+			if (err)
+				return err;
+		}
+
 		platform_set_drvdata(pdev, pd);
 
 		on = __raw_readl(pd->base + 0x4) & S5P_INT_LOCAL_PWR_EN;
-- 
1.7.5.4

[PATCH 3/3] ARM: exynos5: Add clock save and restore

[Russell King - ARM Linux]

On Wed, Jan 09, 2013 at 06:24:41PM +0530, Prasanna Kumar wrote:
> After Suspend-Resume operation, it is observed that CLK_TOP_SRC3 register
> gets modified if the G-Scaler/MFC devices are power gated.
> 
> The clock for G-Scaler gets set to XXTI which results in the device
> running very slow.This issue also seen for MFC.
> 
> To solve above issue, the existing clock framework of exynos5 is used
> to save and restore clocks while power gating instead of accessing
> CLK_SRC_TOP3 register directly.The clock names are read from DT file.
> 
> Please refer below URL to know the background of this issue.
> http://www.mail-archive.com/linux-samsung-soc at vger.kernel.org/msg14347.html.
> 
> Signed-off-by: Prasanna Kumar <prasanna.ps@samsung.com>
> ---
>  arch/arm/mach-exynos/pm_domains.c |  125 +++++++++++++++++++++++++++++++++++++
>  1 files changed, 125 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/pm_domains.c b/arch/arm/mach-exynos/pm_domains.c
> index 9f1351d..2f49de9 100644
> --- a/arch/arm/mach-exynos/pm_domains.c
> +++ b/arch/arm/mach-exynos/pm_domains.c
> @@ -21,9 +21,14 @@
>  #include <linux/of_address.h>
>  #include <linux/of_platform.h>
>  #include <linux/sched.h>
> +#include <linux/clk.h>
> +#include <linux/list.h>
>  
>  #include <mach/regs-pmu.h>
>  #include <plat/devs.h>
> +#include <plat/clock.h>
> +#include <plat/clock-clksrc.h>
> +
>  
>  /*
>   * Exynos specific wrapper around the generic power domain
> @@ -33,7 +38,69 @@ struct exynos_pm_domain {
>  	char const *name;
>  	bool is_off;
>  	struct generic_pm_domain pd;
> +	struct list_head list_pdclks;
> +	struct list_head saved_list_pdclks;
> +	int pd_clks;
> +};
> +
> +struct exynos_pd_clk {
> +	struct list_head node;
> +	struct clk *clk;
>  };

Missing blank line, but that's the least of the problems...

> +static int exynos_pdclk_save(struct exynos_pm_domain *epd)
> +{
> +	struct exynos_pd_clk *pdclk;
> +	struct exynos_pd_clk *saved_pdclk;
> +
> +	list_for_each_entry(pdclk, &epd->list_pdclks, node) {
> +		if (pdclk) {
> +			saved_pdclk = kzalloc(sizeof(struct exynos_pd_clk),
> +					GFP_KERNEL);
> +			if (!saved_pdclk) {
> +				pr_err("%s: failed to allocate memory\n",
> +					__func__);
> +				return -ENOMEM;
> +			}
> +			saved_pdclk->clk = clk_get_parent(pdclk->clk);
> +			if (IS_ERR(saved_pdclk->clk)) {

Congratulations on using the correct macro to check for failure.

> +				pr_err(" Cannot get parent for %s\n",

You don't need a space character before "Cannot", and it should be "Can not".

> +							pdclk->clk->name);
> +				return PTR_ERR(saved_pdclk->clk);

Memory leak.

> +			}
> +			list_add_tail(&saved_pdclk->node,
> +						&epd->saved_list_pdclks);
> +		}
> +	}
> +	return 0;
> +}
> +
> +static void exynos_pdclk_restore(struct exynos_pm_domain *epd)
> +{
> +	struct exynos_pd_clk *pdclk;
> +	struct exynos_pd_clk *saved_pdclk;
> +	struct list_head *p_clk;
> +	struct list_head *p_saved_clk;
> +	int ret;
> +
> +	p_saved_clk = epd->saved_list_pdclks.next;
> +	p_clk = epd->list_pdclks.next;
> +
> +	for ( ; p_saved_clk != &epd->saved_list_pdclks &&
> +		p_clk != &epd->list_pdclks;
> +		p_clk = p_clk->next, p_saved_clk = p_saved_clk->next) {
> +
> +		saved_pdclk = list_entry(p_saved_clk,
> +						struct exynos_pd_clk, node);
> +		pdclk = list_entry(p_clk, struct exynos_pd_clk, node);
> +		if (saved_pdclk && pdclk) {
> +			ret = clk_set_parent(pdclk->clk, saved_pdclk->clk);
> +			if (ret)
> +				pr_err("Failed to set %s as parent of %s\n",
> +				 saved_pdclk->clk->name, pdclk->clk->name);
> +		}
> +	}

So here, you're walking to lists in unison.  What if exynos_pdclk_save()
failed to build the full list of saved clocks?  I suspect things explode.

Moreover, wouldn't it be more efficient to record the old parent in
the main list - it's only another 8 bytes, and it's not like you're
cleaning up and freeing the saved list, so that will save some memory
(probably number_of_clocks * L1 cache line size).

> +	return;
> +}
>  
>  static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on)
>  {
> @@ -45,6 +112,13 @@ static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on)
>  	pd = container_of(domain, struct exynos_pm_domain, pd);
>  	base = pd->base;
>  
> +	if (!power_on) {
> +		if (pd->pd_clks > 0)
> +			if (exynos_pdclk_save(pd))

This can be simplified:

	if (!power_on && pd->pd_clks > 0 && exynos_pdclk_save(pd))

> +				pr_err("Failed to save pdclks for %s\n",
> +								domain->name);

Hmm, exynos_pdclk_save() prints its own diagnostics on error, is there
really a need for two error printks?  If not, then wouldn't:

	if (!power_on && pd->pd_clks > 0)
		exynos_pdclk_save(pd);

be a more readable way to code this?

> +	}
> +
>  	pwr = power_on ? S5P_INT_LOCAL_PWR_EN : 0;
>  	__raw_writel(pwr, base);
>  
> @@ -61,6 +135,11 @@ static int exynos_pd_power(struct generic_pm_domain *domain, bool power_on)
>  		cpu_relax();
>  		usleep_range(80, 100);
>  	}
> +
> +	if (!power_on) {
> +		if (pd->pd_clks > 0)
> +			exynos_pdclk_restore(pd);
> +	}

Same comments here.

>  	return 0;
>  }
>  
> @@ -157,10 +236,48 @@ static struct notifier_block platform_nb = {
>  	.notifier_call = exynos_pm_notifier_call,
>  };
>  
> +static int exynos_read_pdclk_from_dt(struct device_node *dev_node,
> +						struct exynos_pm_domain *pd)
> +{
> +	int i = 0;
> +	const char *clk_name;
> +	struct exynos_pd_clk *pd_clk;
> +	int err = 0;
> +
> +	INIT_LIST_HEAD(&pd->list_pdclks);
> +	INIT_LIST_HEAD(&pd->saved_list_pdclks);
> +
> +	for (i = 0; i < pd->pd_clks; i++) {
> +		pd_clk = kzalloc(sizeof(struct exynos_pd_clk),
> +							GFP_KERNEL);
> +		if (!pd_clk) {
> +			pr_err("%s: failed to allocate memory\n",
> +					__func__);
> +			return -ENOMEM;
> +		}
> +		err = of_property_read_string_index(dev_node,
> +				"samsung,exynos-pd-clks",
> +				i,
> +				&clk_name);
> +		if (err) {
> +			pr_err("failed to read pd_clks\n");

pd_clk is leaked.

> +			return err;
> +		}
> +		pd_clk->clk = clk_get(NULL, clk_name);
> +		if (IS_ERR(pd_clk->clk)) {
> +			pr_err("clk_get failed for %s\n", clk_name);

od_clk is leaked.  Don't we have an interface to get a clk from DT?

> +			return PTR_ERR(pd_clk->clk);
> +		}
> +		list_add(&pd_clk->node, &pd->list_pdclks);
> +	}
> +	return 0;
> +}
> +
>  static __init int exynos_pm_dt_parse_domains(void)
>  {
>  	struct platform_device *pdev;
>  	struct device_node *np;
> +	int err = 0;
>  
>  	for_each_compatible_node(np, NULL, "samsung,exynos4210-pd") {
>  		struct exynos_pm_domain *pd;
> @@ -182,6 +299,14 @@ static __init int exynos_pm_dt_parse_domains(void)
>  		pd->pd.power_on = exynos_pd_power_on;
>  		pd->pd.of_node = np;
>  
> +		pd->pd_clks = of_property_count_strings(np,
> +						"samsung,exynos-pd-clks");
> +		if (pd->pd_clks > 0) {
> +			err = exynos_read_pdclk_from_dt(np, pd);
> +			if (err)
> +				return err;
> +		}
> +
>  		platform_set_drvdata(pdev, pd);
>  
>  		on = __raw_readl(pd->base + 0x4) & S5P_INT_LOCAL_PWR_EN;
> -- 
> 1.7.5.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] ARM: exynos5: Add clock save and restore

[Prasanna Kumar]

Hi Russell King ,

Thanks for your review comments.
I will address them soon.

-- 
Thanks
Prasanna Kumar

[PATCH 0/3] ARM:exynos5:power-domain: Save and restore CLK_TOP_SRC3 via clock framework.

[Kukjin Kim]

Prasanna Kumar wrote:
> 
> After Suspend-Resume operation of exynos5, CLK_TOP_SRC3 register
> modified
> while power gating G-scaler and MFC power domains.This is seen only after
> suspend and resume.
> 
> The solution to this problem is to save CLK_SRC_TOP3 register and restore
> it while powergating. But CLK_SRC_TOP3 register cannot accessed directly
> by power domain code.
> Please refer below URL to know the background of this issue.
> http://www.mail-archive.com/linux-samsung-
> soc at vger.kernel.org/msg14347.html.
> 
> This patch set adds clock framework support for save and restore
> clock register (CLK_SRC_TOP3)  for G-scaler and MFC power domains.
> 
> This patch set depends on
> http://www.mail-archive.com/linux-samsung-
> soc at vger.kernel.org/msg14648.html
> 
> Prasanna Kumar (3):
>   ARM: dts: exynos5: Add power domain clocks to pd node of Gscaler and
> MFC
>   ARM:exynos5:dts: Bindings for clock definitions are added.
>   ARM: exynos5: Add clock save and restore operation(CLK_SRC_TOP3) using
> clock framework.
> 
>  .../bindings/arm/exynos/power_domain.txt           |   14 ++
>  arch/arm/boot/dts/exynos5250.dtsi                  |    2 +
>  arch/arm/mach-exynos/pm_domains.c                  |  125
> ++++++++++++++++++++
>  3 files changed, 141 insertions(+), 0 deletions(-)
> 
> --
> 1.7.5.4

I think, you need to re-submit this after addressing comments from some
guys.

Thanks.

- Kukjin