Re: [PATCH v5 2/2] iio: adc: add support for Nuvoton NCT7201

From: Yu-Hsian Yang
Date: Mon Apr 14 2025 - 10:11:13 EST

Next message: Sasha Levin: "[PATCH AUTOSEL 5.10 11/11] ext4: make block validity check resistent to sb bh corruption"
Previous message: Geert Uytterhoeven: "Re: [PATCH v2 00/12] Add support for Renesas RZ/V2N SoC and EVK"
In reply to: Andy Shevchenko: "Re: [PATCH v5 2/2] iio: adc: add support for Nuvoton NCT7201"
Next in thread: Andy Shevchenko: "Re: [PATCH v5 2/2] iio: adc: add support for Nuvoton NCT7201"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Dear Andy,

Thanks for the review and the comments.
Will fix all.

Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> 於 2025年4月10日週四上午12:19寫道：
>
> On Wed, Apr 09, 2025 at 09:23:51AM +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 all the 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 are
> > supported.
>
> > +#include <linux/array_size.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/types.h>
> > +#include <linux/unaligned.h>
>
> ...
>
> > +#define NCT7201_VIN_MAX 12
>
> Is this in volts? Can you add a unit suffix?

Rename it as
#define NCT7201_MAX_CHANNEL 12

>
> ...
>
> > +#define NCT7201_IN_SCALING 4995
> > +#define NCT720X_IN_SCALING_FACTOR 10000
> Interesting number, just want to confirm it's indeed 4995 and not 4095.
>

These definitions are for real voltage calculations,
the formula is Voltage(V) = 13bitCountValue * 0.0004995
However, the definitions are not used anymore.
So remove them.

> ...
>
> > +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);
> > + 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;
> > + } else {
> > + err = regmap_read(chip->regmap16, NCT7201_REG_VIN_HIGH_LIMIT(chan->address),
> > + &value);
> > + if (err < 0)
> > + return err;
> > + }
>
> if (dir == IIO_EV_DIR_FALLING) {
> err = regmap_read(chip->regmap16, NCT7201_REG_VIN_LOW_LIMIT(chan->address),
> &value);
> } else {
> err = regmap_read(chip->regmap16, NCT7201_REG_VIN_HIGH_LIMIT(chan->address),
> &value);
> }
> if (err)
> return err;
>
> Here and elsewhere why ' < 0' is used? Do you expect positive return values
> from those?
>

In regmap_read function, A value of zero will be returned on success,
a negative errno will be returned in error cases.
We don't have a positive return case.

> > + *val = FIELD_GET(NCT7201_REG_VIN_MASK, value);
> > +
> > + 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);
>
> > + int err = 0;
>
> Useless assignment.
>

> > + if (chan->type != IIO_VOLTAGE)
> > + return -EOPNOTSUPP;
> > +
> > + if (info != IIO_EV_INFO_VALUE)
> > + return -EOPNOTSUPP;
> > +
> > + if (dir == IIO_EV_DIR_FALLING) {
> > + err = regmap_write(chip->regmap16, NCT7201_REG_VIN_LOW_LIMIT(chan->address),
> > + FIELD_PREP(NCT7201_REG_VIN_MASK, val));
> > + if (err < 0)
> > + return err;
> > + } else {
> > + err = regmap_write(chip->regmap16, NCT7201_REG_VIN_HIGH_LIMIT(chan->address),
> > + FIELD_PREP(NCT7201_REG_VIN_MASK, val));
> > + if (err < 0)
> > + return err;
> > + }
> > +
> > + return 0;
>
> if (dir == IIO_EV_DIR_FALLING) {
> err = regmap_write(chip->regmap16, NCT7201_REG_VIN_LOW_LIMIT(chan->address),
> FIELD_PREP(NCT7201_REG_VIN_MASK, val));
> } else {
> err = regmap_write(chip->regmap16, NCT7201_REG_VIN_HIGH_LIMIT(chan->address),
> FIELD_PREP(NCT7201_REG_VIN_MASK, val));
> }
> return err;
>
>
> > +}
>
> ...
>
> > +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;
> > + int err;
> > +
> > + if (chan->type != IIO_VOLTAGE)
> > + return -EOPNOTSUPP;
> > +
> > + mask = BIT(chan->address);
> > +
> > + if (state)
> > + chip->vin_mask |= mask;
> > + else
> > + chip->vin_mask &= ~mask;
>
> > + if (chip->num_vin_channels <= 8) {
> > + err = regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
> > + chip->vin_mask);
> > + if (err < 0)
> > + return err;
> > + } else {
> > + err = regmap_bulk_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
> > + &chip->vin_mask, sizeof(chip->vin_mask));
> > + if (err < 0)
> > + return err;
> > + }
> > +
> > + return 0;
>
> Same as above.
>
> > +}
>
> ...
>
> > +static int nct7201_init_chip(struct nct7201_chip_info *chip)
> > +{
> > + u8 data[2] = {0};
>
> '0' is not needed. Shouldn't this be __le16 or __be16 instead?
>

+ __be16 data = NCT7201_REG_CHANNEL_ENABLE_MASK;

> > + unsigned int value;
> > + int err;
> > +
> > + err = regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION,
> > + NCT7201_BIT_CONFIGURATION_RESET);
> > + if (err < 0)
> > + return dev_err_probe(&chip->client->dev, -EIO,
>
> struct device *dev = &chip->client->dev;
>
> at the top of the function will help a lot in tiding up the below code.
>
> Shadowed error code, why?
>

Do not shadow the return code by -EIO and let the regmap API caller decide.

> > + "Failed to write NCT7201_REG_CONFIGURATION\n");
> > +
> > + /*
> > + * 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);
>
> No sleep? Why? Can't you use fsleep()?
>

+ msleep(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,
>
> Shadowed error code, why?

Do not shadow the return code by -EIO and let the regmap API caller decide.

>
> > + "Failed to power up after reset\n");
> > +
> > + /* Enable Channel */
> > + if (chip->num_vin_channels <= 8) {
> > + data[0] = NCT7201_REG_CHANNEL_ENABLE_1_MASK;
> > + err = regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1, data[0]);
> > + if (err < 0)
> > + return dev_err_probe(&chip->client->dev, -EIO,
>
> Why error code is shadowed?
>

Do not shadow the return code by -EIO and let the regmap API caller decide.

> > + "Failed to write NCT7201_REG_CHANNEL_ENABLE_1\n");
> > + } else {
> > + data[0] = NCT7201_REG_CHANNEL_ENABLE_1_MASK;
> > + data[1] = NCT7201_REG_CHANNEL_ENABLE_2_MASK;
> > + err = regmap_bulk_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
> > + data, ARRAY_SIZE(data));
> > + if (err < 0)
> > + return dev_err_probe(&chip->client->dev, -EIO,
>
> Ditto.
>
> > + "Failed to write NCT7201_REG_CHANNEL_ENABLE_1 and NCT7201_REG_CHANNEL_ENABLE_2\n");
> > + }
>
> Just make it 16-bit type, define one value and use just simple English
> in the error message: "Failed to write channel enable mask\n");
>
> Same to all your error messages.
>

We would examine all the error messages in simple English.

> > + chip->vin_mask = get_unaligned_le16(data);
> > +
> > + /* Start monitoring if needed */
> > + err = regmap_read(chip->regmap, NCT7201_REG_CONFIGURATION, &value);
> > + if (err < 0)
> > + return dev_err_probe(&chip->client->dev, -EIO,
> > + "Failed to read NCT7201_REG_CONFIGURATION\n");
>
> > + regmap_set_bits(chip->regmap, NCT7201_REG_CONFIGURATION, NCT7201_BIT_CONFIGURATION_START);
>
> > + return 0;
>
> No error check? Why?
>

+ err = regmap_set_bits(chip->regmap, NCT7201_REG_CONFIGURATION,
+ NCT7201_BIT_CONFIGURATION_START);
+ if (err)
+ return dev_err_probe(dev, err, "Failed to start monitoring\n");

> > +}
>
> ...
>
> > +static int nct7201_probe(struct i2c_client *client)
> > +{
> > + const struct nct7201_adc_model_data *model_data;
>
> struct device *dev = &client->dev;
>
> > + struct nct7201_chip_info *chip;
> > + struct iio_dev *indio_dev;
> > + int ret;
> > +
> > + model_data = i2c_get_match_data(client);
> > + if (!model_data)
> > + return -EINVAL;
>
> ENODEV is more suitable here.
>
+ return -ENODEV

> > +
> > + 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;
>
> > + chip->client = client;
>
> How exactly is _client_ used elsewhere? Shouldn't it be just a struct device
> pointer?

Yes, it is just a struct device pointer.
In nct7201_init_chip(chip), we would use the chip->client->dev as
dev_err_probe() parameter
>
> > + ret = nct7201_init_chip(chip);
> > + if (ret < 0)
>
> Do you expect positive returned values? What is their meaning?
> Why do you skip them?
>

No, we don't expect positive return values.

> > + 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);
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Next message: Sasha Levin: "[PATCH AUTOSEL 5.10 11/11] ext4: make block validity check resistent to sb bh corruption"
Previous message: Geert Uytterhoeven: "Re: [PATCH v2 00/12] Add support for Renesas RZ/V2N SoC and EVK"
In reply to: Andy Shevchenko: "Re: [PATCH v5 2/2] iio: adc: add support for Nuvoton NCT7201"
Next in thread: Andy Shevchenko: "Re: [PATCH v5 2/2] iio: adc: add support for Nuvoton NCT7201"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]