> -----Original Message----- > From: Vadim Pasternak > Sent: Wednesday, July 25, 2018 7:22 PM > To: 'Darren Hart' <dvhart@xxxxxxxxxxxxx> > Cc: andy.shevchenko@xxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; platform- > driver-x86@xxxxxxxxxxxxxxx; jiri@xxxxxxxxxxx; Michael Shych > <michaelsh@xxxxxxxxxxxx>; ivecera@xxxxxxxxxx > Subject: RE: [PATCH v1 1/8] platform/x86: mlx-platform: Add mlxreg-fan > platform driver activation > > > > > -----Original Message----- > > From: Darren Hart [mailto:dvhart@xxxxxxxxxxxxx] > > Sent: Wednesday, July 25, 2018 6:59 PM > > To: Vadim Pasternak <vadimp@xxxxxxxxxxxx> > > Cc: andy.shevchenko@xxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; platform- > > driver-x86@xxxxxxxxxxxxxxx; jiri@xxxxxxxxxxx; Michael Shych > > <michaelsh@xxxxxxxxxxxx>; ivecera@xxxxxxxxxx > > Subject: Re: [PATCH v1 1/8] platform/x86: mlx-platform: Add mlxreg-fan > > platform driver activation > > > > On Sun, Jul 08, 2018 at 02:31:57PM +0000, Vadim Pasternak wrote: > > > Add mlxreg-fan platform driver activation. FAN driver uses the same > > > regmap infrastructure as others Mellanox platform drivers. Specific > > > registers description for default FAN platform data configuration > > > are added to mlx-platform. There are the registers for tachometers > > > reading, PWM control and FAN ownership control. The last one has a > > > default value, which is set at initialization time through the > > > regmap infrastructure, which is necessary for moving FAN control > > > ownership from hardware to software. > > > > Hi Vadim, > > > > Thanks for the series. > > > > ... > > Hi Darren, > > Thank you for reply. > > > > > > > > +/* Platform FAN default */ > > > +static struct mlxreg_core_data mlxplat_mlxcpld_default_fan_data[] = { > > > + { > > > + .label = "pwm1", > > > + .reg = MLXPLAT_CPLD_LPC_REG_PWM1_OFFSET, > > > + }, > > > + { > > > + .label = "tacho1", > > > + .reg = MLXPLAT_CPLD_LPC_REG_TACHO1_OFFSET, > > > + .mask = GENMASK(7, 0), > > > > to avoid listings like: > > > > $ ls > > tacho1 > > tacho10 > > tacho11 > > tacho12 > > tacho2 > > tacho3 > > > > I would suggest 0 padded indexes in the labels: > > > > tacho01 > > tacho02 > > tacho03 > > ... > > tacho11 > > tacho12 > > tacho13 > > > > Do you have a strong preference for the non-padding indices? > > > > No, I can change it as you suggested. > In mlxreg-fan I am matching only prefix "tacho" > https://kernel.googlesource.com/pub/scm/linux/kernel/git/groeck/linux- > staging/+/hwmon-next/drivers/hwmon/mlxreg-fan.c > But from other hand, I verified now the convention in hwmon, and I see that it follows pwm1, pwm2, fan1, fan2, .. These tacho1, tacho2 will be exposed as fan1_input, fan2_input according to hwmon infra. And this is also what is expected by user space application. So, maybe it's better to leave it as it is? > > ... > > > > > + }, > > > + { > > > + .label = "tacho10", > > > + .reg = MLXPLAT_CPLD_LPC_REG_TACHO10_OFFSET, > > > + .mask = GENMASK(7, 0), > > > + }, > > > + { > > > + .label = "tacho11", > > > + .reg = MLXPLAT_CPLD_LPC_REG_TACHO11_OFFSET, > > > + .mask = GENMASK(7, 0), > > > + }, > > > + { > > > + .label = "tacho12", > > > + .reg = MLXPLAT_CPLD_LPC_REG_TACHO12_OFFSET, > > > + .mask = GENMASK(7, 0), > > > + }, > > > +}; > > > + > > > +static struct mlxreg_core_platform_data mlxplat_default_fan_data = { > > > + .data = mlxplat_mlxcpld_default_fan_data, > > > + .counter = ARRAY_SIZE(mlxplat_mlxcpld_default_fan_data), > > > +}; > > > + > > > static bool mlxplat_mlxcpld_writeable_reg(struct device *dev, > > > unsigned int reg) { > > > switch (reg) { > > > @@ -949,6 +1039,8 @@ static bool > > > mlxplat_mlxcpld_writeable_reg(struct > > device *dev, unsigned int reg) > > > case MLXPLAT_CPLD_LPC_REG_PWR_MASK_OFFSET: > > > case MLXPLAT_CPLD_LPC_REG_FAN_EVENT_OFFSET: > > > case MLXPLAT_CPLD_LPC_REG_FAN_MASK_OFFSET: > > > + case MLXPLAT_CPLD_LPC_REG_PWM1_OFFSET: > > > > Is there likely to be more than 9 PWM in the future? Would 01 make > > more sense? > > No. > Currently all our systems has one PWM for x tachometers (where x could be 3, 4, > 8, 12). > I can imagine that we can have in the future two sets, like {PWM1, X TACHO} > and {PWM2, Y TACHO), but not more. > Bit I can change it 01 for theoretical case. > > > > > > + case MLXPLAT_CPLD_LPC_REG_PWM_CONTROL_OFFSET: > > > > Is this PWM generic or specific to PWM1 - if the latter, can we make > > the defines match up to make the pairing obvious? > > This is generic. The purpose of that to take FAN control ownership by SW. > If SW doesn't do it, during long enough time (`5 minutes if I am not wrong After > BIOS successful completion indication), CPLD will be FAN master. > Two reason for that: to protect the system from overheating in case SW Is dead, > for some customer (this is more for InfiniBand systems) we provide unmanaged > system - with no CPU. > So it can't be per some PWM-TACHo set, but only for all. > > Actually all this FAN support is a new feature in CPLD for our new systems (next > generation with 32x200G Eth, 40x200IB new ASICs). > > > > > > > -- > > Darren Hart > > VMware Open Source Technology Center
