> -----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 > ... > > > + }, > > + { > > + .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
