| Message ID | 20190318105324.2602-1-vadimp@mellanox.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v1,hwmon-next] hwmon: (mlxreg-fan) Add support for fan capability registers | expand |
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@mellanox.com> > --- > 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. > +#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, ®val); > + 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, ®val); > + 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); >
> -----Original Message----- > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck > Sent: Monday, March 18, 2019 4:18 PM > To: Vadim Pasternak <vadimp@mellanox.com> > Cc: linux-hwmon@vger.kernel.org > 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@mellanox.com> > > --- > > 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, ®val); > > + 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, ®val); > > + 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); > >
On 3/18/19 7:31 AM, Vadim Pasternak wrote: > > >> -----Original Message----- >> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck >> Sent: Monday, March 18, 2019 4:18 PM >> To: Vadim Pasternak <vadimp@mellanox.com> >> Cc: linux-hwmon@vger.kernel.org >> 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@mellanox.com> >>> --- >>> 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, ®val); >>> + 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, ®val); >>> + 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); >>> >
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) +#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, ®val); + 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; +} + +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, ®val); + 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. + */ + 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);
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@mellanox.com> --- drivers/hwmon/mlxreg-fan.c | 78 +++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 73 insertions(+), 5 deletions(-)