Message-ID: <20201118015503.GA5636@taoren-ubuntu-R90MNF91>
Date:   Tue, 17 Nov 2020 17:55:04 -0800
From:   Tao Ren <rentao.bupt@...il.com>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Jean Delvare <jdelvare@...e.com>, Jonathan Corbet <corbet@....net>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        linux-hwmon@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        bpf@...r.kernel.org, openbmc@...ts.ozlabs.org, taoren@...com,
        mikechoi@...com
Subject: Re: [PATCH 1/2] hwmon: (max127) Add Maxim MAX127 hardware monitoring
 driver

Hi Guenter,

Thanks for pointing out these problems. I'm working on the comments and
will send out v2 soon.


Cheers,

Tao

On Mon, Nov 16, 2020 at 09:13:52PM -0800, Guenter Roeck wrote:
> On Mon, Nov 16, 2020 at 05:09:43PM -0800, rentao.bupt@...il.com wrote:
> > From: Tao Ren <rentao.bupt@...il.com>
> > 
> > Add hardware monitoring driver for the Maxim MAX127 chip.
> > 
> > MAX127 min/max range handling code is inspired by the max197 driver.
> > 
> > Signed-off-by: Tao Ren <rentao.bupt@...il.com>
> > ---
> >  drivers/hwmon/Kconfig  |   9 ++
> >  drivers/hwmon/Makefile |   1 +
> >  drivers/hwmon/max127.c | 286 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 296 insertions(+)
> >  create mode 100644 drivers/hwmon/max127.c
> > 
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 9d600e0c5584..716df51edc87 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -950,6 +950,15 @@ config SENSORS_MAX1111
> >  	  This driver can also be built as a module. If so, the module
> >  	  will be called max1111.
> >  
> > +config SENSORS_MAX127
> > +	tristate "Maxim MAX127 12-bit 8-channel Data Acquisition System"
> > +	depends on I2C
> > +	help
> > +	  Say y here to support Maxim's MAX127 DAS chips.
> > +
> > +	  This driver can also be built as a module. If so, the module
> > +	  will be called max127.
> > +
> >  config SENSORS_MAX16065
> >  	tristate "Maxim MAX16065 System Manager and compatibles"
> >  	depends on I2C
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index 1083bbfac779..01ca5d3fbad4 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -127,6 +127,7 @@ obj-$(CONFIG_SENSORS_LTC4260)	+= ltc4260.o
> >  obj-$(CONFIG_SENSORS_LTC4261)	+= ltc4261.o
> >  obj-$(CONFIG_SENSORS_LTQ_CPUTEMP) += ltq-cputemp.o
> >  obj-$(CONFIG_SENSORS_MAX1111)	+= max1111.o
> > +obj-$(CONFIG_SENSORS_MAX127)	+= max127.o
> >  obj-$(CONFIG_SENSORS_MAX16065)	+= max16065.o
> >  obj-$(CONFIG_SENSORS_MAX1619)	+= max1619.o
> >  obj-$(CONFIG_SENSORS_MAX1668)	+= max1668.o
> > diff --git a/drivers/hwmon/max127.c b/drivers/hwmon/max127.c
> > new file mode 100644
> > index 000000000000..df74a95bcf28
> > --- /dev/null
> > +++ b/drivers/hwmon/max127.c
> > @@ -0,0 +1,286 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Hardware monitoring driver for MAX127.
> > + *
> > + * Copyright (c) 2020 Facebook Inc.
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/module.h>
> > +#include <linux/sysfs.h>
> > +
> > +/* MAX127 Control Byte. */
> > +#define MAX127_CTRL_START	BIT(7)
> > +#define MAX127_CTRL_SEL_OFFSET	4
> 
> That would better be named _SHIFT.
> 
> > +#define MAX127_CTRL_RNG		BIT(3)
> > +#define MAX127_CTRL_BIP		BIT(2)
> > +#define MAX127_CTRL_PD1		BIT(1)
> > +#define MAX127_CTRL_PD0		BIT(0)
> > +
> > +#define MAX127_NUM_CHANNELS	8
> > +#define MAX127_SET_CHANNEL(ch)	(((ch) & 7) << (MAX127_CTRL_SEL_OFFSET))
> 
> () around MAX127_CTRL_SEL_OFFSET is unnecessary.
> 
> > +
> > +#define MAX127_INPUT_LIMIT	10	/* 10V */
> > +
> > +/*
> > + * MAX127 returns 2 bytes at read:
> > + *   - the first byte contains data[11:4].
> > + *   - the second byte contains data[3:0] (MSB) and 4 dummy 0s (LSB).
> > + */
> > +#define MAX127_DATA1_SHIFT	4
> > +
> > +struct max127_data {
> > +	struct mutex lock;
> > +	struct i2c_client *client;
> > +	int input_limit;
> > +	u8 ctrl_byte[MAX127_NUM_CHANNELS];
> > +};
> > +
> > +static int max127_select_channel(struct max127_data *data, int channel)
> > +{
> > +	int status;
> > +	struct i2c_client *client = data->client;
> > +	struct i2c_msg msg = {
> > +		.addr = client->addr,
> > +		.flags = 0,
> > +		.len = 1,
> > +		.buf = &data->ctrl_byte[channel],
> > +	};
> > +
> > +	status = i2c_transfer(client->adapter, &msg, 1);
> > +	if (status != 1)
> > +		return status;
> > +
> 
> Other drivers assume that this function can return 0. Please
> take that into account as well.
> 
> > +	return 0;
> > +}
> > +
> > +static int max127_read_channel(struct max127_data *data, int channel, u16 *vin)
> > +{
> > +	int status;
> > +	u8 i2c_data[2];
> > +	struct i2c_client *client = data->client;
> > +	struct i2c_msg msg = {
> > +		.addr = client->addr,
> > +		.flags = I2C_M_RD,
> > +		.len = 2,
> > +		.buf = i2c_data,
> > +	};
> > +
> > +	status = i2c_transfer(client->adapter, &msg, 1);
> > +	if (status != 1)
> > +		return status;
> 
> Same as above.
> 
> > +
> > +	*vin = ((i2c_data[0] << 8) | i2c_data[1]) >> MAX127_DATA1_SHIFT;
> 
> THis seems wrong. D4..D11 end up in but 8..15, and D0..D3 end up in bit
> 0..3. Seems to me the upper byte should be left shifted 4 bit.
> The result then needs to be scaled to mV (see below).
> 
> Also, for consistency I would suggest to either use () for both
> parts of the logical or operation or for none.
> 
> > +	return 0;
> > +}
> > +
> > +static ssize_t max127_input_show(struct device *dev,
> > +				 struct device_attribute *dev_attr,
> > +				 char *buf)
> > +{
> > +	u16 vin;
> > +	int status;
> > +	struct max127_data *data = dev_get_drvdata(dev);
> > +	struct sensor_device_attribute *attr = to_sensor_dev_attr(dev_attr);
> > +
> > +	if (mutex_lock_interruptible(&data->lock))
> > +		return -ERESTARTSYS;
> 
> I don't think the _interruptible is warranted in this driver.
> 
> > +
> > +	status = max127_select_channel(data, attr->index);
> > +	if (status)
> > +		goto exit;
> > +
> > +	status = max127_read_channel(data, attr->index, &vin);
> > +	if (status)
> > +		goto exit;
> > +
> > +	status = sprintf(buf, "%u", vin);
> 
> This is not correct. The ABI expects values in milli-Volt, and per datasheet
> the values need to be scaled depending on polarity and range settings (see
> table 3 in datasheet). Also, if the range includes negative numbers,
> the reported voltage can obviously be negative. That means %u (and u16)
> can not be correct. "Transfer Function" in the datasheet describes how to
> convert/scale the received data.
> 
> > +
> > +exit:
> > +	mutex_unlock(&data->lock);
> > +	return status;
> > +}
> > +
> > +static ssize_t max127_range_show(struct device *dev,
> > +				 struct device_attribute *dev_attr,
> > +				 char *buf)
> > +{
> > +	u8 ctrl, rng_bip;
> > +	struct max127_data *data = dev_get_drvdata(dev);
> > +	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(dev_attr);
> > +	int rng_type = attr->nr;	/* 0 for min, 1 for max */
> > +	int channel = attr->index;
> > +	int full = data->input_limit;
> > +	int half = full / 2;
> > +	int range_table[4][2] = {
> > +		[0] = {0, half},	/* RNG=0, BIP=0 */
> > +		[1] = {-half, half},	/* RNG=0, BIP=1 */
> > +		[2] = {0, full},	/* RNG=1, BIP=0 */
> > +		[3] = {-full, full},	/* RNG=1, BIP=1 */
> > +	};
> 
> This can be a static const table. The variables 'full' and 'half'
> are effectively constants and not really needed.
> 
> > +
> > +	if (mutex_lock_interruptible(&data->lock))
> > +		return -ERESTARTSYS;
> > +	ctrl = data->ctrl_byte[channel];
> > +	mutex_unlock(&data->lock);
> 
> This lock is only needed because "ctrl" is written piece by piece.
> I would suggest to rewrite the store function to write ctrl atomically.
> Then the lock here is no longer needed.
> 
> > +
> > +	rng_bip = (ctrl >> 2) & 3;
> > +	return sprintf(buf, "%d", range_table[rng_bip][rng_type]);
> > +}
> > +
> > +static void max127_set_range(struct max127_data *data, int channel)
> > +{
> > +	data->ctrl_byte[channel] |= MAX127_CTRL_RNG;
> > +}
> > +
> > +static void max127_clear_range(struct max127_data *data, int channel)
> > +{
> > +	data->ctrl_byte[channel] &= ~MAX127_CTRL_RNG;
> > +}
> > +
> > +static void max127_set_polarity(struct max127_data *data, int channel)
> > +{
> > +	data->ctrl_byte[channel] |= MAX127_CTRL_BIP;
> > +}
> > +
> > +static void max127_clear_polarity(struct max127_data *data, int channel)
> > +{
> > +	data->ctrl_byte[channel] &= ~MAX127_CTRL_BIP;
> > +}
> > +
> > +static ssize_t max127_range_store(struct device *dev,
> > +				  struct device_attribute *devattr,
> > +				  const char *buf,
> > +				  size_t count)
> > +{
> > +	struct max127_data *data = dev_get_drvdata(dev);
> > +	struct sensor_device_attribute_2 *attr = to_sensor_dev_attr_2(devattr);
> > +	int rng_type = attr->nr;	/* 0 for min, 1 for max */
> > +	int channel = attr->index;
> > +	int full = data->input_limit;
> > +	int half = full / 2;
> > +	long input, output;
> > +
> > +	if (kstrtol(buf, 0, &input))
> > +		return -EINVAL;
> > +
> > +	if (rng_type == 0) {	/* min input */
> > +		if (input <= -full)
> > +			output = -full;
> > +		else if (input < 0)
> > +			output = -half;
> > +		else
> > +			output = 0;
> > +	} else {		/* max input */
> > +		output = (input >= full) ? full : half;
> > +	}
> > +
> 
> With the _info API, I would suggest to separate min and max functions.
> This would both simplify the code and make it easier to read and
> review. 
> 
> > +	if (mutex_lock_interruptible(&data->lock))
> > +		return -ERESTARTSYS;
> 
> This should be rewritten to update "ctrl" in one step.
> Something like
> 
> 	u8 ctrl;
> 	...
> 	ctrl = data->ctrl_byte[channel];
> 	if (output == -MAX127_INPUT_LIMIT)
> 		ctrl |= MAX127_CTRL_RNG | MAX127_CTRL_BIP;
> 	else if (output == -half)
> 		ctrl |= MAX127_CTRL_BIP;
> 		ctrl &= ~MAX127_CTRL_RNG;
> 	else if (output == 0)
> 		ctrl &= ~MAX127_CTRL_BIP;
> 	else lf (output == half)
> 		ctrl &= ~MAX127_CTRL_RNG;
> 	else
> 		ctrl |= MAX127_CTRL_RNG;
> 
> 	data->ctrl_byte[channel] = ctrl;
> 
> I would suggest to separate the min and max functions, though.
> 
> > +
> > +	if (output == -full) {
> > +		max127_set_polarity(data, channel);
> > +		max127_set_range(data, channel);
> > +	} else if (output == -half) {
> > +		max127_set_polarity(data, channel);
> > +		max127_clear_range(data, channel);
> > +	} else if (output == 0) {
> > +		max127_clear_polarity(data, channel);
> > +	} else if (output == half) {
> > +		max127_clear_range(data, channel);
> > +	} else {
> > +		max127_set_range(data, channel);
> > +	}
> > +
> > +	mutex_unlock(&data->lock);
> > +
> > +	return count;
> > +}
> > +
> > +#define MAX127_SENSOR_DEV_ATTR_DEF(ch)					   \
> > +	static SENSOR_DEVICE_ATTR_RO(in##ch##_input, max127_input, ch);	   \
> > +	static SENSOR_DEVICE_ATTR_2_RW(in##ch##_min, max127_range, 0, ch); \
> > +	static SENSOR_DEVICE_ATTR_2_RW(in##ch##_max, max127_range, 1, ch)
> > +
> > +MAX127_SENSOR_DEV_ATTR_DEF(0);
> > +MAX127_SENSOR_DEV_ATTR_DEF(1);
> > +MAX127_SENSOR_DEV_ATTR_DEF(2);
> > +MAX127_SENSOR_DEV_ATTR_DEF(3);
> > +MAX127_SENSOR_DEV_ATTR_DEF(4);
> > +MAX127_SENSOR_DEV_ATTR_DEF(5);
> > +MAX127_SENSOR_DEV_ATTR_DEF(6);
> > +MAX127_SENSOR_DEV_ATTR_DEF(7);
> > +
> > +#define MAX127_SENSOR_DEVICE_ATTR(ch)			\
> > +	&sensor_dev_attr_in##ch##_input.dev_attr.attr,	\
> > +	&sensor_dev_attr_in##ch##_min.dev_attr.attr,	\
> > +	&sensor_dev_attr_in##ch##_max.dev_attr.attr
> > +
> > +static struct attribute *max127_attrs[] = {
> > +	MAX127_SENSOR_DEVICE_ATTR(0),
> > +	MAX127_SENSOR_DEVICE_ATTR(1),
> > +	MAX127_SENSOR_DEVICE_ATTR(2),
> > +	MAX127_SENSOR_DEVICE_ATTR(3),
> > +	MAX127_SENSOR_DEVICE_ATTR(4),
> > +	MAX127_SENSOR_DEVICE_ATTR(5),
> > +	MAX127_SENSOR_DEVICE_ATTR(6),
> > +	MAX127_SENSOR_DEVICE_ATTR(7),
> > +	NULL,
> > +};
> > +
> > +ATTRIBUTE_GROUPS(max127);
> > +
> > +static const struct attribute_group max127_attr_groups = {
> > +	.attrs = max127_attrs,
> > +};
> > +
> > +static int max127_probe(struct i2c_client *client,
> > +			const struct i2c_device_id *id)
> > +{
> > +	int i;
> > +	struct device *hwmon_dev;
> > +	struct max127_data *data;
> > +	struct device *dev = &client->dev;
> > +
> > +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	data->client = client;
> > +	mutex_init(&data->lock);
> > +	data->input_limit = MAX127_INPUT_LIMIT;
> 
> What is the point of input_limit ? It is never modified.
> Why not use MAX127_INPUT_LIMIT directly where needed ?
> 
> > +	for (i = 0; i < ARRAY_SIZE(data->ctrl_byte); i++)
> > +		data->ctrl_byte[i] = (MAX127_CTRL_START |
> > +				      MAX127_SET_CHANNEL(i));
> > +
> > +	hwmon_dev = devm_hwmon_device_register_with_groups(dev,
> > +				client->name, data, max127_groups);
> 
> Please use the devm_hwmon_device_register_with_info() API.
> 
> Thanks,
> Guenter
> 
> > +
> > +	return PTR_ERR_OR_ZERO(hwmon_dev);
> > +}
> > +
> > +static const struct i2c_device_id max127_id[] = {
> > +	{ "max127", 0 },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, max127_id);
> > +
> > +static struct i2c_driver max127_driver = {
> > +	.class		= I2C_CLASS_HWMON,
> > +	.driver = {
> > +		.name	= "max127",
> > +	},
> > +	.probe		= max127_probe,
> > +	.id_table	= max127_id,
> > +};
> > +
> > +module_i2c_driver(max127_driver);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Mike Choi <mikechoi@...com>");
> > +MODULE_AUTHOR("Tao Ren <rentao.bupt@...il.com>");
> > +MODULE_DESCRIPTION("MAX127 Hardware Monitoring driver");
> > -- 
> > 2.17.1
> > 

Powered by blists - more mailing lists