On Sat, 31 Jul 2021 11:06:25 -0700
Guenter Roeck [off-list ref] wrote:

On 7/31/21 9:39 AM, Jonathan Cameron wrote:

quoted

On Tue, 27 Jul 2021 18:35:19 +0200
Andreas Klinger [off-list ref] wrote:
  

quoted

sgp40 is a gas sensor used for measuring the air quality.

This driver is reading the raw resistance value which can be passed to
a userspace algorithm for further calculation.

The raw value is also used to calculate an estimated absolute voc index
in the range from 0 to 500. For this purpose the raw_mean value of the
resistance for which the index value is 250 might be set up as a
calibration step.

Compensation of relative humidity and temperature is supported and can
be used by writing to device attributes of the driver.

There is a predecesor sensor type (sgp30) already existing. This driver
module was not extended because the new sensor is quite different in its
i2c telegrams.

Signed-off-by: Andreas Klinger <ak@it-klinger.de>  

Hi Andreas,

Non standard ABI in here, so we are missing documentation in Documentation/ABI/testing/sysfs-bus-iio-*

Otherwise a few suggestions inline.

Thanks,

Jonathan

  

quoted

---  

[ ... ]

quoted

quoted

+static int sgp40_read_raw(struct iio_dev *indio_dev,
+			struct iio_chan_spec const *chan, int *val,
+			int *val2, long mask)
+{
+	struct sgp40_data *data = iio_priv(indio_dev);
+	int ret;
+	u16 raw;
+	int voc;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&data->lock);
+		ret = sgp40_measure_raw(data, &raw);
+		if (ret) {
+			mutex_unlock(&data->lock);
+			return ret;
+		}
+		*val = raw;
+		ret = IIO_VAL_INT;
+		mutex_unlock(&data->lock);
+		break;
+	case IIO_CHAN_INFO_PROCESSED:
+		mutex_lock(&data->lock);
+		ret = sgp40_measure_raw(data, &raw);
+		if (ret) {
+			mutex_unlock(&data->lock);
+			return ret;
+		}
+		ret = sgp40_calc_voc(data, raw, &voc);
+		if (ret) {
+			mutex_unlock(&data->lock);
+			return ret;
+		}
+		*val = voc;
+		ret = IIO_VAL_INT;
+		mutex_unlock(&data->lock);  

You are holding the lock longer than needed - it would be good
to reduce this, hopefully removing the need for unlocking separately
in each of the error paths.
  

quoted

+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return ret;  

Drop this as you can't get here.
  

Are you sure ? I see several "break;" above.

Doh! In that case, return directly where it has break above so we don't
need to go see if anything else happens in those paths.

Guenter