[PATCH v2 0/2] hwmon: (max127) Add Maxim MAX127 hardware monitoring

[PATCH v2 0/2] hwmon: (max127) Add Maxim MAX127 hardware monitoring

[rentao.bupt]

From: Tao Ren <rentao.bupt@gmail.com>

The patch series adds hardware monitoring driver for the Maxim MAX127
chip.

Patch #1 adds the max127 hardware monitoring driver, and patch #2 adds
documentation for the driver.

Tao Ren (2):
  hwmon: (max127) Add Maxim MAX127 hardware monitoring driver
  docs: hwmon: Document max127 driver

 Documentation/hwmon/index.rst  |   1 +
 Documentation/hwmon/max127.rst |  45 +++++
 drivers/hwmon/Kconfig          |   9 +
 drivers/hwmon/Makefile         |   1 +
 drivers/hwmon/max127.c         | 346 +++++++++++++++++++++++++++++++++
 5 files changed, 402 insertions(+)
 create mode 100644 Documentation/hwmon/max127.rst
 create mode 100644 drivers/hwmon/max127.c

-- 
2.17.1

[PATCH v2 1/2] hwmon: (max127) Add Maxim MAX127 hardware monitoring driver

[rentao.bupt]

From: Tao Ren <rentao.bupt@gmail.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@gmail.com>
---
 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>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/sysfs.h>
+
+/*
+ * 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)
+		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)
+		return -EIO;
+
+	*val = (i2c_data[1] >> MAX127_DATA_SHIFT) |
+		((u16)i2c_data[0] << MAX127_DATA_SHIFT);
+	return 0;
+}
+
+static long max127_process_raw(u8 ctrl_byte, long raw)
+{
+	long scale, weight;
+
+	/*
+	 * MAX127's data coding is binary in unipolar mode with 1 LSB =
+	 * (Full-Scale/4096) and two’s complement binary in bipolar mode
+	 * with 1 LSB = [(2 x |FS|)/4096].
+	 * Refer to MAX127 datasheet, "Transfer Function" section for
+	 * details.
+	 */
+	scale = (ctrl_byte & MAX127_CTRL_RNG) ? MAX127_FULL_RANGE :
+						MAX127_HALF_RANGE;
+	if (ctrl_byte & MAX127_CTRL_BIP) {
+		weight = (raw & MAX127_SIGN_BIT);
+		raw &= ~MAX127_SIGN_BIT;
+		raw -= weight;
+		raw *= 2;
+	}
+
+	return raw * scale / 4096;
+}
+
+static int max127_read_input(struct max127_data *data, int channel, long *val)
+{
+	long raw;
+	int status;
+	struct i2c_client *client = data->client;
+	u8 ctrl_byte = data->ctrl_byte[channel];
+
+	mutex_lock(&data->lock);
+
+	status = max127_select_channel(client, ctrl_byte);
+	if (status)
+		goto exit;
+
+	status = max127_read_channel(client, &raw);
+	if (status)
+		goto exit;
+
+	*val = max127_process_raw(ctrl_byte, raw);
+
+exit:
+	mutex_unlock(&data->lock);
+	return status;
+}
+
+static int max127_read_min(struct max127_data *data, int channel, long *val)
+{
+	u8 rng_bip = (data->ctrl_byte[channel] >> 2) & 3;
+	static const int min_input_map[4] = {
+		0,			/* RNG=0, BIP=0 */
+		-MAX127_HALF_RANGE,	/* RNG=0, BIP=1 */
+		0,			/* RNG=1, BIP=0 */
+		-MAX127_FULL_RANGE,	/* RNG=1, BIP=1 */
+	};
+
+	*val = min_input_map[rng_bip];
+	return 0;
+}
+
+static int max127_read_max(struct max127_data *data, int channel, long *val)
+{
+	u8 rng_bip = (data->ctrl_byte[channel] >> 2) & 3;
+	static const int max_input_map[4] = {
+		MAX127_HALF_RANGE,	/* RNG=0, BIP=0 */
+		MAX127_HALF_RANGE,	/* RNG=0, BIP=1 */
+		MAX127_FULL_RANGE,	/* RNG=1, BIP=0 */
+		MAX127_FULL_RANGE,	/* RNG=1, BIP=1 */
+	};
+
+	*val = max_input_map[rng_bip];
+	return 0;
+}
+
+static int max127_write_min(struct max127_data *data, int channel, long val)
+{
+	u8 ctrl;
+
+	ctrl = data->ctrl_byte[channel];
+	if (val <= -MAX127_FULL_RANGE) {
+		ctrl |= (MAX127_CTRL_RNG | MAX127_CTRL_BIP);
+	} else if (val < 0) {
+		ctrl |= MAX127_CTRL_BIP;
+		ctrl &= ~MAX127_CTRL_RNG;
+	} else {
+		ctrl &= ~MAX127_CTRL_BIP;
+	}
+	data->ctrl_byte[channel] = ctrl;
+
+	return 0;
+}
+
+static int max127_write_max(struct max127_data *data, int channel, long val)
+{
+	if (val >= MAX127_FULL_RANGE)
+		data->ctrl_byte[channel] |= MAX127_CTRL_RNG;
+	else
+		data->ctrl_byte[channel] &= ~MAX127_CTRL_RNG;
+
+	return 0;
+}
+
+static umode_t max127_is_visible(const void *_data,
+				 enum hwmon_sensor_types type,
+				 u32 attr, int channel)
+{
+	if (type == hwmon_in) {
+		switch (attr) {
+		case hwmon_in_input:
+			return 0444;
+
+		case hwmon_in_min:
+		case hwmon_in_max:
+			return 0644;
+
+		default:
+			break;
+		}
+	}
+
+	return 0;
+}
+
+static int max127_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
+{
+	int status;
+	struct max127_data *data = dev_get_drvdata(dev);
+
+	if (type != hwmon_in)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_in_input:
+		status = max127_read_input(data, channel, val);
+		break;
+
+	case hwmon_in_min:
+		status = max127_read_min(data, channel, val);
+		break;
+
+	case hwmon_in_max:
+		status = max127_read_max(data, channel, val);
+		break;
+
+	default:
+		status = -EOPNOTSUPP;
+		break;
+	}
+
+	return status;
+}
+
+static int max127_write(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long val)
+{
+	int status;
+	struct max127_data *data = dev_get_drvdata(dev);
+
+	if (type != hwmon_in)
+		return -EOPNOTSUPP;
+
+	switch (attr) {
+	case hwmon_in_min:
+		status = max127_write_min(data, channel, val);
+		break;
+
+	case hwmon_in_max:
+		status = max127_write_max(data, channel, val);
+		break;
+
+	default:
+		status = -EOPNOTSUPP;
+		break;
+	}
+
+	return status;
+}
+
+static const struct hwmon_ops max127_hwmon_ops = {
+	.is_visible = max127_is_visible,
+	.read = max127_read,
+	.write = max127_write,
+};
+
+static const struct hwmon_channel_info *max127_info[] = {
+	HWMON_CHANNEL_INFO(in,
+			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX,
+			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX,
+			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX,
+			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX,
+			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX,
+			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX,
+			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX,
+			   HWMON_I_INPUT | HWMON_I_MIN | HWMON_I_MAX),
+	NULL,
+};
+
+static const struct hwmon_chip_info max127_chip_info = {
+	.ops = &max127_hwmon_ops,
+	.info = max127_info,
+};
+
+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);
+	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_info(dev, client->name,
+							 data,
+							 &max127_chip_info,
+							 NULL);
+
+	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@fb.com>");
+MODULE_AUTHOR("Tao Ren <rentao.bupt@gmail.com>");
+MODULE_DESCRIPTION("MAX127 Hardware Monitoring driver");
-- 
2.17.1

[PATCH v2 2/2] docs: hwmon: Document max127 driver

[rentao.bupt]

From: Tao Ren <rentao.bupt@gmail.com>

Add documentation for the max127 hardware monitoring driver.

Signed-off-by: Tao Ren <rentao.bupt@gmail.com>
---
 Changes in v2:
   - add more description for min/max sysfs nodes.
   - convert values from volt to millivolt in the document.

 Documentation/hwmon/index.rst  |  1 +
 Documentation/hwmon/max127.rst | 45 ++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)
 create mode 100644 Documentation/hwmon/max127.rst

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 408760d13813..0a07b6000c20 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -111,6 +111,7 @@ Hardware Monitoring Kernel Drivers
    ltc4245
    ltc4260
    ltc4261
+   max127
    max16064
    max16065
    max1619
diff --git a/Documentation/hwmon/max127.rst b/Documentation/hwmon/max127.rst
new file mode 100644
index 000000000000..dc192dd9c37c
--- /dev/null
+++ b/Documentation/hwmon/max127.rst
@@ -0,0 +1,45 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver max127
+====================
+
+Author:
+
+  * Tao Ren <rentao.bupt@gmail.com>
+
+Supported chips:
+
+  * Maxim MAX127
+
+    Prefix: 'max127'
+
+    Datasheet: https://datasheets.maximintegrated.com/en/ds/MAX127-MAX128.pdf
+
+Description
+-----------
+
+The MAX127 is a multirange, 12-bit data acquisition system (DAS) providing
+8 analog input channels that are independently software programmable for
+a variety of ranges. The available ranges are {0,5V}, {0,10V}, {-5,5V}
+and {-10,10V}.
+
+The MAX127 features a 2-wire, I2C-compatible serial interface that allows
+communication among multiple devices using SDA and SCL lines.
+
+Sysfs interface
+---------------
+
+  ============== ==============================================================
+  in[0-7]_input  The input voltage (in mV) of the corresponding channel.
+		 RO
+
+  in[0-7]_min    The lower input limit (in mV) for the corresponding channel.
+		 ADC range and LSB will be updated when the limit is changed.
+		 For the MAX127, it will be adjusted to -10000, -5000, or 0.
+		 RW
+
+  in[0-7]_max    The higher input limit (in mV) for the corresponding channel.
+		 ADC range and LSB will be updated when the limit is changed.
+		 For the MAX127, it will be adjusted to 0, 5000, or 10000.
+		 RW
+  ============== ==============================================================
-- 
2.17.1

Re: [PATCH v2 0/2] hwmon: (max127) Add Maxim MAX127 hardware monitoring

[Andrew Lunn]

On Wed, Nov 18, 2020 at 03:09:27PM -0800, rentao.bupt@gmail.com wrote:
> From: Tao Ren <rentao.bupt@gmail.com>
> 
> The patch series adds hardware monitoring driver for the Maxim MAX127
> chip.

Hi Tao

Why are using sending a hwmon driver to the networking mailing list?

    Andrew

Re: [PATCH v2 0/2] hwmon: (max127) Add Maxim MAX127 hardware monitoring

[Tao Ren]

On Thu, Nov 19, 2020 at 12:27:19AM +0100, Andrew Lunn wrote:
> On Wed, Nov 18, 2020 at 03:09:27PM -0800, rentao.bupt@gmail.com wrote:
> > From: Tao Ren <rentao.bupt@gmail.com>
> > 
> > The patch series adds hardware monitoring driver for the Maxim MAX127
> > chip.
> 
> Hi Tao
> 
> Why are using sending a hwmon driver to the networking mailing list?
> 
>     Andrew

Hi Andrew,

I added netdev because the mailing list is included in "get_maintainer.pl
Documentation/hwmon/index.rst" output. Is it the right command to find
reviewers? Could you please suggest? Thank you.


Cheers,

Tao

Re: [PATCH v2 0/2] hwmon: (max127) Add Maxim MAX127 hardware monitoring

[Guenter Roeck]

On Wed, Nov 18, 2020 at 03:42:53PM -0800, Tao Ren wrote:
> On Thu, Nov 19, 2020 at 12:27:19AM +0100, Andrew Lunn wrote:
> > On Wed, Nov 18, 2020 at 03:09:27PM -0800, rentao.bupt@gmail.com wrote:
> > > From: Tao Ren <rentao.bupt@gmail.com>
> > > 
> > > The patch series adds hardware monitoring driver for the Maxim MAX127
> > > chip.
> > 
> > Hi Tao
> > 
> > Why are using sending a hwmon driver to the networking mailing list?
> > 
> >     Andrew
> 
> Hi Andrew,
> 
> I added netdev because the mailing list is included in "get_maintainer.pl
> Documentation/hwmon/index.rst" output. Is it the right command to find
> reviewers? Could you please suggest? Thank you.

I have no idea why running get_maintainer.pl on
Documentation/hwmon/index.rst returns such a large list of mailing
lists and people. For some reason it includes everyone in the XDP
maintainer list. If anyone has an idea how that happens, please
let me know - we'll want to get this fixed to avoid the same problem
in the future.

Guenter

Re: [PATCH v2 0/2] hwmon: (max127) Add Maxim MAX127 hardware monitoring

[Tao Ren]

On Wed, Nov 18, 2020 at 05:26:53PM -0800, Guenter Roeck wrote:
> On Wed, Nov 18, 2020 at 05:01:19PM -0800, Guenter Roeck wrote:
> > On Wed, Nov 18, 2020 at 03:42:53PM -0800, Tao Ren wrote:
> > > On Thu, Nov 19, 2020 at 12:27:19AM +0100, Andrew Lunn wrote:
> > > > On Wed, Nov 18, 2020 at 03:09:27PM -0800, rentao.bupt@gmail.com wrote:
> > > > > From: Tao Ren <rentao.bupt@gmail.com>
> > > > > 
> > > > > The patch series adds hardware monitoring driver for the Maxim MAX127
> > > > > chip.
> > > > 
> > > > Hi Tao
> > > > 
> > > > Why are using sending a hwmon driver to the networking mailing list?
> > > > 
> > > >     Andrew
> > > 
> > > Hi Andrew,
> > > 
> > > I added netdev because the mailing list is included in "get_maintainer.pl
> > > Documentation/hwmon/index.rst" output. Is it the right command to find
> > > reviewers? Could you please suggest? Thank you.
> > 
> > I have no idea why running get_maintainer.pl on
> > Documentation/hwmon/index.rst returns such a large list of mailing
> > lists and people. For some reason it includes everyone in the XDP
> > maintainer list. If anyone has an idea how that happens, please
> > let me know - we'll want to get this fixed to avoid the same problem
> > in the future.
> > 
> 
> I found it. The XDP maintainer entry has:
> 
> K:    xdp
> 
> This matches Documentation/hwmon/index.rst.
> 
> $ grep xdp Documentation/hwmon/index.rst
>    xdpe12284
> 
> It seems to me that a context match such as "xdp" in MAINTAINERS isn't
> really appropriate. "xdp" matches a total of 348 files in the kernel.
> The large majority of those is not XDP related. The maintainers
> of XDP (and all the listed mailing lists) should not be surprised
> to get a large number of odd review requests if they want to review
> every single patch on files which include the term "xdp".
> 
> Guenter

Thanks Guenter and Andrew. Given xdp maintainers were included by
mistake, I will remove them from the future discussions of this hwmon
patch series.


Cheers,

Tao

Re: [PATCH v2 0/2] hwmon: (max127) Add Maxim MAX127 hardware monitoring

[Jakub Kicinski]

On Wed, 18 Nov 2020 17:26:53 -0800 Guenter Roeck wrote:
> On Wed, Nov 18, 2020 at 05:01:19PM -0800, Guenter Roeck wrote:
> > On Wed, Nov 18, 2020 at 03:42:53PM -0800, Tao Ren wrote:  
> > > On Thu, Nov 19, 2020 at 12:27:19AM +0100, Andrew Lunn wrote:  
> > > > On Wed, Nov 18, 2020 at 03:09:27PM -0800, rentao.bupt@gmail.com wrote:  
> > > > > From: Tao Ren <rentao.bupt@gmail.com>
> > > > > 
> > > > > The patch series adds hardware monitoring driver for the Maxim MAX127
> > > > > chip.  
> > > > 
> > > > Hi Tao
> > > > 
> > > > Why are using sending a hwmon driver to the networking mailing list?
> > > > 
> > > >     Andrew  
> > > 
> > > Hi Andrew,
> > > 
> > > I added netdev because the mailing list is included in "get_maintainer.pl
> > > Documentation/hwmon/index.rst" output. Is it the right command to find
> > > reviewers? Could you please suggest? Thank you.  
> > 
> > I have no idea why running get_maintainer.pl on
> > Documentation/hwmon/index.rst returns such a large list of mailing
> > lists and people. For some reason it includes everyone in the XDP
> > maintainer list. If anyone has an idea how that happens, please
> > let me know - we'll want to get this fixed to avoid the same problem
> > in the future.
> 
> I found it. The XDP maintainer entry has:
> 
> K:    xdp
> 
> This matches Documentation/hwmon/index.rst.
> 
> $ grep xdp Documentation/hwmon/index.rst
>    xdpe12284
> 
> It seems to me that a context match such as "xdp" in MAINTAINERS isn't
> really appropriate. "xdp" matches a total of 348 files in the kernel.
> The large majority of those is not XDP related. The maintainers
> of XDP (and all the listed mailing lists) should not be surprised
> to get a large number of odd review requests if they want to review
> every single patch on files which include the term "xdp".

Agreed, we should fix this. For maintainers with high patch volume life
would be so much easier if people CCed the right folks to get reviews,
so we should try our best to fix get_maintainer.

XDP folks, any opposition to changing the keyword / filename to:

	[^a-z0-9]xdp[^a-z0-9]

?

XDP maintainer match (Was [PATCH v2 0/2] hwmon: (max127) Add Maxim MAX127 hardware monitoring)

[Jesper Dangaard Brouer]

On Thu, 19 Nov 2020 07:46:34 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Wed, 18 Nov 2020 17:26:53 -0800 Guenter Roeck wrote:
> > On Wed, Nov 18, 2020 at 05:01:19PM -0800, Guenter Roeck wrote:  
> > > On Wed, Nov 18, 2020 at 03:42:53PM -0800, Tao Ren wrote:    
> > > > On Thu, Nov 19, 2020 at 12:27:19AM +0100, Andrew Lunn wrote:    
> > > > > On Wed, Nov 18, 2020 at 03:09:27PM -0800, rentao.bupt@gmail.com wrote:    
> > > > > > From: Tao Ren <rentao.bupt@gmail.com>
> > > > > > 
> > > > > > The patch series adds hardware monitoring driver for the Maxim MAX127
> > > > > > chip.    
> > > > > 
> > > > > Hi Tao
> > > > > 
> > > > > Why are using sending a hwmon driver to the networking mailing list?
> > > > > 
> > > > >     Andrew    
> > > > 
> > > > Hi Andrew,
> > > > 
> > > > I added netdev because the mailing list is included in "get_maintainer.pl
> > > > Documentation/hwmon/index.rst" output. Is it the right command to find
> > > > reviewers? Could you please suggest? Thank you.    
> > > 
> > > I have no idea why running get_maintainer.pl on
> > > Documentation/hwmon/index.rst returns such a large list of mailing
> > > lists and people. For some reason it includes everyone in the XDP
> > > maintainer list. If anyone has an idea how that happens, please
> > > let me know - we'll want to get this fixed to avoid the same problem
> > > in the future.  
> > 
> > I found it. The XDP maintainer entry has:
> > 
> > K:    xdp
> > 
> > This matches Documentation/hwmon/index.rst.
> > 
> > $ grep xdp Documentation/hwmon/index.rst
> >    xdpe12284
> > 
> > It seems to me that a context match such as "xdp" in MAINTAINERS isn't
> > really appropriate. "xdp" matches a total of 348 files in the kernel.
> > The large majority of those is not XDP related. The maintainers
> > of XDP (and all the listed mailing lists) should not be surprised
> > to get a large number of odd review requests if they want to review
> > every single patch on files which include the term "xdp".  
> 
> Agreed, we should fix this. For maintainers with high patch volume life
> would be so much easier if people CCed the right folks to get reviews,
> so we should try our best to fix get_maintainer.
> 
> XDP folks, any opposition to changing the keyword / filename to:
> 
> 	[^a-z0-9]xdp[^a-z0-9]
> 
> ?

I think it is a good idea to change the keyword (K:), but I'm not sure
this catch what we want, maybe it does.  The pattern match are meant to
catch drivers containing XDP related bits.

Previously Joe Perches <joe@perches.com> suggested this pattern match,
which I don't fully understand... could you explain Joe?

  (?:\b|_)xdp(?:\b|_)

For the filename (N:) regex match, I'm considering if we should remove
it and list more files explicitly.  I think normal glob * pattern
works, which should be sufficient.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: XDP maintainer match (Was [PATCH v2 0/2] hwmon: (max127) Add Maxim MAX127 hardware monitoring)

[Joe Perches]

On Thu, 2020-11-19 at 17:35 +0100, Jesper Dangaard Brouer wrote:
> On Thu, 19 Nov 2020 07:46:34 -0800 Jakub Kicinski <kuba@kernel.org> wrote:

> I think it is a good idea to change the keyword (K:), but I'm not sure
> this catch what we want, maybe it does.  The pattern match are meant to
> catch drivers containing XDP related bits.
> 
> Previously Joe Perches <joe@perches.com> suggested this pattern match,
> which I don't fully understand... could you explain Joe?
> 
>   (?:\b|_)xdp(?:\b|_)

This regex matches only:

	xdp
	xdp_<anything>
	<anything>_xdp_<anything>
	<anything>_xdp

> For the filename (N:) regex match, I'm considering if we should remove
> it and list more files explicitly.  I think normal glob * pattern
> works, which should be sufficient.

Lists are generally more specific than regex globs.

Re: XDP maintainer match (Was [PATCH v2 0/2] hwmon: (max127) Add Maxim MAX127 hardware monitoring)

[Jakub Kicinski]

On Thu, 19 Nov 2020 09:09:53 -0800 Joe Perches wrote:
> On Thu, 2020-11-19 at 17:35 +0100, Jesper Dangaard Brouer wrote:
> > On Thu, 19 Nov 2020 07:46:34 -0800 Jakub Kicinski <kuba@kernel.org> wrote:  
> 
> > I think it is a good idea to change the keyword (K:), but I'm not sure
> > this catch what we want, maybe it does.  The pattern match are meant to
> > catch drivers containing XDP related bits.
> > 
> > Previously Joe Perches <joe@perches.com> suggested this pattern match,
> > which I don't fully understand... could you explain Joe?
> > 
> >   (?:\b|_)xdp(?:\b|_)  
> 
> This regex matches only:
> 
> 	xdp
> 	xdp_<anything>
> 	<anything>_xdp_<anything>
> 	<anything>_xdp
> 
> > For the filename (N:) regex match, I'm considering if we should remove
> > it and list more files explicitly.  I think normal glob * pattern
> > works, which should be sufficient.  
> 
> Lists are generally more specific than regex globs.

Checking like Alexei did it seems Joe's version is faster and better:

$ git grep -l -E "[^a-z0-9]xdp[^a-z0-9]" | wc -l
295
$ git grep -l -E '(\b|_)xdp(\b|_)' | wc -l
297
$ time git grep -l -E '(\b|_)xdp(\b|_)' > /tmp/a

real	0m5.171s
user	0m32.657s
sys	0m0.664s
$ time git grep -l -E "[^a-z0-9]xdp[^a-z0-9]" > /tmp/b

real	0m16.627s
user	1m48.149s
sys	0m0.977s
09:56 linux$ diff /tmp/a /tmp/b
4d3
< Documentation/networking/index.rst
189d187
< samples/bpf/.gitignore


Joe would you like to send a patch, or should I?

Re: XDP maintainer match (Was [PATCH v2 0/2] hwmon: (max127) Add Maxim MAX127 hardware monitoring)

[Jesper Dangaard Brouer]

On Thu, 19 Nov 2020 09:59:28 -0800
Jakub Kicinski <kuba@kernel.org> wrote:

> On Thu, 19 Nov 2020 09:09:53 -0800 Joe Perches wrote:
> > On Thu, 2020-11-19 at 17:35 +0100, Jesper Dangaard Brouer wrote:  
> > > On Thu, 19 Nov 2020 07:46:34 -0800 Jakub Kicinski <kuba@kernel.org> wrote:    
> >   
> > > I think it is a good idea to change the keyword (K:), but I'm not sure
> > > this catch what we want, maybe it does.  The pattern match are meant to
> > > catch drivers containing XDP related bits.
> > > 
> > > Previously Joe Perches <joe@perches.com> suggested this pattern match,
> > > which I don't fully understand... could you explain Joe?
> > > 
> > >   (?:\b|_)xdp(?:\b|_)    
> > 
> > This regex matches only:
> > 
> > 	xdp
> > 	xdp_<anything>
> > 	<anything>_xdp_<anything>
> > 	<anything>_xdp
> >   
> > > For the filename (N:) regex match, I'm considering if we should remove
> > > it and list more files explicitly.  I think normal glob * pattern
> > > works, which should be sufficient.    
> > 
> > Lists are generally more specific than regex globs.  
> 
> Checking like Alexei did it seems Joe's version is faster and better:
> 
> $ git grep -l -E "[^a-z0-9]xdp[^a-z0-9]" | wc -l
> 295
> $ git grep -l -E '(\b|_)xdp(\b|_)' | wc -l
> 297
> $ time git grep -l -E '(\b|_)xdp(\b|_)' > /tmp/a

Okay, I guess this is the pattern you want: '(\b|_)xdp(\b|_)'

 
> Joe would you like to send a patch, or should I?

As you noticed I already send out a patch, I can send a new with your
pattern, as it seems to be faster.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH v2 0/2] hwmon: (max127) Add Maxim MAX127 hardware monitoring

[Alexei Starovoitov]

On Thu, Nov 19, 2020 at 7:46 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 18 Nov 2020 17:26:53 -0800 Guenter Roeck wrote:
> > On Wed, Nov 18, 2020 at 05:01:19PM -0800, Guenter Roeck wrote:
> > > On Wed, Nov 18, 2020 at 03:42:53PM -0800, Tao Ren wrote:
> > > > On Thu, Nov 19, 2020 at 12:27:19AM +0100, Andrew Lunn wrote:
> > > > > On Wed, Nov 18, 2020 at 03:09:27PM -0800, rentao.bupt@gmail.com wrote:
> > > > > > From: Tao Ren <rentao.bupt@gmail.com>
> > > > > >
> > > > > > The patch series adds hardware monitoring driver for the Maxim MAX127
> > > > > > chip.
> > > > >
> > > > > Hi Tao
> > > > >
> > > > > Why are using sending a hwmon driver to the networking mailing list?
> > > > >
> > > > >     Andrew
> > > >
> > > > Hi Andrew,
> > > >
> > > > I added netdev because the mailing list is included in "get_maintainer.pl
> > > > Documentation/hwmon/index.rst" output. Is it the right command to find
> > > > reviewers? Could you please suggest? Thank you.
> > >
> > > I have no idea why running get_maintainer.pl on
> > > Documentation/hwmon/index.rst returns such a large list of mailing
> > > lists and people. For some reason it includes everyone in the XDP
> > > maintainer list. If anyone has an idea how that happens, please
> > > let me know - we'll want to get this fixed to avoid the same problem
> > > in the future.
> >
> > I found it. The XDP maintainer entry has:
> >
> > K:    xdp
> >
> > This matches Documentation/hwmon/index.rst.
> >
> > $ grep xdp Documentation/hwmon/index.rst
> >    xdpe12284
> >
> > It seems to me that a context match such as "xdp" in MAINTAINERS isn't
> > really appropriate. "xdp" matches a total of 348 files in the kernel.
> > The large majority of those is not XDP related. The maintainers
> > of XDP (and all the listed mailing lists) should not be surprised
> > to get a large number of odd review requests if they want to review
> > every single patch on files which include the term "xdp".
>
> Agreed, we should fix this. For maintainers with high patch volume life
> would be so much easier if people CCed the right folks to get reviews,
> so we should try our best to fix get_maintainer.
>
> XDP folks, any opposition to changing the keyword / filename to:
>
>         [^a-z0-9]xdp[^a-z0-9]

Reducing regex makes sense.
git grep -l -E "xdp"|wc -l
348
git grep -l -E "[^a-z0-9]xdp[^a-z0-9]"|wc -l
295

The false positive match was:
+drivers/hwmon/pmbus/Kconfig
+drivers/hwmon/pmbus/Makefile
+drivers/hwmon/pmbus/xdpe12284.c
+drivers/net/ethernet/natsemi/ns83820.c
+drivers/net/ethernet/neterion/s2io.c
+drivers/net/ethernet/neterion/s2io.h
+drivers/net/ethernet/neterion/vxge/vxge-config.c
+drivers/net/ethernet/neterion/vxge/vxge-config.h
+drivers/net/ethernet/neterion/vxge/vxge-traffic.c
+drivers/net/ethernet/sis/sis900.c
+drivers/net/ethernet/sis/sis900.h
+drivers/net/wireless/ath/ath5k/ath5k.h
+drivers/net/wireless/ath/ath5k/base.c
+drivers/net/wireless/ath/ath5k/debug.c
+drivers/net/wireless/ath/ath5k/dma.c

so it's pretty much hwmon and few drivers.
I agree that sparing xdp from hwmon patches is a good thing :)

Re: [PATCH v2 0/2] hwmon: (max127) Add Maxim MAX127 hardware monitoring

[Andrew Lunn]

On Wed, Nov 18, 2020 at 03:42:53PM -0800, Tao Ren wrote:
> On Thu, Nov 19, 2020 at 12:27:19AM +0100, Andrew Lunn wrote:
> > On Wed, Nov 18, 2020 at 03:09:27PM -0800, rentao.bupt@gmail.com wrote:
> > > From: Tao Ren <rentao.bupt@gmail.com>
> > > 
> > > The patch series adds hardware monitoring driver for the Maxim MAX127
> > > chip.
> > 
> > Hi Tao
> > 
> > Why are using sending a hwmon driver to the networking mailing list?
> > 
> >     Andrew
> 
> Hi Andrew,
> 
> I added netdev because the mailing list is included in "get_maintainer.pl
> Documentation/hwmon/index.rst" output. Is it the right command to find
> reviewers? Could you please suggest? Thank you.

Hi Tae

What you are doing is correct. I suspected it was a get_maintainers
problem. Now we know this, we can figure out why it is adding all
these extra addresses which make no sense. Maybe a bug in the
MAINTAINERS file?

       Andrew