Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1633270 > unrolled thread

[PATCH v2 0/4] iio: accel: adxl345: Add support for buffered readings

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/4] iio: accel: adxl345: Add support for buffered readings Eva Rachel Retuya <eraretuya@gmail.com> - 2017-04-29 09:50 +0200
    [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and data_ready functions Eva Rachel Retuya <eraretuya@gmail.com> - 2017-04-29 10:00 +0200
      Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and  data_ready functions Jonathan Cameron <jic23@kernel.org> - 2017-05-01 02:30 +0200
        Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and  data_ready functions Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-01 21:50 +0200
          Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and data_ready functions Jonathan Cameron <jic23@jic23.retrosnub.co.uk> - 2017-05-01 21:50 +0200
            Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and  data_ready functions Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-01 22:10 +0200
              Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and data_ready functions Jonathan Cameron <jic23@jic23.retrosnub.co.uk> - 2017-05-01 22:20 +0200
        Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and  data_ready functions Eva Rachel Retuya <eraretuya@gmail.com> - 2017-05-02 13:40 +0200
          Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and  data_ready functions Jonathan Cameron <jic23@kernel.org> - 2017-05-02 18:40 +0200
            Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and  data_ready functions Eva Rachel Retuya <eraretuya@gmail.com> - 2017-05-10 15:10 +0200
      Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and  data_ready functions Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-01 13:30 +0200
        Re: [PATCH v2 2/4] iio: accel: adxl345_core: Introduce set_mode and  data_ready functions Eva Rachel Retuya <eraretuya@gmail.com> - 2017-05-02 13:50 +0200
    [PATCH v2 4/4] iio: accel: adxl345: Add support for triggered buffer Eva Rachel Retuya <eraretuya@gmail.com> - 2017-04-29 10:00 +0200
      Re: [PATCH v2 4/4] iio: accel: adxl345: Add support for triggered  buffer Jonathan Cameron <jic23@kernel.org> - 2017-05-01 02:50 +0200
        Re: [PATCH v2 4/4] iio: accel: adxl345: Add support for triggered  buffer Eva Rachel Retuya <eraretuya@gmail.com> - 2017-05-02 14:30 +0200
          Re: [PATCH v2 4/4] iio: accel: adxl345: Add support for triggered  buffer Jonathan Cameron <jic23@kernel.org> - 2017-05-02 18:10 +0200
      Re: [PATCH v2 4/4] iio: accel: adxl345: Add support for triggered buffer Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-01 13:30 +0200
        Re: [PATCH v2 4/4] iio: accel: adxl345: Add support for triggered  buffer Eva Rachel Retuya <eraretuya@gmail.com> - 2017-05-02 14:30 +0200
    [PATCH v2 1/4] dt-bindings: iio: accel: adxl345: Add optional interrupt-names support Eva Rachel Retuya <eraretuya@gmail.com> - 2017-04-29 10:00 +0200
    [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Eva Rachel Retuya <eraretuya@gmail.com> - 2017-04-29 10:00 +0200
      Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Jonathan Cameron <jic23@kernel.org> - 2017-05-01 02:40 +0200
        Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Rob Herring <robh+dt@kernel.org> - 2017-05-02 05:10 +0200
          Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Jonathan Cameron <jic23@kernel.org> - 2017-05-02 18:00 +0200
            Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Eva Rachel Retuya <eraretuya@gmail.com> - 2017-05-10 16:40 +0200
        Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Eva Rachel Retuya <eraretuya@gmail.com> - 2017-05-02 14:00 +0200
      Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-01 14:00 +0200
        Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Eva Rachel Retuya <eraretuya@gmail.com> - 2017-05-02 14:20 +0200
          Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-02 23:10 +0200
            Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Eva Rachel Retuya <eraretuya@gmail.com> - 2017-05-10 15:30 +0200
          Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Jonathan Cameron <jic23@kernel.org> - 2017-05-05 20:30 +0200
            Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger Eva Rachel Retuya <eraretuya@gmail.com> - 2017-05-10 15:40 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1633515 — Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

On 29/04/17 08:49, Eva Rachel Retuya 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.
> 
> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
Coming together nicely, but a few more bits and pieces inline...

One slight worry is that the irq names stuff is to restrictive
as we want to direct different interrupts to different pins if
both are supported!

Jonathan
> ---
> Changes in v2:
> * Provide a detailed commit message
> * Move the of_irq_get_byname() check in core file in order to avoid
>   introducing another parameter in probe()
> * adxl345_irq():
>   * return values directly
>   * switch from iio_trigger_poll() to iio_trigger_poll_chained(), the former
>     should only be called at the top-half not at the bottom-half.
> * adxl345_drdy_trigger_set_state():
>   * move regmap_get_device() to definition block
>   * regmap_update_bits(): line splitting - one parameter per line, remove extra
>     parenthesis
> * probe()
>   * use variable 'regval' to hold value to be written to the register and call
>     regmap_write() unconditionally
>   * fix line splitting in devm_request_threaded_irq() and devm_iio_trigger_alloc()
>   * Switch to devm_iio_trigger_register()
> 
>  drivers/iio/accel/adxl345.h      |   2 +-
>  drivers/iio/accel/adxl345_core.c | 104 ++++++++++++++++++++++++++++++++++++++-
>  drivers/iio/accel/adxl345_i2c.c  |   3 +-
>  drivers/iio/accel/adxl345_spi.c  |   2 +-
>  4 files changed, 107 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> index c1ddf39..d2fa806 100644
> --- a/drivers/iio/accel/adxl345.h
> +++ b/drivers/iio/accel/adxl345.h
> @@ -11,7 +11,7 @@
>  #ifndef _ADXL345_H_
>  #define _ADXL345_H_
>  
> -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);
>  int adxl345_core_remove(struct device *dev);
>  
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index b8a212c..b8be0d7 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -9,15 +9,20 @@
>   */
>  
>  #include <linux/delay.h>
> +#include <linux/interrupt.h>
>  #include <linux/module.h>
> +#include <linux/of_irq.h>
>  #include <linux/regmap.h>
>  
>  #include <linux/iio/iio.h>
> +#include <linux/iio/trigger.h>
>  
>  #include "adxl345.h"
>  
>  #define ADXL345_REG_DEVID		0x00
>  #define ADXL345_REG_POWER_CTL		0x2D
> +#define ADXL345_REG_INT_ENABLE		0x2E
> +#define ADXL345_REG_INT_MAP		0x2F
>  #define ADXL345_REG_INT_SOURCE		0x30
>  #define ADXL345_REG_DATA_FORMAT		0x31
>  #define ADXL345_REG_DATAX0		0x32
> @@ -39,6 +44,8 @@
>  
>  #define ADXL345_DEVID			0xE5
>  
> +#define ADXL345_IRQ_NAME		"adxl345_event"
I'd just put this inline.  It doesn't really give any benefit to
have this defined at the top.
> +
>  /*
>   * In full-resolution mode, scale factor is maintained at ~4 mg/LSB
>   * in all g ranges.
> @@ -49,6 +56,8 @@
>  static const int adxl345_uscale = 38300;
>  
>  struct adxl345_data {
> +	struct iio_trigger *data_ready_trig;
> +	bool data_ready_trig_on;
>  	struct regmap *regmap;
>  	struct mutex lock; /* protect this data structure */
>  	u8 data_range;
> @@ -158,17 +167,62 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
>  	return -EINVAL;
>  }
>  
> +static irqreturn_t adxl345_irq(int irq, void *p)
> +{
> +	struct iio_dev *indio_dev = p;
> +	struct adxl345_data *data = iio_priv(indio_dev);
> +	int ret;
> +	u32 int_stat;
> +
> +	ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE, &int_stat);
> +	if (ret < 0)
> +		return IRQ_HANDLED;
> +
> +	if (int_stat & ADXL345_INT_DATA_READY) {
> +		iio_trigger_poll_chained(data->data_ready_trig);
> +		return IRQ_HANDLED;
> +	}
> +
> +	return IRQ_NONE;
> +}
> +
> +static int adxl345_drdy_trigger_set_state(struct iio_trigger *trig, bool state)
> +{
> +	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> +	struct adxl345_data *data = iio_priv(indio_dev);
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int ret;
> +
> +	ret = regmap_update_bits(data->regmap,
> +				 ADXL345_REG_INT_ENABLE,
> +				 ADXL345_INT_DATA_READY,
> +				 state ? ADXL345_INT_DATA_READY : 0);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to update INT_ENABLE bits\n");
> +		return ret;
> +	}
> +	data->data_ready_trig_on = state;
> +
> +	return ret;
> +}
> +
> +static const struct iio_trigger_ops adxl345_trigger_ops = {
> +	.owner = THIS_MODULE,
> +	.set_trigger_state = adxl345_drdy_trigger_set_state,
> +};
> +
>  static const struct iio_info adxl345_info = {
>  	.driver_module	= THIS_MODULE,
>  	.read_raw	= adxl345_read_raw,
>  };
>  
> -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)
>  {
>  	struct adxl345_data *data;
>  	struct iio_dev *indio_dev;
>  	u32 regval;
> +	int of_irq;
>  	int ret;
>  
>  	ret = regmap_read(regmap, ADXL345_REG_DEVID, &regval);
> @@ -199,6 +253,22 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  		dev_err(dev, "Failed to set data range: %d\n", ret);
>  		return ret;
>  	}
> +	/*
> +	 * 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.
This is an interesting comment.  The usual reason for dual interrupt
pins is precisely to not map all functions to the same one.  That allows
for a saving in querying which interrupt it is by having just the data ready
on one pin and just the events on the other...

Perhaps the current approach won't support that mode of operation?
Clearly we can't merge a binding that enforces them all being the same
and then change it later as it'll be incompatible.

I'm not quite sure how one should do this sort of stuff in DT though.

Rob?
> +	 */
> +	of_irq = of_irq_get_byname(dev->of_node, "INT2");
> +	if (of_irq == irq)
> +		regval = 0xFF;
> +	else
> +		regval = 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;
> +	}
>  
>  	mutex_init(&data->lock);
>  
> @@ -209,6 +279,38 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>  	indio_dev->channels = adxl345_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
>  
> +	if (irq > 0) {
> +		ret = devm_request_threaded_irq(dev,
> +						irq,
> +						NULL,
> +						adxl345_irq,
> +						IRQF_TRIGGER_HIGH |
> +						IRQF_ONESHOT,
> +						ADXL345_IRQ_NAME,
> +						indio_dev);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to request irq: %d\n", irq);
> +			return ret;
> +		}
> +
> +		data->data_ready_trig = devm_iio_trigger_alloc(dev,
> +							       "%s-dev%d",
> +							       indio_dev->name,
> +							       indio_dev->id);
> +		if (!data->data_ready_trig)
> +			return -ENOMEM;
> +
> +		data->data_ready_trig->dev.parent = dev;
> +		data->data_ready_trig->ops = &adxl345_trigger_ops;
> +		iio_trigger_set_drvdata(data->data_ready_trig, indio_dev);
> +
> +		ret = devm_iio_trigger_register(dev, data->data_ready_trig);
> +		if (ret) {
> +			dev_err(dev, "Failed to register trigger: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0)
>  		dev_err(dev, "iio_device_register failed: %d\n", ret);
> diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
> index 05e1ec4..31af702 100644
> --- a/drivers/iio/accel/adxl345_i2c.c
> +++ b/drivers/iio/accel/adxl345_i2c.c
> @@ -34,7 +34,8 @@ static int adxl345_i2c_probe(struct i2c_client *client,
>  		return PTR_ERR(regmap);
>  	}
>  
> -	return adxl345_core_probe(&client->dev, regmap, id ? id->name : NULL);
> +	return adxl345_core_probe(&client->dev, regmap, client->irq,
> +				  id ? id->name : NULL);
>  }
>  
>  static int adxl345_i2c_remove(struct i2c_client *client)
> diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
> index 6d65819..75a8c12 100644
> --- a/drivers/iio/accel/adxl345_spi.c
> +++ b/drivers/iio/accel/adxl345_spi.c
> @@ -42,7 +42,7 @@ static int adxl345_spi_probe(struct spi_device *spi)
>  		return PTR_ERR(regmap);
>  	}
>  
> -	return adxl345_core_probe(&spi->dev, regmap, id->name);
> +	return adxl345_core_probe(&spi->dev, regmap, spi->irq, id->name);
>  }
>  
>  static int adxl345_spi_remove(struct spi_device *spi)
> 

[toc] | [prev] | [next] | [standalone]

#1634097 — Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

On Sun, Apr 30, 2017 at 7:32 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 29/04/17 08:49, Eva Rachel Retuya 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.
>>
>> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
> Coming together nicely, but a few more bits and pieces inline...
>
> One slight worry is that the irq names stuff is to restrictive
> as we want to direct different interrupts to different pins if
> both are supported!

[...]

>> @@ -199,6 +253,22 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>>               dev_err(dev, "Failed to set data range: %d\n", ret);
>>               return ret;
>>       }
>> +     /*
>> +      * 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.
> This is an interesting comment.  The usual reason for dual interrupt
> pins is precisely to not map all functions to the same one.  That allows
> for a saving in querying which interrupt it is by having just the data ready
> on one pin and just the events on the other...
>
> Perhaps the current approach won't support that mode of operation?
> Clearly we can't merge a binding that enforces them all being the same
> and then change it later as it'll be incompatible.
>
> I'm not quite sure how one should do this sort of stuff in DT though.
>
> Rob?

DT should just describe what is connected which I gather here could be
either one or both IRQs. We generally distinguish the IRQs with the
interrupt-names property and then retrieve it as below.

>> +      */
>> +     of_irq = of_irq_get_byname(dev->of_node, "INT2");
>> +     if (of_irq == irq)
>> +             regval = 0xFF;
>> +     else
>> +             regval = 0x00;

[toc] | [prev] | [next] | [standalone]

#1634491 — Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

On 02/05/17 04:01, Rob Herring wrote:
> On Sun, Apr 30, 2017 at 7:32 PM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 29/04/17 08:49, Eva Rachel Retuya 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.
>>>
>>> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
>> Coming together nicely, but a few more bits and pieces inline...
>>
>> One slight worry is that the irq names stuff is to restrictive
>> as we want to direct different interrupts to different pins if
>> both are supported!
> 
> [...]
> 
>>> @@ -199,6 +253,22 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
>>>               dev_err(dev, "Failed to set data range: %d\n", ret);
>>>               return ret;
>>>       }
>>> +     /*
>>> +      * 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.
>> This is an interesting comment.  The usual reason for dual interrupt
>> pins is precisely to not map all functions to the same one.  That allows
>> for a saving in querying which interrupt it is by having just the data ready
>> on one pin and just the events on the other...
>>
>> Perhaps the current approach won't support that mode of operation?
>> Clearly we can't merge a binding that enforces them all being the same
>> and then change it later as it'll be incompatible.
>>
>> I'm not quite sure how one should do this sort of stuff in DT though.
>>
>> Rob?
> 
> DT should just describe what is connected which I gather here could be
> either one or both IRQs. We generally distinguish the IRQs with the
> interrupt-names property and then retrieve it as below.
Picking this branch to continue on I'll grab Eva's replay as well.

Eva said:
> I've thought about this before since to me that's the better approach
> than one or the other. I'm in a time crunch before hence I went with
> this way. The input driver does this as well and what I just did is to
> match what it does. If you could point me some drivers for reference,
> I'll gladly analyze those and present something better on the next
> revision.

So taking both of these and having thought about it a bit more in my
current jet lagged state (I hate travelling - particularly with the
added amusement of a flat tyre on the plane).

To my mind we need to describe what interrupts at there as Rob says.
It's all obvious if there is only one interrupt connected (often
the case I suspect as pins are in short supply on many SoCs).

If we allow the binding to specify both pins (using names to do the
matching to which pin they are on the chip), then we could allow
the driver itself to optimize the usage according to what is enabled.
Note though that this can come later - for now we just need to allow
the specification of both interrupts if they are present.

So lets talk about the ideal ;)
Probably makes sense to separate dataready and the events if possible.
Ideal would be to even allow individual events to have there own pins
as long as there are only two available.  So we need a heuristic to
work out what interrupts to put where.  It doesn't work well as a lookup
table (I tried it)

#define ADXL345_OVERRUN = BIT(0)
#define ADXL345_WATERMARK = BIT(1)
#define ADXL345_FREEFALL = BIT(2)
#define ADXL345_INACTIVITY = BIT(3)
#define ADXL345_ACTIVITY = BIT(4)
#define ADXL345_DOUBLE_TAP = BIT(5)
#define ADXL345_SINGLE_TAP = BIT(6)
#define ADXL345_DATA_READY = BIT(7)

So some function that takes the bitmap of what is enabled and
tries to divide it sensibly.

int adxl345_int_heuristic(u8 input, u8 *output)
{
	long bounce;
	switch (hweight8(&input))
	{
	case 0 ... 1:
		*output = input;
		break;
	case 2:
		*output = BIT(ffs(&input)); //this will put one on each interrupt.
		break;
	case 3 ... 7: //now it gets tricky. Perhaps always have dataready and watermark on own interrupt if set?
		
		if (input & (ADXL345_DATA_READY | ADXL345_WATERMARK)) 
			output = input & (ADXL345_DATA_READY | ADXL345_WATERMARK);
		else // taps always on same one etc...
	}
}

Then your interrupt handler will need to look at the incoming and work out if it
needs to read the status register to know what it has. If it doesn't
need to then it doesn't do so.  Be careful to only clear the right
interrupts though in that case as it is always possible both are set.

Anyhow, right now all that needs to be there is the binding to allow two interrupts.
Absolutely fine if for now the driver only uses the first one.

Jonathan
> 
>>> +      */
>>> +     of_irq = of_irq_get_byname(dev->of_node, "INT2");
>>> +     if (of_irq == irq)
>>> +             regval = 0xFF;
>>> +     else
>>> +             regval = 0x00;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[toc] | [prev] | [next] | [standalone]

#1638882 — Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

On Tue, May 02, 2017 at 04:59:12PM +0100, Jonathan Cameron wrote:
> On 02/05/17 04:01, Rob Herring wrote:
> > On Sun, Apr 30, 2017 at 7:32 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> >> On 29/04/17 08:49, Eva Rachel Retuya 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.
> >>>
> >>> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
> >> Coming together nicely, but a few more bits and pieces inline...
> >>
> >> One slight worry is that the irq names stuff is to restrictive
> >> as we want to direct different interrupts to different pins if
> >> both are supported!
> > 
> > [...]
> > 
> >>> @@ -199,6 +253,22 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> >>>               dev_err(dev, "Failed to set data range: %d\n", ret);
> >>>               return ret;
> >>>       }
> >>> +     /*
> >>> +      * 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.
> >> This is an interesting comment.  The usual reason for dual interrupt
> >> pins is precisely to not map all functions to the same one.  That allows
> >> for a saving in querying which interrupt it is by having just the data ready
> >> on one pin and just the events on the other...
> >>
> >> Perhaps the current approach won't support that mode of operation?
> >> Clearly we can't merge a binding that enforces them all being the same
> >> and then change it later as it'll be incompatible.
> >>
> >> I'm not quite sure how one should do this sort of stuff in DT though.
> >>
> >> Rob?
> > 
> > DT should just describe what is connected which I gather here could be
> > either one or both IRQs. We generally distinguish the IRQs with the
> > interrupt-names property and then retrieve it as below.
> Picking this branch to continue on I'll grab Eva's replay as well.
> 
> Eva said:
> > I've thought about this before since to me that's the better approach
> > than one or the other. I'm in a time crunch before hence I went with
> > this way. The input driver does this as well and what I just did is to
> > match what it does. If you could point me some drivers for reference,
> > I'll gladly analyze those and present something better on the next
> > revision.
> 
> So taking both of these and having thought about it a bit more in my
> current jet lagged state (I hate travelling - particularly with the
> added amusement of a flat tyre on the plane).
> 
> To my mind we need to describe what interrupts at there as Rob says.
> It's all obvious if there is only one interrupt connected (often
> the case I suspect as pins are in short supply on many SoCs).
> 
> If we allow the binding to specify both pins (using names to do the
> matching to which pin they are on the chip), then we could allow
> the driver itself to optimize the usage according to what is enabled.
> Note though that this can come later - for now we just need to allow
> the specification of both interrupts if they are present.
> 
> So lets talk about the ideal ;)
> Probably makes sense to separate dataready and the events if possible.
> Ideal would be to even allow individual events to have there own pins
> as long as there are only two available.  So we need a heuristic to
> work out what interrupts to put where.  It doesn't work well as a lookup
> table (I tried it)
> 
> #define ADXL345_OVERRUN = BIT(0)
> #define ADXL345_WATERMARK = BIT(1)
> #define ADXL345_FREEFALL = BIT(2)
> #define ADXL345_INACTIVITY = BIT(3)
> #define ADXL345_ACTIVITY = BIT(4)
> #define ADXL345_DOUBLE_TAP = BIT(5)
> #define ADXL345_SINGLE_TAP = BIT(6)
> #define ADXL345_DATA_READY = BIT(7)
> 
> So some function that takes the bitmap of what is enabled and
> tries to divide it sensibly.
> 
> int adxl345_int_heuristic(u8 input, u8 *output)
> {
> 	long bounce;
> 	switch (hweight8(&input))
> 	{
> 	case 0 ... 1:
> 		*output = input;
> 		break;
> 	case 2:
> 		*output = BIT(ffs(&input)); //this will put one on each interrupt.
> 		break;
> 	case 3 ... 7: //now it gets tricky. Perhaps always have dataready and watermark on own interrupt if set?
> 		
> 		if (input & (ADXL345_DATA_READY | ADXL345_WATERMARK)) 
> 			output = input & (ADXL345_DATA_READY | ADXL345_WATERMARK);
> 		else // taps always on same one etc...
> 	}
> }
> 
> Then your interrupt handler will need to look at the incoming and work out if it
> needs to read the status register to know what it has. If it doesn't
> need to then it doesn't do so.  Be careful to only clear the right
> interrupts though in that case as it is always possible both are set.
> 
> Anyhow, right now all that needs to be there is the binding to allow two interrupts.
> Absolutely fine if for now the driver only uses the first one.
> 
> Jonathan

Thank you for explaining it well. I'll refer to this while working with
the issue.

Eva

> > 
> >>> +      */
> >>> +     of_irq = of_irq_get_byname(dev->of_node, "INT2");
> >>> +     if (of_irq == irq)
> >>> +             regval = 0xFF;
> >>> +     else
> >>> +             regval = 0x00;
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 

[toc] | [prev] | [next] | [standalone]

#1634363 — Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

On Mon, May 01, 2017 at 01:32:00AM +0100, Jonathan Cameron wrote:
[...]
> Coming together nicely, but a few more bits and pieces inline...
> 
> One slight worry is that the irq names stuff is to restrictive
> as we want to direct different interrupts to different pins if
> both are supported!
> 
> Jonathan
[...]
> > +#define ADXL345_IRQ_NAME		"adxl345_event"
> I'd just put this inline.  It doesn't really give any benefit to
> have this defined at the top.

Ack.

> > +
> >  /*
> >   * In full-resolution mode, scale factor is maintained at ~4 mg/LSB
> >   * in all g ranges.
> > @@ -49,6 +56,8 @@
> >  static const int adxl345_uscale = 38300;
> >  
> >  struct adxl345_data {
> > +	struct iio_trigger *data_ready_trig;
> > +	bool data_ready_trig_on;
> >  	struct regmap *regmap;
> >  	struct mutex lock; /* protect this data structure */
> >  	u8 data_range;
> > @@ -158,17 +167,62 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
> >  	return -EINVAL;
> >  }
> >  
[...]
> > -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)
> >  {
> >  	struct adxl345_data *data;
> >  	struct iio_dev *indio_dev;
> >  	u32 regval;
> > +	int of_irq;
> >  	int ret;
> >  
> >  	ret = regmap_read(regmap, ADXL345_REG_DEVID, &regval);
> > @@ -199,6 +253,22 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> >  		dev_err(dev, "Failed to set data range: %d\n", ret);
> >  		return ret;
> >  	}
> > +	/*
> > +	 * 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.
> This is an interesting comment.  The usual reason for dual interrupt
> pins is precisely to not map all functions to the same one.  That allows
> for a saving in querying which interrupt it is by having just the data ready
> on one pin and just the events on the other...
> 
> Perhaps the current approach won't support that mode of operation?
> Clearly we can't merge a binding that enforces them all being the same
> and then change it later as it'll be incompatible.
> 

I've thought about this before since to me that's the better approach
than one or the other. I'm in a time crunch before hence I went with
this way. The input driver does this as well and what I just did is to
match what it does. If you could point me some drivers for reference,
I'll gladly analyze those and present something better on the next
revision.

Thanks,
Eva

> I'm not quite sure how one should do this sort of stuff in DT though.
> 
> Rob?
> > +	 */
> > +	of_irq = of_irq_get_byname(dev->of_node, "INT2");
> > +	if (of_irq == irq)
> > +		regval = 0xFF;
> > +	else
> > +		regval = 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;
> > +	}
> >  
> >  	mutex_init(&data->lock);
> >  

[toc] | [prev] | [next] | [standalone]

#1633627 — 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@gmail.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

[toc] | [prev] | [next] | [standalone]

#1634369 — Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

On Mon, May 01, 2017 at 02:31:00PM +0300, Andy Shevchenko wrote:
[...]
> > -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.

I'm not sure if what you want is more simpler, is it something like what
this driver does?

http://lxr.free-electrons.com/source/drivers/iio/gyro/mpu3050.h#L41
http://lxr.free-electrons.com/source/drivers/iio/gyro/mpu3050-i2c.c#L34

> 
> >  #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?
> 

I'm uninformed and still learning. Please let me know how to approach
this in some other way.

> > +static const struct iio_trigger_ops adxl345_trigger_ops = {
> 
> > +       .owner = THIS_MODULE,
> 
> Do we still need this kind of lines?
> 

I'm not sure either.
Jonathan, is it OK to omit this and also the one below?

> > +       .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; ?
> 

OK.

Thanks,
Eva

> > +
> > +       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

[toc] | [prev] | [next] | [standalone]

#1634626 — Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

On Tue, May 2, 2017 at 3:15 PM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
> On Mon, May 01, 2017 at 02:31:00PM +0300, Andy Shevchenko wrote:
> [...]
>> > -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.
>
> I'm not sure if what you want is more simpler, is it something like what
> this driver does?

Nope. The driver you were referring to does the same you did.

I'm proposing the above struct to be introduced along with changing
prototype like:

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

In next patch adding interrupt would not touch prototypes at all!

>
> http://lxr.free-electrons.com/source/drivers/iio/gyro/mpu3050.h#L41
> http://lxr.free-electrons.com/source/drivers/iio/gyro/mpu3050-i2c.c#L34

>> > +#include <linux/of_irq.h>
>>
>> Can we get rid of gnostic resource providers?
>>
>
> I'm uninformed and still learning. Please let me know how to approach
> this in some other way.

I suppose something like platform_get_irq(); to use.
But it would be nice to you to investigate more.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1638812 — Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

On Wed, May 03, 2017 at 12:05:00AM +0300, Andy Shevchenko wrote:
> On Tue, May 2, 2017 at 3:15 PM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
> > On Mon, May 01, 2017 at 02:31:00PM +0300, Andy Shevchenko wrote:
> > [...]
> >> > -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.
> >
> > I'm not sure if what you want is more simpler, is it something like what
> > this driver does?
> 
> Nope. The driver you were referring to does the same you did.
> 
> I'm proposing the above struct to be introduced along with changing
> prototype like:
> 
>  -int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> const char *name);
>  +int adxl345_core_probe(struct adxl345_chip *chip);
> 
> In next patch adding interrupt would not touch prototypes at all!
> 

OK, got it. Thanks for clarifying.

> >
> > http://lxr.free-electrons.com/source/drivers/iio/gyro/mpu3050.h#L41
> > http://lxr.free-electrons.com/source/drivers/iio/gyro/mpu3050-i2c.c#L34
> 
> >> > +#include <linux/of_irq.h>
> >>
> >> Can we get rid of gnostic resource providers?
> >>
> >
> > I'm uninformed and still learning. Please let me know how to approach
> > this in some other way.
> 
> I suppose something like platform_get_irq(); to use.
> But it would be nice to you to investigate more.

I had a look and it seems I have to convert to platform_driver in order
to make use of that function. Is this correct?

Eva

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [next] | [standalone]

#1636551 — Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

On 02/05/17 13:15, Eva Rachel Retuya wrote:
> On Mon, May 01, 2017 at 02:31:00PM +0300, Andy Shevchenko wrote:
> [...]
>>> -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.
> 
> I'm not sure if what you want is more simpler, is it something like what
> this driver does?
> 
> http://lxr.free-electrons.com/source/drivers/iio/gyro/mpu3050.h#L41
> http://lxr.free-electrons.com/source/drivers/iio/gyro/mpu3050-i2c.c#L34
> 
>>
>>>  #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?
>>
> 
> I'm uninformed and still learning. Please let me know how to approach
> this in some other way.
> 
>>> +static const struct iio_trigger_ops adxl345_trigger_ops = {
>>
>>> +       .owner = THIS_MODULE,
>>
>> Do we still need this kind of lines?
>>
> 
> I'm not sure either.
> Jonathan, is it OK to omit this and also the one below?
No it's not.  There are ways avoiding the necessity of specifying this
via some macro magic.  It could be done easily enough but hasn't been
yet.

> 
>>> +       .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.
Same issue.  Could be fixed, but right now you need them.

Patches welcome ;)  Basic eventual aim would be to drop
these fields from the structures entirely but obviously
there would have to be some intermediate steps.>
>>>         .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; ?
>>
> 
> OK.
> 
> Thanks,
> Eva
> 
>>> +
>>> +       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

[toc] | [prev] | [next] | [standalone]

#1638822 — Re: [PATCH v2 3/4] iio: accel: adxl345: Setup DATA_READY trigger

On Fri, May 05, 2017 at 07:26:06PM +0100, Jonathan Cameron wrote:
> On 02/05/17 13:15, Eva Rachel Retuya wrote:
> > On Mon, May 01, 2017 at 02:31:00PM +0300, Andy Shevchenko wrote:
> > [...]
> >>> -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.
> > 
> > I'm not sure if what you want is more simpler, is it something like what
> > this driver does?
> > 
> > http://lxr.free-electrons.com/source/drivers/iio/gyro/mpu3050.h#L41
> > http://lxr.free-electrons.com/source/drivers/iio/gyro/mpu3050-i2c.c#L34
> > 
> >>
> >>>  #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?
> >>
> > 
> > I'm uninformed and still learning. Please let me know how to approach
> > this in some other way.
> > 
> >>> +static const struct iio_trigger_ops adxl345_trigger_ops = {
> >>
> >>> +       .owner = THIS_MODULE,
> >>
> >> Do we still need this kind of lines?
> >>
> > 
> > I'm not sure either.
> > Jonathan, is it OK to omit this and also the one below?
> No it's not.  There are ways avoiding the necessity of specifying this
> via some macro magic.  It could be done easily enough but hasn't been
> yet.
> 
> > 
> >>> +       .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.
> Same issue.  Could be fixed, but right now you need them.

Noted, I will leave them as-is.

> 
> Patches welcome ;)  Basic eventual aim would be to drop
> these fields from the structures entirely but obviously
> there would have to be some intermediate steps.>

I'll suggest this as a coding task for Outreachy.

Thanks,
Eva

> >>>         .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; ?
> >>
> > 
> > OK.
> > 
> > Thanks,
> > Eva
> > 
> >>> +
> >>> +       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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web