Re: [PATCH v3 3/4] drivers: iio: chemical: Add support for Sensirion SCD4x CO2 sensor

[Jonathan Cameron]

On Mon, 27 Sep 2021 14:28:24 +0200
Roan van Dijk <roan@xxxxxxxxxxx> wrote:

> On 25-09-2021 18:23, Jonathan Cameron wrote:
> > On Wed, 22 Sep 2021 12:39:24 +0200
> > Roan van Dijk <roan@xxxxxxxxxxx> wrote:
> >   
> >> This is a driver for the SCD4x CO2 sensor from Sensirion. The sensor is
> >> able to measure CO2 concentration, temperature and relative humdity.
> >> The sensor uses a photoacoustic principle for measuring CO2 concentration.
> >> An I2C interface is supported by this driver in order to communicate with
> >> the sensor.
> >>
> >> Signed-off-by: Roan van Dijk <roan@xxxxxxxxxxx>  
> > 
> > Hi Roan,
> > 
> > Only thing in here of significance is that the format for available attribute
> > is wrong + it needs adding to the ABI docs.
> > 
> > Given we are going to have a v4, I noted a few other minor things to tidy up.
> > 
> > Thanks,
> > 
> > Jonathan
> >   
> 
> Thank you for your feedback. I will make a new patch with these fixes.
> 
> The documentation for the _available and the fixes you suggested, have 
> already been added in patch [PATCH v3 4/4]. Do I still need to add 
> something to the ABI documentation or is it fine like this?

Ah. I missed _available documentation for some reason. Sorry about that
it's fine as you have it.

> 
> However, I will change the KernelVersion to 5.15 of the _available 
> description in patch v4. I left this at 5.8 and that's not right, 
> because this information is newly added.
Good spot.

J

> 
> Thanks,
> 
> Roan
> 
> >   
> >> +static int scd4x_read(struct scd4x_state *state, enum scd4x_cmd cmd,
> >> +			void *response, int response_sz)
> >> +{
> >> +	struct i2c_client *client = state->client;
> >> +	char buf[SCD4X_READ_BUF_SIZE];
> >> +	char *rsp = response;
> >> +	int i, ret;
> >> +	char crc;
> >> +
> >> +	/*
> >> +	 * Measurement needs to be stopped before sending commands.
> >> +	 * Except for reading measurement and data ready command.
> >> +	 */
> >> +	if ((cmd != CMD_GET_DATA_READY) && (cmd != CMD_READ_MEAS)) {
> >> +		ret = scd4x_send_command(state, CMD_STOP_MEAS);
> >> +		if (ret)
> >> +			return ret;
> >> +
> >> +		/* execution time for stopping measurement */
> >> +		msleep_interruptible(500);
> >> +	}
> >> +
> >> +	/*CRC byte for every 2 bytes of data */  
> > 
> > /* CRC..
> > 
> > Please check for similar as otherwise we'll get 'cleanup' patches the moment various
> > scripts hit this new code and it'll waste our time!
> >   
> >> +	response_sz += response_sz / 2;
> >> +
> >> +	put_unaligned_be16(cmd, buf);
> >> +	ret = scd4x_i2c_xfer(state, buf, 2, buf, response_sz);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	for (i = 0; i < response_sz; i += 3) {
> >> +		crc = crc8(scd4x_crc8_table, buf + i, 2, CRC8_INIT_VALUE);
> >> +		if (crc != buf[i + 2]) {
> >> +			dev_err(&client->dev, "CRC error\n");
> >> +			return -EIO;
> >> +		}
> >> +
> >> +		*rsp++ = buf[i];
> >> +		*rsp++ = buf[i + 1];
> >> +	}
> >> +
> >> +	/* start measurement */
> >> +	if ((cmd != CMD_GET_DATA_READY) && (cmd != CMD_READ_MEAS)) {
> >> +		ret = scd4x_send_command(state, CMD_START_MEAS);
> >> +		if (ret)
> >> +			return ret;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >> +  
> > 
> > ...
> >   
> >> +
> >> +static IIO_DEVICE_ATTR_RW(calibration_auto_enable, 0);
> >> +static IIO_DEVICE_ATTR_WO(calibration_forced_value, 0);
> >> +
> >> +static IIO_CONST_ATTR(calibration_forced_value_available,
> >> +	       __stringify(SCD4X_FRC_MIN_PPM 1 SCD4X_FRC_MAX_PPM));  
> > 
> > Ah, I wasn't completely clear on this.  See the main ABI doc for
> > _available
> > 
> > Format for this needs to include brackets to indicate it's a range
> > rather than 3 numbers
> > "[MIN 1  MAX]"
> > 
> > 
> > Having added this it also needs to be in the ABI documentation.
> > Whilst somewhat trivial, all ABI should be documented there.
> >   
> >> +
> >> +static struct attribute *scd4x_attrs[] = {
> >> +	&iio_dev_attr_calibration_auto_enable.dev_attr.attr,
> >> +	&iio_dev_attr_calibration_forced_value.dev_attr.attr,
> >> +	&iio_const_attr_calibration_forced_value_available.dev_attr.attr,
> >> +	NULL
> >> +};
> >> +
> >> +static const struct attribute_group scd4x_attr_group = {
> >> +	.attrs = scd4x_attrs,
> >> +};
> >> +
> >> +static const struct iio_info scd4x_info = {
> >> +	.attrs = &scd4x_attr_group,
> >> +	.read_raw = scd4x_read_raw,
> >> +	.write_raw = scd4x_write_raw,
> >> +};
> >> +  
> > 
> > ...
> >   
> >> +
> >> +static irqreturn_t scd4x_trigger_handler(int irq, void *p)
> >> +{
> >> +	struct iio_poll_func *pf = p;
> >> +	struct iio_dev *indio_dev = pf->indio_dev;
> >> +	struct scd4x_state *state = iio_priv(indio_dev);
> >> +	struct {
> >> +		uint16_t data[3];
> >> +		int64_t ts __aligned(8);
> >> +	} scan;
> >> +	int ret;
> >> +	uint16_t buf[3];
> >> +
> >> +	mutex_lock(&state->lock);
> >> +	ret = scd4x_read_poll(state, buf);
> >> +	mutex_unlock(&state->lock);
> >> +	if (ret)
> >> +		goto out;
> >> +
> >> +	memset(&scan, 0, sizeof(scan));
> >> +	memcpy(scan.data, buf, sizeof(buf));  
> > 
> > I missed this before, but why not do the scd4x_read_poll() directly into scan->data after
> > you've done the memset?  That way you avoid the need for a memcpy.
> > 
> > i.e.
> > 
> > 	memset(&scan, 0, sizeof(scan));
> > 	mutex_lock(&state->lock)
> > 	ret = scd4x_read_poll(state, scan->data);
> > 	mutex_unlock(&state->lock);
> > 	if (ret)
> > 	...
> > 
> >   
> >> +
> >> +	iio_push_to_buffers_with_timestamp(indio_dev, &scan, iio_get_time_ns(indio_dev));
> >> +out:
> >> +	iio_trigger_notify_done(indio_dev->trig);
> >> +	return IRQ_HANDLED;
> >> +}
> >> +