| Message ID | 1590469565-14953-1-git-send-email-spatra@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] hwmon: pwm-fan: Add profile support and add remove module support | expand |
On Tue, May 26, 2020 at 10:36:04AM +0530, Sandipan Patra wrote: > This change has 2 parts: > 1. Add support for profiles mode settings. > This allows different fan settings for trip point temp/hyst/pwm. > T194 has multiple fan-profiles support. > > 2. Add pwm-fan remove support. This is essential since the config is > tristate capable. These two are orthogonal, aren't they? So they belong in two patches. You have to expand the binding documentation. > Signed-off-by: Sandipan Patra <spatra@nvidia.com> > --- > drivers/hwmon/pwm-fan.c | 112 ++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 100 insertions(+), 12 deletions(-) > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c > index 30b7b3e..26db589 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); > + > + if (ctx->pwm_fan_profiles <= 0) { > + dev_err(dev, "Profiles used but not defined\n"); > + return -EINVAL; > + } > > - ret = of_property_count_u32_elems(np, "cooling-levels"); > - if (ret <= 0) { > - dev_err(dev, "Wrong data!\n"); > - return ret ? : -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++) { > @@ -390,6 +458,25 @@ static int pwm_fan_probe(struct platform_device *pdev) > return 0; > } > > +static int pwm_fan_remove(struct platform_device *pdev) > +{ > + struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev); > + struct pwm_args args; > + > + if (!ctx) > + return -EINVAL; > + > + if (IS_ENABLED(CONFIG_THERMAL)) > + thermal_cooling_device_unregister(ctx->cdev); > + > + pwm_get_args(ctx->pwm, &args); > + pwm_config(ctx->pwm, 0, args.period); > + pwm_disable(ctx->pwm); What is what you really here? Is it only that the PWM stops oscillating, or is it crucial that the output goes to its inactive level? (The intended semantic of pwm_disable includes that the output goes low, but not all implementations enforce this.) Also please don't introduce new users of pwm_config() and pwm_disable() use pwm_apply() instead. I wonder if this unregistration is "safe". When the driver is in use I'd expect that the hwmon device doesn't go away and so the devm unregistration callback that belongs to devm_hwmon_device_register_with_groups() blocks. But at this time the PWM is already stopped and so the device stops functioning earlier. Best regards Uwe
Hi Uwe, > -----Original Message----- > From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Sent: Tuesday, May 26, 2020 12:35 PM > To: Sandipan Patra <spatra@nvidia.com> > Cc: Thierry Reding <treding@nvidia.com>; Jonathan Hunter > <jonathanh@nvidia.com>; kamil@wypas.org; jdelvare@suse.com; linux@roeck- > us.net; robh+dt@kernel.org; Bibek Basu <bbasu@nvidia.com>; Bitan Biswas > <bbiswas@nvidia.com>; linux-pwm@vger.kernel.org; linux- > hwmon@vger.kernel.org; devicetree@vger.kernel.org; linux- > tegra@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH 1/2] hwmon: pwm-fan: Add profile support and add remove > module support > > External email: Use caution opening links or attachments > > > On Tue, May 26, 2020 at 10:36:04AM +0530, Sandipan Patra wrote: > > This change has 2 parts: > > 1. Add support for profiles mode settings. > > This allows different fan settings for trip point temp/hyst/pwm. > > T194 has multiple fan-profiles support. > > > > 2. Add pwm-fan remove support. This is essential since the config is > > tristate capable. > > These two are orthogonal, aren't they? So they belong in two patches. > > You have to expand the binding documentation. > > > Signed-off-by: Sandipan Patra <spatra@nvidia.com> > > --- > > drivers/hwmon/pwm-fan.c | 112 > > ++++++++++++++++++++++++++++++++++++++++++------ > > 1 file changed, 100 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c index > > 30b7b3e..26db589 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); > > + > > + if (ctx->pwm_fan_profiles <= 0) { > > + dev_err(dev, "Profiles used but not defined\n"); > > + return -EINVAL; > > + } > > > > - ret = of_property_count_u32_elems(np, "cooling-levels"); > > - if (ret <= 0) { > > - dev_err(dev, "Wrong data!\n"); > > - return ret ? : -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++) { > > @@ -390,6 +458,25 @@ static int pwm_fan_probe(struct platform_device > *pdev) > > return 0; > > } > > > > +static int pwm_fan_remove(struct platform_device *pdev) { > > + struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev); > > + struct pwm_args args; > > + > > + if (!ctx) > > + return -EINVAL; > > + > > + if (IS_ENABLED(CONFIG_THERMAL)) > > + thermal_cooling_device_unregister(ctx->cdev); > > + > > + pwm_get_args(ctx->pwm, &args); > > + pwm_config(ctx->pwm, 0, args.period); > > + pwm_disable(ctx->pwm); > > What is what you really here? Is it only that the PWM stops oscillating, or is it > crucial that the output goes to its inactive level? > > (The intended semantic of pwm_disable includes that the output goes low, but > not all implementations enforce this.) > > Also please don't introduce new users of pwm_config() and pwm_disable() use > pwm_apply() instead. > > I wonder if this unregistration is "safe". When the driver is in use I'd expect that > the hwmon device doesn't go away and so the devm unregistration callback that > belongs to > devm_hwmon_device_register_with_groups() blocks. But at this time the PWM > is already stopped and so the device stops functioning earlier. > > Best regards > Uwe > Thanks for reviewing the changes. I see that pwm_fan_shutdown() which has got merged recently, can also be used for module remove functionality. May be it will need a little bit of tweak in the code. However I should have not made both multiple profiles support and fan remove functionality on same patch. For now I will prepare the current patch only to handle multiple profiles and corresponding device tree settings. Thanks & Regards, Sandipan > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
On 5/25/20 10:06 PM, Sandipan Patra wrote: > This change has 2 parts: > 1. Add support for profiles mode settings. > This allows different fan settings for trip point temp/hyst/pwm. > T194 has multiple fan-profiles support. > > 2. Add pwm-fan remove support. This is essential since the config is > tristate capable. > > Signed-off-by: Sandipan Patra <spatra@nvidia.com> > --- > drivers/hwmon/pwm-fan.c | 112 ++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 100 insertions(+), 12 deletions(-) > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c > index 30b7b3e..26db589 100644 > --- a/drivers/hwmon/pwm-fan.c > +++ b/drivers/hwmon/pwm-fan.c [ ... ] > > +static int pwm_fan_remove(struct platform_device *pdev) > +{ > + struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev); > + struct pwm_args args; > + > + if (!ctx) > + return -EINVAL; > + > + if (IS_ENABLED(CONFIG_THERMAL)) > + thermal_cooling_device_unregister(ctx->cdev); > + > + pwm_get_args(ctx->pwm, &args); > + pwm_config(ctx->pwm, 0, args.period); > + pwm_disable(ctx->pwm); > + > + return 0; > +} > + I don't think you actually tested this. I would suggest to make yourself familiar with 'devm' functions and their use, and then resubmit. Thanks, Guenter
On 5/26/20 2:05 AM, Sandipan Patra wrote: > Hi Uwe, > > > >> -----Original Message----- >> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >> Sent: Tuesday, May 26, 2020 12:35 PM >> To: Sandipan Patra <spatra@nvidia.com> >> Cc: Thierry Reding <treding@nvidia.com>; Jonathan Hunter >> <jonathanh@nvidia.com>; kamil@wypas.org; jdelvare@suse.com; linux@roeck- >> us.net; robh+dt@kernel.org; Bibek Basu <bbasu@nvidia.com>; Bitan Biswas >> <bbiswas@nvidia.com>; linux-pwm@vger.kernel.org; linux- >> hwmon@vger.kernel.org; devicetree@vger.kernel.org; linux- >> tegra@vger.kernel.org; linux-kernel@vger.kernel.org >> Subject: Re: [PATCH 1/2] hwmon: pwm-fan: Add profile support and add remove >> module support >> >> External email: Use caution opening links or attachments >> >> >> On Tue, May 26, 2020 at 10:36:04AM +0530, Sandipan Patra wrote: >>> This change has 2 parts: >>> 1. Add support for profiles mode settings. >>> This allows different fan settings for trip point temp/hyst/pwm. >>> T194 has multiple fan-profiles support. >>> >>> 2. Add pwm-fan remove support. This is essential since the config is >>> tristate capable. >> >> These two are orthogonal, aren't they? So they belong in two patches. >> >> You have to expand the binding documentation. >> >>> Signed-off-by: Sandipan Patra <spatra@nvidia.com> >>> --- >>> drivers/hwmon/pwm-fan.c | 112 >>> ++++++++++++++++++++++++++++++++++++++++++------ >>> 1 file changed, 100 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c index >>> 30b7b3e..26db589 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); >>> + >>> + if (ctx->pwm_fan_profiles <= 0) { >>> + dev_err(dev, "Profiles used but not defined\n"); >>> + return -EINVAL; >>> + } >>> >>> - ret = of_property_count_u32_elems(np, "cooling-levels"); >>> - if (ret <= 0) { >>> - dev_err(dev, "Wrong data!\n"); >>> - return ret ? : -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++) { >>> @@ -390,6 +458,25 @@ static int pwm_fan_probe(struct platform_device >> *pdev) >>> return 0; >>> } >>> >>> +static int pwm_fan_remove(struct platform_device *pdev) { >>> + struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev); >>> + struct pwm_args args; >>> + >>> + if (!ctx) >>> + return -EINVAL; >>> + >>> + if (IS_ENABLED(CONFIG_THERMAL)) >>> + thermal_cooling_device_unregister(ctx->cdev); >>> + >>> + pwm_get_args(ctx->pwm, &args); >>> + pwm_config(ctx->pwm, 0, args.period); >>> + pwm_disable(ctx->pwm); >> >> What is what you really here? Is it only that the PWM stops oscillating, or is it >> crucial that the output goes to its inactive level? >> >> (The intended semantic of pwm_disable includes that the output goes low, but >> not all implementations enforce this.) >> >> Also please don't introduce new users of pwm_config() and pwm_disable() use >> pwm_apply() instead. >> >> I wonder if this unregistration is "safe". When the driver is in use I'd expect that >> the hwmon device doesn't go away and so the devm unregistration callback that >> belongs to >> devm_hwmon_device_register_with_groups() blocks. But at this time the PWM >> is already stopped and so the device stops functioning earlier. >> >> Best regards >> Uwe >> > > Thanks for reviewing the changes. > > I see that pwm_fan_shutdown() which has got merged recently, can also be used for > module remove functionality. May be it will need a little bit of tweak in the code. > However I should have not made both multiple profiles support and fan remove functionality on > same patch. > Pointing out explicitly: ret = devm_add_action_or_reset(dev, pwm_fan_pwm_disable, ctx); cdev = devm_thermal_of_cooling_device_register(dev, ...) Guenter
Hi, > -----Original Message----- > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > Sent: Tuesday, May 26, 2020 5:12 PM > To: Sandipan Patra <spatra@nvidia.com>; Thierry Reding > <treding@nvidia.com>; Jonathan Hunter <jonathanh@nvidia.com>; u.kleine- > koenig@pengutronix.de; kamil@wypas.org; jdelvare@suse.com; > robh+dt@kernel.org > Cc: Bibek Basu <bbasu@nvidia.com>; Bitan Biswas <bbiswas@nvidia.com>; > linux-pwm@vger.kernel.org; linux-hwmon@vger.kernel.org; > devicetree@vger.kernel.org; linux-tegra@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH 1/2] hwmon: pwm-fan: Add profile support and add remove > module support > > External email: Use caution opening links or attachments > > > On 5/25/20 10:06 PM, Sandipan Patra wrote: > > This change has 2 parts: > > 1. Add support for profiles mode settings. > > This allows different fan settings for trip point temp/hyst/pwm. > > T194 has multiple fan-profiles support. > > > > 2. Add pwm-fan remove support. This is essential since the config is > > tristate capable. > > > > Signed-off-by: Sandipan Patra <spatra@nvidia.com> > > --- > > drivers/hwmon/pwm-fan.c | 112 > > ++++++++++++++++++++++++++++++++++++++++++------ > > 1 file changed, 100 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c index > > 30b7b3e..26db589 100644 > > --- a/drivers/hwmon/pwm-fan.c > > +++ b/drivers/hwmon/pwm-fan.c > > [ ... ] > > > > > +static int pwm_fan_remove(struct platform_device *pdev) { > > + struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev); > > + struct pwm_args args; > > + > > + if (!ctx) > > + return -EINVAL; > > + > > + if (IS_ENABLED(CONFIG_THERMAL)) > > + thermal_cooling_device_unregister(ctx->cdev); > > + > > + pwm_get_args(ctx->pwm, &args); > > + pwm_config(ctx->pwm, 0, args.period); > > + pwm_disable(ctx->pwm); > > + > > + return 0; > > +} > > + > > I don't think you actually tested this. I would suggest to make yourself familiar > with 'devm' functions and their use, and then resubmit. > Thanks Guenter. I missed to mention about devm while unregistering the cooling device. That would definitely cause a mistake in code. I am noting it for further patch. For a better clarity, I will push next version of this patch to handle only multiple profiles support. "remove fan module" will be supported by a separate patch altogether. Thanks & Regards, Sandipan > Thanks, > Guenter
On Tue, May 26, 2020 at 12:08:14PM +0000, Sandipan Patra wrote: > Hi, > > > > -----Original Message----- > > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > > Sent: Tuesday, May 26, 2020 5:12 PM > > To: Sandipan Patra <spatra@nvidia.com>; Thierry Reding > > <treding@nvidia.com>; Jonathan Hunter <jonathanh@nvidia.com>; u.kleine- > > koenig@pengutronix.de; kamil@wypas.org; jdelvare@suse.com; > > robh+dt@kernel.org > > Cc: Bibek Basu <bbasu@nvidia.com>; Bitan Biswas <bbiswas@nvidia.com>; > > linux-pwm@vger.kernel.org; linux-hwmon@vger.kernel.org; > > devicetree@vger.kernel.org; linux-tegra@vger.kernel.org; linux- > > kernel@vger.kernel.org > > Subject: Re: [PATCH 1/2] hwmon: pwm-fan: Add profile support and add remove > > module support > > > > External email: Use caution opening links or attachments > > > > > > On 5/25/20 10:06 PM, Sandipan Patra wrote: > > > This change has 2 parts: > > > 1. Add support for profiles mode settings. > > > This allows different fan settings for trip point temp/hyst/pwm. > > > T194 has multiple fan-profiles support. > > > > > > 2. Add pwm-fan remove support. This is essential since the config is > > > tristate capable. > > > > > > Signed-off-by: Sandipan Patra <spatra@nvidia.com> > > > --- > > > drivers/hwmon/pwm-fan.c | 112 > > > ++++++++++++++++++++++++++++++++++++++++++------ > > > 1 file changed, 100 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c index > > > 30b7b3e..26db589 100644 > > > --- a/drivers/hwmon/pwm-fan.c > > > +++ b/drivers/hwmon/pwm-fan.c > > > > [ ... ] > > > > > > > > +static int pwm_fan_remove(struct platform_device *pdev) { > > > + struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev); > > > + struct pwm_args args; > > > + > > > + if (!ctx) > > > + return -EINVAL; > > > + > > > + if (IS_ENABLED(CONFIG_THERMAL)) > > > + thermal_cooling_device_unregister(ctx->cdev); > > > + > > > + pwm_get_args(ctx->pwm, &args); > > > + pwm_config(ctx->pwm, 0, args.period); > > > + pwm_disable(ctx->pwm); > > > + > > > + return 0; > > > +} > > > + > > > > I don't think you actually tested this. I would suggest to make yourself familiar > > with 'devm' functions and their use, and then resubmit. > > > > Thanks Guenter. > I missed to mention about devm while unregistering the cooling device. > That would definitely cause a mistake in code. I am noting it for further patch. > For that part, I am extremely surprised that it is not handled by the thermal subsystem. Does each thermal driver need this kind of code ? > For a better clarity, I will push next version of this patch to handle only multiple profiles support. > "remove fan module" will be supported by a separate patch altogether. > I asked you to look into "devm" functionality. I ask you again. If you still think that a remove function is needed, I will require detailed reasoning. Please be prepared to explain why devm functions do not work for this driver. Guenter
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c index 30b7b3e..26db589 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); + + if (ctx->pwm_fan_profiles <= 0) { + dev_err(dev, "Profiles used but not defined\n"); + return -EINVAL; + } - ret = of_property_count_u32_elems(np, "cooling-levels"); - if (ret <= 0) { - dev_err(dev, "Wrong data!\n"); - return ret ? : -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++) { @@ -390,6 +458,25 @@ static int pwm_fan_probe(struct platform_device *pdev) return 0; } +static int pwm_fan_remove(struct platform_device *pdev) +{ + struct pwm_fan_ctx *ctx = platform_get_drvdata(pdev); + struct pwm_args args; + + if (!ctx) + return -EINVAL; + + if (IS_ENABLED(CONFIG_THERMAL)) + thermal_cooling_device_unregister(ctx->cdev); + + pwm_get_args(ctx->pwm, &args); + pwm_config(ctx->pwm, 0, args.period); + pwm_disable(ctx->pwm); + + return 0; +} + + static int pwm_fan_disable(struct device *dev) { struct pwm_fan_ctx *ctx = dev_get_drvdata(dev); @@ -465,6 +552,7 @@ MODULE_DEVICE_TABLE(of, of_pwm_fan_match); static struct platform_driver pwm_fan_driver = { .probe = pwm_fan_probe, + .remove = pwm_fan_remove, .shutdown = pwm_fan_shutdown, .driver = { .name = "pwm-fan",
This change has 2 parts: 1. Add support for profiles mode settings. This allows different fan settings for trip point temp/hyst/pwm. T194 has multiple fan-profiles support. 2. Add pwm-fan remove support. This is essential since the config is tristate capable. Signed-off-by: Sandipan Patra <spatra@nvidia.com> --- drivers/hwmon/pwm-fan.c | 112 ++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 100 insertions(+), 12 deletions(-)