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

[Tomasz Duszynski]

On Tue, Jun 02, 2020 at 07:57:23PM +0200, Tomasz Duszynski wrote:
> On Tue, Jun 02, 2020 at 08:04:16PM +0300, Andy Shevchenko wrote:
> > 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;
> >
> > ?
>
> Matter of style but since I will be doing other changes I can touch that
> too.
>
> >
> > ...
> >
> > > +       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.
> >
>
> Fine.
>
> > ...
> >
> > > +       switch (txbuf[1]) {
> > > +       case SCD30_SERDEV_WRITE:
> >
> > > +               if (memcmp(txbuf, txbuf, txsize)) {
> >
> > I'm not sure I understand this.
> >
>
> Ah, thanks for catching this. tx should be compared to rx.
>
> > > +                       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?
> >
>
> I'd rather leave it as is.
>

On the second thought, that would fit 100 column line.

> > > +                       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