| Message ID | 20201208092931.1074-1-vadimp@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [hwmon-next,1/1] hwmon: (mlxreg-fan) Add support for fan drawers capability and present registers | expand |
On Tue, Dec 08, 2020 at 11:29:31AM +0200, Vadim Pasternak wrote: > Add support for fan drawer's capability and present registers in order > to set mapping between the fan drawers and tachometers. Some systems > are equipped with fan drawers with one tachometer inside. Others with > fan drawers with several tachometers inside. Using present register > along with tachometer-to-drawer mapping allows to skip reading missed > tachometers and expose input for them as zero, instead of exposing > fault code returned by hardware. > > Signed-off-by: Vadim Pasternak <vadimp@nvidia.com> > --- > drivers/hwmon/mlxreg-fan.c | 46 +++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 45 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwmon/mlxreg-fan.c b/drivers/hwmon/mlxreg-fan.c > index ed8d59d4eecb..ab743929a6cd 100644 > --- a/drivers/hwmon/mlxreg-fan.c > +++ b/drivers/hwmon/mlxreg-fan.c > @@ -67,11 +67,13 @@ > * @connected: indicates if tachometer is connected; > * @reg: register offset; > * @mask: fault mask; > + * @prsnt: present register offset; > */ > struct mlxreg_fan_tacho { > bool connected; > u32 reg; > u32 mask; > + u32 prsnt; > }; > > /* > @@ -92,6 +94,7 @@ struct mlxreg_fan_pwm { > * @regmap: register map of parent device; > * @tacho: tachometer data; > * @pwm: PWM data; > + * @tachos_per_drwr - number of tachometers per drawer; > * @samples: minimum allowed samples per pulse; > * @divider: divider value for tachometer RPM calculation; > * @cooling: cooling device levels; > @@ -103,6 +106,7 @@ struct mlxreg_fan { > struct mlxreg_core_platform_data *pdata; > struct mlxreg_fan_tacho tacho[MLXREG_FAN_MAX_TACHO]; > struct mlxreg_fan_pwm pwm; > + int tachos_per_drwr; > int samples; > int divider; > u8 cooling_levels[MLXREG_FAN_MAX_STATE + 1]; > @@ -123,6 +127,26 @@ mlxreg_fan_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, > tacho = &fan->tacho[channel]; > switch (attr) { > case hwmon_fan_input: > + /* > + * Check FAN presence: FAN related bit in presence register is one, > + * if FAN is not physically connected, zero - otherwise. > + */ > + if (tacho->prsnt) { > + err = regmap_read(fan->regmap, tacho->prsnt, ®val); > + if (err) > + return err; > + > + /* > + * Map channel to presence bit - drawer can be equipped with > + * one or few FANs, while presence is indicated per drawer. > + */ > + if ((BIT(channel / fan->tachos_per_drwr) & regval)) { The outer double (( )) is pointless. > + /* FAN is not connected - return zero for FAN speed. */ > + *val = 0; > + return 0; > + } Assuming fan configuration is static, it might make more sense to disable the attribute in the is_visible function instead of checking the presence condition over and over again. > + } > + > err = regmap_read(fan->regmap, tacho->reg, ®val); > if (err) > return err; > @@ -388,9 +412,11 @@ static int mlxreg_fan_speed_divider_get(struct mlxreg_fan *fan, > static int mlxreg_fan_config(struct mlxreg_fan *fan, > struct mlxreg_core_platform_data *pdata) > { > + int tacho_num = 0, regval, regsize, drwr_avail = 0, tacho_avail = 0, i; > struct mlxreg_core_data *data = pdata->data; > bool configured = false; > - int tacho_num = 0, i; > + unsigned long drwrs; > + u32 bit; > int err; > > fan->samples = MLXREG_FAN_TACHO_SAMPLES_PER_PULSE_DEF; > @@ -415,7 +441,9 @@ static int mlxreg_fan_config(struct mlxreg_fan *fan, > > fan->tacho[tacho_num].reg = data->reg; > fan->tacho[tacho_num].mask = data->mask; > + fan->tacho[tacho_num].prsnt = data->reg_prsnt; > fan->tacho[tacho_num++].connected = true; > + tacho_avail++; > } else if (strnstr(data->label, "pwm", sizeof(data->label))) { > if (fan->pwm.connected) { > dev_err(fan->dev, "duplicate pwm entry: %s\n", > @@ -453,6 +481,22 @@ static int mlxreg_fan_config(struct mlxreg_fan *fan, > } > } > > + if (pdata->capability) { > + /* Obtain the number of FAN drawers, supported by system. */ > + err = regmap_read(fan->regmap, pdata->capability, ®val); > + if (err) { > + dev_err(fan->dev, "Failed to query capability register 0x%08x\n", > + pdata->capability); > + return err; > + } > + regsize = regmap_get_val_bytes(fan->regmap); > + drwrs = regval; > + for_each_set_bit(bit, &drwrs, 8 * regsize) > + drwr_avail++; Why not just hweight_long() or hweight32() ? And what is the point of the extra variable ? It is also odd that regval is an int while regmap_read() takes a pointer to an unsigned int as parameter. So the returned value is converted to int and then to unsigned long. Yes, I understand this only takes bits into account which are reported by regmap_get_val_bytes, but regmap_read already takes that into account and won't set bits larger than indicated by val_bytes. So all I can see is a lot of complexity with no gain. > + /* Set the number of tachometers per one drawer. */ > + fan->tachos_per_drwr = tacho_avail / drwr_avail; What if no bit is set, ie drwr_avail == 0 ? Also, what guarantees that tachos_per_drwr is not 0 ? > + } > + > /* Init cooling levels per PWM state. */ > for (i = 0; i < MLXREG_FAN_SPEED_MIN_LEVEL; i++) > fan->cooling_levels[i] = MLXREG_FAN_SPEED_MIN_LEVEL; > -- > 2.11.0 >
> -----Original Message----- > From: Guenter Roeck <linux@roeck-us.net> > Sent: Tuesday, December 08, 2020 9:50 PM > To: Vadim Pasternak <vadimp@nvidia.com> > Cc: linux-hwmon@vger.kernel.org > Subject: Re: [PATCH hwmon-next 1/1] hwmon: (mlxreg-fan) Add support for > fan drawers capability and present registers > > On Tue, Dec 08, 2020 at 11:29:31AM +0200, Vadim Pasternak wrote: > > Add support for fan drawer's capability and present registers in order > > to set mapping between the fan drawers and tachometers. Some systems > > are equipped with fan drawers with one tachometer inside. Others with > > fan drawers with several tachometers inside. Using present register > > along with tachometer-to-drawer mapping allows to skip reading missed > > tachometers and expose input for them as zero, instead of exposing > > fault code returned by hardware. > > > > Signed-off-by: Vadim Pasternak <vadimp@nvidia.com> > > --- > > drivers/hwmon/mlxreg-fan.c | 46 > > +++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 45 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/hwmon/mlxreg-fan.c b/drivers/hwmon/mlxreg-fan.c > > index ed8d59d4eecb..ab743929a6cd 100644 > > --- a/drivers/hwmon/mlxreg-fan.c > > +++ b/drivers/hwmon/mlxreg-fan.c > > @@ -67,11 +67,13 @@ > > * @connected: indicates if tachometer is connected; > > * @reg: register offset; > > * @mask: fault mask; > > + * @prsnt: present register offset; > > */ > > struct mlxreg_fan_tacho { > > bool connected; > > u32 reg; > > u32 mask; > > + u32 prsnt; > > }; > > > > /* > > @@ -92,6 +94,7 @@ struct mlxreg_fan_pwm { > > * @regmap: register map of parent device; > > * @tacho: tachometer data; > > * @pwm: PWM data; > > + * @tachos_per_drwr - number of tachometers per drawer; > > * @samples: minimum allowed samples per pulse; > > * @divider: divider value for tachometer RPM calculation; > > * @cooling: cooling device levels; > > @@ -103,6 +106,7 @@ struct mlxreg_fan { > > struct mlxreg_core_platform_data *pdata; > > struct mlxreg_fan_tacho tacho[MLXREG_FAN_MAX_TACHO]; > > struct mlxreg_fan_pwm pwm; > > + int tachos_per_drwr; > > int samples; > > int divider; > > u8 cooling_levels[MLXREG_FAN_MAX_STATE + 1]; @@ -123,6 +127,26 > @@ > > mlxreg_fan_read(struct device *dev, enum hwmon_sensor_types type, u32 > attr, > > tacho = &fan->tacho[channel]; > > switch (attr) { > > case hwmon_fan_input: > > + /* > > + * Check FAN presence: FAN related bit in presence > register is one, > > + * if FAN is not physically connected, zero - otherwise. > > + */ > > + if (tacho->prsnt) { > > + err = regmap_read(fan->regmap, tacho->prsnt, > ®val); > > + if (err) > > + return err; > > + > > + /* > > + * Map channel to presence bit - drawer can > be equipped with > > + * one or few FANs, while presence is > indicated per drawer. > > + */ > > + if ((BIT(channel / fan->tachos_per_drwr) & > regval)) { > > The outer double (( )) is pointless. > > > + /* FAN is not connected - return zero > for FAN speed. */ > > + *val = 0; > > + return 0; > > + } > > Assuming fan configuration is static, it might make more sense to disable the > attribute in the is_visible function instead of checking the presence condition > over and over again. This is not static: ' tacho->prsnt' is present register. Presence bit 'n' indicates if replaceable FAN drawer{n} is in or out. For example, for system, which can be equipped with 6 drawers, presence register will be zero in case all of them are inserted, and will dynamically change to f.e. to 0x03, in case drawers 1 and 2 are removed. > > > + } > > + > > err = regmap_read(fan->regmap, tacho->reg, ®val); > > if (err) > > return err; > > @@ -388,9 +412,11 @@ static int mlxreg_fan_speed_divider_get(struct > > mlxreg_fan *fan, static int mlxreg_fan_config(struct mlxreg_fan *fan, > > struct mlxreg_core_platform_data *pdata) { > > + int tacho_num = 0, regval, regsize, drwr_avail = 0, tacho_avail = 0, > > +i; > > struct mlxreg_core_data *data = pdata->data; > > bool configured = false; > > - int tacho_num = 0, i; > > + unsigned long drwrs; > > + u32 bit; > > int err; > > > > fan->samples = MLXREG_FAN_TACHO_SAMPLES_PER_PULSE_DEF; > > @@ -415,7 +441,9 @@ static int mlxreg_fan_config(struct mlxreg_fan > > *fan, > > > > fan->tacho[tacho_num].reg = data->reg; > > fan->tacho[tacho_num].mask = data->mask; > > + fan->tacho[tacho_num].prsnt = data->reg_prsnt; > > fan->tacho[tacho_num++].connected = true; > > + tacho_avail++; > > } else if (strnstr(data->label, "pwm", sizeof(data->label))) { > > if (fan->pwm.connected) { > > dev_err(fan->dev, "duplicate pwm entry: > %s\n", @@ -453,6 +481,22 > > @@ static int mlxreg_fan_config(struct mlxreg_fan *fan, > > } > > } > > > > + if (pdata->capability) { > > + /* Obtain the number of FAN drawers, supported by system. > */ > > + err = regmap_read(fan->regmap, pdata->capability, ®val); > > + if (err) { > > + dev_err(fan->dev, "Failed to query capability register > 0x%08x\n", > > + pdata->capability); > > + return err; > > + } > > + regsize = regmap_get_val_bytes(fan->regmap); > > + drwrs = regval; > > + for_each_set_bit(bit, &drwrs, 8 * regsize) > > + drwr_avail++; > > Why not just hweight_long() or hweight32() ? And what is the point of the > extra variable ? It is also odd that regval is an int while > regmap_read() takes a pointer to an unsigned int as parameter. So the > returned value is converted to int and then to unsigned long. > > Yes, I understand this only takes bits into account which are reported by > regmap_get_val_bytes, but regmap_read already takes that into account and > won't set bits larger than indicated by val_bytes. So all I can see is a lot of > complexity with no gain. I see. I'll replace four above lines with just: drwr_avail = hweight32(regval); > > > + /* Set the number of tachometers per one drawer. */ > > + fan->tachos_per_drwr = tacho_avail / drwr_avail; > > What if no bit is set, ie drwr_avail == 0 ? > > Also, what guarantees that tachos_per_drwr is not 0 ? It can't be zero, otherwise it's wrong configuration in registers. I will add check for both 'tacho_avail' and 'drwr_avail' for non-zero and return, something like: if (!tacho_avail || !drwr_avail) { dev_err(fan->dev, "Configuration is invalid: drawers num %d tachos num %d\n", drwr_avail, tacho_avail); return -EINVAL; } > > > + } > > + > > /* Init cooling levels per PWM state. */ > > for (i = 0; i < MLXREG_FAN_SPEED_MIN_LEVEL; i++) > > fan->cooling_levels[i] = MLXREG_FAN_SPEED_MIN_LEVEL; > > -- > > 2.11.0 > >
diff --git a/drivers/hwmon/mlxreg-fan.c b/drivers/hwmon/mlxreg-fan.c index ed8d59d4eecb..ab743929a6cd 100644 --- a/drivers/hwmon/mlxreg-fan.c +++ b/drivers/hwmon/mlxreg-fan.c @@ -67,11 +67,13 @@ * @connected: indicates if tachometer is connected; * @reg: register offset; * @mask: fault mask; + * @prsnt: present register offset; */ struct mlxreg_fan_tacho { bool connected; u32 reg; u32 mask; + u32 prsnt; }; /* @@ -92,6 +94,7 @@ struct mlxreg_fan_pwm { * @regmap: register map of parent device; * @tacho: tachometer data; * @pwm: PWM data; + * @tachos_per_drwr - number of tachometers per drawer; * @samples: minimum allowed samples per pulse; * @divider: divider value for tachometer RPM calculation; * @cooling: cooling device levels; @@ -103,6 +106,7 @@ struct mlxreg_fan { struct mlxreg_core_platform_data *pdata; struct mlxreg_fan_tacho tacho[MLXREG_FAN_MAX_TACHO]; struct mlxreg_fan_pwm pwm; + int tachos_per_drwr; int samples; int divider; u8 cooling_levels[MLXREG_FAN_MAX_STATE + 1]; @@ -123,6 +127,26 @@ mlxreg_fan_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, tacho = &fan->tacho[channel]; switch (attr) { case hwmon_fan_input: + /* + * Check FAN presence: FAN related bit in presence register is one, + * if FAN is not physically connected, zero - otherwise. + */ + if (tacho->prsnt) { + err = regmap_read(fan->regmap, tacho->prsnt, ®val); + if (err) + return err; + + /* + * Map channel to presence bit - drawer can be equipped with + * one or few FANs, while presence is indicated per drawer. + */ + if ((BIT(channel / fan->tachos_per_drwr) & regval)) { + /* FAN is not connected - return zero for FAN speed. */ + *val = 0; + return 0; + } + } + err = regmap_read(fan->regmap, tacho->reg, ®val); if (err) return err; @@ -388,9 +412,11 @@ static int mlxreg_fan_speed_divider_get(struct mlxreg_fan *fan, static int mlxreg_fan_config(struct mlxreg_fan *fan, struct mlxreg_core_platform_data *pdata) { + int tacho_num = 0, regval, regsize, drwr_avail = 0, tacho_avail = 0, i; struct mlxreg_core_data *data = pdata->data; bool configured = false; - int tacho_num = 0, i; + unsigned long drwrs; + u32 bit; int err; fan->samples = MLXREG_FAN_TACHO_SAMPLES_PER_PULSE_DEF; @@ -415,7 +441,9 @@ static int mlxreg_fan_config(struct mlxreg_fan *fan, fan->tacho[tacho_num].reg = data->reg; fan->tacho[tacho_num].mask = data->mask; + fan->tacho[tacho_num].prsnt = data->reg_prsnt; fan->tacho[tacho_num++].connected = true; + tacho_avail++; } else if (strnstr(data->label, "pwm", sizeof(data->label))) { if (fan->pwm.connected) { dev_err(fan->dev, "duplicate pwm entry: %s\n", @@ -453,6 +481,22 @@ static int mlxreg_fan_config(struct mlxreg_fan *fan, } } + if (pdata->capability) { + /* Obtain the number of FAN drawers, supported by system. */ + err = regmap_read(fan->regmap, pdata->capability, ®val); + if (err) { + dev_err(fan->dev, "Failed to query capability register 0x%08x\n", + pdata->capability); + return err; + } + regsize = regmap_get_val_bytes(fan->regmap); + drwrs = regval; + for_each_set_bit(bit, &drwrs, 8 * regsize) + drwr_avail++; + /* Set the number of tachometers per one drawer. */ + fan->tachos_per_drwr = tacho_avail / drwr_avail; + } + /* Init cooling levels per PWM state. */ for (i = 0; i < MLXREG_FAN_SPEED_MIN_LEVEL; i++) fan->cooling_levels[i] = MLXREG_FAN_SPEED_MIN_LEVEL;
Add support for fan drawer's capability and present registers in order to set mapping between the fan drawers and tachometers. Some systems are equipped with fan drawers with one tachometer inside. Others with fan drawers with several tachometers inside. Using present register along with tachometer-to-drawer mapping allows to skip reading missed tachometers and expose input for them as zero, instead of exposing fault code returned by hardware. Signed-off-by: Vadim Pasternak <vadimp@nvidia.com> --- drivers/hwmon/mlxreg-fan.c | 46 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-)