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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3595 bytes --]

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)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]