On Wed, Jul 25, 2018 at 04:42:18PM +0000, Vadim Pasternak wrote: > > > > -----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? Ah, fair enough. Yes, can leave as is. > > > > + 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. Sounds like no need. Can update as needed in the future. > > > > > > > > > + 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). Thanks for the context. Let's leave as is. -- Darren Hart VMware Open Source Technology Center
