> -----Original Message-----
> From: Guenter Roeck <linux@xxxxxxxxxxxx>
> Sent: Tuesday, December 08, 2020 9:50 PM
> To: Vadim Pasternak <vadimp@xxxxxxxxxx>
> Cc: linux-hwmon@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxx>
> > ---
> > 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
> >
