Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Thu, 16 Nov 2023 00:12:39 -0800
From:   Guenter Roeck <linux@roeck-us.net>
To:     Xing Tong Wu <xingtong_wu@163.com>
Cc:     Jean Delvare <jdelvare@suse.com>, linux-hwmon@vger.kernel.org,
        linux-kernel@vger.kernel.org, xingtong.wu@siemens.com,
        tobias.schaffner@siemens.com, gerd.haeussler.ext@siemens.com
Subject: Re: [PATCH 3/3] hwmon: (nct6775) Fix fan speed set failure in
 automatic mode
Message-ID: <949b2ae6-d631-449a-958c-a7d97fed57f6@roeck-us.net>
References: <20231116022330.2696-1-xingtong_wu@163.com>
 <20231116022330.2696-4-xingtong_wu@163.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231116022330.2696-4-xingtong_wu@163.com>
Precedence: bulk

On Thu, Nov 16, 2023 at 10:23:30AM +0800, Xing Tong Wu wrote:
> From: Xing Tong Wu <xingtong.wu@siemens.com>
> 
> Setting the fan speed is only valid in manual mode; it is not possible
> to set the fan's speed in automatic mode.
> Return error and show error message when attempting to set the fan
> speed in automatic mode.
> 
> Signed-off-by: Xing Tong Wu <xingtong.wu@siemens.com>
> ---
>  drivers/hwmon/nct6775-core.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/hwmon/nct6775-core.c b/drivers/hwmon/nct6775-core.c
> index 575db6cb96e9..3fb9896c61ce 100644
> --- a/drivers/hwmon/nct6775-core.c
> +++ b/drivers/hwmon/nct6775-core.c
> @@ -2551,6 +2551,14 @@ store_pwm(struct device *dev, struct device_attribute *attr, const char *buf,
>  	int err;
>  	u16 reg;
>  
> +	if (index == 0 && data->pwm_enable[nr] != manual) {
> +		dev_err(dev,
> +			"The pwm%d doesn't support manual fan speed control in automatic mode.\n",
> +			nr + 1);
> +		dev_err(dev, "Please set pwm%d_enable to manual mode.\n", nr + 1);

No kernel log messages as reponse to user input, please. This
could and likely would clog the kernel log if, for example, some automated
script tries to write into the register.

Please add a comment describing why this is blocked.

> +		return -EOPNOTSUPP;

Wrong error code. I would suggest -EBUSY; that is what is used in the it87
driver.

Guenter

> +	}
> +
>  	err = kstrtoul(buf, 10, &val);
>  	if (err < 0)
>  		return err;
> -- 
> 2.25.1
>