| Message ID | 20230918080314.11959-2-jagathjog1996@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: imu: Add driver and dt-bindings for BMI323 | expand |
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
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
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
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"; > + }; > + };
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
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
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
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 > >
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 > > > >
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
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
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
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 >
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 > > >
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"; + }; + };
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