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.
...
> +/* 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?
...
> + },
> + {
> + .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?
> + 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?
--
Darren Hart
VMware Open Source Technology Center
