Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Date:   Thu, 22 Jun 2023 10:42:31 -0700
From:   Guenter Roeck <linux@roeck-us.net>
To:     Wilken Gottwalt <wilken.gottwalt@posteo.net>
Cc:     linux-kernel@vger.kernel.org, Jean Delvare <jdelvare@suse.com>,
        Jonathan Corbet <corbet@lwn.net>, linux-hwmon@vger.kernel.org
Subject: Re: [PATCH v2] hwmon: corsair-psu: add support for reading PWM
 values and mode
Message-ID: <0eeef6b2-9c9c-4b1a-8538-eb8ef5b1d628@roeck-us.net>
References: <ZJSASByXpzoZ0XyH@monster.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZJSASByXpzoZ0XyH@monster.localdomain>
Precedence: bulk

On Thu, Jun 22, 2023 at 05:09:28PM +0000, Wilken Gottwalt wrote:
> Also updates the documentation accordingly.
> 
> Signed-off-by: Wilken Gottwalt <wilken.gottwalt@posteo.net>
> ---
> Changes in v2:
>   - removed cleanup and typo fixes, sticking to feature adding only
> ---

Applied to hwmon-next.

Note: The patch description should describe what the patch does,
which isn't just "Also updates the documentation accordingly."
I'll fix that up, but please keep in mind for later.

Thanks,
Guenter

>  Documentation/hwmon/corsair-psu.rst |  2 +
>  drivers/hwmon/corsair-psu.c         | 62 ++++++++++++++++++++++++++++-
>  2 files changed, 63 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/hwmon/corsair-psu.rst b/Documentation/hwmon/corsair-psu.rst
> index c389bd21f4f2..fc798c3df1d0 100644
> --- a/Documentation/hwmon/corsair-psu.rst
> +++ b/Documentation/hwmon/corsair-psu.rst
> @@ -69,6 +69,8 @@ power1_input		Total power usage
>  power2_input		Power usage of the 12v psu rail
>  power3_input		Power usage of the 5v psu rail
>  power4_input		Power usage of the 3.3v psu rail
> +pwm1			PWM value, read only
> +pwm1_enable		PWM mode, read only
>  temp1_input		Temperature of the psu vrm component
>  temp1_crit		Temperature max cirtical value of the psu vrm component
>  temp2_input		Temperature of the psu case
> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
> index dc24c566d08b..2389f605ca16 100644
> --- a/drivers/hwmon/corsair-psu.c
> +++ b/drivers/hwmon/corsair-psu.c
> @@ -58,7 +58,8 @@
>  #define OCP_MULTI_RAIL		0x02
>  
>  #define PSU_CMD_SELECT_RAIL	0x00 /* expects length 2 */
> -#define PSU_CMD_RAIL_VOLTS_HCRIT 0x40 /* the rest of the commands expect length 3 */
> +#define PSU_CMD_FAN_PWM		0x3B /* the rest of the commands expect length 3 */
> +#define PSU_CMD_RAIL_VOLTS_HCRIT 0x40
>  #define PSU_CMD_RAIL_VOLTS_LCRIT 0x44
>  #define PSU_CMD_RAIL_AMPS_HCRIT	0x46
>  #define PSU_CMD_TEMP_HCRIT	0x4F
> @@ -76,6 +77,7 @@
>  #define PSU_CMD_UPTIME		0xD2
>  #define PSU_CMD_OCPMODE		0xD8
>  #define PSU_CMD_TOTAL_WATTS	0xEE
> +#define PSU_CMD_FAN_PWM_ENABLE	0xF0
>  #define PSU_CMD_INIT		0xFE
>  
>  #define L_IN_VOLTS		"v_in"
> @@ -145,6 +147,14 @@ static int corsairpsu_linear11_to_int(const u16 val, const int scale)
>  	return (exp >= 0) ? (result << exp) : (result >> -exp);
>  }
>  
> +/* the micro-controller uses percentage values to control pwm */
> +static int corsairpsu_dutycycle_to_pwm(const long dutycycle)
> +{
> +	const int result = (256 << 16) / 100;
> +
> +	return (result * dutycycle) >> 16;
> +}
> +
>  static int corsairpsu_usb_cmd(struct corsairpsu_data *priv, u8 p0, u8 p1, u8 p2, void *data)
>  {
>  	unsigned long time;
> @@ -264,6 +274,24 @@ static int corsairpsu_get_value(struct corsairpsu_data *priv, u8 cmd, u8 rail, l
>  	case PSU_CMD_FAN:
>  		*val = corsairpsu_linear11_to_int(tmp & 0xFFFF, 1);
>  		break;
> +	case PSU_CMD_FAN_PWM_ENABLE:
> +		*val = corsairpsu_linear11_to_int(tmp & 0xFFFF, 1);
> +		/*
> +		 * 0 = automatic mode, means the micro-controller controls the fan using a plan
> +		 *     which can be modified, but changing this plan is not supported by this
> +		 *     driver, the matching PWM mode is automatic fan speed control = PWM 2
> +		 * 1 = fixed mode, fan runs at a fixed speed represented by a percentage
> +		 *     value 0-100, this matches the PWM manual fan speed control = PWM 1
> +		 * technically there is no PWM no fan speed control mode, it would be a combination
> +		 * of 1 at 100%
> +		 */
> +		if (*val == 0)
> +			*val = 2;
> +		break;
> +	case PSU_CMD_FAN_PWM:
> +		*val = corsairpsu_linear11_to_int(tmp & 0xFFFF, 1);
> +		*val = corsairpsu_dutycycle_to_pwm(*val);
> +		break;
>  	case PSU_CMD_RAIL_WATTS:
>  	case PSU_CMD_TOTAL_WATTS:
>  		*val = corsairpsu_linear11_to_int(tmp & 0xFFFF, 1000000);
> @@ -349,6 +377,18 @@ static umode_t corsairpsu_hwmon_fan_is_visible(const struct corsairpsu_data *pri
>  	}
>  }
>  
> +static umode_t corsairpsu_hwmon_pwm_is_visible(const struct corsairpsu_data *priv, u32 attr,
> +					       int channel)
> +{
> +	switch (attr) {
> +	case hwmon_pwm_input:
> +	case hwmon_pwm_enable:
> +		return 0444;
> +	default:
> +		return 0;
> +	}
> +}
> +
>  static umode_t corsairpsu_hwmon_power_is_visible(const struct corsairpsu_data *priv, u32 attr,
>  						 int channel)
>  {
> @@ -416,6 +456,8 @@ static umode_t corsairpsu_hwmon_ops_is_visible(const void *data, enum hwmon_sens
>  		return corsairpsu_hwmon_temp_is_visible(priv, attr, channel);
>  	case hwmon_fan:
>  		return corsairpsu_hwmon_fan_is_visible(priv, attr, channel);
> +	case hwmon_pwm:
> +		return corsairpsu_hwmon_pwm_is_visible(priv, attr, channel);
>  	case hwmon_power:
>  		return corsairpsu_hwmon_power_is_visible(priv, attr, channel);
>  	case hwmon_in:
> @@ -447,6 +489,20 @@ static int corsairpsu_hwmon_temp_read(struct corsairpsu_data *priv, u32 attr, in
>  	return err;
>  }
>  
> +static int corsairpsu_hwmon_pwm_read(struct corsairpsu_data *priv, u32 attr, int channel, long *val)
> +{
> +	switch (attr) {
> +	case hwmon_pwm_input:
> +		return corsairpsu_get_value(priv, PSU_CMD_FAN_PWM, 0, val);
> +	case hwmon_pwm_enable:
> +		return corsairpsu_get_value(priv, PSU_CMD_FAN_PWM_ENABLE, 0, val);
> +	default:
> +		break;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
>  static int corsairpsu_hwmon_power_read(struct corsairpsu_data *priv, u32 attr, int channel,
>  				       long *val)
>  {
> @@ -531,6 +587,8 @@ static int corsairpsu_hwmon_ops_read(struct device *dev, enum hwmon_sensor_types
>  		if (attr == hwmon_fan_input)
>  			return corsairpsu_get_value(priv, PSU_CMD_FAN, 0, val);
>  		return -EOPNOTSUPP;
> +	case hwmon_pwm:
> +		return corsairpsu_hwmon_pwm_read(priv, attr, channel, val);
>  	case hwmon_power:
>  		return corsairpsu_hwmon_power_read(priv, attr, channel, val);
>  	case hwmon_in:
> @@ -579,6 +637,8 @@ static const struct hwmon_channel_info * const corsairpsu_info[] = {
>  			   HWMON_T_INPUT | HWMON_T_LABEL | HWMON_T_CRIT),
>  	HWMON_CHANNEL_INFO(fan,
>  			   HWMON_F_INPUT | HWMON_F_LABEL),
> +	HWMON_CHANNEL_INFO(pwm,
> +			   HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
>  	HWMON_CHANNEL_INFO(power,
>  			   HWMON_P_INPUT | HWMON_P_LABEL,
>  			   HWMON_P_INPUT | HWMON_P_LABEL,