Dear Jonathan Cameron,
Thank you for your comment.
Jonathan Cameron <jic23@xxxxxxxxxx> 於 2024年11月9日 週六 下午10:37寫道：
>
> On Wed, 6 Nov 2024 10:39:16 +0800
> Eason Yang <j2anfernee@xxxxxxxxx> 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@xxxxxxxxx>
> You've gotten some good review already so I may well repeat stuff plus
> will only take a fairly superficial look at this version.
>
>
> > diff --git a/drivers/iio/adc/nct720x.c b/drivers/iio/adc/nct720x.c
> > new file mode 100644
> > index 000000000000..e589479fd06e
> > --- /dev/null
> > +++ b/drivers/iio/adc/nct720x.c
>
> > +
> > +/* List of supported devices */
> > +enum nct720x_chips {
> > + nct7201, nct7202
> This should be replaced by chip specific information structures.
> That ends up a lot more extensible than using an enum and fixing up
> what happens in all code paths on a per enum value basis.
>
We would pass a pointer to the data structure not id to describe the
particular variant.
> > +};
> > +
> > +struct nct720x_chip_info {
> > + struct i2c_client *client;
> > + enum nct720x_chips type;
> > + struct mutex access_lock; /* for multi-byte read and write operations */
> > + int vin_max; /* number of VIN channels */
> > + u32 vin_mask;
> > + bool use_read_byte_vin;
> > +};
>
> > +#define NCT720X_VOLTAGE_CHANNEL(chan, addr) \
> > + { \
> > + .type = IIO_VOLTAGE, \
> > + .indexed = 1, \
> > + .channel = chan, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> > + .address = addr, \
> > + .event_spec = nct720x_events, \
> > + .num_event_specs = ARRAY_SIZE(nct720x_events), \
> > + }
> > +
> > +#define NCT720X_VOLTAGE_CHANNEL_DIFF(chan1, chan2, addr) \
> > + { \
> > + .type = IIO_VOLTAGE, \
> > + .indexed = 1, \
> > + .channel = (chan1), \
> > + .channel2 = (chan2), \
> > + .differential = 1, \
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED), \
> See below - should be _RAW and _SCALE not _PROCESSED.
We would use _RAW and _SCALE to instead of _PROCESSED.
> > + .address = addr, \
> > + .event_spec = nct720x_events, \
> > + .num_event_specs = ARRAY_SIZE(nct720x_events), \
> > + }
> > +
> > +static const struct iio_chan_spec nct720x_channels[] = {
> > + NCT720X_VOLTAGE_CHANNEL(1, 1),
> > + NCT720X_VOLTAGE_CHANNEL(2, 2),
> > + NCT720X_VOLTAGE_CHANNEL(3, 3),
> > + NCT720X_VOLTAGE_CHANNEL(4, 4),
> > + NCT720X_VOLTAGE_CHANNEL(5, 5),
> > + NCT720X_VOLTAGE_CHANNEL(6, 6),
> > + NCT720X_VOLTAGE_CHANNEL(7, 7),
> > + NCT720X_VOLTAGE_CHANNEL(8, 8),
> > + NCT720X_VOLTAGE_CHANNEL(9, 9),
> > + NCT720X_VOLTAGE_CHANNEL(10, 10),
> > + NCT720X_VOLTAGE_CHANNEL(11, 11),
> > + NCT720X_VOLTAGE_CHANNEL(12, 12),
> > + NCT720X_VOLTAGE_CHANNEL_DIFF(1, 2, 1),
> > + NCT720X_VOLTAGE_CHANNEL_DIFF(3, 4, 3),
> > + NCT720X_VOLTAGE_CHANNEL_DIFF(5, 6, 5),
> > + NCT720X_VOLTAGE_CHANNEL_DIFF(7, 8, 7),
> > + NCT720X_VOLTAGE_CHANNEL_DIFF(9, 10, 9),
> > + NCT720X_VOLTAGE_CHANNEL_DIFF(11, 12, 11),
> > +};
> > +
> > +/* Read 1-byte register. Returns unsigned byte data or -ERRNO on error. */
> > +static int nct720x_read_reg(struct nct720x_chip_info *chip, u8 reg)
> > +{
> > + struct i2c_client *client = chip->client;
> > +
> > + return i2c_smbus_read_byte_data(client, reg);
> > +}
> > +
> > +/* Read 1-byte register. Returns unsigned word data or -ERRNO on error. */
> > +static int nct720x_read_word_swapped_reg(struct nct720x_chip_info *chip, u8 reg)
> > +{
> > + struct i2c_client *client = chip->client;
> > +
> > + return i2c_smbus_read_word_swapped(client, reg);
>
> Don't provide these wrappers as they don't add anything useful.
> Make the calls directly inline.
>
We would remove this function and use regmap to instead of i2c API.
> > +}
> > +
> > +/*
> > + * Read 2-byte register. Returns register in big-endian format or
> > + * -ERRNO on error.
> > + */
> > +static int nct720x_read_reg16(struct nct720x_chip_info *chip, u8 reg)
> > +{
> > + struct i2c_client *client = chip->client;
> > + int ret, low;
> > +
> > + mutex_lock(&chip->access_lock);
> guard()
>
Got it.
> > + ret = i2c_smbus_read_byte_data(client, reg);
> > + if (ret >= 0) {
> > + low = ret;
> > + ret = i2c_smbus_read_byte_data(client, reg + 1);
> > + if (ret >= 0)
> > + ret = low | (ret << 8);
> if (ret < 0)
> return ret;
>
> reg = get_unaligned_le16()
> on an appropriate u8 data[2]; ideally filled by a bulk regmap read.
>
We would use regmap read.
>
> > + }
> > +
> > + mutex_unlock(&chip->access_lock);
> > + return ret;
> > +}
> > +
> > +/* Write 1-byte register. Returns 0 or -ERRNO on error. */
> > +static int nct720x_write_reg(struct nct720x_chip_info *chip, u8 reg, u8 val)
> > +{
> > + struct i2c_client *client = chip->client;
> > + int err;
> > +
> > + err = i2c_smbus_write_byte_data(client, reg, val);
> > + /* wait for write command to be finished */
> If this is needed, provide a datasheet reference. It is very ususual to
> see a significant delay needed.
Chip RD do the simulation, we need delay 25 msec in reset register
for registers ready to access.
And I2c write is not needed to delay.
These information would add into datasheet afterwards.
> > + mdelay(10);
> > +
> > + return err;
> > +}
> > +
> > +static int nct720x_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan,
> > + int *val, int *val2, long mask)
> > +{
> > + int index = nct720x_chan_to_index[chan->address];
> > + int v1, v2, volt, err;
> > + struct nct720x_chip_info *chip = iio_priv(indio_dev);
> > +
> > + if (chan->type != IIO_VOLTAGE)
> > + return -EOPNOTSUPP;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_PROCESSED:
> > + mutex_lock(&chip->access_lock);
> > + if (chip->use_read_byte_vin) {
>
> Ah. So resolution doesn't change, you are controlling the i2c acceses?
>
> That should come from the i2c controller capabilities, not DT for this
> device.
>
>
>
The use_read_byte_vin was discussed in dt-binding commit,
We would use read word first and remove read-vin-data-size property.
> > + /*
> > + * MNTVIN Low Byte together with MNTVIN High Byte
> > + * forms the 13-bit count value. If MNTVIN High
> > + * Byte readout is read successively, the
> > + * NCT7201/NCT7202 will latch the MNTVIN Low Byte
> > + * for next read.
> > + */
> > + v1 = nct720x_read_reg(chip, REG_VIN[index]);
> > + if (v1 < 0) {
> > + err = v1;
> > + goto abort;
> > + }
> > +
> > + v2 = nct720x_read_reg(chip, REG_VOLT_LOW_BYTE);
> > + if (v2 < 0) {
> > + err = v2;
> > + goto abort;
> > + }
> > + volt = (v1 << 8) | v2; /* Convert back to 16-bit value */
> > + } else {
> > + /* NCT7201/NCT7202 also supports read word-size data */
> > + volt = nct720x_read_word_swapped_reg(chip, REG_VIN[index]);
> > + }
> > +
> > + /* Voltage(V) = 13bitCountValue * 0.0004995 */
> Present this as _RAW and provide _SCALE to userspace to be able to do this
> maths. Very unusual for an ADC driver to provided processed channels. Normally
> only occurs it there is something non linear going on.
Yes, we would use _RAW and _SCALE to instead of _PROCESSED.
>
>
> > + volt = (volt >> 3) * NCT720X_IN_SCALING;
> > + *val = volt / 10000;
> > + mutex_unlock(&chip->access_lock);
> > + return IIO_VAL_INT;
> > + default:
> > + return -EINVAL;
> > + }
> > +abort:
> > + mutex_unlock(&chip->access_lock);
> > + return err;
> > +}
> > +
> > +static int nct720x_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 nct720x_chip_info *chip = iio_priv(indio_dev);
> > + int v1, v2, err;
> > + int volt = 0;
> > + int index = nct720x_chan_to_index[chan->address];
> > +
> > + if (chan->type != IIO_VOLTAGE)
> > + return -EOPNOTSUPP;
> > +
> > + if (info == IIO_EV_INFO_VALUE) {
> As below - flip logic.
>
> > + if (dir == IIO_EV_DIR_FALLING) {
> > + if (chip->use_read_byte_vin) {
> > + /*
> > + * Low limit VIN Low Byte together with Low limit VIN High Byte
> > + forms the 13-bit count value
> > + */
> > + mutex_lock(&chip->access_lock);
> > + v1 = nct720x_read_reg(chip, REG_VIN_LOW_LIMIT[index]);
> > + if (v1 < 0) {
> > + err = v1;
> > + goto abort;
> > + }
> > +
> > + v2 = nct720x_read_reg(chip, REG_VIN_LOW_LIMIT_LSB[index]);
> > + if (v2 < 0) {
> > + err = v2;
> > + goto abort;
> > + }
> > + mutex_unlock(&chip->access_lock);
> > + volt = (v1 << 8) | v2; /* Convert back to 16-bit value */
>
> rather see this as a get_unaligned_le16 on an array of u8.
> In some cases that ends up quite a bit cheaper and it also documents what is going on.
>
We would use get_unaligned_le16 to convert the array of u8.
> > + } else {
> > + /* NCT7201/NCT7202 also supports read word-size data */
> > + volt = nct720x_read_word_swapped_reg(chip,
> > + REG_VIN_LOW_LIMIT[index]);
> > + }
> > + } else {
> > + if (chip->use_read_byte_vin) {
> > + /*
> > + * High limit VIN Low Byte together with high limit VIN High Byte
> > + * forms the 13-bit count value
> > + */
> > + mutex_lock(&chip->access_lock);
> > + v1 = nct720x_read_reg(chip, REG_VIN_HIGH_LIMIT[index]);
> > + if (v1 < 0) {
> > + err = v1;
> > + goto abort;
> > + }
> > +
> > + v2 = nct720x_read_reg(chip, REG_VIN_HIGH_LIMIT_LSB[index]);
> > + if (v2 < 0) {
> > + err = v2;
> > + goto abort;
> > + }
> > + mutex_unlock(&chip->access_lock);
> > + volt = (v1 << 8) | v2; /* Convert back to 16-bit value */
> > + } else {
> > + /* NCT7201/NCT7202 also supports read word-size data */
> > + volt = nct720x_read_word_swapped_reg(chip,
> > + REG_VIN_HIGH_LIMIT[index]);
> > + }
> > + }
> > + }
> > + /* Voltage(V) = 13bitCountValue * 0.0004995 */
> > + volt = (volt >> 3) * NCT720X_IN_SCALING;
> > + *val = volt / 10000;
> > +
> > + return IIO_VAL_INT;
> > +abort:
> > + mutex_unlock(&chip->access_lock);
> guard() in appropriate places.
> Again, the lock and unlock should ideally be in same scope.
>
We would put guard(mutex) in appropriate places.
> > + return err;
> > +}
> > +
> > +static int nct720x_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 nct720x_chip_info *chip = iio_priv(indio_dev);
> > + int index, err = 0;
> > + long v1, v2, volt;
> > +
> > + index = nct720x_chan_to_index[chan->address];
> > + volt = (val * 10000) / NCT720X_IN_SCALING;
> > + v1 = volt >> 5;
> > + v2 = (volt & 0x1f) << 3;
> Some explanatory comments for this would be good.
>
/* Voltage(V) = 13bitCountValue * 0.0004995 */
volt = (val * NCT720X_IN_SCALING_FACTOR) / NCT720X_IN_SCALING;
v1 = volt >> 5;
v2 = (volt & REG_VIN_LIMIT_LSB_MASK) << 3;
> > +
> > + if (chan->type != IIO_VOLTAGE)
> > + return -EOPNOTSUPP;
> > +
> > + if (info == IIO_EV_INFO_VALUE) {
> Flip logic.
> if (info != IIO_EV_INFO_VALUE)
> return -EINVAL;
>
> if (dir == IIO_EVE_DIR_FALLING) etc
>
> > + if (dir == IIO_EV_DIR_FALLING) {
> > + mutex_lock(&chip->access_lock);
> This is badly nested. Mutex lock and unlock should be in same scope.
> So I'd pull mutex_lock() out of this if stack and use guard(mutex) instead.
> That way you can just return on error.
>
We would put guard(mutex) in appropriate places.
> > + err = nct720x_write_reg(chip, REG_VIN_LOW_LIMIT[index], v1);
> > + if (err < 0) {
> > + pr_err("Failed to write REG_VIN%d_LOW_LIMIT\n", index + 1);
> > + goto abort;
> > + }
> > +
> > + err = nct720x_write_reg(chip, REG_VIN_LOW_LIMIT_LSB[index], v2);
> > + if (err < 0) {
> > + pr_err("Failed to write REG_VIN%d_LOW_LIMIT_LSB\n", index + 1);
> > + goto abort;
> > + }
> > + } else {
> > + mutex_lock(&chip->access_lock);
> > + err = nct720x_write_reg(chip, REG_VIN_HIGH_LIMIT[index], v1);
> > + if (err < 0) {
> > + pr_err("Failed to write REG_VIN%d_HIGH_LIMIT\n", index + 1);
> > + goto abort;
> > + }
> > +
> > + err = nct720x_write_reg(chip, REG_VIN_HIGH_LIMIT_LSB[index], v2);
> > + if (err < 0) {
> > + pr_err("Failed to write REG_VIN%d_HIGH_LIMIT_LSB\n", index + 1);
> > + goto abort;
> > + }
> > + }
> > + }
> > +abort:
> > + mutex_unlock(&chip->access_lock);
> > + return err;
> > +}
>
> > +
> > +static int nct720x_write_event_config(struct iio_dev *indio_dev,
> > + const struct iio_chan_spec *chan,
> > + enum iio_event_type type,
> > + enum iio_event_direction dir,
> > + int state)
> > +{
> > + int err = 0;
> > + struct nct720x_chip_info *chip = iio_priv(indio_dev);
> > + int index = nct720x_chan_to_index[chan->address];
> > + unsigned int mask;
> > +
> > + mask = BIT(index);
> > +
> > + if (chan->type != IIO_VOLTAGE)
> > + return -EOPNOTSUPP;
> > +
> > + if (!state && (chip->vin_mask & mask))
> > + chip->vin_mask &= ~mask;
> > + else if (state && !(chip->vin_mask & mask))
> > + chip->vin_mask |= mask;
> > +
> > + mutex_lock(&chip->access_lock);
> guard(mutex)(&chip->access_lock);
>
We would put guard(mutex) in appropriate places.
> > +
> > + err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_1, chip->vin_mask & 0xff);
> > + if (err < 0) {
> > + pr_err("Failed to write REG_CHANNEL_ENABLE_1\n");
> > + goto abort;
> dev_err()
> return err;
>
> > + }
> > +
> > + if (chip->type == nct7202) {
>
> Gain, base this on a chip_info flag.
>
We would pass a pointer to the data structure and not use chip->type anymore.
> > + err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_2, chip->vin_mask >> 8);
> > + if (err < 0) {
> > + pr_err("Failed to write REG_CHANNEL_ENABLE_2\n");
> > + goto abort;
> Same as above.
Use dev_err
> > + }
> > + }
> > +abort:
> > + mutex_unlock(&chip->access_lock);
> > + return err;
> > +}
> > +
> > +static int nct720x_detect(struct i2c_client *client,
> > + struct i2c_board_info *info)
> > +{
> > + struct i2c_adapter *adapter = client->adapter;
> > +
> > + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> > + I2C_FUNC_SMBUS_WORD_DATA))
> > + return -ENODEV;
> > +
> > + /* Determine the chip type. */
> > + if (i2c_smbus_read_byte_data(client, REG_VENDOR_ID) != NUVOTON_ID ||
> > + i2c_smbus_read_byte_data(client, REG_CHIP_ID) != NCT720X_ID ||
> > + i2c_smbus_read_byte_data(client, REG_DEVICE_ID) != NCT720X_DEVICE_ID)
> > + return -ENODEV;
> > +
> > + strscpy(info->type, "nct720x", I2C_NAME_SIZE);
> as below. It's unusual to find a detect in an IIO driver because the firmware
> normally tells us what is there.
We would pass a pointer to the data structure to obtain the name and
not use chip->type anymore.
> > +
> > + return 0;
> > +}
> > +
> > +static const struct iio_info nct720x_info = {
> > + .read_raw = &nct720x_read_raw,
> > + .read_event_config = &nct720x_read_event_config,
> > + .write_event_config = &nct720x_write_event_config,
> > + .read_event_value = &nct720x_read_event_value,
> > + .write_event_value = &nct720x_write_event_value,
> > +};
> > +
> > +static const struct i2c_device_id nct720x_id[];
> > +
> > +static int nct720x_init_chip(struct nct720x_chip_info *chip)
> > +{
> > + int value = 0;
> > + int err = 0;
> > +
> > + /* Initial reset */
> Maybe ignore datasheet naming and call that CONFIGURATION_INIT CONFIGURATION_RESET
> at which point he comment is unneeded.
>
Understand it.
> > + err = nct720x_write_reg(chip, REG_CONFIGURATION, CONFIGURATION_INIT);
> > + if (err) {
> > + pr_err("Failed to write REG_CONFIGURATION\n");
> > + return err;
> > + }
> > +
> > + /* Enable Channel */
> > + err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_1, 0xff);
> What does 0xFF represent? I guess all channels. If so maybe need
> to build with GENMASK so it is obvious this is enabling 8 channels.
>
Use GENMASK(7, 0) and declare it.
#define REG_CHANNEL_ENABLE_1_MASK GENMASK(7, 0)
> > + if (err) {
> > + pr_err("Failed to write REG_CHANNEL_ENABLE_1\n");
> > + return err;
> > + }
> > +
> > + if (chip->type == nct7202) {
>
> Make this 'data' in the chip_info structure. Probably a flat to say
> there is a REG_CHANNEL_ENABLE_2. That chip->type wants to go away infavour
> of chip->chip_info.X flags.
>
>
We would pass a pointer to the data structure to describe the
particular variant.
> > + err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_2, 0xf);
> > + if (err) {
> > + pr_err("Failed to write REG_CHANNEL_ENABLE_2\n");
> > + return err;
> > + }
> > + }
> > +
> > + value = nct720x_read_reg16(chip, REG_CHANNEL_ENABLE_1);
> > + if (value < 0)
> > + return value;
> > + chip->vin_mask = value;
> > +
> > + /* Start monitoring if needed */
> > + value = nct720x_read_reg(chip, REG_CONFIGURATION);
>
> Using regmap would let you simple do a single bit write in one call
> Definitely look at whether there is anything stopping you using that
> helpful infrastructure.
>
We take some effort to convert regmap API to instead of I2C API
> > + if (value < 0) {
> > + pr_err("Failed to read REG_CONFIGURATION\n");
> > + return value;
> > + }
> > +
> > + value |= CONFIGURATION_START;
> > + err = nct720x_write_reg(chip, REG_CONFIGURATION, value);
> > + if (err < 0) {
> > + pr_err("Failed to write REG_CONFIGURATION\n");
>
> dev_err() if not called only from probe, return dev_err_probe() if only called
> from probe()
>
>
Yes, use dev_err_probe() in probe function.
> > + return err;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int nct720x_probe(struct i2c_client *client)
> > +{
> > + const struct i2c_device_id *id = i2c_client_get_device_id(client);
> > + struct nct720x_chip_info *chip;
> > + struct iio_dev *indio_dev;
> > + int ret;
> > + u32 tmp;
> > +
> > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > + chip = iio_priv(indio_dev);
> > +
> > + if (client->dev.of_node)
> > + chip->type = (enum nct720x_chips)device_get_match_data(&client->dev);
> > + else
> > + chip->type = i2c_match_id(nct720x_id, client)->driver_data;
>
> i2c_get_match_data() but careful with zeros... It is much better to use that with
> an actual pointer.
>
>
We would pass a pointer to the data structure and i2c_get_match_data()
to get info
> > +
> > + chip->vin_max = (chip->type == nct7201) ? NCT7201_VIN_MAX : NCT7202_VIN_MAX;
> > +
> > + ret = of_property_read_u32(client->dev.of_node, "read-vin-data-size", &tmp);
> As in dt binding. If we keep this (I'm doubtful that it makes sense) define a default
> so that the property doesn't need to be provided. 16 would be the most obvious choice.
> Then just don't check the error value when reading the property. That is.
>
> tmp = 16;
> of_property_read_u32();
>
>
>
We already remove read-vin-data-size property and use word read for VIN data.
> > + if (ret < 0) {
> > + pr_err("read-vin-data-size property not found\n");
> > + return ret;
> > + }
> > +
> > + if (tmp == 8) {
> > + chip->use_read_byte_vin = true;
> > + } else if (tmp == 16) {
> > + chip->use_read_byte_vin = false;
> > + } else {
> > + pr_err("invalid read-vin-data-size (%d)\n", tmp);
> > + return -EINVAL;
> > + }
> > +
> > + mutex_init(&chip->access_lock);
> For new code prefer
> ret = devm_mutex_init()
> if (ret)
> return ret;
>
Understand it.
> It only helps in weird debug cases but also costs us very little to support those.
> > +
> > + /* this is only used for device removal purposes */
> > + i2c_set_clientdata(client, indio_dev);
> Won't be needed after the change below.
>
Understand it.
> > +
> > + chip->client = client;
> > +
> > + ret = nct720x_init_chip(chip);
> > + if (ret < 0)
> > + return ret;
> > +
> > + indio_dev->name = id->name;
> Put the name in the chip_info structure. id->name is a complex thing
> when other firmwares come into play, so better to just hard code the strings
> somewhere so we always know what we are getting.
>
We would pass a pointer to the data structure to obtain the name
> > + indio_dev->channels = nct720x_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(nct720x_channels);
> > + indio_dev->info = &nct720x_info;
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > + iio_device_register(indio_dev);
> return devm_iio_device_register();
>
> Then you can drop the remove callback.
>
Understand it.
> > +
> > + return 0;
> > +}
> > +
> > +static void nct720x_remove(struct i2c_client *client)
>
> Don't use wildcards even within the driver. Just name everything
> after one supported part.
> nct7202_remove() etc.
>
>
Remove the nct7202_remove()
> > +{
> > + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > +
> > + iio_device_unregister(indio_dev);
> > +}
> > +
> > +static const unsigned short nct720x_address_list[] = {
> > + 0x1d, 0x1e, 0x35, 0x36, I2C_CLIENT_END
> > +};
> > +
> > +static const struct i2c_device_id nct720x_id[] = {
> > + { "nct7201", nct7201 },
> > + { "nct7202", nct7202 },
> > + {}
> { }
>
> I'm trying to slowly standardise on this formatting in IIO
> so keen not to introduce more cases. It's an arbitrary choice
> but I went with the space.
>
Rewite the data structure as below,
static const struct nct720x_adc_model_data nct7201_model_data = {
.model_name = "nct7201",
.channels = nct7201_channels,
.num_channels = ARRAY_SIZE(nct7201_channels),
.vin_max = 8,
};
static const struct nct720x_adc_model_data nct7202_model_data = {
.model_name = "nct7202",
.channels = nct7202_channels,
.num_channels = ARRAY_SIZE(nct7202_channels),
.vin_max = 12,
};
static const struct i2c_device_id nct720x_id[] = {
{ "nct7201", (kernel_ulong_t)&nct7201_model_data },
{ "nct7202", (kernel_ulong_t)&nct7202_model_data },
{ }
};
> > +};
> > +MODULE_DEVICE_TABLE(i2c, nct720x_id);
> > +
> > +static const struct of_device_id nct720x_of_match[] = {
> > + {
> > + .compatible = "nuvoton,nct7201",
> > + .data = (void *)nct7201
> > + },
> > + {
> > + .compatible = "nuvoton,nct7202",
> > + .data = (void *)nct7202
> Use a pointer to a chip_info structure with all the chip specific
> stuff encoded as data. That is both more extensible and removes some
> ambiguities around whether zero is an error or not.
>
Rewrite as below,
static const struct of_device_id nct720x_of_match[] = {
{
.compatible = "nuvoton,nct7201",
.data = &nct7201_model_data,
},
{
.compatible = "nuvoton,nct7202",
.data = &nct7202_model_data,
},
{ }
};
> > + },
> > + { },
> No trailing comma
Understand it.
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, nct720x_of_match);
> > +
> > +static struct i2c_driver nct720x_driver = {
> > + .driver = {
> > + .name = "nct720x",
> > + .of_match_table = nct720x_of_match,
> > + },
> > + .probe = nct720x_probe,
> > + .remove = nct720x_remove,
> > + .id_table = nct720x_id,
> > + .detect = nct720x_detect,
>
> Do you need detect? That's kind of ancient infrastructure that I thought
> no one used any more (same for the address list).
>
We would remove detect and address list.
> > + .address_list = nct720x_address_list,
> > +};
> > +
> > +module_i2c_driver(nct720x_driver);
> > +
> > +MODULE_AUTHOR("Eason Yang <YHYANG2@xxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Nuvoton NCT720x voltage monitor driver");
> > +MODULE_LICENSE("GPL");
>
