[PATCH v1 0/7] Thermal throttling for SDM845

[PATCH v1 0/7] Thermal throttling for SDM845

[Amit Kucheria]

Add support for thermal throttling on SDM845. 

We introduce a generic .ready callback to be used by cpufreq drivers to
register as a thermal cooling device. If this approach is acceptable I can
send a series converting other cpufreq drivers to use this callback.

Amit Kucheria (7):
  drivers: thermal: of-thermal: Print name of device node with error
  drivers: cpufreq: Add thermal_cooling_device pointer to struct
    cpufreq_policy
  cpu_cooling: Add generic driver ready callback
  cpufreq: qcom-hw: Move to device_initcall
  cpufreq: qcom-hw: Register as a cpufreq cooling device
  arm64: dts: sdm845: Increase alert trip point to 95 degrees
  arm64: dts: sdm845: wireup the thermal trip points to cpufreq

 arch/arm64/boot/dts/qcom/sdm845.dtsi | 161 +++++++++++++++++++++++++--
 drivers/cpufreq/qcom-cpufreq-hw.c    |   7 +-
 drivers/thermal/cpu_cooling.c        |  18 +++
 drivers/thermal/of-thermal.c         |   4 +-
 include/linux/cpu_cooling.h          |   9 ++
 include/linux/cpufreq.h              |   2 +
 6 files changed, 190 insertions(+), 11 deletions(-)

-- 
2.17.1

[PATCH v1 1/7] drivers: thermal: of-thermal: Print name of device node with error

[Amit Kucheria]

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 drivers/thermal/of-thermal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index 4bfdb4a1e47d..5ca2a6b370ea 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -867,14 +867,14 @@ __init *thermal_of_build_thermal_zone(struct device_node *np)
 
 	ret = of_property_read_u32(np, "polling-delay-passive", &prop);
 	if (ret < 0) {
-		pr_err("missing polling-delay-passive property\n");
+		pr_err("%s: missing polling-delay-passive property\n", np->name);
 		goto free_tz;
 	}
 	tz->passive_delay = prop;
 
 	ret = of_property_read_u32(np, "polling-delay", &prop);
 	if (ret < 0) {
-		pr_err("missing polling-delay property\n");
+		pr_err("%s: missing polling-delay property\n", np->name);
 		goto free_tz;
 	}
 	tz->polling_delay = prop;
-- 
2.17.1

[PATCH v1 2/7] drivers: cpufreq: Add thermal_cooling_device pointer to struct cpufreq_policy

[Amit Kucheria]

Several cpufreq drivers register themselves as thermal cooling devices.
Adding a pointer to struct cpufreq_policy removes the need for them to
store this pointer in a private data structure.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 include/linux/cpufreq.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index c86d6d8bdfed..2496549d7573 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -95,6 +95,8 @@ struct cpufreq_policy {
 	struct cpufreq_frequency_table	*freq_table;
 	enum cpufreq_table_sorting freq_table_sorted;
 
+	struct thermal_cooling_device *cooldev; /* Pointer to the cooling
+						 * device if used for thermal mitigation */
 	struct list_head        policy_list;
 	struct kobject		kobj;
 	struct completion	kobj_unregister;
-- 
2.17.1

[PATCH v1 3/7] cpu_cooling: Add generic driver ready callback

[Amit Kucheria]

All cpufreq drivers do similar things to register as a cooling device.
Provide a generic call back so we can get rid of duplicated code in the
drivers.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
Suggested-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/thermal/cpu_cooling.c | 18 ++++++++++++++++++
 include/linux/cpu_cooling.h   |  9 +++++++++
 2 files changed, 27 insertions(+)

diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
index dfd23245f778..5154ffc12332 100644
--- a/drivers/thermal/cpu_cooling.c
+++ b/drivers/thermal/cpu_cooling.c
@@ -694,6 +694,7 @@ __cpufreq_cooling_register(struct device_node *np,
 
 	cpufreq_cdev->clipped_freq = cpufreq_cdev->freq_table[0].frequency;
 	cpufreq_cdev->cdev = cdev;
+	policy->cooldev = cdev;
 
 	mutex_lock(&cooling_list_lock);
 	/* Register the notifier for first cpufreq cooling device */
@@ -785,6 +786,23 @@ of_cpufreq_cooling_register(struct cpufreq_policy *policy)
 }
 EXPORT_SYMBOL_GPL(of_cpufreq_cooling_register);
 
+/**
+ * generic_cpufreq_ready - generic driver callback to register a thermal_cooling_device
+ * @policy: cpufreq policy
+ */
+void generic_cpufreq_ready(struct cpufreq_policy *policy)
+{
+	struct thermal_cooling_device **cdev = &policy->cooldev;
+
+	*cdev = of_cpufreq_cooling_register(policy);
+	if (IS_ERR(*cdev)) {
+		pr_err("Failed to register cpufreq cooling device for CPU%d: %ld\n",
+		       policy->cpu, PTR_ERR(cdev));
+		cdev = NULL;
+	}
+}
+EXPORT_SYMBOL_GPL(generic_cpufreq_ready);
+
 /**
  * cpufreq_cooling_unregister - function to remove cpufreq cooling device.
  * @cdev: thermal cooling device pointer.
diff --git a/include/linux/cpu_cooling.h b/include/linux/cpu_cooling.h
index de0dafb9399d..d7815bb2967a 100644
--- a/include/linux/cpu_cooling.h
+++ b/include/linux/cpu_cooling.h
@@ -65,12 +65,21 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev)
  */
 struct thermal_cooling_device *
 of_cpufreq_cooling_register(struct cpufreq_policy *policy);
+
+/**
+ * generic_cpufreq_ready - generic driver callback to register a thermal_cooling_device
+ * @policy: cpufreq policy
+ */
+void generic_cpufreq_ready(struct cpufreq_policy *policy);
 #else
 static inline struct thermal_cooling_device *
 of_cpufreq_cooling_register(struct cpufreq_policy *policy)
 {
 	return NULL;
 }
+
+void generic_cpufreq_ready(struct cpufreq_policy *policy) {}
+
 #endif /* defined(CONFIG_THERMAL_OF) && defined(CONFIG_CPU_THERMAL) */
 
 #endif /* __CPU_COOLING_H__ */
-- 
2.17.1

[PATCH v1 4/7] cpufreq: qcom-hw: Move to device_initcall

[Amit Kucheria]

subsys_initcall causes problems registering the driver as a thermal
cooling device.

If "faster boot" is the main reason for doing subsys_initcall, this
should be handled in the bootloader or another boot constraint
framework.

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 drivers/cpufreq/qcom-cpufreq-hw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index d83939a1b3d4..649dddd72749 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -296,7 +296,7 @@ static int __init qcom_cpufreq_hw_init(void)
 {
 	return platform_driver_register(&qcom_cpufreq_hw_driver);
 }
-subsys_initcall(qcom_cpufreq_hw_init);
+device_initcall(qcom_cpufreq_hw_init);
 
 static void __exit qcom_cpufreq_hw_exit(void)
 {
-- 
2.17.1

[PATCH v1 5/7] cpufreq: qcom-hw: Register as a cpufreq cooling device

[Amit Kucheria]

Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
---
 drivers/cpufreq/qcom-cpufreq-hw.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 649dddd72749..1c01311e5927 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -5,6 +5,7 @@
 
 #include <linux/bitfield.h>
 #include <linux/cpufreq.h>
+#include <linux/cpu_cooling.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -216,7 +217,10 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy)
 {
 	void __iomem *base = policy->driver_data - REG_PERF_STATE;
+	struct thermal_cooling_device *cdev = policy->cooldev;
 
+	if (cdev)
+		cpufreq_cooling_unregister(cdev);
 	kfree(policy->freq_table);
 	devm_iounmap(&global_pdev->dev, base);
 
@@ -238,6 +242,7 @@ static struct cpufreq_driver cpufreq_qcom_hw_driver = {
 	.init		= qcom_cpufreq_hw_cpu_init,
 	.exit		= qcom_cpufreq_hw_cpu_exit,
 	.fast_switch    = qcom_cpufreq_hw_fast_switch,
+	.ready		= generic_cpufreq_ready,
 	.name		= "qcom-cpufreq-hw",
 	.attr		= qcom_cpufreq_hw_attr,
 };
-- 
2.17.1

Re: [PATCH v1 1/7] drivers: thermal: of-thermal: Print name of device node with error

[Stephen Boyd]

Quoting Amit Kucheria (2019-01-09 16:00:50)
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
>  drivers/thermal/of-thermal.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
> index 4bfdb4a1e47d..5ca2a6b370ea 100644
> --- a/drivers/thermal/of-thermal.c
> +++ b/drivers/thermal/of-thermal.c
> @@ -867,14 +867,14 @@ __init *thermal_of_build_thermal_zone(struct device_node *np)
>  
>         ret = of_property_read_u32(np, "polling-delay-passive", &prop);
>         if (ret < 0) {
> -               pr_err("missing polling-delay-passive property\n");
> +               pr_err("%s: missing polling-delay-passive property\n", np->name);

You should use %pOFn to print node names.

Re: [PATCH v1 2/7] drivers: cpufreq: Add thermal_cooling_device pointer to struct cpufreq_policy

[Matthias Kaehlcke]

On Thu, Jan 10, 2019 at 05:30:51AM +0530, Amit Kucheria wrote:
> Several cpufreq drivers register themselves as thermal cooling devices.
> Adding a pointer to struct cpufreq_policy removes the need for them to
> store this pointer in a private data structure.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
>  include/linux/cpufreq.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index c86d6d8bdfed..2496549d7573 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -95,6 +95,8 @@ struct cpufreq_policy {
>  	struct cpufreq_frequency_table	*freq_table;
>  	enum cpufreq_table_sorting freq_table_sorted;
>  
> +	struct thermal_cooling_device *cooldev; /* Pointer to the cooling
> +						 * device if used for thermal mitigation */
>  	struct list_head        policy_list;
>  	struct kobject		kobj;
>  	struct completion	kobj_unregister;

I've mixed feelings about this. It's definitely desirable to avoid
code duplication and tying the cooling device to the cpufreq_policy is
a convenient way to achieve that. However semantically it seems a bit
odd that a CPU cooling device is part of the cpufreq policy.

Anyway, unless there are better ideas we probably want to be pragmatic
here, so if Viresh is fine with it who am I to complain ;-)

Cheers

Matthias

Re: [PATCH v1 5/7] cpufreq: qcom-hw: Register as a cpufreq cooling device

[Viresh Kumar]

On 10-01-19, 05:30, Amit Kucheria wrote:
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
>  drivers/cpufreq/qcom-cpufreq-hw.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index 649dddd72749..1c01311e5927 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -5,6 +5,7 @@
>  
>  #include <linux/bitfield.h>
>  #include <linux/cpufreq.h>
> +#include <linux/cpu_cooling.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> @@ -216,7 +217,10 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy)
>  {
>  	void __iomem *base = policy->driver_data - REG_PERF_STATE;
> +	struct thermal_cooling_device *cdev = policy->cooldev;
>  
> +	if (cdev)
> +		cpufreq_cooling_unregister(cdev);
>  	kfree(policy->freq_table);
>  	devm_iounmap(&global_pdev->dev, base);
>  
> @@ -238,6 +242,7 @@ static struct cpufreq_driver cpufreq_qcom_hw_driver = {
>  	.init		= qcom_cpufreq_hw_cpu_init,
>  	.exit		= qcom_cpufreq_hw_cpu_exit,
>  	.fast_switch    = qcom_cpufreq_hw_fast_switch,
> +	.ready		= generic_cpufreq_ready,
>  	.name		= "qcom-cpufreq-hw",
>  	.attr		= qcom_cpufreq_hw_attr,
>  };

I liked the idea of reducing code duplication, but not much the
implementation. All we were able to get rid of was a call to
of_cpufreq_cooling_register() and nothing else. Is it worth it ?

Maybe we can add another flag in cpufreq.h:

#define CPUFREQ_AUTO_REGISTER_COOLING_DEV (1 << 7)

and let the core do it all automatically by itself, that will get rid
of code duplication actually.

@Rafael: What do you say ?

-- 
viresh

Re: [PATCH v1 3/7] cpu_cooling: Add generic driver ready callback

[Viresh Kumar]

On 10-01-19, 05:30, Amit Kucheria wrote:
> All cpufreq drivers do similar things to register as a cooling device.
> Provide a generic call back so we can get rid of duplicated code in the
> drivers.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> Suggested-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/thermal/cpu_cooling.c | 18 ++++++++++++++++++
>  include/linux/cpu_cooling.h   |  9 +++++++++
>  2 files changed, 27 insertions(+)

We may be doing this differently, but I found some other issues with
the patch.

> diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c
> index dfd23245f778..5154ffc12332 100644
> --- a/drivers/thermal/cpu_cooling.c
> +++ b/drivers/thermal/cpu_cooling.c
> @@ -694,6 +694,7 @@ __cpufreq_cooling_register(struct device_node *np,
>  
>  	cpufreq_cdev->clipped_freq = cpufreq_cdev->freq_table[0].frequency;
>  	cpufreq_cdev->cdev = cdev;
> +	policy->cooldev = cdev;

Why was this required ? The below routine was already doing it...

>  
>  	mutex_lock(&cooling_list_lock);
>  	/* Register the notifier for first cpufreq cooling device */
> @@ -785,6 +786,23 @@ of_cpufreq_cooling_register(struct cpufreq_policy *policy)
>  }
>  EXPORT_SYMBOL_GPL(of_cpufreq_cooling_register);
>  
> +/**
> + * generic_cpufreq_ready - generic driver callback to register a thermal_cooling_device
> + * @policy: cpufreq policy
> + */
> +void generic_cpufreq_ready(struct cpufreq_policy *policy)
> +{
> +	struct thermal_cooling_device **cdev = &policy->cooldev;
> +
> +	*cdev = of_cpufreq_cooling_register(policy);

... here

> +	if (IS_ERR(*cdev)) {

We never get an error here, only NULL.

> +		pr_err("Failed to register cpufreq cooling device for CPU%d: %ld\n",
> +		       policy->cpu, PTR_ERR(cdev));

The of_cpufreq_cooling_register() routine already prints enough error
messages on failures.

> +		cdev = NULL;

This is a meaningless statement, perhaps you wanted to do *cdev = NULL
:)

-- 
viresh

Re: [PATCH v1 4/7] cpufreq: qcom-hw: Move to device_initcall

[Viresh Kumar]

On 10-01-19, 05:30, Amit Kucheria wrote:
> subsys_initcall causes problems registering the driver as a thermal
> cooling device.
> 
> If "faster boot" is the main reason for doing subsys_initcall, this
> should be handled in the bootloader or another boot constraint
> framework.
> 
> Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> ---
>  drivers/cpufreq/qcom-cpufreq-hw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index d83939a1b3d4..649dddd72749 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -296,7 +296,7 @@ static int __init qcom_cpufreq_hw_init(void)
>  {
>  	return platform_driver_register(&qcom_cpufreq_hw_driver);
>  }
> -subsys_initcall(qcom_cpufreq_hw_init);
> +device_initcall(qcom_cpufreq_hw_init);
>  
>  static void __exit qcom_cpufreq_hw_exit(void)
>  {

Applied. Thanks.

-- 
viresh

Re: [PATCH v1 5/7] cpufreq: qcom-hw: Register as a cpufreq cooling device

[Amit Kucheria]

On Thu, Jan 10, 2019 at 11:42 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 10-01-19, 05:30, Amit Kucheria wrote:
> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> > ---
> >  drivers/cpufreq/qcom-cpufreq-hw.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> > index 649dddd72749..1c01311e5927 100644
> > --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> > @@ -5,6 +5,7 @@
> >
> >  #include <linux/bitfield.h>
> >  #include <linux/cpufreq.h>
> > +#include <linux/cpu_cooling.h>
> >  #include <linux/init.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> > @@ -216,7 +217,10 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> >  static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy)
> >  {
> >       void __iomem *base = policy->driver_data - REG_PERF_STATE;
> > +     struct thermal_cooling_device *cdev = policy->cooldev;
> >
> > +     if (cdev)
> > +             cpufreq_cooling_unregister(cdev);
> >       kfree(policy->freq_table);
> >       devm_iounmap(&global_pdev->dev, base);
> >
> > @@ -238,6 +242,7 @@ static struct cpufreq_driver cpufreq_qcom_hw_driver = {
> >       .init           = qcom_cpufreq_hw_cpu_init,
> >       .exit           = qcom_cpufreq_hw_cpu_exit,
> >       .fast_switch    = qcom_cpufreq_hw_fast_switch,
> > +     .ready          = generic_cpufreq_ready,
> >       .name           = "qcom-cpufreq-hw",
> >       .attr           = qcom_cpufreq_hw_attr,
> >  };
>
> I liked the idea of reducing code duplication, but not much the
> implementation. All we were able to get rid of was a call to
> of_cpufreq_cooling_register() and nothing else. Is it worth it ?
>
> Maybe we can add another flag in cpufreq.h:
>
> #define CPUFREQ_AUTO_REGISTER_COOLING_DEV (1 << 7)
>
> and let the core do it all automatically by itself, that will get rid
> of code duplication actually.

I like the idea of a flag. I'll spin something implementing it in the next rev.

> @Rafael: What do you say ?

Re: [PATCH v1 5/7] cpufreq: qcom-hw: Register as a cpufreq cooling device

[Rafael J. Wysocki]

On Thu, Jan 10, 2019 at 7:12 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 10-01-19, 05:30, Amit Kucheria wrote:
> > Signed-off-by: Amit Kucheria <amit.kucheria@linaro.org>
> > ---
> >  drivers/cpufreq/qcom-cpufreq-hw.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> > index 649dddd72749..1c01311e5927 100644
> > --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> > @@ -5,6 +5,7 @@
> >
> >  #include <linux/bitfield.h>
> >  #include <linux/cpufreq.h>
> > +#include <linux/cpu_cooling.h>
> >  #include <linux/init.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> > @@ -216,7 +217,10 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> >  static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy)
> >  {
> >       void __iomem *base = policy->driver_data - REG_PERF_STATE;
> > +     struct thermal_cooling_device *cdev = policy->cooldev;
> >
> > +     if (cdev)
> > +             cpufreq_cooling_unregister(cdev);
> >       kfree(policy->freq_table);
> >       devm_iounmap(&global_pdev->dev, base);
> >
> > @@ -238,6 +242,7 @@ static struct cpufreq_driver cpufreq_qcom_hw_driver = {
> >       .init           = qcom_cpufreq_hw_cpu_init,
> >       .exit           = qcom_cpufreq_hw_cpu_exit,
> >       .fast_switch    = qcom_cpufreq_hw_fast_switch,
> > +     .ready          = generic_cpufreq_ready,
> >       .name           = "qcom-cpufreq-hw",
> >       .attr           = qcom_cpufreq_hw_attr,
> >  };
>
> I liked the idea of reducing code duplication, but not much the
> implementation. All we were able to get rid of was a call to
> of_cpufreq_cooling_register() and nothing else. Is it worth it ?
>
> Maybe we can add another flag in cpufreq.h:
>
> #define CPUFREQ_AUTO_REGISTER_COOLING_DEV (1 << 7)
>
> and let the core do it all automatically by itself, that will get rid
> of code duplication actually.
>
> @Rafael: What do you say ?

Getting rid of code duplication is good, let's do that.