On Tue, Aug 4, 2020 at 1:04 PM Alexandru Ardelean
<alexandru.ardelean@xxxxxxxxxx> wrote:
> +static irqreturn_t adis16480_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct adis16480 *st = iio_priv(indio_dev);
> + struct adis *adis = &st->adis;
> + int ret, bit, offset, i = 0;
It makes it easier to understand the code if the assignment will be
attached to the actual user, i.e. for_each_set_bit() below.
> + __be16 data[ADIS16495_BURST_MAX_DATA], *buffer, *d;
> + if (!adis->buffer)
> + return -ENOMEM;
Is it possible?
> + mutex_lock(&adis->state_lock);
> + if (adis->current_page != 0) {
> + adis->tx[0] = ADIS_WRITE_REG(ADIS_REG_PAGE_ID);
> + adis->tx[1] = 0;
> + spi_write(adis->spi, adis->tx, 2);
> + }
> +
> + ret = spi_sync(adis->spi, &adis->msg);
> + if (ret)
> + dev_err(&adis->spi->dev, "Failed to read data: %d\n", ret);
No bail out? Then it's dev_warn().
> + adis->current_page = 0;
> + mutex_unlock(&adis->state_lock);
> +
> + if (!(adis->burst && adis->burst->en)) {
> + buffer = adis->buffer;
> + goto push_to_buffers;
> + }
> + /*
> + * After making the burst request, the response can have one or two
> + * "don't care" 16-bit responses, before the BURST_ID.
> + */
> + d = (__be16 *)adis->buffer;
> + for (offset = 0; offset < 3; offset++) {
> + if (d[offset] == ADIS16495_BURST_ID) {
> + offset += 2; /* TEMP_OUT */
> + break;
> + }
> + }
So, we can end up with offset == 3 here. Is it by design?
Otherwise, offset+=2 maybe moved out of the loop.
And in any case comment is needed.
> + for_each_set_bit(bit, indio_dev->active_scan_mask,
> + indio_dev->masklength) {
> + /*
> + * When burst mode is used, temperature is the first data
> + * channel in the sequence, but the temperature scan index
> + * is 10.
> + */
> + if (bit == ADIS16480_SCAN_TEMP) {
> + data[2 * i] = d[offset];
> + } else {
> + /* The lower register data is sequenced first */
> + data[2 * i] = d[2 * bit + offset + 2];
I would do ' + 0', but it's up to you.
> + data[2 * i + 1] = d[2 * bit + offset + 1];
> + }
> + i++;
> + }
> +
> + buffer = data;
> +
> +push_to_buffers:
> + iio_push_to_buffers_with_timestamp(indio_dev, buffer,
> + pf->timestamp);
> +
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
...
> + /* If burst mode is supported, enable it by default */
> + if (st->chip_info->burst) {
> + st->adis.burst = st->chip_info->burst;
> + st->adis.burst_extra_len = st->chip_info->burst->extra_len;
> + }
You may use it directly (the sizeof struct somelike 16 bytes and one
pointer on a 64-bit machine is 8, simply embed it into the struct and
assign it always.
...
+ .extra_len = 12 * sizeof(u16),
__be16 would be precise?
--
With Best Regards,
Andy Shevchenko
