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

[Jeff LaBundy <jeff@xxxxxxxxxxx>]

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@xxxxxxxxxx>
> Signed-off-by: James Ogletree <jogletre@xxxxxxxxxxxxxxxxxxxxx>
> ---
>  .../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@xxxxxxxxxx>

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@xxxxxxxxxx>
> +M:	Fred Treven <fred.treven@xxxxxxxxxx>
> +M:	Ben Bright <ben.bright@xxxxxxxxxx>
> +L:	patches@xxxxxxxxxxxxxxxxxxxxx
> +S:	Supported
> +F:	Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
> +
>  CIRRUS LOGIC DSP FIRMWARE DRIVER
>  M:	Simon Trimmer <simont@xxxxxxxxxxxxxxxxxxxxx>
>  M:	Charles Keepax <ckeepax@xxxxxxxxxxxxxxxxxxxxx>
> -- 
> 2.25.1
> 

Kind regards,
Jeff LaBundy