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 <[email protected]>
> ---
> .../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 <[email protected]>
> +
> +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
<[email protected]> 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 <[email protected]>
> > ---
> > .../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 <[email protected]>
> > +
> > +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 <[email protected]> wrote:
> Hi Krzysztof,
>
> On Mon, Sep 18, 2023 at 5:55 PM Krzysztof Kozlowski
> <[email protected]> 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
Hi Jonathan
On Sun, Sep 24, 2023 at 7:01 PM Jonathan Cameron <[email protected]> wrote:
>
> On Tue, 19 Sep 2023 22:14:40 +0530
> Jagath Jog J <[email protected]> wrote:
>
> > Hi Krzysztof,
> >
> > On Mon, Sep 18, 2023 at 5:55 PM Krzysztof Kozlowski
> > <[email protected]> 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