Re: [PATCH v1 hwmon-next] hwmon: (mlxreg-fan) Add support for fan capability registers

Guenter Roeck <linux@xxxxxxxxxxxx> · Mon, 18 Mar 2019 07:37:11 -0700

On 3/18/19 7:31 AM, Vadim Pasternak wrote:


-----Original Message-----
From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
Sent: Monday, March 18, 2019 4:18 PM
To: Vadim Pasternak <vadimp@xxxxxxxxxxxx>
Cc: linux-hwmon@xxxxxxxxxxxxxxx
Subject: Re: [PATCH v1 hwmon-next] hwmon: (mlxreg-fan) Add support for fan
capability registers

On 3/18/19 3:53 AM, Vadim Pasternak wrote:
Add support for fan capability registers in order to distinct between
the systems which have minor fan configuration differences. This
reduces the amount of code used to describe such systems.
The capability registers provides system specific information about
the number of physically connected tachometers and system specific fan
speed scale parameter.
For example one system can be equipped with twelve fan tachometers,
while the other with for example, eight or six. Or one system should
use default fan speed divider value, while the other has a scale
parameter defined in hardware, which should be used for divider
setting.
Reading this information from the capability registers allows to use
the same fan structure for the systems with the such differences.

Signed-off-by: Vadim Pasternak <vadimp@xxxxxxxxxxxx>
---
   drivers/hwmon/mlxreg-fan.c | 78
+++++++++++++++++++++++++++++++++++++++++++---
   1 file changed, 73 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/mlxreg-fan.c b/drivers/hwmon/mlxreg-fan.c
index db8c6de0b6a0..5a4d5348516a 100644
--- a/drivers/hwmon/mlxreg-fan.c
+++ b/drivers/hwmon/mlxreg-fan.c
@@ -27,7 +27,10 @@
   #define MLXREG_FAN_SPEED_MAX
	(MLXREG_FAN_MAX_STATE * 2)
   #define MLXREG_FAN_SPEED_MIN_LEVEL		2	/* 20 percent
*/
   #define MLXREG_FAN_TACHO_SAMPLES_PER_PULSE_DEF	44
-#define MLXREG_FAN_TACHO_DIVIDER_DEF		1132
+#define MLXREG_FAN_TACHO_DIVIDER_MIN		283
+#define MLXREG_FAN_TACHO_DIVIDER_DEF
	(MLXREG_FAN_TACHO_DIVIDER_MIN \
+						 * 4)

This is where the unnecessary MLXREG_FAN prefix really starts to hurt.

Hi Guenter,

Thank you for quick review.
I just wanted to keep all the names with the same prefix.
Maybe it would be better to keep this convention and just make
names shorter by replacing
s/DIVIDER/DIV ?

#define MLXREG_FAN_TACHO_DIV_MIN		283
#define MLXREG_FAN_TACHO_DIV_DEF		(MLXREG_FAN_TACHO_DIV_MIN * 4)
#define MLXREG_FAN_TACHO_DIV_SCALE_MAX		64


Sure, that would work. Just something to consider for future drivers - a prefix
of, say, MLX instead of MLXREG_FAN would have been sufficient. I am not suggesting
to change that now, though.

Guenter


+#define MLXREG_FAN_TACHO_DIVIDER_SCALE_MAX	64
   /*
    * FAN datasheet defines the formula for RPM calculations as RPM = 15/t-
high.
    * The logic in a programmable device measures the time t-high by
sampling the @@ -360,12 +363,57 @@ static const struct
thermal_cooling_device_ops mlxreg_fan_cooling_ops = {
   	.set_cur_state	= mlxreg_fan_set_cur_state,
   };

+static int mlxreg_fan_connect_verify(struct mlxreg_fan *fan,
+				     struct mlxreg_core_data *data,
+				     bool *connected)
+{
+	u32 regval;
+	int err;
+
+	err = regmap_read(fan->regmap, data->capability, &regval);
+	if (err) {
+		dev_err(fan->dev, "Failed to query capability register
0x%08x\n",
+			data->capability);
+		return err;
+	}
+
+	*connected = (regval & data->bit) ? true : false;
+
+	return 0;
+}

This function could be simplified by returning an int, negative for error or 0/1.
	return !!(regval & data->bit);
Even if not,
	*connected = regval & data->bit;
would do or, if you want to be explicit,
	*connected = !!(regval & data->bit);

+
+static int mlxreg_fan_speed_divider_get(struct mlxreg_fan *fan,
+					struct mlxreg_core_data *data)
+{
+	u32 regval;
+	int err;
+
+	err = regmap_read(fan->regmap, data->capability, &regval);
+	if (err) {
+		dev_err(fan->dev, "Failed to query capability register
0x%08x\n",
+			data->capability);
+		return err;
+	}
+
+	/*
+	 * Set divider value according to the capability register, in case it
+	 * contains valid value. Otherwise use default value. The purpose of
+	 * this validation is to protect against the old hardware, in which
+	 * this register can be un-initialized.

un-initialized -> random ? "can return zero" might be a better description if that
is what it is.


+	 */
+	if (regval > 0 && regval <= MLXREG_FAN_TACHO_DIVIDER_SCALE_MAX)
+		fan->divider = regval * MLXREG_FAN_TACHO_DIVIDER_MIN;
+
+	return 0;
+}
+
   static int mlxreg_fan_config(struct mlxreg_fan *fan,
   			     struct mlxreg_core_platform_data *pdata)
   {
   	struct mlxreg_core_data *data = pdata->data;
-	bool configured = false;
+	bool configured = false, connected = false;
   	int tacho_num = 0, i;
+	int err;

   	fan->samples = MLXREG_FAN_TACHO_SAMPLES_PER_PULSE_DEF;
   	fan->divider = MLXREG_FAN_TACHO_DIVIDER_DEF; @@ -376,6
+424,18 @@
static int mlxreg_fan_config(struct mlxreg_fan *fan,
   					data->label);
   				return -EINVAL;
   			}
+
+			if (data->capability) {
+				err = mlxreg_fan_connect_verify(fan, data,
+								&connected);
+				if (err)
+					return err;
+				if (!connected) {
+					tacho_num++;
+					continue;
+				}
+			}
+
   			fan->tacho[tacho_num].reg = data->reg;
   			fan->tacho[tacho_num].mask = data->mask;
   			fan->tacho[tacho_num++].connected = true; @@ -
394,13 +454,21 @@
static int mlxreg_fan_config(struct mlxreg_fan *fan,
   				return -EINVAL;
   			}
   			/* Validate that conf parameters are not zeros. */
-			if (!data->mask || !data->bit) {
+			if (!data->mask && !data->bit && !data->capability) {
   				dev_err(fan->dev, "invalid conf entry params:
%s\n",
   					data->label);
   				return -EINVAL;
   			} > -			fan->samples = data->mask;
-			fan->divider = data->bit;
+			if (data->capability) {
+				err = mlxreg_fan_speed_divider_get(fan, data);
+				if (err)
+					return err;
+			} else {
+				if (data->mask)
+					fan->samples = data->mask;
+				if (data->bit)
+					fan->divider = data->bit;
+			}
   			configured = true;
   		} else {
   			dev_err(fan->dev, "invalid label: %s\n", data->label);









