Messages in this thread | | | From | Andy Shevchenko <> | Date | Tue, 2 Jun 2020 20:04:16 +0300 | Subject | Re: [PATCH v3 3/4] iio: chemical: scd30: add serial interface driver |
| |
On Tue, Jun 2, 2020 at 7:49 PM Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote: > > Add serial interface driver for the SCD30 sensor.
...
> +static u16 scd30_serdev_cmd_lookup_tbl[] = { > + [CMD_START_MEAS] = 0x0036, > + [CMD_STOP_MEAS] = 0x0037, > + [CMD_MEAS_INTERVAL] = 0x0025, > + [CMD_MEAS_READY] = 0x0027, > + [CMD_READ_MEAS] = 0x0028, > + [CMD_ASC] = 0x003a, > + [CMD_FRC] = 0x0039, > + [CMD_TEMP_OFFSET] = 0x003b, > + [CMD_FW_VERSION] = 0x0020, > + [CMD_RESET] = 0x0034,
Hmm... Can't we keep them ordered by value?
> +};
...
> + ret = wait_for_completion_interruptible_timeout(&priv->meas_ready, > + SCD30_SERDEV_TIMEOUT); > + if (ret > 0) > + ret = 0; > + else if (!ret) > + ret = -ETIMEDOUT; > + > + return ret;
Perhaps
if (ret < 0) return ret; if (ret == 0) return -ETIMEDOUT; return 0;
?
...
> + char txbuf[SCD30_SERDEV_MAX_BUF_SIZE] = { SCD30_SERDEV_ADDR }, > + rxbuf[SCD30_SERDEV_MAX_BUF_SIZE], *rsp = response;
Please, apply type to each variable separately.
...
> + switch (txbuf[1]) { > + case SCD30_SERDEV_WRITE:
> + if (memcmp(txbuf, txbuf, txsize)) {
I'm not sure I understand this.
> + dev_err(state->dev, "wrong message received\n"); > + return -EIO; > + } > + break; > + case SCD30_SERDEV_READ:
> + if (rxbuf[2] != (rxsize - > + SCD30_SERDEV_RX_HEADER_SIZE - > + SCD30_SERDEV_CRC_SIZE)) {
Perhaps you can come up with better indentation/ line split?
> + dev_err(state->dev, > + "received data size does not match header\n"); > + return -EIO; > + } > + > + rxsize -= SCD30_SERDEV_CRC_SIZE; > + crc = get_unaligned_le16(rxbuf + rxsize); > + if (crc != scd30_serdev_calc_crc(rxbuf, rxsize)) { > + dev_err(state->dev, "data integrity check failed\n"); > + return -EIO; > + } > + > + rxsize -= SCD30_SERDEV_RX_HEADER_SIZE; > + memcpy(rsp, rxbuf + SCD30_SERDEV_RX_HEADER_SIZE, rxsize); > + break; > + default: > + dev_err(state->dev, "received unknown op code\n"); > + return -EIO; > + } > + > + return 0; > +}
...
> +static struct serdev_device_driver scd30_serdev_driver = { > + .driver = {
> + .name = KBUILD_MODNAME,
This is not the best what we can do. The name is an ABI and better if it will be constant (independent on file name).
> + .of_match_table = scd30_serdev_of_match, > + .pm = &scd30_pm_ops, > + }, > + .probe = scd30_serdev_probe, > +};
-- With Best Regards, Andy Shevchenko
| |