lkml.org 
[lkml]   [2024]   [May]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v3 9/9] drivers: iio: imu: Add support for adis1657x family
    On Fri, 17 May 2024 10:47:50 +0300
    Ramona Gradinariu <ramona.bolboaca13@gmail.com> wrote:

    > Add support for ADIS1657X family devices in already exiting ADIS16475
    > driver.
    >
    > Signed-off-by: Ramona Gradinariu <ramona.bolboaca13@gmail.com>

    Whilst it's not necessarily vital to support, it I'm curious about
    what happens to the hardware timestamp? I thought we had one driver
    still doing hardware timestamps directly to the buffer, but I can't
    find it so I guess we now deal with alignment in the few devices with
    this support. The st_lsm6dsx has this sort of combining of local
    and fifo timestamps for example.

    As it stands I think you push the same timestamp for all scans read
    from the fifo on a particular watermark interrupt? That isn't
    ideal given we should definitely be able to do better than that.

    > +
    > +static const struct iio_dev_attr *adis16475_fifo_attributes[] = {
    > + &iio_dev_attr_hwfifo_watermark_min.dev_attr.attr,
    > + &iio_dev_attr_hwfifo_watermark_max.dev_attr.attr,
    > + &iio_dev_attr_hwfifo_watermark.dev_attr.attr,
    > + &iio_dev_attr_hwfifo_enabled.dev_attr.attr,
    The autobuilder caught this one. Drop the dev_attr.attr.

    > + NULL
    > +};
    > +

    > +
    > +static const struct iio_buffer_setup_ops adis16475_buffer_ops = {
    > + .postenable = adis16475_buffer_postenable,
    > + .postdisable = adis16475_buffer_postdisable,
    > +};
    > +
    > +static int adis16475_set_watermark(struct iio_dev *indio_dev, unsigned int val)
    > +{
    > + struct adis16475 *st = iio_priv(indio_dev);
    > + int ret;
    > + u16 wm_lvl;
    > +
    > + adis_dev_lock(&st->adis);

    As a follow up perhaps consider defining magic to use guard() for these as there are
    enough users that will be simplified to make it worth the effort.

    > +
    > + val = min_t(unsigned int, val, ADIS16575_MAX_FIFO_WM);
    > +
    > + wm_lvl = ADIS16575_WM_LVL(val - 1);
    > + ret = __adis_update_bits(&st->adis, ADIS16475_REG_FIFO_CTRL, ADIS16575_WM_LVL_MASK, wm_lvl);
    > + if (ret)
    > + goto unlock;
    > +
    > + st->fifo_watermark = val;
    > +
    > +unlock:
    > + adis_dev_unlock(&st->adis);
    > + return ret;
    > +}
    > +




    > @@ -1514,8 +1978,20 @@ static int adis16475_probe(struct spi_device *spi)
    > if (ret)
    > return ret;
    >
    > - ret = devm_adis_setup_buffer_and_trigger(&st->adis, indio_dev,
    > - adis16475_trigger_handler);
    > + if (st->info->flags & ADIS16475_HAS_FIFO) {
    > + ret = devm_adis_setup_buffer_and_trigger_with_attrs(&st->adis, indio_dev,
    > + adis16475_trigger_handler_with_fifo,
    > + &adis16475_buffer_ops,
    > + adis16475_fifo_attributes);
    > + if (ret)
    > + return ret;
    blank line here.

    > + /* Update overflow behavior to always overwrite the oldest sample. */
    > + ret = adis_update_bits(&st->adis, ADIS16475_REG_FIFO_CTRL,
    > + ADIS16575_OVERFLOW_MASK, (u16)ADIS16575_OVERWRITE_OLDEST);
    if (ret) in here.
    > + } else {
    > + ret = devm_adis_setup_buffer_and_trigger(&st->adis, indio_dev,
    > + adis16475_trigger_handler);
    and if (ret) in here and drop the one that follows.

    otherwise we end up with odd code pattern of handling one case in this scope and not
    the other two.

    > + }

    \
     
     \ /
      Last update: 2024-05-19 20:57    [W:4.257 / U:0.060 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site