diff mbox series

[v9,2/5] dt-bindings: input: cirrus,cs40l50: Add initial DT binding

Message ID 20240308222421.188858-3-jogletre@opensource.cirrus.com (mailing list archive)
State Superseded
Headers show
Series Add support for CS40L50 | expand

Commit Message

James Ogletree March 8, 2024, 10:24 p.m. UTC 
The CS40L50 is a haptic driver with waveform memory,
integrated DSP, and closed-loop algorithms.

Add a YAML DT binding document for this device.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com>
---
 .../bindings/input/cirrus,cs40l50.yaml        | 70 +++++++++++++++++++
 MAINTAINERS                                   |  8 +++
 2 files changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml

Comments

Jeff LaBundy March 10, 2024, 7:29 p.m. UTC | #1 
Hi James,

On Fri, Mar 08, 2024 at 10:24:18PM +0000, James Ogletree wrote:
> The CS40L50 is a haptic driver with waveform memory,
> integrated DSP, and closed-loop algorithms.
> 
> Add a YAML DT binding document for this device.
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com>
> ---
>  .../bindings/input/cirrus,cs40l50.yaml        | 70 +++++++++++++++++++
>  MAINTAINERS                                   |  8 +++
>  2 files changed, 78 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml b/Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
> new file mode 100644
> index 000000000000..6a5bdafed56b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
> @@ -0,0 +1,70 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/cirrus,cs40l50.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Cirrus Logic CS40L50 Advanced Haptic Driver
> +
> +maintainers:
> +  - James Ogletree <james.ogletree@cirrus.com>

Nit: this email address and the one in MAINTAINERS don't match the one
you're using to sign off. There is no requirement that they do; I just
wanted to check whether this was intentional.

> +
> +description:
> +  CS40L50 is a haptic driver with waveform memory,
> +  integrated DSP, and closed-loop algorithms.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - cirrus,cs40l50
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reset-gpios:
> +    maxItems: 1
> +
> +  va-supply:
> +    description: Power supply for internal analog circuits.
> +
> +  vp-supply:
> +    description: Power supply for always-on circuits.
> +
> +  vio-supply:
> +    description: Power supply for digital input/output.
> +
> +  vamp-supply:
> +    description: Power supply for the Class D amplifier.

Does L50 support external boost mode? If not, it will always be shorted
directly to VBST on the board, and there is no reason to describe it in
the binding.

If external boost mode is supported, then I recommend extending support
for it in the driver. Perhaps some additional registers must be set if
this supply is present.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - reset-gpios
> +  - vp-supply

Making VP a required supply is likely inconvenient for customers; 99% of
them connect it to a battery, and end up tying this property to a dummy
regulator to keep the driver from bleating.

Only for a wall-powered case would VP be tied to something like a 3.3-V
switching supply, and I imagine those cases are rare. It seems that VP
should be optional.

> +  - vio-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;

Nit: most device trees tend to use 8-column indentation as with kernel code.

> +
> +      haptic-driver@34 {
> +        compatible = "cirrus,cs40l50";
> +        reg = <0x34>;
> +        interrupt-parent = <&gpio>;
> +        interrupts = <113 IRQ_TYPE_LEVEL_LOW>;
> +        reset-gpios = <&gpio 112 GPIO_ACTIVE_LOW>;
> +        vp-supply = <&vreg>;
> +        vio-supply = <&vreg>;

Showing VP and VIO tied to the same supply is not a valid example; VP
typically connects to a battery, and VIO is likely a 1.8-V supply. Their
voltage ranges do not overlap, and hence they cannot be shared. I also
suspect there are sequencing restrictions between them as well.

> +      };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dd5de540ec0b..b71017a187f8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4933,6 +4933,14 @@ F:	sound/pci/hda/cs*
>  F:	sound/pci/hda/hda_cs_dsp_ctl.*
>  F:	sound/soc/codecs/cs*
>  
> +CIRRUS LOGIC HAPTIC DRIVERS
> +M:	James Ogletree <james.ogletree@cirrus.com>
> +M:	Fred Treven <fred.treven@cirrus.com>
> +M:	Ben Bright <ben.bright@cirrus.com>
> +L:	patches@opensource.cirrus.com
> +S:	Supported
> +F:	Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
> +
>  CIRRUS LOGIC DSP FIRMWARE DRIVER
>  M:	Simon Trimmer <simont@opensource.cirrus.com>
>  M:	Charles Keepax <ckeepax@opensource.cirrus.com>
> -- 
> 2.25.1
> 

Kind regards,
Jeff LaBundy
Krzysztof Kozlowski March 11, 2024, 6:31 a.m. UTC | #2 
On 10/03/2024 20:29, Jeff LaBundy wrote:
> 
>> +  - vio-supply
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
> 
> Nit: most device trees tend to use 8-column indentation as with kernel code.

If you meant DTS, then kernel coding style applies, which does not use
spaces. But this is a binding, so please use indentation as defined by
writing-schema: 2 or 4 spaces.


Best regards,
Krzysztof
Jeff LaBundy March 14, 2024, 2:42 p.m. UTC | #3 
Hi Krzysztof,

On Mon, Mar 11, 2024 at 07:31:36AM +0100, Krzysztof Kozlowski wrote:
> On 10/03/2024 20:29, Jeff LaBundy wrote:
> > 
> >> +  - vio-supply
> >> +
> >> +additionalProperties: false
> >> +
> >> +examples:
> >> +  - |
> >> +    #include <dt-bindings/gpio/gpio.h>
> >> +    #include <dt-bindings/interrupt-controller/irq.h>
> >> +
> >> +    i2c {
> >> +      #address-cells = <1>;
> >> +      #size-cells = <0>;
> > 
> > Nit: most device trees tend to use 8-column indentation as with kernel code.
> 
> If you meant DTS, then kernel coding style applies, which does not use
> spaces. But this is a binding, so please use indentation as defined by
> writing-schema: 2 or 4 spaces.

Good to know; I'll follow this definition moving forward.

> 
> 
> Best regards,
> Krzysztof
> 

Kind regards,
Jeff LaBundy
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml b/Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
new file mode 100644
index 000000000000..6a5bdafed56b
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
@@ -0,0 +1,70 @@ 
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/cirrus,cs40l50.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Cirrus Logic CS40L50 Advanced Haptic Driver
+
+maintainers:
+  - James Ogletree <james.ogletree@cirrus.com>
+
+description:
+  CS40L50 is a haptic driver with waveform memory,
+  integrated DSP, and closed-loop algorithms.
+
+properties:
+  compatible:
+    enum:
+      - cirrus,cs40l50
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  reset-gpios:
+    maxItems: 1
+
+  va-supply:
+    description: Power supply for internal analog circuits.
+
+  vp-supply:
+    description: Power supply for always-on circuits.
+
+  vio-supply:
+    description: Power supply for digital input/output.
+
+  vamp-supply:
+    description: Power supply for the Class D amplifier.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - reset-gpios
+  - vp-supply
+  - vio-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      haptic-driver@34 {
+        compatible = "cirrus,cs40l50";
+        reg = <0x34>;
+        interrupt-parent = <&gpio>;
+        interrupts = <113 IRQ_TYPE_LEVEL_LOW>;
+        reset-gpios = <&gpio 112 GPIO_ACTIVE_LOW>;
+        vp-supply = <&vreg>;
+        vio-supply = <&vreg>;
+      };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index dd5de540ec0b..b71017a187f8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4933,6 +4933,14 @@  F:	sound/pci/hda/cs*
 F:	sound/pci/hda/hda_cs_dsp_ctl.*
 F:	sound/soc/codecs/cs*
 
+CIRRUS LOGIC HAPTIC DRIVERS
+M:	James Ogletree <james.ogletree@cirrus.com>
+M:	Fred Treven <fred.treven@cirrus.com>
+M:	Ben Bright <ben.bright@cirrus.com>
+L:	patches@opensource.cirrus.com
+S:	Supported
+F:	Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
+
 CIRRUS LOGIC DSP FIRMWARE DRIVER
 M:	Simon Trimmer <simont@opensource.cirrus.com>
 M:	Charles Keepax <ckeepax@opensource.cirrus.com>