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

[Hugo Villeneuve]

On Tue, 10 Jan 2023 19:50:42 +0100
Christophe JAILLET <christophe.jaillet@xxxxxxxxxx> wrote:

> Le 10/01/2023 à 17:01, Hugo Villeneuve a écrit :
> > From: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>
> > 
> > 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@xxxxxxxxxxxx>
> > ---
> 
> Hi,
> 
> should there be a v3, a few nits below.

Hi,
there will definitely be a V3 (missing include), thanks for the input, I will incorporate these changes into V3.

Hugo.


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


-- 
Hugo Villeneuve <hugo@xxxxxxxxxxx>