Messages in this thread | | | From | Andy Shevchenko <> | Date | Wed, 5 Aug 2020 10:33:03 +0300 | Subject | Re: [PATCH 1/2] iio: imu: adis16480: Add support for burst read function |
| |
On Tue, Aug 4, 2020 at 1:04 PM Alexandru Ardelean <alexandru.ardelean@analog.com> 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
| |