Re: [PATCH v9 3/3] HID: cp2112: Fwnode Support

From: Benjamin Tissoires <bentiss@kernel.org>
Date: 2024-01-18 08:58:34
Also in: linux-devicetree

Hi Danny,

On Jan 17 2024, Danny Kaehn wrote:

Hello folks, wanted to give one more follow up on this
patch/discussion. Would a reasonable next step for me
to help nudge this forward be to submit a v10 addressing
Andy's most recent code comments? Again hoping I'm not being
rude or stepping on toes; just want to make sure I'm doing my
dilligence to move things forward. I'll assume that going ahead
and submitting a v10 with unresolved discussion here isn't a
terrible offense if I don't end up getting a response here in 
the next week or so.

Submitting a v10, even if there are still undecided points is definitely
the way forward. People probably have forgot a lot of things there and
need a refresh on the latest state of it :)

Leaving some links to some of the more key points of the discussion
across the versions of this patch, since it's been ~5 months since
the last activity here.

Discussion began with discussion of using child nodes by name
across DT with ACPI, for binding fwnodes for the CP2112's I2C
and GPIO controllers; since  ACPI requires uppercase names (and
names should specifically not be meaningful in ACPI)
https://lore.kernel.org/all/Y%2F9oO1AE6GK6CQmp@smile.fi.intel.com/ (local)

I think the DT part is fine. Please resubmit it in v10, but probably
drop the previous rev-by and explicitly mention you did so after the
first '---' below your signed-off-by. This should re-trigger a review
from them. Things may have changed since last year, and having another
review would be beneficial IMO.

Andy suggested I use _ADR to identify the child node by index
for ACPI
https://lore.kernel.org/all/ZAi4NjqXTbLpVhPo@smile.fi.intel.com/ (local)

I think I still prefer the "_DSD" approach with the cell-names, but
OTOH, it's not like there is an official ACPI description for this, and
we can basically define whatever we want. So please go ahead with the
_ADR approach IMO, with a couple of changes:

- mention about that in the DT bindings documentation
- please add an enum with those 2 addresses (with kernel doc), to
  document it in the code and not have magic constants in your checks

v9 implemented matching by child node name OR by address depnding
on the type of firmware used
https://lore.kernel.org/all/20230319204802.1364-4-kaehndan@gmail.com/ (local)

See my 2 comments above.

FWIW, I think 2/3 could go directly in as well, but the timing is not
ideal, we are in the middle of the Merge Window.

Some additional discussion on whether matching child nodes by name
is the best approach even for the DT side
(also within the in-line body of this email)
https://lore.kernel.org/all/ZBhoHzTr5l38u%2FkX@smile.fi.intel.com/ (local)

Honestly, not sure we'll have too many users on the ACPI side (besides
myself). So if you really feel uncomfortable, you can always put a
warning that we are using _ADR in the ACPI world as a fallback, but that
we might revisit that in the future (with naming, if we reach to an
agreement).

Cheers,
Benjamin


The DT binding patch in question
https://lore.kernel.org/all/20230319204802.1364-2-kaehndan@gmail.com/ (local)

Thanks,

Danny Kaehn




On Mon, Jul 3 2023 at 13:57:22 +0300 Andy Shevchenko write:

quoted

On Mon, May 01, 2023 at 06:35:44PM -0500, Daniel Kaehn wrote:

quoted

On Mon, Mar 20, 2023 at 9:10 AM Andy Shevchenko
[off-list ref] wrote:

quoted

On Mon, Mar 20, 2023 at 08:40:07AM -0500, Daniel Kaehn wrote:

quoted

On Mon, Mar 20, 2023 at 8:00 AM Andy Shevchenko
[off-list ref] wrote:

quoted

On Mon, Mar 20, 2023 at 02:58:07PM +0200, Andy Shevchenko

Wrote:

quoted

quoted
quoted
quoted
quoted
quoted
On Sun, Mar 19, 2023 at 03:48:02PM -0500, Danny Kaehn

wrote:

quoted

+Cc: Niyas, who is working a lot on filling the gaps in ACPI in

comparison

quoted

     to DT in the Linux kernel. Perhaps he has some ideas or even

better

quoted

     solutions.


...

quoted

+   device_for_each_child_node(&hdev->dev, child) {
+           name = fwnode_get_name(child);
+           ret =

acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);

quoted

quoted
quoted
quoted
quoted
quoted
quoted
+
+           if ((name && strcmp("i2c", name) == 0) ||

(!ret && addr == 0))

quoted

quoted
quoted
quoted
quoted
quoted
quoted
+                   device_set_node(&dev->adap.dev,

child);

quoted

quoted
quoted
quoted
quoted
quoted
quoted
+           else if ((name && strcmp("gpio", name)) == 0

||

quoted

+                                   (!ret && addr == 1))
+                   dev->gc.fwnode = child;
+   }

Please, make addresses defined explicitly. You may also do

it with node naming

quoted

schema:

#define CP2112_I2C_ADR                0
#define CP2112_GPIO_ADR               1

static const char * const cp2112_cell_names[] = {
      [CP2112_I2C_ADR]        = "i2c",
      [CP2112_GPIO_ADR]       = "gpio",
};

      device_for_each_child_node(&hdev->dev, child) {
              name = fwnode_get_name(child);
              if (name) {
                      ret = match_string(cp2112_cell_names,

ARRAY_SIZE(cp2112_cell_names), name);

quoted

                      if (ret >= 0)
                              addr = ret;
              } else
                      ret =

acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);

quoted

              if (ret < 0)
                      ...error handling if needed...

              switch (addr) {
              case CP2112_I2C_ADR:
                      device_set_node(&dev->adap.dev,

child);

quoted

                      break;
              case CP2112_GPIO_ADR:
                      dev->gc.fwnode = child;
                      break;
              default:
                      ...error handling...
              }
      }

Btw, don't you use "reg" property for the child nodes? It

would be better from

quoted

de facto used patterns (we have a couple of mode drivers that

have a common

quoted

code to read "reg" or _ADR() and that code can be split into

a helper and used

quoted

here).

Named nodes _seem_ to be preferred in DT for when there isn't a

logical /

quoted

natural numbering to the child nodes. A.e. for USB, reg is used

to specify

quoted

which port, for I2C, which address on the bus, but for two

parallel and

quoted

independent functions on the same device, it seems named nodes

would make

quoted

more sense in DT. Many examples exist in mainline where named

nodes are used

quoted

in DT in this way.

Okay, I'm not an expert in the DT preferable schemas, so I

believe DT people

quoted

should answer on this.

Hello,

Thanks for all the time spent reviewing this thus far. Following up

to

quoted

see what my next steps might be.

It sounds like we might want some DT folks to weigh in on the

strategy

quoted

used for identifying the child I2C and GPIO nodes for the CP2112
device before moving further toward applying this.

Since the DT list is on this thread (as well as Rob+Krzystof), and
this has sat for a little while, I'm assuming that the ball is in

my

quoted

court to seek out an answer/opinion here. (I know folks get a lot

of

quoted

email, so apologies if the correct move would have been to wait a

bit

quoted

longer before following up! Not intending to be rude.)

Would it be appropriate / expected that I send a separate email

thread

quoted

to the DT mailing list on their opinion here? Or would that create
more confusion/complexity in adding yet another thread? I did

create a

quoted

separate email thread for the initial DT vs. ACPI conversation we

had

quoted

about accessing children by name or index in a unified way due to

the

quoted

differences in upper/lower case and use-cases, but that
(understandably) didn't seem to gain any traction.

Thanks for any insights!

Thanks,
Danny Kaehn

quoted

One example is network cards which provide an mdio bus
bind through the child "mdio". One example of a specifically a
child i2c controller being bound to "i2c" can be found in
pine64,pinephone-keyboard.yaml.
But it's certainly possible this isn't the desired direction

moving forward

quoted

in DT -- my opinion should definitely be taken with a grain of

salt. Maybe

quoted

this is something I should follow up on with DT folks on that

DT vs. ACPI

quoted

thread made earlier.

One thing I did notice when looking at the mfd subsystem is

that most DT

quoted

drivers actually match on the compatible string of the child

nodes, a.e.

quoted

"silabs,cp2112", "silabs,cp2112-gpio".  "silabs,cp2112-i2c". We

could

quoted

implement that here, but I think that would make more sense if

we were to

quoted

actually split the cp2112 into mfd & platform drivers, and

additionally split

quoted

the DT binding by function.

IIRC (but might be very well mistaken) the compatible strings for

children

quoted

quoted
quoted
are discouraged.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help