linux-kernel - Re: [PATCH] iio: accel: adxl355: Add triggered buffer support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 13 Aug 2021 13:03:25 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Puranjay Mohan <puranjay12@...il.com>
Cc:     Michael Hennerich <Michael.Hennerich@...log.com>,
        Jonathan Cameron <jic23@...nel.org>,
        linux-iio <linux-iio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        "Bogdan, Dragos" <Dragos.Bogdan@...log.com>,
        Darius <Darius.Berghe@...log.com>
Subject: Re: [PATCH] iio: accel: adxl355: Add triggered buffer support

On Fri, Aug 13, 2021 at 11:35 AM Puranjay Mohan <puranjay12@...il.com> wrote:
>
> Provide a way for continuous data capture by setting up buffer support. The
> data ready signal exposed at the DRDY pin of the ADXL355 is exploited as
> a hardware interrupt which triggers to fill the buffer.

...

>   *
>   * Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adxl354_adxl355.pdf
>   */
> -

Unrelated change.

...

> +#include <linux/iio/buffer.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>

I would rather regroup this like

linux/*

asm/*

linux/iio*

...

> +#include <linux/of_irq.h>

Okay, this is due to the absence of generic fwnode_irq_get_byname() or so.

...

>  struct adxl355_data {

> +       int irq;

Depending on container_of and frequency of usage this is not a good
location for this.
Strating from regmap pointer is much better (no pointer arithmetics involved).

>         struct regmap *regmap;
>         struct device *dev;
>         struct mutex lock; /* lock to protect op_mode */

>  };

...

> +       ret = regmap_bulk_read(data->regmap, ADXL355_XDATA3_REG,
> +                              &data->buffer.buf[1],
> +                              3);

ARRAY_SIZE()? Or put this 3 to the previous line, it will be easier to read.

Ditto for the rest of the similar code.

> +       if (ret)
> +               goto out_unlock_notify;

...

> +       /*
> +        * Would be good to move it to the generic version.

Something like "TODO: Would be..." ?

> +        */
> +       ret = of_irq_get_byname(dev->of_node, "DRDY");
> +       if (ret > 0)
> +               data->irq = ret;

-- 
With Best Regards,
Andy Shevchenko