| Message ID | 20240205170920.93499-4-danny.kaehn@plexus.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | Firmware Support for USB-HID Devices and CP2112 | expand |
On Mon, Feb 05, 2024 at 11:09:22AM -0600, 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. This has to be in a separate patch, which may predecess this one. ... > + name = fwnode_get_name(child); > + if (name) { > + ret = match_string(cp2112_cell_names, > + ARRAY_SIZE(cp2112_cell_names), name); > + if (ret >= 0) > + addr = ret; > + } > + if (!name || ret < 0) > + ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr); > + > + if (ret < 0) > + continue; I don't like this piece (esp. due to possible matching with node name which may not be so reliable), but I have no better solution right now. Maybe this way (this doesn't particularly solve the issue but seems better to me)? ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr); if (ret) { /* If no ACPI given or compiled, fallback to matching names */ name = fwnode_get_name(child); if (!name) continue; ret = match_string(cp2112_cell_names, ARRAY_SIZE(cp2112_cell_names), name); if (ret < 0) continue; addr = ret; }
Hi Danny, kernel test robot noticed the following build warnings: [auto build test WARNING on hid/for-next] [also build test WARNING on robh/for-next andi-shyti/i2c/i2c-host linus/master v6.8-rc3 next-20240206] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Danny-Kaehn/dt-bindings-i2c-Add-CP2112-HID-USB-to-SMBus-Bridge/20240206-015922 base: https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git for-next patch link: https://lore.kernel.org/r/20240205170920.93499-4-danny.kaehn%40plexus.com patch subject: [PATCH v10 3/3] HID: cp2112: Fwnode Support config: x86_64-randconfig-102-20240206 (https://download.01.org/0day-ci/archive/20240206/202402062306.UUQJhqxi-lkp@intel.com/config) compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240206/202402062306.UUQJhqxi-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202402062306.UUQJhqxi-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/hid/hid-cp2112.c:41: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst * CP2112 Fwnode child names. vim +41 drivers/hid/hid-cp2112.c 39 40 /** > 41 * CP2112 Fwnode child names. 42 * CP2112 sub-functions can be described by named fwnode children or by ACPI _ADR 43 */ 44 static const char * const cp2112_cell_names[] = { 45 [CP2112_I2C_ADR] = "i2c", 46 [CP2112_GPIO_ADR] = "gpio", 47 }; 48
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c index 20a0d1315d90..2ec0e5b95845 100644 --- a/drivers/hid/hid-cp2112.c +++ b/drivers/hid/hid-cp2112.c @@ -27,6 +27,25 @@ #include <linux/usb/ch9.h> #include "hid-ids.h" +/** + * enum cp2112_child_acpi_cell_addrs - Child ACPI addresses for CP2112 sub-functions + * @CP2112_I2C_ADR: Address for I2C node + * @CP2112_GPIO_ADR: Address for GPIO node + */ +enum cp2112_child_acpi_cell_addrs { + CP2112_I2C_ADR = 0, + CP2112_GPIO_ADR = 1, +}; + +/** + * CP2112 Fwnode child names. + * CP2112 sub-functions can be described by named fwnode children or by ACPI _ADR + */ +static const char * const cp2112_cell_names[] = { + [CP2112_I2C_ADR] = "i2c", + [CP2112_GPIO_ADR] = "gpio", +}; + #define CP2112_REPORT_MAX_LENGTH 64 #define CP2112_GPIO_CONFIG_LENGTH 5 #define CP2112_GPIO_GET_LENGTH 2 @@ -1195,7 +1214,11 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id) struct cp2112_device *dev; u8 buf[3]; struct cp2112_smbus_config_report config; + struct fwnode_handle *child; struct gpio_irq_chip *girq; + struct i2c_timings timings; + const char *name; + u32 addr; int ret; dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL); @@ -1209,6 +1232,30 @@ 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); + if (name) { + ret = match_string(cp2112_cell_names, + ARRAY_SIZE(cp2112_cell_names), name); + if (ret >= 0) + addr = ret; + } + if (!name || ret < 0) + ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr); + + if (ret < 0) + continue; + + switch (addr) { + case CP2112_I2C_ADR: + device_set_node(&dev->adap.dev, child); + break; + case CP2112_GPIO_ADR: + dev->gc.fwnode = child; + break; + } + } + ret = hid_parse(hdev); if (ret) { hid_err(hdev, "parse failed\n"); @@ -1254,6 +1301,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 <danny.kaehn@plexus.com> --- Modeled this version based on Andy's email [1], but made the following primary changes: 1. Use enum instead of #defines at Benjamin's request 2. Change if() else on checking name existence to allow a fwnode to have a name that doesn't match to still be checked for its ACPI address (since fwnode_get_name() _does_ still return a name for ACPI nodes) 3. Continue gracefully / silently if matching fwnodes fails ACPI compatibility re-tested in QEMU as per conversations in v8. NOTE: now that I'm not using device_get_named_child_node(), I am no longer being left with a fwnode reference. I am not entirely sure if I _need_ one for how I am using the handles, so I have left out the calls to fwnode_handle_get() and fwnode_hand_put() for now. Plese correct me if this is a situation where a reference should be held until the driver is removed. Note that this has been present since v9, but I intended to include this comment on that patch. [1] https://lore.kernel.org/all/ZBhYXwjPeRiZwxMT@smile.fi.intel.com/ drivers/hid/hid-cp2112.c | 50 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)