[PATCH V2 1/2] hwmon: pwm-fan: Add profile support and add remove module support

[PATCH V2 1/2] hwmon: pwm-fan: Add profile support and add remove module support

[Sandipan Patra]

Add support for profiles mode settings.
This allows different fan settings for trip point temp/hyst/pwm.
Tegra194 has multiple fan-profiles support.

Signed-off-by: Sandipan Patra <spatra@nvidia.com>
---

PATCH V2:
	Cleaned pwm_fan_remove support as it is not required.

 drivers/hwmon/pwm-fan.c | 92 ++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 80 insertions(+), 12 deletions(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 30b7b3e..1d2a416 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -3,8 +3,10 @@
  * pwm-fan.c - Hwmon driver for fans connected to PWM lines.
  *
  * Copyright (c) 2014 Samsung Electronics Co., Ltd.
+ * Copyright (c) 2020, NVIDIA Corporation.
  *
  * Author: Kamil Debski <k.debski@samsung.com>
+ * Author: Sandipan Patra <spatra@nvidia.com>
  */
 
 #include <linux/hwmon.h>
@@ -21,6 +23,8 @@
 #include <linux/timer.h>
 
 #define MAX_PWM 255
+/* Based on OF max device tree node name length */
+#define MAX_PROFILE_NAME_LENGTH	31
 
 struct pwm_fan_ctx {
 	struct mutex lock;
@@ -38,6 +42,12 @@ struct pwm_fan_ctx {
 	unsigned int pwm_fan_state;
 	unsigned int pwm_fan_max_state;
 	unsigned int *pwm_fan_cooling_levels;
+
+	unsigned int pwm_fan_profiles;
+	const char **fan_profile_names;
+	unsigned int **fan_profile_cooling_levels;
+	unsigned int fan_current_profile;
+
 	struct thermal_cooling_device *cdev;
 };
 
@@ -227,28 +237,86 @@ static int pwm_fan_of_get_cooling_data(struct device *dev,
 				       struct pwm_fan_ctx *ctx)
 {
 	struct device_node *np = dev->of_node;
+	struct device_node *base_profile = NULL;
+	struct device_node *profile_np = NULL;
+	const char *default_profile = NULL;
 	int num, i, ret;
 
-	if (!of_find_property(np, "cooling-levels", NULL))
-		return 0;
+	num = of_property_count_u32_elems(np, "cooling-levels");
+	if (num <= 0) {
+		base_profile = of_get_child_by_name(np, "profiles");
+		if (!base_profile) {
+			dev_err(dev, "Wrong Data\n");
+			return -EINVAL;
+		}
+	}
+
+	if (base_profile) {
+		ctx->pwm_fan_profiles =
+			of_get_available_child_count(base_profile);
 
-	ret = of_property_count_u32_elems(np, "cooling-levels");
-	if (ret <= 0) {
-		dev_err(dev, "Wrong data!\n");
-		return ret ? : -EINVAL;
+		if (ctx->pwm_fan_profiles <= 0) {
+			dev_err(dev, "Profiles used but not defined\n");
+			return -EINVAL;
+		}
+
+		ctx->fan_profile_names = devm_kzalloc(dev,
+			sizeof(const char *) * ctx->pwm_fan_profiles,
+							GFP_KERNEL);
+		ctx->fan_profile_cooling_levels = devm_kzalloc(dev,
+			sizeof(int *) * ctx->pwm_fan_profiles,
+							GFP_KERNEL);
+
+		if (!ctx->fan_profile_names
+				|| !ctx->fan_profile_cooling_levels)
+			return -ENOMEM;
+
+		ctx->fan_current_profile = 0;
+		i = 0;
+		for_each_available_child_of_node(base_profile, profile_np) {
+			num = of_property_count_u32_elems(profile_np,
+							"cooling-levels");
+			if (num <= 0) {
+				dev_err(dev, "No data in cooling-levels inside profile node!\n");
+				return -EINVAL;
+			}
+
+			of_property_read_string(profile_np, "name",
+						&ctx->fan_profile_names[i]);
+			if (default_profile &&
+				!strncmp(default_profile,
+				ctx->fan_profile_names[i],
+				MAX_PROFILE_NAME_LENGTH))
+				ctx->fan_current_profile = i;
+
+			ctx->fan_profile_cooling_levels[i] =
+				devm_kzalloc(dev, sizeof(int) * num,
+							GFP_KERNEL);
+			if (!ctx->fan_profile_cooling_levels[i])
+				return -ENOMEM;
+
+			of_property_read_u32_array(profile_np, "cooling-levels",
+				ctx->fan_profile_cooling_levels[i], num);
+			i++;
+		}
 	}
 
-	num = ret;
 	ctx->pwm_fan_cooling_levels = devm_kcalloc(dev, num, sizeof(u32),
 						   GFP_KERNEL);
 	if (!ctx->pwm_fan_cooling_levels)
 		return -ENOMEM;
 
-	ret = of_property_read_u32_array(np, "cooling-levels",
-					 ctx->pwm_fan_cooling_levels, num);
-	if (ret) {
-		dev_err(dev, "Property 'cooling-levels' cannot be read!\n");
-		return ret;
+	if (base_profile) {
+		memcpy(ctx->pwm_fan_cooling_levels,
+		  ctx->fan_profile_cooling_levels[ctx->fan_current_profile],
+						num);
+	} else {
+		ret = of_property_read_u32_array(np, "cooling-levels",
+				ctx->pwm_fan_cooling_levels, num);
+		if (ret) {
+			dev_err(dev, "Property 'cooling-levels' cannot be read!\n");
+			return -EINVAL;
+		}
 	}
 
 	for (i = 0; i < num; i++) {
-- 
2.7.4

[PATCH V2 2/2] arm64: tegra: Add pwm-fan profile settings

[Sandipan Patra]

Add support for profiles in device tree to allow
different fan settings for trip point temp/hyst/pwm.

Signed-off-by: Sandipan Patra <spatra@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
index e15d1ea..ff2b980 100644
--- a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
+++ b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
@@ -219,10 +219,19 @@
 
 	fan: fan {
 		compatible = "pwm-fan";
-		pwms = <&pwm4 0 45334>;
-
-		cooling-levels = <0 64 128 255>;
 		#cooling-cells = <2>;
+		pwms = <&pwm4 0 45334>;
+		profiles {
+			default = "quiet";
+			quiet {
+				state_cap = <4>;
+				cooling-levels = <0 77 120 160 255 255 255 255 255 255>;
+			};
+			cool {
+				state_cap = <4>;
+				cooling-levels = <0 77 120 160 255 255 255 255 255 255>;
+			};
+		};
 	};
 
 	gpio-keys {
-- 
2.7.4

Re: [PATCH V2 1/2] hwmon: pwm-fan: Add profile support and add remove module support

[Guenter Roeck]

On Mon, Jun 01, 2020 at 11:49:13AM +0530, Sandipan Patra wrote:
> Add support for profiles mode settings.
> This allows different fan settings for trip point temp/hyst/pwm.
> Tegra194 has multiple fan-profiles support.
> 
> Signed-off-by: Sandipan Patra <spatra-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

The subject says "remove module support". What is this supposed to be
about ?

The code adds support for multiple cooling "profiles" but, unless I am
really missing something, no means to actually select a profile.
This adds a lot of complexity to the code with zero value.

Either case, and I may have mentioned this before, functionality like this
should really reside in the thermal core and not in individual drivers.

> ---
> 
> PATCH V2:
> 	Cleaned pwm_fan_remove support as it is not required.
> 
>  drivers/hwmon/pwm-fan.c | 92 ++++++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 80 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index 30b7b3e..1d2a416 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -3,8 +3,10 @@
>   * pwm-fan.c - Hwmon driver for fans connected to PWM lines.
>   *
>   * Copyright (c) 2014 Samsung Electronics Co., Ltd.
> + * Copyright (c) 2020, NVIDIA Corporation.
>   *
>   * Author: Kamil Debski <k.debski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> + * Author: Sandipan Patra <spatra-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

You can not claim authorship for a driver by adding a few lines of code
to it.

>   */
>  
>  #include <linux/hwmon.h>
> @@ -21,6 +23,8 @@
>  #include <linux/timer.h>
>  
>  #define MAX_PWM 255
> +/* Based on OF max device tree node name length */
> +#define MAX_PROFILE_NAME_LENGTH	31
>  
>  struct pwm_fan_ctx {
>  	struct mutex lock;
> @@ -38,6 +42,12 @@ struct pwm_fan_ctx {
>  	unsigned int pwm_fan_state;
>  	unsigned int pwm_fan_max_state;
>  	unsigned int *pwm_fan_cooling_levels;
> +
> +	unsigned int pwm_fan_profiles;
> +	const char **fan_profile_names;
> +	unsigned int **fan_profile_cooling_levels;
> +	unsigned int fan_current_profile;
> +
>  	struct thermal_cooling_device *cdev;
>  };
>  
> @@ -227,28 +237,86 @@ static int pwm_fan_of_get_cooling_data(struct device *dev,
>  				       struct pwm_fan_ctx *ctx)
>  {
>  	struct device_node *np = dev->of_node;
> +	struct device_node *base_profile = NULL;
> +	struct device_node *profile_np = NULL;
> +	const char *default_profile = NULL;
>  	int num, i, ret;
>  
> -	if (!of_find_property(np, "cooling-levels", NULL))
> -		return 0;
> +	num = of_property_count_u32_elems(np, "cooling-levels");
> +	if (num <= 0) {
> +		base_profile = of_get_child_by_name(np, "profiles");

You can not just add new properties like this without documenting it
and getting approval by devicetree maintainers.

Guenter

> +		if (!base_profile) {
> +			dev_err(dev, "Wrong Data\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	if (base_profile) {
> +		ctx->pwm_fan_profiles =
> +			of_get_available_child_count(base_profile);
>  
> -	ret = of_property_count_u32_elems(np, "cooling-levels");
> -	if (ret <= 0) {
> -		dev_err(dev, "Wrong data!\n");
> -		return ret ? : -EINVAL;
> +		if (ctx->pwm_fan_profiles <= 0) {
> +			dev_err(dev, "Profiles used but not defined\n");
> +			return -EINVAL;
> +		}
> +
> +		ctx->fan_profile_names = devm_kzalloc(dev,
> +			sizeof(const char *) * ctx->pwm_fan_profiles,
> +							GFP_KERNEL);
> +		ctx->fan_profile_cooling_levels = devm_kzalloc(dev,
> +			sizeof(int *) * ctx->pwm_fan_profiles,
> +							GFP_KERNEL);
> +
> +		if (!ctx->fan_profile_names
> +				|| !ctx->fan_profile_cooling_levels)
> +			return -ENOMEM;
> +
> +		ctx->fan_current_profile = 0;
> +		i = 0;
> +		for_each_available_child_of_node(base_profile, profile_np) {
> +			num = of_property_count_u32_elems(profile_np,
> +							"cooling-levels");
> +			if (num <= 0) {
> +				dev_err(dev, "No data in cooling-levels inside profile node!\n");
> +				return -EINVAL;
> +			}
> +
> +			of_property_read_string(profile_np, "name",
> +						&ctx->fan_profile_names[i]);
> +			if (default_profile &&
> +				!strncmp(default_profile,
> +				ctx->fan_profile_names[i],
> +				MAX_PROFILE_NAME_LENGTH))
> +				ctx->fan_current_profile = i;
> +
> +			ctx->fan_profile_cooling_levels[i] =
> +				devm_kzalloc(dev, sizeof(int) * num,
> +							GFP_KERNEL);
> +			if (!ctx->fan_profile_cooling_levels[i])
> +				return -ENOMEM;
> +
> +			of_property_read_u32_array(profile_np, "cooling-levels",
> +				ctx->fan_profile_cooling_levels[i], num);
> +			i++;
> +		}
>  	}
>  
> -	num = ret;
>  	ctx->pwm_fan_cooling_levels = devm_kcalloc(dev, num, sizeof(u32),
>  						   GFP_KERNEL);
>  	if (!ctx->pwm_fan_cooling_levels)
>  		return -ENOMEM;
>  
> -	ret = of_property_read_u32_array(np, "cooling-levels",
> -					 ctx->pwm_fan_cooling_levels, num);
> -	if (ret) {
> -		dev_err(dev, "Property 'cooling-levels' cannot be read!\n");
> -		return ret;
> +	if (base_profile) {
> +		memcpy(ctx->pwm_fan_cooling_levels,
> +		  ctx->fan_profile_cooling_levels[ctx->fan_current_profile],
> +						num);
> +	} else {
> +		ret = of_property_read_u32_array(np, "cooling-levels",
> +				ctx->pwm_fan_cooling_levels, num);
> +		if (ret) {
> +			dev_err(dev, "Property 'cooling-levels' cannot be read!\n");
> +			return -EINVAL;
> +		}
>  	}
>  
>  	for (i = 0; i < num; i++) {

RE: [PATCH V2 1/2] hwmon: pwm-fan: Add profile support and add remove module support

[Sandipan Patra]

Hi Guenter,

Agreed with the suggestion with regards to fan profile support.
Looked at thermal driver and further planning to implement required changes in thermal core instead of pwm-fan driver.
Dropping the current series. 


Thanks & Regards,
Sandipan

> -----Original Message-----
> From: Guenter Roeck <groeck7-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> On Behalf Of Guenter Roeck
> Sent: Tuesday, June 2, 2020 7:58 PM
> To: Sandipan Patra <spatra-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> Cc: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>; Jonathan Hunter
> <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>; kamil-MZ2JYD8fkKLYtjvyW6yDsg@public.gmane.org; jdelvare-IBi9RG/b67k@public.gmane.org;
> robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org; Bibek Basu
> <bbasu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>; Bitan Biswas <bbiswas-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>; Krishna Yarlagadda
> <kyarlagadda-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>; linux-pwm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
> hwmon-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-
> tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH V2 1/2] hwmon: pwm-fan: Add profile support and add
> remove module support
> 
> External email: Use caution opening links or attachments
> 
> 
> On Mon, Jun 01, 2020 at 11:49:13AM +0530, Sandipan Patra wrote:
> > Add support for profiles mode settings.
> > This allows different fan settings for trip point temp/hyst/pwm.
> > Tegra194 has multiple fan-profiles support.
> >
> > Signed-off-by: Sandipan Patra <spatra-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> The subject says "remove module support". What is this supposed to be about ?
> 
> The code adds support for multiple cooling "profiles" but, unless I am really
> missing something, no means to actually select a profile.
> This adds a lot of complexity to the code with zero value.
> 
> Either case, and I may have mentioned this before, functionality like this should
> really reside in the thermal core and not in individual drivers.
> 
> > ---
> >
> > PATCH V2:
> >       Cleaned pwm_fan_remove support as it is not required.
> >
> >  drivers/hwmon/pwm-fan.c | 92
> > ++++++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 80 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c index
> > 30b7b3e..1d2a416 100644
> > --- a/drivers/hwmon/pwm-fan.c
> > +++ b/drivers/hwmon/pwm-fan.c
> > @@ -3,8 +3,10 @@
> >   * pwm-fan.c - Hwmon driver for fans connected to PWM lines.
> >   *
> >   * Copyright (c) 2014 Samsung Electronics Co., Ltd.
> > + * Copyright (c) 2020, NVIDIA Corporation.
> >   *
> >   * Author: Kamil Debski <k.debski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > + * Author: Sandipan Patra <spatra-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> You can not claim authorship for a driver by adding a few lines of code to it.
> 
> >   */
> >
> >  #include <linux/hwmon.h>
> > @@ -21,6 +23,8 @@
> >  #include <linux/timer.h>
> >
> >  #define MAX_PWM 255
> > +/* Based on OF max device tree node name length */
> > +#define MAX_PROFILE_NAME_LENGTH      31
> >
> >  struct pwm_fan_ctx {
> >       struct mutex lock;
> > @@ -38,6 +42,12 @@ struct pwm_fan_ctx {
> >       unsigned int pwm_fan_state;
> >       unsigned int pwm_fan_max_state;
> >       unsigned int *pwm_fan_cooling_levels;
> > +
> > +     unsigned int pwm_fan_profiles;
> > +     const char **fan_profile_names;
> > +     unsigned int **fan_profile_cooling_levels;
> > +     unsigned int fan_current_profile;
> > +
> >       struct thermal_cooling_device *cdev;  };
> >
> > @@ -227,28 +237,86 @@ static int pwm_fan_of_get_cooling_data(struct
> device *dev,
> >                                      struct pwm_fan_ctx *ctx)  {
> >       struct device_node *np = dev->of_node;
> > +     struct device_node *base_profile = NULL;
> > +     struct device_node *profile_np = NULL;
> > +     const char *default_profile = NULL;
> >       int num, i, ret;
> >
> > -     if (!of_find_property(np, "cooling-levels", NULL))
> > -             return 0;
> > +     num = of_property_count_u32_elems(np, "cooling-levels");
> > +     if (num <= 0) {
> > +             base_profile = of_get_child_by_name(np, "profiles");
> 
> You can not just add new properties like this without documenting it and getting
> approval by devicetree maintainers.
> 
> Guenter
> 
> > +             if (!base_profile) {
> > +                     dev_err(dev, "Wrong Data\n");
> > +                     return -EINVAL;
> > +             }
> > +     }
> > +
> > +     if (base_profile) {
> > +             ctx->pwm_fan_profiles =
> > +                     of_get_available_child_count(base_profile);
> >
> > -     ret = of_property_count_u32_elems(np, "cooling-levels");
> > -     if (ret <= 0) {
> > -             dev_err(dev, "Wrong data!\n");
> > -             return ret ? : -EINVAL;
> > +             if (ctx->pwm_fan_profiles <= 0) {
> > +                     dev_err(dev, "Profiles used but not defined\n");
> > +                     return -EINVAL;
> > +             }
> > +
> > +             ctx->fan_profile_names = devm_kzalloc(dev,
> > +                     sizeof(const char *) * ctx->pwm_fan_profiles,
> > +                                                     GFP_KERNEL);
> > +             ctx->fan_profile_cooling_levels = devm_kzalloc(dev,
> > +                     sizeof(int *) * ctx->pwm_fan_profiles,
> > +                                                     GFP_KERNEL);
> > +
> > +             if (!ctx->fan_profile_names
> > +                             || !ctx->fan_profile_cooling_levels)
> > +                     return -ENOMEM;
> > +
> > +             ctx->fan_current_profile = 0;
> > +             i = 0;
> > +             for_each_available_child_of_node(base_profile, profile_np) {
> > +                     num = of_property_count_u32_elems(profile_np,
> > +                                                     "cooling-levels");
> > +                     if (num <= 0) {
> > +                             dev_err(dev, "No data in cooling-levels inside profile
> node!\n");
> > +                             return -EINVAL;
> > +                     }
> > +
> > +                     of_property_read_string(profile_np, "name",
> > +                                             &ctx->fan_profile_names[i]);
> > +                     if (default_profile &&
> > +                             !strncmp(default_profile,
> > +                             ctx->fan_profile_names[i],
> > +                             MAX_PROFILE_NAME_LENGTH))
> > +                             ctx->fan_current_profile = i;
> > +
> > +                     ctx->fan_profile_cooling_levels[i] =
> > +                             devm_kzalloc(dev, sizeof(int) * num,
> > +                                                     GFP_KERNEL);
> > +                     if (!ctx->fan_profile_cooling_levels[i])
> > +                             return -ENOMEM;
> > +
> > +                     of_property_read_u32_array(profile_np, "cooling-levels",
> > +                             ctx->fan_profile_cooling_levels[i], num);
> > +                     i++;
> > +             }
> >       }
> >
> > -     num = ret;
> >       ctx->pwm_fan_cooling_levels = devm_kcalloc(dev, num, sizeof(u32),
> >                                                  GFP_KERNEL);
> >       if (!ctx->pwm_fan_cooling_levels)
> >               return -ENOMEM;
> >
> > -     ret = of_property_read_u32_array(np, "cooling-levels",
> > -                                      ctx->pwm_fan_cooling_levels, num);
> > -     if (ret) {
> > -             dev_err(dev, "Property 'cooling-levels' cannot be read!\n");
> > -             return ret;
> > +     if (base_profile) {
> > +             memcpy(ctx->pwm_fan_cooling_levels,
> > +               ctx->fan_profile_cooling_levels[ctx->fan_current_profile],
> > +                                             num);
> > +     } else {
> > +             ret = of_property_read_u32_array(np, "cooling-levels",
> > +                             ctx->pwm_fan_cooling_levels, num);
> > +             if (ret) {
> > +                     dev_err(dev, "Property 'cooling-levels' cannot be read!\n");
> > +                     return -EINVAL;
> > +             }
> >       }
> >
> >       for (i = 0; i < num; i++) {