| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <BYAPR12MB30148BCF84846172616AF0C9AD670@BYAPR12MB3014.namprd12.prod.outlook.com>
Date: Wed, 8 Jul 2020 11:27:43 +0000
From: Sandipan Patra <spatra@...dia.com>
To: Guenter Roeck <linux@...ck-us.net>
CC: Thierry Reding <treding@...dia.com>,
Jonathan Hunter <jonathanh@...dia.com>,
"kamil@...as.org" <kamil@...as.org>,
"jdelvare@...e.com" <jdelvare@...e.com>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"u.kleine-koenig@...gutronix.de" <u.kleine-koenig@...gutronix.de>,
Bibek Basu <bbasu@...dia.com>,
Bitan Biswas <bbiswas@...dia.com>,
Krishna Yarlagadda <kyarlagadda@...dia.com>,
"linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
"linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH V2 1/2] hwmon: pwm-fan: Add profile support and add remove
module support
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@...il.com> On Behalf Of Guenter Roeck
> Sent: Tuesday, June 2, 2020 7:58 PM
> To: Sandipan Patra <spatra@...dia.com>
> Cc: Thierry Reding <treding@...dia.com>; Jonathan Hunter
> <jonathanh@...dia.com>; kamil@...as.org; jdelvare@...e.com;
> robh+dt@...nel.org; u.kleine-koenig@...gutronix.de; Bibek Basu
> <bbasu@...dia.com>; Bitan Biswas <bbiswas@...dia.com>; Krishna Yarlagadda
> <kyarlagadda@...dia.com>; linux-pwm@...r.kernel.org; linux-
> hwmon@...r.kernel.org; devicetree@...r.kernel.org; linux-
> tegra@...r.kernel.org; linux-kernel@...r.kernel.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@...dia.com>
>
> 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@...sung.com>
> > + * Author: Sandipan Patra <spatra@...dia.com>
>
> 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++) {
Powered by blists - more mailing lists