| Message ID | 20230319204802.1364-4-kaehndan@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Sun, Mar 19, 2023 at 03:48:02PM -0500, Danny Kaehn wrote: > Support describing the CP2112's I2C and GPIO interfaces in firmware. > > I2C and GPIO child nodes can either be children with names "i2c" and > "gpio", or, for ACPI, device nodes with _ADR Zero and One, > respectively. > > Additionally, support configuring the I2C bus speed from the > clock-frequency device property. Thank you for the update, my comments below. ... > + struct i2c_timings timings; > + struct fwnode_handle *child; Longer line first easier to read. > + u32 addr; > + const char *name; Ditto. ... > + device_for_each_child_node(&hdev->dev, child) { > + name = fwnode_get_name(child); > + ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr); > + > + if ((name && strcmp("i2c", name) == 0) || (!ret && addr == 0)) > + device_set_node(&dev->adap.dev, child); > + else if ((name && strcmp("gpio", name)) == 0 || > + (!ret && addr == 1)) > + dev->gc.fwnode = child; > + } Please, make addresses defined explicitly. You may also do it with node naming 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); if (ret >= 0) addr = ret; } else ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr); if (ret < 0) ...error handling if needed... switch (addr) { case CP2112_I2C_ADR: device_set_node(&dev->adap.dev, child); break; case CP2112_GPIO_ADR: dev->gc.fwnode = child; break; default: ...error handling... } }
+Cc: Niyas, who is working a lot on filling the gaps in ACPI in comparison
to DT in the Linux kernel. Perhaps he has some ideas or even better
solutions.
On Mon, May 01, 2023 at 06:35:44PM -0500, Daniel Kaehn wrote:
> On Mon, Mar 20, 2023 at 9:10 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Mar 20, 2023 at 08:40:07AM -0500, Daniel Kaehn wrote:
> > > On Mon, Mar 20, 2023 at 8:00 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Mon, Mar 20, 2023 at 02:58:07PM +0200, Andy Shevchenko wrote:
> > > > > On Sun, Mar 19, 2023 at 03:48:02PM -0500, Danny Kaehn wrote:
...
> > > > > > + device_for_each_child_node(&hdev->dev, child) {
> > > > > > + name = fwnode_get_name(child);
> > > > > > + ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> > > > > > +
> > > > > > + if ((name && strcmp("i2c", name) == 0) || (!ret && addr == 0))
> > > > > > + device_set_node(&dev->adap.dev, child);
> > > > > > + else if ((name && strcmp("gpio", name)) == 0 ||
> > > > > > + (!ret && addr == 1))
> > > > > > + dev->gc.fwnode = child;
> > > > > > + }
> > > > >
> > > > > Please, make addresses defined explicitly. You may also do it with node naming
> > > > > 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);
> > > > > if (ret >= 0)
> > > > > addr = ret;
> > > > > } else
> > > > > ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
> > > > > if (ret < 0)
> > > > > ...error handling if needed...
> > > > >
> > > > > switch (addr) {
> > > > > case CP2112_I2C_ADR:
> > > > > device_set_node(&dev->adap.dev, child);
> > > > > 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
> > > > de facto used patterns (we have a couple of mode drivers that have a common
> > > > code to read "reg" or _ADR() and that code can be split into a helper and used
> > > > here).
> > >
> > > Named nodes _seem_ to be preferred in DT for when there isn't a logical /
> > > natural numbering to the child nodes. A.e. for USB, reg is used to specify
> > > which port, for I2C, which address on the bus, but for two parallel and
> > > independent functions on the same device, it seems named nodes would make
> > > more sense in DT. Many examples exist in mainline where named nodes are used
> > > in DT in this way.
> >
> > Okay, I'm not an expert in the DT preferable schemas, so I believe DT people
> > should answer on this.
>
> Hello,
>
> Thanks for all the time spent reviewing this thus far. Following up to
> see what my next steps might be.
>
> It sounds like we might want some DT folks to weigh in on the strategy
> 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
> court to seek out an answer/opinion here. (I know folks get a lot of
> email, so apologies if the correct move would have been to wait a bit
> longer before following up! Not intending to be rude.)
>
> Would it be appropriate / expected that I send a separate email thread
> 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
> separate email thread for the initial DT vs. ACPI conversation we had
> about accessing children by name or index in a unified way due to the
> 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
>
> > > 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
> > > in DT -- my opinion should definitely be taken with a grain of salt. Maybe
> > > this is something I should follow up on with DT folks on that DT vs. ACPI
> > > thread made earlier.
> > >
> > > One thing I did notice when looking at the mfd subsystem is that most DT
> > > drivers actually match on the compatible string of the child nodes, a.e.
> > > "silabs,cp2112", "silabs,cp2112-gpio". "silabs,cp2112-i2c". We could
> > > implement that here, but I think that would make more sense if we were to
> > > actually split the cp2112 into mfd & platform drivers, and additionally split
> > > the DT binding by function.
> >
> > IIRC (but might be very well mistaken) the compatible strings for children
> > are discouraged.
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. 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/ Andy suggested I use _ADR to identify the child node by index for ACPI https://lore.kernel.org/all/ZAi4NjqXTbLpVhPo@smile.fi.intel.com/ 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/ 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/ The DT binding patch in question https://lore.kernel.org/all/20230319204802.1364-2-kaehndan@gmail.com/ Thanks, Danny Kaehn On Mon, Jul 3 2023 at 13:57:22 +0300 Andy Shevchenko write: > On Mon, May 01, 2023 at 06:35:44PM -0500, Daniel Kaehn wrote: > > On Mon, Mar 20, 2023 at 9:10 AM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Mon, Mar 20, 2023 at 08:40:07AM -0500, Daniel Kaehn wrote: > > > > On Mon, Mar 20, 2023 at 8:00 AM Andy Shevchenko > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > On Mon, Mar 20, 2023 at 02:58:07PM +0200, Andy Shevchenko Wrote: > > > > > > On Sun, Mar 19, 2023 at 03:48:02PM -0500, Danny Kaehn wrote: > +Cc: Niyas, who is working a lot on filling the gaps in ACPI in comparison > to DT in the Linux kernel. Perhaps he has some ideas or even better > solutions. > > > ... > > > > > > > > + device_for_each_child_node(&hdev->dev, child) { > > > > > > > + name = fwnode_get_name(child); > > > > > > > + ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr); > > > > > > > + > > > > > > > + if ((name && strcmp("i2c", name) == 0) || (!ret && addr == 0)) > > > > > > > + device_set_node(&dev->adap.dev, child); > > > > > > > + else if ((name && strcmp("gpio", name)) == 0 || > > > > > > > + (!ret && addr == 1)) > > > > > > > + dev->gc.fwnode = child; > > > > > > > + } > > > > > > > > > > > > Please, make addresses defined explicitly. You may also do it with node naming > > > > > > 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); > > > > > > if (ret >= 0) > > > > > > addr = ret; > > > > > > } else > > > > > > ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr); > > > > > > if (ret < 0) > > > > > > ...error handling if needed... > > > > > > > > > > > > switch (addr) { > > > > > > case CP2112_I2C_ADR: > > > > > > device_set_node(&dev->adap.dev, child); > > > > > > 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 > > > > > de facto used patterns (we have a couple of mode drivers that have a common > > > > > code to read "reg" or _ADR() and that code can be split into a helper and used > > > > > here). > > > > > > > > Named nodes _seem_ to be preferred in DT for when there isn't a logical / > > > > natural numbering to the child nodes. A.e. for USB, reg is used to specify > > > > which port, for I2C, which address on the bus, but for two parallel and > > > > independent functions on the same device, it seems named nodes would make > > > > more sense in DT. Many examples exist in mainline where named nodes are used > > > > in DT in this way. > > > > > > Okay, I'm not an expert in the DT preferable schemas, so I believe DT people > > > should answer on this. > > > > Hello, > > > > Thanks for all the time spent reviewing this thus far. Following up to > > see what my next steps might be. > > > > It sounds like we might want some DT folks to weigh in on the strategy > > 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 > > court to seek out an answer/opinion here. (I know folks get a lot of > > email, so apologies if the correct move would have been to wait a bit > > longer before following up! Not intending to be rude.) > > > > Would it be appropriate / expected that I send a separate email thread > > 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 > > separate email thread for the initial DT vs. ACPI conversation we had > > about accessing children by name or index in a unified way due to the > > 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 > > > > > > 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 > > > > in DT -- my opinion should definitely be taken with a grain of salt. Maybe > > > > this is something I should follow up on with DT folks on that DT vs. ACPI > > > > thread made earlier. > > > > > > > > One thing I did notice when looking at the mfd subsystem is that most DT > > > > drivers actually match on the compatible string of the child nodes, a.e. > > > > "silabs,cp2112", "silabs,cp2112-gpio". "silabs,cp2112-i2c". We could > > > > implement that here, but I think that would make more sense if we were to > > > > actually split the cp2112 into mfd & platform drivers, and additionally split > > > > the DT binding by function. > > > > > > IIRC (but might be very well mistaken) the compatible strings for children > > > are discouraged. >
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c index 27cadadda7c9..9e327763fd90 100644 --- a/drivers/hid/hid-cp2112.c +++ b/drivers/hid/hid-cp2112.c @@ -1234,6 +1234,10 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id) u8 buf[3]; struct cp2112_smbus_config_report config; struct gpio_irq_chip *girq; + struct i2c_timings timings; + struct fwnode_handle *child; + u32 addr; + const char *name; int ret; dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL); @@ -1247,6 +1251,17 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id) mutex_init(&dev->lock); + device_for_each_child_node(&hdev->dev, child) { + name = fwnode_get_name(child); + ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr); + + if ((name && strcmp("i2c", name) == 0) || (!ret && addr == 0)) + device_set_node(&dev->adap.dev, child); + else if ((name && strcmp("gpio", name)) == 0 || + (!ret && addr == 1)) + dev->gc.fwnode = child; + } + ret = hid_parse(hdev); if (ret) { hid_err(hdev, "parse failed\n"); @@ -1292,6 +1307,9 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id) goto err_power_normal; } + i2c_parse_fw_timings(&dev->adap.dev, &timings, true); + + config.clock_speed = cpu_to_be32(timings.bus_freq_hz); config.retry_time = cpu_to_be16(1); ret = cp2112_hid_output(hdev, (u8 *)&config, sizeof(config),
Support describing the CP2112's I2C and GPIO interfaces in firmware. I2C and GPIO child nodes can either be children with names "i2c" and "gpio", or, for ACPI, device nodes with _ADR Zero and One, respectively. Additionally, support configuring the I2C bus speed from the clock-frequency device property. Signed-off-by: Danny Kaehn <kaehndan@gmail.com> --- drivers/hid/hid-cp2112.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)