linux-kernel - Re: [PATCH v2 1/2] iio: adc: ti-ads7924: add Texas Instruments ADS7924 driver

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: <c4b5c190-5fb5-17f2-69ce-7137ef7f33db@wanadoo.fr>
Date:   Tue, 10 Jan 2023 19:50:42 +0100
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     Hugo Villeneuve <hugo@...ovil.com>, hvilleneuve@...onoff.com,
        jic23@...nel.org, lars@...afoo.de, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org
Cc:     linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] iio: adc: ti-ads7924: add Texas Instruments
 ADS7924 driver

Le 10/01/2023 à 17:01, Hugo Villeneuve a écrit :
> From: Hugo Villeneuve <hvilleneuve@...onoff.com>
> 
> The Texas Instruments ADS7924 is a 4 channels, 12-bit analog to
> digital converter (ADC) with an I2C interface.
> 
> Datasheet: https://www.ti.com/lit/gpn/ads7924
> 
> Signed-off-by: Hugo Villeneuve <hvilleneuve@...onoff.com>
> ---

Hi,

should there be a v3, a few nits below.

CJ

>   MAINTAINERS                  |   7 +
>   drivers/iio/adc/Kconfig      |  11 +
>   drivers/iio/adc/Makefile     |   1 +
>   drivers/iio/adc/ti-ads7924.c | 492 +++++++++++++++++++++++++++++++++++
>   4 files changed, 511 insertions(+)
>   create mode 100644 drivers/iio/adc/ti-ads7924.c
> 

[...]

> +static int ads7924_get_channels_config(struct i2c_client *client,
> +				       struct iio_dev *indio_dev)
> +{
> +	struct ads7924_data *priv = iio_priv(indio_dev);
> +	struct device *dev = priv->dev;
> +	struct fwnode_handle *node;
> +	int num_channels = 0;
> +
> +	device_for_each_child_node(dev, node) {
> +		u32 pval;
> +		unsigned int channel;
> +
> +		if (fwnode_property_read_u32(node, "reg", &pval)) {
> +			dev_err(dev, "invalid reg on %pfw\n", node);
> +			continue;
> +		}
> +
> +		channel = pval;
> +		if (channel >= ADS7924_CHANNELS) {
> +			dev_err(dev, "invalid channel index %d on %pfw\n",
> +				channel, node);
> +			continue;
> +		}
> +
> +		num_channels++;
> +	}
> +
> +	if (num_channels > 0) {
> +		dev_dbg(dev, "found %d ADC channels\n", num_channels);
> +		return 0;
> +	} else {
> +		return -EINVAL;
> +	}

	if (num_channels <= 0)
		return -EINVAL;

	dev_dbg(dev, "found %d ADC channels\n", num_channels);
	return 0;

is much more usual.

> +}
> +

[...]

> +static int ads7924_reset(struct iio_dev *indio_dev)
> +{
> +	struct ads7924_data *data = iio_priv(indio_dev);
> +
> +	if (data->reset_gpio) {
> +		gpiod_set_value(data->reset_gpio, 1); /* Assert. */
> +		/* Educated guess: assert time not specified in datasheet... */
> +		mdelay(100);
> +		gpiod_set_value(data->reset_gpio, 0); /* Deassert. */
> +	} else {
> +		int ret;

having 'ret' near 'struct ads7924_data *data' is more usual and saves 1 LoC.

> +
> +		/*
> +		 * A write of 10101010 to this register will generate a
> +		 * software reset of the ADS7924.
> +		 */
> +		ret = regmap_write(data->regmap, ADS7924_RESET_REG, 0b10101010);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +};