Date:   Fri, 26 May 2023 21:32:36 +0000
From:   Fred Treven <Fred.Treven@...rus.com>
To:     Conor Dooley <conor@...nel.org>
CC:     Ben Bright <Ben.Bright@...rus.com>,
        James Ogletree <James.Ogletree@...rus.com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Simon Trimmer <simont@...nsource.cirrus.com>,
        Charles Keepax <ckeepax@...nsource.cirrus.com>,
        Richard Fitzgerald <rf@...nsource.cirrus.com>,
        "patches@...nsource.cirrus.com" <patches@...nsource.cirrus.com>,
        "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "lee@...nel.org" <lee@...nel.org>
Subject: Re: [PATCH v2 1/5] dt-bindings: input: cirrus,cs40l26: Support for
 CS40L26


Hi Conor,

> On May 26, 2023, at 2:27 PM, Conor Dooley <conor@...nel.org> wrote:
> 
> Yo Fred,
> 
> Tooling does not like your series very much, prob the missing v2's on
> some patches:
> Grabbing thread from lore.kernel.org/all/1685059471-9598-1-git-send-email-fred.treven%40cirrus.com/t.mbox.gz
> Checking for newer revisions
> Grabbing search results from lore.kernel.org
> Analyzing 6 messages in the thread
> Will use the latest revision: v2
> You can pick other revisions using the -vN flag
> Checking attestation on all messages, may take a moment...
> ---
>   ✓ [PATCH v2 1/5] dt-bindings: input: cirrus,cs40l26: Support for CS40L26
>     ✓ Signed: DKIM/cirrus.com
>     + Signed-off-by: Conor Dooley <conor.dooley@...rochip.com>
>   ✓ [PATCH v2 2/5] Input: cs40l26 - Support for CS40L26 Haptic Device
>     ✓ Signed: DKIM/cirrus.com
>     + Signed-off-by: Conor Dooley <conor.dooley@...rochip.com>
>   ERROR: missing [3/5]!
>   ERROR: missing [4/5]!
>   ERROR: missing [5/5]!

Understood. I was uncertain if this was just needed for patches that had been edited or for all new patches. I will resubmit with some other code changes to address other comments I’ve received and will mark the patches with the same version number. 

> 
> On Thu, May 25, 2023 at 07:04:27PM -0500, Fred Treven wrote:
>> Introduce required basic devicetree parameters for the
>> initial commit of CS40L26.
>> 
>> Signed-off-by: Fred Treven <fred.treven@...rus.com>
>> ---
>> .../devicetree/bindings/input/cirrus,cs40l26.yaml  | 102 +++++++++++++++++++++
>> MAINTAINERS                                        |  10 ++
>> 2 files changed, 112 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/input/cirrus,cs40l26.yaml
>> 
>> diff --git a/Documentation/devicetree/bindings/input/cirrus,cs40l26.yaml b/Documentation/devicetree/bindings/input/cirrus,cs40l26.yaml
>> new file mode 100644
>> index 000000000000..9cbc964ebded
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/cirrus,cs40l26.yaml
>> @@ -0,0 +1,102 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/input/cirrus,cs40l26.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Cirrus Logic CS40L26 Boosted Haptic Amplifier
>> +
>> +maintainers:
>> +  - Fred Treven <fred.treven@...rus.com>
>> +
>> +description:
>> +  CS40L26 is a Boosted Haptic Driver with Integrated DSP and Waveform Memory
>> +  with Advanced Closed Loop Algorithms and LRA protection
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - cirrus,cs40l26a
>> +      - cirrus,cs40l26b
>> +      - cirrus,cs40l27a
>> +      - cirrus,cs40l27b
> 
> I had a _brief_ look at the driver - you don't seem to have any match
> data, so are all of these devices actually compatible with eachother?
> 
> IOW, should this be:
> properties:
>  compatible:
>    oneOf:
>      - items:
>          - enum:
>              - cirrus,cs40l26b
>              - cirrus,cs40l27a
>              - cirrus,cs40l27b
>          - const: cirrus,cs40l26a
> 
>      - const: cirrus,cs40l26a
> 
> And then drop all but the cs40l26a in the driver? Apologies if I missed
> some difference in there.

They are all compatible, yes. There is match data in cs40l26-i2c.c and cs40l26-spi.c if you are referring to the of_device_id struct. Please let me know if I’m misunderstanding your meaning here.

> 
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  reset-gpios:
>> +    maxItems: 1
>> +
>> +  VA-supply:
>> +    description: Regulator for VA analog voltage
>> +
>> +  VP-supply:
>> +    description: Regulator for VP peak voltage
>> +
>> +  cirrus,bst-ipk-microamp:
> 
> Are these namings ripped from a datasheet? "bst-ipk" doesn't immediately
> mean anything to me, but I am not familiar with these devices.

Yes, they are taken from the data sheet with “bst-ipk” referring to boost inductor peak current consumption. I do think it makes sense to have clearer naming here so I can go ahead and make these changes.

> 
>> +    description:
>> +      Maximum amount of current that can be drawn by the device's boost converter.
>> +    multipleOf: 50000
>> +    minimum: 1600000
>> +    maximum: 4800000
>> +    default: 4500000
>> +
>> +  cirrus,bst-ctl-microvolt:
> 
> Ditto here. If there aren't rips, then maybe it'd be a good idea to use
> full words.

Same as above. Is ripped from DS, but doesn’t need to be called this if it’s not appropriate	

> 
>> +    description:
>> +      Disable boost exploratory mode.
>> +
>> +      In exploratory mode the analog maximum peak current limit of 4.5 A
>> +      (tolerance of + 160 mA) will be applied. This is required for the
>> +      device to successfully detect a boost inductor short.
>> +
>> +      Boost exploratory mode allows the device to overshoot the set boost peak
>> +      current limit (i.e. if current peak limit is set to 2.5 A to protect the
>> +      battery inductor, the current limit will be opened up to 4.5 A for
>> +      several milliseconds at boost startup).
>> +      This has potential to damage the boost inductor.
>> +
>> +      Disabling this mode will prevent this from happening; it will also
>> +      prevent the device from detecting boost inductor short errors.
>> +    type: boolean
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - reset-gpios
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/gpio/gpio.h>
>> +    #include <dt-bindings/input/input.h>
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      cs40l26@58 {
> 
> Generally using generic node names what we want, so something matching
> the class of device. Section 2.2.2 "Generic Names Recommendation" of the
> dt spec contains a bunch of ones to pick from, but I don't really know
> where "haptic amplifier" fits in!
> 
> Cheers,
> Conor.

Understood. I’ll try to find a better way to name the node.

Best regards,
Fred

Powered by blists - more mailing lists