From: Ban Feng <baneric926@gmail.com>
To: Zev Weiss <zev@bewilderbeest.net>
Cc: jdelvare@suse.com, linux@roeck-us.net, robh+dt@kernel.org,
krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
corbet@lwn.net, linux-hwmon@vger.kernel.org,
devicetree@vger.kernel.org, kcfeng0@nuvoton.com,
kwliu@nuvoton.com, openbmc@lists.ozlabs.org,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
DELPHINE_CHIU@wiwynn.com, naresh.solanki@9elements.com,
billy_tsai@aspeedtech.com
Subject: Re: [PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y
Date: Thu, 7 Mar 2024 15:35:31 +0800 [thread overview]
Message-ID: <CALz278Zgfgob573vgWz4PgC7vb=i8xt3kC1hSjo_cQi00B0XAg@mail.gmail.com> (raw)
In-Reply-To: <a90ed00c-f836-4fb6-8191-9974937e3eb7@hatter.bewilderbeest.net>
Hi Zev,
On Sat, Mar 2, 2024 at 4:19 PM Zev Weiss <zev@bewilderbeest.net> wrote:
>
> On Mon, Feb 26, 2024 at 04:56:06PM PST, baneric926@gmail.com wrote:
> >From: Ban Feng <kcfeng0@nuvoton.com>
> >
> >NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.
> >
> >Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
> >---
> > Documentation/hwmon/index.rst | 1 +
> > Documentation/hwmon/nct7363.rst | 33 +++
> > MAINTAINERS | 2 +
> > drivers/hwmon/Kconfig | 11 +
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/nct7363.c | 412 ++++++++++++++++++++++++++++++++
> > 6 files changed, 460 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 c7ed1f73ac06..302f954b45be 100644
> >--- a/Documentation/hwmon/index.rst
> >+++ b/Documentation/hwmon/index.rst
> >@@ -165,6 +165,7 @@ Hardware Monitoring Kernel Drivers
> > mp5990
> > 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..89699c95aa4b
> >--- /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@nuvoton.com>
> >+
> >+
> >+Description
> >+-----------
> >+
> >+The NCT7363Y is a Fan controller which provides up to 16 independent
> >+FAN input monitors, and up to 16 independent PWM output 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 7b1efefed7c4..7ca66b713e30 100644
> >--- a/MAINTAINERS
> >+++ b/MAINTAINERS
> >@@ -15089,6 +15089,8 @@ M: Ban Feng <kcfeng0@nuvoton.com>
> > L: linux-hwmon@vger.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@kernel.org>
> >diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> >index a608264da87d..a297b5409b04 100644
> >--- a/drivers/hwmon/Kconfig
> >+++ b/drivers/hwmon/Kconfig
> >@@ -1616,6 +1616,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 47be39af5c03..d5e7531204df 100644
> >--- a/drivers/hwmon/Makefile
> >+++ b/drivers/hwmon/Makefile
> >@@ -167,6 +167,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..c79d3ca4f111
> >--- /dev/null
> >+++ b/drivers/hwmon/nct7363.c
> >@@ -0,0 +1,412 @@
> >+// 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_GPIO_0_3 0x20
> >+#define NCT7363_REG_GPIO_4_7 0x21
> >+#define NCT7363_REG_GPIO_10_13 0x22
> >+#define NCT7363_REG_GPIO_14_17 0x23
> >+#define NCT7363_REG_PWMEN_0_7 0x38
> >+#define NCT7363_REG_PWMEN_8_15 0x39
> >+#define NCT7363_REG_FANINEN_0_7 0x41
> >+#define NCT7363_REG_FANINEN_8_15 0x42
> >+#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) % 4) * 2))
> >+#define FANIN_SEL(x) (BIT(1) << (((x) % 4) * 2))
> >+
> >+#define NCT7363_FANINX_LVAL_MASK GENMASK(4, 0)
> >+#define NCT7363_FANIN_MASK GENMASK(12, 0)
> >+
> >+#define NCT7363_PWM_COUNT 16
> >+
> >+static inline unsigned long FAN_FROM_REG(u16 val)
> >+{
> >+ /* In case fan is stopped or divide by 0 */
> >+ if (val == NCT7363_FANIN_MASK || val == 0)
> >+ return 0;
> >+
> >+ return (1350000UL / val);
> >+}
> >+
> >+static const struct of_device_id nct7363_of_match[] = {
> >+ { .compatible = "nuvoton,nct7363" },
>
> As far as I can see from the code in this driver, it looks like this
> driver should also be compatible with the nct7362; shall we add the ID
> table entry for it now? (Though I only have a datasheet for the
> nct7362, not the nct7363, so I don't know exactly how they differ.)
As far as I know, the difference between these two ICs is 0x2A~0x2C
Fading LED for 7362, and 0x2A Watchdog Timer for 7363.
In my v1 patch, I indeed add the nct7362 to the ID table, however,
this makes it more complicated and our datasheet isn't public yet.
I think maybe supporting more chips will be done in the future, but not now.
>
> >+ { },
> >+};
> >+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;
> >+ u16 cnt, rpm;
> >+ int ret = 0;
> >+
> >+ switch (attr) {
> >+ case hwmon_fan_input:
> >+ /*
> >+ * High-byte register should be read first to latch
> >+ * synchronous low-byte value
> >+ */
> >+ ret = regmap_read(data->regmap,
> >+ NCT7363_REG_FANINX_HVAL(channel), &hi);
> >+ if (ret)
> >+ return ret;
> >+
> >+ ret = regmap_read(data->regmap,
> >+ NCT7363_REG_FANINX_LVAL(channel), &lo);
> >+ if (ret)
> >+ return ret;
>
> I think this pair of reads should be done under data->lock to ensure
> that the LVAL read gets the right latched value in a scenario where
> multiple threads executing this function end up with their register
> reads interleaved.
ok, fix in v5
>
> >+
> >+ cnt = (hi << 5) | (lo & NCT7363_FANINX_LVAL_MASK);
> >+ rpm = FAN_FROM_REG(cnt);
> >+ *val = (long)rpm;
>
> Given that rpm is assigned from an unsigned long (FAN_FROM_REG()) and
> then to a long (*val), is there any reason we want it as a u16 in
> between the two? Or for that matter, why not just:
>
> *val = FAN_FROM_REG(cnt);
>
> ?
I'll modify this to align with the style of the nct7363_read_pwm function in v5.
>
> >+ return 0;
> >+ default:
> >+ return -EOPNOTSUPP;
> >+ }
> >+}
> >+
> >+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;
> >+ u16 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;
> >+
> >+ mutex_lock(&data->lock);
> >+ ret = regmap_write(data->regmap,
> >+ NCT7363_REG_FSCPXDUTY(channel), val);
> >+ mutex_unlock(&data->lock);
>
> ...though here, I'm not sure the locking is needed -- is there something
> that necessitates it for a single register write?
ok, fix in v5
>
> >+
> >+ 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),
> >+ 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),
> >+ 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)
> >+{
> >+ u8 i, gpio0_3 = 0, gpio4_7 = 0, gpio10_13 = 0, gpio14_17 = 0;
> >+ int ret;
> >+
> >+ for (i = 0; i < NCT7363_PWM_COUNT; i++) {
> >+ if (i < 4) {
> >+ if (data->pwm_mask & BIT(i))
> >+ gpio0_3 |= PWM_SEL(i);
> >+ if (data->fanin_mask & BIT(i))
> >+ gpio10_13 |= FANIN_SEL(i);
> >+ } else if (i < 8) {
> >+ if (data->pwm_mask & BIT(i))
> >+ gpio4_7 |= PWM_SEL(i);
> >+ if (data->fanin_mask & BIT(i))
> >+ gpio14_17 |= FANIN_SEL(i);
> >+ } else if (i < 12) {
> >+ if (data->pwm_mask & BIT(i))
> >+ gpio10_13 |= PWM_SEL(i);
> >+ if (data->fanin_mask & BIT(i))
> >+ gpio0_3 |= FANIN_SEL(i);
> >+ } else {
> >+ if (data->pwm_mask & BIT(i))
> >+ gpio14_17 |= PWM_SEL(i);
> >+ if (data->fanin_mask & BIT(i))
> >+ gpio4_7 |= FANIN_SEL(i);
> >+ }
> >+ }
>
> With the caveat that I haven't actually sketched it out myself to be
> sure, might it be a bit cleaner to do this with a 4-element array
> indexed by 'i / 4' instead of a big if/else chain and a bunch of
> near-duplicated blocks?
ok, fix in v5
>
> >+
> >+ /* Pin Function Configuration */
> >+ ret = regmap_write(data->regmap, NCT7363_REG_GPIO_0_3, gpio0_3);
> >+ if (ret < 0)
> >+ return ret;
> >+ ret = regmap_write(data->regmap, NCT7363_REG_GPIO_4_7, gpio4_7);
> >+ if (ret < 0)
> >+ return ret;
> >+ ret = regmap_write(data->regmap, NCT7363_REG_GPIO_10_13, gpio10_13);
> >+ if (ret < 0)
> >+ return ret;
> >+ ret = regmap_write(data->regmap, NCT7363_REG_GPIO_14_17, gpio14_17);
> >+ if (ret < 0)
> >+ return ret;
> >+
> >+ /* PWM and FANIN Monitoring Enable */
> >+ ret = regmap_write(data->regmap, NCT7363_REG_PWMEN_0_7,
> >+ data->pwm_mask & 0xff);
> >+ if (ret < 0)
> >+ return ret;
> >+ ret = regmap_write(data->regmap, NCT7363_REG_PWMEN_8_15,
> >+ (data->pwm_mask >> 8) & 0xff);
> >+ if (ret < 0)
> >+ return ret;
> >+ ret = regmap_write(data->regmap, NCT7363_REG_FANINEN_0_7,
> >+ data->fanin_mask & 0xff);
> >+ if (ret < 0)
> >+ return ret;
> >+ ret = regmap_write(data->regmap, NCT7363_REG_FANINEN_8_15,
> >+ (data->fanin_mask >> 8) & 0xff);
> >+ if (ret < 0)
> >+ return ret;
> >+
> >+ return 0;
> >+}
> >+
> >+static int nct7363_present_pwm_fanin(struct device *dev,
> >+ struct device_node *child,
> >+ struct nct7363_data *data)
> >+{
> >+ struct of_phandle_args args;
> >+ int ret, fanin_cnt;
> >+ u8 *fanin_ch;
> >+ u8 ch, index;
> >+
> >+ ret = of_parse_phandle_with_args(child, "pwms", "#pwm-cells",
> >+ 0, &args);
> >+ if (ret)
> >+ return ret;
> >+
> >+ data->pwm_mask |= BIT(args.args[0]);
>
> Perhaps we should have
>
> if (args.args[0] >= NCT7363_PWM_COUNT)
> return -EINVAL;
>
> here?
ok, fix in v5
>
> >+
> >+ fanin_cnt = of_property_count_u8_elems(child, "tach-ch");
> >+ if (fanin_cnt < 1)
>
> fanin_cnt < 1 || fanin_cnt >= NCT7363_PWM_COUNT
ok, fix in v5
>
> >+ return -EINVAL;
> >+
> >+ fanin_ch = devm_kcalloc(dev, fanin_cnt, sizeof(*fanin_ch), GFP_KERNEL);
>
> Keeping this allocation around persistently for the whole lifetime of
> the device seems unnecessary -- it's not used beyond this function,
> right? In fact, dynamically allocating it at all seems like overkill,
> considering that in addition to being temporary, it's also got a pretty
> small upper bound on its size. I'd think a simple
>
> u8 fanin_ch[NCT7363_PWM_COUNT];
>
> would suffice? (Along with the check above to ensure fanin_cnt is
> within range of course.)
ok, fix in v5
>
> >+ if (!fanin_ch)
> >+ return -ENOMEM;
> >+
> >+ 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];
>
> ...and likewise a range check here too?
ok, fix in v5
>
> >+ data->fanin_mask |= BIT(index);
> >+ }
> >+
>
> Should we also grab the pulses-per-revolution property here and factor
> that in in FAN_FROM_REG()?
So far our FanPoles in tachometer count calculation formula is always 4.
Therefore, I don't add the pulses-per-revolution property into FAN_FROM_REG().
>
> >+ return 0;
> >+}
> >+
> >+static const struct regmap_config nct7363_regmap_config = {
> >+ .reg_bits = 8,
> >+ .val_bits = 8,
> >+};
> >+
> >+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,
>
> Maybe add an i2c_device_id table to accompany the of_match table?
ok, fix in v5
Thanks,
Ban
>
> >+ .driver = {
> >+ .name = "nct7363",
> >+ .of_match_table = nct7363_of_match,
> >+ },
> >+ .probe = nct7363_probe,
> >+};
> >+
> >+module_i2c_driver(nct7363_driver);
> >+
> >+MODULE_AUTHOR("CW Ho <cwho@nuvoton.com>");
> >+MODULE_AUTHOR("Ban Feng <kcfeng0@nuvoton.com>");
> >+MODULE_DESCRIPTION("NCT7363 Hardware Monitoring Driver");
> >+MODULE_LICENSE("GPL");
> >--
> >2.34.1
> >
> >
WARNING: multiple messages have this Message-ID (diff)
From: Ban Feng <baneric926@gmail.com>
To: Zev Weiss <zev@bewilderbeest.net>
Cc: linux-hwmon@vger.kernel.org, devicetree@vger.kernel.org,
conor+dt@kernel.org, jdelvare@suse.com, corbet@lwn.net,
openbmc@lists.ozlabs.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, DELPHINE_CHIU@wiwynn.com,
naresh.solanki@9elements.com, billy_tsai@aspeedtech.com,
kcfeng0@nuvoton.com, robh+dt@kernel.org,
krzysztof.kozlowski+dt@linaro.org, kwliu@nuvoton.com,
linux@roeck-us.net
Subject: Re: [PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y
Date: Thu, 7 Mar 2024 15:35:31 +0800 [thread overview]
Message-ID: <CALz278Zgfgob573vgWz4PgC7vb=i8xt3kC1hSjo_cQi00B0XAg@mail.gmail.com> (raw)
In-Reply-To: <a90ed00c-f836-4fb6-8191-9974937e3eb7@hatter.bewilderbeest.net>
Hi Zev,
On Sat, Mar 2, 2024 at 4:19 PM Zev Weiss <zev@bewilderbeest.net> wrote:
>
> On Mon, Feb 26, 2024 at 04:56:06PM PST, baneric926@gmail.com wrote:
> >From: Ban Feng <kcfeng0@nuvoton.com>
> >
> >NCT7363Y is an I2C based hardware monitoring chip from Nuvoton.
> >
> >Signed-off-by: Ban Feng <kcfeng0@nuvoton.com>
> >---
> > Documentation/hwmon/index.rst | 1 +
> > Documentation/hwmon/nct7363.rst | 33 +++
> > MAINTAINERS | 2 +
> > drivers/hwmon/Kconfig | 11 +
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/nct7363.c | 412 ++++++++++++++++++++++++++++++++
> > 6 files changed, 460 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 c7ed1f73ac06..302f954b45be 100644
> >--- a/Documentation/hwmon/index.rst
> >+++ b/Documentation/hwmon/index.rst
> >@@ -165,6 +165,7 @@ Hardware Monitoring Kernel Drivers
> > mp5990
> > 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..89699c95aa4b
> >--- /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@nuvoton.com>
> >+
> >+
> >+Description
> >+-----------
> >+
> >+The NCT7363Y is a Fan controller which provides up to 16 independent
> >+FAN input monitors, and up to 16 independent PWM output 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 7b1efefed7c4..7ca66b713e30 100644
> >--- a/MAINTAINERS
> >+++ b/MAINTAINERS
> >@@ -15089,6 +15089,8 @@ M: Ban Feng <kcfeng0@nuvoton.com>
> > L: linux-hwmon@vger.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@kernel.org>
> >diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> >index a608264da87d..a297b5409b04 100644
> >--- a/drivers/hwmon/Kconfig
> >+++ b/drivers/hwmon/Kconfig
> >@@ -1616,6 +1616,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 47be39af5c03..d5e7531204df 100644
> >--- a/drivers/hwmon/Makefile
> >+++ b/drivers/hwmon/Makefile
> >@@ -167,6 +167,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..c79d3ca4f111
> >--- /dev/null
> >+++ b/drivers/hwmon/nct7363.c
> >@@ -0,0 +1,412 @@
> >+// 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_GPIO_0_3 0x20
> >+#define NCT7363_REG_GPIO_4_7 0x21
> >+#define NCT7363_REG_GPIO_10_13 0x22
> >+#define NCT7363_REG_GPIO_14_17 0x23
> >+#define NCT7363_REG_PWMEN_0_7 0x38
> >+#define NCT7363_REG_PWMEN_8_15 0x39
> >+#define NCT7363_REG_FANINEN_0_7 0x41
> >+#define NCT7363_REG_FANINEN_8_15 0x42
> >+#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) % 4) * 2))
> >+#define FANIN_SEL(x) (BIT(1) << (((x) % 4) * 2))
> >+
> >+#define NCT7363_FANINX_LVAL_MASK GENMASK(4, 0)
> >+#define NCT7363_FANIN_MASK GENMASK(12, 0)
> >+
> >+#define NCT7363_PWM_COUNT 16
> >+
> >+static inline unsigned long FAN_FROM_REG(u16 val)
> >+{
> >+ /* In case fan is stopped or divide by 0 */
> >+ if (val == NCT7363_FANIN_MASK || val == 0)
> >+ return 0;
> >+
> >+ return (1350000UL / val);
> >+}
> >+
> >+static const struct of_device_id nct7363_of_match[] = {
> >+ { .compatible = "nuvoton,nct7363" },
>
> As far as I can see from the code in this driver, it looks like this
> driver should also be compatible with the nct7362; shall we add the ID
> table entry for it now? (Though I only have a datasheet for the
> nct7362, not the nct7363, so I don't know exactly how they differ.)
As far as I know, the difference between these two ICs is 0x2A~0x2C
Fading LED for 7362, and 0x2A Watchdog Timer for 7363.
In my v1 patch, I indeed add the nct7362 to the ID table, however,
this makes it more complicated and our datasheet isn't public yet.
I think maybe supporting more chips will be done in the future, but not now.
>
> >+ { },
> >+};
> >+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;
> >+ u16 cnt, rpm;
> >+ int ret = 0;
> >+
> >+ switch (attr) {
> >+ case hwmon_fan_input:
> >+ /*
> >+ * High-byte register should be read first to latch
> >+ * synchronous low-byte value
> >+ */
> >+ ret = regmap_read(data->regmap,
> >+ NCT7363_REG_FANINX_HVAL(channel), &hi);
> >+ if (ret)
> >+ return ret;
> >+
> >+ ret = regmap_read(data->regmap,
> >+ NCT7363_REG_FANINX_LVAL(channel), &lo);
> >+ if (ret)
> >+ return ret;
>
> I think this pair of reads should be done under data->lock to ensure
> that the LVAL read gets the right latched value in a scenario where
> multiple threads executing this function end up with their register
> reads interleaved.
ok, fix in v5
>
> >+
> >+ cnt = (hi << 5) | (lo & NCT7363_FANINX_LVAL_MASK);
> >+ rpm = FAN_FROM_REG(cnt);
> >+ *val = (long)rpm;
>
> Given that rpm is assigned from an unsigned long (FAN_FROM_REG()) and
> then to a long (*val), is there any reason we want it as a u16 in
> between the two? Or for that matter, why not just:
>
> *val = FAN_FROM_REG(cnt);
>
> ?
I'll modify this to align with the style of the nct7363_read_pwm function in v5.
>
> >+ return 0;
> >+ default:
> >+ return -EOPNOTSUPP;
> >+ }
> >+}
> >+
> >+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;
> >+ u16 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;
> >+
> >+ mutex_lock(&data->lock);
> >+ ret = regmap_write(data->regmap,
> >+ NCT7363_REG_FSCPXDUTY(channel), val);
> >+ mutex_unlock(&data->lock);
>
> ...though here, I'm not sure the locking is needed -- is there something
> that necessitates it for a single register write?
ok, fix in v5
>
> >+
> >+ 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),
> >+ 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),
> >+ 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)
> >+{
> >+ u8 i, gpio0_3 = 0, gpio4_7 = 0, gpio10_13 = 0, gpio14_17 = 0;
> >+ int ret;
> >+
> >+ for (i = 0; i < NCT7363_PWM_COUNT; i++) {
> >+ if (i < 4) {
> >+ if (data->pwm_mask & BIT(i))
> >+ gpio0_3 |= PWM_SEL(i);
> >+ if (data->fanin_mask & BIT(i))
> >+ gpio10_13 |= FANIN_SEL(i);
> >+ } else if (i < 8) {
> >+ if (data->pwm_mask & BIT(i))
> >+ gpio4_7 |= PWM_SEL(i);
> >+ if (data->fanin_mask & BIT(i))
> >+ gpio14_17 |= FANIN_SEL(i);
> >+ } else if (i < 12) {
> >+ if (data->pwm_mask & BIT(i))
> >+ gpio10_13 |= PWM_SEL(i);
> >+ if (data->fanin_mask & BIT(i))
> >+ gpio0_3 |= FANIN_SEL(i);
> >+ } else {
> >+ if (data->pwm_mask & BIT(i))
> >+ gpio14_17 |= PWM_SEL(i);
> >+ if (data->fanin_mask & BIT(i))
> >+ gpio4_7 |= FANIN_SEL(i);
> >+ }
> >+ }
>
> With the caveat that I haven't actually sketched it out myself to be
> sure, might it be a bit cleaner to do this with a 4-element array
> indexed by 'i / 4' instead of a big if/else chain and a bunch of
> near-duplicated blocks?
ok, fix in v5
>
> >+
> >+ /* Pin Function Configuration */
> >+ ret = regmap_write(data->regmap, NCT7363_REG_GPIO_0_3, gpio0_3);
> >+ if (ret < 0)
> >+ return ret;
> >+ ret = regmap_write(data->regmap, NCT7363_REG_GPIO_4_7, gpio4_7);
> >+ if (ret < 0)
> >+ return ret;
> >+ ret = regmap_write(data->regmap, NCT7363_REG_GPIO_10_13, gpio10_13);
> >+ if (ret < 0)
> >+ return ret;
> >+ ret = regmap_write(data->regmap, NCT7363_REG_GPIO_14_17, gpio14_17);
> >+ if (ret < 0)
> >+ return ret;
> >+
> >+ /* PWM and FANIN Monitoring Enable */
> >+ ret = regmap_write(data->regmap, NCT7363_REG_PWMEN_0_7,
> >+ data->pwm_mask & 0xff);
> >+ if (ret < 0)
> >+ return ret;
> >+ ret = regmap_write(data->regmap, NCT7363_REG_PWMEN_8_15,
> >+ (data->pwm_mask >> 8) & 0xff);
> >+ if (ret < 0)
> >+ return ret;
> >+ ret = regmap_write(data->regmap, NCT7363_REG_FANINEN_0_7,
> >+ data->fanin_mask & 0xff);
> >+ if (ret < 0)
> >+ return ret;
> >+ ret = regmap_write(data->regmap, NCT7363_REG_FANINEN_8_15,
> >+ (data->fanin_mask >> 8) & 0xff);
> >+ if (ret < 0)
> >+ return ret;
> >+
> >+ return 0;
> >+}
> >+
> >+static int nct7363_present_pwm_fanin(struct device *dev,
> >+ struct device_node *child,
> >+ struct nct7363_data *data)
> >+{
> >+ struct of_phandle_args args;
> >+ int ret, fanin_cnt;
> >+ u8 *fanin_ch;
> >+ u8 ch, index;
> >+
> >+ ret = of_parse_phandle_with_args(child, "pwms", "#pwm-cells",
> >+ 0, &args);
> >+ if (ret)
> >+ return ret;
> >+
> >+ data->pwm_mask |= BIT(args.args[0]);
>
> Perhaps we should have
>
> if (args.args[0] >= NCT7363_PWM_COUNT)
> return -EINVAL;
>
> here?
ok, fix in v5
>
> >+
> >+ fanin_cnt = of_property_count_u8_elems(child, "tach-ch");
> >+ if (fanin_cnt < 1)
>
> fanin_cnt < 1 || fanin_cnt >= NCT7363_PWM_COUNT
ok, fix in v5
>
> >+ return -EINVAL;
> >+
> >+ fanin_ch = devm_kcalloc(dev, fanin_cnt, sizeof(*fanin_ch), GFP_KERNEL);
>
> Keeping this allocation around persistently for the whole lifetime of
> the device seems unnecessary -- it's not used beyond this function,
> right? In fact, dynamically allocating it at all seems like overkill,
> considering that in addition to being temporary, it's also got a pretty
> small upper bound on its size. I'd think a simple
>
> u8 fanin_ch[NCT7363_PWM_COUNT];
>
> would suffice? (Along with the check above to ensure fanin_cnt is
> within range of course.)
ok, fix in v5
>
> >+ if (!fanin_ch)
> >+ return -ENOMEM;
> >+
> >+ 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];
>
> ...and likewise a range check here too?
ok, fix in v5
>
> >+ data->fanin_mask |= BIT(index);
> >+ }
> >+
>
> Should we also grab the pulses-per-revolution property here and factor
> that in in FAN_FROM_REG()?
So far our FanPoles in tachometer count calculation formula is always 4.
Therefore, I don't add the pulses-per-revolution property into FAN_FROM_REG().
>
> >+ return 0;
> >+}
> >+
> >+static const struct regmap_config nct7363_regmap_config = {
> >+ .reg_bits = 8,
> >+ .val_bits = 8,
> >+};
> >+
> >+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,
>
> Maybe add an i2c_device_id table to accompany the of_match table?
ok, fix in v5
Thanks,
Ban
>
> >+ .driver = {
> >+ .name = "nct7363",
> >+ .of_match_table = nct7363_of_match,
> >+ },
> >+ .probe = nct7363_probe,
> >+};
> >+
> >+module_i2c_driver(nct7363_driver);
> >+
> >+MODULE_AUTHOR("CW Ho <cwho@nuvoton.com>");
> >+MODULE_AUTHOR("Ban Feng <kcfeng0@nuvoton.com>");
> >+MODULE_DESCRIPTION("NCT7363 Hardware Monitoring Driver");
> >+MODULE_LICENSE("GPL");
> >--
> >2.34.1
> >
> >
next prev parent reply other threads:[~2024-03-07 7:35 UTC|newest]
Thread overview: 56+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-27 0:56 [PATCH v4 0/3] hwmon: Driver for Nuvoton NCT7363Y baneric926
2024-02-27 0:56 ` baneric926
2024-02-27 0:56 ` [PATCH v4 1/3] dt-bindings: hwmon: fan: Add fan binding to schema baneric926
2024-02-27 0:56 ` baneric926
2024-03-05 0:22 ` Zev Weiss
2024-03-05 0:22 ` Zev Weiss
2024-03-05 0:41 ` Guenter Roeck
2024-03-05 0:41 ` Guenter Roeck
2024-03-05 0:49 ` Guenter Roeck
2024-03-05 0:49 ` Guenter Roeck
2024-03-07 7:41 ` Ban Feng
2024-03-07 7:41 ` Ban Feng
2024-03-07 18:53 ` Guenter Roeck
2024-03-07 18:53 ` Guenter Roeck
2024-03-18 0:58 ` Ban Feng
2024-03-18 0:58 ` Ban Feng
2024-02-27 0:56 ` [PATCH v4 2/3] dt-bindings: hwmon: Add NCT7363Y documentation baneric926
2024-02-27 0:56 ` baneric926
2024-02-28 7:30 ` Paul Menzel
2024-02-28 7:30 ` Paul Menzel
2024-03-06 7:56 ` Ban Feng
2024-03-06 7:56 ` Ban Feng
2024-02-27 0:56 ` [PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y baneric926
2024-02-27 0:56 ` baneric926
2024-02-28 7:57 ` Paul Menzel
2024-02-28 7:57 ` Paul Menzel
2024-02-28 9:03 ` Guenter Roeck
2024-02-28 9:03 ` Guenter Roeck
2024-02-28 11:03 ` Paul Menzel
2024-02-28 11:03 ` Paul Menzel
2024-02-28 16:03 ` Guenter Roeck
2024-02-28 16:03 ` Guenter Roeck
2024-02-28 16:25 ` Commit messages (was: [PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y) Paul Menzel
2024-02-28 16:25 ` Paul Menzel
2024-03-07 0:58 ` Ban Feng
2024-03-07 0:58 ` Ban Feng
2024-03-07 0:55 ` [PATCH v4 3/3] hwmon: Driver for Nuvoton NCT7363Y Ban Feng
2024-03-07 0:55 ` Ban Feng
2024-03-02 8:18 ` Zev Weiss
2024-03-02 8:18 ` Zev Weiss
2024-03-05 0:28 ` Zev Weiss
2024-03-05 0:28 ` Zev Weiss
2024-03-07 7:36 ` Ban Feng
2024-03-07 7:36 ` Ban Feng
2024-03-07 7:35 ` Ban Feng [this message]
2024-03-07 7:35 ` Ban Feng
2024-03-12 23:18 ` Zev Weiss
2024-03-12 23:18 ` Zev Weiss
2024-03-12 23:58 ` Guenter Roeck
2024-03-12 23:58 ` Guenter Roeck
2024-03-13 0:21 ` Zev Weiss
2024-03-13 0:21 ` Zev Weiss
2024-03-18 1:02 ` Ban Feng
2024-03-18 1:02 ` Ban Feng
2024-03-19 0:35 ` Zev Weiss
2024-03-19 0:35 ` Zev Weiss
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CALz278Zgfgob573vgWz4PgC7vb=i8xt3kC1hSjo_cQi00B0XAg@mail.gmail.com' \
--to=baneric926@gmail.com \
--cc=DELPHINE_CHIU@wiwynn.com \
--cc=billy_tsai@aspeedtech.com \
--cc=conor+dt@kernel.org \
--cc=corbet@lwn.net \
--cc=devicetree@vger.kernel.org \
--cc=jdelvare@suse.com \
--cc=kcfeng0@nuvoton.com \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=kwliu@nuvoton.com \
--cc=linux-doc@vger.kernel.org \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=naresh.solanki@9elements.com \
--cc=openbmc@lists.ozlabs.org \
--cc=robh+dt@kernel.org \
--cc=zev@bewilderbeest.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.