[RFC 0/2] iio: imu: Add driver and dt-bindings for BMI323

[RFC 0/2] iio: imu: Add driver and dt-bindings for BMI323

[Jagath Jog J]

Add dt-bindings and IIO driver for Bosch BMI323 a 6 axis IMU.

Jagath Jog J (2):
  dt-bindings: iio: imu: Add DT binding doc for BMI323
  iio: imu: Add driver for BMI323 IMU

 .../bindings/iio/imu/bosch,bmi323.yaml        |   81 +
 MAINTAINERS                                   |    7 +
 drivers/iio/imu/Kconfig                       |    1 +
 drivers/iio/imu/Makefile                      |    1 +
 drivers/iio/imu/bmi323/Kconfig                |   33 +
 drivers/iio/imu/bmi323/Makefile               |    7 +
 drivers/iio/imu/bmi323/bmi323.h               |  198 ++
 drivers/iio/imu/bmi323/bmi323_core.c          | 2260 +++++++++++++++++
 drivers/iio/imu/bmi323/bmi323_i2c.c           |  115 +
 drivers/iio/imu/bmi323/bmi323_spi.c           |  106 +
 10 files changed, 2809 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml
 create mode 100644 drivers/iio/imu/bmi323/Kconfig
 create mode 100644 drivers/iio/imu/bmi323/Makefile
 create mode 100644 drivers/iio/imu/bmi323/bmi323.h
 create mode 100644 drivers/iio/imu/bmi323/bmi323_core.c
 create mode 100644 drivers/iio/imu/bmi323/bmi323_i2c.c
 create mode 100644 drivers/iio/imu/bmi323/bmi323_spi.c

-- 
2.20.1

[RFC 1/2] dt-bindings: iio: imu: Add DT binding doc for BMI323

[Jagath Jog J]

Add devicetree description document for Bosch BMI323, a 6-Axis IMU.

Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
---
 .../bindings/iio/imu/bosch,bmi323.yaml        | 81 +++++++++++++++++++
 1 file changed, 81 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml

diff --git a/Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml b/Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml
new file mode 100644
index 000000000000..9c08988103c5
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml
@@ -0,0 +1,81 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/imu/bosch,bmi323.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Bosch BMI323 6-Axis IMU
+
+maintainers:
+  - Jagath Jog J <jagathjog1996@gmail.com>
+
+description:
+  BMI323 is a 6-axis inertial measurement unit that supports acceleration and
+  gyroscopic measurements with hardware fifo buffering. Sensor also provides
+  events information such as motion, steps, orientation, single and double
+  tap detection.
+
+properties:
+  compatible:
+    const: bosch,bmi323
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-names:
+    enum:
+      - INT1
+      - INT2
+    description: |
+      set to "INT1" if INT1 pin should be used as interrupt input, set
+      to "INT2" if INT2 pin should be used instead
+
+  drive-open-drain:
+    description: |
+      set if the specified interrupt pin should be configured as
+      open drain. If not set, defaults to push-pull.
+
+required:
+  - compatible
+  - reg
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    // Example for I2C
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        bmi323@68 {
+            compatible = "bosch,bmi323";
+            reg = <0x68>;
+            interrupt-parent = <&gpio1>;
+            interrupts = <29 IRQ_TYPE_EDGE_RISING>;
+            interrupt-names = "INT1";
+        };
+    };
+  - |
+    // Example for SPI
+    #include <dt-bindings/interrupt-controller/irq.h>
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        bmi323@0 {
+            compatible = "bosch,bmi323";
+            reg = <0>;
+            spi-max-frequency = <10000000>;
+            interrupt-parent = <&gpio2>;
+            interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
+            interrupt-names = "INT2";
+        };
+    };
-- 
2.20.1

Re: [RFC 1/2] dt-bindings: iio: imu: Add DT binding doc for BMI323

[Krzysztof Kozlowski]

On 18/09/2023 10:03, Jagath Jog J wrote:
> Add devicetree description document for Bosch BMI323, a 6-Axis IMU.

I don't know why this is RFC and cover letter does not explain it. Shall
I just ignore it? Patch is no ready? Recently at least two times someone
was disappointed that his code marked as RFC received my review.

A nit, subject: drop second/last, redundant "DT binding doc for". The
"dt-bindings" prefix is already stating that these are bindings. Four
words entirely redundant and duplicating what prefix is saying...


> 
> Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
> ---
>  .../bindings/iio/imu/bosch,bmi323.yaml        | 81 +++++++++++++++++++
>  1 file changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml b/Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml
> new file mode 100644
> index 000000000000..9c08988103c5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml
> @@ -0,0 +1,81 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/imu/bosch,bmi323.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Bosch BMI323 6-Axis IMU
> +
> +maintainers:
> +  - Jagath Jog J <jagathjog1996@gmail.com>
> +
> +description:
> +  BMI323 is a 6-axis inertial measurement unit that supports acceleration and
> +  gyroscopic measurements with hardware fifo buffering. Sensor also provides
> +  events information such as motion, steps, orientation, single and double
> +  tap detection.
> +
> +properties:
> +  compatible:
> +    const: bosch,bmi323
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-names:
> +    enum:
> +      - INT1
> +      - INT2
> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      set to "INT1" if INT1 pin should be used as interrupt input, set
> +      to "INT2" if INT2 pin should be used instead

And what happens with other INT pin? Remains floating?

> +
> +  drive-open-drain:
> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      set if the specified interrupt pin should be configured as
> +      open drain. If not set, defaults to push-pull.

Missing supplies. Are you sure device does not use any electric energy?

> +
> +required:
> +  - compatible
> +  - reg
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    // Example for I2C
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +> +        bmi323@68 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +            compatible = "bosch,bmi323";
> +            reg = <0x68>;
> +            interrupt-parent = <&gpio1>;
> +            interrupts = <29 IRQ_TYPE_EDGE_RISING>;
> +            interrupt-names = "INT1";
> +        };
> +    };
> +  - |
> +    // Example for SPI
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    spi {


It's the same as other example. No difference. Drop.

Best regards,
Krzysztof

Re: [RFC 1/2] dt-bindings: iio: imu: Add DT binding doc for BMI323

[Jagath Jog J]

Hi Krzysztof,

On Mon, Sep 18, 2023 at 5:55 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 18/09/2023 10:03, Jagath Jog J wrote:
> > Add devicetree description document for Bosch BMI323, a 6-Axis IMU.
>
> I don't know why this is RFC and cover letter does not explain it. Shall
> I just ignore it? Patch is no ready? Recently at least two times someone
> was disappointed that his code marked as RFC received my review.

Thank you for reviewing. This was the sensor's first patch series,
so I initially submitted it as an RFC. I will mark it as "Patch"
in the next series.

>
> A nit, subject: drop second/last, redundant "DT binding doc for". The
> "dt-bindings" prefix is already stating that these are bindings. Four
> words entirely redundant and duplicating what prefix is saying...

Sure I will remove redundant words from the subject line.

>
>
> >
> > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
> > ---
> >  .../bindings/iio/imu/bosch,bmi323.yaml        | 81 +++++++++++++++++++
> >  1 file changed, 81 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml b/Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml
> > new file mode 100644
> > index 000000000000..9c08988103c5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml
> > @@ -0,0 +1,81 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/imu/bosch,bmi323.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Bosch BMI323 6-Axis IMU
> > +
> > +maintainers:
> > +  - Jagath Jog J <jagathjog1996@gmail.com>
> > +
> > +description:
> > +  BMI323 is a 6-axis inertial measurement unit that supports acceleration and
> > +  gyroscopic measurements with hardware fifo buffering. Sensor also provides
> > +  events information such as motion, steps, orientation, single and double
> > +  tap detection.
> > +
> > +properties:
> > +  compatible:
> > +    const: bosch,bmi323
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  interrupt-names:
> > +    enum:
> > +      - INT1
> > +      - INT2
> > +    description: |
>
> Do not need '|' unless you need to preserve formatting.
>
> > +      set to "INT1" if INT1 pin should be used as interrupt input, set
> > +      to "INT2" if INT2 pin should be used instead
>
> And what happens with other INT pin? Remains floating?`

Yes, the other pin is unconnected. The driver provides
support for either INT1, INT2, or no interrupt configuration.
I should have added minItems, maxItems, and const,
I will add these in the next series.

>
> > +
> > +  drive-open-drain:
> > +    description: |
>
> Do not need '|' unless you need to preserve formatting.

Sure I will remove this.

>
> > +      set if the specified interrupt pin should be configured as
> > +      open drain. If not set, defaults to push-pull.
>
> Missing supplies. Are you sure device does not use any electric energy?

Sorry, I missed adding supply, I will add it in the next series.

>
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +allOf:
> > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    // Example for I2C
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +> +        bmi323@68 {
>
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

I intend to utilize the generic term 'imu' (representing the
accelerometer and gyrometer)
even though it's not listed in the generic-names recommendation.
Would this be acceptable?

>
> > +            compatible = "bosch,bmi323";
> > +            reg = <0x68>;
> > +            interrupt-parent = <&gpio1>;
> > +            interrupts = <29 IRQ_TYPE_EDGE_RISING>;
> > +            interrupt-names = "INT1";
> > +        };
> > +    };
> > +  - |
> > +    // Example for SPI
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    spi {
>
>
> It's the same as other example. No difference. Drop.

Sure I will keep only one example.

>
> Best regards,
> Krzysztof
>

Regards,
Jagath

Re: [RFC 1/2] dt-bindings: iio: imu: Add DT binding doc for BMI323

[Jonathan Cameron]

On Tue, 19 Sep 2023 22:14:40 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

> Hi Krzysztof,
> 
> On Mon, Sep 18, 2023 at 5:55 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > On 18/09/2023 10:03, Jagath Jog J wrote:  
> > > Add devicetree description document for Bosch BMI323, a 6-Axis IMU.  
> >
> > I don't know why this is RFC and cover letter does not explain it. Shall
> > I just ignore it? Patch is no ready? Recently at least two times someone
> > was disappointed that his code marked as RFC received my review.  
> 
> Thank you for reviewing. This was the sensor's first patch series,
> so I initially submitted it as an RFC. I will mark it as "Patch"
> in the next series.

Have more confidence!  RFCs need to have clearly stated questions.
If you don't have any then you are putting forwards driver for review
in ordering to get it merged upstream - so PATCH is appropriate.
As you'll see many IIO drivers go through a 'few' revisions once they
are posted (hopefully not too many!)

Krzysztof is great at reviewing whatever shows up, but in many
cases reviewers won't look at an RFC (unless a big discussion starts) because
they aren't interested in open questions, just code that the author considers
ready.

Jonathan

Re: [RFC 1/2] dt-bindings: iio: imu: Add DT binding doc for BMI323

[Jagath Jog J]

Hi Jonathan

On Sun, Sep 24, 2023 at 7:01 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Tue, 19 Sep 2023 22:14:40 +0530
> Jagath Jog J <jagathjog1996@gmail.com> wrote:
>
> > Hi Krzysztof,
> >
> > On Mon, Sep 18, 2023 at 5:55 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> > >
> > > On 18/09/2023 10:03, Jagath Jog J wrote:
> > > > Add devicetree description document for Bosch BMI323, a 6-Axis IMU.
> > >
> > > I don't know why this is RFC and cover letter does not explain it. Shall
> > > I just ignore it? Patch is no ready? Recently at least two times someone
> > > was disappointed that his code marked as RFC received my review.
> >
> > Thank you for reviewing. This was the sensor's first patch series,
> > so I initially submitted it as an RFC. I will mark it as "Patch"
> > in the next series.
>
> Have more confidence!  RFCs need to have clearly stated questions.
> If you don't have any then you are putting forwards driver for review
> in ordering to get it merged upstream - so PATCH is appropriate.
> As you'll see many IIO drivers go through a 'few' revisions once they
> are posted (hopefully not too many!)
>
> Krzysztof is great at reviewing whatever shows up, but in many
> cases reviewers won't look at an RFC (unless a big discussion starts) because
> they aren't interested in open questions, just code that the author considers
> ready.

Thank you for your feedback and guidance. I appreciate
the insight into the RFC process and the importance of
clearly stated questions. I'll keep this in mind when
preparing future submissions.

Regards
Jagath

>
> Jonathan

Re: [RFC 1/2] dt-bindings: iio: imu: Add DT binding doc for BMI323

[Jonathan Cameron]

On Mon, 18 Sep 2023 13:33:13 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

> Add devicetree description document for Bosch BMI323, a 6-Axis IMU.
> 
> Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
> ---
>  .../bindings/iio/imu/bosch,bmi323.yaml        | 81 +++++++++++++++++++
>  1 file changed, 81 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml b/Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml
> new file mode 100644
> index 000000000000..9c08988103c5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml
> @@ -0,0 +1,81 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/imu/bosch,bmi323.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Bosch BMI323 6-Axis IMU
> +
> +maintainers:
> +  - Jagath Jog J <jagathjog1996@gmail.com>
> +
> +description:
> +  BMI323 is a 6-axis inertial measurement unit that supports acceleration and
> +  gyroscopic measurements with hardware fifo buffering. Sensor also provides
> +  events information such as motion, steps, orientation, single and double
> +  tap detection.
> +
> +properties:
> +  compatible:
> +    const: bosch,bmi323
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  interrupt-names:
> +    enum:
> +      - INT1
> +      - INT2
> +    description: |
> +      set to "INT1" if INT1 pin should be used as interrupt input, set
> +      to "INT2" if INT2 pin should be used instead

Why not both?  Sure driver might elect to use only one, but the binding
describes the hardware not the driver and both might be wired.

Lots of different sources of interrupts so might be advantageous
to split them up across two wires.   A simple case being to route
errors to one and everything 'good' to the other.  No obligation to
support that in the Linux driver though if you don't need to.

> +
> +  drive-open-drain:
> +    description: |
> +      set if the specified interrupt pin should be configured as
> +      open drain. If not set, defaults to push-pull.

Two pins.  Might be different so you need two controls.

> +
> +required:
> +  - compatible
> +  - reg

As mentioned, need power supplies specified and marked as required
(though they may be provided via always on regulators and rely on stubs
being created by the regulator subsystem on a given board).
Looks like there are at least 2 supplies.

> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    // Example for I2C
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        bmi323@68 {
> +            compatible = "bosch,bmi323";
> +            reg = <0x68>;
> +            interrupt-parent = <&gpio1>;
> +            interrupts = <29 IRQ_TYPE_EDGE_RISING>;
> +            interrupt-names = "INT1";
> +        };
> +    };
> +  - |
> +    // Example for SPI
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        bmi323@0 {
> +            compatible = "bosch,bmi323";
> +            reg = <0>;
> +            spi-max-frequency = <10000000>;
> +            interrupt-parent = <&gpio2>;
> +            interrupts = <12 IRQ_TYPE_EDGE_FALLING>;
> +            interrupt-names = "INT2";
> +        };
> +    };

Re: [RFC 1/2] dt-bindings: iio: imu: Add DT binding doc for BMI323

[Jagath Jog J]

HI Jonathan,

On Sun, Sep 24, 2023 at 7:07 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 18 Sep 2023 13:33:13 +0530
> Jagath Jog J <jagathjog1996@gmail.com> wrote:
>
> > Add devicetree description document for Bosch BMI323, a 6-Axis IMU.
> >
> > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
> > ---
> >  .../bindings/iio/imu/bosch,bmi323.yaml        | 81 +++++++++++++++++++
> >  1 file changed, 81 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml b/Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml
> > new file mode 100644
> > index 000000000000..9c08988103c5
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml
> > @@ -0,0 +1,81 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/imu/bosch,bmi323.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Bosch BMI323 6-Axis IMU
> > +
> > +maintainers:
> > +  - Jagath Jog J <jagathjog1996@gmail.com>
> > +
> > +description:
> > +  BMI323 is a 6-axis inertial measurement unit that supports acceleration and
> > +  gyroscopic measurements with hardware fifo buffering. Sensor also provides
> > +  events information such as motion, steps, orientation, single and double
> > +  tap detection.
> > +
> > +properties:
> > +  compatible:
> > +    const: bosch,bmi323
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  interrupt-names:
> > +    enum:
> > +      - INT1
> > +      - INT2
> > +    description: |
> > +      set to "INT1" if INT1 pin should be used as interrupt input, set
> > +      to "INT2" if INT2 pin should be used instead
>
> Why not both?  Sure driver might elect to use only one, but the binding
> describes the hardware not the driver and both might be wired.

If both interrupt pins are wired, should the DTS file need to define
both of the pins?

>
> Lots of different sources of interrupts so might be advantageous
> to split them up across two wires.   A simple case being to route
> errors to one and everything 'good' to the other.  No obligation to
> support that in the Linux driver though if you don't need to.

Sure I will split into two different wires in bindings.

>
> > +
> > +  drive-open-drain:
> > +    description: |
> > +      set if the specified interrupt pin should be configured as
> > +      open drain. If not set, defaults to push-pull.
>
> Two pins.  Might be different so you need two controls.

Sure, In bindings I will add two different drive controls.
If both interrupt pins are wired with different drive options
should the DTS file still define both of the pins?

>
> > +
> > +required:
> > +  - compatible
> > +  - reg
>
> As mentioned, need power supplies specified and marked as required
> (though they may be provided via always on regulators and rely on stubs
> being created by the regulator subsystem on a given board).
> Looks like there are at least 2 supplies.

Sure, I will add 2 power supplies in the next series.

Regards
Jagath

Re: [RFC 1/2] dt-bindings: iio: imu: Add DT binding doc for BMI323

[Jonathan Cameron]

On Thu, 28 Sep 2023 03:07:22 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

> HI Jonathan,
> 
> On Sun, Sep 24, 2023 at 7:07 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Mon, 18 Sep 2023 13:33:13 +0530
> > Jagath Jog J <jagathjog1996@gmail.com> wrote:
> >  
> > > Add devicetree description document for Bosch BMI323, a 6-Axis IMU.
> > >
> > > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
> > > ---
> > >  .../bindings/iio/imu/bosch,bmi323.yaml        | 81 +++++++++++++++++++
> > >  1 file changed, 81 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml b/Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml
> > > new file mode 100644
> > > index 000000000000..9c08988103c5
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iio/imu/bosch,bmi323.yaml
> > > @@ -0,0 +1,81 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iio/imu/bosch,bmi323.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Bosch BMI323 6-Axis IMU
> > > +
> > > +maintainers:
> > > +  - Jagath Jog J <jagathjog1996@gmail.com>
> > > +
> > > +description:
> > > +  BMI323 is a 6-axis inertial measurement unit that supports acceleration and
> > > +  gyroscopic measurements with hardware fifo buffering. Sensor also provides
> > > +  events information such as motion, steps, orientation, single and double
> > > +  tap detection.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: bosch,bmi323
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  interrupt-names:
> > > +    enum:
> > > +      - INT1
> > > +      - INT2
> > > +    description: |
> > > +      set to "INT1" if INT1 pin should be used as interrupt input, set
> > > +      to "INT2" if INT2 pin should be used instead  
> >
> > Why not both?  Sure driver might elect to use only one, but the binding
> > describes the hardware not the driver and both might be wired.  
> 
> If both interrupt pins are wired, should the DTS file need to define
> both of the pins?

Yes it should. + we need the names to know which is which.
You could rely on order, but it's more flexible to not do so, particularly
when you also need to support case where only one is wired.


Jonathan

Re: [RFC 1/2] dt-bindings: iio: imu: Add DT binding doc for BMI323

[Jagath Jog J]

Hi Jonathan,

Few more questions before sending the next series.

On Sat, Sep 30, 2023 at 9:35 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Thu, 28 Sep 2023 03:07:22 +0530
> Jagath Jog J <jagathjog1996@gmail.com> wrote:
>
> > HI Jonathan,
> >
> > On Sun, Sep 24, 2023 at 7:07 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > >
> > > On Mon, 18 Sep 2023 13:33:13 +0530
> > > Jagath Jog J <jagathjog1996@gmail.com> wrote:
> > >
> > > > Add devicetree description document for Bosch BMI323, a 6-Axis IMU.
> > > >
> > > > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>

> > > > +  interrupts:
> > > > +    maxItems: 1
> > > > +
> > > > +  interrupt-names:
> > > > +    enum:
> > > > +      - INT1
> > > > +      - INT2
> > > > +    description: |
> > > > +      set to "INT1" if INT1 pin should be used as interrupt input, set
> > > > +      to "INT2" if INT2 pin should be used instead
> > >
> > > Why not both?  Sure driver might elect to use only one, but the binding
> > > describes the hardware not the driver and both might be wired.
> >
> > If both interrupt pins are wired, should the DTS file need to define
> > both of the pins?
>
> Yes it should. + we need the names to know which is which.
> You could rely on order, but it's more flexible to not do so, particularly
> when you also need to support case where only one is wired.

In the driver, I currently prioritize INT1 over INT2 when checking
(bmi323_trigger_probe(..)) based on the interrupt-names defined
in the device tree. However, I'm open to suggestions on the best
way to ensure that the order doesn't affect the selection process
when both interrupts are defined in the device tree.

Each feature, such as data-ready, watermark, tap, and others, supports
either INT1 or INT2. Based on the interrupt pin defined in the device tree,
I configure the all the features accordingly.

Regarding your earlier suggestion to have two different controls for
drive-open-drain, do I need to define sensor-specific drive controls
in bindings for both interrupt pins?
for ex: bosch,irq{1,2}-open-drain

Regards
Jagath
>
>
> Jonathan
>
>

Re: [RFC 1/2] dt-bindings: iio: imu: Add DT binding doc for BMI323

[Jonathan Cameron]

On Sun, 8 Oct 2023 11:54:39 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

> Hi Jonathan,
> 
> Few more questions before sending the next series.
> 
> On Sat, Sep 30, 2023 at 9:35 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Thu, 28 Sep 2023 03:07:22 +0530
> > Jagath Jog J <jagathjog1996@gmail.com> wrote:
> >  
> > > HI Jonathan,
> > >
> > > On Sun, Sep 24, 2023 at 7:07 PM Jonathan Cameron <jic23@kernel.org> wrote:  
> > > >
> > > > On Mon, 18 Sep 2023 13:33:13 +0530
> > > > Jagath Jog J <jagathjog1996@gmail.com> wrote:
> > > >  
> > > > > Add devicetree description document for Bosch BMI323, a 6-Axis IMU.
> > > > >
> > > > > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>  
> 
> > > > > +  interrupts:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  interrupt-names:
> > > > > +    enum:
> > > > > +      - INT1
> > > > > +      - INT2
> > > > > +    description: |
> > > > > +      set to "INT1" if INT1 pin should be used as interrupt input, set
> > > > > +      to "INT2" if INT2 pin should be used instead  
> > > >
> > > > Why not both?  Sure driver might elect to use only one, but the binding
> > > > describes the hardware not the driver and both might be wired.  
> > >
> > > If both interrupt pins are wired, should the DTS file need to define
> > > both of the pins?  
> >
> > Yes it should. + we need the names to know which is which.
> > You could rely on order, but it's more flexible to not do so, particularly
> > when you also need to support case where only one is wired.  
> 
> In the driver, I currently prioritize INT1 over INT2 when checking
> (bmi323_trigger_probe(..)) based on the interrupt-names defined
> in the device tree. However, I'm open to suggestions on the best
> way to ensure that the order doesn't affect the selection process
> when both interrupts are defined in the device tree.

If they are both present it is absolutely fine to pick one in preference
to the other.

> 
> Each feature, such as data-ready, watermark, tap, and others, supports
> either INT1 or INT2. Based on the interrupt pin defined in the device tree,
> I configure the all the features accordingly.

That's an implementation choice to do them all based on one interrupt
pin so absolutely fine to do that in the Linux driver, but the dt binding
should allow for other choices as there are sometimes efficiency gains
in doing so.

> 
> Regarding your earlier suggestion to have two different controls for
> drive-open-drain, do I need to define sensor-specific drive controls
> in bindings for both interrupt pins?
> for ex: bosch,irq{1,2}-open-drain

Hmm. We do have precedence for a single control e.g.
nxp,fxls8962af.yaml as drive-open-drain.  So perhaps just go with that
and if anyone is needs different values we can figure it out later.
pin control (which is where that binding item comes from) seems to have
examples doing much the same.  Sets of pins with a single drive-open-drain
entry.

Linus, any comments on this as you've dealt with far more similar cases
than me!

Jonathan

> 
> Regards
> Jagath
> >
> >
> > Jonathan
> >
> >  

Re: [RFC 1/2] dt-bindings: iio: imu: Add DT binding doc for BMI323

[Linus Walleij]

On Tue, Oct 10, 2023 at 10:59 AM Jonathan Cameron <jic23@kernel.org> wrote:
> Jagath Jog J <jagathjog1996@gmail.com> wrote:

> > Regarding your earlier suggestion to have two different controls for
> > drive-open-drain, do I need to define sensor-specific drive controls
> > in bindings for both interrupt pins?
> > for ex: bosch,irq{1,2}-open-drain
>
> Hmm. We do have precedence for a single control e.g.
> nxp,fxls8962af.yaml as drive-open-drain.  So perhaps just go with that
> and if anyone is needs different values we can figure it out later.
> pin control (which is where that binding item comes from) seems to have
> examples doing much the same.  Sets of pins with a single drive-open-drain
> entry.
>
> Linus, any comments on this as you've dealt with far more similar cases
> than me!

Also st,st-sensors use drive-open-drain.

And that in turn is used because the pin control subsystem use that
exact property. (See
Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml)

So use that.

(I'm so happy to be able to provide a definitive answer for once!)

Yours,
Linus Walleij

Re: [RFC 1/2] dt-bindings: iio: imu: Add DT binding doc for BMI323

[Jonathan Cameron]

On Tue, 10 Oct 2023 11:06:18 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Tue, Oct 10, 2023 at 10:59 AM Jonathan Cameron <jic23@kernel.org> wrote:
> > Jagath Jog J <jagathjog1996@gmail.com> wrote:  
> 
> > > Regarding your earlier suggestion to have two different controls for
> > > drive-open-drain, do I need to define sensor-specific drive controls
> > > in bindings for both interrupt pins?
> > > for ex: bosch,irq{1,2}-open-drain  
> >
> > Hmm. We do have precedence for a single control e.g.
> > nxp,fxls8962af.yaml as drive-open-drain.  So perhaps just go with that
> > and if anyone is needs different values we can figure it out later.
> > pin control (which is where that binding item comes from) seems to have
> > examples doing much the same.  Sets of pins with a single drive-open-drain
> > entry.
> >
> > Linus, any comments on this as you've dealt with far more similar cases
> > than me!  
> 
> Also st,st-sensors use drive-open-drain.
> 
> And that in turn is used because the pin control subsystem use that
> exact property. (See
> Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml)
> 
> So use that.
> 
> (I'm so happy to be able to provide a definitive answer for once!)

We kind of lost the question along the way.  Wasn't so much about whether
there was a generic binding but more about whether it is worth providing
separate controls for the two IRQ pins?  Or just assume no one is crazy
enough to play that level of mix and match.

J
> 
> Yours,
> Linus Walleij

Re: [RFC 1/2] dt-bindings: iio: imu: Add DT binding doc for BMI323

[Linus Walleij]

On Tue, Oct 10, 2023 at 4:42 PM Jonathan Cameron <jic23@kernel.org> wrote:

> We kind of lost the question along the way.  Wasn't so much about whether
> there was a generic binding but more about whether it is worth providing
> separate controls for the two IRQ pins?  Or just assume no one is crazy
> enough to play that level of mix and match.

Ugh no, that's upfront design for a nonexistent use case.

- First, to even consider open drain the designer need to be really
  short of IRQ lines/rails, and, despite knowing it's a bad idea, decide
  to share this line between several peripherals, even though it will
  require I2C traffic to just determine which one even fired the IRQ.

- Second, be interested in using two IRQs to distinguish between
  different events? When we just faced the situation that we had
  too few IRQ lines so we need to start sharing them with open
  drain...?

It's not gonna happen.

Stay with just drive-open-drain; and configure them all as that if
that property is set.

Yours,
Linus Walleij

Re: [RFC 1/2] dt-bindings: iio: imu: Add DT binding doc for BMI323

[Jonathan Cameron]

On Tue, 10 Oct 2023 21:51:17 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Tue, Oct 10, 2023 at 4:42 PM Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > We kind of lost the question along the way.  Wasn't so much about whether
> > there was a generic binding but more about whether it is worth providing
> > separate controls for the two IRQ pins?  Or just assume no one is crazy
> > enough to play that level of mix and match.  
> 
> Ugh no, that's upfront design for a nonexistent use case.
> 
> - First, to even consider open drain the designer need to be really
>   short of IRQ lines/rails, and, despite knowing it's a bad idea, decide
>   to share this line between several peripherals, even though it will
>   require I2C traffic to just determine which one even fired the IRQ.
> 
> - Second, be interested in using two IRQs to distinguish between
>   different events? When we just faced the situation that we had
>   too few IRQ lines so we need to start sharing them with open
>   drain...?
> 
> It's not gonna happen.
> 
> Stay with just drive-open-drain; and configure them all as that if
> that property is set.

Good insights, I'd not really thought about the wider reasons for using
this :)  Not done any circuit design or embedded board bring up in a
long while.

Thanks!

> 
> Yours,
> Linus Walleij
> 

Re: [RFC 1/2] dt-bindings: iio: imu: Add DT binding doc for BMI323

[Jagath Jog J]

Hi,

On Fri, Oct 13, 2023 at 1:46 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Tue, 10 Oct 2023 21:51:17 +0200
> Linus Walleij <linus.walleij@linaro.org> wrote:
>
> > On Tue, Oct 10, 2023 at 4:42 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > > We kind of lost the question along the way.  Wasn't so much about whether
> > > there was a generic binding but more about whether it is worth providing
> > > separate controls for the two IRQ pins?  Or just assume no one is crazy
> > > enough to play that level of mix and match.
> >
> > Ugh no, that's upfront design for a nonexistent use case.
> >
> > - First, to even consider open drain the designer need to be really
> >   short of IRQ lines/rails, and, despite knowing it's a bad idea, decide
> >   to share this line between several peripherals, even though it will
> >   require I2C traffic to just determine which one even fired the IRQ.
> >
> > - Second, be interested in using two IRQs to distinguish between
> >   different events? When we just faced the situation that we had
> >   too few IRQ lines so we need to start sharing them with open
> >   drain...?
> >
> > It's not gonna happen.
> >
> > Stay with just drive-open-drain; and configure them all as that if
> > that property is set.
>
> Good insights, I'd not really thought about the wider reasons for using
> this :)  Not done any circuit design or embedded board bring up in a
> long while.
>
> Thanks!

Thank you for the explanation and suggestion.

Regards
Jagath.

>
> >
> > Yours,
> > Linus Walleij
> >
>

[RFC 2/2] iio: imu: Add driver for BMI323 IMU

[Jagath Jog J]

The Bosch BMI323 is a 6-axis low-power IMU that provide measurements for
acceleration, angular rate, and temperature. This sensor includes
motion-triggered interrupt features, such as a step counter, tap detection,
and activity/inactivity interrupt capabilities.

The driver supports various functionalities, including data ready, FIFO
data handling, and events such as tap detection, step counting, and
activity interrupts

Datasheet: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi323-ds000.pdf
Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
---
 MAINTAINERS                          |    7 +
 drivers/iio/imu/Kconfig              |    1 +
 drivers/iio/imu/Makefile             |    1 +
 drivers/iio/imu/bmi323/Kconfig       |   33 +
 drivers/iio/imu/bmi323/Makefile      |    7 +
 drivers/iio/imu/bmi323/bmi323.h      |  198 +++
 drivers/iio/imu/bmi323/bmi323_core.c | 2260 ++++++++++++++++++++++++++
 drivers/iio/imu/bmi323/bmi323_i2c.c  |  115 ++
 drivers/iio/imu/bmi323/bmi323_spi.c  |  106 ++
 9 files changed, 2728 insertions(+)
 create mode 100644 drivers/iio/imu/bmi323/Kconfig
 create mode 100644 drivers/iio/imu/bmi323/Makefile
 create mode 100644 drivers/iio/imu/bmi323/bmi323.h
 create mode 100644 drivers/iio/imu/bmi323/bmi323_core.c
 create mode 100644 drivers/iio/imu/bmi323/bmi323_i2c.c
 create mode 100644 drivers/iio/imu/bmi323/bmi323_spi.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 4fe86f4ec383..7479e187270a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3605,6 +3605,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/accel/bosch,bma400.yaml
 F:	drivers/iio/accel/bma400*
 
+BOSCH SENSORTEC BMI323 IMU IIO DRIVER
+M:	Jagath Jog J <jagathjog1996@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/imu/bosch,bma400.yaml
+F:	drivers/iio/imu/bmi323/
+
 BPF JIT for ARM
 M:	Shubham Bansal <illusionist.neo@gmail.com>
 L:	bpf@vger.kernel.org
diff --git a/drivers/iio/imu/Kconfig b/drivers/iio/imu/Kconfig
index c2f97629e9cd..6c9a85294bc1 100644
--- a/drivers/iio/imu/Kconfig
+++ b/drivers/iio/imu/Kconfig
@@ -54,6 +54,7 @@ config ADIS16480
 
 source "drivers/iio/imu/bmi160/Kconfig"
 source "drivers/iio/imu/bno055/Kconfig"
+source "drivers/iio/imu/bmi323/Kconfig"
 
 config FXOS8700
 	tristate
diff --git a/drivers/iio/imu/Makefile b/drivers/iio/imu/Makefile
index 6eb612034722..627406476357 100644
--- a/drivers/iio/imu/Makefile
+++ b/drivers/iio/imu/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_IIO_ADIS_LIB) += adis_lib.o
 
 obj-y += bmi160/
 obj-y += bno055/
+obj-y += bmi323/
 
 obj-$(CONFIG_FXOS8700) += fxos8700_core.o
 obj-$(CONFIG_FXOS8700_I2C) += fxos8700_i2c.o
diff --git a/drivers/iio/imu/bmi323/Kconfig b/drivers/iio/imu/bmi323/Kconfig
new file mode 100644
index 000000000000..ab37b285393c
--- /dev/null
+++ b/drivers/iio/imu/bmi323/Kconfig
@@ -0,0 +1,33 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# BMI323 IMU driver
+#
+
+config BMI323
+	tristate
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+
+config BMI323_I2C
+	tristate "Bosch BMI323 I2C driver"
+	depends on I2C
+	select BMI323
+	select REGMAP_I2C
+	help
+	  Enable support for the Bosch BMI323 6-Axis IMU connected to I2C
+	  interface.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called bmi323_i2c.
+
+config BMI323_SPI
+	tristate "Bosch BMI323 SPI driver"
+	depends on SPI
+	select BMI323
+	select REGMAP_SPI
+	help
+	  Enable support for the Bosch BMI323 6-Axis IMU connected to SPI
+	  interface.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called bmi323_spi.
diff --git a/drivers/iio/imu/bmi323/Makefile b/drivers/iio/imu/bmi323/Makefile
new file mode 100644
index 000000000000..a6a6dc0207c9
--- /dev/null
+++ b/drivers/iio/imu/bmi323/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for Bosch BMI323 IMU
+#
+obj-$(CONFIG_BMI323) += bmi323_core.o
+obj-$(CONFIG_BMI323_I2C) += bmi323_i2c.o
+obj-$(CONFIG_BMI323_SPI) += bmi323_spi.o
diff --git a/drivers/iio/imu/bmi323/bmi323.h b/drivers/iio/imu/bmi323/bmi323.h
new file mode 100644
index 000000000000..bb04894761ee
--- /dev/null
+++ b/drivers/iio/imu/bmi323/bmi323.h
@@ -0,0 +1,198 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * IIO driver for Bosch BMI323 6-Axis IMU
+ *
+ * Copyright (C) 2023, Jagath Jog J <jagathjog1996@gmail.com>
+ */
+
+#ifndef _BMI323_H_
+#define _BMI323_H_
+
+#include <linux/regmap.h>
+#include <linux/bits.h>
+
+#define BMI323_I2C_DUMMY			2
+#define BMI323_SPI_DUMMY			1
+
+/* Register map */
+
+#define BMI323_CHIP_ID_REG			0x00
+#define BMI323_CHIP_ID_VAL			0x0043
+#define BMI323_ERR_REG				0x01
+#define BMI323_STATUS_REG			0x02
+
+/* Accelero/Gyro/Temp data registers */
+#define BMI323_ACCEL_X_REG			0x03
+#define BMI323_GYRO_X_REG			0x06
+#define BMI323_TEMP_REG				0x09
+
+/* Status registers */
+#define BMI323_STATUS_INT1_REG			0x0D
+#define BMI323_STATUS_INT2_REG			0x0E
+#define BMI323_STATUS_NOMOTION_MSK		BIT(0)
+#define BMI323_STATUS_MOTION_MSK		BIT(1)
+#define BMI323_STATUS_STP_WTR_MSK		BIT(5)
+#define BMI323_STATUS_TAP_MSK			BIT(8)
+#define BMI323_STATUS_ERROR_MSK			BIT(10)
+#define BMI323_STATUS_TMP_DRDY_MSK		BIT(11)
+#define BMI323_STATUS_GYR_DRDY_MSK		BIT(12)
+#define BMI323_STATUS_ACC_DRDY_MSK		BIT(13)
+#define BMI323_STATUS_ACC_GYR_DRDY_MSK		GENMASK(13, 12)
+#define BMI323_STATUS_FIFO_WTRMRK_MSK		BIT(14)
+#define BMI323_STATUS_FIFO_FULL_MSK		BIT(15)
+
+/* Feature registers */
+#define BMI323_FEAT_IO0_REG			0x10
+#define BMI323_FEAT_IO0_XYZ_NOMOTION_MSK	GENMASK(2, 0)
+#define BMI323_FEAT_IO0_XYZ_MOTION_MSK		GENMASK(5, 3)
+#define BMI323_FEAT_IO0_STP_CNT_MSK		BIT(9)
+#define BMI323_FEAT_IO0_S_TAP_MSK		BIT(12)
+#define BMI323_FEAT_IO0_D_TAP_MSK		BIT(13)
+#define BMI323_FEAT_IO1_REG			0x11
+#define BMI323_FEAT_IO2_REG			0x12
+#define BMI323_FEAT_IO_STATUS_REG		0x14
+#define BMI323_FEAT_IO_STATUS_MSK		BIT(0)
+
+/* FIFO registers */
+#define BMI323_FIFO_FILL_LEVEL_REG		0x15
+#define BMI323_FIFO_DATA_REG			0x16
+
+/* Accelero/Gyro config registers */
+#define BMI323_ACC_CONF_REG			0x20
+#define BMI323_GYRO_CONF_REG			0x21
+#define BMI323_ACC_GYRO_CONF_MODE_MSK		GENMASK(14, 12)
+#define BMI323_ACC_GYRO_CONF_ODR_MSK		GENMASK(3, 0)
+#define BMI323_ACC_GYRO_CONF_SCL_MSK		GENMASK(6, 4)
+#define BMI323_ACC_GYRO_CONF_BW_MSK		BIT(7)
+#define BMI323_ACC_GYRO_CONF_AVG_MSK		GENMASK(10, 8)
+
+/* FIFO registers */
+#define BMI323_FIFO_WTRMRK_REG			0x35
+#define BMI323_FIFO_CONF_REG			0x36
+#define BMI323_FIFO_CONF_STP_FUL_MSK		BIT(0)
+#define BMI323_FIFO_CONF_ACC_GYR_EN_MSK		GENMASK(10, 9)
+#define BMI323_FIFO_CTRL_REG			0x37
+#define BMI323_FIFO_FLUSH_MSK			BIT(0)
+
+/* Interrupt pin config registers */
+#define BMI323_IO_INT_CTR_REG			0x38
+#define BMI323_IO_INT1_LVL_MSK			BIT(0)
+#define BMI323_IO_INT1_OD_MSK			BIT(1)
+#define BMI323_IO_INT1_OP_EN_MSK		BIT(2)
+#define BMI323_IO_INT1_LVL_OD_OP_MSK		GENMASK(2, 0)
+#define BMI323_IO_INT2_LVL_MSK			BIT(8)
+#define BMI323_IO_INT2_OD_MSK			BIT(9)
+#define BMI323_IO_INT2_OP_EN_MSK		BIT(10)
+#define BMI323_IO_INT2_LVL_OD_OP_MSK		GENMASK(10, 8)
+#define BMI323_IO_INT_CONF_REG			0x39
+#define BMI323_IO_INT_LTCH_MSK			BIT(0)
+#define BMI323_INT_MAP1_REG			0x3A
+#define BMI323_INT_MAP2_REG			0x3B
+#define BMI323_NOMOTION_MSK			GENMASK(1, 0)
+#define BMI323_MOTION_MSK			GENMASK(3, 2)
+#define BMI323_STEP_CNT_MSK			GENMASK(11, 10)
+#define BMI323_TAP_MSK				GENMASK(1, 0)
+#define BMI323_TMP_DRDY_MSK			GENMASK(7, 6)
+#define BMI323_GYR_DRDY_MSK			GENMASK(9, 8)
+#define BMI323_ACC_DRDY_MSK			GENMASK(11, 10)
+#define BMI323_FIFO_WTRMRK_MSK			GENMASK(13, 12)
+#define BMI323_FIFO_FULL_MSK			GENMASK(15, 14)
+
+/* Feature registers */
+#define BMI323_FEAT_CTRL_REG			0x40
+#define BMI323_FEAT_ENG_EN_MSK			BIT(0)
+#define BMI323_FEAT_DATA_ADDR			0x41
+#define BMI323_FEAT_DATA_TX			0x42
+#define BMI323_FEAT_DATA_STATUS			0x43
+#define BMI323_FEAT_DATA_TX_RDY_MSK		BIT(1)
+#define BMI323_FEAT_EVNT_EXT_REG		0x47
+#define BMI323_FEAT_EVNT_EXT_S_MSK		BIT(3)
+#define BMI323_FEAT_EVNT_EXT_D_MSK		BIT(4)
+
+#define BMI323_CMD_REG				0x7E
+#define BMI323_RST_VAL				0xDEAF
+#define BMI323_CFG_RES_REG			0x7F
+
+/* Extended registers */
+#define BMI323_GEN_SET1_REG			0x02
+#define BMI323_GEN_SET1_MODE_MSK		BIT(0)
+#define BMI323_GEN_HOLD_DUR_MSK			GENMASK(4, 1)
+
+/* Any Motion/No Motion config registers */
+#define BMI323_ANYMO1_REG			0x05
+#define BMI323_NOMO1_REG			0x08
+#define BMI323_MO2_OFFSET			0x01
+#define BMI323_MO3_OFFSET			0x02
+#define BMI323_MO1_REF_UP_MSK			BIT(12)
+#define BMI323_MO1_SLOPE_TH_MSK			GENMASK(11, 0)
+#define BMI323_MO2_HYSTR_MSK			GENMASK(9, 0)
+#define BMI323_MO3_DURA_MSK			GENMASK(12, 0)
+
+/* Step counter config registers */
+#define BMI323_STEP_SC1_REG			0x10
+#define BMI323_STEP_SC1_WTRMRK_MSK		GENMASK(9, 0)
+#define BMI323_STEP_SC1_RST_CNT_MSK		BIT(10)
+#define BMI323_STEP_SC1_REG			0x10
+
+/* Tap gesture config registers */
+#define BMI323_TAP1_REG				0x1E
+#define BMI323_TAP1_AXIS_SEL_MSK		GENMASK(1, 0)
+#define BMI323_TAP1_TIMOUT_MSK			BIT(2)
+#define BMI323_TAP1_MAX_PEAKS_MSK		GENMASK(5, 3)
+#define BMI323_TAP1_MODE_MSK			GENMASK(7, 6)
+#define BMI323_TAP2_REG				0x1F
+#define BMI323_TAP2_THRES_MSK			GENMASK(9, 0)
+#define BMI323_TAP2_MAX_DUR_MSK			GENMASK(15, 10)
+#define BMI323_TAP3_REG				0x20
+#define BMI323_TAP3_QUIET_TIM_MSK		GENMASK(15, 12)
+#define BMI323_TAP3_QT_BW_TAP_MSK		GENMASK(11, 8)
+#define BMI323_TAP3_QT_AFT_GES_MSK		GENMASK(15, 12)
+
+#define BMI323_MOTION_THRES_SCALE		512
+#define BMI323_MOTION_HYSTR_SCALE		512
+#define BMI323_MOTION_DURAT_SCALE		50
+#define BMI323_TAP_THRES_SCALE			512
+#define BMI323_DUR_BW_TAP_SCALE			200
+#define BMI323_QUITE_TIM_GES_SCALE		25
+#define BMI323_MAX_GES_DUR_SCALE		25
+
+/*
+ * The formula to calculate temperature in C.
+ * See datasheet section 6.1.1, Register Map Overview
+ *
+ * T_C = (temp_raw / 512) + 23
+ */
+#define BMI323_TEMP_OFFSET			11776
+#define BMI323_TEMP_SCALE			1953125
+
+/*
+ * The BMI323 features a FIFO with a capacity of 2048 bytes. Each frame
+ * consists of accelerometer (X, Y, Z) data and gyroscope (X, Y, Z) data,
+ * totaling 6 words or 12 bytes. The FIFO buffer can hold a total of
+ * 170 frames.
+ *
+ * If a watermark interrupt is configured for 170 frames, the interrupt will
+ * trigger when the FIFO reaches 169 frames, so limit the maximum watermark
+ * level to 169 frames. In terms of data, 169 frames would equal 1014 bytes,
+ * which is approximately 2 frames before the FIFO reaches its full capacity.
+ * See datasheet section 5.7.3 FIFO Buffer Interrupts
+ */
+#define BMI323_BYTES_PER_SAMPLE			2
+#define BMI323_FIFO_LENGTH_IN_BYTES		2048
+#define BMI323_FIFO_FRAME_LENGTH		6
+#define BMI323_FIFO_FULL_IN_FRAMES		\
+	((BMI323_FIFO_LENGTH_IN_BYTES /		\
+	(BMI323_BYTES_PER_SAMPLE * BMI323_FIFO_FRAME_LENGTH)) - 1)
+#define BMI323_FIFO_FULL_IN_WORDS		\
+	(BMI323_FIFO_FULL_IN_FRAMES * BMI323_FIFO_FRAME_LENGTH)
+
+#define BMI323_INT_MICRO_TO_RAW(val, val2, scale) (((val) * (scale)) + \
+						  (((val2) * (scale)) / MEGA))
+
+#define BMI323_RAW_TO_MICRO(raw, scale) ((((raw) % (scale)) * MEGA) / scale)
+
+struct device;
+int bmi323_core_probe(struct device *dev);
+extern const struct regmap_config bmi323_regmap_config;
+
+#endif
diff --git a/drivers/iio/imu/bmi323/bmi323_core.c b/drivers/iio/imu/bmi323/bmi323_core.c
new file mode 100644
index 000000000000..adbcfbc6b97d
--- /dev/null
+++ b/drivers/iio/imu/bmi323/bmi323_core.c
@@ -0,0 +1,2260 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * IIO core driver for Bosch BMI323 6-Axis IMU.
+ *
+ * Copyright (C) 2023, Jagath Jog J <jagathjog1996@gmail.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/units.h>
+
+#include <asm/unaligned.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/events.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#include "bmi323.h"
+
+enum bmi323_sensor_type {
+	BMI323_ACCEL,
+	BMI323_GYRO,
+};
+
+enum bmi323_opr_mode {
+	ACC_GYRO_MODE_DISABLE = 0x00,
+	GYRO_DRIVE_MODE_ENABLED = 0x01,
+	ACC_GYRO_MODE_DUTYCYCLE = 0x03,
+	ACC_GYRO_MODE_CONTINOUS = 0x04,
+	ACC_GYRO_MODE_HIGH_PERF = 0x07,
+};
+
+enum bmi323_state {
+	BMI323_IDLE,
+	BMI323_BUFFER_DRDY_TRIGGERED,
+	BMI323_BUFFER_FIFO,
+};
+
+enum bmi323_irq_pin {
+	BMI323_IRQ_DISABLED,
+	BMI323_IRQ_INT1,
+	BMI323_IRQ_INT2,
+};
+
+enum bmi323_3db_bw {
+	BMI323_BW_ODR_BY_2,
+	BMI323_BW_ODR_BY_4,
+};
+
+enum bmi323_scan {
+	BMI323_ACCEL_X,
+	BMI323_ACCEL_Y,
+	BMI323_ACCEL_Z,
+	BMI323_GYRO_X,
+	BMI323_GYRO_Y,
+	BMI323_GYRO_Z,
+	BMI323_CHAN_MAX
+};
+
+struct bmi323_hw {
+	u8 data;
+	u8 config;
+	const int (*scale_table)[2];
+	int scale_table_len;
+};
+
+/*
+ * The accelerometer supports +-2G/4G/8G/16G ranges, and the resolution of
+ * each sample is 16 bits, signed.
+ * At +-8G the scale can calculated by
+ * ((8 + 8) * 9.80665 / (2^16 - 1)) * 10^6 = 2394.23819 scale in micro
+ *
+ */
+static const int bmi323_accel_scale[][2] = {
+	{ 0, 598 },
+	{ 0, 1197 },
+	{ 0, 2394 },
+	{ 0, 4788 },
+};
+
+static const int bmi323_gyro_scale[][2] = {
+	{ 0, 66 },
+	{ 0, 133 },
+	{ 0, 266 },
+	{ 0, 532 },
+	{ 0, 1065 },
+};
+
+static const int bmi323_accel_gyro_avrg[] = {0, 2, 4, 8, 16, 32, 64};
+
+static const struct bmi323_hw bmi323_hw[2] = {
+	[BMI323_ACCEL] = {
+		.data = BMI323_ACCEL_X_REG,
+		.config = BMI323_ACC_CONF_REG,
+		.scale_table = bmi323_accel_scale,
+		.scale_table_len = ARRAY_SIZE(bmi323_accel_scale),
+	},
+	[BMI323_GYRO] = {
+		.data = BMI323_GYRO_X_REG,
+		.config = BMI323_GYRO_CONF_REG,
+		.scale_table = bmi323_gyro_scale,
+		.scale_table_len = ARRAY_SIZE(bmi323_gyro_scale),
+	},
+};
+
+struct bmi323_data {
+	struct device *dev;
+	struct regmap *regmap;
+	struct iio_mount_matrix orientation;
+	enum bmi323_irq_pin irq_pin;
+	struct iio_trigger *trig;
+	bool drdy_trigger_enabled;
+	enum bmi323_state state;
+	s64 fifo_tstamp, old_fifo_tstamp;
+	u32 odrns[2];
+	u32 odrhz[2];
+	unsigned int feature_events;
+
+	/*
+	 * Lock to protect the members of device's private data from concurrent
+	 * access and also to serialize the access of extended registers.
+	 * See bmi323_write_ext_reg(..) for more info.
+	 */
+	struct mutex mutex;
+	int watermark;
+	__le16 fifo_buff[BMI323_FIFO_FULL_IN_WORDS] __aligned(IIO_DMA_MINALIGN);
+	struct {
+		__le16 channels[6];
+		s64 ts __aligned(8);
+	} buffer;
+	__le16 steps_count[2];
+};
+
+const struct regmap_config bmi323_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.max_register = BMI323_CFG_RES_REG,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+};
+EXPORT_SYMBOL_NS_GPL(bmi323_regmap_config, IIO_BMI323);
+
+static const struct iio_mount_matrix *
+bmi323_get_mount_matrix(const struct iio_dev *idev,
+			const struct iio_chan_spec *chan)
+{
+	struct bmi323_data *data = iio_priv(idev);
+
+	return &data->orientation;
+}
+
+static const struct iio_chan_spec_ext_info bmi323_ext_info[] = {
+	IIO_MOUNT_MATRIX(IIO_SHARED_BY_TYPE, bmi323_get_mount_matrix),
+	{ }
+};
+
+static const struct iio_event_spec bmi323_step_wtrmrk_event = {
+	.type = IIO_EV_TYPE_CHANGE,
+	.dir = IIO_EV_DIR_NONE,
+	.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
+			       BIT(IIO_EV_INFO_VALUE),
+};
+
+static const struct iio_event_spec bmi323_accel_event[] = {
+	{
+		.type = IIO_EV_TYPE_MAG,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
+				       BIT(IIO_EV_INFO_PERIOD) |
+				       BIT(IIO_EV_INFO_HYSTERESIS) |
+				       BIT(IIO_EV_INFO_ENABLE),
+	},
+	{
+		.type = IIO_EV_TYPE_MAG,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_shared_by_type = BIT(IIO_EV_INFO_VALUE) |
+				       BIT(IIO_EV_INFO_PERIOD) |
+				       BIT(IIO_EV_INFO_HYSTERESIS) |
+				       BIT(IIO_EV_INFO_ENABLE),
+	},
+	{
+		.type = IIO_EV_TYPE_GESTURE,
+		.dir = IIO_EV_DIR_SINGLETAP,
+		.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
+				       BIT(IIO_EV_INFO_VALUE) |
+				       BIT(IIO_EV_INFO_RESET_TIMEOUT),
+	},
+	{
+		.type = IIO_EV_TYPE_GESTURE,
+		.dir = IIO_EV_DIR_DOUBLETAP,
+		.mask_shared_by_type = BIT(IIO_EV_INFO_ENABLE) |
+				       BIT(IIO_EV_INFO_VALUE) |
+				       BIT(IIO_EV_INFO_RESET_TIMEOUT) |
+				       BIT(IIO_EV_INFO_TAP2_MIN_DELAY),
+	},
+};
+
+#define BMI323_ACCEL_CHANNEL(_type, _axis, _index) {			\
+	.type = _type,							\
+	.modified = 1,							\
+	.channel2 = IIO_MOD_##_axis,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) |	\
+				    BIT(IIO_CHAN_INFO_SCALE),		\
+	.info_mask_shared_by_type_available =				\
+				    BIT(IIO_CHAN_INFO_SAMP_FREQ) |	\
+				    BIT(IIO_CHAN_INFO_SCALE),		\
+	.scan_index = _index,						\
+	.scan_type = {							\
+		.sign = 's',						\
+		.realbits = 16,						\
+		.storagebits = 16,					\
+		.endianness = IIO_LE,					\
+	},								\
+	.ext_info = bmi323_ext_info,					\
+	.event_spec = bmi323_accel_event,				\
+	.num_event_specs = ARRAY_SIZE(bmi323_accel_event),		\
+}
+
+#define BMI323_GYRO_CHANNEL(_type, _axis, _index) {			\
+	.type = _type,							\
+	.modified = 1,							\
+	.channel2 = IIO_MOD_##_axis,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ) |	\
+				    BIT(IIO_CHAN_INFO_SCALE),		\
+	.info_mask_shared_by_type_available =				\
+				    BIT(IIO_CHAN_INFO_SAMP_FREQ) |	\
+				    BIT(IIO_CHAN_INFO_SCALE),		\
+	.scan_index = _index,						\
+	.scan_type = {							\
+		.sign = 's',						\
+		.realbits = 16,						\
+		.storagebits = 16,					\
+		.endianness = IIO_LE,					\
+	},								\
+	.ext_info = bmi323_ext_info,					\
+}
+
+static const struct iio_chan_spec bmi323_channels[] = {
+	BMI323_ACCEL_CHANNEL(IIO_ACCEL, X, BMI323_ACCEL_X),
+	BMI323_ACCEL_CHANNEL(IIO_ACCEL, Y, BMI323_ACCEL_Y),
+	BMI323_ACCEL_CHANNEL(IIO_ACCEL, Z, BMI323_ACCEL_Z),
+	BMI323_GYRO_CHANNEL(IIO_ANGL_VEL, X, BMI323_GYRO_X),
+	BMI323_GYRO_CHANNEL(IIO_ANGL_VEL, Y, BMI323_GYRO_Y),
+	BMI323_GYRO_CHANNEL(IIO_ANGL_VEL, Z, BMI323_GYRO_Z),
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_OFFSET) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = -1,
+	},
+	{
+		.type = IIO_STEPS,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_ENABLE),
+		.scan_index = -1,
+		.event_spec = &bmi323_step_wtrmrk_event,
+		.num_event_specs = 1,
+
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(BMI323_CHAN_MAX),
+};
+
+static const int bmi323_acc_gyro_odr[][2] = {
+	{ 0, 781250 },
+	{ 1, 562500 },
+	{ 3, 125000 },
+	{ 6, 250000 },
+	{ 12, 500000 },
+	{ 25, 0 },
+	{ 50, 0 },
+	{ 100, 0 },
+	{ 200, 0 },
+	{ 400, 0 },
+	{ 800, 0 },
+};
+
+static const int bmi323_acc_gyro_odrns[] = {
+	1280 * MEGA,
+	640 * MEGA,
+	320 * MEGA,
+	160 * MEGA,
+	80 * MEGA,
+	40 * MEGA,
+	20 * MEGA,
+	10 * MEGA,
+	5 * MEGA,
+	2500000,
+	1250000,
+};
+
+static enum bmi323_sensor_type bmi323_iio_to_sensor(enum iio_chan_type iio_type)
+{
+	switch (iio_type) {
+	case IIO_ACCEL:
+		return BMI323_ACCEL;
+	case IIO_ANGL_VEL:
+		return BMI323_GYRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bmi323_set_mode(struct bmi323_data *data,
+			   enum bmi323_sensor_type sensor,
+			   enum bmi323_opr_mode mode)
+{
+	return regmap_update_bits(data->regmap, bmi323_hw[sensor].config,
+				  BMI323_ACC_GYRO_CONF_MODE_MSK,
+				  FIELD_PREP(BMI323_ACC_GYRO_CONF_MODE_MSK,
+					     mode));
+}
+
+/*
+ * When writing data to extended register there must be no communication to
+ * any other register before write transaction is complete.
+ * See datasheet section 6.2 Extended Register Map Description.
+ */
+static int bmi323_write_ext_reg(struct bmi323_data *data, unsigned int ext_addr,
+				unsigned int ext_data)
+{
+	int ret, feature_status;
+
+	mutex_lock(&data->mutex);
+	ret = regmap_read(data->regmap, BMI323_FEAT_DATA_STATUS,
+			  &feature_status);
+	if (ret)
+		goto unlock_out;
+
+	if (!FIELD_GET(BMI323_FEAT_DATA_TX_RDY_MSK, feature_status)) {
+		ret = -EBUSY;
+		goto unlock_out;
+	}
+
+	ret = regmap_write(data->regmap, BMI323_FEAT_DATA_ADDR, ext_addr);
+	if (ret)
+		goto unlock_out;
+
+	ret = regmap_write(data->regmap, BMI323_FEAT_DATA_TX, ext_data);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+	return ret;
+}
+
+/*
+ * When reading data from extended register there must be no communication to
+ * any other register before read transaction is complete.
+ * See datasheet section 6.2 Extended Register Map Description.
+ */
+static int bmi323_read_ext_reg(struct bmi323_data *data, unsigned int ext_addr,
+			       unsigned int *ext_data)
+{
+	int ret, feature_status;
+
+	mutex_lock(&data->mutex);
+	ret = regmap_read(data->regmap, BMI323_FEAT_DATA_STATUS,
+			  &feature_status);
+	if (ret)
+		goto unlock_out;
+
+	if (!FIELD_GET(BMI323_FEAT_DATA_TX_RDY_MSK, feature_status)) {
+		ret = -EBUSY;
+		goto unlock_out;
+	}
+
+	ret = regmap_write(data->regmap, BMI323_FEAT_DATA_ADDR, ext_addr);
+	if (ret)
+		goto unlock_out;
+
+	ret = regmap_read(data->regmap, BMI323_FEAT_DATA_TX, ext_data);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+	return ret;
+}
+
+static int bmi323_update_ext_reg(struct bmi323_data *data,
+				 unsigned int ext_addr,
+				 unsigned int mask, unsigned int ext_data)
+{
+	unsigned int value;
+	int ret;
+
+	ret = bmi323_read_ext_reg(data, ext_addr, &value);
+	if (ret)
+		return ret;
+
+	set_mask_bits(&value, mask, ext_data);
+
+	ret = bmi323_write_ext_reg(data, ext_addr, value);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int bmi323_get_error_status(struct bmi323_data *data)
+{
+	int error, ret;
+
+	ret = regmap_read(data->regmap, BMI323_ERR_REG, &error);
+	if (ret)
+		return ret;
+
+	if (error)
+		dev_err(data->dev, "Sensor error 0x%x\n", error);
+
+	return error;
+}
+
+static int bmi323_feature_engine_events(struct bmi323_data *data,
+					const unsigned int event_mask,
+					bool state)
+{
+	unsigned int value;
+	int ret;
+
+	ret = regmap_read(data->regmap, BMI323_FEAT_IO0_REG, &value);
+	if (ret)
+		return ret;
+
+	/* Register must be cleared before changing an active config */
+	ret = regmap_write(data->regmap, BMI323_FEAT_IO0_REG, 0);
+	if (ret)
+		return ret;
+
+	if (state)
+		value |= event_mask;
+	else
+		value &= ~event_mask;
+
+	ret = regmap_write(data->regmap, BMI323_FEAT_IO0_REG, value);
+	if (ret)
+		return ret;
+
+	return regmap_write(data->regmap, BMI323_FEAT_IO_STATUS_REG,
+			    BMI323_FEAT_IO_STATUS_MSK);
+}
+
+static int bmi323_step_wtrmrk_en(struct bmi323_data *data, int state)
+{
+	enum bmi323_irq_pin step_irq;
+	int ret;
+
+	mutex_lock(&data->mutex);
+	if (!FIELD_GET(BMI323_FEAT_IO0_STP_CNT_MSK, data->feature_events)) {
+		mutex_unlock(&data->mutex);
+		return -EINVAL;
+	}
+	mutex_unlock(&data->mutex);
+
+	if (state)
+		step_irq = data->irq_pin;
+	else
+		step_irq = BMI323_IRQ_DISABLED;
+
+	ret = bmi323_update_ext_reg(data, BMI323_STEP_SC1_REG,
+				    BMI323_STEP_SC1_WTRMRK_MSK,
+				    FIELD_PREP(BMI323_STEP_SC1_WTRMRK_MSK,
+					       state ? 1 : 0));
+	if (ret)
+		return ret;
+
+	mutex_lock(&data->mutex);
+	ret = regmap_update_bits(data->regmap, BMI323_INT_MAP1_REG,
+				 BMI323_STEP_CNT_MSK,
+				 FIELD_PREP(BMI323_STEP_CNT_MSK, step_irq));
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int bmi323_motion_config_reg(enum iio_event_direction dir)
+{
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		return BMI323_ANYMO1_REG;
+	case IIO_EV_DIR_FALLING:
+		return BMI323_NOMO1_REG;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bmi323_motion_event_en(struct bmi323_data *data,
+				  enum iio_event_direction dir, int state)
+{
+	int config, ret, msk, raw, field_value = 0;
+	enum bmi323_irq_pin motion_irq;
+	int irq_msk, irq_field_val;
+
+	if (state)
+		motion_irq = data->irq_pin;
+	else
+		motion_irq = BMI323_IRQ_DISABLED;
+
+	switch (dir) {
+	case IIO_EV_DIR_RISING:
+		msk = BMI323_FEAT_IO0_XYZ_MOTION_MSK;
+		raw = 512;
+		config = BMI323_ANYMO1_REG;
+		irq_msk = BMI323_MOTION_MSK;
+		set_mask_bits(&irq_field_val, BMI323_MOTION_MSK,
+			      FIELD_PREP(BMI323_MOTION_MSK, motion_irq));
+		set_mask_bits(&field_value, BMI323_FEAT_IO0_XYZ_MOTION_MSK,
+			      FIELD_PREP(BMI323_FEAT_IO0_XYZ_MOTION_MSK,
+					 state ? 7 : 0));
+		break;
+	case IIO_EV_DIR_FALLING:
+		msk = BMI323_FEAT_IO0_XYZ_NOMOTION_MSK;
+		raw = 0;
+		config = BMI323_NOMO1_REG;
+		irq_msk = BMI323_NOMOTION_MSK;
+		set_mask_bits(&irq_field_val, BMI323_NOMOTION_MSK,
+			      FIELD_PREP(BMI323_NOMOTION_MSK, motion_irq));
+		set_mask_bits(&field_value, BMI323_FEAT_IO0_XYZ_NOMOTION_MSK,
+			      FIELD_PREP(BMI323_FEAT_IO0_XYZ_NOMOTION_MSK,
+					 state ? 7 : 0));
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	mutex_lock(&data->mutex);
+	ret = bmi323_feature_engine_events(data, msk, state);
+	mutex_unlock(&data->mutex);
+	if (ret)
+		return ret;
+
+	ret = bmi323_update_ext_reg(data, config,
+				    BMI323_MO1_REF_UP_MSK,
+				    FIELD_PREP(BMI323_MO1_REF_UP_MSK, 0));
+	if (ret)
+		return ret;
+
+	/* Set initial value to avoid interrupts while enabling*/
+	ret = bmi323_update_ext_reg(data, config,
+				    BMI323_MO1_SLOPE_TH_MSK,
+				    FIELD_PREP(BMI323_MO1_SLOPE_TH_MSK, raw));
+	if (ret)
+		return ret;
+
+	mutex_lock(&data->mutex);
+	ret = regmap_update_bits(data->regmap, BMI323_INT_MAP1_REG, irq_msk,
+				 irq_field_val);
+	if (ret)
+		goto unlock_out;
+
+	set_mask_bits(&data->feature_events, msk, field_value);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+	return ret;
+}
+
+static int bmi323_tap_event_en(struct bmi323_data *data,
+			       enum iio_event_direction dir, int state)
+{
+	enum bmi323_irq_pin tap_irq;
+	int ret, tap_enabled;
+
+	mutex_lock(&data->mutex);
+	if (data->odrhz[BMI323_ACCEL] < 200) {
+		dev_err(data->dev, "Invalid accelrometer parameter\n");
+		ret = -EINVAL;
+		goto unlock_out;
+	}
+
+	switch (dir) {
+	case IIO_EV_DIR_SINGLETAP:
+		ret = bmi323_feature_engine_events(data,
+						   BMI323_FEAT_IO0_S_TAP_MSK,
+						   state);
+		if (ret)
+			goto unlock_out;
+
+		set_mask_bits(&data->feature_events, BMI323_FEAT_IO0_S_TAP_MSK,
+			      FIELD_PREP(BMI323_FEAT_IO0_S_TAP_MSK, state));
+		break;
+	case IIO_EV_DIR_DOUBLETAP:
+		ret = bmi323_feature_engine_events(data,
+						   BMI323_FEAT_IO0_D_TAP_MSK,
+						   state);
+		if (ret)
+			goto unlock_out;
+
+		set_mask_bits(&data->feature_events, BMI323_FEAT_IO0_D_TAP_MSK,
+			      FIELD_PREP(BMI323_FEAT_IO0_D_TAP_MSK, state));
+		break;
+	default:
+		ret = -EINVAL;
+		goto unlock_out;
+	}
+
+	tap_enabled = FIELD_GET(BMI323_FEAT_IO0_S_TAP_MSK |
+				BMI323_FEAT_IO0_D_TAP_MSK,
+				data->feature_events);
+
+	if (tap_enabled)
+		tap_irq = data->irq_pin;
+	else
+		tap_irq = BMI323_IRQ_DISABLED;
+
+	ret = regmap_update_bits(data->regmap, BMI323_INT_MAP2_REG,
+				 BMI323_TAP_MSK,
+				 FIELD_PREP(BMI323_TAP_MSK, tap_irq));
+	if (ret)
+		goto unlock_out;
+
+	mutex_unlock(&data->mutex);
+
+	if (state) {
+		ret = bmi323_update_ext_reg(data, BMI323_TAP1_REG,
+					    BMI323_TAP1_MAX_PEAKS_MSK,
+					    FIELD_PREP(BMI323_TAP1_MAX_PEAKS_MSK,
+						       0x04));
+		if (ret)
+			return ret;
+
+		ret = bmi323_update_ext_reg(data, BMI323_TAP1_REG,
+					    BMI323_TAP1_AXIS_SEL_MSK,
+					    FIELD_PREP(BMI323_TAP1_AXIS_SEL_MSK,
+						       0x03));
+		if (ret)
+			return ret;
+
+		return bmi323_update_ext_reg(data, BMI323_TAP1_REG,
+					     BMI323_TAP1_TIMOUT_MSK,
+					     FIELD_PREP(BMI323_TAP1_TIMOUT_MSK,
+							0));
+	}
+
+	return ret;
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+	return ret;
+}
+
+static ssize_t in_accel_gesture_tap_max_dur_show(struct device *dev,
+						 struct device_attribute *attr,
+						 char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct bmi323_data *data = iio_priv(indio_dev);
+	unsigned int reg_value, raw;
+	int ret, val[2];
+
+	ret = bmi323_read_ext_reg(data, BMI323_TAP2_REG, &reg_value);
+	if (ret)
+		return ret;
+
+	raw = FIELD_GET(BMI323_TAP2_MAX_DUR_MSK, reg_value);
+	val[0] = raw / BMI323_MAX_GES_DUR_SCALE;
+	val[1] = BMI323_RAW_TO_MICRO(raw, BMI323_MAX_GES_DUR_SCALE);
+
+	return iio_format_value(buf, IIO_VAL_INT_PLUS_MICRO, 2, val);
+}
+
+static ssize_t in_accel_gesture_tap_max_dur_store(struct device *dev,
+						  struct device_attribute *attr,
+						  const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct bmi323_data *data = iio_priv(indio_dev);
+	int ret, val_int, val_fract, raw;
+
+	ret = iio_str_to_fixpoint(buf, 100000, &val_int, &val_fract);
+	if (ret)
+		return ret;
+
+	raw = BMI323_INT_MICRO_TO_RAW(val_int, val_fract,
+				      BMI323_MAX_GES_DUR_SCALE);
+	if (raw < 0 || raw > 63)
+		return -EINVAL;
+
+	ret = bmi323_update_ext_reg(data, BMI323_TAP2_REG,
+				    BMI323_TAP2_MAX_DUR_MSK,
+				    FIELD_PREP(BMI323_TAP2_MAX_DUR_MSK, raw));
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+/*
+ * Maximum duration from first tap within the second tap is expected to happen.
+ * This timeout is applicable only if gesture_tap_timeout is enabled.
+ */
+static IIO_DEVICE_ATTR_RW(in_accel_gesture_tap_max_dur, 0);
+
+static ssize_t in_accel_gesture_tap_timeout_show(struct device *dev,
+						 struct device_attribute *attr,
+						 char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct bmi323_data *data = iio_priv(indio_dev);
+	unsigned int reg_value, raw;
+	int ret;
+
+	ret = bmi323_read_ext_reg(data, BMI323_TAP1_REG, &reg_value);
+	if (ret)
+		return ret;
+
+	raw = FIELD_GET(BMI323_TAP1_TIMOUT_MSK, reg_value);
+
+	return iio_format_value(buf, IIO_VAL_INT, 1, &raw);
+}
+
+static ssize_t in_accel_gesture_tap_timeout_store(struct device *dev,
+						  struct device_attribute *attr,
+						  const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct bmi323_data *data = iio_priv(indio_dev);
+	int ret, val;
+
+	if (kstrtoint(buf, 10, &val))
+		return -EINVAL;
+
+	if (!(val == 0 || val == 1))
+		return -EINVAL;
+
+	ret = bmi323_update_ext_reg(data, BMI323_TAP1_REG,
+				    BMI323_TAP1_TIMOUT_MSK,
+				    FIELD_PREP(BMI323_TAP1_TIMOUT_MSK, val));
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+/* Enable/disable gesture confirmation with wait time */
+static IIO_DEVICE_ATTR_RW(in_accel_gesture_tap_timeout, 0);
+
+static IIO_CONST_ATTR(in_accel_gesture_tap_max_dur_available,
+		      "[0.0 0.04 2.52]");
+
+static IIO_CONST_ATTR(in_accel_gesture_doubletap_tap2_min_delay_available,
+		      "[0.005 0.005 0.075]");
+
+static IIO_CONST_ATTR(in_accel_gesture_tap_reset_timeout_available,
+		      "[0.04 0.04 0.6]");
+
+static IIO_CONST_ATTR(in_accel_gesture_tap_value_available, "[0.0 0.002 1.99]");
+
+static IIO_CONST_ATTR(in_accel_mag_value_available, "[0.0 0.002 7.99]");
+
+static IIO_CONST_ATTR(in_accel_mag_period_available, "[0.0 0.02 162.0]");
+
+static IIO_CONST_ATTR(in_accel_mag_hysteresis_available, "[0.0 0.002 1.99]");
+
+static struct attribute *bmi323_event_attributes[] = {
+	&iio_const_attr_in_accel_gesture_tap_value_available.dev_attr.attr,
+	&iio_const_attr_in_accel_gesture_tap_reset_timeout_available.dev_attr.attr,
+	&iio_const_attr_in_accel_gesture_doubletap_tap2_min_delay_available.dev_attr.attr,
+	&iio_const_attr_in_accel_gesture_tap_max_dur_available.dev_attr.attr,
+	&iio_dev_attr_in_accel_gesture_tap_timeout.dev_attr.attr,
+	&iio_dev_attr_in_accel_gesture_tap_max_dur.dev_attr.attr,
+	&iio_const_attr_in_accel_mag_value_available.dev_attr.attr,
+	&iio_const_attr_in_accel_mag_period_available.dev_attr.attr,
+	&iio_const_attr_in_accel_mag_hysteresis_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group bmi323_event_attribute_group = {
+	.attrs = bmi323_event_attributes,
+};
+
+static int bmi323_write_event_config(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir, int state)
+{
+	struct bmi323_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (type) {
+	case IIO_EV_TYPE_MAG:
+		ret = bmi323_motion_event_en(data, dir, state);
+		return ret;
+	case IIO_EV_TYPE_GESTURE:
+		return bmi323_tap_event_en(data, dir, state);
+	case IIO_EV_TYPE_CHANGE:
+		ret = bmi323_step_wtrmrk_en(data, state);
+		return ret;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bmi323_read_event_config(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan,
+				    enum iio_event_type type,
+				    enum iio_event_direction dir)
+{
+	struct bmi323_data *data = iio_priv(indio_dev);
+	int ret, value, reg_val;
+
+	switch (chan->type) {
+	case IIO_ACCEL:
+		mutex_lock(&data->mutex);
+		switch (dir) {
+		case IIO_EV_DIR_SINGLETAP:
+			ret = FIELD_GET(BMI323_FEAT_IO0_S_TAP_MSK,
+					data->feature_events);
+			break;
+		case IIO_EV_DIR_DOUBLETAP:
+			ret = FIELD_GET(BMI323_FEAT_IO0_D_TAP_MSK,
+					data->feature_events);
+			break;
+		case IIO_EV_DIR_RISING:
+			value = FIELD_GET(BMI323_FEAT_IO0_XYZ_MOTION_MSK,
+					  data->feature_events);
+			ret = value ? 1 : 0;
+			break;
+		case IIO_EV_DIR_FALLING:
+			value = FIELD_GET(BMI323_FEAT_IO0_XYZ_NOMOTION_MSK,
+					  data->feature_events);
+			ret = value ? 1 : 0;
+			break;
+		default:
+			ret = -EINVAL;
+			break;
+		}
+		mutex_unlock(&data->mutex);
+		return ret;
+	case IIO_STEPS:
+		mutex_lock(&data->mutex);
+		ret = regmap_read(data->regmap, BMI323_INT_MAP1_REG, &reg_val);
+		mutex_unlock(&data->mutex);
+		if (ret)
+			return ret;
+
+		return FIELD_GET(BMI323_STEP_CNT_MSK, reg_val) ? 1 : 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bmi323_write_event_value(struct iio_dev *indio_dev,
+				    const struct iio_chan_spec *chan,
+				    enum iio_event_type type,
+				    enum iio_event_direction dir,
+				    enum iio_event_info info,
+				    int val, int val2)
+{
+	struct bmi323_data *data = iio_priv(indio_dev);
+	unsigned int raw;
+	int reg;
+
+	switch (type) {
+	case IIO_EV_TYPE_GESTURE:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			if (!(val == 0 || val == 1))
+				return -EINVAL;
+
+			raw = BMI323_INT_MICRO_TO_RAW(val, val2,
+						      BMI323_TAP_THRES_SCALE);
+
+			return bmi323_update_ext_reg(data, BMI323_TAP2_REG,
+						     BMI323_TAP2_THRES_MSK,
+						     FIELD_PREP(BMI323_TAP2_THRES_MSK,
+								raw));
+		case IIO_EV_INFO_RESET_TIMEOUT:
+			if (val != 0 || val2 < 40000 || val2 > 600000)
+				return -EINVAL;
+
+			raw = BMI323_INT_MICRO_TO_RAW(val, val2,
+						      BMI323_QUITE_TIM_GES_SCALE);
+
+			return bmi323_update_ext_reg(data, BMI323_TAP3_REG,
+						     BMI323_TAP3_QT_AFT_GES_MSK,
+						     FIELD_PREP(BMI323_TAP3_QT_AFT_GES_MSK,
+								raw));
+		case IIO_EV_INFO_TAP2_MIN_DELAY:
+			if (val != 0 || val2 < 5000 || val2 > 75000)
+				return -EINVAL;
+
+			raw = BMI323_INT_MICRO_TO_RAW(val, val2,
+						      BMI323_DUR_BW_TAP_SCALE);
+
+			return bmi323_update_ext_reg(data, BMI323_TAP3_REG,
+						     BMI323_TAP3_QT_BW_TAP_MSK,
+						     FIELD_PREP(BMI323_TAP3_QT_BW_TAP_MSK,
+								raw));
+		default:
+			return -EINVAL;
+		}
+	case IIO_EV_TYPE_MAG:
+		reg = bmi323_motion_config_reg(dir);
+		if (reg < 0)
+			return -EINVAL;
+
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			if (val < 0 || val > 7)
+				return -EINVAL;
+
+			raw = BMI323_INT_MICRO_TO_RAW(val, val2,
+						      BMI323_MOTION_THRES_SCALE);
+
+			return bmi323_update_ext_reg(data, reg,
+						     BMI323_MO1_SLOPE_TH_MSK,
+						     FIELD_PREP(BMI323_MO1_SLOPE_TH_MSK,
+								raw));
+		case IIO_EV_INFO_PERIOD:
+			if (val < 0 || val > 162)
+				return -EINVAL;
+
+			raw = BMI323_INT_MICRO_TO_RAW(val, val2,
+						      BMI323_MOTION_DURAT_SCALE);
+
+			return bmi323_update_ext_reg(data,
+						     reg + BMI323_MO3_OFFSET,
+						     BMI323_MO3_DURA_MSK,
+						     FIELD_PREP(BMI323_MO3_DURA_MSK,
+								raw));
+		case IIO_EV_INFO_HYSTERESIS:
+			if (!(val == 0 || val == 1))
+				return -EINVAL;
+
+			raw = BMI323_INT_MICRO_TO_RAW(val, val2,
+						      BMI323_MOTION_HYSTR_SCALE);
+
+			return bmi323_update_ext_reg(data,
+						     reg + BMI323_MO2_OFFSET,
+						     BMI323_MO2_HYSTR_MSK,
+						     FIELD_PREP(BMI323_MO2_HYSTR_MSK,
+								raw));
+		default:
+			return -EINVAL;
+		}
+	case IIO_EV_TYPE_CHANGE:
+		if (val < 0 || val > 20460)
+			return -EINVAL;
+
+		raw = val / 20;
+		return bmi323_update_ext_reg(data, BMI323_STEP_SC1_REG,
+					     BMI323_STEP_SC1_WTRMRK_MSK,
+					     FIELD_PREP(BMI323_STEP_SC1_WTRMRK_MSK,
+							raw));
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bmi323_read_event_value(struct iio_dev *indio_dev,
+				   const struct iio_chan_spec *chan,
+				   enum iio_event_type type,
+				   enum iio_event_direction dir,
+				   enum iio_event_info info,
+				   int *val, int *val2)
+{
+	struct bmi323_data *data = iio_priv(indio_dev);
+	unsigned int raw, reg_value;
+	int ret, reg;
+
+	switch (type) {
+	case IIO_EV_TYPE_GESTURE:
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			ret = bmi323_read_ext_reg(data, BMI323_TAP2_REG,
+						  &reg_value);
+			if (ret)
+				return ret;
+
+			raw = FIELD_GET(BMI323_TAP2_THRES_MSK, reg_value);
+			*val = raw / BMI323_TAP_THRES_SCALE;
+			*val2 = BMI323_RAW_TO_MICRO(raw, BMI323_TAP_THRES_SCALE);
+			return IIO_VAL_INT_PLUS_MICRO;
+		case IIO_EV_INFO_RESET_TIMEOUT:
+			ret = bmi323_read_ext_reg(data, BMI323_TAP3_REG,
+						  &reg_value);
+			if (ret)
+				return ret;
+
+			raw = FIELD_GET(BMI323_TAP3_QT_AFT_GES_MSK, reg_value);
+			*val = 0;
+			*val2 = BMI323_RAW_TO_MICRO(raw,
+						    BMI323_QUITE_TIM_GES_SCALE);
+			return IIO_VAL_INT_PLUS_MICRO;
+		case IIO_EV_INFO_TAP2_MIN_DELAY:
+			ret = bmi323_read_ext_reg(data, BMI323_TAP3_REG,
+						  &reg_value);
+			if (ret)
+				return ret;
+
+			raw = FIELD_GET(BMI323_TAP3_QT_BW_TAP_MSK, reg_value);
+			*val = 0;
+			*val2 = BMI323_RAW_TO_MICRO(raw,
+						    BMI323_DUR_BW_TAP_SCALE);
+			return IIO_VAL_INT_PLUS_MICRO;
+		default:
+			return -EINVAL;
+		}
+	case IIO_EV_TYPE_MAG:
+		reg = bmi323_motion_config_reg(dir);
+		if (reg < 0)
+			return -EINVAL;
+
+		switch (info) {
+		case IIO_EV_INFO_VALUE:
+			ret = bmi323_read_ext_reg(data, reg, &reg_value);
+			if (ret)
+				return ret;
+
+			raw = FIELD_GET(BMI323_MO1_SLOPE_TH_MSK, reg_value);
+			*val = raw / BMI323_MOTION_THRES_SCALE;
+			*val2 = BMI323_RAW_TO_MICRO(raw,
+						    BMI323_MOTION_THRES_SCALE);
+			return IIO_VAL_INT_PLUS_MICRO;
+		case IIO_EV_INFO_PERIOD:
+			ret = bmi323_read_ext_reg(data,
+						  reg + BMI323_MO3_OFFSET,
+						  &reg_value);
+			if (ret)
+				return ret;
+
+			raw = FIELD_GET(BMI323_MO3_DURA_MSK, reg_value);
+			*val = raw / BMI323_MOTION_DURAT_SCALE;
+			*val2 = BMI323_RAW_TO_MICRO(raw,
+						    BMI323_MOTION_DURAT_SCALE);
+			return IIO_VAL_INT_PLUS_MICRO;
+		case IIO_EV_INFO_HYSTERESIS:
+			ret = bmi323_read_ext_reg(data,
+						  reg + BMI323_MO2_OFFSET,
+						  &reg_value);
+			if (ret)
+				return ret;
+
+			raw = FIELD_GET(BMI323_MO2_HYSTR_MSK, reg_value);
+			*val = raw / BMI323_MOTION_HYSTR_SCALE;
+			*val2 = BMI323_RAW_TO_MICRO(raw,
+						    BMI323_MOTION_HYSTR_SCALE);
+			return IIO_VAL_INT_PLUS_MICRO;
+		default:
+			return -EINVAL;
+		}
+	case IIO_EV_TYPE_CHANGE:
+		ret = bmi323_read_ext_reg(data, BMI323_STEP_SC1_REG,
+					  &reg_value);
+		if (ret)
+			return ret;
+
+		raw = FIELD_GET(BMI323_STEP_SC1_WTRMRK_MSK, reg_value);
+		*val = raw * 20;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int __bmi323_fifo_flush(struct iio_dev *indio_dev)
+{
+	struct bmi323_data *data = iio_priv(indio_dev);
+	int i, ret, fifo_lvl, frame_count, bit;
+	__le16 *frame, *channels;
+	u64 sample_period;
+	s64 tstamp;
+
+	mutex_lock(&data->mutex);
+	ret = regmap_read(data->regmap, BMI323_FIFO_FILL_LEVEL_REG, &fifo_lvl);
+	if (ret)
+		goto unlock_out;
+
+	if (fifo_lvl > BMI323_FIFO_FULL_IN_WORDS)
+		fifo_lvl = BMI323_FIFO_FULL_IN_WORDS;
+
+	frame_count = fifo_lvl / BMI323_FIFO_FRAME_LENGTH;
+	if (!frame_count) {
+		ret = -EINVAL;
+		goto unlock_out;
+	}
+
+	if (fifo_lvl % BMI323_FIFO_FRAME_LENGTH)
+		dev_warn(data->dev, "Bad FIFO alignment\n");
+
+	/*
+	 * Approximate timestamps for each of the sample based on the sampling
+	 * frequency, timestamp for last sample and number of samples.
+	 */
+	if (data->old_fifo_tstamp) {
+		sample_period = data->fifo_tstamp - data->old_fifo_tstamp;
+		do_div(sample_period, frame_count);
+	} else {
+		sample_period = data->odrns[BMI323_ACCEL];
+	}
+
+	tstamp = data->fifo_tstamp - (frame_count - 1) * sample_period;
+
+	ret = regmap_noinc_read(data->regmap, BMI323_FIFO_DATA_REG,
+				&data->fifo_buff[0],
+				fifo_lvl * BMI323_BYTES_PER_SAMPLE);
+	if (ret)
+		goto unlock_out;
+
+	for (i = 0; i < frame_count; i++) {
+		frame = &data->fifo_buff[i * BMI323_FIFO_FRAME_LENGTH];
+		channels = &data->buffer.channels[0];
+
+		for_each_set_bit(bit, indio_dev->active_scan_mask,
+				 BMI323_CHAN_MAX) {
+			channels[bit] = frame[bit];
+		}
+
+		iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
+						   tstamp);
+
+		tstamp += sample_period;
+	}
+
+	ret = frame_count;
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+	return ret;
+}
+
+static int bmi323_set_watermark(struct iio_dev *indio_dev, unsigned int val)
+{
+	struct bmi323_data *data = iio_priv(indio_dev);
+
+	if (val > BMI323_FIFO_FULL_IN_FRAMES)
+		val = BMI323_FIFO_FULL_IN_FRAMES;
+
+	mutex_lock(&data->mutex);
+	data->watermark = val;
+	mutex_unlock(&data->mutex);
+
+	return 0;
+}
+
+static int bmi323_fifo_disable(struct bmi323_data *data)
+{
+	int ret;
+
+	mutex_lock(&data->mutex);
+	ret = regmap_write(data->regmap, BMI323_FIFO_CONF_REG, 0);
+	if (ret)
+		goto unlock_out;
+
+	ret = regmap_update_bits(data->regmap, BMI323_INT_MAP2_REG,
+				 BMI323_FIFO_WTRMRK_MSK,
+				 FIELD_PREP(BMI323_FIFO_WTRMRK_MSK, 0));
+	if (ret)
+		goto unlock_out;
+
+	data->fifo_tstamp = 0;
+	data->state = BMI323_IDLE;
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+	return ret;
+}
+
+static int bmi323_buffer_predisable(struct iio_dev *indio_dev)
+{
+	struct bmi323_data *data = iio_priv(indio_dev);
+
+	if (iio_device_get_current_mode(indio_dev) == INDIO_BUFFER_TRIGGERED)
+		return 0;
+
+	return bmi323_fifo_disable(data);
+}
+
+static int bmi323_update_watermark(struct bmi323_data *data)
+{
+	int wtrmrk;
+
+	wtrmrk = data->watermark * BMI323_FIFO_FRAME_LENGTH;
+
+	return regmap_write(data->regmap, BMI323_FIFO_WTRMRK_REG, wtrmrk);
+}
+
+static int bmi323_fifo_enable(struct bmi323_data *data)
+{
+	int ret;
+
+	mutex_lock(&data->mutex);
+	ret = regmap_update_bits(data->regmap, BMI323_FIFO_CONF_REG,
+				 BMI323_FIFO_CONF_ACC_GYR_EN_MSK,
+				 FIELD_PREP(BMI323_FIFO_CONF_ACC_GYR_EN_MSK,
+					    3));
+	if (ret)
+		goto unlock_out;
+
+	ret = regmap_update_bits(data->regmap, BMI323_INT_MAP2_REG,
+				 BMI323_FIFO_WTRMRK_MSK,
+				 FIELD_PREP(BMI323_FIFO_WTRMRK_MSK,
+					    data->irq_pin));
+	if (ret)
+		goto unlock_out;
+
+	ret = bmi323_update_watermark(data);
+	if (ret)
+		goto unlock_out;
+
+	ret = regmap_write(data->regmap, BMI323_FIFO_CTRL_REG,
+			   BMI323_FIFO_FLUSH_MSK);
+	if (ret)
+		goto unlock_out;
+
+	data->state = BMI323_BUFFER_FIFO;
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+	return ret;
+}
+
+static int bmi323_buffer_preenable(struct iio_dev *indio_dev)
+{
+	struct bmi323_data *data = iio_priv(indio_dev);
+	int ret = 0;
+
+	mutex_lock(&data->mutex);
+	/*
+	 * When the ODR of the accelerometer and gyroscope do not match, the
+	 * maximum ODR value between the accelerometer and gyroscope is used
+	 * for FIFO and the signal with lower ODR will insert dummy frame.
+	 * So allow buffer read only when ODR's of accelero and gyro are equal.
+	 * See datasheet section 5.7 "FIFO Data Buffering".
+	 */
+	if (data->odrns[BMI323_ACCEL] != data->odrns[BMI323_GYRO]) {
+		dev_err(data->dev, "Accelero and Gyro ODR doesn't match\n");
+		ret = -EINVAL;
+	}
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int bmi323_buffer_postenable(struct iio_dev *indio_dev)
+{
+	struct bmi323_data *data = iio_priv(indio_dev);
+
+	if (iio_device_get_current_mode(indio_dev) == INDIO_BUFFER_TRIGGERED)
+		return 0;
+
+	return bmi323_fifo_enable(data);
+}
+
+static ssize_t hwfifo_watermark_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct bmi323_data *data = iio_priv(indio_dev);
+	int wm;
+
+	mutex_lock(&data->mutex);
+	wm = data->watermark;
+	mutex_unlock(&data->mutex);
+
+	return sysfs_emit(buf, "%d\n", wm);
+}
+
+static ssize_t hwfifo_enabled_show(struct device *dev,
+				   struct device_attribute *attr,
+				   char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct bmi323_data *data = iio_priv(indio_dev);
+	bool state;
+
+	mutex_lock(&data->mutex);
+	state = data->state == BMI323_BUFFER_FIFO ? true : false;
+	mutex_unlock(&data->mutex);
+
+	return sysfs_emit(buf, "%d\n", state);
+}
+
+static IIO_DEVICE_ATTR_RO(hwfifo_watermark, 0);
+static IIO_DEVICE_ATTR_RO(hwfifo_enabled, 0);
+
+static const struct iio_dev_attr *bmi323_fifo_attributes[] = {
+	&iio_dev_attr_hwfifo_watermark,
+	&iio_dev_attr_hwfifo_enabled,
+	NULL
+};
+
+static const struct iio_buffer_setup_ops bmi323_buffer_ops = {
+	.preenable = bmi323_buffer_preenable,
+	.postenable = bmi323_buffer_postenable,
+	.predisable = bmi323_buffer_predisable,
+};
+
+static irqreturn_t bmi323_irq_thread_handler(int irq, void *private)
+{
+	struct iio_dev *indio_dev = private;
+	struct bmi323_data *data = iio_priv(indio_dev);
+	unsigned int status_addr, status, feature_event;
+	s64 timestamp = iio_get_time_ns(indio_dev);
+	int ret;
+
+	if (data->irq_pin == BMI323_IRQ_INT1)
+		status_addr = BMI323_STATUS_INT1_REG;
+	else
+		status_addr = BMI323_STATUS_INT2_REG;
+
+	mutex_lock(&data->mutex);
+	ret = regmap_read(data->regmap, status_addr, &status);
+	mutex_unlock(&data->mutex);
+	if (ret)
+		return IRQ_NONE;
+
+	if (!status || FIELD_GET(BMI323_STATUS_ERROR_MSK, status)) {
+		dev_err(data->dev, "status error = 0x%x\n", status);
+		return IRQ_NONE;
+	}
+
+	if (FIELD_GET(BMI323_STATUS_FIFO_WTRMRK_MSK, status)) {
+		data->old_fifo_tstamp = data->fifo_tstamp;
+		data->fifo_tstamp = iio_get_time_ns(indio_dev);
+		ret = __bmi323_fifo_flush(indio_dev);
+		if (ret < 0)
+			return IRQ_NONE;
+	}
+
+	if (FIELD_GET(BMI323_STATUS_ACC_GYR_DRDY_MSK, status))
+		iio_trigger_poll_nested(data->trig);
+
+	if (FIELD_GET(BMI323_STATUS_MOTION_MSK, status))
+		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+							     IIO_MOD_X_OR_Y_OR_Z,
+							     IIO_EV_TYPE_MAG,
+							     IIO_EV_DIR_RISING),
+			       timestamp);
+
+	if (FIELD_GET(BMI323_STATUS_NOMOTION_MSK, status))
+		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+							     IIO_MOD_X_OR_Y_OR_Z,
+							     IIO_EV_TYPE_MAG,
+							     IIO_EV_DIR_FALLING),
+			       timestamp);
+
+	if (FIELD_GET(BMI323_STATUS_STP_WTR_MSK, status))
+		iio_push_event(indio_dev, IIO_MOD_EVENT_CODE(IIO_STEPS, 0,
+							     IIO_NO_MOD,
+							     IIO_EV_TYPE_CHANGE,
+							     IIO_EV_DIR_NONE),
+			       timestamp);
+
+	if (FIELD_GET(BMI323_STATUS_TAP_MSK, status)) {
+		mutex_lock(&data->mutex);
+		ret = regmap_read(data->regmap, BMI323_FEAT_EVNT_EXT_REG,
+				  &feature_event);
+		mutex_unlock(&data->mutex);
+		if (ret)
+			return IRQ_NONE;
+
+		if (FIELD_GET(BMI323_FEAT_EVNT_EXT_S_MSK, feature_event)) {
+			iio_push_event(indio_dev,
+				       IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+							  IIO_MOD_X_OR_Y_OR_Z,
+							  IIO_EV_TYPE_GESTURE,
+							  IIO_EV_DIR_SINGLETAP),
+				       timestamp);
+		}
+
+		if (FIELD_GET(BMI323_FEAT_EVNT_EXT_D_MSK, feature_event))
+			iio_push_event(indio_dev,
+				       IIO_MOD_EVENT_CODE(IIO_ACCEL, 0,
+							  IIO_MOD_X_OR_Y_OR_Z,
+							  IIO_EV_TYPE_GESTURE,
+							  IIO_EV_DIR_DOUBLETAP),
+				       timestamp);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int bmi323_set_drdy_irq(struct bmi323_data *data,
+			       enum bmi323_irq_pin irq_pin)
+{
+	int ret;
+
+	ret = regmap_update_bits(data->regmap, BMI323_INT_MAP2_REG,
+				 BMI323_GYR_DRDY_MSK,
+				 FIELD_PREP(BMI323_GYR_DRDY_MSK, irq_pin));
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(data->regmap, BMI323_INT_MAP2_REG,
+				  BMI323_ACC_DRDY_MSK,
+				  FIELD_PREP(BMI323_ACC_DRDY_MSK, irq_pin));
+}
+
+static int bmi323_data_rdy_trigger_set_state(struct iio_trigger *trig,
+					     bool state)
+{
+	struct bmi323_data *data = iio_trigger_get_drvdata(trig);
+	enum bmi323_irq_pin irq_pin;
+	int ret;
+
+	mutex_lock(&data->mutex);
+
+	if (data->state == BMI323_BUFFER_FIFO) {
+		dev_warn(data->dev, "Can't set trigger when FIFO enabled\n");
+		ret = -EBUSY;
+		goto unlock_out;
+	}
+
+	if (state) {
+		data->state = BMI323_BUFFER_DRDY_TRIGGERED;
+		irq_pin = data->irq_pin;
+	} else {
+		data->state = BMI323_IDLE;
+		irq_pin = BMI323_IRQ_DISABLED;
+	}
+
+	ret = bmi323_set_drdy_irq(data, irq_pin);
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+	return ret;
+}
+
+static const struct iio_trigger_ops bmi323_trigger_ops = {
+	.set_trigger_state = &bmi323_data_rdy_trigger_set_state,
+};
+
+static irqreturn_t bmi323_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct bmi323_data *data = iio_priv(indio_dev);
+	int ret;
+
+	/* Lock to protect the data->buffer */
+	mutex_lock(&data->mutex);
+	ret = regmap_bulk_read(data->regmap, BMI323_ACCEL_X_REG,
+			       &data->buffer.channels,
+			       ARRAY_SIZE(data->buffer.channels));
+	if (ret)
+		goto unlock_out;
+
+	iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
+					   iio_get_time_ns(indio_dev));
+	mutex_unlock(&data->mutex);
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+
+unlock_out:
+	mutex_unlock(&data->mutex);
+	return IRQ_NONE;
+}
+
+static int bmi323_set_average(struct bmi323_data *data,
+			      enum bmi323_sensor_type sensor, int avg)
+{
+	int ret, raw;
+
+	for (raw = 0; raw < ARRAY_SIZE(bmi323_accel_gyro_avrg); raw++)
+		if (avg == bmi323_accel_gyro_avrg[raw])
+			break;
+
+	if (raw >= ARRAY_SIZE(bmi323_accel_gyro_avrg))
+		return -EINVAL;
+
+	mutex_lock(&data->mutex);
+	ret = regmap_update_bits(data->regmap, bmi323_hw[sensor].config,
+				 BMI323_ACC_GYRO_CONF_AVG_MSK,
+				 FIELD_PREP(BMI323_ACC_GYRO_CONF_AVG_MSK,
+					    raw));
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int bmi323_get_average(struct bmi323_data *data,
+			      enum bmi323_sensor_type sensor, int *avg)
+{
+	int ret, value, raw;
+
+	mutex_lock(&data->mutex);
+	ret = regmap_read(data->regmap, bmi323_hw[sensor].config, &value);
+	mutex_unlock(&data->mutex);
+	if (ret)
+		return ret;
+
+	raw = FIELD_GET(BMI323_ACC_GYRO_CONF_AVG_MSK, value);
+	*avg = bmi323_accel_gyro_avrg[raw];
+
+	return ret;
+}
+
+static ssize_t in_accel_gyro_averaging_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct bmi323_data *data = iio_priv(indio_dev);
+	int ret, avg;
+
+	ret = bmi323_get_average(data, BMI323_ACCEL, &avg);
+	if (ret)
+		return ret;
+
+	return sysfs_emit(buf, "%d\n", avg);
+}
+
+static ssize_t in_accel_gyro_averaging_store(struct device *dev,
+					     struct device_attribute *attr,
+					     const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct bmi323_data *data = iio_priv(indio_dev);
+	int ret, avg;
+
+	ret = kstrtoint(buf, 0, &avg);
+	if (ret)
+		return ret;
+
+	ret = bmi323_set_average(data, BMI323_ACCEL, avg);
+	if (ret)
+		return ret;
+
+	ret = bmi323_set_average(data, BMI323_GYRO, avg);
+	if (ret)
+		return ret;
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR_RW(in_accel_gyro_averaging, 0);
+static IIO_CONST_ATTR(in_accel_gyro_averaging_available, "2 4 8 16 32 64");
+
+static struct attribute *bmi323_attributes[] = {
+	&iio_dev_attr_in_accel_gyro_averaging.dev_attr.attr,
+	&iio_const_attr_in_accel_gyro_averaging_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group bmi323_attrs_group = {
+	.attrs = bmi323_attributes,
+};
+
+static int bmi323_enable_steps(struct bmi323_data *data, int val)
+{
+	int ret;
+
+	if (data->odrhz[BMI323_ACCEL] < 200) {
+		dev_err(data->dev, "Invalid accelrometer parameter\n");
+		return -EINVAL;
+	}
+
+	ret = bmi323_feature_engine_events(data, BMI323_FEAT_IO0_STP_CNT_MSK,
+					   val ? 1 : 0);
+	if (ret)
+		return ret;
+
+	set_mask_bits(&data->feature_events, BMI323_FEAT_IO0_STP_CNT_MSK,
+		      FIELD_PREP(BMI323_FEAT_IO0_STP_CNT_MSK, val ? 1 : 0));
+
+	return ret;
+}
+
+static int bmi323_read_steps(struct bmi323_data *data, int *val)
+{
+	int ret;
+
+	if (!FIELD_GET(BMI323_FEAT_IO0_STP_CNT_MSK, data->feature_events))
+		return -EINVAL;
+
+	ret = regmap_bulk_read(data->regmap, BMI323_FEAT_IO2_REG,
+			       data->steps_count,
+			       ARRAY_SIZE(data->steps_count));
+	if (ret)
+		return ret;
+
+	*val = get_unaligned_le32(data->steps_count);
+
+	return IIO_VAL_INT;
+}
+
+static int bmi323_read_axis(struct bmi323_data *data,
+			    struct iio_chan_spec const *chan, int *val)
+{
+	enum bmi323_sensor_type sensor;
+	unsigned int value;
+	u8 addr;
+	int ret;
+
+	ret = bmi323_get_error_status(data);
+	if (ret)
+		return -EINVAL;
+
+	sensor = bmi323_iio_to_sensor(chan->type);
+	addr = bmi323_hw[sensor].data + (chan->channel2 - IIO_MOD_X);
+
+	ret = regmap_read(data->regmap, addr, &value);
+	if (ret)
+		return ret;
+
+	*val = sign_extend32(le16_to_cpup((const __le16 *)&value),
+			     chan->scan_type.realbits - 1);
+
+	return IIO_VAL_INT;
+}
+
+static int bmi323_get_temp_data(struct bmi323_data *data, int *val)
+{
+	unsigned int value;
+	int ret;
+
+	ret = bmi323_get_error_status(data);
+	if (ret)
+		return -EINVAL;
+
+	ret = regmap_read(data->regmap, BMI323_TEMP_REG, &value);
+	if (ret)
+		return ret;
+
+	*val = sign_extend32(le16_to_cpup((const __le16 *)&value), 15);
+
+	return IIO_VAL_INT;
+}
+
+static int bmi323_get_odr(struct bmi323_data *data, enum iio_chan_type type,
+			  int *odr, int *uodr)
+{
+	enum bmi323_sensor_type sensor;
+	int ret, value, odr_raw;
+
+	sensor = bmi323_iio_to_sensor(type);
+
+	ret = regmap_read(data->regmap, bmi323_hw[sensor].config, &value);
+	if (ret)
+		return ret;
+
+	odr_raw = FIELD_GET(BMI323_ACC_GYRO_CONF_ODR_MSK, value);
+	*odr = bmi323_acc_gyro_odr[odr_raw - 1][0];
+	*uodr = bmi323_acc_gyro_odr[odr_raw - 1][1];
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int bmi323_configure_power_mode(struct bmi323_data *data,
+				       enum bmi323_sensor_type sensor,
+				       int odr_index)
+{
+	enum bmi323_opr_mode mode;
+
+	if (bmi323_acc_gyro_odr[odr_index][0] <= 25)
+		mode = ACC_GYRO_MODE_DUTYCYCLE;
+	else
+		mode = ACC_GYRO_MODE_CONTINOUS;
+
+	return bmi323_set_mode(data, sensor, mode);
+}
+
+static int bmi323_set_odr(struct bmi323_data *data,
+			  enum bmi323_sensor_type sensor, int odr, int uodr)
+{
+	int odr_raw, ret;
+
+	odr_raw = ARRAY_SIZE(bmi323_acc_gyro_odr);
+
+	while (odr_raw--)
+		if (odr == bmi323_acc_gyro_odr[odr_raw][0] &&
+		    uodr == bmi323_acc_gyro_odr[odr_raw][1])
+			break;
+	if (odr_raw < 0)
+		return -EINVAL;
+
+	ret = bmi323_configure_power_mode(data, sensor, odr_raw);
+	if (ret)
+		return -EINVAL;
+
+	data->odrhz[sensor] = bmi323_acc_gyro_odr[odr_raw][0];
+	data->odrns[sensor] = bmi323_acc_gyro_odrns[odr_raw];
+
+	odr_raw++;
+
+	return regmap_update_bits(data->regmap, bmi323_hw[sensor].config,
+				  BMI323_ACC_GYRO_CONF_ODR_MSK,
+				  FIELD_PREP(BMI323_ACC_GYRO_CONF_ODR_MSK,
+					     odr_raw));
+}
+
+static int bmi323_get_scale(struct bmi323_data *data, enum iio_chan_type type,
+			    int *val2)
+{
+	enum bmi323_sensor_type sensor;
+	int ret, value, scale_raw;
+
+	sensor = bmi323_iio_to_sensor(type);
+
+	ret = regmap_read(data->regmap, bmi323_hw[sensor].config, &value);
+	if (ret)
+		return ret;
+
+	scale_raw = FIELD_GET(BMI323_ACC_GYRO_CONF_SCL_MSK, value);
+	*val2 = bmi323_hw[sensor].scale_table[scale_raw][1];
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int bmi323_set_scale(struct bmi323_data *data,
+			    enum bmi323_sensor_type sensor, int val, int val2)
+{
+	int scale_raw;
+
+	if (data->state != BMI323_IDLE)
+		return -EBUSY;
+
+	scale_raw = bmi323_hw[sensor].scale_table_len;
+
+	while (scale_raw--)
+		if (val == bmi323_hw[sensor].scale_table[scale_raw][0] &&
+		    val2 == bmi323_hw[sensor].scale_table[scale_raw][1])
+			break;
+	if (scale_raw < 0)
+		return -EINVAL;
+
+	return regmap_update_bits(data->regmap, bmi323_hw[sensor].config,
+				  BMI323_ACC_GYRO_CONF_SCL_MSK,
+				  FIELD_PREP(BMI323_ACC_GYRO_CONF_SCL_MSK,
+					     scale_raw));
+}
+
+static int bmi323_read_avail(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     const int **vals, int *type, int *length,
+			     long mask)
+{
+	enum bmi323_sensor_type sensor;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		*vals = (const int *)bmi323_acc_gyro_odr;
+		*length = ARRAY_SIZE(bmi323_acc_gyro_odr) * 2;
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_SCALE:
+		sensor = bmi323_iio_to_sensor(chan->type);
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		*vals = (const int *)bmi323_hw[sensor].scale_table;
+		*length = bmi323_hw[sensor].scale_table_len * 2;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bmi323_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan, int val,
+			    int val2, long mask)
+{
+	struct bmi323_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		mutex_lock(&data->mutex);
+		ret = bmi323_set_odr(data, bmi323_iio_to_sensor(chan->type),
+				     val, val2);
+		mutex_unlock(&data->mutex);
+		iio_device_release_direct_mode(indio_dev);
+		return ret;
+	case IIO_CHAN_INFO_SCALE:
+		ret = iio_device_claim_direct_mode(indio_dev);
+		if (ret)
+			return ret;
+
+		mutex_lock(&data->mutex);
+		ret = bmi323_set_scale(data, bmi323_iio_to_sensor(chan->type),
+				       val, val2);
+		mutex_unlock(&data->mutex);
+		iio_device_release_direct_mode(indio_dev);
+		return ret;
+	case IIO_CHAN_INFO_ENABLE:
+		mutex_lock(&data->mutex);
+		ret = bmi323_enable_steps(data, val);
+		mutex_unlock(&data->mutex);
+		return ret;
+	case IIO_CHAN_INFO_PROCESSED:
+		if (val || !FIELD_GET(BMI323_FEAT_IO0_STP_CNT_MSK,
+				      data->feature_events))
+			return -EINVAL;
+
+		/* Clear step counter value */
+		return bmi323_update_ext_reg(data, BMI323_STEP_SC1_REG,
+					     BMI323_STEP_SC1_RST_CNT_MSK,
+					     FIELD_PREP(BMI323_STEP_SC1_RST_CNT_MSK,
+							1));
+	default:
+		return -EINVAL;
+	}
+}
+
+static int bmi323_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan, int *val,
+			   int *val2, long mask)
+{
+	struct bmi323_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		mutex_lock(&data->mutex);
+		ret = bmi323_read_steps(data, val);
+		mutex_unlock(&data->mutex);
+		return ret;
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->type) {
+		case IIO_ACCEL:
+		case IIO_ANGL_VEL:
+			ret = iio_device_claim_direct_mode(indio_dev);
+			if (ret)
+				return ret;
+
+			mutex_lock(&data->mutex);
+			ret = bmi323_read_axis(data, chan, val);
+			mutex_unlock(&data->mutex);
+
+			iio_device_release_direct_mode(indio_dev);
+			return ret;
+		case IIO_TEMP:
+			mutex_lock(&data->mutex);
+			ret = bmi323_get_temp_data(data, val);
+			mutex_unlock(&data->mutex);
+			return ret;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		mutex_lock(&data->mutex);
+		ret = bmi323_get_odr(data, chan->type, val, val2);
+		mutex_unlock(&data->mutex);
+		return ret;
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_ACCEL:
+		case IIO_ANGL_VEL:
+			*val = 0;
+			mutex_lock(&data->mutex);
+			ret = bmi323_get_scale(data, chan->type, val2);
+			mutex_unlock(&data->mutex);
+			return ret;
+		case IIO_TEMP:
+			*val = BMI323_TEMP_SCALE / MEGA;
+			*val2 = BMI323_TEMP_SCALE % MEGA;
+			return IIO_VAL_INT_PLUS_MICRO;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_OFFSET:
+		switch (chan->type) {
+		case IIO_TEMP:
+			*val = BMI323_TEMP_OFFSET;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_ENABLE:
+		mutex_lock(&data->mutex);
+		*val = FIELD_GET(BMI323_FEAT_IO0_STP_CNT_MSK,
+				 data->feature_events);
+		mutex_unlock(&data->mutex);
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info bmi323_info = {
+	.read_raw = bmi323_read_raw,
+	.write_raw = bmi323_write_raw,
+	.read_avail = bmi323_read_avail,
+	.attrs = &bmi323_attrs_group,
+	.hwfifo_set_watermark = bmi323_set_watermark,
+	.write_event_config = bmi323_write_event_config,
+	.read_event_config = bmi323_read_event_config,
+	.write_event_value = bmi323_write_event_value,
+	.read_event_value = bmi323_read_event_value,
+	.event_attrs = &bmi323_event_attribute_group,
+};
+
+#define BMI323_SCAN_MASK_ACCEL_3AXIS		\
+	(BIT(BMI323_ACCEL_X) | BIT(BMI323_ACCEL_Y) | BIT(BMI323_ACCEL_Z))
+
+#define BMI323_SCAN_MASK_GYRO_3AXIS		\
+	(BIT(BMI323_GYRO_X) | BIT(BMI323_GYRO_Y) | BIT(BMI323_GYRO_Z))
+
+static const unsigned long bmi323_avail_scan_masks[] = {
+	/* 3-axis accel + 3-axis gyro */
+	BMI323_SCAN_MASK_ACCEL_3AXIS | BMI323_SCAN_MASK_GYRO_3AXIS,
+	/* 3-axis accel */
+	BMI323_SCAN_MASK_ACCEL_3AXIS,
+	/* 3-axis gyro */
+	BMI323_SCAN_MASK_GYRO_3AXIS,
+	0
+};
+
+static int bmi323_int_pin_config(struct bmi323_data *data,
+				 enum bmi323_irq_pin irq_pin,
+				 bool active_high, bool open_drain, bool latch)
+{
+	unsigned int mask, field_value = 0;
+	int ret;
+
+	switch (irq_pin) {
+	case BMI323_IRQ_INT1:
+		mask = BMI323_IO_INT1_LVL_OD_OP_MSK;
+
+		set_mask_bits(&field_value, BMI323_IO_INT1_LVL_MSK,
+			      FIELD_PREP(BMI323_IO_INT1_LVL_MSK, active_high));
+
+		set_mask_bits(&field_value, BMI323_IO_INT1_OD_MSK,
+			      FIELD_PREP(BMI323_IO_INT1_OD_MSK, open_drain));
+
+		set_mask_bits(&field_value, BMI323_IO_INT1_OP_EN_MSK,
+			      FIELD_PREP(BMI323_IO_INT1_OP_EN_MSK, 1));
+		break;
+	case BMI323_IRQ_INT2:
+		mask = BMI323_IO_INT2_LVL_OD_OP_MSK;
+
+		set_mask_bits(&field_value, BMI323_IO_INT2_LVL_MSK,
+			      FIELD_PREP(BMI323_IO_INT2_LVL_MSK, active_high));
+
+		set_mask_bits(&field_value, BMI323_IO_INT2_OD_MSK,
+			      FIELD_PREP(BMI323_IO_INT2_OD_MSK, open_drain));
+
+		set_mask_bits(&field_value, BMI323_IO_INT2_OP_EN_MSK,
+			      FIELD_PREP(BMI323_IO_INT2_OP_EN_MSK, 1));
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_update_bits(data->regmap, BMI323_IO_INT_CONF_REG,
+				 BMI323_IO_INT_LTCH_MSK,
+				 FIELD_PREP(BMI323_IO_INT_LTCH_MSK, latch));
+	if (ret)
+		return ret;
+
+	ret = bmi323_update_ext_reg(data, BMI323_GEN_SET1_REG,
+				    BMI323_GEN_HOLD_DUR_MSK,
+				    FIELD_PREP(BMI323_GEN_HOLD_DUR_MSK, 0));
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(data->regmap, BMI323_IO_INT_CTR_REG, mask,
+				  field_value);
+}
+
+static int bmi323_trigger_probe(struct bmi323_data *data,
+				struct iio_dev *indio_dev)
+{
+	bool open_drain, active_high, latch;
+	struct fwnode_handle *fwnode;
+	enum bmi323_irq_pin irq_pin;
+	int ret, irq, irq_type;
+	struct irq_data *desc;
+
+	fwnode = dev_fwnode(data->dev);
+	if (!fwnode)
+		return -ENODEV;
+
+	irq = fwnode_irq_get_byname(fwnode, "INT1");
+	if (irq > 0) {
+		irq_pin = BMI323_IRQ_INT1;
+	} else {
+		irq = fwnode_irq_get_byname(fwnode, "INT2");
+		if (irq <= 0)
+			return 0;
+
+		irq_pin = BMI323_IRQ_INT2;
+	}
+
+	desc = irq_get_irq_data(irq);
+	if (!desc) {
+		dev_err(data->dev, "Could not find IRQ %d\n", irq);
+		return -EINVAL;
+	}
+
+	irq_type = irqd_get_trigger_type(desc);
+
+	switch (irq_type) {
+	case IRQF_TRIGGER_RISING:
+		latch = false;
+		active_high = true;
+		break;
+	case IRQF_TRIGGER_HIGH:
+		latch = true;
+		active_high = true;
+		break;
+	case IRQF_TRIGGER_FALLING:
+		latch = false;
+		active_high = false;
+		break;
+	case IRQF_TRIGGER_LOW:
+		latch = true;
+		active_high = false;
+		break;
+	default:
+		dev_err(data->dev, "Invalid interrupt type 0x%x specified\n",
+			irq_type);
+		return -EINVAL;
+	}
+
+	open_drain = fwnode_property_read_bool(fwnode, "drive-open-drain");
+
+	ret = bmi323_int_pin_config(data, irq_pin, active_high, open_drain,
+				    latch);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Failed to configure irq line\n");
+
+	data->trig = devm_iio_trigger_alloc(data->dev, "%s-trig-%d",
+					    indio_dev->name, irq_pin);
+	if (!data->trig)
+		return -ENOMEM;
+
+	data->trig->ops = &bmi323_trigger_ops;
+	iio_trigger_set_drvdata(data->trig, data);
+
+	ret = devm_request_threaded_irq(data->dev, irq, NULL,
+					bmi323_irq_thread_handler,
+					irq_type | IRQF_ONESHOT,
+					"bmi323-int", indio_dev);
+	if (ret)
+		return dev_err_probe(data->dev, ret, "Failed to request IRQ\n");
+
+	ret = devm_iio_trigger_register(data->dev, data->trig);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Trigger registration failed\n");
+
+	data->irq_pin = irq_pin;
+
+	return ret;
+}
+
+static int bmi323_feature_engine_enable(struct bmi323_data *data, bool en)
+{
+	unsigned int feature_status;
+	int ret, i;
+
+	if (en) {
+		ret = regmap_write(data->regmap, BMI323_FEAT_IO2_REG,
+				   0x012c);
+		if (ret)
+			return ret;
+
+		ret = regmap_write(data->regmap, BMI323_FEAT_IO_STATUS_REG,
+				   BMI323_FEAT_IO_STATUS_MSK);
+		if (ret)
+			return ret;
+
+		ret = regmap_write(data->regmap, BMI323_FEAT_CTRL_REG,
+				   BMI323_FEAT_ENG_EN_MSK);
+		if (ret)
+			return ret;
+
+		i = 5;
+		do {
+			ret = regmap_read(data->regmap, BMI323_FEAT_IO1_REG,
+					  &feature_status);
+			if (ret)
+				return ret;
+
+			i--;
+			mdelay(2);
+		} while (feature_status != 0x01 && i);
+
+		if (feature_status != 0x01) {
+			dev_err(data->dev, "Failed to enable feature engine\n");
+			return -EINVAL;
+		}
+
+		return ret;
+	} else {
+		return regmap_write(data->regmap, BMI323_FEAT_CTRL_REG, 0);
+	}
+}
+
+static void bmi323_disable(void *data_ptr)
+{
+	struct bmi323_data *data = data_ptr;
+
+	bmi323_set_mode(data, BMI323_ACCEL, ACC_GYRO_MODE_DISABLE);
+	bmi323_set_mode(data, BMI323_GYRO, ACC_GYRO_MODE_DISABLE);
+}
+
+static int bmi323_set_bw(struct bmi323_data *data,
+			 enum bmi323_sensor_type sensor, enum bmi323_3db_bw bw)
+{
+	return regmap_update_bits(data->regmap, bmi323_hw[sensor].config,
+				  BMI323_ACC_GYRO_CONF_BW_MSK,
+				  FIELD_PREP(BMI323_ACC_GYRO_CONF_BW_MSK, bw));
+}
+
+static int bmi323_init(struct bmi323_data *data)
+{
+	int ret, val;
+
+	/*
+	 * Perform soft reset to make sure the device is in a known state after
+	 * start up. A delay of 1.5 ms is required after reset.
+	 * See datasheet section 5.17 "Soft Reset".
+	 */
+	ret = regmap_write(data->regmap, BMI323_CMD_REG, BMI323_RST_VAL);
+	if (ret)
+		return ret;
+
+	usleep_range(1500, 2000);
+
+	/*
+	 * Dummy read is required to enable SPI interface after reset.
+	 * See datasheet section 7.2.1 "Protocol Selection".
+	 */
+	regmap_read(data->regmap, BMI323_CHIP_ID_REG, &val);
+
+	ret = regmap_read(data->regmap, BMI323_CHIP_ID_REG, &val);
+	if (ret)
+		return ret;
+
+	if ((val & 0xFF) != BMI323_CHIP_ID_VAL) {
+		dev_err(data->dev, "Chip ID mismatch\n");
+		return -EINVAL;
+	}
+
+	ret = bmi323_feature_engine_enable(data, true);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(data->regmap, BMI323_ERR_REG, &val);
+	if (ret)
+		return ret;
+
+	if (val) {
+		dev_err(data->dev, "Sensor power error = 0x%x\n", val);
+		return -EINVAL;
+	}
+
+	ret = regmap_read(data->regmap, BMI323_STATUS_REG, &val);
+	if (ret)
+		return ret;
+
+	if (val != 0x01) {
+		dev_err(data->dev, "Sensor initialization error = 0x%x\n", val);
+		return -EINVAL;
+	}
+
+	/*
+	 * Set the Bandwidth coefficient which defines the 3 dB cutoff
+	 * frequency in relation to the ODR.
+	 */
+	ret = bmi323_set_bw(data, BMI323_ACCEL, BMI323_BW_ODR_BY_2);
+	if (ret)
+		return -EINVAL;
+
+	ret = bmi323_set_bw(data, BMI323_GYRO, BMI323_BW_ODR_BY_2);
+	if (ret)
+		return -EINVAL;
+
+	ret = bmi323_set_odr(data, BMI323_ACCEL, 25, 0);
+	if (ret)
+		return -EINVAL;
+
+	ret = bmi323_set_odr(data, BMI323_GYRO, 25, 0);
+	if (ret)
+		return -EINVAL;
+
+	ret = devm_add_action_or_reset(data->dev, bmi323_disable, data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+int bmi323_core_probe(struct device *dev)
+{
+	static const char * const regulator_names[] = { "vdd", "vddio" };
+	struct iio_dev *indio_dev;
+	struct bmi323_data *data;
+	struct regmap *regmap;
+	int ret;
+
+	regmap = dev_get_regmap(dev, NULL);
+	if (!regmap) {
+		dev_err(dev, "No regmap\n");
+		return -EINVAL;
+	}
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulator_names),
+					     regulator_names);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable regulators\n");
+
+	data = iio_priv(indio_dev);
+	data->dev = dev;
+	data->regmap = regmap;
+	mutex_init(&data->mutex);
+
+	ret = bmi323_init(data);
+	if (ret)
+		return -EINVAL;
+
+	ret = iio_read_mount_matrix(dev, &data->orientation);
+	if (ret)
+		return ret;
+
+	indio_dev->name = "bmi323-imu";
+	indio_dev->info = &bmi323_info;
+	indio_dev->channels = bmi323_channels;
+	indio_dev->num_channels = ARRAY_SIZE(bmi323_channels);
+	indio_dev->available_scan_masks = bmi323_avail_scan_masks;
+	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
+	dev_set_drvdata(data->dev, indio_dev);
+
+	ret = bmi323_trigger_probe(data, indio_dev);
+	if (ret)
+		return -EINVAL;
+
+	ret = devm_iio_triggered_buffer_setup_ext(data->dev, indio_dev,
+						  &iio_pollfunc_store_time,
+						  bmi323_trigger_handler,
+						  IIO_BUFFER_DIRECTION_IN,
+						  &bmi323_buffer_ops,
+						  bmi323_fifo_attributes);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Failed to setup trigger buffer\n");
+
+	ret = devm_iio_device_register(data->dev, indio_dev);
+	if (ret)
+		return dev_err_probe(data->dev, ret,
+				     "Unable to register iio device\n");
+
+	return ret;
+}
+EXPORT_SYMBOL_NS_GPL(bmi323_core_probe, IIO_BMI323);
+
+MODULE_DESCRIPTION("Bosch BMI323 IMU driver");
+MODULE_AUTHOR("Jagath Jog J <jagathjog1996@gmail.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/iio/imu/bmi323/bmi323_i2c.c b/drivers/iio/imu/bmi323/bmi323_i2c.c
new file mode 100644
index 000000000000..afbaa3d3c638
--- /dev/null
+++ b/drivers/iio/imu/bmi323/bmi323_i2c.c
@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * I2C driver for Bosch BMI323 6-Axis IMU.
+ *
+ * Copyright (C) 2023, Jagath Jog J <jagathjog1996@gmail.com>
+ */
+
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#include "bmi323.h"
+
+/*
+ * From BMI323 datasheet section 4: Notes on the Serial Interface Support.
+ * Each I2C register read operation requires to read two dummy bytes before
+ * the actual payload.
+ */
+static int bmi323_regmap_i2c_read(void *context, const void *reg_buf,
+				  size_t reg_size, void *val_buf,
+				  size_t val_size)
+{
+	struct device *dev = context;
+	struct i2c_client *i2c = to_i2c_client(dev);
+	struct i2c_msg msgs[2];
+	u8 *buff = NULL;
+	int ret;
+
+	buff = kmalloc(val_size + BMI323_I2C_DUMMY, GFP_KERNEL);
+	if (!buff)
+		return -ENOMEM;
+
+	msgs[0].addr = i2c->addr;
+	msgs[0].flags = i2c->flags;
+	msgs[0].len = reg_size;
+	msgs[0].buf = (u8 *)reg_buf;
+
+	msgs[1].addr = i2c->addr;
+	msgs[1].len = val_size + BMI323_I2C_DUMMY;
+	msgs[1].buf = (u8 *)buff;
+	msgs[1].flags = i2c->flags | I2C_M_RD;
+
+	ret = i2c_transfer(i2c->adapter, msgs, ARRAY_SIZE(msgs));
+	if (ret < 0) {
+		kfree(buff);
+		return -EIO;
+	}
+
+	memcpy(val_buf, buff + BMI323_I2C_DUMMY, val_size);
+	kfree(buff);
+
+	return 0;
+}
+
+static int bmi323_regmap_i2c_write(void *context, const void *data,
+				   size_t count)
+{
+	struct device *dev = context;
+	struct i2c_client *i2c = to_i2c_client(dev);
+	int ret;
+	u8 reg;
+
+	reg = *(u8 *)data;
+	ret = i2c_smbus_write_i2c_block_data(i2c, reg, count - 1,
+					     data + sizeof(u8));
+
+	return ret;
+}
+
+static struct regmap_bus bmi323_regmap_bus = {
+	.read = bmi323_regmap_i2c_read,
+	.write = bmi323_regmap_i2c_write,
+};
+
+static int bmi323_i2c_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;
+	struct regmap *regmap;
+
+	regmap = devm_regmap_init(dev, &bmi323_regmap_bus, dev,
+				  &bmi323_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "Failed to initialize I2C Regmap\n");
+
+	return bmi323_core_probe(dev);
+}
+
+static const struct i2c_device_id bmi323_i2c_ids[] = {
+	{ "bmi323", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, bmi323_i2c_ids);
+
+static const struct of_device_id bmi323_of_i2c_match[] = {
+	{ .compatible = "bosch,bmi323" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, bmi323_of_i2c_match);
+
+static struct i2c_driver bmi323_i2c_driver = {
+	.driver = {
+		.name = "bmi323",
+		.of_match_table = bmi323_of_i2c_match,
+	},
+	.probe_new = bmi323_i2c_probe,
+	.id_table = bmi323_i2c_ids,
+};
+module_i2c_driver(bmi323_i2c_driver);
+
+MODULE_DESCRIPTION("Bosch BMI323 IMU driver");
+MODULE_AUTHOR("Jagath Jog J <jagathjog1996@gmail.com>");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_BMI323);
diff --git a/drivers/iio/imu/bmi323/bmi323_spi.c b/drivers/iio/imu/bmi323/bmi323_spi.c
new file mode 100644
index 000000000000..2b802a0c6d9d
--- /dev/null
+++ b/drivers/iio/imu/bmi323/bmi323_spi.c
@@ -0,0 +1,106 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SPI driver for Bosch BMI323 6-Axis IMU.
+ *
+ * Copyright (C) 2023, Jagath Jog J <jagathjog1996@gmail.com>
+ */
+
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/spi/spi.h>
+
+#include "bmi323.h"
+
+/*
+ * From BMI323 datasheet section 4: Notes on the Serial Interface Support.
+ * Each SPI register read operation requires to read one dummy byte before
+ * the actual payload.
+ */
+static int bmi323_regmap_spi_read(void *context, const void *reg_buf,
+				  size_t reg_size, void *val_buf,
+				  size_t val_size)
+{
+	struct spi_device *spi = context;
+	u8 reg, *buff = NULL;
+	int ret;
+
+	buff = kmalloc(val_size + BMI323_SPI_DUMMY, GFP_KERNEL);
+	if (!buff)
+		return -ENOMEM;
+
+	reg = *(u8 *)reg_buf | 0x80;
+	ret = spi_write_then_read(spi, &reg, sizeof(reg), buff,
+				  val_size + BMI323_SPI_DUMMY);
+	if (ret) {
+		kfree(buff);
+		return ret;
+	}
+
+	memcpy(val_buf, buff + BMI323_SPI_DUMMY, val_size);
+	kfree(buff);
+
+	return ret;
+}
+
+static int bmi323_regmap_spi_write(void *context, const void *data,
+				   size_t count)
+{
+	struct spi_device *spi = context;
+
+	return spi_write(spi, data, count);
+}
+
+static struct regmap_bus bmi323_regmap_bus = {
+	.read = bmi323_regmap_spi_read,
+	.write = bmi323_regmap_spi_write,
+};
+
+static int bmi323_spi_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct regmap *regmap;
+	int dummy;
+
+	regmap = devm_regmap_init(dev, &bmi323_regmap_bus, dev,
+				  &bmi323_regmap_config);
+
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "Failed to initialize SPI Regmap\n");
+
+	/*
+	 * Dummy read is required to enable SPI interface after POR.
+	 * See datasheet section 7.2.1 "Protocol Selection".
+	 */
+	regmap_read(regmap, BMI323_CHIP_ID_REG, &dummy);
+
+	return bmi323_core_probe(dev);
+}
+
+static const struct spi_device_id bmi323_spi_ids[] = {
+	{ "bmi323", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(spi, bmi323_spi_ids);
+
+static const struct of_device_id bmi323_of_spi_match[] = {
+	{ .compatible = "bosch,bmi323" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, bmi323_of_spi_match);
+
+static struct spi_driver bmi323_spi_driver = {
+	.driver = {
+		.name = "bmi323",
+		.of_match_table = bmi323_of_spi_match,
+	},
+	.probe = bmi323_spi_probe,
+	.id_table = bmi323_spi_ids,
+};
+module_spi_driver(bmi323_spi_driver);
+
+MODULE_DESCRIPTION("Bosch BMI323 IMU driver");
+MODULE_AUTHOR("Jagath Jog J <jagathjog1996@gmail.com>");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(IIO_BMI323);
-- 
2.20.1

Re: [RFC 2/2] iio: imu: Add driver for BMI323 IMU

[Andy Shevchenko]

On Mon, Sep 18, 2023 at 01:33:14PM +0530, Jagath Jog J wrote:
> The Bosch BMI323 is a 6-axis low-power IMU that provide measurements for
> acceleration, angular rate, and temperature. This sensor includes
> motion-triggered interrupt features, such as a step counter, tap detection,
> and activity/inactivity interrupt capabilities.
> 
> The driver supports various functionalities, including data ready, FIFO
> data handling, and events such as tap detection, step counting, and
> activity interrupts

Missing period.

...

> +#include <linux/regmap.h>
> +#include <linux/bits.h>

Ordered?

Missing units.h.

...

> +#define BMI323_INT_MICRO_TO_RAW(val, val2, scale) (((val) * (scale)) + \
> +						  (((val2) * (scale)) / MEGA))

Better to split:

#define BMI323_INT_MICRO_TO_RAW(val, val2, scale)
	((val) * (scale) + ((val2) * (scale)) / MEGA)

Also note dropped redundant parentheses.

...

> +#define BMI323_RAW_TO_MICRO(raw, scale) ((((raw) % (scale)) * MEGA) / scale)

...

> +struct bmi323_data {

> +	u32 odrns[2];
> +	u32 odrhz[2];


> +	__le16 steps_count[2];
> +};

I'm wondering if these 2:s anyhow semantically the same? Shouldn't a definition
be used instead of magic number?

...

> +	320 * MEGA,
> +	160 * MEGA,
> +	80 * MEGA,
> +	40 * MEGA,
> +	20 * MEGA,
> +	10 * MEGA,
> +	5 * MEGA,
> +	2500000,

	2500 * KILO,

for the sake of consistency?

> +	1250000,

	1250 * KILO,

> +};

...

> +static int bmi323_write_ext_reg(struct bmi323_data *data, unsigned int ext_addr,
> +				unsigned int ext_data)
> +{
> +	int ret, feature_status;
> +
> +	mutex_lock(&data->mutex);

You can start using cleanup.h, it will reduce your code by a few percents!
But the point is it makes it less error prone and less verbose.

Ditto for the entire code base.

> +	ret = regmap_read(data->regmap, BMI323_FEAT_DATA_STATUS,
> +			  &feature_status);
> +	if (ret)
> +		goto unlock_out;
> +
> +	if (!FIELD_GET(BMI323_FEAT_DATA_TX_RDY_MSK, feature_status)) {
> +		ret = -EBUSY;
> +		goto unlock_out;
> +	}
> +
> +	ret = regmap_write(data->regmap, BMI323_FEAT_DATA_ADDR, ext_addr);
> +	if (ret)
> +		goto unlock_out;
> +
> +	ret = regmap_write(data->regmap, BMI323_FEAT_DATA_TX, ext_data);
> +
> +unlock_out:
> +	mutex_unlock(&data->mutex);
> +	return ret;
> +}

...

> +static int bmi323_update_ext_reg(struct bmi323_data *data,
> +				 unsigned int ext_addr,
> +				 unsigned int mask, unsigned int ext_data)
> +{
> +	unsigned int value;
> +	int ret;
> +
> +	ret = bmi323_read_ext_reg(data, ext_addr, &value);
> +	if (ret)
> +		return ret;
> +
> +	set_mask_bits(&value, mask, ext_data);

> +	ret = bmi323_write_ext_reg(data, ext_addr, value);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

	return _write_ext_reg(...);

> +}

...

	unsigned int state_value = GENMASK();

> +	switch (dir) {
> +	case IIO_EV_DIR_RISING:
> +		msk = BMI323_FEAT_IO0_XYZ_MOTION_MSK;
> +		raw = 512;
> +		config = BMI323_ANYMO1_REG;
> +		irq_msk = BMI323_MOTION_MSK;
> +		set_mask_bits(&irq_field_val, BMI323_MOTION_MSK,
> +			      FIELD_PREP(BMI323_MOTION_MSK, motion_irq));
> +		set_mask_bits(&field_value, BMI323_FEAT_IO0_XYZ_MOTION_MSK,
> +			      FIELD_PREP(BMI323_FEAT_IO0_XYZ_MOTION_MSK,
> +					 state ? 7 : 0));

state_value

> +		break;
> +	case IIO_EV_DIR_FALLING:
> +		msk = BMI323_FEAT_IO0_XYZ_NOMOTION_MSK;
> +		raw = 0;
> +		config = BMI323_NOMO1_REG;
> +		irq_msk = BMI323_NOMOTION_MSK;
> +		set_mask_bits(&irq_field_val, BMI323_NOMOTION_MSK,
> +			      FIELD_PREP(BMI323_NOMOTION_MSK, motion_irq));
> +		set_mask_bits(&field_value, BMI323_FEAT_IO0_XYZ_NOMOTION_MSK,
> +			      FIELD_PREP(BMI323_FEAT_IO0_XYZ_NOMOTION_MSK,
> +					 state ? 7 : 0));

Ditto.

> +		break;
> +	default:
> +		return -EINVAL;
> +	}

...

> +static ssize_t in_accel_gesture_tap_max_dur_show(struct device *dev,
> +						 struct device_attribute *attr,
> +						 char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct bmi323_data *data = iio_priv(indio_dev);
> +	unsigned int reg_value, raw;
> +	int ret, val[2];

Why val is int (i.e. not unsigned)?

> +	ret = bmi323_read_ext_reg(data, BMI323_TAP2_REG, &reg_value);
> +	if (ret)
> +		return ret;
> +
> +	raw = FIELD_GET(BMI323_TAP2_MAX_DUR_MSK, reg_value);

> +	val[0] = raw / BMI323_MAX_GES_DUR_SCALE;
> +	val[1] = BMI323_RAW_TO_MICRO(raw, BMI323_MAX_GES_DUR_SCALE);

> +	return iio_format_value(buf, IIO_VAL_INT_PLUS_MICRO, 2, val);

ARRAY_SIZE()

> +}

...

> +static ssize_t in_accel_gesture_tap_timeout_store(struct device *dev,
> +						  struct device_attribute *attr,
> +						  const char *buf, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct bmi323_data *data = iio_priv(indio_dev);
> +	int ret, val;

> +	if (kstrtoint(buf, 10, &val))
> +		return -EINVAL;

Don't shadow the error code.


> +	if (!(val == 0 || val == 1))
> +		return -EINVAL;

So, effectively you should use kstrtobool().

> +	ret = bmi323_update_ext_reg(data, BMI323_TAP1_REG,
> +				    BMI323_TAP1_TIMOUT_MSK,
> +				    FIELD_PREP(BMI323_TAP1_TIMOUT_MSK, val));
> +	if (ret)
> +		return ret;
> +
> +	return len;
> +}

...

> +static const struct attribute_group bmi323_event_attribute_group = {
> +	.attrs = bmi323_event_attributes,
> +};

ATTRIBUTE_GROUPS() ?

...

> +{
> +	struct bmi323_data *data = iio_priv(indio_dev);

> +	int ret;

Unneeded variable, return directly.

> +
> +	switch (type) {
> +	case IIO_EV_TYPE_MAG:
> +		ret = bmi323_motion_event_en(data, dir, state);
> +		return ret;
> +	case IIO_EV_TYPE_GESTURE:
> +		return bmi323_tap_event_en(data, dir, state);
> +	case IIO_EV_TYPE_CHANGE:
> +		ret = bmi323_step_wtrmrk_en(data, state);
> +		return ret;
> +	default:
> +		return -EINVAL;
> +	}
> +}

...

> +		case IIO_EV_INFO_RESET_TIMEOUT:
> +			if (val != 0 || val2 < 40000 || val2 > 600000)
> +				return -EINVAL;

			if (val || ...

Use is_range() from minmax.h.

> +
> +			raw = BMI323_INT_MICRO_TO_RAW(val, val2,
> +						      BMI323_QUITE_TIM_GES_SCALE);
> +
> +			return bmi323_update_ext_reg(data, BMI323_TAP3_REG,
> +						     BMI323_TAP3_QT_AFT_GES_MSK,
> +						     FIELD_PREP(BMI323_TAP3_QT_AFT_GES_MSK,

...

> +		case IIO_EV_INFO_TAP2_MIN_DELAY:
> +			if (val != 0 || val2 < 5000 || val2 > 75000)

Ditto.

> +				return -EINVAL;
> +
> +			raw = BMI323_INT_MICRO_TO_RAW(val, val2,
> +						      BMI323_DUR_BW_TAP_SCALE);
> +
> +			return bmi323_update_ext_reg(data, BMI323_TAP3_REG,
> +						     BMI323_TAP3_QT_BW_TAP_MSK,
> +						     FIELD_PREP(BMI323_TAP3_QT_BW_TAP_MSK,
> +								raw));

...

> +		case IIO_EV_INFO_VALUE:
> +			if (val < 0 || val > 7)
> +				return -EINVAL;

Ditto.

> +			raw = BMI323_INT_MICRO_TO_RAW(val, val2,
> +						      BMI323_MOTION_THRES_SCALE);
> +
> +			return bmi323_update_ext_reg(data, reg,
> +						     BMI323_MO1_SLOPE_TH_MSK,
> +						     FIELD_PREP(BMI323_MO1_SLOPE_TH_MSK,
> +								raw));
> +		case IIO_EV_INFO_PERIOD:
> +			if (val < 0 || val > 162)

Ditto.

> +				return -EINVAL;
> +
> +			raw = BMI323_INT_MICRO_TO_RAW(val, val2,
> +						      BMI323_MOTION_DURAT_SCALE);
> +
> +			return bmi323_update_ext_reg(data,
> +						     reg + BMI323_MO3_OFFSET,
> +						     BMI323_MO3_DURA_MSK,
> +						     FIELD_PREP(BMI323_MO3_DURA_MSK,
> +								raw));
> +		case IIO_EV_INFO_HYSTERESIS:
> +			if (!(val == 0 || val == 1))

Ditto.

> +				return -EINVAL;
> +
> +			raw = BMI323_INT_MICRO_TO_RAW(val, val2,
> +						      BMI323_MOTION_HYSTR_SCALE);
> +
> +			return bmi323_update_ext_reg(data,
> +						     reg + BMI323_MO2_OFFSET,
> +						     BMI323_MO2_HYSTR_MSK,
> +						     FIELD_PREP(BMI323_MO2_HYSTR_MSK,
> +								raw));

...

> +	case IIO_EV_TYPE_CHANGE:
> +		if (val < 0 || val > 20460)

Ditto.

> +			return -EINVAL;
> +
> +		raw = val / 20;
> +		return bmi323_update_ext_reg(data, BMI323_STEP_SC1_REG,
> +					     BMI323_STEP_SC1_WTRMRK_MSK,
> +					     FIELD_PREP(BMI323_STEP_SC1_WTRMRK_MSK,
> +							raw));

...

> +	if (val > BMI323_FIFO_FULL_IN_FRAMES)
> +		val = BMI323_FIFO_FULL_IN_FRAMES;

min()

...

> +	ret = regmap_update_bits(data->regmap, BMI323_FIFO_CONF_REG,
> +				 BMI323_FIFO_CONF_ACC_GYR_EN_MSK,
> +				 FIELD_PREP(BMI323_FIFO_CONF_ACC_GYR_EN_MSK,
> +					    3));

GENMASK() ?

> +	if (ret)
> +		goto unlock_out;

...

> +	state = data->state == BMI323_BUFFER_FIFO ? true : false;

== already results in boolean type.

...

> +static IIO_DEVICE_ATTR_RO(hwfifo_watermark, 0);
> +static IIO_DEVICE_ATTR_RO(hwfifo_enabled, 0);

Place them closer to the respective callbacks.

...

> +	if (!status || FIELD_GET(BMI323_STATUS_ERROR_MSK, status)) {
> +		dev_err(data->dev, "status error = 0x%x\n", status);

If it's not your interrupt you may flood the logs here.

> +		return IRQ_NONE;
> +	}

...

> +	int ret, raw;

Why raw is signed?

> +	for (raw = 0; raw < ARRAY_SIZE(bmi323_accel_gyro_avrg); raw++)
> +		if (avg == bmi323_accel_gyro_avrg[raw])
> +			break;

> +	if (raw >= ARRAY_SIZE(bmi323_accel_gyro_avrg))

When is the > part true?

> +		return -EINVAL;

...

> +static const struct attribute_group bmi323_attrs_group = {
> +	.attrs = bmi323_attributes,
> +};

ATTRIBUTE_GROUPS() ?

...

> +	ret = bmi323_feature_engine_events(data, BMI323_FEAT_IO0_STP_CNT_MSK,
> +					   val ? 1 : 0);

Ternary here...

> +	if (ret)
> +		return ret;
> +
> +	set_mask_bits(&data->feature_events, BMI323_FEAT_IO0_STP_CNT_MSK,
> +		      FIELD_PREP(BMI323_FEAT_IO0_STP_CNT_MSK, val ? 1 : 0));

...and here are dups.

> +	return ret;

Can it be not 0 here?

...

> +static int bmi323_get_temp_data(struct bmi323_data *data, int *val)
> +{
> +	unsigned int value;

Why it's not defined as __le16 to begin with?

> +	int ret;
> +
> +	ret = bmi323_get_error_status(data);
> +	if (ret)
> +		return -EINVAL;
> +
> +	ret = regmap_read(data->regmap, BMI323_TEMP_REG, &value);
> +	if (ret)
> +		return ret;
> +
> +	*val = sign_extend32(le16_to_cpup((const __le16 *)&value), 15);

No, simply no castings here.

> +	return IIO_VAL_INT;
> +}

...

> +	if (bmi323_acc_gyro_odr[odr_index][0] <= 25)

Why not positive check: if (... > 25) ... else ...?

> +		mode = ACC_GYRO_MODE_DUTYCYCLE;
> +	else
> +		mode = ACC_GYRO_MODE_CONTINOUS;

...

> +	int odr_raw, ret;

Why odr_raw is signed?

> +
> +	odr_raw = ARRAY_SIZE(bmi323_acc_gyro_odr);
> +
> +	while (odr_raw--)
> +		if (odr == bmi323_acc_gyro_odr[odr_raw][0] &&
> +		    uodr == bmi323_acc_gyro_odr[odr_raw][1])
> +			break;
> +	if (odr_raw < 0)
> +		return -EINVAL;

In one case in the code you used for-loop, here is while-loop. Maybe a bit of
consistency?

...

> +static int bmi323_set_scale(struct bmi323_data *data,
> +			    enum bmi323_sensor_type sensor, int val, int val2)
> +{
> +	int scale_raw;
> +
> +	if (data->state != BMI323_IDLE)
> +		return -EBUSY;
> +
> +	scale_raw = bmi323_hw[sensor].scale_table_len;
> +
> +	while (scale_raw--)
> +		if (val == bmi323_hw[sensor].scale_table[scale_raw][0] &&
> +		    val2 == bmi323_hw[sensor].scale_table[scale_raw][1])
> +			break;
> +	if (scale_raw < 0)
> +		return -EINVAL;

Note, here is a different case and hence fine to be while-loop.

> +	return regmap_update_bits(data->regmap, bmi323_hw[sensor].config,
> +				  BMI323_ACC_GYRO_CONF_SCL_MSK,
> +				  FIELD_PREP(BMI323_ACC_GYRO_CONF_SCL_MSK,
> +					     scale_raw));
> +}

...

> +	fwnode = dev_fwnode(data->dev);
> +	if (!fwnode)
> +		return -ENODEV;
> +
> +	irq = fwnode_irq_get_byname(fwnode, "INT1");
> +	if (irq > 0) {
> +		irq_pin = BMI323_IRQ_INT1;
> +	} else {
> +		irq = fwnode_irq_get_byname(fwnode, "INT2");
> +		if (irq <= 0)

When can it be == 0?

> +			return 0;
> +
> +		irq_pin = BMI323_IRQ_INT2;
> +	}

...

> +	irq_type = irqd_get_trigger_type(desc);

> +

Redundant blank line.

> +	switch (irq_type) {
> +	case IRQF_TRIGGER_RISING:
> +		latch = false;
> +		active_high = true;
> +		break;
> +	case IRQF_TRIGGER_HIGH:
> +		latch = true;
> +		active_high = true;
> +		break;
> +	case IRQF_TRIGGER_FALLING:
> +		latch = false;
> +		active_high = false;
> +		break;
> +	case IRQF_TRIGGER_LOW:
> +		latch = true;
> +		active_high = false;
> +		break;
> +	default:
> +		dev_err(data->dev, "Invalid interrupt type 0x%x specified\n",
> +			irq_type);
> +		return -EINVAL;

Here and above, why not dev_err_probe() as you used it already below?

> +	}

...

> +	if (en) {

> +		ret = regmap_write(data->regmap, BMI323_FEAT_IO2_REG,
> +				   0x012c);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_write(data->regmap, BMI323_FEAT_IO_STATUS_REG,
> +				   BMI323_FEAT_IO_STATUS_MSK);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_write(data->regmap, BMI323_FEAT_CTRL_REG,
> +				   BMI323_FEAT_ENG_EN_MSK);
> +		if (ret)
> +			return ret;

> +		i = 5;
> +		do {
> +			ret = regmap_read(data->regmap, BMI323_FEAT_IO1_REG,
> +					  &feature_status);
> +			if (ret)
> +				return ret;
> +
> +			i--;
> +			mdelay(2);
> +		} while (feature_status != 0x01 && i);

NIH regmap_read_poll_timeout().

> +		if (feature_status != 0x01) {
> +			dev_err(data->dev, "Failed to enable feature engine\n");
> +			return -EINVAL;
> +		}
> +
> +		return ret;

> +	} else {

Redundant. But here it's okay to leave (I can understand the justification).

> +		return regmap_write(data->regmap, BMI323_FEAT_CTRL_REG, 0);
> +	}

...

> +	if ((val & 0xFF) != BMI323_CHIP_ID_VAL) {

GENMASK() ? (BIT(x) - 1) ? A defined constant?

> +		dev_err(data->dev, "Chip ID mismatch\n");
> +		return -EINVAL;

Why not dev_err_probe() in this entire function?

> +	}

...

> +	ret = devm_add_action_or_reset(data->dev, bmi323_disable, data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

	return devm_...

...

> +	regmap = dev_get_regmap(dev, NULL);
> +	if (!regmap) {
> +		dev_err(dev, "No regmap\n");
> +		return -EINVAL;

Why not dev_err_probe()?

> +	}

> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>

...

> +static int bmi323_regmap_i2c_read(void *context, const void *reg_buf,
> +				  size_t reg_size, void *val_buf,
> +				  size_t val_size)
> +{
> +	struct device *dev = context;
> +	struct i2c_client *i2c = to_i2c_client(dev);
> +	struct i2c_msg msgs[2];

> +	u8 *buff = NULL;

Redundant assignment.

> +	int ret;
> +
> +	buff = kmalloc(val_size + BMI323_I2C_DUMMY, GFP_KERNEL);

size_add() and include overflow.h for it.

> +	if (!buff)
> +		return -ENOMEM;
> +
> +	msgs[0].addr = i2c->addr;
> +	msgs[0].flags = i2c->flags;
> +	msgs[0].len = reg_size;
> +	msgs[0].buf = (u8 *)reg_buf;
> +
> +	msgs[1].addr = i2c->addr;
> +	msgs[1].len = val_size + BMI323_I2C_DUMMY;
> +	msgs[1].buf = (u8 *)buff;
> +	msgs[1].flags = i2c->flags | I2C_M_RD;
> +
> +	ret = i2c_transfer(i2c->adapter, msgs, ARRAY_SIZE(msgs));
> +	if (ret < 0) {
> +		kfree(buff);

With cleanup.h this will look better.

> +		return -EIO;
> +	}
> +
> +	memcpy(val_buf, buff + BMI323_I2C_DUMMY, val_size);
> +	kfree(buff);
> +
> +	return 0;
> +}

...

> +static int bmi323_regmap_i2c_write(void *context, const void *data,
> +				   size_t count)
> +{
> +	struct device *dev = context;
> +	struct i2c_client *i2c = to_i2c_client(dev);
> +	int ret;
> +	u8 reg;
> +
> +	reg = *(u8 *)data;
> +	ret = i2c_smbus_write_i2c_block_data(i2c, reg, count - 1,
> +					     data + sizeof(u8));
> +
> +	return ret;
> +}

Hmm... Don't we have a better approach for these? regmap doesn't provide SMBus
accessors?

...

> +static const struct i2c_device_id bmi323_i2c_ids[] = {
> +	{ "bmi323", 0 },

', 0' is redundant

> +	{ }
> +};

...

> +static int bmi323_regmap_spi_read(void *context, const void *reg_buf,
> +				  size_t reg_size, void *val_buf,
> +				  size_t val_size)
> +{
> +	struct spi_device *spi = context;
> +	u8 reg, *buff = NULL;
> +	int ret;
> +
> +	buff = kmalloc(val_size + BMI323_SPI_DUMMY, GFP_KERNEL);

As per i2c part.

> +	if (!buff)
> +		return -ENOMEM;
> +
> +	reg = *(u8 *)reg_buf | 0x80;

Doesn't regmap configuration provide a way to set this?

> +	ret = spi_write_then_read(spi, &reg, sizeof(reg), buff,
> +				  val_size + BMI323_SPI_DUMMY);
> +	if (ret) {
> +		kfree(buff);
> +		return ret;
> +	}
> +
> +	memcpy(val_buf, buff + BMI323_SPI_DUMMY, val_size);
> +	kfree(buff);
> +
> +	return ret;
> +}

...

> +	regmap = devm_regmap_init(dev, &bmi323_regmap_bus, dev,
> +				  &bmi323_regmap_config);

> +

Redundant blank line.

> +	if (IS_ERR(regmap))
> +		return dev_err_probe(dev, PTR_ERR(regmap),
> +				     "Failed to initialize SPI Regmap\n");
> +

...

> +static const struct spi_device_id bmi323_spi_ids[] = {
> +	{ "bmi323", 0 },

As per above.

> +	{ }
> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 2/2] iio: imu: Add driver for BMI323 IMU

[Jagath Jog J]

Hi Andy,

Thank you for reviewing.

On Mon, Sep 18, 2023 at 3:34 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Sep 18, 2023 at 01:33:14PM +0530, Jagath Jog J wrote:
> > The Bosch BMI323 is a 6-axis low-power IMU that provide measurements for
> > acceleration, angular rate, and temperature. This sensor includes
> > motion-triggered interrupt features, such as a step counter, tap detection,
> > and activity/inactivity interrupt capabilities.
> >
> > The driver supports various functionalities, including data ready, FIFO
> > data handling, and events such as tap detection, step counting, and
> > activity interrupts
>
> Missing period.
>
> ...
>
> > +#include <linux/regmap.h>
> > +#include <linux/bits.h>
>
> Ordered?
>
> Missing units.h.

Sure I will correct these in the next series.
Please note that I omitted certain portions of your reviews
while responding, and I fully agree with the comments that
I didn't address. I intend to make the necessary corrections
in the next series.
....

> > +struct bmi323_data {
>
> > +     u32 odrns[2];
> > +     u32 odrhz[2];
>
>
> > +     __le16 steps_count[2];
> > +};
>
> I'm wondering if these 2:s anyhow semantically the same? Shouldn't a definition
> be used instead of magic number?

The arrays odrns[] and odrhz[] are used to store the ODR in nanoseconds and
frequency for both the accelerometer and gyro. Instead of the magic number 2,
I will define an enum. For steps_count[] array is of size 2 words and
I will define
a separate macro.

> ...
>
> > +static int bmi323_write_ext_reg(struct bmi323_data *data, unsigned int ext_addr,
> > +                             unsigned int ext_data)
> > +{
> > +     int ret, feature_status;
> > +
> > +     mutex_lock(&data->mutex);
>
> You can start using cleanup.h, it will reduce your code by a few percents!
> But the point is it makes it less error prone and less verbose.
>
> Ditto for the entire code base.

Sure, thanks for pointing this I will go through cleanup.h.
If required I will get back with some questions.

>
> > +     ret = regmap_read(data->regmap, BMI323_FEAT_DATA_STATUS,
> > +                       &feature_status);
> > +     if (ret)
> > +             goto unlock_out;
> > +
> > +     if (!FIELD_GET(BMI323_FEAT_DATA_TX_RDY_MSK, feature_status)) {
> > +             ret = -EBUSY;
> > +             goto unlock_out;
> > +     }
> > +
> > +     ret = regmap_write(data->regmap, BMI323_FEAT_DATA_ADDR, ext_addr);
> > +     if (ret)
> > +             goto unlock_out;
> > +
> > +     ret = regmap_write(data->regmap, BMI323_FEAT_DATA_TX, ext_data);
> > +
> > +unlock_out:
> > +     mutex_unlock(&data->mutex);
> > +     return ret;
> > +}
> ...
>
>         unsigned int state_value = GENMASK();
>
> > +     switch (dir) {
> > +     case IIO_EV_DIR_RISING:
> > +             msk = BMI323_FEAT_IO0_XYZ_MOTION_MSK;
> > +             raw = 512;
> > +             config = BMI323_ANYMO1_REG;
> > +             irq_msk = BMI323_MOTION_MSK;
> > +             set_mask_bits(&irq_field_val, BMI323_MOTION_MSK,
> > +                           FIELD_PREP(BMI323_MOTION_MSK, motion_irq));
> > +             set_mask_bits(&field_value, BMI323_FEAT_IO0_XYZ_MOTION_MSK,
> > +                           FIELD_PREP(BMI323_FEAT_IO0_XYZ_MOTION_MSK,
> > +                                      state ? 7 : 0));
>
> state_value

Sorry I didn't get this, I am updating field_value based on state value.
What is the purpose of state_value?

>
> > +             break;
> > +     case IIO_EV_DIR_FALLING:
> > +             msk = BMI323_FEAT_IO0_XYZ_NOMOTION_MSK;
> > +             raw = 0;
> > +             config = BMI323_NOMO1_REG;
> > +             irq_msk = BMI323_NOMOTION_MSK;
> > +             set_mask_bits(&irq_field_val, BMI323_NOMOTION_MSK,
> > +                           FIELD_PREP(BMI323_NOMOTION_MSK, motion_irq));
> > +             set_mask_bits(&field_value, BMI323_FEAT_IO0_XYZ_NOMOTION_MSK,
> > +                           FIELD_PREP(BMI323_FEAT_IO0_XYZ_NOMOTION_MSK,
> > +                                      state ? 7 : 0));
>
> Ditto.
>
> > +             break;
> > +     default:
> > +             return -EINVAL;
> > +     }
>
> ...
>
> > +static ssize_t in_accel_gesture_tap_max_dur_show(struct device *dev,
> > +                                              struct device_attribute *attr,
> > +                                              char *buf)
> > +{
> > +     struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +     struct bmi323_data *data = iio_priv(indio_dev);
> > +     unsigned int reg_value, raw;
> > +     int ret, val[2];
>
> Why val is int (i.e. not unsigned)?

iio_format_value() expects int* so I used int.

>
> > +     ret = bmi323_read_ext_reg(data, BMI323_TAP2_REG, &reg_value);
> > +     if (ret)
> > +             return ret;
> > +
> > +     raw = FIELD_GET(BMI323_TAP2_MAX_DUR_MSK, reg_value);
>
> > +     val[0] = raw / BMI323_MAX_GES_DUR_SCALE;
> > +     val[1] = BMI323_RAW_TO_MICRO(raw, BMI323_MAX_GES_DUR_SCALE);
>
> > +     return iio_format_value(buf, IIO_VAL_INT_PLUS_MICRO, 2, val);
>
> ARRAY_SIZE()
Okay, I will use ARRAY_SIZE() instead of number.

>
> > +static const struct attribute_group bmi323_event_attribute_group = {
> > +     .attrs = bmi323_event_attributes,
> > +};
>
> ATTRIBUTE_GROUPS() ?

Okay, I will use ATTRIBUTE_GROUPS.

>
> ...
>
> > +     state = data->state == BMI323_BUFFER_FIFO ? true : false;
>
> == already results in boolean type.

Sure I will directly assign the result of comparison.
state = (data->state == BMI323_BUFFER_FIFO);

> ...
>
> > +     int ret, raw;
>
> Why raw is signed?

I don't have a specific reason for this; however, since it
stores register value, it should be unsigned. I will make
this correction in the next series

>
> > +     for (raw = 0; raw < ARRAY_SIZE(bmi323_accel_gyro_avrg); raw++)
> > +             if (avg == bmi323_accel_gyro_avrg[raw])
> > +                     break;
>
> > +     if (raw >= ARRAY_SIZE(bmi323_accel_gyro_avrg))
>
> When is the > part true?
>
> > +             return -EINVAL;
>

I missed this, > is not possible, I should have used while() here
also, I will correct this in the next series.


> > +     ret = bmi323_feature_engine_events(data, BMI323_FEAT_IO0_STP_CNT_MSK,
> > +                                        val ? 1 : 0);
>
> Ternary here...
>
> > +     if (ret)
> > +             return ret;
> > +
> > +     set_mask_bits(&data->feature_events, BMI323_FEAT_IO0_STP_CNT_MSK,
> > +                   FIELD_PREP(BMI323_FEAT_IO0_STP_CNT_MSK, val ? 1 : 0));
>
> ...and here are dups.

Is the ternary operator not permitted to use?

>
> > +     return ret;
>
> Can it be not 0 here?
>
> ...
>
> > +static int bmi323_get_temp_data(struct bmi323_data *data, int *val)
> > +{
> > +     unsigned int value;
>
> Why it's not defined as __le16 to begin with?

To match the regmap_read() val parameter I used unsigned int*.

Note: each sensor register values are 16 bit wide
and regmap is configured with .val_bits = 16.

> > +
> > +     ret = bmi323_get_error_status(data);
> > +     if (ret)
> > +             return -EINVAL;
> > +
> > +     ret = regmap_read(data->regmap, BMI323_TEMP_REG, &value);
> > +     if (ret)
> > +             return ret;
> > +
> > +     *val = sign_extend32(le16_to_cpup((const __le16 *)&value), 15);
>
> No, simply no castings here.
>
> > +     return IIO_VAL_INT;
> > +}
>
> ...
>
> > +     if (bmi323_acc_gyro_odr[odr_index][0] <= 25)
>
> Why not positive check: if (... > 25) ... else ...?
>
> > +             mode = ACC_GYRO_MODE_DUTYCYCLE;
> > +     else
> > +             mode = ACC_GYRO_MODE_CONTINOUS;

Sure, this can also be used. I will update this

>
> ...
>
> > +     int odr_raw, ret;
>
> Why odr_raw is signed?

In the below conditions, I am checking for -ve value so
odr_raw is signed.

>
> > +
> > +     odr_raw = ARRAY_SIZE(bmi323_acc_gyro_odr);
> > +
> > +     while (odr_raw--)
> > +             if (odr == bmi323_acc_gyro_odr[odr_raw][0] &&
> > +                 uodr == bmi323_acc_gyro_odr[odr_raw][1])
> > +                     break;
> > +     if (odr_raw < 0)
> > +             return -EINVAL;
>
> In one case in the code you used for-loop, here is while-loop. Maybe a bit of
> consistency?

Sure, for other case, I will use a while loop instead of a for loop.

>
> > +     fwnode = dev_fwnode(data->dev);
> > +     if (!fwnode)
> > +             return -ENODEV;
> > +
> > +     irq = fwnode_irq_get_byname(fwnode, "INT1");
> > +     if (irq > 0) {
> > +             irq_pin = BMI323_IRQ_INT1;
> > +     } else {
> > +             irq = fwnode_irq_get_byname(fwnode, "INT2");
> > +             if (irq <= 0)
>
> When can it be == 0?

Right, fwnode_irq_get_byname won't return 0, I will correct this
in the next series.

>
> > +     if (en) {
>
> > +             ret = regmap_write(data->regmap, BMI323_FEAT_IO2_REG,
> > +                                0x012c);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = regmap_write(data->regmap, BMI323_FEAT_IO_STATUS_REG,
> > +                                BMI323_FEAT_IO_STATUS_MSK);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = regmap_write(data->regmap, BMI323_FEAT_CTRL_REG,
> > +                                BMI323_FEAT_ENG_EN_MSK);
> > +             if (ret)
> > +                     return ret;
>
> > +             i = 5;
> > +             do {
> > +                     ret = regmap_read(data->regmap, BMI323_FEAT_IO1_REG,
> > +                                       &feature_status);
> > +                     if (ret)
> > +                             return ret;
> > +
> > +                     i--;
> > +                     mdelay(2);
> > +             } while (feature_status != 0x01 && i);
>
> NIH regmap_read_poll_timeout().

Okay.

>
> > +             if (feature_status != 0x01) {
> > +                     dev_err(data->dev, "Failed to enable feature engine\n");
> > +                     return -EINVAL;
> > +             }
> > +
> > +             return ret;
>
> > +     } else {
>
> Redundant. But here it's okay to leave (I can understand the justification).
>
> > +             return regmap_write(data->regmap, BMI323_FEAT_CTRL_REG, 0);
> > +     }
>
> ...
>
> > +     if ((val & 0xFF) != BMI323_CHIP_ID_VAL) {
>
> GENMASK() ? (BIT(x) - 1) ? A defined constant?
>
> > +             dev_err(data->dev, "Chip ID mismatch\n");
> > +             return -EINVAL;
>
> Why not dev_err_probe() in this entire function?

Okay I will make use of dev_err_probe() here and in all
probe paths.

>
> > +     ret = devm_add_action_or_reset(data->dev, bmi323_disable, data);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return 0;
>
>         return devm_...
>
> ...
>
> > +     regmap = dev_get_regmap(dev, NULL);
> > +     if (!regmap) {
> > +             dev_err(dev, "No regmap\n");
> > +             return -EINVAL;
>
> Why not dev_err_probe()?

There was no int return value from dev_get_regmap(),
I think I can use -EINVAL for err in dev_err_probe as well.

>
> > +     }
>
>
> > +static int bmi323_regmap_i2c_write(void *context, const void *data,
> > +                                size_t count)
> > +{
> > +     struct device *dev = context;
> > +     struct i2c_client *i2c = to_i2c_client(dev);
> > +     int ret;
> > +     u8 reg;
> > +
> > +     reg = *(u8 *)data;
> > +     ret = i2c_smbus_write_i2c_block_data(i2c, reg, count - 1,
> > +                                          data + sizeof(u8));
> > +
> > +     return ret;
> > +}
>
> Hmm... Don't we have a better approach for these? regmap doesn't provide SMBus
> accessors?

Custom implementation is required for the 'read' operation, while
'write' can utilize the regmap SMBus accessors. Is it okay to have
only custom read while write uses the SMBus accessor?

>
> > +static int bmi323_regmap_spi_read(void *context, const void *reg_buf,
> > +                               size_t reg_size, void *val_buf,
> > +                               size_t val_size)
> > +{
> > +     struct spi_device *spi = context;
> > +     u8 reg, *buff = NULL;
> > +     int ret;
> > +
> > +     buff = kmalloc(val_size + BMI323_SPI_DUMMY, GFP_KERNEL);
 >
> As per i2c part.
>
> > +     if (!buff)
> > +             return -ENOMEM;
> > +
> > +     reg = *(u8 *)reg_buf | 0x80;
>
> Doesn't regmap configuration provide a way to set this?

Okay, I will make use of regmap .read_flag_mask member.
I will update this in the next series.

>
> > +     ret = spi_write_then_read(spi, &reg, sizeof(reg), buff,
> > +                               val_size + BMI323_SPI_DUMMY);
> > +     if (ret) {
> > +             kfree(buff);
> > +             return ret;
> > +     }
> > +
> > +     memcpy(val_buf, buff + BMI323_SPI_DUMMY, val_size);
> > +     kfree(buff);
> > +
> > +     return ret;
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Thank you
Jagath

Re: [RFC 2/2] iio: imu: Add driver for BMI323 IMU

[Andy Shevchenko]

On Wed, Sep 20, 2023 at 04:13:51AM +0530, Jagath Jog J wrote:
> On Mon, Sep 18, 2023 at 3:34 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Sep 18, 2023 at 01:33:14PM +0530, Jagath Jog J wrote:

...

> >         unsigned int state_value = GENMASK();
> >
> > > +     switch (dir) {
> > > +     case IIO_EV_DIR_RISING:
> > > +             msk = BMI323_FEAT_IO0_XYZ_MOTION_MSK;
> > > +             raw = 512;
> > > +             config = BMI323_ANYMO1_REG;
> > > +             irq_msk = BMI323_MOTION_MSK;
> > > +             set_mask_bits(&irq_field_val, BMI323_MOTION_MSK,
> > > +                           FIELD_PREP(BMI323_MOTION_MSK, motion_irq));
> > > +             set_mask_bits(&field_value, BMI323_FEAT_IO0_XYZ_MOTION_MSK,
> > > +                           FIELD_PREP(BMI323_FEAT_IO0_XYZ_MOTION_MSK,
> > > +                                      state ? 7 : 0));
> >
> > state_value
> 
> Sorry I didn't get this, I am updating field_value based on state value.
> What is the purpose of state_value?

Something like this I meant:

	unsigned int state_value = state ? GENMASK(2, 0) : 0;
	...
	switch (dir) {
	case IIO_EV_DIR_RISING:
		...
		set_mask_bits(&field_value, BMI323_FEAT_IO0_XYZ_MOTION_MSK,
			      FIELD_PREP(BMI323_FEAT_IO0_XYZ_MOTION_MSK, state_value));

> > > +             break;
> > > +     case IIO_EV_DIR_FALLING:
> > > +             msk = BMI323_FEAT_IO0_XYZ_NOMOTION_MSK;
> > > +             raw = 0;
> > > +             config = BMI323_NOMO1_REG;
> > > +             irq_msk = BMI323_NOMOTION_MSK;
> > > +             set_mask_bits(&irq_field_val, BMI323_NOMOTION_MSK,
> > > +                           FIELD_PREP(BMI323_NOMOTION_MSK, motion_irq));
> > > +             set_mask_bits(&field_value, BMI323_FEAT_IO0_XYZ_NOMOTION_MSK,
> > > +                           FIELD_PREP(BMI323_FEAT_IO0_XYZ_NOMOTION_MSK,
> > > +                                      state ? 7 : 0));

Ditto.

> > > +             break;
> > > +     default:
> > > +             return -EINVAL;
> > > +     }

...

> > > +     ret = bmi323_feature_engine_events(data, BMI323_FEAT_IO0_STP_CNT_MSK,
> > > +                                        val ? 1 : 0);
> >
> > Ternary here...
> >
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     set_mask_bits(&data->feature_events, BMI323_FEAT_IO0_STP_CNT_MSK,
> > > +                   FIELD_PREP(BMI323_FEAT_IO0_STP_CNT_MSK, val ? 1 : 0));
> >
> > ...and here are dups.
> 
> Is the ternary operator not permitted to use?

Yes, it's permitted. My point that you can calculate once the value

	unsigned int value = val ? 1 : 0;

and use it everywhere where it makes sense.

...

> > > +static int bmi323_get_temp_data(struct bmi323_data *data, int *val)
> > > +{
> > > +     unsigned int value;
> >
> > Why it's not defined as __le16 to begin with?
> 
> To match the regmap_read() val parameter I used unsigned int*.
> 
> Note: each sensor register values are 16 bit wide
> and regmap is configured with .val_bits = 16.

> > > +     ret = regmap_read(data->regmap, BMI323_TEMP_REG, &value);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     *val = sign_extend32(le16_to_cpup((const __le16 *)&value), 15);
> >
> > No, simply no castings here.

This is an interesting case.

Your regmap configuration tells about endianess of the accessors. Default
IIRC is defined either by bus or by driver itself.

That said, since you are not using _raw variants of the accessors,
the above will give you a wrong result on BE.

Hence

	*val = sign_extend32(&value), 15);

should be enough.

(Note, the _raw variants take void pointer on purpose.)

...

> > > +     regmap = dev_get_regmap(dev, NULL);
> > > +     if (!regmap) {
> > > +             dev_err(dev, "No regmap\n");
> > > +             return -EINVAL;
> >
> > Why not dev_err_probe()?
> 
> There was no int return value from dev_get_regmap(),
> I think I can use -EINVAL for err in dev_err_probe as well.

Yes, it's fine.

> > > +     }

...

> > > +static int bmi323_regmap_i2c_write(void *context, const void *data,
> > > +                                size_t count)
> > > +{
> > > +     struct device *dev = context;
> > > +     struct i2c_client *i2c = to_i2c_client(dev);
> > > +     int ret;
> > > +     u8 reg;
> > > +
> > > +     reg = *(u8 *)data;
> > > +     ret = i2c_smbus_write_i2c_block_data(i2c, reg, count - 1,
> > > +                                          data + sizeof(u8));
> > > +
> > > +     return ret;
> > > +}
> >
> > Hmm... Don't we have a better approach for these? regmap doesn't provide SMBus
> > accessors?
> 
> Custom implementation is required for the 'read' operation, while
> 'write' can utilize the regmap SMBus accessors. Is it okay to have
> only custom read while write uses the SMBus accessor?

Yes, why not, but I don't know if regmap API allows this.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 2/2] iio: imu: Add driver for BMI323 IMU

[Jagath Jog J]

Hi Andy,


On Wed, Sep 20, 2023 at 4:13 AM Jagath Jog J <jagathjog1996@gmail.com> wrote:
>
> On Mon, Sep 18, 2023 at 3:34 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Mon, Sep 18, 2023 at 01:33:14PM +0530, Jagath Jog J wrote:
> > > The Bosch BMI323 is a 6-axis low-power IMU that provide measurements for
> > > acceleration, angular rate, and temperature. This sensor includes
> > > motion-triggered interrupt features, such as a step counter, tap detection,
> > > and activity/inactivity interrupt capabilities.
> > >

...

> > > +static const struct attribute_group bmi323_event_attribute_group = {
> > > +     .attrs = bmi323_event_attributes,
> > > +};
> >
> > ATTRIBUTE_GROUPS() ?
>
> Okay, I will use ATTRIBUTE_GROUPS.

The ATTRIBUTE_GROUP(bmi323_event) macro will define two variables,
bmi323_event_groups and bmi323_event_group. The event_attrs member
of iio_info is of type struct attribute_group*, which means that
bmi323_event_groups will remain unused. Since I am using a single
event group, Can I keep it the same way?


Regards
Jagath

Jagath

Re: [RFC 2/2] iio: imu: Add driver for BMI323 IMU

[Jonathan Cameron]

On Sun, 8 Oct 2023 11:55:32 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

> Hi Andy,
> 
> 
> On Wed, Sep 20, 2023 at 4:13 AM Jagath Jog J <jagathjog1996@gmail.com> wrote:
> >
> > On Mon, Sep 18, 2023 at 3:34 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:  
> > >
> > > On Mon, Sep 18, 2023 at 01:33:14PM +0530, Jagath Jog J wrote:  
> > > > The Bosch BMI323 is a 6-axis low-power IMU that provide measurements for
> > > > acceleration, angular rate, and temperature. This sensor includes
> > > > motion-triggered interrupt features, such as a step counter, tap detection,
> > > > and activity/inactivity interrupt capabilities.
> > > >  
> 
> ...
> 
> > > > +static const struct attribute_group bmi323_event_attribute_group = {
> > > > +     .attrs = bmi323_event_attributes,
> > > > +};  
> > >
> > > ATTRIBUTE_GROUPS() ?  
> >
> > Okay, I will use ATTRIBUTE_GROUPS.  
> 
> The ATTRIBUTE_GROUP(bmi323_event) macro will define two variables,
> bmi323_event_groups and bmi323_event_group. The event_attrs member
> of iio_info is of type struct attribute_group*, which means that
> bmi323_event_groups will remain unused. Since I am using a single
> event group, Can I keep it the same way?

Yes, don't use that macro as not appropriate in this case.


> 
> 
> Regards
> Jagath
> 
> Jagath

Re: [RFC 2/2] iio: imu: Add driver for BMI323 IMU

[Jonathan Cameron]

On Mon, 18 Sep 2023 13:33:14 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

> The Bosch BMI323 is a 6-axis low-power IMU that provide measurements for
> acceleration, angular rate, and temperature. This sensor includes
> motion-triggered interrupt features, such as a step counter, tap detection,
> and activity/inactivity interrupt capabilities.
> 
> The driver supports various functionalities, including data ready, FIFO
> data handling, and events such as tap detection, step counting, and
> activity interrupts
> 
> Datasheet: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi323-ds000.pdf
> Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>

Given Andy has done a thorough review (as he always does!)
and there is quite a bit in my review queue, I'll just scan
through quickly and call out a few things.  Will take a closer
look at next version.

One thing that is useful for a complex driver like this is to include
(typically in the cover letter) a full listing of what shows up in sysfs.
That gives an easy way to check the ABI looks right without having to figure
out what all the generated file names are from the code.

Thanks,

Jonathan



> ---
>  MAINTAINERS                          |    7 +
>  drivers/iio/imu/Kconfig              |    1 +
>  drivers/iio/imu/Makefile             |    1 +
>  drivers/iio/imu/bmi323/Kconfig       |   33 +
>  drivers/iio/imu/bmi323/Makefile      |    7 +
>  drivers/iio/imu/bmi323/bmi323.h      |  198 +++
>  drivers/iio/imu/bmi323/bmi323_core.c | 2260 ++++++++++++++++++++++++++
>  drivers/iio/imu/bmi323/bmi323_i2c.c  |  115 ++
>  drivers/iio/imu/bmi323/bmi323_spi.c  |  106 ++
>  9 files changed, 2728 insertions(+)
>  create mode 100644 drivers/iio/imu/bmi323/Kconfig
>  create mode 100644 drivers/iio/imu/bmi323/Makefile
>  create mode 100644 drivers/iio/imu/bmi323/bmi323.h
>  create mode 100644 drivers/iio/imu/bmi323/bmi323_core.c
>  create mode 100644 drivers/iio/imu/bmi323/bmi323_i2c.c
>  create mode 100644 drivers/iio/imu/bmi323/bmi323_spi.c
> 

> diff --git a/drivers/iio/imu/bmi323/bmi323_core.c b/drivers/iio/imu/bmi323/bmi323_core.c
> new file mode 100644
> index 000000000000..adbcfbc6b97d
> --- /dev/null
> +++ b/drivers/iio/imu/bmi323/bmi323_core.c
> @@ -0,0 +1,2260 @@

> +struct bmi323_data {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct iio_mount_matrix orientation;
> +	enum bmi323_irq_pin irq_pin;
> +	struct iio_trigger *trig;
> +	bool drdy_trigger_enabled;
> +	enum bmi323_state state;
> +	s64 fifo_tstamp, old_fifo_tstamp;
> +	u32 odrns[2];
> +	u32 odrhz[2];
> +	unsigned int feature_events;
> +
> +	/*
> +	 * Lock to protect the members of device's private data from concurrent
> +	 * access and also to serialize the access of extended registers.
> +	 * See bmi323_write_ext_reg(..) for more info.
> +	 */
> +	struct mutex mutex;
> +	int watermark;
> +	__le16 fifo_buff[BMI323_FIFO_FULL_IN_WORDS] __aligned(IIO_DMA_MINALIGN);
> +	struct {
> +		__le16 channels[6];
> +		s64 ts __aligned(8);

Hopefully Andy's aligned_s64 set will land soon and we can tidy this up.
I'm a bit unsure of this, but can you overlap some of these buffers or are
they used concurrently? (if they are then we have problems with DMA safety.)

Perhaps an anonymous union is appropriate?



> +	} buffer;
> +	__le16 steps_count[2];
> +};





> +static int bmi323_buffer_preenable(struct iio_dev *indio_dev)
> +{
> +	struct bmi323_data *data = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	mutex_lock(&data->mutex);
> +	/*
> +	 * When the ODR of the accelerometer and gyroscope do not match, the
> +	 * maximum ODR value between the accelerometer and gyroscope is used
> +	 * for FIFO and the signal with lower ODR will insert dummy frame.
yuk.
> +	 * So allow buffer read only when ODR's of accelero and gyro are equal.
> +	 * See datasheet section 5.7 "FIFO Data Buffering".

Good :)

> +	 */
> +	if (data->odrns[BMI323_ACCEL] != data->odrns[BMI323_GYRO]) {
> +		dev_err(data->dev, "Accelero and Gyro ODR doesn't match\n");
> +		ret = -EINVAL;
> +	}
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}

> +
> +static int bmi323_set_drdy_irq(struct bmi323_data *data,
> +			       enum bmi323_irq_pin irq_pin)
> +{
> +	int ret;
> +
> +	ret = regmap_update_bits(data->regmap, BMI323_INT_MAP2_REG,
> +				 BMI323_GYR_DRDY_MSK,
> +				 FIELD_PREP(BMI323_GYR_DRDY_MSK, irq_pin));
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(data->regmap, BMI323_INT_MAP2_REG,
> +				  BMI323_ACC_DRDY_MSK,
> +				  FIELD_PREP(BMI323_ACC_DRDY_MSK, irq_pin));
> +}
> +
> +static int bmi323_data_rdy_trigger_set_state(struct iio_trigger *trig,
> +					     bool state)
> +{
> +	struct bmi323_data *data = iio_trigger_get_drvdata(trig);
> +	enum bmi323_irq_pin irq_pin;
> +	int ret;
> +
> +	mutex_lock(&data->mutex);
guard(mutex)(&data->mutex);

> +
> +	if (data->state == BMI323_BUFFER_FIFO) {
> +		dev_warn(data->dev, "Can't set trigger when FIFO enabled\n");
> +		ret = -EBUSY;
> +		goto unlock_out;
> +	}
> +
> +	if (state) {
> +		data->state = BMI323_BUFFER_DRDY_TRIGGERED;
> +		irq_pin = data->irq_pin;
> +	} else {
> +		data->state = BMI323_IDLE;
> +		irq_pin = BMI323_IRQ_DISABLED;
> +	}
> +
> +	ret = bmi323_set_drdy_irq(data, irq_pin);
> +
> +unlock_out:
> +	mutex_unlock(&data->mutex);
> +	return ret;
> +}
> +
> +static const struct iio_trigger_ops bmi323_trigger_ops = {
> +	.set_trigger_state = &bmi323_data_rdy_trigger_set_state,
> +};
> +
> +static irqreturn_t bmi323_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct bmi323_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	/* Lock to protect the data->buffer */
> +	mutex_lock(&data->mutex);

scoped_guard(mutex)(&data->mutex); might work well here though it
will push up the indent.

> +	ret = regmap_bulk_read(data->regmap, BMI323_ACCEL_X_REG,
> +			       &data->buffer.channels,
> +			       ARRAY_SIZE(data->buffer.channels));
> +	if (ret)
> +		goto unlock_out;
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, &data->buffer,
> +					   iio_get_time_ns(indio_dev));
> +	mutex_unlock(&data->mutex);
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +
> +unlock_out:
> +	mutex_unlock(&data->mutex);
> +	return IRQ_NONE;
> +}
> +



> +static IIO_DEVICE_ATTR_RW(in_accel_gyro_averaging, 0);
> +static IIO_CONST_ATTR(in_accel_gyro_averaging_available, "2 4 8 16 32 64");
> +
> +static struct attribute *bmi323_attributes[] = {
> +	&iio_dev_attr_in_accel_gyro_averaging.dev_attr.attr,
> +	&iio_const_attr_in_accel_gyro_averaging_available.dev_attr.attr,

So averaging often maps directly to oversampling.  Kind of different names
for the same thing.  Perhaps that standard ABI can be used?
It tends to make sampling frequency reporting need to take it into account
though as that drops as divided by oversampling ratio.

> +	NULL
> +};
> +

> +}
> +
> +static int bmi323_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int val,
> +			    int val2, long mask)
> +{
> +	struct bmi323_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
> +
> +		mutex_lock(&data->mutex);
> +		ret = bmi323_set_odr(data, bmi323_iio_to_sensor(chan->type),
> +				     val, val2);
> +		mutex_unlock(&data->mutex);
> +		iio_device_release_direct_mode(indio_dev);
> +		return ret;
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = iio_device_claim_direct_mode(indio_dev);
> +		if (ret)
> +			return ret;
> +
> +		mutex_lock(&data->mutex);
> +		ret = bmi323_set_scale(data, bmi323_iio_to_sensor(chan->type),
> +				       val, val2);
> +		mutex_unlock(&data->mutex);
> +		iio_device_release_direct_mode(indio_dev);
> +		return ret;
> +	case IIO_CHAN_INFO_ENABLE:
> +		mutex_lock(&data->mutex);
> +		ret = bmi323_enable_steps(data, val);
> +		mutex_unlock(&data->mutex);
> +		return ret;
> +	case IIO_CHAN_INFO_PROCESSED:
> +		if (val || !FIELD_GET(BMI323_FEAT_IO0_STP_CNT_MSK,
> +				      data->feature_events))
> +			return -EINVAL;
> +
> +		/* Clear step counter value */
> +		return bmi323_update_ext_reg(data, BMI323_STEP_SC1_REG,
> +					     BMI323_STEP_SC1_RST_CNT_MSK,
> +					     FIELD_PREP(BMI323_STEP_SC1_RST_CNT_MSK,
> +							1));
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int bmi323_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long mask)
> +{
> +	struct bmi323_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_PROCESSED:
> +		mutex_lock(&data->mutex);

	scoped_guard() might be mice for some of these cases
or push the guards down into the function being called perhaps.


> +		ret = bmi323_read_steps(data, val);
> +		mutex_unlock(&data->mutex);
> +		return ret;
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +		case IIO_ANGL_VEL:
> +			ret = iio_device_claim_direct_mode(indio_dev);
> +			if (ret)
> +				return ret;
> +
> +			mutex_lock(&data->mutex);
> +			ret = bmi323_read_axis(data, chan, val);
> +			mutex_unlock(&data->mutex);
> +
> +			iio_device_release_direct_mode(indio_dev);
> +			return ret;
> +		case IIO_TEMP:
> +			mutex_lock(&data->mutex);
> +			ret = bmi323_get_temp_data(data, val);
> +			mutex_unlock(&data->mutex);
> +			return ret;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		mutex_lock(&data->mutex);
> +		ret = bmi323_get_odr(data, chan->type, val, val2);
> +		mutex_unlock(&data->mutex);
> +		return ret;
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_ACCEL:
> +		case IIO_ANGL_VEL:
> +			*val = 0;
> +			mutex_lock(&data->mutex);
> +			ret = bmi323_get_scale(data, chan->type, val2);
> +			mutex_unlock(&data->mutex);
> +			return ret;
> +		case IIO_TEMP:
> +			*val = BMI323_TEMP_SCALE / MEGA;
> +			*val2 = BMI323_TEMP_SCALE % MEGA;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_OFFSET:
> +		switch (chan->type) {
> +		case IIO_TEMP:
> +			*val = BMI323_TEMP_OFFSET;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_ENABLE:
> +		mutex_lock(&data->mutex);
> +		*val = FIELD_GET(BMI323_FEAT_IO0_STP_CNT_MSK,
> +				 data->feature_events);
> +		mutex_unlock(&data->mutex);
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info bmi323_info = {
> +	.read_raw = bmi323_read_raw,
> +	.write_raw = bmi323_write_raw,
> +	.read_avail = bmi323_read_avail,
> +	.attrs = &bmi323_attrs_group,
> +	.hwfifo_set_watermark = bmi323_set_watermark,
> +	.write_event_config = bmi323_write_event_config,
> +	.read_event_config = bmi323_read_event_config,
> +	.write_event_value = bmi323_write_event_value,
> +	.read_event_value = bmi323_read_event_value,
> +	.event_attrs = &bmi323_event_attribute_group,
> +};
> +
> +#define BMI323_SCAN_MASK_ACCEL_3AXIS		\
> +	(BIT(BMI323_ACCEL_X) | BIT(BMI323_ACCEL_Y) | BIT(BMI323_ACCEL_Z))
> +
> +#define BMI323_SCAN_MASK_GYRO_3AXIS		\
> +	(BIT(BMI323_GYRO_X) | BIT(BMI323_GYRO_Y) | BIT(BMI323_GYRO_Z))
> +
> +static const unsigned long bmi323_avail_scan_masks[] = {
> +	/* 3-axis accel + 3-axis gyro */
> +	BMI323_SCAN_MASK_ACCEL_3AXIS | BMI323_SCAN_MASK_GYRO_3AXIS,

Can you poke this an see if you get what you expect which is the minimum
sufficient set of channels.  Matti pointed out earlier that the search
logic isn't well documented in iio_scan_mask_match() but it
looks to match against first case where the requested values are a subset.
So this would need to be in the opposite order or you will always
get everything turned on.

Chances are we have this wrong in other drivers as well :(
Won't break things, but may mean that we over read in some configurations.

> +	/* 3-axis accel */
> +	BMI323_SCAN_MASK_ACCEL_3AXIS,
> +	/* 3-axis gyro */
> +	BMI323_SCAN_MASK_GYRO_3AXIS,
> +	0
> +};
> +
> +static int bmi323_int_pin_config(struct bmi323_data *data,
> +				 enum bmi323_irq_pin irq_pin,
> +				 bool active_high, bool open_drain, bool latch)
> +{
> +	unsigned int mask, field_value = 0;
> +	int ret;
> +
> +	switch (irq_pin) {
> +	case BMI323_IRQ_INT1:
> +		mask = BMI323_IO_INT1_LVL_OD_OP_MSK;
> +
> +		set_mask_bits(&field_value, BMI323_IO_INT1_LVL_MSK,
> +			      FIELD_PREP(BMI323_IO_INT1_LVL_MSK, active_high));
> +
> +		set_mask_bits(&field_value, BMI323_IO_INT1_OD_MSK,
> +			      FIELD_PREP(BMI323_IO_INT1_OD_MSK, open_drain));
> +
> +		set_mask_bits(&field_value, BMI323_IO_INT1_OP_EN_MSK,
> +			      FIELD_PREP(BMI323_IO_INT1_OP_EN_MSK, 1));
Given you are filling in a value that starts as zero and don't overwrite values
you set earlier this is much simpler as
		mask = ...
		field_value = FIELD_PREP(...) |
			      FIELD_PREP(...) |
			      FIELD_PREP(...);

Also move working this out to just before it is used. Right now it is hard
to spot where that is.

> +		break;
> +	case BMI323_IRQ_INT2:
> +		mask = BMI323_IO_INT2_LVL_OD_OP_MSK;
> +
> +		set_mask_bits(&field_value, BMI323_IO_INT2_LVL_MSK,
> +			      FIELD_PREP(BMI323_IO_INT2_LVL_MSK, active_high));
> +
> +		set_mask_bits(&field_value, BMI323_IO_INT2_OD_MSK,
> +			      FIELD_PREP(BMI323_IO_INT2_OD_MSK, open_drain));
> +
> +		set_mask_bits(&field_value, BMI323_IO_INT2_OP_EN_MSK,
> +			      FIELD_PREP(BMI323_IO_INT2_OP_EN_MSK, 1));
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_update_bits(data->regmap, BMI323_IO_INT_CONF_REG,
> +				 BMI323_IO_INT_LTCH_MSK,
> +				 FIELD_PREP(BMI323_IO_INT_LTCH_MSK, latch));
> +	if (ret)
> +		return ret;
> +
> +	ret = bmi323_update_ext_reg(data, BMI323_GEN_SET1_REG,
> +				    BMI323_GEN_HOLD_DUR_MSK,
> +				    FIELD_PREP(BMI323_GEN_HOLD_DUR_MSK, 0));
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(data->regmap, BMI323_IO_INT_CTR_REG, mask,
> +				  field_value);
> +}
> +
> +static int bmi323_trigger_probe(struct bmi323_data *data,
> +				struct iio_dev *indio_dev)
> +{
> +	bool open_drain, active_high, latch;
> +	struct fwnode_handle *fwnode;
> +	enum bmi323_irq_pin irq_pin;
> +	int ret, irq, irq_type;
> +	struct irq_data *desc;
> +
> +	fwnode = dev_fwnode(data->dev);
> +	if (!fwnode)
> +		return -ENODEV;
> +
> +	irq = fwnode_irq_get_byname(fwnode, "INT1");
> +	if (irq > 0) {
> +		irq_pin = BMI323_IRQ_INT1;
> +	} else {
> +		irq = fwnode_irq_get_byname(fwnode, "INT2");
> +		if (irq <= 0)
> +			return 0;
> +
> +		irq_pin = BMI323_IRQ_INT2;
> +	}
> +
> +	desc = irq_get_irq_data(irq);
> +	if (!desc) {
> +		dev_err(data->dev, "Could not find IRQ %d\n", irq);
> +		return -EINVAL;
> +	}
> +
> +	irq_type = irqd_get_trigger_type(desc);
> +
> +	switch (irq_type) {
> +	case IRQF_TRIGGER_RISING:
> +		latch = false;
> +		active_high = true;
> +		break;
> +	case IRQF_TRIGGER_HIGH:
> +		latch = true;
> +		active_high = true;
> +		break;
> +	case IRQF_TRIGGER_FALLING:
> +		latch = false;
> +		active_high = false;
> +		break;
> +	case IRQF_TRIGGER_LOW:
> +		latch = true;
> +		active_high = false;
> +		break;
> +	default:
> +		dev_err(data->dev, "Invalid interrupt type 0x%x specified\n",
> +			irq_type);
> +		return -EINVAL;
> +	}
> +
> +	open_drain = fwnode_property_read_bool(fwnode, "drive-open-drain");
> +
> +	ret = bmi323_int_pin_config(data, irq_pin, active_high, open_drain,
> +				    latch);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret,
> +				     "Failed to configure irq line\n");
> +
> +	data->trig = devm_iio_trigger_alloc(data->dev, "%s-trig-%d",
> +					    indio_dev->name, irq_pin);
> +	if (!data->trig)
> +		return -ENOMEM;
> +
> +	data->trig->ops = &bmi323_trigger_ops;
> +	iio_trigger_set_drvdata(data->trig, data);
> +
> +	ret = devm_request_threaded_irq(data->dev, irq, NULL,
> +					bmi323_irq_thread_handler,
> +					irq_type | IRQF_ONESHOT,

I think if you leave the irq_type bit out, it will be set up to match what was
specified in firmware anyway. Could be wrong on that though so check.

> +					"bmi323-int", indio_dev);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret, "Failed to request IRQ\n");
> +
> +	ret = devm_iio_trigger_register(data->dev, data->trig);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret,
> +				     "Trigger registration failed\n");
> +
> +	data->irq_pin = irq_pin;
> +
> +	return ret;
> +}
> +
> +static int bmi323_feature_engine_enable(struct bmi323_data *data, bool en)
> +{
> +	unsigned int feature_status;
> +	int ret, i;
> +
> +	if (en) {
> +		ret = regmap_write(data->regmap, BMI323_FEAT_IO2_REG,
> +				   0x012c);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_write(data->regmap, BMI323_FEAT_IO_STATUS_REG,
> +				   BMI323_FEAT_IO_STATUS_MSK);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_write(data->regmap, BMI323_FEAT_CTRL_REG,
> +				   BMI323_FEAT_ENG_EN_MSK);
> +		if (ret)
> +			return ret;
> +
> +		i = 5;

Why 5?

> +		do {
> +			ret = regmap_read(data->regmap, BMI323_FEAT_IO1_REG,
> +					  &feature_status);
> +			if (ret)
> +				return ret;
> +
> +			i--;
> +			mdelay(2);
> +		} while (feature_status != 0x01 && i);

GET_FIELD() always rather than directly masking out values
It means we have a clear name for what is being checked.

> +
> +		if (feature_status != 0x01) {
> +			dev_err(data->dev, "Failed to enable feature engine\n");
> +			return -EINVAL;
> +		}
> +
> +		return ret;
> +	} else {
> +		return regmap_write(data->regmap, BMI323_FEAT_CTRL_REG, 0);
Flip the logic for readability by reducing indent
	if (!en)
		return regmap_write()

	ret = regmap_write() etc

> +	}
> +}

> +static int bmi323_init(struct bmi323_data *data)
> +{
> +	int ret, val;
> +
> +	/*
> +	 * Perform soft reset to make sure the device is in a known state after
> +	 * start up. A delay of 1.5 ms is required after reset.
> +	 * See datasheet section 5.17 "Soft Reset".
> +	 */
> +	ret = regmap_write(data->regmap, BMI323_CMD_REG, BMI323_RST_VAL);
> +	if (ret)
> +		return ret;
> +
> +	usleep_range(1500, 2000);
> +
> +	/*
> +	 * Dummy read is required to enable SPI interface after reset.
> +	 * See datasheet section 7.2.1 "Protocol Selection".
> +	 */
> +	regmap_read(data->regmap, BMI323_CHIP_ID_REG, &val);
> +
> +	ret = regmap_read(data->regmap, BMI323_CHIP_ID_REG, &val);
> +	if (ret)
> +		return ret;
> +
> +	if ((val & 0xFF) != BMI323_CHIP_ID_VAL) {

FIELD_GET() and appropriate mask given this is half of a 16 bit register.

> +		dev_err(data->dev, "Chip ID mismatch\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = bmi323_feature_engine_enable(data, true);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap, BMI323_ERR_REG, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val) {
> +		dev_err(data->dev, "Sensor power error = 0x%x\n", val);
> +		return -EINVAL;
I think this is only called in probe() so an use dev_err_probe()
in here as well.

> +	}
> +
> +	ret = regmap_read(data->regmap, BMI323_STATUS_REG, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val != 0x01) {

FIELD_GET() and appropriate definition.

> +		dev_err(data->dev, "Sensor initialization error = 0x%x\n", val);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Set the Bandwidth coefficient which defines the 3 dB cutoff
> +	 * frequency in relation to the ODR.
> +	 */
> +	ret = bmi323_set_bw(data, BMI323_ACCEL, BMI323_BW_ODR_BY_2);
> +	if (ret)
> +		return -EINVAL;
> +
> +	ret = bmi323_set_bw(data, BMI323_GYRO, BMI323_BW_ODR_BY_2);
> +	if (ret)
> +		return -EINVAL;
> +
> +	ret = bmi323_set_odr(data, BMI323_ACCEL, 25, 0);
> +	if (ret)
> +		return -EINVAL;
> +
> +	ret = bmi323_set_odr(data, BMI323_GYRO, 25, 0);
> +	if (ret)
> +		return -EINVAL;
> +
> +	ret = devm_add_action_or_reset(data->dev, bmi323_disable, data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +int bmi323_core_probe(struct device *dev)
> +{
> +	static const char * const regulator_names[] = { "vdd", "vddio" };
> +	struct iio_dev *indio_dev;
> +	struct bmi323_data *data;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	regmap = dev_get_regmap(dev, NULL);
> +	if (!regmap) {
> +		dev_err(dev, "No regmap\n");
> +		return -EINVAL;
> +	}
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulator_names),
> +					     regulator_names);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable regulators\n");
> +
> +	data = iio_priv(indio_dev);
> +	data->dev = dev;
> +	data->regmap = regmap;
> +	mutex_init(&data->mutex);
> +
> +	ret = bmi323_init(data);
> +	if (ret)
> +		return -EINVAL;
> +
> +	ret = iio_read_mount_matrix(dev, &data->orientation);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = "bmi323-imu";
> +	indio_dev->info = &bmi323_info;
> +	indio_dev->channels = bmi323_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(bmi323_channels);
> +	indio_dev->available_scan_masks = bmi323_avail_scan_masks;
> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> +	dev_set_drvdata(data->dev, indio_dev);
> +
> +	ret = bmi323_trigger_probe(data, indio_dev);

What if interrupt isn't wired?  Do we need it for basic read of channels?
Would expect the interfaces provided to be more limited, but not that we
would provide none at all.

> +	if (ret)
> +		return -EINVAL;
> +
> +	ret = devm_iio_triggered_buffer_setup_ext(data->dev, indio_dev,
> +						  &iio_pollfunc_store_time,
> +						  bmi323_trigger_handler,
> +						  IIO_BUFFER_DIRECTION_IN,
> +						  &bmi323_buffer_ops,
> +						  bmi323_fifo_attributes);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret,
> +				     "Failed to setup trigger buffer\n");
> +
> +	ret = devm_iio_device_register(data->dev, indio_dev);
> +	if (ret)
> +		return dev_err_probe(data->dev, ret,
> +				     "Unable to register iio device\n");
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(bmi323_core_probe, IIO_BMI323);
> +
> +MODULE_DESCRIPTION("Bosch BMI323 IMU driver");
> +MODULE_AUTHOR("Jagath Jog J <jagathjog1996@gmail.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/iio/imu/bmi323/bmi323_i2c.c b/drivers/iio/imu/bmi323/bmi323_i2c.c
> new file mode 100644
> index 000000000000..afbaa3d3c638
> --- /dev/null
> +++ b/drivers/iio/imu/bmi323/bmi323_i2c.c
> @@ -0,0 +1,115 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * I2C driver for Bosch BMI323 6-Axis IMU.
> + *
> + * Copyright (C) 2023, Jagath Jog J <jagathjog1996@gmail.com>
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +#include "bmi323.h"
> +
> +/*
> + * From BMI323 datasheet section 4: Notes on the Serial Interface Support.
> + * Each I2C register read operation requires to read two dummy bytes before
> + * the actual payload.
> + */
> +static int bmi323_regmap_i2c_read(void *context, const void *reg_buf,
> +				  size_t reg_size, void *val_buf,
> +				  size_t val_size)
> +{
> +	struct device *dev = context;
> +	struct i2c_client *i2c = to_i2c_client(dev);
> +	struct i2c_msg msgs[2];
> +	u8 *buff = NULL;
> +	int ret;
> +
> +	buff = kmalloc(val_size + BMI323_I2C_DUMMY, GFP_KERNEL);
> +	if (!buff)
> +		return -ENOMEM;
> +
> +	msgs[0].addr = i2c->addr;
> +	msgs[0].flags = i2c->flags;
> +	msgs[0].len = reg_size;
> +	msgs[0].buf = (u8 *)reg_buf;
> +
> +	msgs[1].addr = i2c->addr;
> +	msgs[1].len = val_size + BMI323_I2C_DUMMY;
> +	msgs[1].buf = (u8 *)buff;
> +	msgs[1].flags = i2c->flags | I2C_M_RD;
> +
> +	ret = i2c_transfer(i2c->adapter, msgs, ARRAY_SIZE(msgs));
> +	if (ret < 0) {
> +		kfree(buff);
> +		return -EIO;
> +	}
> +
> +	memcpy(val_buf, buff + BMI323_I2C_DUMMY, val_size);

Annoyingly can't do same trick as I suggest for SPI as we need
the address send vs when data turns up to be correct.

Whilst this code is 'generic' do you know the max size of the
buffer that might be read?  If it's small I'd just use an array
on the stack.  If large, then the cleanup.h stuff will help
with code, but it's still annoying to have to do an allocation
in here.  You can probably put something in context alongside
dev.

> +	kfree(buff);
> +
> +	return 0;
> +}
> +
> +static int bmi323_regmap_i2c_write(void *context, const void *data,
> +				   size_t count)
> +{
> +	struct device *dev = context;
> +	struct i2c_client *i2c = to_i2c_client(dev);
> +	int ret;
> +	u8 reg;
> +
> +	reg = *(u8 *)data;
> +	ret = i2c_smbus_write_i2c_block_data(i2c, reg, count - 1,
> +					     data + sizeof(u8));
> +
	return i2c_smbus...

> +	return ret;
> +}
> +
> +static struct regmap_bus bmi323_regmap_bus = {
> +	.read = bmi323_regmap_i2c_read,
> +	.write = bmi323_regmap_i2c_write,
> +};
> +
> +static int bmi323_i2c_probe(struct i2c_client *i2c)
> +{
> +	struct device *dev = &i2c->dev;
> +	struct regmap *regmap;
> +
> +	regmap = devm_regmap_init(dev, &bmi323_regmap_bus, dev,
> +				  &bmi323_regmap_config);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(dev, PTR_ERR(regmap),
> +				     "Failed to initialize I2C Regmap\n");
> +
> +	return bmi323_core_probe(dev);
> +}
> +
> +static const struct i2c_device_id bmi323_i2c_ids[] = {
> +	{ "bmi323", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, bmi323_i2c_ids);
> +
> +static const struct of_device_id bmi323_of_i2c_match[] = {
> +	{ .compatible = "bosch,bmi323" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, bmi323_of_i2c_match);
> +
> +static struct i2c_driver bmi323_i2c_driver = {
> +	.driver = {
> +		.name = "bmi323",
> +		.of_match_table = bmi323_of_i2c_match,
> +	},
> +	.probe_new = bmi323_i2c_probe,
> +	.id_table = bmi323_i2c_ids,
> +};
> +module_i2c_driver(bmi323_i2c_driver);
> +
> +MODULE_DESCRIPTION("Bosch BMI323 IMU driver");
> +MODULE_AUTHOR("Jagath Jog J <jagathjog1996@gmail.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(IIO_BMI323);
> diff --git a/drivers/iio/imu/bmi323/bmi323_spi.c b/drivers/iio/imu/bmi323/bmi323_spi.c
> new file mode 100644
> index 000000000000..2b802a0c6d9d
> --- /dev/null
> +++ b/drivers/iio/imu/bmi323/bmi323_spi.c
> @@ -0,0 +1,106 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SPI driver for Bosch BMI323 6-Axis IMU.
> + *
> + * Copyright (C) 2023, Jagath Jog J <jagathjog1996@gmail.com>
> + */
> +
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/spi/spi.h>
> +
> +#include "bmi323.h"
> +
> +/*
> + * From BMI323 datasheet section 4: Notes on the Serial Interface Support.
> + * Each SPI register read operation requires to read one dummy byte before
> + * the actual payload.
> + */
> +static int bmi323_regmap_spi_read(void *context, const void *reg_buf,
> +				  size_t reg_size, void *val_buf,
> +				  size_t val_size)
> +{
> +	struct spi_device *spi = context;
> +	u8 reg, *buff = NULL;
> +	int ret;
> +
> +	buff = kmalloc(val_size + BMI323_SPI_DUMMY, GFP_KERNEL);

Hmm.  Regmap has pad_bits (which can be multiple bytes) but this case
is unusual in that they only apply to reads.

I wonder if we can make this cheaper though rather than having
to handle either some context or having dynamic allocations in here.

How about making the write bigger?  Does that have any effect?
Looks like don't care state in Figure 31.  If that's the case,
send some zeros on that as it's known fixed size (2 bytes including
the padding) and then you can directly use the read buffer without
yet another memcpy.


> +	if (!buff)
> +		return -ENOMEM;
> +
> +	reg = *(u8 *)reg_buf | 0x80;
> +	ret = spi_write_then_read(spi, &reg, sizeof(reg), buff,
> +				  val_size + BMI323_SPI_DUMMY);
> +	if (ret) {
> +		kfree(buff);
> +		return ret;
> +	}
> +
> +	memcpy(val_buf, buff + BMI323_SPI_DUMMY, val_size);
> +	kfree(buff);
> +
> +	return ret;
> +}
> +
> +static int bmi323_regmap_spi_write(void *context, const void *data,
> +				   size_t count)
> +{
> +	struct spi_device *spi = context;
> +
> +	return spi_write(spi, data, count);
> +}
> +
> +static struct regmap_bus bmi323_regmap_bus = {
> +	.read = bmi323_regmap_spi_read,
> +	.write = bmi323_regmap_spi_write,
> +};

Re: [RFC 2/2] iio: imu: Add driver for BMI323 IMU

[Jagath Jog J]

Hi Jonathan,

Thank you for the review.

On Sun, Sep 24, 2023 at 8:01 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Mon, 18 Sep 2023 13:33:14 +0530
> Jagath Jog J <jagathjog1996@gmail.com> wrote:
>
> > The Bosch BMI323 is a 6-axis low-power IMU that provide measurements for
> > acceleration, angular rate, and temperature. This sensor includes
> > motion-triggered interrupt features, such as a step counter, tap detection,
> > and activity/inactivity interrupt capabilities.
> >
> > The driver supports various functionalities, including data ready, FIFO
> > data handling, and events such as tap detection, step counting, and
> > activity interrupts
> >
> > Datasheet: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi323-ds000.pdf
> > Signed-off-by: Jagath Jog J <jagathjog1996@gmail.com>
>
> Given Andy has done a thorough review (as he always does!)
> and there is quite a bit in my review queue, I'll just scan
> through quickly and call out a few things.  Will take a closer
> look at next version.
>
> One thing that is useful for a complex driver like this is to include
> (typically in the cover letter) a full listing of what shows up in sysfs.
> That gives an easy way to check the ABI looks right without having to figure
> out what all the generated file names are from the code.

Sure, I will add all ABI's in the cover letter.

Please note that I omitted certain portions of your reviews
while responding, and I agree with the review comments that
I didn't address. I intend to make the necessary corrections
in the next series.

>
> Thanks,
>
> Jonathan
>

> > +struct bmi323_data {
> > +     struct device *dev;
> > +     struct regmap *regmap;
> > +     struct iio_mount_matrix orientation;
> > +     enum bmi323_irq_pin irq_pin;
> > +     struct iio_trigger *trig;
> > +     bool drdy_trigger_enabled;
> > +     enum bmi323_state state;
> > +     s64 fifo_tstamp, old_fifo_tstamp;
> > +     u32 odrns[2];
> > +     u32 odrhz[2];
> > +     unsigned int feature_events;
> > +
> > +     /*
> > +      * Lock to protect the members of device's private data from concurrent
> > +      * access and also to serialize the access of extended registers.
> > +      * See bmi323_write_ext_reg(..) for more info.
> > +      */
> > +     struct mutex mutex;
> > +     int watermark;
> > +     __le16 fifo_buff[BMI323_FIFO_FULL_IN_WORDS] __aligned(IIO_DMA_MINALIGN);
> > +     struct {
> > +             __le16 channels[6];
> > +             s64 ts __aligned(8);
>
> Hopefully Andy's aligned_s64 set will land soon and we can tidy this up.
> I'm a bit unsure of this, but can you overlap some of these buffers or are
> they used concurrently? (if they are then we have problems with DMA safety.)
>
> Perhaps an anonymous union is appropriate?

Yes both buffers are used at the same time. In fifo_flush
fifo_buff is used to store all fifo data, and buffer is
used to push a single data frame to iio buffers, overlapping
will corrupt the data, so I used separate buffers for both.

> > +static int bmi323_data_rdy_trigger_set_state(struct iio_trigger *trig,
> > +                                          bool state)
> > +{
> > +     struct bmi323_data *data = iio_trigger_get_drvdata(trig);
> > +     enum bmi323_irq_pin irq_pin;
> > +     int ret;
> > +
> > +     mutex_lock(&data->mutex);
> guard(mutex)(&data->mutex);

Andy also suggested the same.
Sure I will use helpers from cleanup.h.

> > +static IIO_DEVICE_ATTR_RW(in_accel_gyro_averaging, 0);
> > +static IIO_CONST_ATTR(in_accel_gyro_averaging_available, "2 4 8 16 32 64");
> > +
> > +static struct attribute *bmi323_attributes[] = {
> > +     &iio_dev_attr_in_accel_gyro_averaging.dev_attr.attr,
> > +     &iio_const_attr_in_accel_gyro_averaging_available.dev_attr.attr,
>
> So averaging often maps directly to oversampling.  Kind of different names
> for the same thing.  Perhaps that standard ABI can be used?
> It tends to make sampling frequency reporting need to take it into account
> though as that drops as divided by oversampling ratio.

Yes, oversampling can be used, but changing the average
value doesn't alter the sampling frequency. The sampling
frequency is same even with the increase in averaging value.

> > +#define BMI323_SCAN_MASK_ACCEL_3AXIS         \
> > +     (BIT(BMI323_ACCEL_X) | BIT(BMI323_ACCEL_Y) | BIT(BMI323_ACCEL_Z))
> > +
> > +#define BMI323_SCAN_MASK_GYRO_3AXIS          \
> > +     (BIT(BMI323_GYRO_X) | BIT(BMI323_GYRO_Y) | BIT(BMI323_GYRO_Z))
> > +
> > +static const unsigned long bmi323_avail_scan_masks[] = {
> > +     /* 3-axis accel + 3-axis gyro */
> > +     BMI323_SCAN_MASK_ACCEL_3AXIS | BMI323_SCAN_MASK_GYRO_3AXIS,
>
> Can you poke this an see if you get what you expect which is the minimum
> sufficient set of channels.  Matti pointed out earlier that the search
> logic isn't well documented in iio_scan_mask_match() but it
> looks to match against first case where the requested values are a subset.
> So this would need to be in the opposite order or you will always
> get everything turned on.

Sure, I will check and update you further on this.

>
> Chances are we have this wrong in other drivers as well :(
> Won't break things, but may mean that we over read in some configurations.
>
> > +     /* 3-axis accel */
> > +     BMI323_SCAN_MASK_ACCEL_3AXIS,
> > +     /* 3-axis gyro */
> > +     BMI323_SCAN_MASK_GYRO_3AXIS,
> > +     0
> > +};
> > +

> > +     irq_type = irqd_get_trigger_type(desc);
> > +
> > +     switch (irq_type) {
> > +     case IRQF_TRIGGER_RISING:
> > +             latch = false;
> > +             active_high = true;
> > +             break;
> > +     case IRQF_TRIGGER_HIGH:
> > +             latch = true;
> > +             active_high = true;
> > +             break;
> > +     case IRQF_TRIGGER_FALLING:
> > +             latch = false;
> > +             active_high = false;
> > +             break;
> > +     case IRQF_TRIGGER_LOW:
> > +             latch = true;
> > +             active_high = false;
> > +             break;
> > +     default:
> > +             dev_err(data->dev, "Invalid interrupt type 0x%x specified\n",
> > +                     irq_type);
> > +             return -EINVAL;
> > +     }
> > +
> > +     open_drain = fwnode_property_read_bool(fwnode, "drive-open-drain");
> > +
> > +     ret = bmi323_int_pin_config(data, irq_pin, active_high, open_drain,
> > +                                 latch);
> > +     if (ret)
> > +             return dev_err_probe(data->dev, ret,
> > +                                  "Failed to configure irq line\n");
> > +
> > +     data->trig = devm_iio_trigger_alloc(data->dev, "%s-trig-%d",
> > +                                         indio_dev->name, irq_pin);
> > +     if (!data->trig)
> > +             return -ENOMEM;
> > +
> > +     data->trig->ops = &bmi323_trigger_ops;
> > +     iio_trigger_set_drvdata(data->trig, data);
> > +
> > +     ret = devm_request_threaded_irq(data->dev, irq, NULL,
> > +                                     bmi323_irq_thread_handler,
> > +                                     irq_type | IRQF_ONESHOT,
>
> I think if you leave the irq_type bit out, it will be set up to match what was
> specified in firmware anyway. Could be wrong on that though so check.

Yes, irq_type is not required __setup_irq() is handling that.
Thanks for pointing it out.

> > +
> > +static int bmi323_feature_engine_enable(struct bmi323_data *data, bool en)
> > +{
> > +     unsigned int feature_status;
> > +     int ret, i;
> > +
> > +     if (en) {
> > +             ret = regmap_write(data->regmap, BMI323_FEAT_IO2_REG,
> > +                                0x012c);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = regmap_write(data->regmap, BMI323_FEAT_IO_STATUS_REG,
> > +                                BMI323_FEAT_IO_STATUS_MSK);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             ret = regmap_write(data->regmap, BMI323_FEAT_CTRL_REG,
> > +                                BMI323_FEAT_ENG_EN_MSK);
> > +             if (ret)
> > +                     return ret;
> > +
> > +             i = 5;
>
> Why 5?

No specific reason, during testing the feature engine was
taking around 4 milliseconds, so I thought of checking
every 2 milliseconds and max of 5 trials.

>
> > +             do {
> > +                     ret = regmap_read(data->regmap, BMI323_FEAT_IO1_REG,
> > +                                       &feature_status);
> > +                     if (ret)
> > +                             return ret;
> > +
> > +                     i--;
> > +                     mdelay(2);
> > +             } while (feature_status != 0x01 && i);

> > +
> > +     indio_dev->name = "bmi323-imu";
> > +     indio_dev->info = &bmi323_info;
> > +     indio_dev->channels = bmi323_channels;
> > +     indio_dev->num_channels = ARRAY_SIZE(bmi323_channels);
> > +     indio_dev->available_scan_masks = bmi323_avail_scan_masks;
> > +     indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> > +     dev_set_drvdata(data->dev, indio_dev);
> > +
> > +     ret = bmi323_trigger_probe(data, indio_dev);
>
> What if interrupt isn't wired?  Do we need it for basic read of channels?
> Would expect the interfaces provided to be more limited, but not that we
> would provide none at all.

Yes, the basic read of channels will be available even
if none of the interrupts are wired and trigger buffer
through sysfs or hrt timer is also available.

>
> > +     if (ret)
> > +             return -EINVAL;
> > +
> > +     ret = devm_iio_triggered_buffer_setup_ext(data->dev, indio_dev,
> > +                                               &iio_pollfunc_store_time,
> > +                                               bmi323_trigger_handler,
> > +                                               IIO_BUFFER_DIRECTION_IN,
> > +                                               &bmi323_buffer_ops,
> > +                                               bmi323_fifo_attributes);
> > +     if (ret)
> > +             return dev_err_probe(data->dev, ret,
> > +                                  "Failed to setup trigger buffer\n");

> > + * From BMI323 datasheet section 4: Notes on the Serial Interface Support.
> > + * Each I2C register read operation requires to read two dummy bytes before
> > + * the actual payload.
> > + */
> > +static int bmi323_regmap_i2c_read(void *context, const void *reg_buf,
> > +                               size_t reg_size, void *val_buf,
> > +                               size_t val_size)
> > +{
> > +     struct device *dev = context;
> > +     struct i2c_client *i2c = to_i2c_client(dev);
> > +     struct i2c_msg msgs[2];
> > +     u8 *buff = NULL;
> > +     int ret;
> > +
> > +     buff = kmalloc(val_size + BMI323_I2C_DUMMY, GFP_KERNEL);
> > +     if (!buff)
> > +             return -ENOMEM;
> > +
> > +     msgs[0].addr = i2c->addr;
> > +     msgs[0].flags = i2c->flags;
> > +     msgs[0].len = reg_size;
> > +     msgs[0].buf = (u8 *)reg_buf;
> > +
> > +     msgs[1].addr = i2c->addr;
> > +     msgs[1].len = val_size + BMI323_I2C_DUMMY;
> > +     msgs[1].buf = (u8 *)buff;
> > +     msgs[1].flags = i2c->flags | I2C_M_RD;
> > +
> > +     ret = i2c_transfer(i2c->adapter, msgs, ARRAY_SIZE(msgs));
> > +     if (ret < 0) {
> > +             kfree(buff);
> > +             return -EIO;
> > +     }
> > +
> > +     memcpy(val_buf, buff + BMI323_I2C_DUMMY, val_size);
>
> Annoyingly can't do same trick as I suggest for SPI as we need
> the address send vs when data turns up to be correct.
>
> Whilst this code is 'generic' do you know the max size of the
> buffer that might be read?  If it's small I'd just use an array
> on the stack.  If large, then the cleanup.h stuff will help
> with code, but it's still annoying to have to do an allocation
> in here.  You can probably put something in context alongside
> dev.

A buffer size of 2028 bytes is required when configuring
the FIFO watermark for maximum capacity. Since the
necessary buffer size is substantial, I am allocating
it dynamically.

I will try to use an additional buffer from the device's
private structure and pass it through the context.
This approach will help reduce the memory allocation
and deallocation on every device access.

> > + * From BMI323 datasheet section 4: Notes on the Serial Interface Support.
> > + * Each SPI register read operation requires to read one dummy byte before
> > + * the actual payload.
> > + */
> > +static int bmi323_regmap_spi_read(void *context, const void *reg_buf,
> > +                               size_t reg_size, void *val_buf,
> > +                               size_t val_size)
> > +{
> > +     struct spi_device *spi = context;
> > +     u8 reg, *buff = NULL;
> > +     int ret;
> > +
> > +     buff = kmalloc(val_size + BMI323_SPI_DUMMY, GFP_KERNEL);
>
> Hmm.  Regmap has pad_bits (which can be multiple bytes) but this case
> is unusual in that they only apply to reads.
>
> I wonder if we can make this cheaper though rather than having
> to handle either some context or having dynamic allocations in here.
>
> How about making the write bigger?  Does that have any effect?
> Looks like don't care state in Figure 31.  If that's the case,
> send some zeros on that as it's known fixed size (2 bytes including
> the padding) and then you can directly use the read buffer without
> yet another memcpy.

For spi with pad_bits=8 and without any custom read and
write functions, regmap_read() works but regmap_write()
does not. Write is also adding 8 bits of padding and
the device is treating it as data.
(7.2.3 SPI Protocol Figure 30)

>
>
> > +     if (!buff)
> > +             return -ENOMEM;
> > +
> > +     reg = *(u8 *)reg_buf | 0x80;
> > +     ret = spi_write_then_read(spi, &reg, sizeof(reg), buff,
> > +                               val_size + BMI323_SPI_DUMMY);
> > +     if (ret) {
> > +             kfree(buff);
> > +             return ret;
> > +     }
> > +
> > +     memcpy(val_buf, buff + BMI323_SPI_DUMMY, val_size);
> > +     kfree(buff);
> > +
> > +     return ret;

Regards
Jagath

Re: [RFC 2/2] iio: imu: Add driver for BMI323 IMU

[Denis Benato]

Hello,

Some devices (as my asus rog ally) have an ACPI node describing a BOSC0200 sensor. The IC being used in those devices is a  bmi323 but as a result of how the ACPI table reports that device, it is detected by the existing kernel module and we have no way of differentiating until after the chip ID probe.

The module loaded is bmc150-accel-i2c.c which currently doesn't support the bmi323 and the loading of the module just fails at chip check.

I have solved the problem by expanding the current bmc150-accel-i2c.c and bmc150-accel-core.c files to handle that IC in almost every part: gyroscope, accelerometer and temperature sensor.

What is the best way of organizing code to have this module mainlined? Is it correct leaving files called bmc150-accel-* even if it is managing another IC and and not just the accelerometer part anymore?

TIA for your time.

Best regards,
Denis Benato

Re: [RFC 2/2] iio: imu: Add driver for BMI323 IMU

[Jagath Jog J]

Hi Denis,

On Thu, Sep 28, 2023 at 2:55 AM Denis Benato <benato.denis96@gmail.com> wrote:
>
> Hello,
>
> Some devices (as my asus rog ally) have an ACPI node describing a BOSC0200 sensor. The IC being used in those devices is a  bmi323 but as a result of how the ACPI table reports that device, it is detected by the existing kernel module and we have no way of differentiating until after the chip ID probe.
>
> The module loaded is bmc150-accel-i2c.c which currently doesn't support the bmi323 and the loading of the module just fails at chip check.

bmc150 driver supports multiple accelerometer sensors such as
bma222, bma280, bmi055 and all of them are having similar
register map, but the bmi323 register map is completely different
from bmc150.


>
> I have solved the problem by expanding the current bmc150-accel-i2c.c and bmc150-accel-core.c files to handle that IC in almost every part: gyroscope, accelerometer and temperature sensor.
>
> What is the best way of organizing code to have this module mainlined? Is it correct leaving files called bmc150-accel-* even if it is managing another IC and and not just the accelerometer part anymore?
>
> TIA for your time.
>
> Best regards,
> Denis Benato

Regards

Jagath

Re: [RFC 2/2] iio: imu: Add driver for BMI323 IMU

[Jonathan Cameron]

On Fri, 29 Sep 2023 13:29:13 +0530
Jagath Jog J <jagathjog1996@gmail.com> wrote:

> Hi Denis,
> 
> On Thu, Sep 28, 2023 at 2:55 AM Denis Benato <benato.denis96@gmail.com> wrote:
> >
> > Hello,
> >
> > Some devices (as my asus rog ally) have an ACPI node describing a BOSC0200 sensor. The IC being used in those devices is a  bmi323 but as a result of how the ACPI table reports that device, it is detected by the existing kernel module and we have no way of differentiating until after the chip ID probe.
> >
> > The module loaded is bmc150-accel-i2c.c which currently doesn't support the bmi323 and the loading of the module just fails at chip check.  
> 
> bmc150 driver supports multiple accelerometer sensors such as
> bma222, bma280, bmi055 and all of them are having similar
> register map, but the bmi323 register map is completely different
> from bmc150.

Horrible bios hacks that depend on a particular driver stack
are always a pain.

Hmm. Andy (handy ACPI expert), any suggestion?

We could maybe do a wrapper driver that does appropriate checks and wraps
the probe + remove from the two drivers?  Whilst we can obviously have a
single driver that deals with radically different devices I'm not
particularly keen on that as it tends to make things less maintainable.

Jonathan

> 
> 
> >
> > I have solved the problem by expanding the current bmc150-accel-i2c.c and bmc150-accel-core.c files to handle that IC in almost every part: gyroscope, accelerometer and temperature sensor.
> >
> > What is the best way of organizing code to have this module mainlined? Is it correct leaving files called bmc150-accel-* even if it is managing another IC and and not just the accelerometer part anymore?
> >
> > TIA for your time.
> >
> > Best regards,
> > Denis Benato  
> 
> Regards
> 
> Jagath

Re: [RFC 2/2] iio: imu: Add driver for BMI323 IMU

[Denis Benato]

Hello Jagath,

On 9/29/23 09:59, Jagath Jog J wrote:
> Hi Denis,
> 
> On Thu, Sep 28, 2023 at 2:55 AM Denis Benato <benato.denis96@gmail.com> wrote:
>>
>> Hello,
>>
>> Some devices (as my asus rog ally) have an ACPI node describing a BOSC0200 sensor. The IC being used in those devices is a  bmi323 but as a result of how the ACPI table reports that device, it is detected by the existing kernel module and we have no way of differentiating until after the chip ID probe.
>>
>> The module loaded is bmc150-accel-i2c.c which currently doesn't support the bmi323 and the loading of the module just fails at chip check.
> 
> bmc150 driver supports multiple accelerometer sensors such as
> bma222, bma280, bmi055 and all of them are having similar
> register map, but the bmi323 register map is completely different
> from bmc150.

I apologize for the confusion.

What I was trying to say is that to have the bmi323 working in those aforementioned devices bmc150 will need to be modified: that is the probe function that ends up being executed, fact that cannot be changed because it depends on the ACPI implementation shipped on those devices. 

Therefore I was asking about the best way of handing control to the new driver and how that should be organized: in my implementation the new bmi323 code was written inside the bmc150-accel-core.c and only shares sleep/suspend, probe and removal functions in addition to checking for the new chip presence before checking for any bmc150 chip as that issues an i2c write, while the check for the bmi323 only requires an i2c read.

We also have done duplicate work as I have written a driver for that chip myself, but it's not as good as yours because my hardware didn't come with the IRQ pin connected and so I couldn't develop triggers and I only got the i2c interface working.

> 
> 
>>
>> I have solved the problem by expanding the current bmc150-accel-i2c.c and bmc150-accel-core.c files to handle that IC in almost every part: gyroscope, accelerometer and temperature sensor.
>>
>> What is the best way of organizing code to have this module mainlined? Is it correct leaving files called bmc150-accel-* even if it is managing another IC and and not just the accelerometer part anymore?
>>
>> TIA for your time.
>>
>> Best regards,
>> Denis Benato
> 
> Regards
> 
> Jagath

TIA for your time.

Best regards,
Denis Benato

Re: [RFC 2/2] iio: imu: Add driver for BMI323 IMU

[Jagath Jog J]

Hi Denis, Jonathan

On Sun, Oct 1, 2023 at 7:23 PM Denis Benato <benato.denis96@gmail.com> wrote:
>
> Hello Jagath,
>
> On 9/29/23 09:59, Jagath Jog J wrote:
> > Hi Denis,
> >
> > On Thu, Sep 28, 2023 at 2:55 AM Denis Benato <benato.denis96@gmail.com> wrote:
> >>
> >> Hello,
> >>
> >> Some devices (as my asus rog ally) have an ACPI node describing a BOSC0200 sensor. The IC being used in those devices is a  bmi323 but as a result of how the ACPI table reports that device, it is detected by the existing kernel module and we have no way of differentiating until after the chip ID probe.
> >>
> >> The module loaded is bmc150-accel-i2c.c which currently doesn't support the bmi323 and the loading of the module just fails at chip check.
> >
> > bmc150 driver supports multiple accelerometer sensors such as
> > bma222, bma280, bmi055 and all of them are having similar
> > register map, but the bmi323 register map is completely different
> > from bmc150.
>
> I apologize for the confusion.
>
> What I was trying to say is that to have the bmi323 working in those aforementioned devices bmc150 will need to be modified: that is the probe function that ends up being executed, fact that cannot be changed because it depends on the ACPI implementation shipped on those devices.
>
> Therefore I was asking about the best way of handing control to the new driver and how that should be organized: in my implementation the new bmi323 code was written inside the bmc150-accel-core.c and only shares sleep/suspend, probe and removal functions in addition to checking for the new chip presence before checking for any bmc150 chip as that issues an i2c write, while the check for the bmi323 only requires an i2c read.

Means you want to handle control to the standalone driver from bmc150.
Sorry, I didn't find any examples.

Important thing to handle is the bmi323 private structure and call required
exported functions from another driver.

Jonathan: Can you suggest any example wrapper drivers which handle that way?

>
> We also have done duplicate work as I have written a driver for that chip myself, but it's not as good as yours because my hardware didn't come with the IRQ pin connected and so I couldn't develop triggers and I only got the i2c interface working.
>
> >
> >
> >>
> >> I have solved the problem by expanding the current bmc150-accel-i2c.c and bmc150-accel-core.c files to handle that IC in almost every part: gyroscope, accelerometer and temperature sensor.
> >>
> >> What is the best way of organizing code to have this module mainlined? Is it correct leaving files called bmc150-accel-* even if it is managing another IC and and not just the accelerometer part anymore?
> >>
> >> TIA for your time.
> >>
> >> Best regards,
> >> Denis Benato
> >
> > Regards
> >
> > Jagath
>
> TIA for your time.
>
> Best regards,
> Denis Benato

Regards
Jagath

Re: [RFC 2/2] iio: imu: Add driver for BMI323 IMU

[Jonathan Cameron]

> 
> > > +struct bmi323_data {
> > > +     struct device *dev;
> > > +     struct regmap *regmap;
> > > +     struct iio_mount_matrix orientation;
> > > +     enum bmi323_irq_pin irq_pin;
> > > +     struct iio_trigger *trig;
> > > +     bool drdy_trigger_enabled;
> > > +     enum bmi323_state state;
> > > +     s64 fifo_tstamp, old_fifo_tstamp;
> > > +     u32 odrns[2];
> > > +     u32 odrhz[2];
> > > +     unsigned int feature_events;
> > > +
> > > +     /*
> > > +      * Lock to protect the members of device's private data from concurrent
> > > +      * access and also to serialize the access of extended registers.
> > > +      * See bmi323_write_ext_reg(..) for more info.
> > > +      */
> > > +     struct mutex mutex;
> > > +     int watermark;
> > > +     __le16 fifo_buff[BMI323_FIFO_FULL_IN_WORDS] __aligned(IIO_DMA_MINALIGN);
> > > +     struct {
> > > +             __le16 channels[6];
> > > +             s64 ts __aligned(8);  
> >
> > Hopefully Andy's aligned_s64 set will land soon and we can tidy this up.
> > I'm a bit unsure of this, but can you overlap some of these buffers or are
> > they used concurrently? (if they are then we have problems with DMA safety.)
> >
> > Perhaps an anonymous union is appropriate?  
> 
> Yes both buffers are used at the same time. In fifo_flush
> fifo_buff is used to store all fifo data, and buffer is
> used to push a single data frame to iio buffers, overlapping
> will corrupt the data, so I used separate buffers for both.

Ah. So the structure is used in 2 ways.

1. As a target for DMA, which means it should live in the cacheline we
are saving for that purpsoe.
2. As a place to build up data.  

In general we should be careful with doing 2 as that could race with
DMA and end up with data corruption, however you only use it that
way in flush_fifo where both the DMA and this usage under under 
the mutex.  Hence I think you are fine.


> 
> > > +static IIO_DEVICE_ATTR_RW(in_accel_gyro_averaging, 0);
> > > +static IIO_CONST_ATTR(in_accel_gyro_averaging_available, "2 4 8 16 32 64");
> > > +
> > > +static struct attribute *bmi323_attributes[] = {
> > > +     &iio_dev_attr_in_accel_gyro_averaging.dev_attr.attr,
> > > +     &iio_const_attr_in_accel_gyro_averaging_available.dev_attr.attr,  
> >
> > So averaging often maps directly to oversampling.  Kind of different names
> > for the same thing.  Perhaps that standard ABI can be used?
> > It tends to make sampling frequency reporting need to take it into account
> > though as that drops as divided by oversampling ratio.  
> 
> Yes, oversampling can be used, but changing the average
> value doesn't alter the sampling frequency. The sampling
> frequency is same even with the increase in averaging value.

Ok.  That's unusual so good to know.
> > > +static int bmi323_feature_engine_enable(struct bmi323_data *data, bool en)
> > > +{
> > > +     unsigned int feature_status;
> > > +     int ret, i;
> > > +
> > > +     if (en) {
> > > +             ret = regmap_write(data->regmap, BMI323_FEAT_IO2_REG,
> > > +                                0x012c);
> > > +             if (ret)
> > > +                     return ret;
> > > +
> > > +             ret = regmap_write(data->regmap, BMI323_FEAT_IO_STATUS_REG,
> > > +                                BMI323_FEAT_IO_STATUS_MSK);
> > > +             if (ret)
> > > +                     return ret;
> > > +
> > > +             ret = regmap_write(data->regmap, BMI323_FEAT_CTRL_REG,
> > > +                                BMI323_FEAT_ENG_EN_MSK);
> > > +             if (ret)
> > > +                     return ret;
> > > +
> > > +             i = 5;  
> >
> > Why 5?  
> 
> No specific reason, during testing the feature engine was
> taking around 4 milliseconds, so I thought of checking
> every 2 milliseconds and max of 5 trials.

That's a good reason. Just add a comment to that say that.



> 
> > > + * From BMI323 datasheet section 4: Notes on the Serial Interface Support.
> > > + * Each SPI register read operation requires to read one dummy byte before
> > > + * the actual payload.
> > > + */
> > > +static int bmi323_regmap_spi_read(void *context, const void *reg_buf,
> > > +                               size_t reg_size, void *val_buf,
> > > +                               size_t val_size)
> > > +{
> > > +     struct spi_device *spi = context;
> > > +     u8 reg, *buff = NULL;
> > > +     int ret;
> > > +
> > > +     buff = kmalloc(val_size + BMI323_SPI_DUMMY, GFP_KERNEL);  
> >
> > Hmm.  Regmap has pad_bits (which can be multiple bytes) but this case
> > is unusual in that they only apply to reads.
> >
> > I wonder if we can make this cheaper though rather than having
> > to handle either some context or having dynamic allocations in here.
> >
> > How about making the write bigger?  Does that have any effect?
> > Looks like don't care state in Figure 31.  If that's the case,
> > send some zeros on that as it's known fixed size (2 bytes including
> > the padding) and then you can directly use the read buffer without
> > yet another memcpy.  
> 
> For spi with pad_bits=8 and without any custom read and
> write functions, regmap_read() works but regmap_write()
> does not. Write is also adding 8 bits of padding and
> the device is treating it as data.
> (7.2.3 SPI Protocol Figure 30)

Understood. I looked it up too before suggesting this local hack.
You'll still need a custom regmap, but at least this trick would
allow you to avoid allocating a buffer in the read function.

Jonathan

Re: [RFC 2/2] iio: imu: Add driver for BMI323 IMU

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1308 bytes --]

Hello,

On Mon, Sep 18, 2023 at 01:33:14PM +0530, Jagath Jog J wrote:
> The Bosch BMI323 is a 6-axis low-power IMU that provide measurements for
> acceleration, angular rate, and temperature. This sensor includes
> motion-triggered interrupt features, such as a step counter, tap detection,
> and activity/inactivity interrupt capabilities.
> 
> The driver supports various functionalities, including data ready, FIFO
> data handling, and events such as tap detection, step counting, and
> activity interrupts
> 
> Datasheet: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi323-ds000.pdf

Maybe put this link better in the driver.

> +static struct i2c_driver bmi323_i2c_driver = {
> +	.driver = {
> +		.name = "bmi323",
> +		.of_match_table = bmi323_of_i2c_match,
> +	},
> +	.probe_new = bmi323_i2c_probe,
> +	.id_table = bmi323_i2c_ids,
> +};
> +module_i2c_driver(bmi323_i2c_driver);

If you want to compile this driver after v6.6-rc2 (which includes 
commit 5eb1e6e459cf ("i2c: Drop legacy callback .probe_new()")) better
use .probe here instead of .probe_new().

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 2/2] iio: imu: Add driver for BMI323 IMU

[Andy Shevchenko]

On Wed, Sep 27, 2023 at 11:57:08AM +0200, Uwe Kleine-König wrote:
> On Mon, Sep 18, 2023 at 01:33:14PM +0530, Jagath Jog J wrote:

...

> > Datasheet: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi323-ds000.pdf
> 
> Maybe put this link better in the driver.

Why? We have a handful commits with this and it's better to see the link
to the datasheet without browsing the source code.

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 2/2] iio: imu: Add driver for BMI323 IMU

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 813 bytes --]

On Wed, Sep 27, 2023 at 03:35:02PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 27, 2023 at 11:57:08AM +0200, Uwe Kleine-König wrote:
> > On Mon, Sep 18, 2023 at 01:33:14PM +0530, Jagath Jog J wrote:
> 
> ...
> 
> > > Datasheet: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi323-ds000.pdf
> > 
> > Maybe put this link better in the driver.
> 
> Why? We have a handful commits with this and it's better to see the link
> to the datasheet without browsing the source code.

But if you later work on a problem in the driver, it's better to see the
link without browsing git history. :-)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC 2/2] iio: imu: Add driver for BMI323 IMU

[Andy Shevchenko]

On Wed, Sep 27, 2023 at 04:34:43PM +0200, Uwe Kleine-König wrote:
> On Wed, Sep 27, 2023 at 03:35:02PM +0300, Andy Shevchenko wrote:
> > On Wed, Sep 27, 2023 at 11:57:08AM +0200, Uwe Kleine-König wrote:
> > > On Mon, Sep 18, 2023 at 01:33:14PM +0530, Jagath Jog J wrote:

...

> > > > Datasheet: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi323-ds000.pdf
> > > 
> > > Maybe put this link better in the driver.
> > 
> > Why? We have a handful commits with this and it's better to see the link
> > to the datasheet without browsing the source code.
> 
> But if you later work on a problem in the driver, it's better to see the
> link without browsing git history. :-)

Both make sense.


-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 2/2] iio: imu: Add driver for BMI323 IMU

[Jagath Jog J]

Hi Uwe Kleine-König,

On Wed, Sep 27, 2023 at 3:27 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Mon, Sep 18, 2023 at 01:33:14PM +0530, Jagath Jog J wrote:
> > The Bosch BMI323 is a 6-axis low-power IMU that provide measurements for
> > acceleration, angular rate, and temperature. This sensor includes
> > motion-triggered interrupt features, such as a step counter, tap detection,
> > and activity/inactivity interrupt capabilities.
> >
> > The driver supports various functionalities, including data ready, FIFO
> > data handling, and events such as tap detection, step counting, and
> > activity interrupts
> >
> > Datasheet: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi323-ds000.pdf
>
> Maybe put this link better in the driver.

Yes, if there are multiple commits on the driver, the datasheet
link will move further down with the initial commit. I will add
datasheet link in the driver.

>
> > +static struct i2c_driver bmi323_i2c_driver = {
> > +     .driver = {
> > +             .name = "bmi323",
> > +             .of_match_table = bmi323_of_i2c_match,
> > +     },
> > +     .probe_new = bmi323_i2c_probe,
> > +     .id_table = bmi323_i2c_ids,
> > +};
> > +module_i2c_driver(bmi323_i2c_driver);
>
> If you want to compile this driver after v6.6-rc2 (which includes
> commit 5eb1e6e459cf ("i2c: Drop legacy callback .probe_new()")) better
> use .probe here instead of .probe_new().

Thanks for pointing it out.
I switched to v6.6-rc3 and I will change to .probe.

Regards
Jagath

Jagath

>
> Best regards
> Uwe
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [RFC 2/2] iio: imu: Add driver for BMI323 IMU

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2046 bytes --]

On Thu, Sep 28, 2023 at 11:49:01PM +0530, Jagath Jog J wrote:
> Hi Uwe Kleine-König,
> 
> On Wed, Sep 27, 2023 at 3:27 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Hello,
> >
> > On Mon, Sep 18, 2023 at 01:33:14PM +0530, Jagath Jog J wrote:
> > > The Bosch BMI323 is a 6-axis low-power IMU that provide measurements for
> > > acceleration, angular rate, and temperature. This sensor includes
> > > motion-triggered interrupt features, such as a step counter, tap detection,
> > > and activity/inactivity interrupt capabilities.
> > >
> > > The driver supports various functionalities, including data ready, FIFO
> > > data handling, and events such as tap detection, step counting, and
> > > activity interrupts
> > >
> > > Datasheet: https://www.bosch-sensortec.com/media/boschsensortec/downloads/datasheets/bst-bmi323-ds000.pdf
> >
> > Maybe put this link better in the driver.
> 
> Yes, if there are multiple commits on the driver, the datasheet
> link will move further down with the initial commit. I will add
> datasheet link in the driver.
> 
> >
> > > +static struct i2c_driver bmi323_i2c_driver = {
> > > +     .driver = {
> > > +             .name = "bmi323",
> > > +             .of_match_table = bmi323_of_i2c_match,
> > > +     },
> > > +     .probe_new = bmi323_i2c_probe,
> > > +     .id_table = bmi323_i2c_ids,
> > > +};
> > > +module_i2c_driver(bmi323_i2c_driver);
> >
> > If you want to compile this driver after v6.6-rc2 (which includes
> > commit 5eb1e6e459cf ("i2c: Drop legacy callback .probe_new()")) better
> > use .probe here instead of .probe_new().
> 
> Thanks for pointing it out.
> I switched to v6.6-rc3 and I will change to .probe.

Note that you can use .probe (with that prototype) already since
v6.3-rc2. So there is no hard requirement to go to v6.6-rc3.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]