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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zyx0GUudmSui8Y2I@smile.fi.intel.com>
Date: Thu, 7 Nov 2024 10:02:33 +0200
From: Andy Shevchenko <andy@...nel.org>
To: Eason Yang <j2anfernee@...il.com>
Cc: avifishman70@...il.com, tmaimon77@...il.com, tali.perry1@...il.com,
	venture@...gle.com, yuenn@...gle.com, benjaminfair@...gle.com,
	jic23@...nel.org, lars@...afoo.de, robh@...nel.org,
	krzk+dt@...nel.org, conor+dt@...nel.org, nuno.sa@...log.com,
	dlechner@...libre.com, javier.carrasco.cruz@...il.com,
	marcelo.schmitt@...log.com, olivier.moysan@...s.st.com,
	mitrutzceclan@...il.com, matteomartelli3@...il.com,
	alisadariana@...il.com, joao.goncalves@...adex.com,
	marius.cristea@...rochip.com, mike.looijmans@...ic.nl,
	chanh@...amperecomputing.com, KWLIU@...oton.com,
	yhyang2@...oton.com, openbmc@...ts.ozlabs.org,
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/2] iio: adc: add Nuvoton NCT720x ADC driver

On Wed, Nov 06, 2024 at 10:39:16AM +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.

...

> + * Copyright (c) 2022 Nuvoton Inc.

2024?

...

+ array_size.h
+ bits.h

> +#include <linux/delay.h>
> +#include <linux/device.h>

+ err.h

> +#include <linux/i2c.h>

> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

Move these to be a separate group...

> +#include <linux/init.h>

+ mod_devicetable.h

> +#include <linux/module.h>
> +#include <linux/mutex.h>

+ types.h

...somewhere here after a blank line.

...

> +static const u8 REG_VIN[VIN_MAX] = { 0x00, 0x01, 0x02, 0x03, 0x04, 0x05,
> +				     0x06, 0x07, 0x08, 0x09, 0x0A, 0x0B};

Better to have it done like this

static const u8 REG_VIN[VIN_MAX] = {
	0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07,	/* 0 - 7 */
	0x08, 0x09, 0x0A, 0x0B,				/* 8 - 11 */
};

Note the following: the values are placed on separate lines with trailing
commas on each line and with grouping by powwer-of-2 per line.

> +static const u8 REG_VIN_HIGH_LIMIT[VIN_MAX] = { 0x20, 0x22, 0x24, 0x26, 0x28, 0x2A,
> +						0x2C, 0x2E, 0x30, 0x32, 0x34, 0x36};
> +static const u8 REG_VIN_LOW_LIMIT[VIN_MAX] = { 0x21, 0x23, 0x25, 0x27, 0x29, 0x2B,
> +					       0x2D, 0x2F, 0x31, 0x33, 0x35, 0x37};
> +static const u8 REG_VIN_HIGH_LIMIT_LSB[VIN_MAX] = { 0x40, 0x42, 0x44, 0x46, 0x48, 0x4A,
> +						    0x4C, 0x4E, 0x50, 0x52, 0x54, 0x56};
> +static const u8 REG_VIN_LOW_LIMIT_LSB[VIN_MAX] = { 0x41, 0x43, 0x45, 0x47, 0x49, 0x4B,
> +						   0x4D, 0x4F, 0x51, 0x53, 0x55, 0x57};
> +static u8 nct720x_chan_to_index[] = {
> +	0,	/* Not used */
> +	0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11
> +};

Ditto for the above initialisers.

> +
> +

One blank line is enough.

> +/* List of supported devices */
> +enum nct720x_chips {
> +	nct7201, nct7202

Leave trailing comma.

> +};

...

> +/* 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);
> +}
> +
> +/*
> + * 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);

If you are going with this, use cleanup.h.

> +	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);

Hmm... Does it require to have two separate transactions? In case of regmap,
can't bulk transfer be performed for that?

> +	}
> +
> +	mutex_unlock(&chip->access_lock);
> +	return ret;
> +}

I have no explanations why regmap I2C can't be used.

...

> +/* 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 */

Is there any justification for the chosen value (I mean datasheet
recommendations or so)?

> +	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) {
> +			/*
> +			 * 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 */

Hmm... I would expect volt to be __be16, but since you have two separate reads
it's probably fine.

> +		} else {
> +			/* NCT7201/NCT7202 also supports read word-size data */
> +			volt = nct720x_read_word_swapped_reg(chip, REG_VIN[index]);
> +		}
> +
> +		/* Voltage(V) = 13bitCountValue * 0.0004995 */
> +		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;

Yeah, this will gone with cleanup.h in use.
Ditto for all other places like this.

> +}

...

> +{
> +	struct nct720x_chip_info *chip = iio_priv(indio_dev);
> +	int index, err = 0;
> +	long v1, v2, volt;

Why long? Wouldn't int / unsigned int suffice for v1 / v2?

> +
> +	index = nct720x_chan_to_index[chan->address];
> +	volt = (val * 10000) / NCT720X_IN_SCALING;

Please, use named constant for 10000 as you used it at least twice in the code.

> +	v1 = volt >> 5;
> +	v2 = (volt & 0x1f) << 3;

In the similar way it seems like some endianess and bit width manipulations.

Can v be an array of u8:s which you just fill with put_unaligned_be16() or so?

> +	if (chan->type != IIO_VOLTAGE)
> +		return -EOPNOTSUPP;
> +
> +	if (info == IIO_EV_INFO_VALUE) {
> +		if (dir == IIO_EV_DIR_FALLING) {
> +			mutex_lock(&chip->access_lock);
> +			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;
> +}

...

> +	strscpy(info->type, "nct720x", I2C_NAME_SIZE);

Use 2-argument strscpy().

...

> +static const struct i2c_device_id nct720x_id[];

Remove this. It's not needed.

...

> +static int nct720x_init_chip(struct nct720x_chip_info *chip)
> +{

> +	int value = 0;

Why signed?

> +	int err = 0;


Redundant assignments.

> +	/* Initial reset */
> +	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);

Magic 0xff.
Should be GENMASK() ? Anything else?

> +	if (err) {
> +		pr_err("Failed to write REG_CHANNEL_ENABLE_1\n");
> +		return err;
> +	}
> +
> +	if (chip->type == nct7202) {
> +		err = nct720x_write_reg(chip, REG_CHANNEL_ENABLE_2, 0xf);

Ditto.

> +		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);
> +	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");
> +		return err;
> +	}
> +
> +	return 0;
> +}

...

> +	ret = of_property_read_u32(client->dev.of_node, "read-vin-data-size", &tmp);

No OF-centric APIs in a new code, please.
Use device_property_read_u32() and include property.h for that.

> +	if (ret < 0) {
> +		pr_err("read-vin-data-size property not found\n");
> +		return ret;
> +	}

...

> +	mutex_init(&chip->access_lock);

	ret = devm_mutex_init(...);

...

> +	/* this is only used for device removal purposes */
> +	i2c_set_clientdata(client, indio_dev);

Will be gone after removing ->remove().

...

> +	iio_device_register(indio_dev);

No error checks? In any case devm_ variant especially needs this.

> +	return 0;

	return devm_iio_device_register(...);

...

> +static const struct i2c_device_id nct720x_id[] = {
> +	{ "nct7201", nct7201 },
> +	{ "nct7202", nct7202 },

Please, use chip_info custom data structure instead of enum values.

> +	{}
> +};
> +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

As per above and leave trailing commas.

> +	},
> +	{ },

Drop trailing comma in the terminator entries.

> +};

> +

Redundant blank line.

> +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,
> +	.address_list = nct720x_address_list,
> +};

> +

Redundant blank line.

> +module_i2c_driver(nct720x_driver);

-- 
With Best Regards,
Andy Shevchenko



Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ