linux-kernel - Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Mon, 1 May 2017 14:31:00 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Eva Rachel Retuya <eraretuya@...il.com>
Cc:     Jonathan Cameron <jic23@...nel.org>, linux-iio@...r.kernel.org,
        Hartmut Knaack <knaack.h@....de>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Peter Meerwald <pmeerw@...erw.net>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Michael Hennerich <michael.hennerich@...log.com>,
        Daniel Baluta <daniel.baluta@...il.com>,
        Alison Schofield <amsfield22@...il.com>,
        Florian Vaussard <florian.vaussard@...g-vd.ch>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

On Sat, Apr 29, 2017 at 10:49 AM, Eva Rachel Retuya <eraretuya@...il.com> wrote:
> The ADXL345 provides a DATA_READY interrupt function to signal
> availability of new data. This interrupt function is latched and can be
> cleared by reading the data registers. The polarity is set to active
> high by default.
>
> Support this functionality by setting it up as an IIO trigger.
>
> In addition, two output pins INT1 and INT2 are available for driving
> interrupts. Allow mapping to either pins by specifying the
> interrupt-names property in device tree.

> -int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> +int adxl345_core_probe(struct device *dev, struct regmap *regmap, int irq,
>                        const char *name);

I think I commented this once. Instead of increasing parameters,
please introduce a new struct (as separate preparatory patch) which
will hold current parameters. Let's call it
strut adxl345_chip {
 struct device *dev;
 struct regmap *regmap;
 const char *name;
};

I insisnt in this chage.

>  #include <linux/delay.h>
> +#include <linux/interrupt.h>
>  #include <linux/module.h>

> +#include <linux/of_irq.h>

Can we get rid of gnostic resource providers?

> +static const struct iio_trigger_ops adxl345_trigger_ops = {

> +       .owner = THIS_MODULE,

Do we still need this kind of lines?

> +       .set_trigger_state = adxl345_drdy_trigger_set_state,
> +};

>  static const struct iio_info adxl345_info = {

>         .driver_module  = THIS_MODULE,

Ditto, though it's in the current code.

>         .read_raw       = adxl345_read_raw,
>  };

> +       /*
> +        * Any bits set to 0 send their respective interrupts to the INT1 pin,
> +        * whereas bits set to 1 send their respective interrupts to the INT2
> +        * pin. Map all interrupts to the specified pin.
> +        */
> +       of_irq = of_irq_get_byname(dev->of_node, "INT2");

So, can we get it in resourse provider agnostic way?

> +       if (of_irq == irq)
> +               regval = 0xFF;
> +       else
> +               regval = 0x00;

regval = of_irq == irq ? 0xff : 0x00; ?

> +
> +       ret = regmap_write(data->regmap, ADXL345_REG_INT_MAP, regval);
> +       if (ret < 0) {
> +               dev_err(dev, "Failed to set up interrupts: %d\n", ret);
> +               return ret;
> +       }

-- 
With Best Regards,
Andy Shevchenko