From: Tao Ren <[email protected]>
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
From: Tao Ren <[email protected]>
Add documentation for the max127 hardware monitoring driver.
Signed-off-by: Tao Ren <[email protected]>
---
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 <[email protected]>
+
+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
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]>
---
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 <[email protected]>");
+MODULE_AUTHOR("Tao Ren <[email protected]>");
+MODULE_DESCRIPTION("MAX127 Hardware Monitoring driver");
--
2.17.1
On Wed, Nov 18, 2020 at 03:09:27PM -0800, [email protected] wrote:
> From: Tao Ren <[email protected]>
>
> 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
On Thu, Nov 19, 2020 at 12:27:19AM +0100, Andrew Lunn wrote:
> On Wed, Nov 18, 2020 at 03:09:27PM -0800, [email protected] wrote:
> > From: Tao Ren <[email protected]>
> >
> > 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
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, [email protected] wrote:
> > > From: Tao Ren <[email protected]>
> > >
> > > 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
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, [email protected] wrote:
> > > From: Tao Ren <[email protected]>
> > >
> > > 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
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, [email protected] wrote:
> > > > > From: Tao Ren <[email protected]>
> > > > >
> > > > > 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
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, [email protected] wrote:
> > > > > From: Tao Ren <[email protected]>
> > > > >
> > > > > 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]
?
On Thu, 19 Nov 2020 07:46:34 -0800
Jakub Kicinski <[email protected]> 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, [email protected] wrote:
> > > > > > From: Tao Ren <[email protected]>
> > > > > >
> > > > > > 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 <[email protected]> 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
On Thu, Nov 19, 2020 at 7:46 AM Jakub Kicinski <[email protected]> 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, [email protected] wrote:
> > > > > > From: Tao Ren <[email protected]>
> > > > > >
> > > > > > 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 :)
On Thu, 2020-11-19 at 17:35 +0100, Jesper Dangaard Brouer wrote:
> On Thu, 19 Nov 2020 07:46:34 -0800 Jakub Kicinski <[email protected]> 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 <[email protected]> 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.
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 <[email protected]> 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 <[email protected]> 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?
On Thu, 19 Nov 2020 09:59:28 -0800
Jakub Kicinski <[email protected]> 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 <[email protected]> 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 <[email protected]> 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