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

[Hugo Villeneuve]

On Sat, 14 Jan 2023 16:28:41 +0000
Jonathan Cameron <jic23@xxxxxxxxxx> wrote:

> On Fri, 13 Jan 2023 14:49:58 -0500
> Hugo Villeneuve <hugo@xxxxxxxxxxx> wrote:
> 
> > 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
> 
> This counts as a normal tag, so there shouldn't be blank line between
> it and the SOB.

Fixed.

> 
> A few other small things inline noticed on this read through.
> I can fix these up whilst applying if nothing else comes up for v3
> and DT binding reviewers are happy.  If you are doing a v4 for
> other reasons, please address these comments in that.

Ok,
I have already prepared a V4 with all these changes, just in case. Thank you for your comments.


> Jonathan
> 
> 
> 
> > 
> > Signed-off-by: Hugo Villeneuve <hvilleneuve@xxxxxxxxxxxx>
> ...
> > diff --git a/drivers/iio/adc/ti-ads7924.c b/drivers/iio/adc/ti-ads7924.c
> > new file mode 100644
> > index 000000000000..c24fae4ef8e0
> > --- /dev/null
> > +++ b/drivers/iio/adc/ti-ads7924.c
> > @@ -0,0 +1,474 @@
> 
> ...
> 
> > +static int ads7924_read_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan, int *val,
> > +			    int *val2, long mask)
> > +{
> > +	int ret, vref_uv;
> > +	struct ads7924_data *data = iio_priv(indio_dev);
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		mutex_lock(&data->lock);
> > +		ret = ads7924_get_adc_result(data, chan, val);
> > +		mutex_unlock(&data->lock);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		return IIO_VAL_INT;
> > +	case IIO_CHAN_INFO_SCALE:
> > +		vref_uv = regulator_get_voltage(data->vref_reg);
> > +		if (vref_uv < 0)
> > +			return -EINVAL;
> 
> Better to return the error value from regulator_get_voltage() rather
> than replace it with -EINVAL.

Of course, done.

> > +
> > +		*val =  vref_uv / 1000; /* Convert reg voltage to mV */
> > +		*val2 = ADS7924_BITS;
> > +		return IIO_VAL_FRACTIONAL_LOG2;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static const struct iio_info ads7924_info = {
> > +	.read_raw = ads7924_read_raw,
> > +};
> > +
> > +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)
> 
> How would it be less than 0?  if (!num_channels) works fine I think.

Done.

> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ads7924_set_conv_mode(struct ads7924_data *data, int mode)
> > +{
> > +	int ret;
> > +	unsigned int mode_field;
> > +	struct device *dev = data->dev;
> > +
> > +	/*
> > +	 * When switching between modes, be sure to first select the Awake mode
> > +	 * and then switch to the desired mode. This procedure ensures the
> > +	 * internal control logic is properly synchronized.
> > +	 */
> > +	if (mode != ADS7924_MODECNTRL_IDLE) {
> > +		mode_field = FIELD_PREP(ADS7924_MODECNTRL_MODE_MASK,
> > +					ADS7924_MODECNTRL_AWAKE);
> > +
> > +		ret = regmap_update_bits(data->regmap, ADS7924_MODECNTRL_REG,
> > +					 ADS7924_MODECNTRL_MODE_MASK,
> > +					 mode_field);
> > +		if (ret) {
> > +			dev_warn(dev, "failed to set awake mode (%pe)\n",
> > +				 ERR_PTR(ret));
> 
> As below.

Agreed, converted to dev_err.

> 
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	mode_field = FIELD_PREP(ADS7924_MODECNTRL_MODE_MASK, mode);
> > +
> > +	ret = regmap_update_bits(data->regmap, ADS7924_MODECNTRL_REG,
> > +				 ADS7924_MODECNTRL_MODE_MASK, mode_field);
> > +	if (ret)
> > +		dev_warn(dev, "failed to set mode %d (%pe)\n", mode,
> > +			 ERR_PTR(ret));
> 
> Why warning? Seems like a fairly critical error to me.
> dev_err() more appropriate perhaps.

Agreed, converted to dev_err.

> > +
> > +	return ret;
> > +}
> > +
> 
> 


-- 
Hugo Villeneuve <hugo@xxxxxxxxxxx>