Hi Conor, > On May 26, 2023, at 2:27 PM, Conor Dooley <conor@xxxxxxxxxx> 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@xxxxxxxxxxxxx> > ✓ [PATCH v2 2/5] Input: cs40l26 - Support for CS40L26 Haptic Device > ✓ Signed: DKIM/cirrus.com > + Signed-off-by: Conor Dooley <conor.dooley@xxxxxxxxxxxxx> > 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@xxxxxxxxxx> >> --- >> .../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@xxxxxxxxxx> >> + >> +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
