From: Roan van Dijk <roan@protonic.nl>
To: Jonathan Cameron <jic23@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>,
Tomasz Duszynski <tomasz.duszynski@octakon.com>,
linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, david@protonic.nl,
Lars-Peter Clausen <lars@metafoo.de>
Subject: Re: [PATCH v3 3/4] drivers: iio: chemical: Add support for Sensirion SCD4x CO2 sensor
Date: Mon, 27 Sep 2021 14:28:24 +0200 [thread overview]
Message-ID: <b9cffd75-0b86-544c-4832-070d12606de4@protonic.nl> (raw)
In-Reply-To: <20210925172307.305be961@jic23-huawei>
On 25-09-2021 18:23, Jonathan Cameron wrote:
> On Wed, 22 Sep 2021 12:39:24 +0200
> Roan van Dijk <roan@protonic.nl> 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@protonic.nl>
>
> 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?
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.
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;
>> +}
>> +
next prev parent reply other threads:[~2021-09-27 12:28 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-22 10:39 [PATCH v3 0/4] iio: chemical: Add support for Sensirion SCD4x CO2 sensor Roan van Dijk
2021-09-22 10:39 ` [PATCH v3 1/4] dt-bindings: iio: chemical: sensirion,scd4x: Add yaml description Roan van Dijk
2021-09-22 10:39 ` [PATCH v3 2/4] MAINTAINERS: Add myself as maintainer of the scd4x driver Roan van Dijk
2021-09-22 10:39 ` [PATCH v3 3/4] drivers: iio: chemical: Add support for Sensirion SCD4x CO2 sensor Roan van Dijk
2021-09-25 16:23 ` Jonathan Cameron
2021-09-27 12:28 ` Roan van Dijk [this message]
2021-09-30 15:59 ` Jonathan Cameron
2021-09-22 10:39 ` [PATCH v3 4/4] iio: documentation: Document scd4x calibration use Roan van Dijk
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b9cffd75-0b86-544c-4832-070d12606de4@protonic.nl \
--to=roan@protonic.nl \
--cc=david@protonic.nl \
--cc=devicetree@vger.kernel.org \
--cc=jic23@kernel.org \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robh+dt@kernel.org \
--cc=tomasz.duszynski@octakon.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).