| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CALz278YCpDYd6Lyj0qdAS9xv0_HYfz7y3a3L7WW4+9A65nS7zQ@mail.gmail.com>
Date: Tue, 15 Oct 2024 10:49:26 +0800
From: Ban Feng <baneric926@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: jdelvare@...e.com, robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
conor+dt@...nel.org, corbet@....net, linux-hwmon@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org, openbmc@...ts.ozlabs.org, kwliu@...oton.com,
kcfeng0@...oton.com, DELPHINE_CHIU@...ynn.com, Bonnie_Lo@...ynn.com
Subject: Re: [PATCH v5 2/2] hwmon: Add driver for I2C chip Nuvoton NCT7363Y
Hi Guenter,
On Thu, Jun 20, 2024 at 10:23 PM Guenter Roeck <linux@...ck-us.net> wrote:
>
> On Fri, Mar 22, 2024 at 04:11:58PM +0800, baneric926@...il.com wrote:
> > From: Ban Feng <kcfeng0@...oton.com>
> >
> > The NCT7363Y is a fan controller which provides up to 16
> > independent FAN input monitors. It can report each FAN input count
> > values. The NCT7363Y also provides up to 16 independent PWM
> > outputs. Each PWM can output specific PWM signal by manual mode to
> > control the FAN duty outside.
> >
> > Signed-off-by: Ban Feng <kcfeng0@...oton.com>
>
> Sorry for the late reply. This got somehow lost in my queue.
>
> > ---
> > Documentation/hwmon/index.rst | 1 +
> > Documentation/hwmon/nct7363.rst | 33 +++
> > MAINTAINERS | 2 +
> > drivers/hwmon/Kconfig | 11 +
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/nct7363.c | 396 ++++++++++++++++++++++++++++++++
> > 6 files changed, 444 insertions(+)
> > create mode 100644 Documentation/hwmon/nct7363.rst
> > create mode 100644 drivers/hwmon/nct7363.c
> >
> > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> > index 1ca7a4fe1f8f..0874f2f754f4 100644
> > --- a/Documentation/hwmon/index.rst
> > +++ b/Documentation/hwmon/index.rst
> > @@ -170,6 +170,7 @@ Hardware Monitoring Kernel Drivers
> > mpq8785
> > nct6683
> > nct6775
> > + nct7363
> > nct7802
> > nct7904
> > npcm750-pwm-fan
> > diff --git a/Documentation/hwmon/nct7363.rst b/Documentation/hwmon/nct7363.rst
> > new file mode 100644
> > index 000000000000..1a6abce3a433
> > --- /dev/null
> > +++ b/Documentation/hwmon/nct7363.rst
> > @@ -0,0 +1,33 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +Kernel driver nct7363
> > +=====================
> > +
> > +Supported chip:
> > +
> > + * Nuvoton NCT7363Y
> > +
> > + Prefix: nct7363
> > +
> > + Addresses: I2C 0x20, 0x21, 0x22, 0x23
> > +
> > +Author: Ban Feng <kcfeng0@...oton.com>
> > +
> > +
> > +Description
> > +-----------
> > +
> > +The NCT7363Y is a fan controller which provides up to 16 independent
> > +FAN input monitors, and up to 16 independent PWM outputs with SMBus interface.
> > +
> > +
> > +Sysfs entries
> > +-------------
> > +
> > +Currently, the driver supports the following features:
> > +
> > +========== ==========================================
> > +fanX_input provide current fan rotation value in RPM
> > +
> > +pwmX get or set PWM fan control value.
> > +========== ==========================================
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 2705e44ffc0c..c016a0bed476 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -15221,6 +15221,8 @@ M: Ban Feng <kcfeng0@...oton.com>
> > L: linux-hwmon@...r.kernel.org
> > S: Maintained
> > F: Documentation/devicetree/bindings/hwmon/nuvoton,nct7363.yaml
> > +F: Documentation/hwmon/nct7363.rst
> > +F: drivers/hwmon/nct7363.c
> >
> > NETDEVSIM
> > M: Jakub Kicinski <kuba@...nel.org>
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 83945397b6eb..4ff19ea11001 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1658,6 +1658,17 @@ config SENSORS_NCT6775_I2C
> > This driver can also be built as a module. If so, the module
> > will be called nct6775-i2c.
> >
> > +config SENSORS_NCT7363
> > + tristate "Nuvoton NCT7363Y"
> > + depends on I2C
> > + select REGMAP_I2C
> > + help
> > + If you say yes here you get support for the Nuvoton NCT7363Y
> > + hardware monitoring chip.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called nct7363.
> > +
> > config SENSORS_NCT7802
> > tristate "Nuvoton NCT7802Y"
> > depends on I2C
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index 5c31808f6378..cf7be22b916a 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -171,6 +171,7 @@ obj-$(CONFIG_SENSORS_NCT6775_CORE) += nct6775-core.o
> > nct6775-objs := nct6775-platform.o
> > obj-$(CONFIG_SENSORS_NCT6775) += nct6775.o
> > obj-$(CONFIG_SENSORS_NCT6775_I2C) += nct6775-i2c.o
> > +obj-$(CONFIG_SENSORS_NCT7363) += nct7363.o
> > obj-$(CONFIG_SENSORS_NCT7802) += nct7802.o
> > obj-$(CONFIG_SENSORS_NCT7904) += nct7904.o
> > obj-$(CONFIG_SENSORS_NPCM7XX) += npcm750-pwm-fan.o
> > diff --git a/drivers/hwmon/nct7363.c b/drivers/hwmon/nct7363.c
> > new file mode 100644
> > index 000000000000..858296f5d5b3
> > --- /dev/null
> > +++ b/drivers/hwmon/nct7363.c
> > @@ -0,0 +1,396 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (c) 2023 Nuvoton Technology corporation.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/err.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +
> > +#define NCT7363_REG_FUNC_CFG_BASE(x) (0x20 + (x))
> > +#define NCT7363_REG_PWMEN_BASE(x) (0x38 + (x))
> > +#define NCT7363_REG_FANINEN_BASE(x) (0x41 + (x))
> > +#define NCT7363_REG_FANINX_HVAL(x) (0x48 + ((x) * 2))
> > +#define NCT7363_REG_FANINX_LVAL(x) (0x49 + ((x) * 2))
> > +#define NCT7363_REG_FSCPXDUTY(x) (0x90 + ((x) * 2))
> > +
> > +#define PWM_SEL(x) (BIT(0) << ((x) * 2))
> > +#define FANIN_SEL(x) (BIT(1) << ((x < 8) ? \
> > + (((x) + 8) * 2) : \
> > + (((x) % 8) * 2)))
> > +#define VALUE_TO_REG(x, y) (((x) >> ((y) * 8)) & 0xFF)
> > +
> > +#define NCT7363_FANINX_LVAL_MASK GENMASK(4, 0)
> > +#define NCT7363_FANIN_MASK GENMASK(12, 0)
> > +
> > +#define NCT7363_PWM_COUNT 16
> > +
> > +static inline unsigned int FAN_FROM_REG(u16 val)
>
> Please use lower case for functions, even if inline.
ok, I'll fix it in v6.
>
> > +{
> > + if (val == NCT7363_FANIN_MASK || val == 0)
> > + return 0;
> > +
> > + return (1350000UL / val);
> > +}
> > +
> > +enum chips { nct7363, nct7362 };
> > +
>
> Those enums are not actually used. Are they needed ?
This added is for nct7362, in case we need to add the difference between them.
>
> > +static const struct i2c_device_id nct7363_id[] = {
> > + { "nct7363", nct7363 },
> > + { "nct7362", nct7362 },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(i2c, nct7363_id);
> > +
> > +static const struct of_device_id nct7363_of_match[] = {
> > + { .compatible = "nuvoton,nct7363", .data = (void *)nct7363 },
> > + { .compatible = "nuvoton,nct7362", .data = (void *)nct7362 },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(of, nct7363_of_match);
> > +
> > +struct nct7363_data {
> > + struct regmap *regmap;
> > + struct mutex lock; /* protect register access */
> > +
> > + u16 fanin_mask;
> > + u16 pwm_mask;
> > +};
> > +
> > +static int nct7363_read_fan(struct device *dev, u32 attr, int channel,
> > + long *val)
> > +{
> > + struct nct7363_data *data = dev_get_drvdata(dev);
> > + unsigned int hi, lo, rpm;
> > + int ret = 0;
> > + u16 cnt;
> > +
> > + switch (attr) {
> > + case hwmon_fan_input:
> > + /*
> > + * High-byte register should be read first to latch
> > + * synchronous low-byte value
> > + */
> > + mutex_lock(&data->lock);
> > + ret = regmap_read(data->regmap,
> > + NCT7363_REG_FANINX_HVAL(channel), &hi);
> > + if (ret)
> > + goto out;
> > +
> > + ret = regmap_read(data->regmap,
> > + NCT7363_REG_FANINX_LVAL(channel), &lo);
>
> Please consider using regmap_read_bulk() to avoid the locks.
ok, I'll fix it in v6.
>
> > + if (ret)
> > + goto out;
> > + mutex_unlock(&data->lock);
> > +
> > + cnt = (hi << 5) | (lo & NCT7363_FANINX_LVAL_MASK);
> > + rpm = FAN_FROM_REG(cnt);
> > + *val = (long)rpm;
>
> rpm and the typecast are unnecessary. Just use
> *val = fan_from_reg(cnt);
ok, I'll fix it in v6.
>
> > + return 0;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +
> > +out:
> > + mutex_unlock(&data->lock);
> > + return ret;
> > +}
> > +
> > +static umode_t nct7363_fan_is_visible(const void *_data, u32 attr, int channel)
> > +{
> > + const struct nct7363_data *data = _data;
> > +
> > + switch (attr) {
> > + case hwmon_fan_input:
> > + if (data->fanin_mask & BIT(channel))
> > + return 0444;
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int nct7363_read_pwm(struct device *dev, u32 attr, int channel,
> > + long *val)
> > +{
> > + struct nct7363_data *data = dev_get_drvdata(dev);
> > + unsigned int regval;
> > + int ret;
> > +
> > + switch (attr) {
> > + case hwmon_pwm_input:
> > + ret = regmap_read(data->regmap,
> > + NCT7363_REG_FSCPXDUTY(channel), ®val);
> > + if (ret)
> > + return ret;
> > +
> > + *val = (long)regval;
> > + return 0;
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +}
> > +
> > +static int nct7363_write_pwm(struct device *dev, u32 attr, int channel,
> > + long val)
> > +{
> > + struct nct7363_data *data = dev_get_drvdata(dev);
> > + int ret;
> > +
> > + switch (attr) {
> > + case hwmon_pwm_input:
> > + if (val < 0 || val > 255)
> > + return -EINVAL;
> > +
> > + ret = regmap_write(data->regmap,
> > + NCT7363_REG_FSCPXDUTY(channel), val);
> > +
> > + return ret;
> > +
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +}
> > +
> > +static umode_t nct7363_pwm_is_visible(const void *_data, u32 attr, int channel)
> > +{
> > + const struct nct7363_data *data = _data;
> > +
> > + switch (attr) {
> > + case hwmon_pwm_input:
> > + if (data->pwm_mask & BIT(channel))
> > + return 0644;
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int nct7363_read(struct device *dev, enum hwmon_sensor_types type,
> > + u32 attr, int channel, long *val)
> > +{
> > + switch (type) {
> > + case hwmon_fan:
> > + return nct7363_read_fan(dev, attr, channel, val);
> > + case hwmon_pwm:
> > + return nct7363_read_pwm(dev, attr, channel, val);
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +}
> > +
> > +static int nct7363_write(struct device *dev, enum hwmon_sensor_types type,
> > + u32 attr, int channel, long val)
> > +{
> > + switch (type) {
> > + case hwmon_pwm:
> > + return nct7363_write_pwm(dev, attr, channel, val);
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +}
> > +
> > +static umode_t nct7363_is_visible(const void *data,
> > + enum hwmon_sensor_types type,
> > + u32 attr, int channel)
> > +{
> > + switch (type) {
> > + case hwmon_fan:
> > + return nct7363_fan_is_visible(data, attr, channel);
> > + case hwmon_pwm:
> > + return nct7363_pwm_is_visible(data, attr, channel);
> > + default:
> > + return 0;
> > + }
> > +}
> > +
> > +static const struct hwmon_channel_info *nct7363_info[] = {
> > + HWMON_CHANNEL_INFO(fan,
> > + HWMON_F_INPUT,
> > + HWMON_F_INPUT,
> > + HWMON_F_INPUT,
> > + HWMON_F_INPUT,
> > + HWMON_F_INPUT,
> > + HWMON_F_INPUT,
> > + HWMON_F_INPUT,
> > + HWMON_F_INPUT,
> > + HWMON_F_INPUT,
> > + HWMON_F_INPUT,
> > + HWMON_F_INPUT,
> > + HWMON_F_INPUT,
> > + HWMON_F_INPUT,
> > + HWMON_F_INPUT,
> > + HWMON_F_INPUT,
> > + HWMON_F_INPUT),
>
> Other potential attributes:
>
> - enable (register 0x41, 0x42, and possibly 0x40)
> - alarm (register 0x34, 0x35)
> - min (register 0x6c, 0x6d)
ok, I'll consider min and alarm in v6.
>
> > + HWMON_CHANNEL_INFO(pwm,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT,
> > + HWMON_PWM_INPUT),
>
> Other potential attributes:
>
> - enable (register 0x38)
> - freq (register 0x91, ...)
>
> All those could be added later, of course, but I would suggest to at least
> add the fan speed low limit and alarm attributes.
ok, I'll consider min and alarm in v6.
>
> > + NULL
> > +};
> > +
> > +static const struct hwmon_ops nct7363_hwmon_ops = {
> > + .is_visible = nct7363_is_visible,
> > + .read = nct7363_read,
> > + .write = nct7363_write,
> > +};
> > +
> > +static const struct hwmon_chip_info nct7363_chip_info = {
> > + .ops = &nct7363_hwmon_ops,
> > + .info = nct7363_info,
> > +};
> > +
> > +static int nct7363_init_chip(struct nct7363_data *data)
> > +{
> > + u32 func_config = 0;
> > + int i, ret;
> > +
> > + /* Pin Function Configuration */
> > + for (i = 0; i < NCT7363_PWM_COUNT; i++) {
> > + if (data->pwm_mask & BIT(i))
> > + func_config |= PWM_SEL(i);
> > + if (data->fanin_mask & BIT(i))
> > + func_config |= FANIN_SEL(i);
> > + }
> > +
> > + for (i = 0; i < 4; i++) {
> > + ret = regmap_write(data->regmap, NCT7363_REG_FUNC_CFG_BASE(i),
> > + VALUE_TO_REG(func_config, i));
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > + /* PWM and FANIN Monitoring Enable */
> > + for (i = 0; i < 2; i++) {
> > + ret = regmap_write(data->regmap, NCT7363_REG_PWMEN_BASE(i),
> > + VALUE_TO_REG(data->pwm_mask, i));
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = regmap_write(data->regmap, NCT7363_REG_FANINEN_BASE(i),
> > + VALUE_TO_REG(data->fanin_mask, i));
> > + if (ret < 0)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int nct7363_present_pwm_fanin(struct device *dev,
> > + struct device_node *child,
> > + struct nct7363_data *data)
> > +{
> > + u8 fanin_ch[NCT7363_PWM_COUNT];
> > + struct of_phandle_args args;
> > + int ret, fanin_cnt;
> > + u8 ch, index;
> > +
> > + ret = of_parse_phandle_with_args(child, "pwms", "#pwm-cells",
> > + 0, &args);
> > + if (ret)
> > + return ret;
> > +
> > + if (args.args[0] >= NCT7363_PWM_COUNT)
> > + return -EINVAL;
> > + data->pwm_mask |= BIT(args.args[0]);
> > +
> > + fanin_cnt = of_property_count_u8_elems(child, "tach-ch");
> > + if (fanin_cnt < 1 || fanin_cnt > NCT7363_PWM_COUNT)
> > + return -EINVAL;
> > +
> > + ret = of_property_read_u8_array(child, "tach-ch", fanin_ch, fanin_cnt);
> > + if (ret)
> > + return ret;
> > +
> > + for (ch = 0; ch < fanin_cnt; ch++) {
> > + index = fanin_ch[ch];
> > + if (index >= NCT7363_PWM_COUNT)
> > + return -EINVAL;
> > + data->fanin_mask |= BIT(index);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct regmap_config nct7363_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 8,
>
> Your call to make, but this doesn't use regmap caching capabilities which
> might be really useful here. Most of the registers (all but the count and
> status registers, plus the gpio input registers if/when gpio support is
> added) are not volatile and could be cached.
ok, I'll add it in v6.
>
> > +};
> > +
> > +static int nct7363_probe(struct i2c_client *client)
> > +{
> > + struct device *dev = &client->dev;
> > + struct device_node *child;
> > + struct nct7363_data *data;
> > + struct device *hwmon_dev;
> > + int ret;
> > +
> > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + data->regmap = devm_regmap_init_i2c(client, &nct7363_regmap_config);
> > + if (IS_ERR(data->regmap))
> > + return PTR_ERR(data->regmap);
> > +
> > + mutex_init(&data->lock);
> > +
> > + for_each_child_of_node(dev->of_node, child) {
> > + ret = nct7363_present_pwm_fanin(dev, child, data);
> > + if (ret) {
> > + of_node_put(child);
> > + return ret;
> > + }
> > + }
> > +
> > + /* Initialize the chip */
> > + ret = nct7363_init_chip(data);
> > + if (ret)
> > + return ret;
> > +
> > + hwmon_dev =
> > + devm_hwmon_device_register_with_info(dev, client->name, data,
> > + &nct7363_chip_info, NULL);
> > + return PTR_ERR_OR_ZERO(hwmon_dev);
> > +}
> > +
> > +static struct i2c_driver nct7363_driver = {
> > + .class = I2C_CLASS_HWMON,
> > + .driver = {
> > + .name = "nct7363",
> > + .of_match_table = nct7363_of_match,
> > + },
> > + .probe = nct7363_probe,
> > + .id_table = nct7363_id,
> > +};
> > +
> > +module_i2c_driver(nct7363_driver);
> > +
> > +MODULE_AUTHOR("CW Ho <cwho@...oton.com>");
> > +MODULE_AUTHOR("Ban Feng <kcfeng0@...oton.com>");
> > +MODULE_DESCRIPTION("NCT7363 Hardware Monitoring Driver");
> > +MODULE_LICENSE("GPL");
Powered by blists - more mailing lists