diff mbox series

[v2,1/5] dt-bindings: input: cirrus,cs40l26: Support for CS40L26

Message ID 1685059471-9598-1-git-send-email-fred.treven@cirrus.com (mailing list archive)
State New
Headers show
Series [v2,1/5] dt-bindings: input: cirrus,cs40l26: Support for CS40L26 | expand

Commit Message

Fred Treven May 26, 2023, 12:04 a.m. UTC 
Introduce required basic devicetree parameters for the
initial commit of CS40L26.

Signed-off-by: Fred Treven <fred.treven@cirrus.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

Comments

Conor Dooley May 26, 2023, 7:27 p.m. UTC | #1 
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@microchip.com>
	  ✓ [PATCH v2 2/5] Input: cs40l26 - Support for CS40L26 Haptic Device
	    ✓ Signed: DKIM/cirrus.com
	    + Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
	  ERROR: missing [3/5]!
	  ERROR: missing [4/5]!
	  ERROR: missing [5/5]!
	
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@cirrus.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@cirrus.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.

> +
> +  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.

> +    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.

> +    description: Maximum target voltage to which DSP may increase the VBST supply.
> +    multipleOf: 50000
> +    minimum: 2550000
> +    maximum: 11000000
> +    default: 11000000
> +
> +  cirrus,bst-exploratory-mode-disable:

This one is a lot better ;)

> +    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.

> +        compatible = "cirrus,cs40l26a";
> +        reg = <0x58>;
> +        interrupt-parent = <&gpio>;
> +        interrupts = <57 IRQ_TYPE_LEVEL_LOW>;
> +        reset-gpios = <&gpio 54 GPIO_ACTIVE_LOW>;
> +        VA-supply = <&dummy_vreg>;
> +        VP-supply = <&dummy_vreg>;
> +        cirrus,bst-ctl-microvolt = <2600000>;
> +        cirrus,bst-ipk-microamp = <1650000>;
> +        cirrus,bst-exploratory-mode-disable;
> +      };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2b073facf399..d72ed4957b0b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4926,6 +4926,16 @@ L:	netdev@vger.kernel.org
>  S:	Maintained
>  F:	drivers/net/ethernet/cirrus/ep93xx_eth.c
>  
> +CIRRUS LOGIC HAPTICS DRIVER
> +M:	Fred Treven <fred.treven@cirrus.com>
> +M:	Ben Bright <ben.bright@cirrus.com>
> +M:	James Ogletree <james.ogletree@cirrus.com>
> +L:	patches@opensource.cirrus.com
> +S:	Supported
> +W:	https://github.com/CirrusLogic/linux-drivers/wiki
> +T:	git https://github.com/CirrusLogic/linux-drivers.git
> +F:	Documentation/devicetree/bindings/input/cirrus,cs40l26.yaml
> +
>  CIRRUS LOGIC LOCHNAGAR DRIVER
>  M:	Charles Keepax <ckeepax@opensource.cirrus.com>
>  M:	Richard Fitzgerald <rf@opensource.cirrus.com>
> -- 
> 2.7.4
>
Fred Treven May 26, 2023, 9:32 p.m. UTC | #2 
Hi Conor,

> On May 26, 2023, at 2:27 PM, Conor Dooley <conor@kernel.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@microchip.com>
>   ✓ [PATCH v2 2/5] Input: cs40l26 - Support for CS40L26 Haptic Device
>     ✓ Signed: DKIM/cirrus.com
>     + Signed-off-by: Conor Dooley <conor.dooley@microchip.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@cirrus.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@cirrus.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
Conor Dooley May 26, 2023, 10:03 p.m. UTC | #3 
On Fri, May 26, 2023 at 09:32:36PM +0000, Fred Treven wrote:
> > On May 26, 2023, at 2:27 PM, Conor Dooley <conor@kernel.org> wrote:
> > 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@microchip.com>
> >   ✓ [PATCH v2 2/5] Input: cs40l26 - Support for CS40L26 Haptic Device
> >     ✓ Signed: DKIM/cirrus.com
> >     + Signed-off-by: Conor Dooley <conor.dooley@microchip.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. 

I just whack an N into git format-patch's --reroll-count/-v option, and
use the same across the whole series. Makes people's and tool's lives
easier :)

> >> +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.

What I saw looking in the driver was:
+static const struct of_device_id cs40l26_of_match[CS40L26_NUM_DEVS + 1] = {
+	{ .compatible = "cirrus,cs40l26a" },
+	{ .compatible = "cirrus,cs40l26b" },
+	{ .compatible = "cirrus,cs40l27a" },
+	{ .compatible = "cirrus,cs40l27b" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, cs40l26_of_match);

So you have a bunch of compatibles, but didn't immediately appear to be
doing anything different depending on which one of them you get.
What I meant was populating the data field of of_device_id with something
different depending on the compatible.
If the programming model is the same, you can document that they are all
compatible with "cirrus,cs40l26a", rather than having to add new entries
to the match table. Also has the advantage that if you bring out a
cirrus,cs40l27c that is compatible with the existing devices, then no
changes are needed to software to support it ;)

Or perhaps you are doing something different based on the compatible
that I just did not notice.

Cheers,
Conor.

(The [CS40L26_NUM_DEVS + 1] is also usually just [] btw)
diff mbox series

Patch

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@cirrus.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
+
+  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:
+    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:
+    description: Maximum target voltage to which DSP may increase the VBST supply.
+    multipleOf: 50000
+    minimum: 2550000
+    maximum: 11000000
+    default: 11000000
+
+  cirrus,bst-exploratory-mode-disable:
+    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 {
+        compatible = "cirrus,cs40l26a";
+        reg = <0x58>;
+        interrupt-parent = <&gpio>;
+        interrupts = <57 IRQ_TYPE_LEVEL_LOW>;
+        reset-gpios = <&gpio 54 GPIO_ACTIVE_LOW>;
+        VA-supply = <&dummy_vreg>;
+        VP-supply = <&dummy_vreg>;
+        cirrus,bst-ctl-microvolt = <2600000>;
+        cirrus,bst-ipk-microamp = <1650000>;
+        cirrus,bst-exploratory-mode-disable;
+      };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 2b073facf399..d72ed4957b0b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4926,6 +4926,16 @@  L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/ethernet/cirrus/ep93xx_eth.c
 
+CIRRUS LOGIC HAPTICS DRIVER
+M:	Fred Treven <fred.treven@cirrus.com>
+M:	Ben Bright <ben.bright@cirrus.com>
+M:	James Ogletree <james.ogletree@cirrus.com>
+L:	patches@opensource.cirrus.com
+S:	Supported
+W:	https://github.com/CirrusLogic/linux-drivers/wiki
+T:	git https://github.com/CirrusLogic/linux-drivers.git
+F:	Documentation/devicetree/bindings/input/cirrus,cs40l26.yaml
+
 CIRRUS LOGIC LOCHNAGAR DRIVER
 M:	Charles Keepax <ckeepax@opensource.cirrus.com>
 M:	Richard Fitzgerald <rf@opensource.cirrus.com>