| Message ID | 20241211230648.205806-5-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 |
On Wed, Dec 11, 2024 at 11:06:45PM +0000, Lothar Rubusch wrote: > Remove interrupts from the list of required properties. The ADXL345 > provides two interrupt lines. Anyway, the interrupts are an option, to > be used for additional event features. The driver can measure without > interrupts. Hence, interrupts should never have been required for the > ADXL345. Thus having interrupts required can be considered to be a > mistake. Partially this explains my question on previous patch, so consider reordering them. And with combined knowledge, your driver now depends on interrupt names to setup interrupts. "interrupts" property alone is not sufficient, so you should encode it in the binding and explain in rationale why this is required (it is a change in ABI). https://elixir.bootlin.com/linux/v6.8-rc3/source/Documentation/devicetree/bindings/example-schema.yaml#L193 Best regards, Krzysztof
On Thu, Dec 12, 2024 at 9:11 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Wed, Dec 11, 2024 at 11:06:45PM +0000, Lothar Rubusch wrote: > > Remove interrupts from the list of required properties. The ADXL345 > > provides two interrupt lines. Anyway, the interrupts are an option, to > > be used for additional event features. The driver can measure without > > interrupts. Hence, interrupts should never have been required for the > > ADXL345. Thus having interrupts required can be considered to be a > > mistake. > > Partially this explains my question on previous patch, so consider > reordering them. > I understand. > And with combined knowledge, your driver now depends on interrupt names > to setup interrupts. "interrupts" property alone is not sufficient, so > you should encode it in the binding and explain in rationale why this is > required (it is a change in ABI). > > https://elixir.bootlin.com/linux/v6.8-rc3/source/Documentation/devicetree/bindings/example-schema.yaml#L193 > The accelerometer does not need interrupts connected/configured for basic functionality. Interrupt declaration allows for additional features. Then there are two possible interrupt lines, only one is connected. Thus, either only one INT out of two, or none needs to be configured in the DT depending on the hardware setup. This also needs to be configured then in the sensor, which INT line to use for signalling. Thus we need the information if INT1 or INT2 was setup, if any. Hence, configuring an "interrupts" property only makes sense, if also a "interrupt-names" is configured, and vice versa. None of them are required for basic accelerometer functionality. Thank you so much for providing me the link to the annotated example-schema. I'll try then to set vice versa dependency of interrupts and interrupt-names and hope.. I'm sure you'll let me know right away if I'm doing something wrong. Seriously, thanks the link is really helpful! Best, L > Best regards, > Krzysztof >
On Fri, Dec 13, 2024 at 09:06:39AM +0100, Lothar Rubusch wrote: > On Thu, Dec 12, 2024 at 9:11 AM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > > On Wed, Dec 11, 2024 at 11:06:45PM +0000, Lothar Rubusch wrote: > > > Remove interrupts from the list of required properties. The ADXL345 > > > provides two interrupt lines. Anyway, the interrupts are an option, to > > > be used for additional event features. The driver can measure without > > > interrupts. Hence, interrupts should never have been required for the > > > ADXL345. Thus having interrupts required can be considered to be a > > > mistake. > > > > Partially this explains my question on previous patch, so consider > > reordering them. > > > > I understand. > > > And with combined knowledge, your driver now depends on interrupt names > > to setup interrupts. "interrupts" property alone is not sufficient, so > > you should encode it in the binding and explain in rationale why this is > > required (it is a change in ABI). > > > > https://elixir.bootlin.com/linux/v6.8-rc3/source/Documentation/devicetree/bindings/example-schema.yaml#L193 > > > > The accelerometer does not need interrupts connected/configured for > basic functionality. Interrupt declaration allows for additional > features. Then there are two possible interrupt lines, only one is > connected. Thus, either only one INT out of two, or none needs to be > configured in the DT depending on the hardware setup. This also needs > to be configured then in the sensor, which INT line to use for > signalling. Thus we need the information if INT1 or INT2 was setup, if > any. I meant, explain in the commit msg. > > Hence, configuring an "interrupts" property only makes sense, if also > a "interrupt-names" is configured, and vice versa. None of them are > required for basic accelerometer functionality. I know, I already stated this. But almost every question should have its answer in the commit msg. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml index 0fe878473..30ba4d3fb 100644 --- a/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml +++ b/Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml @@ -44,7 +44,6 @@ properties: required: - compatible - reg - - interrupts allOf: - $ref: /schemas/spi/spi-peripheral-props.yaml#
Remove interrupts from the list of required properties. The ADXL345 provides two interrupt lines. Anyway, the interrupts are an option, to be used for additional event features. The driver can measure without interrupts. Hence, interrupts should never have been required for the ADXL345. Thus having interrupts required can be considered to be a mistake. Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> --- Documentation/devicetree/bindings/iio/accel/adi,adxl345.yaml | 1 - 1 file changed, 1 deletion(-)