On Tue, Jun 2, 2020 at 7:49 PM Tomasz Duszynski
<tomasz.duszynski@xxxxxxxxxxx> 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
