[RFC PATCH 1/4] dt-bindings: iio: accel: fxls8962af: add interrupt options

[RFC PATCH 1/4] dt-bindings: iio: accel: fxls8962af: add interrupt options

[Sean Nyekjaer]

This in done for supporting hw buffered sampling

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
This series depends on "iio: accel: add support for
FXLS8962AF/FXLS8964AF accelerometers"

 .../bindings/iio/accel/nxp,fxls8962af.yaml           | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml b/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
index c7be7a1679ab..e0e5542377df 100644
--- a/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
@@ -32,6 +32,16 @@ properties:
   interrupts:
     maxItems: 1
 
+  interrupt-names:
+    maxItems: 1
+    items:
+      enum:
+        - INT1
+        - INT2
+
+  drive-open-drain:
+    type: boolean
+
 required:
   - compatible
   - reg
@@ -51,6 +61,7 @@ examples:
             reg = <0x62>;
             interrupt-parent = <&gpio0>;
             interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "INT1";
         };
     };
   - |
@@ -66,5 +77,6 @@ examples:
             spi-max-frequency = <4000000>;
             interrupt-parent = <&gpio0>;
             interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "INT1";
         };
     };
-- 
2.31.0

[RFC PATCH 2/4] iio: accel: fxls8962af: add interrupt support

[Sean Nyekjaer]

Preparation commit for the next that adds hw buffered sampling

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
This series depends on "iio: accel: add support for
FXLS8962AF/FXLS8964AF accelerometers"

 drivers/iio/accel/fxls8962af-core.c | 116 ++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

diff --git a/drivers/iio/accel/fxls8962af-core.c b/drivers/iio/accel/fxls8962af-core.c
index b47d81bebf43..848f3d68f5d4 100644
--- a/drivers/iio/accel/fxls8962af-core.c
+++ b/drivers/iio/accel/fxls8962af-core.c
@@ -15,6 +15,7 @@
 #include <linux/bits.h>
 #include <linux/bitfield.h>
 #include <linux/module.h>
+#include <linux/of_irq.h>
 #include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 #include <linux/regmap.h>
@@ -54,6 +55,10 @@
 #define FXLS8962AF_SC3_WAKE_ODR_PREP(x)		FIELD_PREP(FXLS8962AF_SC3_WAKE_ODR_MASK, x)
 #define FXLS8962AF_SC3_WAKE_ODR_GET(x)		FIELD_GET(FXLS8962AF_SC3_WAKE_ODR_MASK, x)
 #define FXLS8962AF_SENS_CONFIG4			0x18
+#define FXLS8962AF_SC4_INT_PP_OD_MASK		BIT(1)
+#define FXLS8962AF_SC4_INT_PP_OD_PREP(x)	FIELD_PREP(FXLS8962AF_SC4_INT_PP_OD_MASK, x)
+#define FXLS8962AF_SC4_INT_POL_MASK		BIT(0)
+#define FXLS8962AF_SC4_INT_POL_PREP(x)		FIELD_PREP(FXLS8962AF_SC4_INT_POL_MASK, x)
 #define FXLS8962AF_SENS_CONFIG5			0x19
 
 #define FXLS8962AF_WAKE_IDLE_LSB		0x1b
@@ -62,6 +67,9 @@
 
 #define FXLS8962AF_INT_EN			0x20
 #define FXLS8962AF_INT_PIN_SEL			0x21
+#define FXLS8962AF_INT_PIN_SEL_MASK		GENMASK(7, 0)
+#define FXLS8962AF_INT_PIN_SEL_INT1		0x00
+#define FXLS8962AF_INT_PIN_SEL_INT2		GENMASK(7, 0)
 
 #define FXLS8962AF_OFF_X			0x22
 #define FXLS8962AF_OFF_Y			0x23
@@ -142,6 +150,11 @@ enum {
 	fxls8962af_idx_ts,
 };
 
+enum fxls8962af_int_pin {
+	FXLS8962AF_PIN_INT1,
+	FXLS8962AF_PIN_INT2,
+};
+
 static int fxls8962af_drdy(struct fxls8962af_data *data)
 {
 	struct device *dev = regmap_get_device(data->regmap);
@@ -559,6 +572,20 @@ static int fxls8962af_reset(struct fxls8962af_data *data)
 	return ret;
 }
 
+static irqreturn_t fxls8962af_interrupt(int irq, void *p)
+{
+	struct iio_dev *indio_dev = p;
+	struct fxls8962af_data *data = iio_priv(indio_dev);
+	unsigned int reg;
+	int ret;
+
+	ret = regmap_read(data->regmap, FXLS8962AF_INT_STATUS, &reg);
+	if (ret < 0)
+		return IRQ_NONE;
+
+	return IRQ_NONE;
+}
+
 static void fxls8962af_regulator_disable(void *data_ptr)
 {
 	struct fxls8962af_data *data = data_ptr;
@@ -578,6 +605,89 @@ static void fxls8962af_pm_disable(void *dev_ptr)
 	fxls8962af_standby(iio_priv(indio_dev));
 }
 
+static void fxls8962af_get_irq(struct device_node *of_node, enum fxls8962af_int_pin *pin)
+{
+	int irq;
+
+	irq = of_irq_get_byname(of_node, "INT2");
+	if (irq > 0) {
+		*pin = FXLS8962AF_PIN_INT2;
+		return;
+	}
+
+	*pin = FXLS8962AF_PIN_INT1;
+}
+
+static int fxls8962af_irq_setup(struct iio_dev *indio_dev, int irq)
+{
+	struct fxls8962af_data *data = iio_priv(indio_dev);
+	struct device *dev = regmap_get_device(data->regmap);
+	unsigned long irq_type;
+	bool irq_active_high;
+	enum fxls8962af_int_pin int_pin;
+	u8 int_pin_sel;
+	int ret;
+
+	fxls8962af_get_irq(dev->of_node, &int_pin);
+	switch (int_pin) {
+	case FXLS8962AF_PIN_INT1:
+		int_pin_sel = FXLS8962AF_INT_PIN_SEL_INT1;
+		break;
+	case FXLS8962AF_PIN_INT2:
+		int_pin_sel = FXLS8962AF_INT_PIN_SEL_INT2;
+		break;
+	default:
+		dev_err(dev, "unsupported int pin selected\n");
+		return -EINVAL;
+	}
+
+	ret = regmap_update_bits(data->regmap, FXLS8962AF_INT_PIN_SEL,
+				 FXLS8962AF_INT_PIN_SEL_MASK,
+				 int_pin_sel);
+	if (ret)
+		return ret;
+
+	irq_type = irqd_get_trigger_type(irq_get_irq_data(irq));
+
+	switch (irq_type) {
+	case IRQF_TRIGGER_HIGH:
+	case IRQF_TRIGGER_RISING:
+		irq_active_high = true;
+		break;
+	case IRQF_TRIGGER_LOW:
+	case IRQF_TRIGGER_FALLING:
+		irq_active_high = false;
+		break;
+	default:
+		dev_info(dev, "mode %lx unsupported\n", irq_type);
+		return -EINVAL;
+	}
+
+	ret = regmap_update_bits(data->regmap, FXLS8962AF_SENS_CONFIG4,
+				 FXLS8962AF_SC4_INT_POL_MASK,
+				 FXLS8962AF_SC4_INT_POL_PREP(irq_active_high));
+	if (ret < 0)
+		return ret;
+
+	if (device_property_read_bool(dev, "drive-open-drain")) {
+		ret = regmap_update_bits(data->regmap, FXLS8962AF_SENS_CONFIG4,
+					 FXLS8962AF_SC4_INT_PP_OD_MASK,
+					 FXLS8962AF_SC4_INT_PP_OD_PREP(1));
+		if (ret < 0)
+			return ret;
+
+		irq_type |= IRQF_SHARED;
+	}
+
+	ret = devm_request_threaded_irq(dev,
+					irq,
+					NULL, fxls8962af_interrupt,
+					irq_type | IRQF_ONESHOT,
+					indio_dev->name, indio_dev);
+
+	return ret;
+}
+
 int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq)
 {
 	struct fxls8962af_data *data;
@@ -637,6 +747,12 @@ int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq)
 	if (ret < 0)
 		return ret;
 
+	if (irq) {
+		ret = fxls8962af_irq_setup(indio_dev, irq);
+		if (ret < 0)
+			return ret;
+	}
+
 	ret = pm_runtime_set_active(dev);
 	if (ret < 0)
 		return ret;
-- 
2.31.0

Re: [RFC PATCH 2/4] iio: accel: fxls8962af: add interrupt support

[Jonathan Cameron]

On Wed, 28 Apr 2021 10:22:01 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> Preparation commit for the next that adds hw buffered sampling
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>

Entirely trivial comment inline.  Otherwise looks good.

> ---
> This series depends on "iio: accel: add support for
> FXLS8962AF/FXLS8964AF accelerometers"
> 
>  drivers/iio/accel/fxls8962af-core.c | 116 ++++++++++++++++++++++++++++
>  1 file changed, 116 insertions(+)
> 
> diff --git a/drivers/iio/accel/fxls8962af-core.c b/drivers/iio/accel/fxls8962af-core.c
> index b47d81bebf43..848f3d68f5d4 100644
> --- a/drivers/iio/accel/fxls8962af-core.c
> +++ b/drivers/iio/accel/fxls8962af-core.c
> @@ -15,6 +15,7 @@
>  #include <linux/bits.h>
>  #include <linux/bitfield.h>
>  #include <linux/module.h>
> +#include <linux/of_irq.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/regmap.h>
> @@ -54,6 +55,10 @@
>  #define FXLS8962AF_SC3_WAKE_ODR_PREP(x)		FIELD_PREP(FXLS8962AF_SC3_WAKE_ODR_MASK, x)
>  #define FXLS8962AF_SC3_WAKE_ODR_GET(x)		FIELD_GET(FXLS8962AF_SC3_WAKE_ODR_MASK, x)
>  #define FXLS8962AF_SENS_CONFIG4			0x18
> +#define FXLS8962AF_SC4_INT_PP_OD_MASK		BIT(1)
> +#define FXLS8962AF_SC4_INT_PP_OD_PREP(x)	FIELD_PREP(FXLS8962AF_SC4_INT_PP_OD_MASK, x)
> +#define FXLS8962AF_SC4_INT_POL_MASK		BIT(0)
> +#define FXLS8962AF_SC4_INT_POL_PREP(x)		FIELD_PREP(FXLS8962AF_SC4_INT_POL_MASK, x)
>  #define FXLS8962AF_SENS_CONFIG5			0x19
>  
>  #define FXLS8962AF_WAKE_IDLE_LSB		0x1b
> @@ -62,6 +67,9 @@
>  
>  #define FXLS8962AF_INT_EN			0x20
>  #define FXLS8962AF_INT_PIN_SEL			0x21
> +#define FXLS8962AF_INT_PIN_SEL_MASK		GENMASK(7, 0)
> +#define FXLS8962AF_INT_PIN_SEL_INT1		0x00
> +#define FXLS8962AF_INT_PIN_SEL_INT2		GENMASK(7, 0)
>  
>  #define FXLS8962AF_OFF_X			0x22
>  #define FXLS8962AF_OFF_Y			0x23
> @@ -142,6 +150,11 @@ enum {
>  	fxls8962af_idx_ts,
>  };
>  
> +enum fxls8962af_int_pin {
> +	FXLS8962AF_PIN_INT1,
> +	FXLS8962AF_PIN_INT2,
> +};
> +
>  static int fxls8962af_drdy(struct fxls8962af_data *data)
>  {
>  	struct device *dev = regmap_get_device(data->regmap);
> @@ -559,6 +572,20 @@ static int fxls8962af_reset(struct fxls8962af_data *data)
>  	return ret;
>  }
>  
> +static irqreturn_t fxls8962af_interrupt(int irq, void *p)
> +{
> +	struct iio_dev *indio_dev = p;
> +	struct fxls8962af_data *data = iio_priv(indio_dev);
> +	unsigned int reg;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, FXLS8962AF_INT_STATUS, &reg);
> +	if (ret < 0)
> +		return IRQ_NONE;
> +
> +	return IRQ_NONE;
> +}
> +
>  static void fxls8962af_regulator_disable(void *data_ptr)
>  {
>  	struct fxls8962af_data *data = data_ptr;
> @@ -578,6 +605,89 @@ static void fxls8962af_pm_disable(void *dev_ptr)
>  	fxls8962af_standby(iio_priv(indio_dev));
>  }
>  
> +static void fxls8962af_get_irq(struct device_node *of_node, enum fxls8962af_int_pin *pin)
> +{
> +	int irq;
> +
> +	irq = of_irq_get_byname(of_node, "INT2");
> +	if (irq > 0) {
> +		*pin = FXLS8962AF_PIN_INT2;
> +		return;
> +	}
> +
> +	*pin = FXLS8962AF_PIN_INT1;
> +}
> +
> +static int fxls8962af_irq_setup(struct iio_dev *indio_dev, int irq)
> +{
> +	struct fxls8962af_data *data = iio_priv(indio_dev);
> +	struct device *dev = regmap_get_device(data->regmap);
> +	unsigned long irq_type;
> +	bool irq_active_high;
> +	enum fxls8962af_int_pin int_pin;
> +	u8 int_pin_sel;
> +	int ret;
> +
> +	fxls8962af_get_irq(dev->of_node, &int_pin);
> +	switch (int_pin) {
> +	case FXLS8962AF_PIN_INT1:
> +		int_pin_sel = FXLS8962AF_INT_PIN_SEL_INT1;
> +		break;
> +	case FXLS8962AF_PIN_INT2:
> +		int_pin_sel = FXLS8962AF_INT_PIN_SEL_INT2;
> +		break;
> +	default:
> +		dev_err(dev, "unsupported int pin selected\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_update_bits(data->regmap, FXLS8962AF_INT_PIN_SEL,
> +				 FXLS8962AF_INT_PIN_SEL_MASK,
> +				 int_pin_sel);
> +	if (ret)
> +		return ret;
> +
> +	irq_type = irqd_get_trigger_type(irq_get_irq_data(irq));
> +
> +	switch (irq_type) {
> +	case IRQF_TRIGGER_HIGH:
> +	case IRQF_TRIGGER_RISING:
> +		irq_active_high = true;
> +		break;
> +	case IRQF_TRIGGER_LOW:
> +	case IRQF_TRIGGER_FALLING:
> +		irq_active_high = false;
> +		break;
> +	default:
> +		dev_info(dev, "mode %lx unsupported\n", irq_type);
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_update_bits(data->regmap, FXLS8962AF_SENS_CONFIG4,
> +				 FXLS8962AF_SC4_INT_POL_MASK,
> +				 FXLS8962AF_SC4_INT_POL_PREP(irq_active_high));
> +	if (ret < 0)
> +		return ret;
> +
> +	if (device_property_read_bool(dev, "drive-open-drain")) {
> +		ret = regmap_update_bits(data->regmap, FXLS8962AF_SENS_CONFIG4,
> +					 FXLS8962AF_SC4_INT_PP_OD_MASK,
> +					 FXLS8962AF_SC4_INT_PP_OD_PREP(1));
> +		if (ret < 0)
> +			return ret;
> +
> +		irq_type |= IRQF_SHARED;
> +	}
> +
> +	ret = devm_request_threaded_irq(dev,
> +					irq,
> +					NULL, fxls8962af_interrupt,
> +					irq_type | IRQF_ONESHOT,
> +					indio_dev->name, indio_dev);
> +
> +	return ret;

Combine these last two lines into 
return devm_request_threaded_irq(...)


> +}
> +
>  int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq)
>  {
>  	struct fxls8962af_data *data;
> @@ -637,6 +747,12 @@ int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq)
>  	if (ret < 0)
>  		return ret;
>  
> +	if (irq) {
> +		ret = fxls8962af_irq_setup(indio_dev, irq);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	ret = pm_runtime_set_active(dev);
>  	if (ret < 0)
>  		return ret;

Re: [RFC PATCH 2/4] iio: accel: fxls8962af: add interrupt support

[Lars-Peter Clausen]

On 4/28/21 10:22 AM, Sean Nyekjaer wrote:
> Preparation commit for the next that adds hw buffered sampling
>
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
> [...]
>   
> +static void fxls8962af_get_irq(struct device_node *of_node, enum fxls8962af_int_pin *pin)
> +{
> +	int irq;
> +
> +	irq = of_irq_get_byname(of_node, "INT2");

For this I'd use device_property_match_string(dev, "interrupt-names", 
"INT2"). Means it won't try to map the interrupt again, and also this is 
the only place where the driver directly depends on OF, everything else 
already uses the device_ API.


> +	if (irq > 0) {
> +		*pin = FXLS8962AF_PIN_INT2;
> +		return;
> +	}
> +
> +	*pin = FXLS8962AF_PIN_INT1;
> +}

Re: [RFC PATCH 2/4] iio: accel: fxls8962af: add interrupt support

[Andy Shevchenko]

On Thu, Apr 29, 2021 at 11:58 AM Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 4/28/21 10:22 AM, Sean Nyekjaer wrote:
> > Preparation commit for the next that adds hw buffered sampling

...

> > +     irq = of_irq_get_byname(of_node, "INT2");
>
> For this I'd use device_property_match_string(dev, "interrupt-names",
> "INT2"). Means it won't try to map the interrupt again, and also this is
> the only place where the driver directly depends on OF, everything else
> already uses the device_ API.

Why not platform_get_irq_byname_optional() ?

> > +     if (irq > 0) {
> > +             *pin = FXLS8962AF_PIN_INT2;
> > +             return;
> > +     }

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 2/4] iio: accel: fxls8962af: add interrupt support

[Lars-Peter Clausen]

On 4/29/21 11:35 AM, Andy Shevchenko wrote:
> On Thu, Apr 29, 2021 at 11:58 AM Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 4/28/21 10:22 AM, Sean Nyekjaer wrote:
>>> Preparation commit for the next that adds hw buffered sampling
> ...
>
>>> +     irq = of_irq_get_byname(of_node, "INT2");
>> For this I'd use device_property_match_string(dev, "interrupt-names",
>> "INT2"). Means it won't try to map the interrupt again, and also this is
>> the only place where the driver directly depends on OF, everything else
>> already uses the device_ API.
> Why not platform_get_irq_byname_optional() ?
Because it is not a platform device :)

Re: [RFC PATCH 2/4] iio: accel: fxls8962af: add interrupt support

[Andy Shevchenko]

On Thu, Apr 29, 2021 at 12:37 PM Lars-Peter Clausen <lars@metafoo.de> wrote:
>
> On 4/29/21 11:35 AM, Andy Shevchenko wrote:
> > On Thu, Apr 29, 2021 at 11:58 AM Lars-Peter Clausen <lars@metafoo.de> wrote:
> >> On 4/28/21 10:22 AM, Sean Nyekjaer wrote:
> >>> Preparation commit for the next that adds hw buffered sampling
> > ...
> >
> >>> +     irq = of_irq_get_byname(of_node, "INT2");
> >> For this I'd use device_property_match_string(dev, "interrupt-names",
> >> "INT2"). Means it won't try to map the interrupt again, and also this is
> >> the only place where the driver directly depends on OF, everything else
> >> already uses the device_ API.
> > Why not platform_get_irq_byname_optional() ?
> Because it is not a platform device :)

Then device_property reading like this isn't really needed.

What is missed is fwnode_irq_get_by_name() API which will do the right
things on any resource provider.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 2/4] iio: accel: fxls8962af: add interrupt support

[Lars-Peter Clausen]

On 4/29/21 1:35 PM, Andy Shevchenko wrote:
> On Thu, Apr 29, 2021 at 12:37 PM Lars-Peter Clausen <lars@metafoo.de> wrote:
>> On 4/29/21 11:35 AM, Andy Shevchenko wrote:
>>> On Thu, Apr 29, 2021 at 11:58 AM Lars-Peter Clausen <lars@metafoo.de> wrote:
>>>> On 4/28/21 10:22 AM, Sean Nyekjaer wrote:
>>>>> Preparation commit for the next that adds hw buffered sampling
>>> ...
>>>
>>>>> +     irq = of_irq_get_byname(of_node, "INT2");
>>>> For this I'd use device_property_match_string(dev, "interrupt-names",
>>>> "INT2"). Means it won't try to map the interrupt again, and also this is
>>>> the only place where the driver directly depends on OF, everything else
>>>> already uses the device_ API.
>>> Why not platform_get_irq_byname_optional() ?
>> Because it is not a platform device :)
> Then device_property reading like this isn't really needed.
Why?

Re: [RFC PATCH 2/4] iio: accel: fxls8962af: add interrupt support

[Andy Shevchenko]

On Thu, Apr 29, 2021 at 10:19 PM Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 4/29/21 1:35 PM, Andy Shevchenko wrote:
> > On Thu, Apr 29, 2021 at 12:37 PM Lars-Peter Clausen <lars@metafoo.de> wrote:
> >> On 4/29/21 11:35 AM, Andy Shevchenko wrote:
> >>> On Thu, Apr 29, 2021 at 11:58 AM Lars-Peter Clausen <lars@metafoo.de> wrote:
> >>>> On 4/28/21 10:22 AM, Sean Nyekjaer wrote:
> >>>>> Preparation commit for the next that adds hw buffered sampling
> >>> ...
> >>>
> >>>>> +     irq = of_irq_get_byname(of_node, "INT2");
> >>>> For this I'd use device_property_match_string(dev, "interrupt-names",
> >>>> "INT2"). Means it won't try to map the interrupt again, and also this is
> >>>> the only place where the driver directly depends on OF, everything else
> >>>> already uses the device_ API.
> >>> Why not platform_get_irq_byname_optional() ?
> >> Because it is not a platform device :)
> > Then device_property reading like this isn't really needed.
> Why?

Because it doesn't bring any value in this case and actually makes
readers confused. ACPI doesn't have properties started with # (they
are special for OF and hiding it behind device property API is not
correct).

So, either leave it OF, or introduce the above API (or use existing
fwnode_get_irq() by index).

-- 
With Best Regards,
Andy Shevchenko

[RFC PATCH 3/4] iio: accel: fxls8962af: add hw buffered sampling

[Sean Nyekjaer]

When buffered sampling is enabled, the accelerometer will dump data into
the internal fifo and interrupt at watermark. Then the driver flushes
all data to the iio buffer.
As the accelerometer doesn't have internal timestamps, they are aproximated
between to interrupts.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
This series depends on "iio: accel: add support for
FXLS8962AF/FXLS8964AF accelerometers"

Any good pratice in general to how much to fill the fifo?
The accelerometer a internal buffer with 32 samples and I have set the watermark to 16.


 drivers/iio/accel/fxls8962af-core.c | 215 +++++++++++++++++++++++++++-
 1 file changed, 214 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/accel/fxls8962af-core.c b/drivers/iio/accel/fxls8962af-core.c
index 848f3d68f5d4..2bd5c6d76b63 100644
--- a/drivers/iio/accel/fxls8962af-core.c
+++ b/drivers/iio/accel/fxls8962af-core.c
@@ -20,13 +20,17 @@
 #include <linux/regulator/consumer.h>
 #include <linux/regmap.h>
 
+#include <linux/iio/buffer.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/kfifo_buf.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
 
 #include "fxls8962af.h"
 
 #define FXLS8962AF_INT_STATUS			0x00
 #define FXLS8962AF_INT_STATUS_SRC_BOOT		BIT(0)
+#define FXLS8962AF_INT_STATUS_SRC_BUF		BIT(5)
 #define FXLS8962AF_INT_STATUS_SRC_DRDY		BIT(7)
 #define FXLS8962AF_TEMP_OUT			0x01
 #define FXLS8962AF_VECM_LSB			0x02
@@ -34,6 +38,9 @@
 #define FXLS8962AF_OUT_Y_LSB			0x06
 #define FXLS8962AF_OUT_Z_LSB			0x08
 #define FXLS8962AF_BUF_STATUS			0x0b
+#define FXLS8962AF_BUF_STATUS_BUF_CNT		GENMASK(5, 0)
+#define FXLS8962AF_BUF_STATUS_BUF_OVF		BIT(6)
+#define FXLS8962AF_BUF_STATUS_BUF_WMRK		BIT(7)
 #define FXLS8962AF_BUF_X_LSB			0x0c
 #define FXLS8962AF_BUF_Y_LSB			0x0e
 #define FXLS8962AF_BUF_Z_LSB			0x10
@@ -66,6 +73,7 @@
 #define FXLS8962AF_ASLP_COUNT_LSB		0x1e
 
 #define FXLS8962AF_INT_EN			0x20
+#define FXLS8962AF_INT_EN_BUF_EN		BIT(6)
 #define FXLS8962AF_INT_PIN_SEL			0x21
 #define FXLS8962AF_INT_PIN_SEL_MASK		GENMASK(7, 0)
 #define FXLS8962AF_INT_PIN_SEL_INT1		0x00
@@ -76,7 +84,10 @@
 #define FXLS8962AF_OFF_Z			0x24
 
 #define FXLS8962AF_BUF_CONFIG1			0x26
+#define FXLS8962AF_BC1_BUF_MODE_MASK		GENMASK(6, 5)
+#define FXLS8962AF_BC1_BUF_MODE_PREP(x)		FIELD_PREP(FXLS8962AF_BC1_BUF_MODE_MASK, x)
 #define FXLS8962AF_BUF_CONFIG2			0x27
+#define FXLS8962AF_BUF_CONFIG2_BUF_WMRK		GENMASK(5, 0)
 
 #define FXLS8962AF_ORIENT_STATUS		0x28
 #define FXLS8962AF_ORIENT_CONFIG		0x29
@@ -106,6 +117,7 @@
 
 #define FXLS8962AF_AUTO_SUSPEND_DELAY_MS	2000
 
+#define FXLS8962AF_FIFO_LENGTH			32
 #define FXLS8962AF_SCALE_TABLE_LEN		4
 #define FXLS8962AF_SAMP_FREQ_TABLE_LEN		13
 
@@ -133,6 +145,11 @@ struct fxls8962af_data {
 	struct regmap *regmap;
 	const struct fxls8962af_chip_info *chip_info;
 	struct regulator *vdd_reg;
+	struct {
+		__le16 channels[3];
+		s64 ts __aligned(8);
+	} scan;
+	int64_t timestamp, old_timestamp; /* Only used in hw fifo mode. */
 	struct iio_mount_matrix orientation;
 };
 
@@ -433,7 +450,10 @@ static int fxls8962af_read_raw(struct iio_dev *indio_dev,
 			ret = fxls8962af_get_temp(data, val);
 			break;
 		case IIO_ACCEL:
-			ret = fxls8962af_get_axis(data, chan, val);
+			if (iio_buffer_enabled(indio_dev))
+				ret = -EBUSY;
+			else
+				ret = fxls8962af_get_axis(data, chan, val);
 			break;
 		default:
 			ret = -EINVAL;
@@ -572,6 +592,174 @@ static int fxls8962af_reset(struct fxls8962af_data *data)
 	return ret;
 }
 
+static int fxls8962af_fifo_set_mode(struct fxls8962af_data *data, bool onoff)
+{
+	int ret;
+
+	ret = regmap_update_bits(data->regmap, FXLS8962AF_BUF_CONFIG1,
+				 FXLS8962AF_BC1_BUF_MODE_MASK,
+				 FXLS8962AF_BC1_BUF_MODE_PREP(onoff));
+
+	return ret;
+}
+
+
+static int fxls8962af_buffer_preenable(struct iio_dev *indio_dev)
+{
+	struct fxls8962af_data *data = iio_priv(indio_dev);
+
+	return fxls8962af_power_on(data);
+}
+
+static int fxls8962af_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct fxls8962af_data *data = iio_priv(indio_dev);
+	int ret = 0;
+
+	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
+		return 0;
+
+	fxls8962af_standby(data);
+
+	/* Enable buffer interrupt*/
+	ret = regmap_update_bits(data->regmap, FXLS8962AF_INT_EN,
+			FXLS8962AF_INT_EN_BUF_EN,
+			FXLS8962AF_INT_EN_BUF_EN);
+	if (ret < 0)
+		return ret;
+
+	/* Enable watermark at max fifo size*/
+	ret = regmap_update_bits(data->regmap, FXLS8962AF_BUF_CONFIG2,
+			FXLS8962AF_BUF_CONFIG2_BUF_WMRK,
+			FXLS8962AF_BUF_CONFIG2_BUF_WMRK / 2);
+	if (ret < 0)
+		return ret;
+
+	ret = fxls8962af_fifo_set_mode(data, true);
+
+	fxls8962af_active(data);
+
+	return ret;
+}
+
+static int fxls8962af_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct fxls8962af_data *data = iio_priv(indio_dev);
+	int ret = 0;
+
+	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
+		return 0;
+
+	fxls8962af_standby(data);
+
+	/* Disable buffer interrupt*/
+	ret = regmap_update_bits(data->regmap, FXLS8962AF_INT_EN,
+			FXLS8962AF_INT_EN_BUF_EN, 0);
+	if (ret < 0)
+		return ret;
+
+	fxls8962af_fifo_set_mode(data, false);
+
+	fxls8962af_active(data);
+
+	return 0;
+}
+
+static int fxls8962af_buffer_postdisable(struct iio_dev *indio_dev)
+{
+	struct fxls8962af_data *data = iio_priv(indio_dev);
+
+	return fxls8962af_power_off(data);
+}
+
+static const struct iio_buffer_setup_ops fxls8962af_buffer_ops = {
+	.preenable = fxls8962af_buffer_preenable,
+	.postenable = fxls8962af_buffer_postenable,
+	.predisable = fxls8962af_buffer_predisable,
+	.postdisable = fxls8962af_buffer_postdisable,
+};
+
+static int fxls8962af_fifo_transfer(struct fxls8962af_data *data,
+				    char *buffer, int samples)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	int sample_length = 3 * 2;
+	int ret;
+	int total_length = samples * sample_length;
+
+	ret = regmap_raw_read(data->regmap, FXLS8962AF_BUF_X_LSB, buffer,
+			      total_length);
+	if (ret < 0)
+		dev_err(dev, "Error transferring data from fifo: %d\n", ret);
+
+	return ret;
+}
+
+static int fxls8962af_fifo_flush(struct iio_dev *indio_dev)
+{
+	struct fxls8962af_data *data = iio_priv(indio_dev);
+	struct device *dev = regmap_get_device(data->regmap);
+	u16 buffer[FXLS8962AF_FIFO_LENGTH * 3];
+	uint64_t sample_period;
+	unsigned int reg;
+	int64_t tstamp;
+	int ret, i;
+	u8 count;
+
+	ret = regmap_read(data->regmap, FXLS8962AF_BUF_STATUS, &reg);
+	if (ret < 0)
+		return ret;
+
+	if (reg & FXLS8962AF_BUF_STATUS_BUF_OVF) {
+		dev_err(dev, "Buffer overflown");
+		return -1;
+	}
+
+	count = reg & FXLS8962AF_BUF_STATUS_BUF_CNT;
+
+	if (!count)
+		return 0;
+
+	data->old_timestamp = data->timestamp;
+	data->timestamp = iio_get_time_ns(indio_dev);
+
+	/* Approximate timestamps for each of the sample based on the sampling,
+	 * frequency, timestamp for last sample and number of samples.
+	 */
+	sample_period = (data->timestamp - data->old_timestamp);
+	do_div(sample_period, count);
+	tstamp = data->timestamp - (count - 1) * sample_period;
+
+
+	ret = fxls8962af_fifo_transfer(data, (u8 *)buffer, count);
+	if (ret)
+		return ret;
+
+	/*
+	 * Ideally we want the IIO core to handle the demux when running in fifo
+	 * mode but not when running in triggered buffer mode. Unfortunately
+	 * this does not seem to be possible, so stick with driver demux for
+	 * now.
+	 */
+	for (i = 0; i < count; i++) {
+		int j, bit;
+
+		j = 0;
+		for_each_set_bit(bit, indio_dev->active_scan_mask,
+				 indio_dev->masklength) {
+			memcpy(&data->scan.channels[j++], &buffer[i * 3 + bit],
+			       sizeof(data->scan.channels[0]));
+		}
+
+		iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
+						   tstamp);
+
+		tstamp += sample_period;
+	}
+
+	return count;
+}
+
 static irqreturn_t fxls8962af_interrupt(int irq, void *p)
 {
 	struct iio_dev *indio_dev = p;
@@ -583,9 +771,32 @@ static irqreturn_t fxls8962af_interrupt(int irq, void *p)
 	if (ret < 0)
 		return IRQ_NONE;
 
+	if (reg & FXLS8962AF_INT_STATUS_SRC_BUF) {
+		ret = fxls8962af_fifo_flush(indio_dev);
+		if (ret < 0)
+			return IRQ_NONE;
+
+		return IRQ_HANDLED;
+	}
+
 	return IRQ_NONE;
 }
 
+static int fxls8962af_fifo_setup(struct iio_dev *indio_dev)
+{
+	struct iio_buffer *buffer;
+
+	buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
+	if (!buffer)
+		return -ENOMEM;
+
+	iio_device_attach_buffer(indio_dev, buffer);
+	indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
+	indio_dev->setup_ops = &fxls8962af_buffer_ops;
+
+	return 0;
+}
+
 static void fxls8962af_regulator_disable(void *data_ptr)
 {
 	struct fxls8962af_data *data = data_ptr;
@@ -751,6 +962,8 @@ int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq)
 		ret = fxls8962af_irq_setup(indio_dev, irq);
 		if (ret < 0)
 			return ret;
+
+		fxls8962af_fifo_setup(indio_dev);
 	}
 
 	ret = pm_runtime_set_active(dev);
-- 
2.31.0

Re: [RFC PATCH 3/4] iio: accel: fxls8962af: add hw buffered sampling

[Sean Nyekjaer]

On 28/04/2021 10.22, Sean Nyekjaer wrote:
> |@@ -433,7 +450,10 @@ static int fxls8962af_read_raw(struct iio_dev 
> *indio_dev, ret = fxls8962af_get_temp(data, val); break; case 
> IIO_ACCEL: - ret = fxls8962af_get_axis(data, chan, val); + if 
> (iio_buffer_enabled(indio_dev))|
|Seeing the iio_device_claim_direct_mode() is doing exactly the same 
check :)|
> |+ ret = -EBUSY; + else + ret = fxls8962af_get_axis(data, chan, val); 
> break; default: ret = -EINVAL;|

Re: [RFC PATCH 3/4] iio: accel: fxls8962af: add hw buffered sampling

[Jonathan Cameron]

On Wed, 28 Apr 2021 13:05:38 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> On 28/04/2021 10.22, Sean Nyekjaer wrote:
> > |@@ -433,7 +450,10 @@ static int fxls8962af_read_raw(struct iio_dev 
> > *indio_dev, ret = fxls8962af_get_temp(data, val); break; case 
> > IIO_ACCEL: - ret = fxls8962af_get_axis(data, chan, val); + if 
> > (iio_buffer_enabled(indio_dev))|  
> |Seeing the iio_device_claim_direct_mode() is doing exactly the same 
> check :)|

And in a race free fashion ;)

> > |+ ret = -EBUSY; + else + ret = fxls8962af_get_axis(data, chan, val); 
> > break; default: ret = -EINVAL;|  
> 

Re: [RFC PATCH 3/4] iio: accel: fxls8962af: add hw buffered sampling

[Jonathan Cameron]

On Wed, 28 Apr 2021 10:22:02 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> When buffered sampling is enabled, the accelerometer will dump data into
> the internal fifo and interrupt at watermark. Then the driver flushes
> all data to the iio buffer.
> As the accelerometer doesn't have internal timestamps, they are aproximated
> between to interrupts.

two? 

This tends to be a noisy approach, so people often try to apply a filter.
However, no need to do that for an initial version.

There are some things in here referring to enabling triggered modes, but I'm
not seeing code to actually do so.  The fun question when dealing with fifos
and triggered mode is what the interface is to switch between the two.
One option I think we've used before is to just have 'no trigger' match
up to fifo mode and if a trigger is set, don't use the fifo.

> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
> This series depends on "iio: accel: add support for
> FXLS8962AF/FXLS8964AF accelerometers"
> 
> Any good pratice in general to how much to fill the fifo?
> The accelerometer a internal buffer with 32 samples and I have set the watermark to 16.

The watermark should be exposed to userspace and we have ABI for that.  Default is normally
set to 1 IIRC on basis it will look the same as a device without a fifo to userspace.
Possible we aren't consistent on that though.

> 
> 
>  drivers/iio/accel/fxls8962af-core.c | 215 +++++++++++++++++++++++++++-
>  1 file changed, 214 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/accel/fxls8962af-core.c b/drivers/iio/accel/fxls8962af-core.c
> index 848f3d68f5d4..2bd5c6d76b63 100644
> --- a/drivers/iio/accel/fxls8962af-core.c
> +++ b/drivers/iio/accel/fxls8962af-core.c
> @@ -20,13 +20,17 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/regmap.h>
>  
> +#include <linux/iio/buffer.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/kfifo_buf.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
>  
>  #include "fxls8962af.h"
>  
>  #define FXLS8962AF_INT_STATUS			0x00
>  #define FXLS8962AF_INT_STATUS_SRC_BOOT		BIT(0)
> +#define FXLS8962AF_INT_STATUS_SRC_BUF		BIT(5)
>  #define FXLS8962AF_INT_STATUS_SRC_DRDY		BIT(7)
>  #define FXLS8962AF_TEMP_OUT			0x01
>  #define FXLS8962AF_VECM_LSB			0x02
> @@ -34,6 +38,9 @@
>  #define FXLS8962AF_OUT_Y_LSB			0x06
>  #define FXLS8962AF_OUT_Z_LSB			0x08
>  #define FXLS8962AF_BUF_STATUS			0x0b
> +#define FXLS8962AF_BUF_STATUS_BUF_CNT		GENMASK(5, 0)
> +#define FXLS8962AF_BUF_STATUS_BUF_OVF		BIT(6)
> +#define FXLS8962AF_BUF_STATUS_BUF_WMRK		BIT(7)
>  #define FXLS8962AF_BUF_X_LSB			0x0c
>  #define FXLS8962AF_BUF_Y_LSB			0x0e
>  #define FXLS8962AF_BUF_Z_LSB			0x10
> @@ -66,6 +73,7 @@
>  #define FXLS8962AF_ASLP_COUNT_LSB		0x1e
>  
>  #define FXLS8962AF_INT_EN			0x20
> +#define FXLS8962AF_INT_EN_BUF_EN		BIT(6)
>  #define FXLS8962AF_INT_PIN_SEL			0x21
>  #define FXLS8962AF_INT_PIN_SEL_MASK		GENMASK(7, 0)
>  #define FXLS8962AF_INT_PIN_SEL_INT1		0x00
> @@ -76,7 +84,10 @@
>  #define FXLS8962AF_OFF_Z			0x24
>  
>  #define FXLS8962AF_BUF_CONFIG1			0x26
> +#define FXLS8962AF_BC1_BUF_MODE_MASK		GENMASK(6, 5)
> +#define FXLS8962AF_BC1_BUF_MODE_PREP(x)		FIELD_PREP(FXLS8962AF_BC1_BUF_MODE_MASK, x)
, (x))

Good practice to always bracket parameters to protect against odd inputs.

>  #define FXLS8962AF_BUF_CONFIG2			0x27
> +#define FXLS8962AF_BUF_CONFIG2_BUF_WMRK		GENMASK(5, 0)
>  
>  #define FXLS8962AF_ORIENT_STATUS		0x28
>  #define FXLS8962AF_ORIENT_CONFIG		0x29
> @@ -106,6 +117,7 @@
>  
>  #define FXLS8962AF_AUTO_SUSPEND_DELAY_MS	2000
>  
> +#define FXLS8962AF_FIFO_LENGTH			32
>  #define FXLS8962AF_SCALE_TABLE_LEN		4
>  #define FXLS8962AF_SAMP_FREQ_TABLE_LEN		13
>  
> @@ -133,6 +145,11 @@ struct fxls8962af_data {
>  	struct regmap *regmap;
>  	const struct fxls8962af_chip_info *chip_info;
>  	struct regulator *vdd_reg;
> +	struct {
> +		__le16 channels[3];
> +		s64 ts __aligned(8);
> +	} scan;
> +	int64_t timestamp, old_timestamp; /* Only used in hw fifo mode. */
>  	struct iio_mount_matrix orientation;
>  };
>  
> @@ -433,7 +450,10 @@ static int fxls8962af_read_raw(struct iio_dev *indio_dev,
>  			ret = fxls8962af_get_temp(data, val);
>  			break;
>  		case IIO_ACCEL:
> -			ret = fxls8962af_get_axis(data, chan, val);
> +			if (iio_buffer_enabled(indio_dev))

This races so use the claim_direct() stuff if you need the protection.
If you can drop it, then even better :)

> +				ret = -EBUSY;
> +			else
> +				ret = fxls8962af_get_axis(data, chan, val);
>  			break;
>  		default:
>  			ret = -EINVAL;
> @@ -572,6 +592,174 @@ static int fxls8962af_reset(struct fxls8962af_data *data)
>  	return ret;
>  }
>  
> +static int fxls8962af_fifo_set_mode(struct fxls8962af_data *data, bool onoff)
> +{
> +	int ret;
> +
> +	ret = regmap_update_bits(data->regmap, FXLS8962AF_BUF_CONFIG1,
> +				 FXLS8962AF_BC1_BUF_MODE_MASK,
> +				 FXLS8962AF_BC1_BUF_MODE_PREP(onoff));
> +
> +	return ret;

return regmap_update_bits(...

> +}

one line.

> +
> +
> +static int fxls8962af_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct fxls8962af_data *data = iio_priv(indio_dev);
> +
> +	return fxls8962af_power_on(data);

No real point in bothering with the local variable.

	fxls8962af_power_on(iio_priv(indio_dev));

> +}
> +
> +static int fxls8962af_buffer_postenable(struct iio_dev *indio_dev)
> +{
> +	struct fxls8962af_data *data = iio_priv(indio_dev);
> +	int ret = 0;

Don't think this value is used anywhere.

> +
> +	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
> +		return 0;
> +
> +	fxls8962af_standby(data);
> +
> +	/* Enable buffer interrupt*/
> +	ret = regmap_update_bits(data->regmap, FXLS8962AF_INT_EN,
> +			FXLS8962AF_INT_EN_BUF_EN,
> +			FXLS8962AF_INT_EN_BUF_EN);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Enable watermark at max fifo size*/
> +	ret = regmap_update_bits(data->regmap, FXLS8962AF_BUF_CONFIG2,
> +			FXLS8962AF_BUF_CONFIG2_BUF_WMRK,
> +			FXLS8962AF_BUF_CONFIG2_BUF_WMRK / 2);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = fxls8962af_fifo_set_mode(data, true);
> +
> +	fxls8962af_active(data);
> +
> +	return ret;
> +}
> +
> +static int fxls8962af_buffer_predisable(struct iio_dev *indio_dev)
> +{
> +	struct fxls8962af_data *data = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	if (indio_dev->currentmode == INDIO_BUFFER_TRIGGERED)
> +		return 0;

Are you currently supporting this? 

> +
> +	fxls8962af_standby(data);
> +
> +	/* Disable buffer interrupt*/
> +	ret = regmap_update_bits(data->regmap, FXLS8962AF_INT_EN,
> +			FXLS8962AF_INT_EN_BUF_EN, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	fxls8962af_fifo_set_mode(data, false);
> +
> +	fxls8962af_active(data);
> +
> +	return 0;
> +}
> +
> +static int fxls8962af_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> +	struct fxls8962af_data *data = iio_priv(indio_dev);
> +
> +	return fxls8962af_power_off(data);
> +}
> +
> +static const struct iio_buffer_setup_ops fxls8962af_buffer_ops = {
> +	.preenable = fxls8962af_buffer_preenable,
> +	.postenable = fxls8962af_buffer_postenable,
> +	.predisable = fxls8962af_buffer_predisable,
> +	.postdisable = fxls8962af_buffer_postdisable,
> +};
> +
> +static int fxls8962af_fifo_transfer(struct fxls8962af_data *data,
> +				    char *buffer, int samples)

Why use a char * for buffer rather than the u16 it seems to be below?

> +{
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int sample_length = 3 * 2;

3 * sizeof(*buffer) having fixed the type of that to u16 *

> +	int ret;
> +	int total_length = samples * sample_length;
> +
> +	ret = regmap_raw_read(data->regmap, FXLS8962AF_BUF_X_LSB, buffer,
> +			      total_length);
> +	if (ret < 0)
> +		dev_err(dev, "Error transferring data from fifo: %d\n", ret);
> +
> +	return ret;
> +}
> +
> +static int fxls8962af_fifo_flush(struct iio_dev *indio_dev)
> +{
> +	struct fxls8962af_data *data = iio_priv(indio_dev);
> +	struct device *dev = regmap_get_device(data->regmap);
> +	u16 buffer[FXLS8962AF_FIFO_LENGTH * 3];
> +	uint64_t sample_period;
> +	unsigned int reg;
> +	int64_t tstamp;
> +	int ret, i;
> +	u8 count;
> +
> +	ret = regmap_read(data->regmap, FXLS8962AF_BUF_STATUS, &reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (reg & FXLS8962AF_BUF_STATUS_BUF_OVF) {
> +		dev_err(dev, "Buffer overflown");
> +		return -1;
> +	}
> +
> +	count = reg & FXLS8962AF_BUF_STATUS_BUF_CNT;
> +
> +	if (!count)
> +		return 0;
> +
> +	data->old_timestamp = data->timestamp;
> +	data->timestamp = iio_get_time_ns(indio_dev);
> +
> +	/* Approximate timestamps for each of the sample based on the sampling,
/* 
 * Approximate

It's a bit kernel subsystem dependent but in IIO I try to keep us to this style.

> +	 * frequency, timestamp for last sample and number of samples.
> +	 */
> +	sample_period = (data->timestamp - data->old_timestamp);
> +	do_div(sample_period, count);
> +	tstamp = data->timestamp - (count - 1) * sample_period;
> +
> +

Single blank line.

> +	ret = fxls8962af_fifo_transfer(data, (u8 *)buffer, count);

As mentioned above change the type this takes to u16 * to avoid casting
fun that I don't think adds anything useful.

> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Ideally we want the IIO core to handle the demux when running in fifo
> +	 * mode but not when running in triggered buffer mode. Unfortunately
> +	 * this does not seem to be possible, so stick with driver demux for
> +	 * now.

At the moment I don't think you are supporting triggered buffer mode, so I'm
not totally sure what you mean here.  If you just mean the necessary parsing
of the hardware fifo input into the layout for the kfifo, then that's something
we've discussed a few times IIRC, but I'm not sure what needs doing is quite
consistent enough to make it practical.

> +	 */
> +	for (i = 0; i < count; i++) {
> +		int j, bit;
> +
> +		j = 0;
> +		for_each_set_bit(bit, indio_dev->active_scan_mask,
> +				 indio_dev->masklength) {
> +			memcpy(&data->scan.channels[j++], &buffer[i * 3 + bit],
> +			       sizeof(data->scan.channels[0]));
> +		}
> +
> +		iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
> +						   tstamp);
> +
> +		tstamp += sample_period;
> +	}
> +
> +	return count;
> +}
> +
>  static irqreturn_t fxls8962af_interrupt(int irq, void *p)
>  {
>  	struct iio_dev *indio_dev = p;
> @@ -583,9 +771,32 @@ static irqreturn_t fxls8962af_interrupt(int irq, void *p)
>  	if (ret < 0)
>  		return IRQ_NONE;
>  
> +	if (reg & FXLS8962AF_INT_STATUS_SRC_BUF) {
> +		ret = fxls8962af_fifo_flush(indio_dev);
> +		if (ret < 0)
> +			return IRQ_NONE;
> +
> +		return IRQ_HANDLED;
> +	}
> +
>  	return IRQ_NONE;
>  }
>  
> +static int fxls8962af_fifo_setup(struct iio_dev *indio_dev)
> +{
> +	struct iio_buffer *buffer;
> +
> +	buffer = devm_iio_kfifo_allocate(&indio_dev->dev);
> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	iio_device_attach_buffer(indio_dev, buffer);
> +	indio_dev->modes |= INDIO_BUFFER_SOFTWARE;
> +	indio_dev->setup_ops = &fxls8962af_buffer_ops;
> +
> +	return 0;
> +}
> +
>  static void fxls8962af_regulator_disable(void *data_ptr)
>  {
>  	struct fxls8962af_data *data = data_ptr;
> @@ -751,6 +962,8 @@ int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq)
>  		ret = fxls8962af_irq_setup(indio_dev, irq);
>  		if (ret < 0)
>  			return ret;
> +
> +		fxls8962af_fifo_setup(indio_dev);
>  	}
>  
>  	ret = pm_runtime_set_active(dev);

Re: [RFC PATCH 3/4] iio: accel: fxls8962af: add hw buffered sampling

[Sean Nyekjaer]

On 28/04/2021 18.32, Jonathan Cameron wrote:
>> When buffered sampling is enabled, the accelerometer will dump data into
>> the internal fifo and interrupt at watermark. Then the driver flushes
>> all data to the iio buffer.
>> As the accelerometer doesn't have internal timestamps, they are aproximated
>> between to interrupts.
> two?
>
> This tends to be a noisy approach, so people often try to apply a filter.
> However, no need to do that for an initial version.
>
> There are some things in here referring to enabling triggered modes, but I'm
> not seeing code to actually do so.  The fun question when dealing with fifos
> and triggered mode is what the interface is to switch between the two.
> One option I think we've used before is to just have 'no trigger' match
> up to fifo mode and if a trigger is set, don't use the fifo.
>
Thanks Jonathan.

Fixed the above text to:
As the accelerometer doesn't have internal timestamps,
they are approximated between the current and last interrupt.

I don't know the correct term here, the accelerometer via the watermark, 
is doing interrupts.
Is that called no-trigger / trigger ?

/Sean

Re: [RFC PATCH 3/4] iio: accel: fxls8962af: add hw buffered sampling

[Jonathan Cameron]

On Thu, 29 Apr 2021 09:40:00 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> On 28/04/2021 18.32, Jonathan Cameron wrote:
> >> When buffered sampling is enabled, the accelerometer will dump data into
> >> the internal fifo and interrupt at watermark. Then the driver flushes
> >> all data to the iio buffer.
> >> As the accelerometer doesn't have internal timestamps, they are aproximated
> >> between to interrupts.  
> > two?
> >
> > This tends to be a noisy approach, so people often try to apply a filter.
> > However, no need to do that for an initial version.
> >
> > There are some things in here referring to enabling triggered modes, but I'm
> > not seeing code to actually do so.  The fun question when dealing with fifos
> > and triggered mode is what the interface is to switch between the two.
> > One option I think we've used before is to just have 'no trigger' match
> > up to fifo mode and if a trigger is set, don't use the fifo.
> >  
> Thanks Jonathan.
> 
> Fixed the above text to:
> As the accelerometer doesn't have internal timestamps,
> they are approximated between the current and last interrupt.
Nice.

> 
> I don't know the correct term here, the accelerometer via the watermark, 
> is doing interrupts.
> Is that called no-trigger / trigger ?

Triggers are one per scan (Set of samples take at approximately the same time)
So anything involving a fifo is without trigger (we don't expose anything because
it is of no use for doing synchronous capture across multiple devices).

One day I'll add a terminology section to the docs!

J

> 
> /Sean

[RFC PATCH 4/4] iio: accel: fxls8962af: fix errata bug E3 - I2C burst reads

[Sean Nyekjaer]

When flushing the hw fifo there is a bug in the I2C that prevents burst
reads of more than one sample pair.

Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
This series depends on "iio: accel: add support for
FXLS8962AF/FXLS8964AF accelerometers"

 drivers/iio/accel/fxls8962af-core.c | 27 +++++++++++++++++++++++----
 drivers/iio/accel/fxls8962af-i2c.c  |  2 +-
 drivers/iio/accel/fxls8962af-spi.c  |  2 +-
 drivers/iio/accel/fxls8962af.h      |  2 +-
 4 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/accel/fxls8962af-core.c b/drivers/iio/accel/fxls8962af-core.c
index 2bd5c6d76b63..fad9e756d313 100644
--- a/drivers/iio/accel/fxls8962af-core.c
+++ b/drivers/iio/accel/fxls8962af-core.c
@@ -149,6 +149,7 @@ struct fxls8962af_data {
 		__le16 channels[3];
 		s64 ts __aligned(8);
 	} scan;
+	bool i2c_device;
 	int64_t timestamp, old_timestamp; /* Only used in hw fifo mode. */
 	struct iio_mount_matrix orientation;
 };
@@ -684,11 +685,27 @@ static int fxls8962af_fifo_transfer(struct fxls8962af_data *data,
 {
 	struct device *dev = regmap_get_device(data->regmap);
 	int sample_length = 3 * 2;
-	int ret;
+	int ret, i;
 	int total_length = samples * sample_length;
 
-	ret = regmap_raw_read(data->regmap, FXLS8962AF_BUF_X_LSB, buffer,
-			      total_length);
+	if (data->i2c_device) {
+		/* Due to errata bug:
+		 * E3: FIFO burst read operation error using I2C interface
+		 * We have to avoid burst reads on I2C..
+		 */
+		for (i = 0; i < samples; i++) {
+			ret = regmap_raw_read(data->regmap, FXLS8962AF_BUF_X_LSB,
+					      &buffer[i * sample_length],
+					      sample_length);
+			if (ret < 0)
+				goto out;
+		}
+	} else {
+		ret = regmap_raw_read(data->regmap, FXLS8962AF_BUF_X_LSB, buffer,
+				      total_length);
+	}
+
+ out:
 	if (ret < 0)
 		dev_err(dev, "Error transferring data from fifo: %d\n", ret);
 
@@ -899,7 +916,8 @@ static int fxls8962af_irq_setup(struct iio_dev *indio_dev, int irq)
 	return ret;
 }
 
-int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq)
+int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq,
+			  bool i2c_device)
 {
 	struct fxls8962af_data *data;
 	struct iio_dev *indio_dev;
@@ -913,6 +931,7 @@ int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq)
 	data = iio_priv(indio_dev);
 	dev_set_drvdata(dev, indio_dev);
 	data->regmap = regmap;
+	data->i2c_device = i2c_device;
 
 	ret = iio_read_mount_matrix(dev, "mount-matrix", &data->orientation);
 	if (ret)
diff --git a/drivers/iio/accel/fxls8962af-i2c.c b/drivers/iio/accel/fxls8962af-i2c.c
index cba12160a714..03bd7ef285d0 100644
--- a/drivers/iio/accel/fxls8962af-i2c.c
+++ b/drivers/iio/accel/fxls8962af-i2c.c
@@ -24,7 +24,7 @@ static int fxls8962af_probe(struct i2c_client *client)
 		return PTR_ERR(regmap);
 	}
 
-	return fxls8962af_core_probe(&client->dev, regmap, client->irq);
+	return fxls8962af_core_probe(&client->dev, regmap, client->irq, true);
 }
 
 static const struct i2c_device_id fxls8962af_id[] = {
diff --git a/drivers/iio/accel/fxls8962af-spi.c b/drivers/iio/accel/fxls8962af-spi.c
index cb971b76d135..77186220f6dc 100644
--- a/drivers/iio/accel/fxls8962af-spi.c
+++ b/drivers/iio/accel/fxls8962af-spi.c
@@ -24,7 +24,7 @@ static int fxls8962af_probe(struct spi_device *spi)
 		return PTR_ERR(regmap);
 	}
 
-	return fxls8962af_core_probe(&spi->dev, regmap, spi->irq);
+	return fxls8962af_core_probe(&spi->dev, regmap, spi->irq, false);
 }
 
 static const struct of_device_id fxls8962af_spi_of_match[] = {
diff --git a/drivers/iio/accel/fxls8962af.h b/drivers/iio/accel/fxls8962af.h
index b67572c3ef06..e428163926b7 100644
--- a/drivers/iio/accel/fxls8962af.h
+++ b/drivers/iio/accel/fxls8962af.h
@@ -13,7 +13,7 @@ enum {
 	fxls8964af,
 };
 
-int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq);
+int fxls8962af_core_probe(struct device *dev, struct regmap *regmap, int irq, bool i2c_device);
 int fxls8962af_core_remove(struct device *dev);
 
 extern const struct dev_pm_ops fxls8962af_pm_ops;
-- 
2.31.0

Re: [RFC PATCH 4/4] iio: accel: fxls8962af: fix errata bug E3 - I2C burst reads

[Andy Shevchenko]

On Wed, Apr 28, 2021 at 11:22 AM Sean Nyekjaer <sean@geanix.com> wrote:
>
> When flushing the hw fifo there is a bug in the I2C that prevents burst
> reads of more than one sample pair.
>
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
> This series depends on "iio: accel: add support for
> FXLS8962AF/FXLS8964AF accelerometers"

This should be part of that series.

Besides, see below.

...

> +       bool i2c_device;


> +       data->i2c_device = i2c_device;

This is redundant. Use i2c_verify_client() instead.


-- 
With Best Regards,
Andy Shevchenko

Re: [RFC PATCH 4/4] iio: accel: fxls8962af: fix errata bug E3 - I2C burst reads

[Sean Nyekjaer]

On 28/04/2021 13.24, Andy Shevchenko wrote:
> This should be part of that series.
OK, will include next week...
>
> Besides, see below.
>
> ...
>
>> +       bool i2c_device;
>> +       data->i2c_device = i2c_device;
> This is redundant. Use i2c_verify_client() instead.
Thanks I've been looking for a that functionality :)

/Sean

Re: [RFC PATCH 1/4] dt-bindings: iio: accel: fxls8962af: add interrupt options

[Jonathan Cameron]

On Wed, 28 Apr 2021 10:22:00 +0200
Sean Nyekjaer <sean@geanix.com> wrote:

> This in done for supporting hw buffered sampling
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>

Please squash into the original binding patch.
Bindings should reflect the hardware features rather than what the driver
happens to be currently taking advantage of.

Occasionally we'll do it in multiple parts because we aren't sure what
the binding will look like, but in this case you've posted this before
the other has been applied so just merge them together.

Thanks,

Jonathan

> ---
> This series depends on "iio: accel: add support for
> FXLS8962AF/FXLS8964AF accelerometers"
> 
>  .../bindings/iio/accel/nxp,fxls8962af.yaml           | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml b/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
> index c7be7a1679ab..e0e5542377df 100644
> --- a/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
> +++ b/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
> @@ -32,6 +32,16 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  interrupt-names:
> +    maxItems: 1
> +    items:
> +      enum:
> +        - INT1
> +        - INT2
> +
> +  drive-open-drain:
> +    type: boolean
> +
>  required:
>    - compatible
>    - reg
> @@ -51,6 +61,7 @@ examples:
>              reg = <0x62>;
>              interrupt-parent = <&gpio0>;
>              interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "INT1";
>          };
>      };
>    - |
> @@ -66,5 +77,6 @@ examples:
>              spi-max-frequency = <4000000>;
>              interrupt-parent = <&gpio0>;
>              interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "INT1";
>          };
>      };

Re: [RFC PATCH 1/4] dt-bindings: iio: accel: fxls8962af: add interrupt options

[Rob Herring]

On Wed, Apr 28, 2021 at 10:22:00AM +0200, Sean Nyekjaer wrote:
> This in done for supporting hw buffered sampling
> 
> Signed-off-by: Sean Nyekjaer <sean@geanix.com>
> ---
> This series depends on "iio: accel: add support for
> FXLS8962AF/FXLS8964AF accelerometers"
> 
>  .../bindings/iio/accel/nxp,fxls8962af.yaml           | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml b/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
> index c7be7a1679ab..e0e5542377df 100644
> --- a/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
> +++ b/Documentation/devicetree/bindings/iio/accel/nxp,fxls8962af.yaml
> @@ -32,6 +32,16 @@ properties:
>    interrupts:
>      maxItems: 1
>  
> +  interrupt-names:
> +    maxItems: 1
> +    items:
> +      enum:
> +        - INT1
> +        - INT2

You can simplify this to:

interrupt-names:
  enum:
    - INT1
    - INT2


> +
> +  drive-open-drain:
> +    type: boolean
> +
>  required:
>    - compatible
>    - reg
> @@ -51,6 +61,7 @@ examples:
>              reg = <0x62>;
>              interrupt-parent = <&gpio0>;
>              interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "INT1";
>          };
>      };
>    - |
> @@ -66,5 +77,6 @@ examples:
>              spi-max-frequency = <4000000>;
>              interrupt-parent = <&gpio0>;
>              interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "INT1";
>          };
>      };
> -- 
> 2.31.0
>