Re: [1/1] hwmon: (mlxreg-fan) Add support for Mellanox FAN driver

[Guenter Roeck <linux@roeck-us.net>]

On Wed, Jun 20, 2018 at 05:09:38PM +0000, Vadim Pasternak wrote:
> Driver obtains PWM and tachometers registers location according to the
> system configuration and creates FAN/PWM hwmon objects and a cooling
> device. PWM and tachometers are controlled through the on-board
> programmable device, which exports its register map. This device could be
> attached to any bus type, for which register mapping is supported. Single
> instance is created with one PWM control, up to 12 tachometers and one
> cooling device. It could be as many instances as programmable device
> supports.
> 
> Currently driver will be activated from the Mellanox platform driver:
> drivers/platform/x86/mlx-platform.c.
> For the future ARM based systems it could be activated from the ARM
> platform module.
> 

Your call, but no devicetree support ?

> Signed-off-by: Vadim Pasternak <vadimp@mellanox.com>
> ---
>  drivers/hwmon/Kconfig      |  12 ++
>  drivers/hwmon/Makefile     |   1 +
>  drivers/hwmon/mlxreg-fan.c | 466 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 479 insertions(+)
>  create mode 100644 drivers/hwmon/mlxreg-fan.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index f10840a..103d4bc 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -937,6 +937,18 @@ config SENSORS_MCP3021
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called mcp3021.
>  
> +config SENSORS_MLXREG_FAN
> +	tristate "Mellanox Mellanox FAN driver"
> +	depends on MELLANOX_PLATFORM
> +	depends on REGMAP

Every other driver uses "select REGMAP".

> +	depends on THERMAL

Usually that would be "THERMAL || THERMAL=n". I understand the code
as written makes it mandatory, but it is still unusual.

> +	help
> +	  This option enables support for the FAN control on the Mellanox
> +	  Ethernet and InfiniBand switches. The driver can be activated by the
> +	  platform device add call. Say Y to enable these. To compile this
> +	  driver as a module, choose 'M' here: the module will be called
> +	  mlxreg-fan.
> +
>  config SENSORS_TC654
>  	tristate "Microchip TC654/TC655 and compatibles"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index e7d52a3..cac3c06 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -129,6 +129,7 @@ obj-$(CONFIG_SENSORS_MAX31790)	+= max31790.o
>  obj-$(CONFIG_SENSORS_MC13783_ADC)+= mc13783-adc.o
>  obj-$(CONFIG_SENSORS_MCP3021)	+= mcp3021.o
>  obj-$(CONFIG_SENSORS_TC654)	+= tc654.o
> +obj-$(CONFIG_SENSORS_MLXREG_FAN) += mlxreg-fan.o
>  obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o
>  obj-$(CONFIG_SENSORS_NCT6683)	+= nct6683.o
>  obj-$(CONFIG_SENSORS_NCT6775)	+= nct6775.o
> diff --git a/drivers/hwmon/mlxreg-fan.c b/drivers/hwmon/mlxreg-fan.c
> new file mode 100644
> index 0000000..c2bf43f
> --- /dev/null
> +++ b/drivers/hwmon/mlxreg-fan.c
> @@ -0,0 +1,466 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR BSD-3-Clause)
> +//
> +// Copyright (c) 2018 Mellanox Technologies. All rights reserved.
> +// Copyright (c) 2018 Vadim Pasternak <vadimp@mellanox.com>
> +
> +#include <linux/bitops.h>
> +#include <linux/device.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>

Should not be needed.

> +#include <linux/module.h>
> +#include <linux/platform_data/mlxreg.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/thermal.h>
> +
> +#define MLXREG_FAN_MAX_TACHO		12
> +#define MLXREG_FAN_MAX_STATE		10
> +#define MLXREG_FAN_MIN_DUTY		51	/* 20% */
> +#define MLXREG_FAN_MAX_DUTY		255	/* 100% */
> +/* Minimum and maximum FAN allowed speed in percent: from 20% to 100%. Values

Standard multi-line comments please.

> + * MLXREG_FAN_MAX_STATE + x, where x is between 2 and 10 are used for
> + * setting FAN speed dynamic minimum. For example, if value is set to 14 (40%)
> + * cooling levels vector will be set to 4, 4, 4, 4, 4, 5, 6, 7, 8, 9, 10 to
> + * introduce PWM speed in percent: 40, 40, 40, 40, 40, 50, 60. 70, 80, 90, 100.
> + */
> +#define MLXREG_FAN_SPEED_MIN		(MLXREG_FAN_MAX_STATE + 2)
> +#define MLXREG_FAN_SPEED_MAX		(MLXREG_FAN_MAX_STATE * 2)
> +#define MLXREG_FAN_SPEED_MIN_LEVEL	2	/* 20 percent */
> +#define MLXREG_FAN_TACHO_ROUND_DEF	500
> +#define MLXREG_FAN_TACHO_DIVIDER_DEF	1132
> +#define MLXREG_FAN_GET_RPM(val, d, r)	(15000000 / ((val) * (d) / 100 + (r)))
> +#define MLXREG_FAN_GET_FAULT(val, mask) (((val) ^ (mask)) ? false : true)
> +#define MLXREG_FAN_PWM_DUTY2STATE(duty)	(DIV_ROUND_CLOSEST((duty) *	\
> +					 MLXREG_FAN_MAX_STATE,		\
> +					 MLXREG_FAN_MAX_DUTY))
> +#define MLXREG_FAN_PWM_STATE2DUTY(stat)	(DIV_ROUND_CLOSEST((stat) *	\
> +					 MLXREG_FAN_MAX_DUTY,		\
> +					 MLXREG_FAN_MAX_STATE))
> +
> +/**

Those data structures are only used internally in this driver. Sure you want
them documented as API ?

> + * struct mlxreg_fan_tacho - tachometer data:
> + *
> + * @connected: indicates if tachometer is connected;
> + * @pwm_connected: indicates if PWM is connected;
> + * @reg: register offset;
> + * @mask: fault mask;
> + */
> +struct mlxreg_fan_tacho {
> +	bool connected;
> +	int reg;
> +	int mask;
> +};
> +
> +/**
> + * struct mlxreg_fan_pwm - PWM data:
> + *
> + * @connected: indicates if PWM is connected;
> + * @reg: register offset;
> + */
> +struct mlxreg_fan_pwm {
> +	bool connected;
> +	int reg;
> +};
> +
> +/**
> + * struct mlxreg_fan - private data:
> + *
> + * @pdev: platform device;

The only use of pdev is to dereference pdev->dev. Might as well
put it here directly instead of dereferencing it repeatedly.

> + * @pdata: platform data;

The only use of pdata outside the probe function is to dereference
regmap. Might as well store regmap directly.

> + * @tacho: tachometer data;
> + * @pwm: PWM data;
> + * @round: round value for tachometer RPM calculation;
> + * @divider: divider value for tachometer RPM calculation;
> + * @cooling: cooling device levels;
> + * @cdev: cooling device;
> + */
> +struct mlxreg_fan {
> +	struct platform_device *pdev;
> +	struct mlxreg_core_platform_data *pdata;
> +	struct mlxreg_fan_tacho tacho[MLXREG_FAN_MAX_TACHO];
> +	struct mlxreg_fan_pwm pwm;
> +	int round;
> +	int divider;
> +	u8 cooling_levels[MLXREG_FAN_MAX_STATE + 1];
> +	struct thermal_cooling_device *cdev;
> +};
> +
> +static int
> +mlxreg_fan_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +		int channel, long *val)
> +{
> +	struct mlxreg_fan *mlxreg_fan = dev_get_drvdata(dev);
> +	struct mlxreg_fan_tacho *tacho;
> +	u32 regval;
> +	int err;
> +
> +	switch (type) {
> +	case hwmon_fan:
> +		tacho = &mlxreg_fan->tacho[channel];
> +		switch (attr) {
> +		case hwmon_fan_input:
> +			err = regmap_read(mlxreg_fan->pdata->regmap,
> +					  tacho->reg, &regval);
> +			if (err)
> +				return err;
> +
> +			*val = MLXREG_FAN_GET_RPM(regval, mlxreg_fan->divider,
> +						  mlxreg_fan->round);
> +			break;
> +
> +		case hwmon_fan_fault:
> +			err = regmap_read(mlxreg_fan->pdata->regmap,
> +					  tacho->reg, &regval);
> +			if (err)
> +				return err;
> +
> +			*val = MLXREG_FAN_GET_FAULT(regval, tacho->mask);
> +			break;
> +
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +		break;
> +
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +			err = regmap_read(mlxreg_fan->pdata->regmap,
> +					  mlxreg_fan->pwm.reg, &regval);
> +			if (err)
> +				return err;
> +
> +			*val = regval;
> +			break;
> +
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +		break;
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +mlxreg_fan_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> +		 int channel, long val)
> +{
> +	struct mlxreg_fan *mlxreg_fan = dev_get_drvdata(dev);
> +
> +	switch (type) {
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +			if (val < MLXREG_FAN_MIN_DUTY || val >
> +			    MLXREG_FAN_MAX_DUTY)

Please plit lines after ||

> +				return -EINVAL;
> +			return regmap_write(mlxreg_fan->pdata->regmap,
> +					    mlxreg_fan->pwm.reg, val);
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +		break;
> +
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static umode_t
> +mlxreg_fan_is_visible(const void *data, enum hwmon_sensor_types type, u32 attr,
> +		      int channel)
> +{
> +	switch (type) {
> +	case hwmon_fan:
> +		if (!(((struct mlxreg_fan *)data)->tacho[channel].connected))
> +			return 0;
> +
> +		switch (attr) {
> +		case hwmon_fan_input:
> +		case hwmon_fan_fault:
> +			return 0444;
> +		default:
> +			break;
> +		}
> +		break;
> +
> +	case hwmon_pwm:
> +		switch (attr) {
> +		case hwmon_pwm_input:
> +			return 0644;
> +		default:
> +			break;
> +		}
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static const u32 mlxreg_fan_hwmon_fan_config[] = {
> +	HWMON_F_INPUT | HWMON_F_FAULT,
> +	HWMON_F_INPUT | HWMON_F_FAULT,
> +	HWMON_F_INPUT | HWMON_F_FAULT,
> +	HWMON_F_INPUT | HWMON_F_FAULT,
> +	HWMON_F_INPUT | HWMON_F_FAULT,
> +	HWMON_F_INPUT | HWMON_F_FAULT,
> +	HWMON_F_INPUT | HWMON_F_FAULT,
> +	HWMON_F_INPUT | HWMON_F_FAULT,
> +	HWMON_F_INPUT | HWMON_F_FAULT,
> +	HWMON_F_INPUT | HWMON_F_FAULT,
> +	HWMON_F_INPUT | HWMON_F_FAULT,
> +	HWMON_F_INPUT | HWMON_F_FAULT,
> +	0
> +};
> +
> +static const struct hwmon_channel_info mlxreg_fan_hwmon_fan = {
> +	.type = hwmon_fan,
> +	.config = mlxreg_fan_hwmon_fan_config,
> +};
> +
> +static const u32 mlxreg_fan_hwmon_pwm_config[] = {
> +	HWMON_PWM_INPUT,
> +	0
> +};
> +
> +static const struct hwmon_channel_info mlxreg_fan_hwmon_pwm = {
> +	.type = hwmon_pwm,
> +	.config = mlxreg_fan_hwmon_pwm_config,
> +};
> +
> +static const struct hwmon_channel_info *mlxreg_fan_hwmon_info[] = {
> +	&mlxreg_fan_hwmon_fan,
> +	&mlxreg_fan_hwmon_pwm,
> +	NULL
> +};
> +
> +static const struct hwmon_ops mlxreg_fan_hwmon_hwmon_ops = {
> +	.is_visible = mlxreg_fan_is_visible,
> +	.read = mlxreg_fan_read,
> +	.write = mlxreg_fan_write,
> +};
> +
> +static const struct hwmon_chip_info mlxreg_fan_hwmon_chip_info = {
> +	.ops = &mlxreg_fan_hwmon_hwmon_ops,
> +	.info = mlxreg_fan_hwmon_info,
> +};
> +
> +static int mlxreg_fan_get_max_state(struct thermal_cooling_device *cdev,
> +				    unsigned long *state)
> +{
> +	*state = MLXREG_FAN_MAX_STATE;
> +	return 0;
> +}
> +
> +static int mlxreg_fan_get_cur_state(struct thermal_cooling_device *cdev,
> +				    unsigned long *state)
> +
> +{
> +	struct mlxreg_fan *mlxreg_fan = cdev->devdata;
> +	u32 regval;
> +	int err;
> +
> +	err = regmap_read(mlxreg_fan->pdata->regmap, mlxreg_fan->pwm.reg,
> +			  &regval);
> +	if (err) {
> +		dev_err(&mlxreg_fan->pdev->dev, "Failed to query PWM duty\n");
> +		return err;
> +	}
> +
> +	*state = MLXREG_FAN_PWM_DUTY2STATE(regval);
> +
> +	return 0;
> +}
> +
> +static int mlxreg_fan_set_cur_state(struct thermal_cooling_device *cdev,
> +				    unsigned long state)
> +
> +{
> +	struct mlxreg_fan *mlxreg_fan = cdev->devdata;
> +	unsigned long cur_state;
> +	u32 regval;
> +	int i;
> +	int err;
> +
> +	/* Verify if this request is for changing allowed FAN dynamical

/*
 * comment 
 */

> +	 * minimum. If it is - update cooling levels accordingly and update
> +	 * state, if current state is below the newly requested minimum state.
> +	 * For example, if current state is 5, and minimal state is to be
> +	 * changed from 4 to 6, mlxreg_fan->cooling_levels[0 to 5] will be
> +	 * changed all from 4 to 6. And state 5 (mlxreg_fan->cooling_levels[4])
> +	 * should be overwritten.
> +	 */
> +	if (state >= MLXREG_FAN_SPEED_MIN && state <= MLXREG_FAN_SPEED_MAX) {
> +		state -= MLXREG_FAN_MAX_STATE;
> +		for (i = 0; i < state; i++)
> +			mlxreg_fan->cooling_levels[i] = state;
> +		for (i = state; i <= MLXREG_FAN_MAX_STATE; i++)
> +			mlxreg_fan->cooling_levels[i] = i;
> +
> +		err = regmap_read(mlxreg_fan->pdata->regmap,
> +				  mlxreg_fan->pwm.reg, &regval);
> +		if (err) {
> +			dev_err(&mlxreg_fan->pdev->dev, "Failed to query PWM duty\n");
> +			return err;
> +		}
> +
> +		cur_state = MLXREG_FAN_PWM_DUTY2STATE(regval);
> +		if (state < cur_state)
> +			return 0;
> +
> +		state = cur_state;
> +	}
> +
> +	if (state > MLXREG_FAN_MAX_STATE)
> +		return -EINVAL;
> +
> +	/* Normalize the state to the valid speed range. */
> +	state = mlxreg_fan->cooling_levels[state];
> +	err = regmap_write(mlxreg_fan->pdata->regmap, mlxreg_fan->pwm.reg,
> +			   MLXREG_FAN_PWM_STATE2DUTY(state));
> +	if (err) {
> +		dev_err(&mlxreg_fan->pdev->dev, "Failed to write PWM duty\n");
> +		return err;
> +	}
> +	return 0;
> +}
> +
> +static const struct thermal_cooling_device_ops mlxreg_fan_cooling_ops = {
> +	.get_max_state	= mlxreg_fan_get_max_state,
> +	.get_cur_state	= mlxreg_fan_get_cur_state,
> +	.set_cur_state	= mlxreg_fan_set_cur_state,
> +};
> +
> +static int mlxreg_fan_config(struct mlxreg_fan *mlxreg_fan)
> +{
> +	struct mlxreg_core_platform_data *pdata = mlxreg_fan->pdata;

Pass mlxreg_core_platform_data as argument to avoid needing it 
in mlxreg_fan.

> +	struct mlxreg_core_data *data = pdata->data;
> +	int tacho_num = 0, i;
> +
> +	mlxreg_fan->round = MLXREG_FAN_TACHO_ROUND_DEF;
> +	mlxreg_fan->divider = MLXREG_FAN_TACHO_DIVIDER_DEF;
> +	for (i = 0; i < pdata->counter; i++, data++) {
> +		if (strnstr(data->label, "tacho", sizeof(data->label))) {
> +			if (tacho_num == MLXREG_FAN_MAX_TACHO) {
> +				dev_err(&mlxreg_fan->pdev->dev, "invalid tacho entry: %s\n",
> +					data->label);
> +				return -EINVAL;
> +			}
> +			mlxreg_fan->tacho[tacho_num].reg = data->reg;
> +			mlxreg_fan->tacho[tacho_num].mask = data->mask;
> +			mlxreg_fan->tacho[tacho_num++].connected = true;
> +		} else if (strnstr(data->label, "pwm", sizeof(data->label))) {
> +			if (mlxreg_fan->pwm.connected) {
> +				dev_err(&mlxreg_fan->pdev->dev, "invalid pwm entry: %s\n",
> +					data->label);
> +				return -EINVAL;
> +			}
> +			mlxreg_fan->pwm.reg = data->reg;
> +			mlxreg_fan->pwm.connected = true;
> +		} else if (strnstr(data->label, "conf", sizeof(data->label))) {
> +			mlxreg_fan->round = data->mask;
> +			mlxreg_fan->divider = data->bit;
> +		} else {
> +			dev_err(&mlxreg_fan->pdev->dev, "invalid label: %s\n",
> +				data->label);
> +			return -EINVAL;
> +		}
> +		dev_info(&mlxreg_fan->pdev->dev, "label: %s, offset:0x%02x\n",
> +			 data->label, data->reg);

This is debugging information. Please no noise.

> +	}
> +
> +	/* Init cooling levels per PWM state. */
> +	for (i = 0; i < MLXREG_FAN_SPEED_MIN_LEVEL; i++)
> +		mlxreg_fan->cooling_levels[i] = MLXREG_FAN_SPEED_MIN_LEVEL;
> +	for (i = MLXREG_FAN_SPEED_MIN_LEVEL; i <= MLXREG_FAN_MAX_STATE; i++)
> +		mlxreg_fan->cooling_levels[i] = i;
> +
> +	return 0;
> +}
> +
> +static int mlxreg_fan_probe(struct platform_device *pdev)
> +{
> +	struct mlxreg_core_platform_data *pdata;
> +	struct mlxreg_fan *mlxreg_fan;
> +	struct device *hwm;
> +	const char *name;
> +	int err;
> +
> +	pdata = dev_get_platdata(&pdev->dev);
> +	if (!pdata) {
> +		dev_err(&pdev->dev, "Failed to get platform data.\n");
> +		return -EINVAL;
> +	}
> +
> +	mlxreg_fan = devm_kzalloc(&pdev->dev, sizeof(*mlxreg_fan), GFP_KERNEL);
> +	if (!mlxreg_fan)
> +		return -ENOMEM;
> +
> +	mlxreg_fan->pdev = pdev;
> +	mlxreg_fan->pdata = pdata;
> +	dev_set_drvdata(&pdev->dev, mlxreg_fan);
> +

platform_set_drvdata()

> +	err = mlxreg_fan_config(mlxreg_fan);
> +	if (err)
> +		return err;
> +
> +	if (pdev->id > -1)
> +		name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s%d",
> +				      "mlxreg_fan", pdev->id);
> +	else
> +		name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "mlxreg_fan");

This is very unusual. Why do you need a per-device-id name ? Why not just
use a single name such as "mlxreg_fan" like every other driver ?

> +	if (!name)
> +		return -ENOMEM;
> +
> +	hwm = devm_hwmon_device_register_with_info(&pdev->dev, name,
> +						   mlxreg_fan,
> +						   &mlxreg_fan_hwmon_chip_info,
> +						   NULL);
> +	if (IS_ERR(hwm)) {
> +		dev_err(&pdev->dev, "Failed to register hwmon device\n");
> +		return PTR_ERR(hwm);
> +	}
> +
> +	mlxreg_fan->cdev = thermal_cooling_device_register("mlxreg_fan",
> +						mlxreg_fan,
> +						&mlxreg_fan_cooling_ops);
> +	if (IS_ERR(mlxreg_fan->cdev)) {
> +		dev_err(&pdev->dev, "Failed to register cooling device\n");
> +		return PTR_ERR(mlxreg_fan->cdev);
> +	}

This effectively makes the driver hard dependent on THERMAL. Ok with me
if that is what you want, it just seems unusual to limit the driver's use
case like that.

> +
> +	return 0;
> +}
> +
> +static int mlxreg_fan_remove(struct platform_device *pdev)
> +{
> +	struct mlxreg_fan *mlxreg_fan = platform_get_drvdata(pdev);
> +
> +	thermal_cooling_device_unregister(mlxreg_fan->cdev);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver mlxreg_fan_driver = {
> +	.driver = {
> +	    .name = "mlxreg-fan",
> +	},
> +	.probe = mlxreg_fan_probe,
> +	.remove = mlxreg_fan_remove,
> +};
> +
> +module_platform_driver(mlxreg_fan_driver);
> +
> +MODULE_AUTHOR("Vadim Pasternak <vadimp@mellanox.com>");
> +MODULE_DESCRIPTION("Mellanox FAN driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:mlxreg-fan");