Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Thu, 24 Feb 2022 18:10:37 -0800
From:   Guenter Roeck <linux@roeck-us.net>
To:     Armin Wolf <W_Armin@gmx.de>
Cc:     hdegoede@redhat.com, jdelvare@suse.com,
        linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] hwmon: (sch5627) Add pwmX_auto_channels_temp
 support
Message-ID: <20220225021037.GA1976317@roeck-us.net>
References: <20220224061210.16452-1-W_Armin@gmx.de>
 <20220224061210.16452-3-W_Armin@gmx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220224061210.16452-3-W_Armin@gmx.de>
Precedence: bulk

On Thu, Feb 24, 2022 at 07:12:10AM +0100, Armin Wolf wrote:
> After doing some research, it seems that Fujitsu's
> hardware monitoring solution exports data describing
> which temperature sensors affect which fans, similar
> to the data in fan_source of the ftsteutates driver.
> Writing 0 into these registers forces the fans to
> full speed.
> Export this data with standard attributes.
> 
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  Documentation/hwmon/sch5627.rst |  4 +++
>  drivers/hwmon/sch5627.c         | 61 +++++++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+)
> 
> --
> 2.30.2
> 
> diff --git a/Documentation/hwmon/sch5627.rst b/Documentation/hwmon/sch5627.rst
> index 187682e99114..ecb4fc84d045 100644
> --- a/Documentation/hwmon/sch5627.rst
> +++ b/Documentation/hwmon/sch5627.rst
> @@ -20,6 +20,10 @@ Description
>  SMSC SCH5627 Super I/O chips include complete hardware monitoring
>  capabilities. They can monitor up to 5 voltages, 4 fans and 8 temperatures.
> 
> +In addition, the SCH5627 exports data describing which temperature sensors
> +affect the speed of each fan. Setting pwmX_auto_channels_temp to 0 forces
> +the corresponding fan to full speed until another value is written.
> +
>  The SMSC SCH5627 hardware monitoring part also contains an integrated
>  watchdog. In order for this watchdog to function some motherboard specific
>  initialization most be done by the BIOS, so if the watchdog is not enabled
> diff --git a/drivers/hwmon/sch5627.c b/drivers/hwmon/sch5627.c
> index 72c3f6757e34..25fbbd4c9a2b 100644
> --- a/drivers/hwmon/sch5627.c
> +++ b/drivers/hwmon/sch5627.c
> @@ -52,6 +52,9 @@ static const u16 SCH5627_REG_FAN[SCH5627_NO_FANS] = {
>  static const u16 SCH5627_REG_FAN_MIN[SCH5627_NO_FANS] = {
>  	0x62, 0x64, 0x66, 0x68 };
> 
> +static const u16 SCH5627_REG_PWM_MAP[SCH5627_NO_FANS] = {
> +	0xA0, 0xA1, 0xA2, 0xA3 };
> +
>  static const u16 SCH5627_REG_IN_MSB[SCH5627_NO_IN] = {
>  	0x22, 0x23, 0x24, 0x25, 0x189 };
>  static const u16 SCH5627_REG_IN_LSN[SCH5627_NO_IN] = {
> @@ -223,6 +226,9 @@ static int reg_to_rpm(u16 reg)
>  static umode_t sch5627_is_visible(const void *drvdata, enum hwmon_sensor_types type, u32 attr,
>  				  int channel)
>  {
> +	if (type == hwmon_pwm && attr == hwmon_pwm_auto_channels_temp)
> +		return 0644;
> +
>  	return 0444;
>  }
> 
> @@ -278,6 +284,23 @@ static int sch5627_read(struct device *dev, enum hwmon_sensor_types type, u32 at
>  			break;
>  		}
>  		break;
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_auto_channels_temp:
> +			mutex_lock(&data->update_lock);
> +			ret = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_PWM_MAP[channel]);
> +			mutex_unlock(&data->update_lock);
> +
> +			if (ret < 0)
> +				return ret;
> +
> +			*val = ret;
> +
> +			return 0;
> +		default:
> +			break;
> +		}
> +		break;
>  	case hwmon_in:
>  		ret = sch5627_update_in(data);
>  		if (ret < 0)
> @@ -318,10 +341,42 @@ static int sch5627_read_string(struct device *dev, enum hwmon_sensor_types type,
>  	return -EOPNOTSUPP;
>  }
> 
> +static int sch5627_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
> +			 long val)
> +{
> +	struct sch5627_data *data = dev_get_drvdata(dev);
> +	int ret;
> +
> +	switch (type) {
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_auto_channels_temp:
> +			/* registers are 8 bit wide */
> +			if (val > U8_MAX || val < 0)
> +				return -EINVAL;
> +
> +			mutex_lock(&data->update_lock);
> +			ret = sch56xx_write_virtual_reg(data->addr, SCH5627_REG_PWM_MAP[channel],
> +							val);
> +			mutex_unlock(&data->update_lock);
> +
> +			return ret;
> +		default:
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
>  static const struct hwmon_ops sch5627_ops = {
>  	.is_visible = sch5627_is_visible,
>  	.read = sch5627_read,
>  	.read_string = sch5627_read_string,
> +	.write = sch5627_write,
>  };
> 
>  static const struct hwmon_channel_info *sch5627_info[] = {
> @@ -342,6 +397,12 @@ static const struct hwmon_channel_info *sch5627_info[] = {
>  			   HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_FAULT,
>  			   HWMON_F_INPUT | HWMON_F_MIN | HWMON_F_FAULT
>  			   ),
> +	HWMON_CHANNEL_INFO(pwm,
> +			   HWMON_PWM_AUTO_CHANNELS_TEMP,
> +			   HWMON_PWM_AUTO_CHANNELS_TEMP,
> +			   HWMON_PWM_AUTO_CHANNELS_TEMP,
> +			   HWMON_PWM_AUTO_CHANNELS_TEMP
> +			   ),
>  	HWMON_CHANNEL_INFO(in,
>  			   HWMON_I_INPUT | HWMON_I_LABEL,
>  			   HWMON_I_INPUT | HWMON_I_LABEL,