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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----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

> 
> > +#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);
> >





[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux