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

[Christophe JAILLET <christophe.jaillet@wanadoo.fr>]

Le 10/01/2023 à 17:01, Hugo Villeneuve a écrit :
> From: Hugo Villeneuve <hvilleneuve@dimonoff.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@dimonoff.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;
> +};