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. 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. 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. > + > + *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. > + 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. > + 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. > + > + return ret; > +} > +
