Re: [PATCH 1/2] iio: imu: adis16480: Add support for burst read function

[Andy Shevchenko]

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