linux-kernel - Re: [PATCH v3 3/4] iio: chemical: scd30: add serial interface driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VfjWG_3XC5FJoaU7XXJc+04JTbEKdjZK=g6ffBPvJNhxA@mail.gmail.com>
Date:   Tue, 2 Jun 2020 20:04:16 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Tomasz Duszynski <tomasz.duszynski@...akon.com>
Cc:     linux-iio <linux-iio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Jonathan Cameron <jic23@...nel.org>,
        Peter Meerwald <pmeerw@...erw.net>
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@...akon.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