Message-ID: <20201104055413.GA124480@roeck-us.net>
Date:   Tue, 3 Nov 2020 21:54:13 -0800
From:   Guenter Roeck <linux@...ck-us.net>
To:     alexandru.tachici@...log.com
Cc:     linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, robh+dt@...nel.org
Subject: Re: [PATCH 1/3] hwmon: ltc2992: Add support

On Thu, Oct 29, 2020 at 11:49:09AM +0200, alexandru.tachici@...log.com wrote:
> From: Alexandru Tachici <alexandru.tachici@...log.com>
> 
> LTC2992 is a rail-to-rail system monitor that
> measures current, voltage, and power of two supplies.
> 
> Two ADCs simultaneously measure each supply’s current.
> A third ADC monitors the input voltages and four
> auxiliary external voltages.
> 
> Signed-off-by: Alexandru Tachici <alexandru.tachici@...log.com>

High level comment: checkpatch finds a number of spelling errors.
Please fix.

> ---
>  Documentation/hwmon/index.rst   |   1 +
>  Documentation/hwmon/ltc2992.rst |  51 +++
>  drivers/hwmon/Kconfig           |  11 +
>  drivers/hwmon/Makefile          |   1 +
>  drivers/hwmon/ltc2992.c         | 735 ++++++++++++++++++++++++++++++++
>  5 files changed, 799 insertions(+)
>  create mode 100644 Documentation/hwmon/ltc2992.rst
>  create mode 100644 drivers/hwmon/ltc2992.c
> 
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index e6b91ab12978..f759d70ae9fd 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -104,6 +104,7 @@ Hardware Monitoring Kernel Drivers
>     ltc2947
>     ltc2978
>     ltc2990
> +   ltc2992
>     ltc3815
>     ltc4151
>     ltc4215
> diff --git a/Documentation/hwmon/ltc2992.rst b/Documentation/hwmon/ltc2992.rst
> new file mode 100644
> index 000000000000..1dd48ef9f655
> --- /dev/null
> +++ b/Documentation/hwmon/ltc2992.rst
> @@ -0,0 +1,51 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +Kernel driver ltc2992
> +=====================
> +
> +Supported chips:
> +  * Linear Technology LTC2992
> +    Prefix: 'ltc2992'
> +    Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ltc2992.pdf
> +
> +Author: Alexandru Tachici <alexandru.tachici@...log.com>
> +
> +
> +Description
> +-----------
> +
> +This driver supports hardware monitoring for Linear Technology LTC2992 power monitor.
> +
> +LTC2992 is is a rail-to-rail system monitor that measures current,
> +voltage, and power of two supplies.
> +
> +Two ADCs simultaneously measure each supply’s current. A third ADC monitors
> +the input voltages and four auxiliary external voltages.
> +
> +
> +Sysfs entries
> +-------------
> +
> +The following attributes are supported. Limits are read-write,
> +all other attributes are read-only.
> +
> +inX_input		Measured voltage.
> +inX_min			Minimum voltage.
> +inX_max			Maximum voltage.
> +inX_min_alarm		Voltage low alarm.
> +inX_max_alarm		Voltage high alarm.
> +inX_alarm		An overvoltage or undervoltage occured. Cleared on read.
> +
> +currX_input		Measured current.
> +currX_min		Minimum current.
> +currX_max		Maximum current.
> +currX_min_alarm		Current low alarm.
> +currX_max_alarm		Current high alarm.
> +currX_alarm		An overvoltage or undervoltage occured. Cleared on read.
> +
> +powerX_input		Measured power.
> +powerX_min		Minimum power.
> +powerX_max		Maximum power.
> +powerX_min_alarm	Power low alarm.
> +powerX_max_alarm	Power high alarm.
> +powerX_alarm		An overpower or underpower occured. Cleared on read.
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index a850e4f0e0bd..bf9e387270d6 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -858,6 +858,17 @@ config SENSORS_LTC2990
>  	  This driver can also be built as a module. If so, the module will
>  	  be called ltc2990.
>  
> +config SENSORS_LTC2992
> +	tristate "Linear Technology LTC2992"
> +	depends on I2C
> +	help
> +	  If you say yes here you get support for Linear Technology LTC2992
> +	  I2C System Monitor. The LTC2992 measures current, voltage, and
> +	  power of two supplies.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called ltc2992.
> +
>  config SENSORS_LTC4151
>  	tristate "Linear Technology LTC4151"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 9db2903b61e5..d6172c4807c4 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -118,6 +118,7 @@ obj-$(CONFIG_SENSORS_LTC2947)	+= ltc2947-core.o
>  obj-$(CONFIG_SENSORS_LTC2947_I2C) += ltc2947-i2c.o
>  obj-$(CONFIG_SENSORS_LTC2947_SPI) += ltc2947-spi.o
>  obj-$(CONFIG_SENSORS_LTC2990)	+= ltc2990.o
> +obj-$(CONFIG_SENSORS_LTC2992)	+= ltc2992.o
>  obj-$(CONFIG_SENSORS_LTC4151)	+= ltc4151.o
>  obj-$(CONFIG_SENSORS_LTC4215)	+= ltc4215.o
>  obj-$(CONFIG_SENSORS_LTC4222)	+= ltc4222.o
> diff --git a/drivers/hwmon/ltc2992.c b/drivers/hwmon/ltc2992.c
> new file mode 100644
> index 000000000000..940d92b4a1d0
> --- /dev/null
> +++ b/drivers/hwmon/ltc2992.c
> @@ -0,0 +1,735 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +/*
> + * LTC2992 - Dual Wide Range Power Monitor
> + *
> + * Copyright 2020 Analog Devices Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/bits.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +
> +#define LTC2992_CTRLB			0x01
> +#define LTC2992_FAULT1			0x03
> +#define LTC2992_POWER1			0x05
> +#define LTC2992_POWER1_MAX		0x08
> +#define LTC2992_POWER1_MIN		0x0B
> +#define LTC2992_POWER1_MAX_THRESH	0x0E
> +#define LTC2992_POWER1_MIN_THRESH	0x11
> +#define LTC2992_DSENSE1			0x14
> +#define LTC2992_DSENSE1_MAX		0x16
> +#define LTC2992_DSENSE1_MIN		0x18
> +#define LTC2992_DSENSE1_MAX_THRESH	0x1A
> +#define LTC2992_DSENSE1_MIN_THRESH	0x1C
> +#define LTC2992_SENSE1			0x1E
> +#define LTC2992_SENSE1_MAX		0x20
> +#define LTC2992_SENSE1_MIN		0x22
> +#define LTC2992_SENSE1_MAX_THRESH	0x24
> +#define LTC2992_SENSE1_MIN_THRESH	0x26
> +#define LTC2992_G1			0x28
> +#define LTC2992_G1_MAX			0x2A
> +#define LTC2992_G1_MIN			0x2C
> +#define LTC2992_G1_MAX_THRESH		0x2E
> +#define LTC2992_G1_MIN_THRESH		0x30
> +#define LTC2992_FAULT2			0x35
> +#define LTC2992_G2			0x5A
> +#define LTC2992_G2_MAX			0x5C
> +#define LTC2992_G2_MIN			0x5E
> +#define LTC2992_G2_MAX_THRESH		0x60
> +#define LTC2992_G2_MIN_THRESH		0x62
> +#define LTC2992_G3			0x64
> +#define LTC2992_G3_MAX			0x66
> +#define LTC2992_G3_MIN			0x68
> +#define LTC2992_G3_MAX_THRESH		0x6A
> +#define LTC2992_G3_MIN_THRESH		0x6C
> +#define LTC2992_G4			0x6E
> +#define LTC2992_G4_MAX			0x70
> +#define LTC2992_G4_MIN			0x72
> +#define LTC2992_G4_MAX_THRESH		0x74
> +#define LTC2992_G4_MIN_THRESH		0x76
> +#define LTC2992_FAULT3			0x92
> +
> +#define LTC2992_POWER(x)		(LTC2992_POWER1 + ((x) * 0x32))
> +#define LTC2992_POWER_MAX(x)		(LTC2992_POWER1_MAX + ((x) * 0x32))
> +#define LTC2992_POWER_MIN(x)		(LTC2992_POWER1_MIN + ((x) * 0x32))
> +#define LTC2992_POWER_MAX_THRESH(x)	(LTC2992_POWER1_MAX_THRESH + ((x) * 0x32))
> +#define LTC2992_POWER_MIN_THRESH(x)	(LTC2992_POWER1_MIN_THRESH + ((x) * 0x32))
> +#define LTC2992_DSENSE(x)		(LTC2992_DSENSE1 + ((x) * 0x32))
> +#define LTC2992_DSENSE_MAX(x)		(LTC2992_DSENSE1_MAX + ((x) * 0x32))
> +#define LTC2992_DSENSE_MIN(x)		(LTC2992_DSENSE1_MIN + ((x) * 0x32))
> +#define LTC2992_DSENSE_MAX_THRESH(x)	(LTC2992_DSENSE1_MAX_THRESH + ((x) * 0x32))
> +#define LTC2992_DSENSE_MIN_THRESH(x)	(LTC2992_DSENSE1_MIN_THRESH + ((x) * 0x32))
> +#define LTC2992_SENSE(x)		(LTC2992_SENSE1 + ((x) * 0x32))
> +#define LTC2992_SENSE_MAX(x)		(LTC2992_SENSE1_MAX + ((x) * 0x32))
> +#define LTC2992_SENSE_MIN(x)		(LTC2992_SENSE1_MIN + ((x) * 0x32))
> +#define LTC2992_SENSE_MAX_THRESH(x)	(LTC2992_SENSE1_MAX_THRESH + ((x) * 0x32))
> +#define LTC2992_SENSE_MIN_THRESH(x)	(LTC2992_SENSE1_MIN_THRESH + ((x) * 0x32))
> +#define LTC2992_POWER_FAULT(x)		(LTC2992_FAULT1 + ((x) * 0x32))
> +#define LTC2992_SENSE_FAULT(x)		(LTC2992_FAULT1 + ((x) * 0x32))
> +#define LTC2992_DSENSE_FAULT(x)		(LTC2992_FAULT1 + ((x) * 0x32))
> +
> +/* FAULT1 FAULT2 registers common bitfields */
> +#define LTC2992_POWER_FAULT_MSK		GENMASK(7, 6)
> +#define LTC2992_DSENSE_FAULT_MSK	GENMASK(5, 4)
> +#define LTC2992_SENSE_FAULT_MSK		GENMASK(3, 2)
> +
> +/* FAULT1 bitfields */
> +#define LTC2992_GPIO1_FAULT_MSK		GENMASK(1, 0)
> +
> +/* FAULT2 bitfields */
> +#define LTC2992_GPIO2_FAULT_MSK		GENMASK(1, 0)
> +
> +/* FAULT3 bitfields */
> +#define LTC2992_GPIO3_FAULT_MSK		GENMASK(7, 6)
> +#define LTC2992_GPIO4_FAULT_MSK		GENMASK(5, 4)
> +
> +#define LTC2992_IADC_NANOV_LSB		12500
> +#define LTC2992_VADC_UV_LSB		25000
> +#define LTC2992_VADC_GPIO_UV_LSB	500
> +
> +struct ltc2992_state {
> +	struct i2c_client		*client;
> +	struct regmap			*regmap;
> +	u32				r_sense_uohm[2];
> +};
> +
> +struct ltc2992_gpio_regs {
> +	u8	data;
> +	u8	max;
> +	u8	min;
> +	u8	max_thresh;
> +	u8	min_thresh;
> +	u8	alarm;
> +	u8	alarm_msk;
> +};
> +
> +static const struct ltc2992_gpio_regs ltc2992_gpio_addr_map[] = {
> +	{
> +		.data = LTC2992_G1,
> +		.max = LTC2992_G1_MAX,
> +		.min = LTC2992_G1_MIN,
> +		.max_thresh = LTC2992_G1_MAX_THRESH,
> +		.min_thresh = LTC2992_G1_MIN_THRESH,
> +		.alarm = LTC2992_FAULT1,
> +		.alarm_msk = LTC2992_GPIO1_FAULT_MSK,
> +	},
> +	{
> +		.data = LTC2992_G2,
> +		.max = LTC2992_G2_MAX,
> +		.min = LTC2992_G2_MIN,
> +		.max_thresh = LTC2992_G2_MAX_THRESH,
> +		.min_thresh = LTC2992_G2_MIN_THRESH,
> +		.alarm = LTC2992_FAULT2,
> +		.alarm_msk = LTC2992_GPIO2_FAULT_MSK,
> +	},
> +	{
> +		.data = LTC2992_G3,
> +		.max = LTC2992_G3_MAX,
> +		.min = LTC2992_G3_MIN,
> +		.max_thresh = LTC2992_G3_MAX_THRESH,
> +		.min_thresh = LTC2992_G3_MIN_THRESH,
> +		.alarm = LTC2992_FAULT3,
> +		.alarm_msk = LTC2992_GPIO3_FAULT_MSK,
> +	},
> +	{
> +		.data = LTC2992_G4,
> +		.max = LTC2992_G4_MAX,
> +		.min = LTC2992_G4_MIN,
> +		.max_thresh = LTC2992_G4_MAX_THRESH,
> +		.min_thresh = LTC2992_G4_MIN_THRESH,
> +		.alarm = LTC2992_FAULT3,
> +		.alarm_msk = LTC2992_GPIO4_FAULT_MSK,
> +	},
> +};
> +
> +static int ltc2992_read_reg(struct ltc2992_state *st, u8 addr, const u8 reg_len, u32 *val)
> +{

The return value is at most three bytes, and it is always 0 or positive.
That means it is really unnecessary to pass a pointer to the return
value. I would suggest to just return the value directly as int,
combined with the error.

> +	u8 regvals[4];
> +	int ret;
> +	int i;
> +
> +	ret = regmap_bulk_read(st->regmap, addr, regvals, reg_len);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = 0;
> +	for (i = 0; i < reg_len; i++)
> +		*val |= regvals[reg_len - i - 1] << (i * 8);
> +
> +	return 0;
> +}
> +
> +static int ltc2992_write_reg(struct ltc2992_state *st, u8 addr, const u8 reg_len, u32 val)
> +{
> +	u8 regvals[4];
> +	int i;
> +
> +	for (i = 0; i < reg_len; i++)
> +		regvals[reg_len - i - 1] = (val >> (i * 8)) & 0xFF;
> +
> +	return regmap_bulk_write(st->regmap, addr, regvals, reg_len);
> +}
> +
> +static umode_t ltc2992_is_visible(const void *data, enum hwmon_sensor_types type, u32 attr,
> +				  int channel)
> +{
> +	const struct ltc2992_state *st = data;
> +
> +	switch (type) {
> +	case hwmon_in:
> +		switch (attr) {
> +		case hwmon_in_input:
> +		case hwmon_in_min:
> +		case hwmon_in_max:
> +		case hwmon_in_alarm:
> +			return 0444;
> +		case hwmon_in_min_alarm:
> +		case hwmon_in_max_alarm:
> +			return 0644;
> +		}
> +		break;
> +	case hwmon_curr:
> +		switch (attr) {
> +		case hwmon_curr_input:
> +		case hwmon_curr_min:
> +		case hwmon_curr_max:
> +		case hwmon_curr_alarm:
> +			if (st->r_sense_uohm[channel])
> +				return 0444;
> +			break;
> +		case hwmon_curr_min_alarm:
> +		case hwmon_curr_max_alarm:
> +			if (st->r_sense_uohm[channel])
> +				return 0644;
> +			break;
> +		}
> +		break;
> +	case hwmon_power:
> +		switch (attr) {
> +		case hwmon_power_input:
> +		case hwmon_power_min:
> +		case hwmon_power_max:
> +		case hwmon_power_alarm:
> +			if (st->r_sense_uohm[channel])
> +				return 0444;
> +			break;
> +		case hwmon_power_min_alarm:
> +		case hwmon_power_max_alarm:
> +			if (st->r_sense_uohm[channel])
> +				return 0644;
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ltc2992_get_voltage(struct ltc2992_state *st, u32 reg, u32 scale, long *val)
> +{
> +	u32 reg_val;
> +	int ret;
> +
> +	ret = ltc2992_read_reg(st, reg, 2, &reg_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	reg_val = reg_val >> 4;
> +	*val = DIV_ROUND_CLOSEST(reg_val * scale, 1000);
> +
> +	return 0;
> +}
> +
> +static int ltc2992_set_voltage(struct ltc2992_state *st, u32 reg, u32 scale, long val)
> +{
> +	val = DIV_ROUND_CLOSEST(val * 1000, scale);
> +	val = val << 4;
> +
> +	return ltc2992_write_reg(st, reg, 2, val);
> +}
> +
> +static int ltc2992_read_gpio_alarm(struct ltc2992_state *st, int nr_gpio, long *val)
> +{
> +	u32 reg_val;
> +	int ret;
> +
> +	ret = ltc2992_read_reg(st, ltc2992_gpio_addr_map[nr_gpio].alarm, 1, &reg_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = !!(reg_val & ltc2992_gpio_addr_map[nr_gpio].alarm_msk);
> +
> +	reg_val &= ~ltc2992_gpio_addr_map[nr_gpio].alarm_msk;
> +	return ltc2992_write_reg(st, ltc2992_gpio_addr_map[nr_gpio].alarm, 1, reg_val);
> +}
> +
> +static int ltc2992_read_gpios_in(struct device *dev, u32 attr, int nr_gpio, long *val)
> +{
> +	struct ltc2992_state *st = dev_get_drvdata(dev);
> +	u32 reg;
> +
> +	switch (attr) {
> +	case hwmon_in_input:
> +		reg = ltc2992_gpio_addr_map[nr_gpio].data;
> +		break;
> +	case hwmon_in_min:
> +		reg = ltc2992_gpio_addr_map[nr_gpio].min;
> +		break;
> +	case hwmon_in_max:
> +		reg = ltc2992_gpio_addr_map[nr_gpio].max;
> +		break;
> +	case hwmon_in_min_alarm:
> +		reg = ltc2992_gpio_addr_map[nr_gpio].min_thresh;
> +		break;
> +	case hwmon_in_max_alarm:
> +		reg = ltc2992_gpio_addr_map[nr_gpio].max_thresh;
> +		break;
> +	case hwmon_in_alarm:
> +		return ltc2992_read_gpio_alarm(st, nr_gpio, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return ltc2992_get_voltage(st, reg, LTC2992_VADC_GPIO_UV_LSB, val);
> +}
> +
> +static int ltc2992_read_in(struct device *dev, u32 attr, int channel, long *val)
> +{
> +	struct ltc2992_state *st = dev_get_drvdata(dev);
> +	u32 reg_val;
> +	u32 reg;
> +	int ret;
> +
> +	if (channel > 1)
> +		return ltc2992_read_gpios_in(dev, attr, channel - 2, val);
> +
> +	switch (attr) {
> +	case hwmon_in_input:
> +		reg = LTC2992_SENSE(channel);
> +		break;
> +	case hwmon_in_min:
> +		reg = LTC2992_SENSE_MIN(channel);
> +		break;
> +	case hwmon_in_max:
> +		reg = LTC2992_SENSE_MAX(channel);
> +		break;
> +	case hwmon_in_min_alarm:
> +		reg = LTC2992_SENSE_MIN_THRESH(channel);
> +		break;
> +	case hwmon_in_max_alarm:
> +		reg = LTC2992_SENSE_MAX_THRESH(channel);
> +		break;
> +	case hwmon_in_alarm:
> +		ret = ltc2992_read_reg(st, LTC2992_SENSE_FAULT(channel), 1, &reg_val);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = !!(reg_val & LTC2992_SENSE_FAULT_MSK);
> +
> +		reg_val &= ~LTC2992_SENSE_FAULT_MSK;
> +		return ltc2992_write_reg(st, LTC2992_SENSE_FAULT(channel), 1, reg_val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return ltc2992_get_voltage(st, reg, LTC2992_VADC_UV_LSB, val);
> +}
> +
> +static int ltc2992_get_current(struct ltc2992_state *st, u32 reg, u32 channel, long *val)
> +{
> +	u32 reg_val;
> +	int ret;
> +
> +	ret = ltc2992_read_reg(st, reg, 2, &reg_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	reg_val = reg_val >> 4;
> +	*val = DIV_ROUND_CLOSEST(reg_val * LTC2992_IADC_NANOV_LSB, st->r_sense_uohm[channel]);
> +
> +	return 0;
> +}
> +
> +static int ltc2992_set_current(struct ltc2992_state *st, u32 reg, u32 channel, long val)
> +{
> +	u32 reg_val;
> +
> +	reg_val = DIV_ROUND_CLOSEST(val * st->r_sense_uohm[channel], LTC2992_IADC_NANOV_LSB);
> +	reg_val = reg_val << 4;
> +
> +	return ltc2992_write_reg(st, reg, 2, reg_val);
> +}
> +
> +static int ltc2992_read_curr(struct device *dev, u32 attr, int channel, long *val)
> +{
> +	struct ltc2992_state *st = dev_get_drvdata(dev);
> +	u32 reg_val;
> +	u32 reg;
> +	int ret;
> +
> +	switch (attr) {
> +	case hwmon_curr_input:
> +		reg = LTC2992_DSENSE(channel);
> +		break;
> +	case hwmon_curr_min:
> +		reg = LTC2992_DSENSE_MIN(channel);
> +		break;
> +	case hwmon_curr_max:
> +		reg = LTC2992_DSENSE_MAX(channel);
> +		break;

The datasheet is a bit vague what those registers report, but I
_think_ it is peak minimum and maximum data. If so, that should
be reported with currX_lowest and currX_highest. Resetting
those values should be done by writing into curr_reset_history
It looks like there is only one reset bit (CTRLB[3]). With that, you
can either implement all of {in,curr,power}_reset_history
and have each reset everything, or pick just one reset_history
attribute and document that it resets all lowest and highest
attributes.

> +	case hwmon_curr_min_alarm:
> +		reg = LTC2992_DSENSE_MIN_THRESH(channel);
> +		break;
> +	case hwmon_curr_max_alarm:
> +		reg = LTC2992_DSENSE_MAX_THRESH(channel);
> +		break;

This is wrong. The alarm attributes indicate that a limit has been exceeded.
However, LTC2992_DSENSE_MAX_THRESH() and LTC2992_DSENSE_MIN_THRESH()
point to limit registers. Those should be reported and set with
currX_min and currX_max.

> +	case hwmon_curr_alarm:
> +		ret = ltc2992_read_reg(st, LTC2992_DSENSE_FAULT(channel), 1, &reg_val);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = !!(reg_val & LTC2992_DSENSE_FAULT_MSK);
> +
This is also wrong. The chip reports min_alarm and max_alarm
separately, and thus only the min_alarm and max_alarm attributes
should be provided. curr_alarm should only be provided if
the chip does not separate alarm reasons.

The same problems apply to the power and voltage sensors.

> +		reg_val &= ~LTC2992_DSENSE_FAULT_MSK;
> +		return ltc2992_write_reg(st, LTC2992_DSENSE_FAULT(channel), 1, reg_val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return ltc2992_get_current(st, reg, channel, val);
> +}
> +
> +static int ltc2992_get_power(struct ltc2992_state *st, u32 reg, u32 channel, long *val)
> +{
> +	u32 reg_val;
> +	int ret;
> +
> +	ret = ltc2992_read_reg(st, reg, 3, &reg_val);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = mul_u64_u32_div(reg_val, LTC2992_VADC_UV_LSB * LTC2992_IADC_NANOV_LSB,
> +			       st->r_sense_uohm[channel] * 1000);
> +
> +	return 0;
> +}
> +
> +static int ltc2992_set_power(struct ltc2992_state *st, u32 reg, u32 channel, long val)
> +{
> +	u32 reg_val;
> +
> +	reg_val = mul_u64_u32_div(val, st->r_sense_uohm[channel] * 1000,
> +				  LTC2992_VADC_UV_LSB * LTC2992_IADC_NANOV_LSB);
> +
> +	return ltc2992_write_reg(st, reg, 3, reg_val);
> +}
> +
> +static int ltc2992_read_power(struct device *dev, u32 attr, int channel, long *val)
> +{
> +	struct ltc2992_state *st = dev_get_drvdata(dev);
> +	u32 reg_val;
> +	u32 reg;
> +	int ret;
> +
> +	switch (attr) {
> +	case hwmon_power_input:
> +		reg = LTC2992_POWER(channel);
> +		break;
> +	case hwmon_power_min:
> +		reg = LTC2992_POWER_MIN(channel);
> +		break;
> +	case hwmon_power_max:
> +		reg = LTC2992_POWER_MAX(channel);
> +		break;
> +	case hwmon_power_min_alarm:
> +		reg = LTC2992_POWER_MIN_THRESH(channel);
> +		break;
> +	case hwmon_power_max_alarm:
> +		reg = LTC2992_POWER_MAX_THRESH(channel);
> +		break;
> +	case hwmon_power_alarm:
> +		ret = ltc2992_read_reg(st, LTC2992_POWER_FAULT(channel), 1, &reg_val);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = !!(reg_val & LTC2992_POWER_FAULT_MSK);
> +
> +		reg_val &= ~LTC2992_POWER_FAULT_MSK;
> +		return ltc2992_write_reg(st, LTC2992_POWER_FAULT(channel), 1, reg_val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return ltc2992_get_power(st, reg, channel, val);
> +}
> +
> +static int ltc2992_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
> +			long *val)
> +{
> +	switch (type) {
> +	case hwmon_in:
> +		return ltc2992_read_in(dev, attr, channel, val);
> +	case hwmon_curr:
> +		return ltc2992_read_curr(dev, attr, channel, val);
> +	case hwmon_power:
> +		return ltc2992_read_power(dev, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int ltc2992_write_curr(struct device *dev, u32 attr, int channel, long val)
> +{
> +	struct ltc2992_state *st = dev_get_drvdata(dev);
> +	u32 reg = 0;

Unnecessary initialization.

> +
> +	switch (attr) {
> +	case hwmon_curr_min_alarm:
> +		reg = LTC2992_DSENSE_MIN_THRESH(channel);
> +		break;
> +	case hwmon_curr_max_alarm:
> +		reg = LTC2992_DSENSE_MAX_THRESH(channel);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return ltc2992_set_current(st, reg, channel, val);
> +}
> +
> +static int ltc2992_write_gpios_in(struct device *dev, u32 attr, int nr_gpio, long val)
> +{
> +	struct ltc2992_state *st = dev_get_drvdata(dev);
> +	u32 reg;
> +
> +	switch (attr) {
> +	case hwmon_in_min_alarm:
> +		reg = ltc2992_gpio_addr_map[nr_gpio].min_thresh;
> +		break;
> +	case hwmon_in_max_alarm:
> +		reg = ltc2992_gpio_addr_map[nr_gpio].max_thresh;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return ltc2992_set_voltage(st, reg, LTC2992_VADC_GPIO_UV_LSB, val);
> +}
> +
> +static int ltc2992_write_in(struct device *dev, u32 attr, int channel, long val)
> +{
> +	struct ltc2992_state *st = dev_get_drvdata(dev);
> +	u32 reg = 0;
> +
> +	if (channel > 1)
> +		return ltc2992_write_gpios_in(dev, attr, channel - 2, val);
> +
> +	switch (attr) {
> +	case hwmon_curr_min_alarm:
> +		reg = LTC2992_SENSE_MIN_THRESH(channel);
> +		break;
> +	case hwmon_curr_max_alarm:
> +		reg = LTC2992_SENSE_MAX_THRESH(channel);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return ltc2992_set_voltage(st, reg, LTC2992_VADC_UV_LSB, val);
> +}
> +
> +static int ltc2992_write_power(struct device *dev, u32 attr, int channel, long val)
> +{
> +	struct ltc2992_state *st = dev_get_drvdata(dev);
> +	u32 reg = 0;
> +
> +	switch (attr) {
> +	case hwmon_power_min_alarm:
> +		reg = LTC2992_POWER_MIN_THRESH(channel);
> +		break;
> +	case hwmon_power_max_alarm:
> +		reg = LTC2992_POWER_MAX_THRESH(channel);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return ltc2992_set_power(st, reg, channel, val);
> +}
> +
> +static int ltc2992_write(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel,
> +			 long val)
> +{
> +	switch (type) {
> +	case hwmon_in:
> +		return ltc2992_write_in(dev, attr, channel, val);
> +	case hwmon_curr:
> +		return ltc2992_write_curr(dev, attr, channel, val);
> +	case hwmon_power:
> +		return ltc2992_write_power(dev, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static const struct hwmon_ops ltc2992_hwmon_ops = {
> +	.is_visible = ltc2992_is_visible,
> +	.read = ltc2992_read,
> +	.write = ltc2992_write,
> +};
> +
> +static const u32 ltc2992_in_config[] = {
> +	HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_MIN_ALARM |
> +	HWMON_I_MAX_ALARM | HWMON_I_ALARM,
> +	HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_MIN_ALARM |
> +	HWMON_I_MAX_ALARM | HWMON_I_ALARM,
> +	HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_MIN_ALARM |
> +	HWMON_I_MAX_ALARM | HWMON_I_ALARM,
> +	HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_MIN_ALARM |
> +	HWMON_I_MAX_ALARM | HWMON_I_ALARM,
> +	HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_MIN_ALARM |
> +	HWMON_I_MAX_ALARM | HWMON_I_ALARM,
> +	HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX | HWMON_I_MIN_ALARM |
> +	HWMON_I_MAX_ALARM | HWMON_I_ALARM,
> +	0
> +};
> +
> +static const struct hwmon_channel_info ltc2992_in = {
> +	.type = hwmon_in,
> +	.config = ltc2992_in_config,
> +};
> +
> +static const u32 ltc2992_curr_config[] = {
> +	HWMON_C_INPUT | HWMON_C_MIN | HWMON_C_MAX | HWMON_C_MIN_ALARM | HWMON_C_MAX_ALARM |
> +	HWMON_C_ALARM,
> +	HWMON_C_INPUT | HWMON_C_MIN | HWMON_C_MAX | HWMON_C_MIN_ALARM | HWMON_C_MAX_ALARM |
> +	HWMON_C_ALARM,
> +	0
> +};
> +
> +static const struct hwmon_channel_info ltc2992_curr = {
> +	.type = hwmon_curr,
> +	.config = ltc2992_curr_config,
> +};
> +
> +static const u32 ltc2992_power_config[] = {
> +	HWMON_P_INPUT | HWMON_P_MIN | HWMON_P_MAX | HWMON_P_MIN_ALARM | HWMON_P_MAX_ALARM |
> +	HWMON_P_ALARM,
> +	HWMON_P_INPUT | HWMON_P_MIN | HWMON_P_MAX | HWMON_P_MIN_ALARM | HWMON_P_MAX_ALARM |
> +	HWMON_P_ALARM,
> +	0
> +};
> +
> +static const struct hwmon_channel_info ltc2992_power = {
> +	.type = hwmon_power,
> +	.config = ltc2992_power_config,
> +};
> +
> +static const struct hwmon_channel_info *ltc2992_info[] = {
> +	&ltc2992_in,
> +	&ltc2992_curr,
> +	&ltc2992_power,
> +	NULL
> +};
> +
> +static const struct hwmon_chip_info ltc2992_chip_info = {
> +	.ops = &ltc2992_hwmon_ops,
> +	.info = ltc2992_info,
> +};
> +
> +static const struct regmap_config ltc2992_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = 0xE8,
> +};
> +
> +static int ltc2992_parse_dt(struct ltc2992_state *st)
> +{
> +	struct fwnode_handle *fwnode;
> +	struct fwnode_handle *child;
> +	u32 addr;
> +	u32 val;
> +	int ret;
> +
> +	fwnode = dev_fwnode(&st->client->dev);

This is the only use of st->client. Just pass dev as parameter
instead.

> +
> +	fwnode_for_each_available_child_node(fwnode, child) {
> +		ret = fwnode_property_read_u32(child, "reg", &addr);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (addr > 1)
> +			return -EINVAL;
> +
> +		ret = fwnode_property_read_u32(child, "shunt-resistor-micro-ohms", &val);
> +		if (!ret)
> +			st->r_sense_uohm[addr] = val;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ltc2992_i2c_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> +	struct device *hwmon_dev;
> +	struct ltc2992_state *st;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> +		return -ENODEV;
> +
> +	st = devm_kzalloc(&client->dev, sizeof(*st), GFP_KERNEL);
> +	if (!st)
> +		return -ENOMEM;
> +
> +	st->client = client;
> +	st->regmap = devm_regmap_init_i2c(client, &ltc2992_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return PTR_ERR(st->regmap);
> +
> +	ret = ltc2992_parse_dt(st);
> +	if (ret < 0)
> +		return ret;
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(&client->dev, client->name, st,
> +							 &ltc2992_chip_info, NULL);
> +
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct of_device_id ltc2992_of_match[] = {
> +	{ .compatible = "adi,ltc2992" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ltc2992_of_match);
> +
> +static const struct i2c_device_id ltc2992_i2c_id[] = {
> +	{"ltc2992", 0},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, ltc2992_i2c_id);
> +
> +static struct i2c_driver ltc2992_i2c_driver = {
> +	.driver = {
> +		.name = "ltc2992",
> +		.of_match_table = ltc2992_of_match,
> +	},
> +	.probe    = ltc2992_i2c_probe,

The probe function is being deprecated. Please use probe_new instead.

> +	.id_table = ltc2992_i2c_id,
> +};
> +
> +module_i2c_driver(ltc2992_i2c_driver);
> +
> +MODULE_AUTHOR("Alexandru Tachici <alexandru.tachici@...log.com>");
> +MODULE_DESCRIPTION("Hwmon driver for Linear Technology 2992");
> +MODULE_LICENSE("Dual BSD/GPL");

Powered by blists - more mailing lists