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>
