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 <[email protected]>
---
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 <[email protected]>
+ * Author: Sandipan Patra <[email protected]>
*/
#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
Add support for profiles in device tree to allow
different fan settings for trip point temp/hyst/pwm.
Signed-off-by: Sandipan Patra <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
> + * Author: Sandipan Patra <[email protected]>
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++) {
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 <[email protected]> On Behalf Of Guenter Roeck
> Sent: Tuesday, June 2, 2020 7:58 PM
> To: Sandipan Patra <[email protected]>
> Cc: Thierry Reding <[email protected]>; Jonathan Hunter
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; Bibek Basu
> <[email protected]>; Bitan Biswas <[email protected]>; Krishna Yarlagadda
> <[email protected]>; [email protected]; linux-
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> 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 <[email protected]>
>
> 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 <[email protected]>
> > + * Author: Sandipan Patra <[email protected]>
>
> 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++) {