| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z_aeEuIk9brES6dM@smile.fi.intel.com>
Date: Wed, 9 Apr 2025 19:19:30 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Eason Yang <j2anfernee@...il.com>
Cc: jic23@...nel.org, lars@...afoo.de, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, dlechner@...libre.com, nuno.sa@...log.com,
javier.carrasco.cruz@...il.com, gstols@...libre.com,
tgamblin@...libre.com, alisadariana@...il.com,
antoniu.miclaus@...log.com, eblanc@...libre.com,
jstephan@...libre.com, matteomartelli3@...il.com,
angelogioacchino.delregno@...labora.com, herve.codina@...tlin.com,
marcelo.schmitt@...log.com, chanh@...amperecomputing.com,
KWLIU@...oton.com, yhyang2@...oton.com, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 2/2] iio: adc: add support for Nuvoton NCT7201
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?
...
> +#define NCT7201_IN_SCALING 4995
Interesting number, just want to confirm it's indeed 4995 and not 4095.
...
> +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?
> + *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?
> + 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?
> + "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()?
> + 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?
> + "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?
> + "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.
> + 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?
> +}
...
> +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.
> +
> + 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?
> + ret = nct7201_init_chip(chip);
> + if (ret < 0)
Do you expect positive returned values? What is their meaning?
Why do you skip them?
> + 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
Powered by blists - more mailing lists