On Mon, Nov 23, 2020 at 05:16:41AM -0800, Guenter Roeck wrote: > On Sun, Nov 22, 2020 at 11:54:49PM -0800, Tao Ren wrote: > > On Sun, Nov 22, 2020 at 10:33:42AM -0800, Guenter Roeck wrote: > > > On Thu, Nov 19, 2020 at 09:53:23AM -0800, [email protected] wrote: > > > > From: Tao Ren <[email protected]> > > > > > > > > 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 <[email protected]> > > > > > > Nice cleanup. Couple of minor comments. > > > > > > Thanks, > > > Guenter > > > > > > > --- > > > > Changes in v3: > > > > - no code change. xdp maintainers were removed from to/cc list. > > > > Changes in v2: > > > > - replace devm_hwmon_device_register_with_groups() with > > > > devm_hwmon_device_register_with_info() API. > > > > - divide min/max read and write methods to separate functions. > > > > - fix raw-to-vin conversion logic. > > > > - refine ctrl_byte handling so mutex is not needed to protect the > > > > byte. > > > > - improve i2c_transfer() error handling. > > > > - a few other improvements (comments, variable naming, and etc.). > > > > > > > > drivers/hwmon/Kconfig | 9 ++ > > > > drivers/hwmon/Makefile | 1 + > > > > drivers/hwmon/max127.c | 346 +++++++++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 356 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..3df4c225a6a2 > > > > --- /dev/null > > > > +++ b/drivers/hwmon/max127.c > > > > @@ -0,0 +1,346 @@ > > > > +// 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> > > > > > > Not needed. > > > > > > > +#include <linux/i2c.h> > > > > +#include <linux/init.h> > > > > +#include <linux/module.h> > > > > +#include <linux/sysfs.h> > > > > > > Not needed. > > > > Thanks for pointing it out. Both includes are deleted in v4. > > > > > > > > > + > > > > +/* > > > > + * MAX127 Control Byte. Refer to MAX127 datasheet, Table 1 > > > > "Control-Byte > > > > + * Format" for details. > > > > + */ > > > > +#define MAX127_CTRL_START BIT(7) > > > > +#define MAX127_CTRL_SEL_SHIFT 4 > > > > +#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_SHIFT) > > > > + > > > > +/* > > > > + * MAX127 channel input ranges. Refer to MAX127 datasheet, Table 3 > > > > "Range > > > > + * and Polarity Selection" for details. > > > > + */ > > > > +#define MAX127_FULL_RANGE 10000 /* 10V */ > > > > +#define MAX127_HALF_RANGE 5000 /* 5V */ > > > > + > > > > +/* > > > > + * 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). > > > > + * Refer to MAX127 datasheet, "Read a Conversion (Read Cycle)" section > > > > + * for details. > > > > + */ > > > > +#define MAX127_DATA_LEN 2 > > > > +#define MAX127_DATA_SHIFT 4 > > > > + > > > > +#define MAX127_SIGN_BIT BIT(11) > > > > + > > > > +struct max127_data { > > > > + struct mutex lock; > > > > + struct i2c_client *client; > > > > + u8 ctrl_byte[MAX127_NUM_CHANNELS]; > > > > +}; > > > > + > > > > +static int max127_select_channel(struct i2c_client *client, u8 > > > > ctrl_byte) > > > > +{ > > > > + int status; > > > > + struct i2c_msg msg = { > > > > + .addr = client->addr, > > > > + .flags = 0, > > > > + .len = sizeof(ctrl_byte), > > > > + .buf = &ctrl_byte, > > > > + }; > > > > + > > > > + status = i2c_transfer(client->adapter, &msg, 1); > > > > + if (status < 0) > > > > + return status; > > > > + else if (status != 1) > > > > > > else after return is not needed. > > > > > > > + return -EIO; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int max127_read_channel(struct i2c_client *client, long *val) > > > > +{ > > > > + int status; > > > > + u8 i2c_data[MAX127_DATA_LEN]; > > > > + struct i2c_msg msg = { > > > > + .addr = client->addr, > > > > + .flags = I2C_M_RD, > > > > + .len = sizeof(i2c_data), > > > > + .buf = i2c_data, > > > > + }; > > > > + > > > > + status = i2c_transfer(client->adapter, &msg, 1); > > > > + if (status < 0) > > > > + return status; > > > > + else if (status != 1) > > > > > > else after return is not needed. > > > > I'm not sure if I understand your suggestion correctly, and I added this > > branch to handle the case when i2c_transfer() returns 0, which means the > > message is not transferred. Perhaps it would never happen? but anyhow, I > > just simplified the check in v4; please kindly review and let me know if > > you have further suggestions. > > > > if (status < 0) > return status; > if (status != 1) > return -EIO; > > Again, else after return is not needed (and static analyzers will complain > about it). > > Guenter
Ahh, I get it now (I didn't know static analyzers would complain about it). Thanks for the sharing, and it's fixed in v5. Cheers, Tao
