linux-kernel - Re: [PATCH v6 2/2] iio: adc: add support for Nuvoton NCT7201

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250418171326.07634113@jic23-huawei>
Date: Fri, 18 Apr 2025 17:13:26 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Eason Yang <j2anfernee@...il.com>
Cc: 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,
 alisadariana@...il.com, tgamblin@...libre.com, olivier.moysan@...s.st.com,
 antoniu.miclaus@...log.com, eblanc@...libre.com,
 andriy.shevchenko@...ux.intel.com, joao.goncalves@...adex.com,
 tobias.sperling@...ting.com, marcelo.schmitt@...log.com,
 angelogioacchino.delregno@...labora.com, thomas.bonnefille@...tlin.com,
 herve.codina@...tlin.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 v6 2/2] iio: adc: add support for Nuvoton NCT7201

On Wed, 16 Apr 2025 16:17:34 +0800
Eason Yang <j2anfernee@...il.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 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.
> 
> Signed-off-by: Eason Yang <j2anfernee@...il.com>
Hi Eason,

A few trivial things from another glance through.

Thanks,

Jonathan


> +
> +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)
> +{
> +	struct device *dev = chip->dev;
> +	__le16 data = cpu_to_le16(NCT7201_REG_CHANNEL_ENABLE_MASK);

Assign this value down near where it is used. That will generally help
readability.

> +	unsigned int value;
> +	int err;
> +
> +	err = regmap_write(chip->regmap, NCT7201_REG_CONFIGURATION,
> +			   NCT7201_BIT_CONFIGURATION_RESET);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to reset chip\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.
I'd hyphenate power-up perhaps.

> +	 */
> +	fsleep(25000);
> +	err = regmap_read(chip->regmap, NCT7201_REG_BUSY_STATUS, &value);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to read busy status\n");
> +	if (!(value & NCT7201_BIT_PWR_UP))
> +		return dev_err_probe(dev, -EIO, "Failed to power up after reset\n");
> +
> +	/* Enable Channel */
> +	if (chip->num_vin_channels <= 8)
> +		err = regmap_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
> +				   NCT7201_REG_CHANNEL_ENABLE_MASK);

This is a little odd as I think you are writing an 8 bit register with a 12 bit mask.
Sure it works, but confusing thing to see.  If some future 6 channel device comes
along this <= 8 will seem odd too. Maybe generate the mask from teh number
of channels?

> +	else
> +		err = regmap_bulk_write(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
> +					&data, sizeof(data));
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to enable channel\n");
> +
> +	err = regmap_bulk_read(chip->regmap, NCT7201_REG_CHANNEL_ENABLE_1,
> +			       &chip->vin_mask, sizeof(chip->vin_mask));
> +	if (err)
> +		return dev_err_probe(dev, err,
> +				     "Failed to read channel enable register\n");
> +
> +	/* Start monitoring if needed */
> +	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");
> +
> +	return 0;
> +}