| Message ID | 20241226055313.2841977-3-j2anfernee@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: add Nuvoton NCT7201 ADC driver | expand |
On Thu, Dec 26, 2024 at 01:53:13PM +0800, Eason Yang wrote: > Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver > > NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to > 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for > independent alarm signals, and the all threshold values could be set for > system protection without any timing delay. It also supports reset input > RSTIN# to recover system from a fault condition. > > Currently, only single-edge mode conversion and threshold events support. ... > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + err = regmap_read(chip->regmap16, NCT7201_REG_VIN(chan->address), &value); > + if (err < 0) > + return err; > + volt = value; > + *val = volt >> 3; I am not sure I understand this. If it's a shifted field, you rather should fix it by using FIELD_*() macros from bitfield.h, otherwise it's even more confusing. > + return IIO_VAL_INT; > + case IIO_CHAN_INFO_SCALE: > + /* From the datasheet, we have to multiply by 0.0004995 */ > + *val = 0; > + *val2 = 499500; > + return IIO_VAL_INT_PLUS_NANO; > + default: > + return -EINVAL; > + } ... > + *val = volt >> 3; Ditto. ... > + v1 = val >> 5; > + v2 = FIELD_PREP(NCT7201_REG_VIN_LIMIT_LSB_MASK, val) << 3; Ditto. > + if (chan->type != IIO_VOLTAGE) > + return -EOPNOTSUPP; > + > + if (info == IIO_EV_INFO_VALUE) { > + guard(mutex)(&chip->access_lock); > + if (dir == IIO_EV_DIR_FALLING) { > + regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT(chan->address), v1); > + regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT_LSB(chan->address), v2); > + } else { > + regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT(chan->address), v1); > + regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT_LSB(chan->address), v2); > + } This needs a comment why regmap_bulk_write() can't be used. > + } ... > +static int nct7201_init_chip(struct nct7201_chip_info *chip) > +{ > + u8 data[2]; > + unsigned int value; > + int err; > + > + regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION, > + NCT7201_BIT_CONFIGURATION_RESET); No error check? > + /* > + * After about 25 msecs, the device should be ready and then > + * the Power Up bit will be set to 1. If not, wait for it. > + */ > + mdelay(25); > + err = regmap_read(chip->regmap, NCT7201_REG_BUSY_STATUS, &value); > + if (err < 0) > + return err; > + if (!(value & NCT7201_BIT_PWR_UP)) > + return dev_err_probe(&chip->client->dev, -EIO, "failed to power up after reset\n"); > + > + /* Enable Channel */ > + guard(mutex)(&chip->access_lock); > + regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1, > + NCT7201_REG_CHANNEL_ENABLE_1_MASK); > + if (chip->num_vin_channels == 12) > + regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2, > + NCT7201_REG_CHANNEL_ENABLE_2_MASK); > + > + err = regmap_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1, &value); > + if (err < 0) > + return err; > + data[0] = value; > + > + err = regmap_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2, &value); > + if (err < 0) > + return err; > + data[1] = value; > + > + value = get_unaligned_le16(data); > + chip->vin_mask = value; > + > + /* Start monitoring if needed */ > + err = regmap_read(chip->regmap, NCT7201_REG_CONFIGURATION, &value); > + if (err < 0) { > + dev_err_probe(&chip->client->dev, -EIO, "Failed to read REG_CONFIGURATION\n"); > + return value; You already used return dev_err_probe(...); above, why here it's different? You want return something in addition to the error code? Why? > + } > + > + value |= NCT7201_BIT_CONFIGURATION_START; > + regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION, value); > + > + return 0; > +} ... > +static const struct regmap_config nct7201_regmap8_config = { > + .name = "vin-data-read-byte", > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = 0xff, > +}; > + > +static const struct regmap_config nct7201_regmap16_config = { > + .name = "vin-data-read-word", > + .reg_bits = 8, > + .val_bits = 16, > + .max_register = 0xff, > +}; I don't see how these configurations will prevent, e.g., debugfs to access 16-bit registers via 8-bit IO and vice versa.
On Thu, 26 Dec 2024 13:53:13 +0800 Eason Yang <j2anfernee@gmail.com> wrote: > Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver > > NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to > 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for > independent alarm signals, and the all threshold values could be set for > system protection without any timing delay. It also supports reset input > RSTIN# to recover system from a fault condition. > > Currently, only single-edge mode conversion and threshold events support. > > Signed-off-by: Eason Yang <j2anfernee@gmail.com> Hi Eason, Various minor comments in addition to what Andy has posted already. Jonathan > diff --git a/drivers/iio/adc/nct7201.c b/drivers/iio/adc/nct7201.c > new file mode 100644 > index 000000000000..9ad4d2919461 > --- /dev/null > +++ b/drivers/iio/adc/nct7201.c > @@ -0,0 +1,449 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Driver for Nuvoton nct7201 and nct7202 power monitor chips. > + * > + * Copyright (c) 2024 Nuvoton Inc. > + */ > + > +#include <linux/array_size.h> > +#include <linux/bits.h> > +#include <linux/cleanup.h> > +#include <linux/delay.h> > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/i2c.h> > +#include <linux/init.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/regmap.h> > +#include <linux/types.h> > +#include <linux/unaligned.h> > + > +#include <linux/iio/events.h> > +#include <linux/iio/iio.h> > + > +#define NCT7201_VIN_MAX 12 /* Counted from 1 */ > +#define NCT7201_IN_SCALING 4995 > +#define NCT7201_IN_SCALING_FACTOR 10000 > + > +#define NCT7201_REG_INTERRUPT_STATUS_1 0x0C > +#define NCT7201_REG_INTERRUPT_STATUS_2 0x0D > +#define NCT7201_REG_VOLT_LOW_BYTE 0x0F > +#define NCT7201_REG_CONFIGURATION 0x10 > +#define NCT7201_BIT_CONFIGURATION_START BIT(0) > +#define NCT7201_BIT_CONFIGURATION_ALERT_MSK BIT(1) > +#define NCT7201_BIT_CONFIGURATION_CONV_RATE BIT(2) > +#define NCT7201_BIT_CONFIGURATION_RESET BIT(7) > + > +#define NCT7201_REG_ADVANCED_CONFIGURATION 0x11 > +#define NCT7201_BIT_ADVANCED_CONF_MOD_ALERT BIT(0) > +#define NCT7201_BIT_ADVANCED_CONF_MOD_STS BIT(1) > +#define NCT7201_BIT_ADVANCED_CONF_FAULT_QUEUE BIT(2) > +#define NCT7201_BIT_ADVANCED_CONF_EN_DEEP_SHUTDOWN BIT(4) > +#define NCT7201_BIT_ADVANCED_CONF_EN_SMB_TIMEOUT BIT(5) > +#define NCT7201_BIT_ADVANCED_CONF_MOD_RSTIN BIT(7) > + > +#define NCT7201_REG_CHANNEL_INPUT_MODE 0x12 > +#define NCT7201_REG_CHANNEL_ENABLE_1 0x13 > +#define NCT7201_REG_CHANNEL_ENABLE_1_MASK GENMASK(7, 0) > +#define NCT7201_REG_CHANNEL_ENABLE_2 0x14 > +#define NCT7201_REG_CHANNEL_ENABLE_2_MASK GENMASK(3, 0) As below. I'd treat these two registers as one larger register. > +static int nct7201_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + int *val, int *val2, long mask) > +{ > + u16 volt; > + unsigned int value; > + int err; > + struct nct7201_chip_info *chip = iio_priv(indio_dev); > + > + if (chan->type != IIO_VOLTAGE) > + return -EOPNOTSUPP; > + > + guard(mutex)(&chip->access_lock); > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + err = regmap_read(chip->regmap16, NCT7201_REG_VIN(chan->address), &value); > + if (err < 0) > + return err; > + volt = value; > + *val = volt >> 3; As below, likely a FIELD_GET() is appropriate here. > + return IIO_VAL_INT; > + case IIO_CHAN_INFO_SCALE: > + /* From the datasheet, we have to multiply by 0.0004995 */ > + *val = 0; > + *val2 = 499500; > + return IIO_VAL_INT_PLUS_NANO; > + default: > + return -EINVAL; > + } > +} > + > +static int nct7201_read_event_value(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, > + enum iio_event_info info, > + int *val, int *val2) > +{ > + struct nct7201_chip_info *chip = iio_priv(indio_dev); > + u16 volt; > + unsigned int value; > + int err; > + > + if (chan->type != IIO_VOLTAGE) > + return -EOPNOTSUPP; > + > + if (info != IIO_EV_INFO_VALUE) > + return -EINVAL; > + > + if (dir == IIO_EV_DIR_FALLING) { > + err = regmap_read(chip->regmap16, NCT7201_REG_VIN_LOW_LIMIT(chan->address), &value); > + if (err < 0) > + return err; > + volt = value; > + } else { > + err = regmap_read(chip->regmap16, NCT7201_REG_VIN_HIGH_LIMIT(chan->address), &value); > + if (err < 0) > + return err; > + volt = value; > + } > + > + *val = volt >> 3; As Andy pointed out, likely a FIELD_GET() makes sense here. > + > + return IIO_VAL_INT; > +} > + > +static int nct7201_write_event_value(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, > + enum iio_event_info info, > + int val, int val2) > +{ > + struct nct7201_chip_info *chip = iio_priv(indio_dev); > + long v1, v2; > + > + v1 = val >> 5; > + v2 = FIELD_PREP(NCT7201_REG_VIN_LIMIT_LSB_MASK, val) << 3; > + > + if (chan->type != IIO_VOLTAGE) > + return -EOPNOTSUPP; > + > + if (info == IIO_EV_INFO_VALUE) { I'd flip this to if (info != IIO_EV_INFO_VALUE) return -EOPNOTSUPP; guard(). > + guard(mutex)(&chip->access_lock); > + if (dir == IIO_EV_DIR_FALLING) { > + regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT(chan->address), v1); > + regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT_LSB(chan->address), v2); Check for errors. > + } else { > + regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT(chan->address), v1); > + regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT_LSB(chan->address), v2); > + } > + } > + > + return 0; > +} > + > +static int nct7201_write_event_config(struct iio_dev *indio_dev, > + const struct iio_chan_spec *chan, > + enum iio_event_type type, > + enum iio_event_direction dir, > + bool state) > +{ > + struct nct7201_chip_info *chip = iio_priv(indio_dev); > + unsigned int mask; > + > + if (chan->type != IIO_VOLTAGE) > + return -EOPNOTSUPP; > + > + mask = BIT(chan->address); > + > + if (!state && (chip->vin_mask & mask)) > + chip->vin_mask &= ~mask; > + else if (state && !(chip->vin_mask & mask)) > + chip->vin_mask |= mask; > + > + guard(mutex)(&chip->access_lock); > + regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1, > + chip->vin_mask & NCT7201_REG_CHANNEL_ENABLE_1_MASK); > + if (chip->num_vin_channels == 12) > + regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2, > + chip->vin_mask & NCT7201_REG_CHANNEL_ENABLE_2_MASK); This feels odd. Don't you need to shift that vin_mask to get rid of channels that are enabled via ENABLE_1? > + > + return 0; > +} > +static int nct7201_init_chip(struct nct7201_chip_info *chip) > +{ > + u8 data[2]; > + unsigned int value; > + int err; > + > + regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION, > + NCT7201_BIT_CONFIGURATION_RESET); Add a comment on why you don't check return value (or do check it if appropriate). > + > + /* > + * After about 25 msecs, the device should be ready and then > + * the Power Up bit will be set to 1. If not, wait for it. > + */ > + mdelay(25); > + err = regmap_read(chip->regmap, NCT7201_REG_BUSY_STATUS, &value); > + if (err < 0) > + return err; > + if (!(value & NCT7201_BIT_PWR_UP)) > + return dev_err_probe(&chip->client->dev, -EIO, "failed to power up after reset\n"); > + > + /* Enable Channel */ > + guard(mutex)(&chip->access_lock); > + regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1, > + NCT7201_REG_CHANNEL_ENABLE_1_MASK); Check return value. This is over an I2C bus, not the most reliable of transports! Consider doing this differently and using a bulk write for the larger case. if (chip->num_vin_channels <= 8) ret = regmap_write(); else ret = regmap_bulk_write(); However as you read ENABLE_2 unconditionally below, can you instead just always use a bulk write here? > + if (chip->num_vin_channels == 12) > + regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2, > + NCT7201_REG_CHANNEL_ENABLE_2_MASK); > + > + err = regmap_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1, &value); > + if (err < 0) > + return err; > + data[0] = value; > + > + err = regmap_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2, &value); > + if (err < 0) > + return err; > + data[1] = value; Why does reading these back make sense? Is there a reason the write above might not work if the I2C transfer is Acked? If this is part of discovery protocol for how many channels are there, then add comments. > + > + value = get_unaligned_le16(data); > + chip->vin_mask = value; > + > + /* Start monitoring if needed */ > + err = regmap_read(chip->regmap, NCT7201_REG_CONFIGURATION, &value); > + if (err < 0) { > + dev_err_probe(&chip->client->dev, -EIO, "Failed to read REG_CONFIGURATION\n"); > + return value; Why return value if an error occurred? retuen dev_err_probe(); > + } > + > + value |= NCT7201_BIT_CONFIGURATION_START; > + regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION, value); regmap_set_bits() > + > + return 0; > +} > + > +static const struct regmap_config nct7201_regmap8_config = { > + .name = "vin-data-read-byte", > + .reg_bits = 8, > + .val_bits = 8, > + .max_register = 0xff, > +}; > + > +static const struct regmap_config nct7201_regmap16_config = { > + .name = "vin-data-read-word", > + .reg_bits = 8, > + .val_bits = 16, > + .max_register = 0xff, > +}; > + > +static int nct7201_probe(struct i2c_client *client) > +{ > + const struct nct7201_adc_model_data *model_data; > + struct nct7201_chip_info *chip; > + struct iio_dev *indio_dev; > + int ret; > + > + model_data = i2c_get_match_data(client); > + if (!model_data) > + return -EINVAL; > + > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip)); > + if (!indio_dev) > + return -ENOMEM; > + chip = iio_priv(indio_dev); > + > + chip->regmap = devm_regmap_init_i2c(client, &nct7201_regmap8_config); > + if (IS_ERR(chip->regmap)) > + return dev_err_probe(&client->dev, PTR_ERR(chip->regmap), > + "Failed to init regmap\n"); > + > + chip->regmap16 = devm_regmap_init_i2c(client, &nct7201_regmap16_config); > + if (IS_ERR(chip->regmap16)) > + return dev_err_probe(&client->dev, PTR_ERR(chip->regmap16), > + "Failed to init regmap16\n"); > + > + chip->num_vin_channels = model_data->num_vin_channels; > + > + ret = devm_mutex_init(&client->dev, &chip->access_lock); > + if (ret) > + return ret; > + > + chip->client = client; > + > + ret = nct7201_init_chip(chip); > + if (ret < 0) > + return ret; > + > + indio_dev->name = model_data->model_name; > + indio_dev->channels = model_data->channels; > + indio_dev->num_channels = model_data->num_channels; > + if (client->irq) > + indio_dev->info = &nct7201_info; > + else > + indio_dev->info = &nct7201_info_no_irq; > + indio_dev->modes = INDIO_DIRECT_MODE; > + > + return devm_iio_device_register(&client->dev, indio_dev); > +}
Dear Andy, Thanks for your comments. Andy Shevchenko <andriy.shevchenko@linux.intel.com> 於 2024年12月27日 週五 下午8:14寫道： > > On Thu, Dec 26, 2024 at 01:53:13PM +0800, Eason Yang wrote: > > Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver > > > > NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to > > 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for > > independent alarm signals, and the all threshold values could be set for > > system protection without any timing delay. It also supports reset input > > RSTIN# to recover system from a fault condition. > > > > Currently, only single-edge mode conversion and threshold events support. > > ... > > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + err = regmap_read(chip->regmap16, NCT7201_REG_VIN(chan->address), &value); > > + if (err < 0) > > + return err; > > + volt = value; > > > + *val = volt >> 3; > > I am not sure I understand this. If it's a shifted field, you rather > should fix it by using FIELD_*() macros from bitfield.h, otherwise > it's even more confusing. > + #define NCT7201_REG_VIN_MASK GENMASK(15, 3) - *val = volt >> 3; + *val = FIELD_GET(NCT7201_REG_VIN_MASK, volt); > > + return IIO_VAL_INT; > > + case IIO_CHAN_INFO_SCALE: > > + /* From the datasheet, we have to multiply by 0.0004995 */ > > + *val = 0; > > + *val2 = 499500; > > + return IIO_VAL_INT_PLUS_NANO; > > + default: > > + return -EINVAL; > > + } > > ... > > > + *val = volt >> 3; > > Ditto. > > > ... > > > + v1 = val >> 5; > > + v2 = FIELD_PREP(NCT7201_REG_VIN_LIMIT_LSB_MASK, val) << 3; > > Ditto. > The VIN reading supports Byte read (One Byte) and Word read (Two Byte), the same as the VIN writing. We don't need to separate 13-bit val into two bytes. We can use 16-bit regmap to write val with right shift 3 bit to replace write 8-bit regmap register twice. > > + if (chan->type != IIO_VOLTAGE) > > + return -EOPNOTSUPP; > > + > > + if (info == IIO_EV_INFO_VALUE) { > > + guard(mutex)(&chip->access_lock); > > + if (dir == IIO_EV_DIR_FALLING) { > > + regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT(chan->address), v1); > > + regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT_LSB(chan->address), v2); > > + } else { > > + regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT(chan->address), v1); > > + regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT_LSB(chan->address), v2); > > + } if (dir == IIO_EV_DIR_FALLING) - regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT(chan->address), v1); - regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT_LSB(chan->address), v2); + regmap_write(chip->regmap16, NCT7201_REG_VIN_LOW_LIMIT(chan->address), FIELD_PREP(NCT7201_REG_VIN_MASK, val)); else - regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT(chan->address), v1); - regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT_LSB(chan->address), v2); + regmap_write(chip->regmap16, NCT7201_REG_VIN_HIGH_LIMIT(chan->address), FIELD_PREP(NCT7201_REG_VIN_MASK, val)); > > This needs a comment why regmap_bulk_write() can't be used. > We can't use regmap_bulk_write() since the chip limit. regmap_bulk_write(chip->regmap, ..., ..., 2) , the first byte is well written, but the second byte don't changed. > > + } > > ... > > > +static int nct7201_init_chip(struct nct7201_chip_info *chip) > > +{ > > + u8 data[2]; > > + unsigned int value; > > + int err; > > + > > + regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION, > > + NCT7201_BIT_CONFIGURATION_RESET); > > No error check? > As David Lechner mentioned, Better would be `return dev_err_probe()`. But it is very rare for regmap_write() to fail so usually we don't print an error message for these. So we would remove print an error message for all remap_write. > > + /* > > + * After about 25 msecs, the device should be ready and then > > + * the Power Up bit will be set to 1. If not, wait for it. > > + */ > > + mdelay(25); > > + err = regmap_read(chip->regmap, NCT7201_REG_BUSY_STATUS, &value); > > + if (err < 0) > > + return err; > > + if (!(value & NCT7201_BIT_PWR_UP)) > > + return dev_err_probe(&chip->client->dev, -EIO, "failed to power up after reset\n"); > > + > > + /* Enable Channel */ > > + guard(mutex)(&chip->access_lock); > > + regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1, > > + NCT7201_REG_CHANNEL_ENABLE_1_MASK); > > + if (chip->num_vin_channels == 12) > > + regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2, > > + NCT7201_REG_CHANNEL_ENABLE_2_MASK); > > + > > + err = regmap_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1, &value); > > + if (err < 0) > > + return err; > > + data[0] = value; > > + > > + err = regmap_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2, &value); > > + if (err < 0) > > + return err; > > + data[1] = value; > > + > > + value = get_unaligned_le16(data); > > + chip->vin_mask = value; > > + > > + /* Start monitoring if needed */ > > + err = regmap_read(chip->regmap, NCT7201_REG_CONFIGURATION, &value); > > + if (err < 0) { > > > + dev_err_probe(&chip->client->dev, -EIO, "Failed to read REG_CONFIGURATION\n"); > > + return value; > > You already used > > return dev_err_probe(...); > > above, why here it's different? You want return something in addition to the > error code? Why? > It should use return dev_err_probe(&chip->client->dev, -EIO, "Failed to read REG_CONFIGURATION\n"); > > + } > > + > > + value |= NCT7201_BIT_CONFIGURATION_START; > > + regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION, value); > > + > > + return 0; > > +} > > ... > > > +static const struct regmap_config nct7201_regmap8_config = { > > + .name = "vin-data-read-byte", > > + .reg_bits = 8, > > + .val_bits = 8, > > + .max_register = 0xff, > > +}; > > + > > +static const struct regmap_config nct7201_regmap16_config = { > > + .name = "vin-data-read-word", > > + .reg_bits = 8, > > + .val_bits = 16, > > + .max_register = 0xff, > > +}; > > I don't see how these configurations will prevent, e.g., debugfs to access > 16-bit registers via 8-bit IO and vice versa. > Read VIN info can use word read or byte read, and other registers should use byte read. The design is that VIN info registers are used 16-bit debugfs to access and other registers are used 8-bit debugfs to access. We need to probe 8-bit regmap and 16-bit regmap, but I have no idea how to prevent 8-bit IO to access 16-bit registers and vice versa. > -- > With Best Regards, > Andy Shevchenko > >
Dear Jonathan, Thanks for your comments. Some questions are replied in Andy's comments. We use FIELD_GET and FIELD_PREP to handle bit shift operations. About regmap_write case, we remove print an error message according to David Lechner mentioned. If check for errors are needed, we would recovery it. Jonathan Cameron <jic23@kernel.org> 於 2024年12月28日 週六 下午9:35寫道： > > On Thu, 26 Dec 2024 13:53:13 +0800 > Eason Yang <j2anfernee@gmail.com> wrote: > > > Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver > > > > NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to > > 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for > > independent alarm signals, and the all threshold values could be set for > > system protection without any timing delay. It also supports reset input > > RSTIN# to recover system from a fault condition. > > > > Currently, only single-edge mode conversion and threshold events support. > > > > Signed-off-by: Eason Yang <j2anfernee@gmail.com> > Hi Eason, > > Various minor comments in addition to what Andy has > posted already. > > Jonathan > > > diff --git a/drivers/iio/adc/nct7201.c b/drivers/iio/adc/nct7201.c > > new file mode 100644 > > index 000000000000..9ad4d2919461 > > --- /dev/null > > +++ b/drivers/iio/adc/nct7201.c > > @@ -0,0 +1,449 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Driver for Nuvoton nct7201 and nct7202 power monitor chips. > > + * > > + * Copyright (c) 2024 Nuvoton Inc. > > + */ > > + > > +#include <linux/array_size.h> > > +#include <linux/bits.h> > > +#include <linux/cleanup.h> > > +#include <linux/delay.h> > > +#include <linux/device.h> > > +#include <linux/err.h> > > +#include <linux/i2c.h> > > +#include <linux/init.h> > > +#include <linux/mod_devicetable.h> > > +#include <linux/module.h> > > +#include <linux/mutex.h> > > +#include <linux/regmap.h> > > +#include <linux/types.h> > > +#include <linux/unaligned.h> > > + > > +#include <linux/iio/events.h> > > +#include <linux/iio/iio.h> > > + > > +#define NCT7201_VIN_MAX 12 /* Counted from 1 */ > > +#define NCT7201_IN_SCALING 4995 > > +#define NCT7201_IN_SCALING_FACTOR 10000 > > + > > +#define NCT7201_REG_INTERRUPT_STATUS_1 0x0C > > +#define NCT7201_REG_INTERRUPT_STATUS_2 0x0D > > +#define NCT7201_REG_VOLT_LOW_BYTE 0x0F > > +#define NCT7201_REG_CONFIGURATION 0x10 > > +#define NCT7201_BIT_CONFIGURATION_START BIT(0) > > +#define NCT7201_BIT_CONFIGURATION_ALERT_MSK BIT(1) > > +#define NCT7201_BIT_CONFIGURATION_CONV_RATE BIT(2) > > +#define NCT7201_BIT_CONFIGURATION_RESET BIT(7) > > + > > +#define NCT7201_REG_ADVANCED_CONFIGURATION 0x11 > > +#define NCT7201_BIT_ADVANCED_CONF_MOD_ALERT BIT(0) > > +#define NCT7201_BIT_ADVANCED_CONF_MOD_STS BIT(1) > > +#define NCT7201_BIT_ADVANCED_CONF_FAULT_QUEUE BIT(2) > > +#define NCT7201_BIT_ADVANCED_CONF_EN_DEEP_SHUTDOWN BIT(4) > > +#define NCT7201_BIT_ADVANCED_CONF_EN_SMB_TIMEOUT BIT(5) > > +#define NCT7201_BIT_ADVANCED_CONF_MOD_RSTIN BIT(7) > > + > > +#define NCT7201_REG_CHANNEL_INPUT_MODE 0x12 > > +#define NCT7201_REG_CHANNEL_ENABLE_1 0x13 > > +#define NCT7201_REG_CHANNEL_ENABLE_1_MASK GENMASK(7, 0) > > +#define NCT7201_REG_CHANNEL_ENABLE_2 0x14 > > +#define NCT7201_REG_CHANNEL_ENABLE_2_MASK GENMASK(3, 0) > > As below. I'd treat these two registers as one larger register. > > > +static int nct7201_read_raw(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, > > + int *val, int *val2, long mask) > > +{ > > + u16 volt; > > + unsigned int value; > > + int err; > > + struct nct7201_chip_info *chip = iio_priv(indio_dev); > > + > > + if (chan->type != IIO_VOLTAGE) > > + return -EOPNOTSUPP; > > + > > + guard(mutex)(&chip->access_lock); > > + switch (mask) { > > + case IIO_CHAN_INFO_RAW: > > + err = regmap_read(chip->regmap16, NCT7201_REG_VIN(chan->address), &value); > > + if (err < 0) > > + return err; > > + volt = value; > > + *val = volt >> 3; > > As below, likely a FIELD_GET() is appropriate here. > > > + return IIO_VAL_INT; > > + case IIO_CHAN_INFO_SCALE: > > + /* From the datasheet, we have to multiply by 0.0004995 */ > > + *val = 0; > > + *val2 = 499500; > > + return IIO_VAL_INT_PLUS_NANO; > > + default: > > + return -EINVAL; > > + } > > +} > > + > > +static int nct7201_read_event_value(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + enum iio_event_type type, > > + enum iio_event_direction dir, > > + enum iio_event_info info, > > + int *val, int *val2) > > +{ > > + struct nct7201_chip_info *chip = iio_priv(indio_dev); > > + u16 volt; > > + unsigned int value; > > + int err; > > + > > + if (chan->type != IIO_VOLTAGE) > > + return -EOPNOTSUPP; > > + > > + if (info != IIO_EV_INFO_VALUE) > > + return -EINVAL; > > + > > + if (dir == IIO_EV_DIR_FALLING) { > > + err = regmap_read(chip->regmap16, NCT7201_REG_VIN_LOW_LIMIT(chan->address), &value); > > + if (err < 0) > > + return err; > > + volt = value; > > + } else { > > + err = regmap_read(chip->regmap16, NCT7201_REG_VIN_HIGH_LIMIT(chan->address), &value); > > + if (err < 0) > > + return err; > > + volt = value; > > + } > > + > > + *val = volt >> 3; > As Andy pointed out, likely a FIELD_GET() makes sense here. > > > + > > + return IIO_VAL_INT; > > +} > > + > > +static int nct7201_write_event_value(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + enum iio_event_type type, > > + enum iio_event_direction dir, > > + enum iio_event_info info, > > + int val, int val2) > > +{ > > + struct nct7201_chip_info *chip = iio_priv(indio_dev); > > + long v1, v2; > > + > > + v1 = val >> 5; > > + v2 = FIELD_PREP(NCT7201_REG_VIN_LIMIT_LSB_MASK, val) << 3; > > + > > + if (chan->type != IIO_VOLTAGE) > > + return -EOPNOTSUPP; > > + > > + if (info == IIO_EV_INFO_VALUE) { > I'd flip this to > if (info != IIO_EV_INFO_VALUE) > return -EOPNOTSUPP; > > guard(). > > > + guard(mutex)(&chip->access_lock); > > + if (dir == IIO_EV_DIR_FALLING) { > > + regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT(chan->address), v1); > > + regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT_LSB(chan->address), v2); > > Check for errors. > > > + } else { > > + regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT(chan->address), v1); > > + regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT_LSB(chan->address), v2); > > + } > > + } > > + > > + return 0; > > +} > > > + > > +static int nct7201_write_event_config(struct iio_dev *indio_dev, > > + const struct iio_chan_spec *chan, > > + enum iio_event_type type, > > + enum iio_event_direction dir, > > + bool state) > > +{ > > + struct nct7201_chip_info *chip = iio_priv(indio_dev); > > + unsigned int mask; > > + > > + if (chan->type != IIO_VOLTAGE) > > + return -EOPNOTSUPP; > > + > > + mask = BIT(chan->address); > > + > > + if (!state && (chip->vin_mask & mask)) > > + chip->vin_mask &= ~mask; > > + else if (state && !(chip->vin_mask & mask)) > > + chip->vin_mask |= mask; > > + > > + guard(mutex)(&chip->access_lock); > > + regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1, > > + chip->vin_mask & NCT7201_REG_CHANNEL_ENABLE_1_MASK); > > + if (chip->num_vin_channels == 12) > > + regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2, > > + chip->vin_mask & NCT7201_REG_CHANNEL_ENABLE_2_MASK); > > This feels odd. Don't you need to shift that vin_mask to get rid of channels > that are enabled via ENABLE_1? > We need to right shift 8 bit chip->vin_mask and write into ENABLE_2. > > + if (chip->num_vin_channels == 12) > > + regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2, - chip->vin_mask & NCT7201_REG_CHANNEL_ENABLE_2_MASK); + FIELD_GET(GENMASK(15, 8), chip->vin_mask) & NCT7201_REG_CHANNEL_ENABLE_2_MASK); > > + > > + return 0; > > +} > > > +static int nct7201_init_chip(struct nct7201_chip_info *chip) > > +{ > > + u8 data[2]; > > + unsigned int value; > > + int err; > > + > > + regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION, > > + NCT7201_BIT_CONFIGURATION_RESET); > > Add a comment on why you don't check return value (or do check it if appropriate). > > > + > > + /* > > + * After about 25 msecs, the device should be ready and then > > + * the Power Up bit will be set to 1. If not, wait for it. > > + */ > > + mdelay(25); > > + err = regmap_read(chip->regmap, NCT7201_REG_BUSY_STATUS, &value); > > + if (err < 0) > > + return err; > > + if (!(value & NCT7201_BIT_PWR_UP)) > > + return dev_err_probe(&chip->client->dev, -EIO, "failed to power up after reset\n"); > > + > > + /* Enable Channel */ > > + guard(mutex)(&chip->access_lock); > > + regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1, > > + NCT7201_REG_CHANNEL_ENABLE_1_MASK); > > Check return value. This is over an I2C bus, not the most reliable of > transports! > > Consider doing this differently and using a bulk write for the larger > case. > > if (chip->num_vin_channels <= 8) > ret = regmap_write(); > else > ret = regmap_bulk_write(); > > However as you read ENABLE_2 unconditionally below, can you instead just > always use a bulk write here? > We can't use regmap_bulk_write() due to the chip's limit. regmap_bulk_write(chip->regmap, ..., ..., 2) , the first byte is well written, but the second byte don't changed. > > + if (chip->num_vin_channels == 12) > > + regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2, > > + NCT7201_REG_CHANNEL_ENABLE_2_MASK); > > + > > + err = regmap_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1, &value); > > + if (err < 0) > > + return err; > > + data[0] = value; > > + > > + err = regmap_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2, &value); > > + if (err < 0) > > + return err; > > + data[1] = value; > > Why does reading these back make sense? Is there a reason the write > above might not work if the I2C transfer is Acked? > > If this is part of discovery protocol for how many channels are there, then > add comments. > We remove read back enable registers, just assign data value after regmap_write() successfully. Like, err = regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1, NCT7201_REG_CHANNEL_ENABLE_1_MASK); if (err) { dev_err(&chip->client->dev, "Failed to write REG_CHANNEL_ENABLE_1\n"); return err; } data[0] = NCT7201_REG_CHANNEL_ENABLE_1_MASK; > > + > > + value = get_unaligned_le16(data); > > + chip->vin_mask = value; > > + > > + /* Start monitoring if needed */ > > + err = regmap_read(chip->regmap, NCT7201_REG_CONFIGURATION, &value); > > + if (err < 0) { > > + dev_err_probe(&chip->client->dev, -EIO, "Failed to read REG_CONFIGURATION\n"); > > + return value; > > Why return value if an error occurred? > retuen dev_err_probe(); > > > + } > > + > > + value |= NCT7201_BIT_CONFIGURATION_START; > > + regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION, value); > > regmap_set_bits() > - value |= NCT7201_BIT_CONFIGURATION_START; - regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION, value); + regmap_set_bits(chip->regmap, NCT7201_REG_CONFIGURATION, NCT7201_BIT_CONFIGURATION_START); > > + > > + return 0; > > +} > > + > > +static const struct regmap_config nct7201_regmap8_config = { > > + .name = "vin-data-read-byte", > > + .reg_bits = 8, > > + .val_bits = 8, > > + .max_register = 0xff, > > +}; > > + > > +static const struct regmap_config nct7201_regmap16_config = { > > + .name = "vin-data-read-word", > > + .reg_bits = 8, > > + .val_bits = 16, > > + .max_register = 0xff, > > +}; > > + > > +static int nct7201_probe(struct i2c_client *client) > > +{ > > + const struct nct7201_adc_model_data *model_data; > > + struct nct7201_chip_info *chip; > > + struct iio_dev *indio_dev; > > + int ret; > > + > > + model_data = i2c_get_match_data(client); > > + if (!model_data) > > + return -EINVAL; > > + > > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip)); > > + if (!indio_dev) > > + return -ENOMEM; > > + chip = iio_priv(indio_dev); > > + > > + chip->regmap = devm_regmap_init_i2c(client, &nct7201_regmap8_config); > > + if (IS_ERR(chip->regmap)) > > + return dev_err_probe(&client->dev, PTR_ERR(chip->regmap), > > + "Failed to init regmap\n"); > > + > > + chip->regmap16 = devm_regmap_init_i2c(client, &nct7201_regmap16_config); > > + if (IS_ERR(chip->regmap16)) > > + return dev_err_probe(&client->dev, PTR_ERR(chip->regmap16), > > + "Failed to init regmap16\n"); > > + > > + chip->num_vin_channels = model_data->num_vin_channels; > > + > > + ret = devm_mutex_init(&client->dev, &chip->access_lock); > > + if (ret) > > + return ret; > > + > > + chip->client = client; > > + > > + ret = nct7201_init_chip(chip); > > + if (ret < 0) > > + return ret; > > + > > + indio_dev->name = model_data->model_name; > > + indio_dev->channels = model_data->channels; > > + indio_dev->num_channels = model_data->num_channels; > > + if (client->irq) > > + indio_dev->info = &nct7201_info; > > + else > > + indio_dev->info = &nct7201_info_no_irq; > > + indio_dev->modes = INDIO_DIRECT_MODE; > > + > > + return devm_iio_device_register(&client->dev, indio_dev); > > +}
On 1/6/25 2:33 AM, Yu-Hsian Yang wrote: > Dear Andy, > > Thanks for your comments. > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> 於 2024年12月27日 週五 下午8:14寫道： >> >> On Thu, Dec 26, 2024 at 01:53:13PM +0800, Eason Yang wrote: >>> Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver >>> >>> NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to >>> 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for >>> independent alarm signals, and the all threshold values could be set for >>> system protection without any timing delay. It also supports reset input >>> RSTIN# to recover system from a fault condition. >>> >>> Currently, only single-edge mode conversion and threshold events support. >> ... >> >>> +static const struct regmap_config nct7201_regmap8_config = { >>> + .name = "vin-data-read-byte", >>> + .reg_bits = 8, >>> + .val_bits = 8, >>> + .max_register = 0xff, >>> +}; >>> + >>> +static const struct regmap_config nct7201_regmap16_config = { >>> + .name = "vin-data-read-word", >>> + .reg_bits = 8, >>> + .val_bits = 16, >>> + .max_register = 0xff, >>> +}; >> >> I don't see how these configurations will prevent, e.g., debugfs to access >> 16-bit registers via 8-bit IO and vice versa. >> > > Read VIN info can use word read or byte read, and other registers > should use byte read. > > The design is that VIN info registers are used 16-bit debugfs to access and > other registers are used 8-bit debugfs to access. > > We need to probe 8-bit regmap and 16-bit regmap, > but I have no idea how to prevent 8-bit IO to access 16-bit registers > and vice versa. You can do this with struct regmap_access_table via wr_table and rd_table in the struct regmap_config.
diff --git a/MAINTAINERS b/MAINTAINERS index 9c5328cbef17..592ccca88f04 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2799,6 +2799,7 @@ F: arch/arm/mach-npcm/ F: arch/arm64/boot/dts/nuvoton/ F: drivers/*/*/*npcm* F: drivers/*/*npcm* +F: drivers/iio/adc/nct7201.c F: drivers/rtc/rtc-nct3018y.c F: include/dt-bindings/clock/nuvoton,npcm7xx-clock.h F: include/dt-bindings/clock/nuvoton,npcm845-clk.h diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index 849c90203071..abd8b21fdbac 100644 --- a/drivers/iio/adc/Kconfig +++ b/drivers/iio/adc/Kconfig @@ -1048,6 +1048,16 @@ config NAU7802 To compile this driver as a module, choose M here: the module will be called nau7802. +config NCT7201 + tristate "Nuvoton Instruments NCT7201 and NCT7202 Power Monitor" + depends on I2C + select REGMAP_I2C + help + If you say yes here you get support for the Nuvoton NCT7201 and + NCT7202 Voltage Monitor. + This driver can also be built as a module. If so, the module + will be called nct7201. + config NPCM_ADC tristate "Nuvoton NPCM ADC driver" depends on ARCH_NPCM || COMPILE_TEST diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index ee19afba62b7..0108472e141c 100644 --- a/drivers/iio/adc/Makefile +++ b/drivers/iio/adc/Makefile @@ -94,6 +94,7 @@ obj-$(CONFIG_MESON_SARADC) += meson_saradc.o obj-$(CONFIG_MP2629_ADC) += mp2629_adc.o obj-$(CONFIG_MXS_LRADC_ADC) += mxs-lradc-adc.o obj-$(CONFIG_NAU7802) += nau7802.o +obj-$(CONFIG_NCT7201) += nct7201.o obj-$(CONFIG_NPCM_ADC) += npcm_adc.o obj-$(CONFIG_PAC1921) += pac1921.o obj-$(CONFIG_PAC1934) += pac1934.o diff --git a/drivers/iio/adc/nct7201.c b/drivers/iio/adc/nct7201.c new file mode 100644 index 000000000000..9ad4d2919461 --- /dev/null +++ b/drivers/iio/adc/nct7201.c @@ -0,0 +1,449 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Driver for Nuvoton nct7201 and nct7202 power monitor chips. + * + * Copyright (c) 2024 Nuvoton Inc. + */ + +#include <linux/array_size.h> +#include <linux/bits.h> +#include <linux/cleanup.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/init.h> +#include <linux/mod_devicetable.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/regmap.h> +#include <linux/types.h> +#include <linux/unaligned.h> + +#include <linux/iio/events.h> +#include <linux/iio/iio.h> + +#define NCT7201_VIN_MAX 12 /* Counted from 1 */ +#define NCT7201_IN_SCALING 4995 +#define NCT7201_IN_SCALING_FACTOR 10000 + +#define NCT7201_REG_INTERRUPT_STATUS_1 0x0C +#define NCT7201_REG_INTERRUPT_STATUS_2 0x0D +#define NCT7201_REG_VOLT_LOW_BYTE 0x0F +#define NCT7201_REG_CONFIGURATION 0x10 +#define NCT7201_BIT_CONFIGURATION_START BIT(0) +#define NCT7201_BIT_CONFIGURATION_ALERT_MSK BIT(1) +#define NCT7201_BIT_CONFIGURATION_CONV_RATE BIT(2) +#define NCT7201_BIT_CONFIGURATION_RESET BIT(7) + +#define NCT7201_REG_ADVANCED_CONFIGURATION 0x11 +#define NCT7201_BIT_ADVANCED_CONF_MOD_ALERT BIT(0) +#define NCT7201_BIT_ADVANCED_CONF_MOD_STS BIT(1) +#define NCT7201_BIT_ADVANCED_CONF_FAULT_QUEUE BIT(2) +#define NCT7201_BIT_ADVANCED_CONF_EN_DEEP_SHUTDOWN BIT(4) +#define NCT7201_BIT_ADVANCED_CONF_EN_SMB_TIMEOUT BIT(5) +#define NCT7201_BIT_ADVANCED_CONF_MOD_RSTIN BIT(7) + +#define NCT7201_REG_CHANNEL_INPUT_MODE 0x12 +#define NCT7201_REG_CHANNEL_ENABLE_1 0x13 +#define NCT7201_REG_CHANNEL_ENABLE_1_MASK GENMASK(7, 0) +#define NCT7201_REG_CHANNEL_ENABLE_2 0x14 +#define NCT7201_REG_CHANNEL_ENABLE_2_MASK GENMASK(3, 0) +#define NCT7201_REG_INTERRUPT_MASK_1 0x15 +#define NCT7201_REG_INTERRUPT_MASK_2 0x16 +#define NCT7201_REG_BUSY_STATUS 0x1E +#define NCT7201_BIT_BUSY BIT(0) +#define NCT7201_BIT_PWR_UP BIT(1) +#define NCT7201_REG_ONE_SHOT 0x1F +#define NCT7201_REG_SMUS_ADDRESS 0xFC +#define NCT7201_REG_VIN_LIMIT_LSB_MASK GENMASK(4, 0) + +#define NCT7201_REG_VIN(i) (i) +#define NCT7201_REG_VIN_HIGH_LIMIT(i) (0x20 + (i) * 2) +#define NCT7201_REG_VIN_LOW_LIMIT(i) (0x21 + (i) * 2) +#define NCT7201_REG_VIN_HIGH_LIMIT_LSB(i) (0x40 + (i) * 2) +#define NCT7201_REG_VIN_LOW_LIMIT_LSB(i) (0x41 + (i) * 2) + +struct nct7201_chip_info { + struct i2c_client *client; + struct mutex access_lock; /* for multi-byte read and write operations */ + struct regmap *regmap; + struct regmap *regmap16; + int num_vin_channels; + u32 vin_mask; +}; + +struct nct7201_adc_model_data { + const char *model_name; + const struct iio_chan_spec *channels; + const int num_channels; + int num_vin_channels; +}; + +static const struct iio_event_spec nct7201_events[] = { + { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_RISING, + .mask_separate = BIT(IIO_EV_INFO_VALUE) | + BIT(IIO_EV_INFO_ENABLE), + }, { + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_FALLING, + .mask_separate = BIT(IIO_EV_INFO_VALUE) | + BIT(IIO_EV_INFO_ENABLE), + }, +}; + +#define NCT7201_VOLTAGE_CHANNEL(chan, addr) \ + { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .channel = chan, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + .address = addr, \ + .event_spec = nct7201_events, \ + .num_event_specs = ARRAY_SIZE(nct7201_events), \ + } + +static const struct iio_chan_spec nct7201_channels[] = { + NCT7201_VOLTAGE_CHANNEL(1, 0), + NCT7201_VOLTAGE_CHANNEL(2, 1), + NCT7201_VOLTAGE_CHANNEL(3, 2), + NCT7201_VOLTAGE_CHANNEL(4, 3), + NCT7201_VOLTAGE_CHANNEL(5, 4), + NCT7201_VOLTAGE_CHANNEL(6, 5), + NCT7201_VOLTAGE_CHANNEL(7, 6), + NCT7201_VOLTAGE_CHANNEL(8, 7), +}; + +static const struct iio_chan_spec nct7202_channels[] = { + NCT7201_VOLTAGE_CHANNEL(1, 0), + NCT7201_VOLTAGE_CHANNEL(2, 1), + NCT7201_VOLTAGE_CHANNEL(3, 2), + NCT7201_VOLTAGE_CHANNEL(4, 3), + NCT7201_VOLTAGE_CHANNEL(5, 4), + NCT7201_VOLTAGE_CHANNEL(6, 5), + NCT7201_VOLTAGE_CHANNEL(7, 6), + NCT7201_VOLTAGE_CHANNEL(8, 7), + NCT7201_VOLTAGE_CHANNEL(9, 8), + NCT7201_VOLTAGE_CHANNEL(10, 9), + NCT7201_VOLTAGE_CHANNEL(11, 10), + NCT7201_VOLTAGE_CHANNEL(12, 11), +}; + +static int nct7201_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + int *val, int *val2, long mask) +{ + u16 volt; + unsigned int value; + int err; + struct nct7201_chip_info *chip = iio_priv(indio_dev); + + if (chan->type != IIO_VOLTAGE) + return -EOPNOTSUPP; + + guard(mutex)(&chip->access_lock); + switch (mask) { + case IIO_CHAN_INFO_RAW: + err = regmap_read(chip->regmap16, NCT7201_REG_VIN(chan->address), &value); + if (err < 0) + return err; + volt = value; + *val = volt >> 3; + return IIO_VAL_INT; + case IIO_CHAN_INFO_SCALE: + /* From the datasheet, we have to multiply by 0.0004995 */ + *val = 0; + *val2 = 499500; + return IIO_VAL_INT_PLUS_NANO; + default: + return -EINVAL; + } +} + +static int nct7201_read_event_value(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + enum iio_event_info info, + int *val, int *val2) +{ + struct nct7201_chip_info *chip = iio_priv(indio_dev); + u16 volt; + unsigned int value; + int err; + + if (chan->type != IIO_VOLTAGE) + return -EOPNOTSUPP; + + if (info != IIO_EV_INFO_VALUE) + return -EINVAL; + + if (dir == IIO_EV_DIR_FALLING) { + err = regmap_read(chip->regmap16, NCT7201_REG_VIN_LOW_LIMIT(chan->address), &value); + if (err < 0) + return err; + volt = value; + } else { + err = regmap_read(chip->regmap16, NCT7201_REG_VIN_HIGH_LIMIT(chan->address), &value); + if (err < 0) + return err; + volt = value; + } + + *val = volt >> 3; + + return IIO_VAL_INT; +} + +static int nct7201_write_event_value(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + enum iio_event_info info, + int val, int val2) +{ + struct nct7201_chip_info *chip = iio_priv(indio_dev); + long v1, v2; + + v1 = val >> 5; + v2 = FIELD_PREP(NCT7201_REG_VIN_LIMIT_LSB_MASK, val) << 3; + + if (chan->type != IIO_VOLTAGE) + return -EOPNOTSUPP; + + if (info == IIO_EV_INFO_VALUE) { + guard(mutex)(&chip->access_lock); + if (dir == IIO_EV_DIR_FALLING) { + regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT(chan->address), v1); + regmap_write(chip->regmap, NCT7201_REG_VIN_LOW_LIMIT_LSB(chan->address), v2); + } else { + regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT(chan->address), v1); + regmap_write(chip->regmap, NCT7201_REG_VIN_HIGH_LIMIT_LSB(chan->address), v2); + } + } + + return 0; +} + +static int nct7201_read_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir) +{ + struct nct7201_chip_info *chip = iio_priv(indio_dev); + + if (chan->type != IIO_VOLTAGE) + return -EOPNOTSUPP; + + return !!(chip->vin_mask & BIT(chan->address)); +} + +static int nct7201_write_event_config(struct iio_dev *indio_dev, + const struct iio_chan_spec *chan, + enum iio_event_type type, + enum iio_event_direction dir, + bool state) +{ + struct nct7201_chip_info *chip = iio_priv(indio_dev); + unsigned int mask; + + if (chan->type != IIO_VOLTAGE) + return -EOPNOTSUPP; + + mask = BIT(chan->address); + + if (!state && (chip->vin_mask & mask)) + chip->vin_mask &= ~mask; + else if (state && !(chip->vin_mask & mask)) + chip->vin_mask |= mask; + + guard(mutex)(&chip->access_lock); + regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1, + chip->vin_mask & NCT7201_REG_CHANNEL_ENABLE_1_MASK); + if (chip->num_vin_channels == 12) + regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2, + chip->vin_mask & NCT7201_REG_CHANNEL_ENABLE_2_MASK); + + return 0; +} + +static const struct iio_info nct7201_info = { + .read_raw = nct7201_read_raw, + .read_event_config = nct7201_read_event_config, + .write_event_config = nct7201_write_event_config, + .read_event_value = nct7201_read_event_value, + .write_event_value = nct7201_write_event_value, +}; + +static const struct iio_info nct7201_info_no_irq = { + .read_raw = nct7201_read_raw, +}; + +static const struct nct7201_adc_model_data nct7201_model_data = { + .model_name = "nct7201", + .channels = nct7201_channels, + .num_channels = ARRAY_SIZE(nct7201_channels), + .num_vin_channels = 8, +}; + +static const struct nct7201_adc_model_data nct7202_model_data = { + .model_name = "nct7202", + .channels = nct7202_channels, + .num_channels = ARRAY_SIZE(nct7202_channels), + .num_vin_channels = 12, +}; + +static int nct7201_init_chip(struct nct7201_chip_info *chip) +{ + u8 data[2]; + unsigned int value; + int err; + + regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION, + NCT7201_BIT_CONFIGURATION_RESET); + + /* + * After about 25 msecs, the device should be ready and then + * the Power Up bit will be set to 1. If not, wait for it. + */ + mdelay(25); + err = regmap_read(chip->regmap, NCT7201_REG_BUSY_STATUS, &value); + if (err < 0) + return err; + if (!(value & NCT7201_BIT_PWR_UP)) + return dev_err_probe(&chip->client->dev, -EIO, "failed to power up after reset\n"); + + /* Enable Channel */ + guard(mutex)(&chip->access_lock); + regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1, + NCT7201_REG_CHANNEL_ENABLE_1_MASK); + if (chip->num_vin_channels == 12) + regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2, + NCT7201_REG_CHANNEL_ENABLE_2_MASK); + + err = regmap_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1, &value); + if (err < 0) + return err; + data[0] = value; + + err = regmap_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_2, &value); + if (err < 0) + return err; + data[1] = value; + + value = get_unaligned_le16(data); + chip->vin_mask = value; + + /* Start monitoring if needed */ + err = regmap_read(chip->regmap, NCT7201_REG_CONFIGURATION, &value); + if (err < 0) { + dev_err_probe(&chip->client->dev, -EIO, "Failed to read REG_CONFIGURATION\n"); + return value; + } + + value |= NCT7201_BIT_CONFIGURATION_START; + regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION, value); + + return 0; +} + +static const struct regmap_config nct7201_regmap8_config = { + .name = "vin-data-read-byte", + .reg_bits = 8, + .val_bits = 8, + .max_register = 0xff, +}; + +static const struct regmap_config nct7201_regmap16_config = { + .name = "vin-data-read-word", + .reg_bits = 8, + .val_bits = 16, + .max_register = 0xff, +}; + +static int nct7201_probe(struct i2c_client *client) +{ + const struct nct7201_adc_model_data *model_data; + struct nct7201_chip_info *chip; + struct iio_dev *indio_dev; + int ret; + + model_data = i2c_get_match_data(client); + if (!model_data) + return -EINVAL; + + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip)); + if (!indio_dev) + return -ENOMEM; + chip = iio_priv(indio_dev); + + chip->regmap = devm_regmap_init_i2c(client, &nct7201_regmap8_config); + if (IS_ERR(chip->regmap)) + return dev_err_probe(&client->dev, PTR_ERR(chip->regmap), + "Failed to init regmap\n"); + + chip->regmap16 = devm_regmap_init_i2c(client, &nct7201_regmap16_config); + if (IS_ERR(chip->regmap16)) + return dev_err_probe(&client->dev, PTR_ERR(chip->regmap16), + "Failed to init regmap16\n"); + + chip->num_vin_channels = model_data->num_vin_channels; + + ret = devm_mutex_init(&client->dev, &chip->access_lock); + if (ret) + return ret; + + chip->client = client; + + ret = nct7201_init_chip(chip); + if (ret < 0) + return ret; + + indio_dev->name = model_data->model_name; + indio_dev->channels = model_data->channels; + indio_dev->num_channels = model_data->num_channels; + if (client->irq) + indio_dev->info = &nct7201_info; + else + indio_dev->info = &nct7201_info_no_irq; + indio_dev->modes = INDIO_DIRECT_MODE; + + return devm_iio_device_register(&client->dev, indio_dev); +} + +static const struct i2c_device_id nct7201_id[] = { + { .name = "nct7201", .driver_data = (kernel_ulong_t)&nct7201_model_data }, + { .name = "nct7202", .driver_data = (kernel_ulong_t)&nct7202_model_data }, + { } +}; +MODULE_DEVICE_TABLE(i2c, nct7201_id); + +static const struct of_device_id nct7201_of_match[] = { + { + .compatible = "nuvoton,nct7201", + .data = &nct7201_model_data, + }, + { + .compatible = "nuvoton,nct7202", + .data = &nct7202_model_data, + }, + { } +}; +MODULE_DEVICE_TABLE(of, nct7201_of_match); + +static struct i2c_driver nct7201_driver = { + .driver = { + .name = "nct7201", + .of_match_table = nct7201_of_match, + }, + .probe = nct7201_probe, + .id_table = nct7201_id, +}; +module_i2c_driver(nct7201_driver); + +MODULE_AUTHOR("Eason Yang <j2anfernee@gmail.com>"); +MODULE_DESCRIPTION("Nuvoton NCT7201 voltage monitor driver"); +MODULE_LICENSE("GPL");
Add Nuvoton NCT7201/NCT7202 system voltage monitor 12-bit ADC driver NCT7201/NCT7202 supports up to 12 analog voltage monitor inputs and up to 4 SMBus addresses by ADDR pin. Meanwhile, ALERT# hardware event pins for independent alarm signals, and the all threshold values could be set for system protection without any timing delay. It also supports reset input RSTIN# to recover system from a fault condition. Currently, only single-edge mode conversion and threshold events support. Signed-off-by: Eason Yang <j2anfernee@gmail.com> --- MAINTAINERS | 1 + drivers/iio/adc/Kconfig | 10 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/nct7201.c | 449 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 461 insertions(+) create mode 100644 drivers/iio/adc/nct7201.c