diff mbox series

[v7,3/7] dt-bindings: iio: accel: adxl345: add interrupt-names

Message ID 20241213211909.40896-4-l.rubusch@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series iio: accel: adxl345: add FIFO operating with IRQ triggered watermark events | expand

Commit Message

Lothar Rubusch Dec. 13, 2024, 9:19 p.m. UTC
Add interrupt-names INT1 and INT2 for the two interrupt lines of the
sensor.

When one of the two interrupt lines is connected, the interrupt as its
interrupt-name, need to be declared in the devicetree. The driver then
configures the sensor to indicate its events on either INT1 or INT2.

If no interrupt is configured, then no interrupt-name should be
configured, and vice versa. In this case the sensor runs in FIFO BYPASS
mode. This allows sensor measurements, but none of the sensor events.

Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
---
 .../devicetree/bindings/iio/accel/adi,adxl345.yaml     | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Rob Herring (Arm) Dec. 13, 2024, 10:42 p.m. UTC | #1
On Fri, 13 Dec 2024 21:19:05 +0000, Lothar Rubusch wrote:
> Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> sensor.
> 
> When one of the two interrupt lines is connected, the interrupt as its
> interrupt-name, need to be declared in the devicetree. The driver then
> configures the sensor to indicate its events on either INT1 or INT2.
> 
> If no interrupt is configured, then no interrupt-name should be
> configured, and vice versa. In this case the sensor runs in FIFO BYPASS
> mode. This allows sensor measurements, but none of the sensor events.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> ---
>  .../devicetree/bindings/iio/accel/adi,adxl345.yaml     | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:
./Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml:45:17: [error] string value is redundantly quoted with any quotes (quoted-strings)
./Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml:46:22: [error] string value is redundantly quoted with any quotes (quoted-strings)

dtschema/dtc warnings/errors:

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20241213211909.40896-4-l.rubusch@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Jonathan Cameron Dec. 14, 2024, 12:10 p.m. UTC | #2
On Fri, 13 Dec 2024 21:19:05 +0000
Lothar Rubusch <l.rubusch@gmail.com> wrote:

> Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> sensor.
> 
> When one of the two interrupt lines is connected, the interrupt as its
> interrupt-name, need to be declared in the devicetree. The driver then
> configures the sensor to indicate its events on either INT1 or INT2.
> 
> If no interrupt is configured, then no interrupt-name should be
> configured, and vice versa. In this case the sensor runs in FIFO BYPASS
> mode. This allows sensor measurements, but none of the sensor events.
> 
> Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>

Just to repeat what I sent in reply to v6 (well after you'd posted this).
Maybe we can maintain compatibility with the binding before this by adding
a default of INT1.

Then you'd need to drop the dependency on interrupt-names.

I'm not sure though if the checking of number of entries will work against
a default. Give it a go and see what happens :)

We are lucky that we can't have bindings in the wild assuming ordering
of the two interrupts due to the maxItems being set for interrupts.

It's a messy corner, perhaps we should just not bother in the binding,
but keep that default handling in the driver?

DT binding folk, what do you think the best way of handling this is?

Jonathan

> ---
>  .../devicetree/bindings/iio/accel/adi,adxl345.yaml     | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> index bc46ed00f..4f971035b 100644
> --- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> +++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
> @@ -37,6 +37,14 @@ properties:
>    interrupts:
>      maxItems: 1

That is not going to work if we have two interrupts specified.
It does at least mean we only need 

>  
> +  interrupt-names:
> +    items:
> +      - enum: [INT1, INT2]
> +
> +dependencies:
> +  interrupts: [ 'interrupt-names' ]
> +  interrupt-names: [ 'interrupts' ]
> +
>  required:
>    - compatible
>    - reg
> @@ -60,6 +68,7 @@ examples:
>              reg = <0x2a>;
>              interrupt-parent = <&gpio0>;
>              interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "INT1";
>          };
>      };
>    - |
> @@ -78,5 +87,6 @@ examples:
>              spi-cpha;
>              interrupt-parent = <&gpio0>;
>              interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> +            interrupt-names = "INT2";
>          };
>      };
Conor Dooley Dec. 15, 2024, 2:56 p.m. UTC | #3
On Sat, Dec 14, 2024 at 12:10:57PM +0000, Jonathan Cameron wrote:
> On Fri, 13 Dec 2024 21:19:05 +0000
> Lothar Rubusch <l.rubusch@gmail.com> wrote:
> 
> > Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> > sensor.
> > 
> > When one of the two interrupt lines is connected, the interrupt as its
> > interrupt-name, need to be declared in the devicetree. The driver then
> > configures the sensor to indicate its events on either INT1 or INT2.
> > 
> > If no interrupt is configured, then no interrupt-name should be
> > configured, and vice versa. In this case the sensor runs in FIFO BYPASS
> > mode. This allows sensor measurements, but none of the sensor events.
> > 
> > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> 
> Just to repeat what I sent in reply to v6 (well after you'd posted this).
> Maybe we can maintain compatibility with the binding before this by adding
> a default of INT1.

But can you make that assumption? If we did, and it's not universally
true, we break systems that had INT2 connected that previously worked.

> Then you'd need to drop the dependency on interrupt-names.
> 
> I'm not sure though if the checking of number of entries will work against
> a default. Give it a go and see what happens :)
> 
> We are lucky that we can't have bindings in the wild assuming ordering
> of the two interrupts due to the maxItems being set for interrupts.
> 
> It's a messy corner, perhaps we should just not bother in the binding,
> but keep that default handling in the driver?
> 
> DT binding folk, what do you think the best way of handling this is?
Jonathan Cameron Dec. 19, 2024, 5:58 p.m. UTC | #4
On Sun, 15 Dec 2024 14:56:58 +0000
Conor Dooley <conor@kernel.org> wrote:

> On Sat, Dec 14, 2024 at 12:10:57PM +0000, Jonathan Cameron wrote:
> > On Fri, 13 Dec 2024 21:19:05 +0000
> > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> >   
> > > Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> > > sensor.
> > > 
> > > When one of the two interrupt lines is connected, the interrupt as its
> > > interrupt-name, need to be declared in the devicetree. The driver then
> > > configures the sensor to indicate its events on either INT1 or INT2.
> > > 
> > > If no interrupt is configured, then no interrupt-name should be
> > > configured, and vice versa. In this case the sensor runs in FIFO BYPASS
> > > mode. This allows sensor measurements, but none of the sensor events.
> > > 
> > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>  
> > 
> > Just to repeat what I sent in reply to v6 (well after you'd posted this).
> > Maybe we can maintain compatibility with the binding before this by adding
> > a default of INT1.  
> 
> But can you make that assumption? If we did, and it's not universally
> true, we break systems that had INT2 connected that previously worked.

I guess there is a possibility of a driver in some other OS assuming INT2, but
seems an odd 'default' choice.  Also odd for a writer of DT for a platform
to assume it.

There is a thing that comes up in spec orgs when discussing whether to
rush out an errata.  "Is this bug something people would get wrong
thinking the answer was clear, or something where the would ask a question?"
Anyone who thinks INT2 is the obvious choice for me falls into the would
ask category.

However, in the linux driver we would would go from assuming no interrupts
to assuming the wrong one.  That's indeed bad.  So I guess this doesn't work.
Oh well no default it is.

Jonathan

> 
> > Then you'd need to drop the dependency on interrupt-names.
> > 
> > I'm not sure though if the checking of number of entries will work against
> > a default. Give it a go and see what happens :)
> > 
> > We are lucky that we can't have bindings in the wild assuming ordering
> > of the two interrupts due to the maxItems being set for interrupts.
> > 
> > It's a messy corner, perhaps we should just not bother in the binding,
> > but keep that default handling in the driver?
> > 
> > DT binding folk, what do you think the best way of handling this is?
Conor Dooley Dec. 19, 2024, 6:21 p.m. UTC | #5
On Thu, Dec 19, 2024 at 05:58:15PM +0000, Jonathan Cameron wrote:
> On Sun, 15 Dec 2024 14:56:58 +0000
> Conor Dooley <conor@kernel.org> wrote:
> 
> > On Sat, Dec 14, 2024 at 12:10:57PM +0000, Jonathan Cameron wrote:
> > > On Fri, 13 Dec 2024 21:19:05 +0000
> > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > >   
> > > > Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> > > > sensor.
> > > > 
> > > > When one of the two interrupt lines is connected, the interrupt as its
> > > > interrupt-name, need to be declared in the devicetree. The driver then
> > > > configures the sensor to indicate its events on either INT1 or INT2.
> > > > 
> > > > If no interrupt is configured, then no interrupt-name should be
> > > > configured, and vice versa. In this case the sensor runs in FIFO BYPASS
> > > > mode. This allows sensor measurements, but none of the sensor events.
> > > > 
> > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>  
> > > 
> > > Just to repeat what I sent in reply to v6 (well after you'd posted this).
> > > Maybe we can maintain compatibility with the binding before this by adding
> > > a default of INT1.  
> > 
> > But can you make that assumption? If we did, and it's not universally
> > true, we break systems that had INT2 connected that previously worked.
> 
> I guess there is a possibility of a driver in some other OS assuming INT2, but
> seems an odd 'default' choice.

Ye, I think that it is unlikely a driver author would think that way.

> Also odd for a writer of DT for a platform
> to assume it.

I agree, I think it is unlikely that someone would assume it'd work like
this. I think a lack of attention paid to the schematic of the board is
a more likely culprit.

> There is a thing that comes up in spec orgs when discussing whether to
> rush out an errata.  "Is this bug something people would get wrong
> thinking the answer was clear, or something where the would ask a question?"
> Anyone who thinks INT2 is the obvious choice for me falls into the would
> ask category.
> 
> However, in the linux driver we would would go from assuming no interrupts
> to assuming the wrong one.  That's indeed bad.  So I guess this doesn't work.
> Oh well no default it is.

I don't think you really lose anything from having no default. The
driver works just fine without the interrupt, so the albeit small risk
just doesn't seem worth it.
Lothar Rubusch Dec. 25, 2024, 1:01 p.m. UTC | #6
On Thu, Dec 19, 2024 at 7:21 PM Conor Dooley <conor@kernel.org> wrote:
>
> On Thu, Dec 19, 2024 at 05:58:15PM +0000, Jonathan Cameron wrote:
> > On Sun, 15 Dec 2024 14:56:58 +0000
> > Conor Dooley <conor@kernel.org> wrote:
> >
> > > On Sat, Dec 14, 2024 at 12:10:57PM +0000, Jonathan Cameron wrote:
> > > > On Fri, 13 Dec 2024 21:19:05 +0000
> > > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > > >
> > > > > Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> > > > > sensor.
> > > > >
> > > > > When one of the two interrupt lines is connected, the interrupt as its
> > > > > interrupt-name, need to be declared in the devicetree. The driver then
> > > > > configures the sensor to indicate its events on either INT1 or INT2.
> > > > >
> > > > > If no interrupt is configured, then no interrupt-name should be
> > > > > configured, and vice versa. In this case the sensor runs in FIFO BYPASS
> > > > > mode. This allows sensor measurements, but none of the sensor events.
> > > > >
> > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > >
> > > > Just to repeat what I sent in reply to v6 (well after you'd posted this).
> > > > Maybe we can maintain compatibility with the binding before this by adding
> > > > a default of INT1.
> > >
> > > But can you make that assumption? If we did, and it's not universally
> > > true, we break systems that had INT2 connected that previously worked.
> >
> > I guess there is a possibility of a driver in some other OS assuming INT2, but
> > seems an odd 'default' choice.
>
> Ye, I think that it is unlikely a driver author would think that way.
>
> > Also odd for a writer of DT for a platform
> > to assume it.
>
> I agree, I think it is unlikely that someone would assume it'd work like
> this. I think a lack of attention paid to the schematic of the board is
> a more likely culprit.
>
> > There is a thing that comes up in spec orgs when discussing whether to
> > rush out an errata.  "Is this bug something people would get wrong
> > thinking the answer was clear, or something where the would ask a question?"
> > Anyone who thinks INT2 is the obvious choice for me falls into the would
> > ask category.
> >
> > However, in the linux driver we would would go from assuming no interrupts
> > to assuming the wrong one.  That's indeed bad.  So I guess this doesn't work.
> > Oh well no default it is.
>
> I don't think you really lose anything from having no default. The
> driver works just fine without the interrupt, so the albeit small risk
> just doesn't seem worth it.

Agree. To be honest, I'm not sure if I catch the point here. IMHO,
falling back to FIFO bypass should match with backward compatibility.
Please let me know what I'm missing here.

I would prefer just to check for a specified INT name. Then configure
the specified interrupt line in the probe. In this sense, the
interrupt line is only useful also if the INT name is defined in the
DT. If no INT name is specified, the iio driver will setup FIFO_BYPASS
which is the legacy behavior (according to the datasheet: if none of
the FIFO mode bits are set, defaults to bypass mode). This is the new
behavior.

The old iio driver did not use interrupts at all. It stayed in
FIFO_BYPASS mode (or did not change it, after power on, it assumes
FIFO_BYPASS to my interpretation). Thus declaring the IRQ line yes or
no, with or without INT names - for the iio driver implementation
before this patch series, should not make any difference. It uses
FIFO_BYPASS in all cases.

The input driver (AFAIR we already agreed on ignoring this driver)
needed interrupts. Defining INT names here does not change anything,
either. The input driver configures by default INT1 and simply ignores
what was specified as INT names in the DT.

I cannot really think of any third case here. Please, let me know if
I'm wrong. If not I will keep the above explained behavior, since to
my understanding it should match the desired compatibility
requirements. Am I wrong here?

Sorry for the late answer. Best,
L
Conor Dooley Dec. 27, 2024, 5:55 p.m. UTC | #7
On Wed, Dec 25, 2024 at 02:01:50PM +0100, Lothar Rubusch wrote:
> On Thu, Dec 19, 2024 at 7:21 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > On Thu, Dec 19, 2024 at 05:58:15PM +0000, Jonathan Cameron wrote:
> > > On Sun, 15 Dec 2024 14:56:58 +0000
> > > Conor Dooley <conor@kernel.org> wrote:
> > >
> > > > On Sat, Dec 14, 2024 at 12:10:57PM +0000, Jonathan Cameron wrote:
> > > > > On Fri, 13 Dec 2024 21:19:05 +0000
> > > > > Lothar Rubusch <l.rubusch@gmail.com> wrote:
> > > > >
> > > > > > Add interrupt-names INT1 and INT2 for the two interrupt lines of the
> > > > > > sensor.
> > > > > >
> > > > > > When one of the two interrupt lines is connected, the interrupt as its
> > > > > > interrupt-name, need to be declared in the devicetree. The driver then
> > > > > > configures the sensor to indicate its events on either INT1 or INT2.
> > > > > >
> > > > > > If no interrupt is configured, then no interrupt-name should be
> > > > > > configured, and vice versa. In this case the sensor runs in FIFO BYPASS
> > > > > > mode. This allows sensor measurements, but none of the sensor events.
> > > > > >
> > > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com>
> > > > >
> > > > > Just to repeat what I sent in reply to v6 (well after you'd posted this).
> > > > > Maybe we can maintain compatibility with the binding before this by adding
> > > > > a default of INT1.
> > > >
> > > > But can you make that assumption? If we did, and it's not universally
> > > > true, we break systems that had INT2 connected that previously worked.
> > >
> > > I guess there is a possibility of a driver in some other OS assuming INT2, but
> > > seems an odd 'default' choice.
> >
> > Ye, I think that it is unlikely a driver author would think that way.
> >
> > > Also odd for a writer of DT for a platform
> > > to assume it.
> >
> > I agree, I think it is unlikely that someone would assume it'd work like
> > this. I think a lack of attention paid to the schematic of the board is
> > a more likely culprit.
> >
> > > There is a thing that comes up in spec orgs when discussing whether to
> > > rush out an errata.  "Is this bug something people would get wrong
> > > thinking the answer was clear, or something where the would ask a question?"
> > > Anyone who thinks INT2 is the obvious choice for me falls into the would
> > > ask category.
> > >
> > > However, in the linux driver we would would go from assuming no interrupts
> > > to assuming the wrong one.  That's indeed bad.  So I guess this doesn't work.
> > > Oh well no default it is.
> >
> > I don't think you really lose anything from having no default. The
> > driver works just fine without the interrupt, so the albeit small risk
> > just doesn't seem worth it.
> 
> Agree. To be honest, I'm not sure if I catch the point here. IMHO,
> falling back to FIFO bypass should match with backward compatibility.
> Please let me know what I'm missing here.

Ye, falling back to the current behaviour is fine. The only problematic
thing is not checking "for a specified INT name" but assuming the
provided interrupt is either INT1 or INT2 when interrupt-names is not
provided.

> 
> I would prefer just to check for a specified INT name. Then configure
> the specified interrupt line in the probe. In this sense, the
> interrupt line is only useful also if the INT name is defined in the
> DT. If no INT name is specified, the iio driver will setup FIFO_BYPASS
> which is the legacy behavior (according to the datasheet: if none of
> the FIFO mode bits are set, defaults to bypass mode). This is the new
> behavior.
> 
> The old iio driver did not use interrupts at all. It stayed in
> FIFO_BYPASS mode (or did not change it, after power on, it assumes
> FIFO_BYPASS to my interpretation). Thus declaring the IRQ line yes or
> no, with or without INT names - for the iio driver implementation
> before this patch series, should not make any difference. It uses
> FIFO_BYPASS in all cases.
> 
> The input driver (AFAIR we already agreed on ignoring this driver)
> needed interrupts. Defining INT names here does not change anything,
> either. The input driver configures by default INT1 and simply ignores
> what was specified as INT names in the DT.
> 
> I cannot really think of any third case here. Please, let me know if
> I'm wrong. If not I will keep the above explained behavior, since to
> my understanding it should match the desired compatibility
> requirements. Am I wrong here?
> 
> Sorry for the late answer. Best,
> L
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
index bc46ed00f..4f971035b 100644
--- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
+++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml
@@ -37,6 +37,14 @@  properties:
   interrupts:
     maxItems: 1
 
+  interrupt-names:
+    items:
+      - enum: [INT1, INT2]
+
+dependencies:
+  interrupts: [ 'interrupt-names' ]
+  interrupt-names: [ 'interrupts' ]
+
 required:
   - compatible
   - reg
@@ -60,6 +68,7 @@  examples:
             reg = <0x2a>;
             interrupt-parent = <&gpio0>;
             interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "INT1";
         };
     };
   - |
@@ -78,5 +87,6 @@  examples:
             spi-cpha;
             interrupt-parent = <&gpio0>;
             interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+            interrupt-names = "INT2";
         };
     };