Messages in this thread | | | Subject | Re: [PATCH 2/3] iio: sps30: add support for serial interface | From | Lars-Peter Clausen <> | Date | Sun, 25 Apr 2021 17:52:47 +0200 |
| |
On 4/25/21 3:55 PM, Tomasz Duszynski wrote: > [...] > > +struct sps30_serial_priv { > + struct completion new_frame; > + char buf[SPS30_SERIAL_MAX_BUF_SIZE]; The driver uses char, but the serdev API uses unsigned char. Just to avoid any surprises I'd use unsigned char for all the buffers in the driver as well. > + int num; > + unsigned int chksum; > + bool escaped; > + bool done; > +}; > + > +static int sps30_serial_xfer(struct sps30_state *state, const char *buf, int size) > +{ > + struct serdev_device *serdev = to_serdev_device(state->dev); > + struct sps30_serial_priv *priv = state->priv; > + int ret; > + > + priv->num = 0; > + priv->chksum = 0; > + priv->escaped = false; > + priv->done = false; Hm... no locking with regards to the serdev callback. I guess the assumption is that we'll never receive any data without explicitly requesting it. > + > + ret = serdev_device_write(serdev, buf, size, SPS30_SERIAL_TIMEOUT); > + if (ret < 0) > + return ret; > + if (ret != size) > + return -EIO; > + > + ret = wait_for_completion_interruptible_timeout(&priv->new_frame, SPS30_SERIAL_TIMEOUT); > + if (ret < 0) > + return ret; > + if (!ret) > + return -ETIMEDOUT; > + > + return 0; > +} > [...] > +static bool sps30_serial_frame_valid(struct sps30_state *state, const char *buf) > +{ > + struct sps30_serial_priv *priv = state->priv; > + > + if ((priv->num < SPS30_SERIAL_FRAME_MIN_SIZE) || > + (priv->num != SPS30_SERIAL_FRAME_MIN_SIZE + > + priv->buf[SPS30_SERIAL_FRAME_MISO_LEN_OFFSET])) { > + dev_err(state->dev, "frame has invalid number of bytes\n"); > + return false; > + } > + > + if ((priv->buf[SPS30_SERIAL_FRAME_ADR_OFFSET] != buf[SPS30_SERIAL_FRAME_ADR_OFFSET]) || > + (priv->buf[SPS30_SERIAL_FRAME_CMD_OFFSET] != buf[SPS30_SERIAL_FRAME_CMD_OFFSET])) { > + dev_err(state->dev, "frame has wrong ADR and CMD bytes\n"); > + return false; > + } > + > + if (priv->buf[SPS30_SERIAL_FRAME_MISO_STATE_OFFSET]) { > + dev_err(state->dev, "frame with non-zero state received (0x%02x)\n", > + priv->buf[SPS30_SERIAL_FRAME_MISO_STATE_OFFSET]); > + //return false; What's with the out commented line? > + } > + > + if (priv->buf[priv->num - 2] != priv->chksum) { > + dev_err(state->dev, "frame integrity check failed\n"); > + return false; > + } > + > + return true; > +} > + > +static int sps30_serial_command(struct sps30_state *state, char cmd, void *arg, int arg_size, > + void *rsp, int rsp_size) > +{ > + struct sps30_serial_priv *priv = state->priv; > + char buf[SPS30_SERIAL_MAX_BUF_SIZE]; > + int ret, size; > + > + size = sps30_serial_prep_frame(buf, cmd, arg, arg_size); > + ret = sps30_serial_xfer(state, buf, size); > + if (ret) > + return ret; > + > + if (!sps30_serial_frame_valid(state, buf)) > + return -EIO; > + > + if (rsp) { > + rsp_size = clamp((int)priv->buf[SPS30_SERIAL_FRAME_MISO_LEN_OFFSET], 0, rsp_size); If buf is unsigned char this can be a min_t(unsigned int, ...). And maybe also make rsp_size unsigned int. > + memcpy(rsp, &priv->buf[SPS30_SERIAL_FRAME_MISO_DATA_OFFSET], rsp_size); > + } > + > + return rsp_size; > +} > + > +static int sps30_serial_receive_buf(struct serdev_device *serdev, const unsigned char *buf, > + size_t size) > +{ > + struct iio_dev *indio_dev = dev_get_drvdata(&serdev->dev); > + struct sps30_serial_priv *priv; > + struct sps30_state *state; > + unsigned char byte; > + int i; > + > + if (!indio_dev) > + return 0;
> + > + state = iio_priv(indio_dev); > + priv = state->priv; > + > + /* just in case device put some unexpected data on the bus */ > + if (priv->done) > + return size; > + > + /* wait for the start of frame */ > + if (!priv->num && size && buf[0] != SPS30_SERIAL_SOF_EOF) > + return 1; > + > + if (priv->num + size >= ARRAY_SIZE(priv->buf)) > + size = ARRAY_SIZE(priv->buf) - priv->num; > + > + for (i = 0; i < size; i++) { > + byte = buf[i]; > + /* remove stuffed bytes on-the-fly */ > + if (byte == SPS30_SERIAL_ESCAPE_CHAR) { > + priv->escaped = true; > + continue; > + } > + > + byte = sps30_serial_get_byte(priv->escaped, byte); > + if (priv->escaped && !byte) > + dev_warn(state->dev, "unrecognized escaped char (0x%02x)\n", byte); > + priv->chksum += byte; > + /* incrementing here would complete rx just after reading SOF */ > + priv->buf[priv->num] = byte; > + > + if (priv->num++ && !priv->escaped && byte == SPS30_SERIAL_SOF_EOF) {
This is a bit to tricky for my taste.
How about.
priv->num++
if (priv->num > 1 && ...)
> + /* SOF, EOF and checksum itself are not checksummed */ > + priv->chksum -= 2 * SPS30_SERIAL_SOF_EOF + priv->buf[priv->num - 2]; > + priv->chksum = (unsigned char)~priv->chksum; To keep the whole checksum stuff simpler, maybe just compute it in sps30_serial_frame_valid() over the whole set of data. > + priv->done = true; > + complete(&priv->new_frame); > + i++; > + break; > + } > + > + priv->escaped = false; > + } > + > + return i; > +} > [...] > +static int sps30_serial_probe(struct serdev_device *serdev) > +{ > [...] > + return sps30_probe(dev, KBUILD_MODNAME, priv, &sps30_serial_ops); Usually the IIO device name should just be the part number. Ideally the application should not care about the backend. I'd just pass "sps30" here for the name. > +}
| |