Message ID | 20250312195951.1579682-4-jthies@google.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | Load cros_ec_ucsi from OF and ACPI definitions | expand |
On 12/03/2025 20:59, Jameson Thies wrote: > Check for cros_ec_ucsi to be defined in the OF device tree or an ACPI > node. If it is defined by either OF or ACPI, it does not need to be > added as a subdevice of cros_ec_dev. No, it does not have to. You just populate the children and appropriate devices will be created automatically. None of parent devices should ever check if the child exist to create a child - it makes no sense. Best regards, Krzysztof
On 13/03/2025 08:11, Krzysztof Kozlowski wrote: > On 12/03/2025 20:59, Jameson Thies wrote: >> Check for cros_ec_ucsi to be defined in the OF device tree or an ACPI >> node. If it is defined by either OF or ACPI, it does not need to be >> added as a subdevice of cros_ec_dev. > > No, it does not have to. You just populate the children and appropriate Uh, I did not notice that it is !of_find_compatible_node(), so this comment should be rephrased - you just add MFD children anyway and if there is no node, they won't be created. > devices will be created automatically. None of parent devices should > ever check if the child exist to create a child - it makes no sense. This is still valid - none of parents should be poking around to see if there is a child or not. The core handles it, DT handles it etc. Best regards, Krzysztof
Hi Jameson, kernel test robot noticed the following build errors: [auto build test ERROR on 9fc83373f0ffb8834da48b1446a5c2fef9525bb1] url: https://github.com/intel-lab-lkp/linux/commits/Jameson-Thies/dt-bindings-Add-cros-ec-ucsi-to-cros-ec-typec-device-tree-documentation/20250313-040216 base: 9fc83373f0ffb8834da48b1446a5c2fef9525bb1 patch link: https://lore.kernel.org/r/20250312195951.1579682-4-jthies%40google.com patch subject: [PATCH v1 3/3] mfd: cros_ec: Don't add cros_ec_ucsi if it is defined in OF or ACPI config: i386-buildonly-randconfig-001-20250313 (https://download.01.org/0day-ci/archive/20250313/202503132256.f6p359iT-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250313/202503132256.f6p359iT-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/202503132256.f6p359iT-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/mfd/cros_ec_dev.c: In function 'ec_device_probe': >> drivers/mfd/cros_ec_dev.c:273:61: error: passing argument 3 of 'device_find_child' from incompatible pointer type [-Werror=incompatible-pointer-types] 273 | ucsi_acpi_match); | ^~~~~~~~~~~~~~~ | | | int (*)(struct device *, void *) In file included from include/linux/acpi.h:14, from drivers/mfd/cros_ec_dev.c:8: include/linux/device.h:1101:49: note: expected 'device_match_t' {aka 'int (*)(struct device *, const void *)'} but argument is of type 'int (*)(struct device *, void *)' 1101 | device_match_t match); | ~~~~~~~~~~~~~~~^~~~~ cc1: some warnings being treated as errors vim +/device_find_child +273 drivers/mfd/cros_ec_dev.c 181 182 static int ec_device_probe(struct platform_device *pdev) 183 { 184 int retval = -ENOMEM; 185 struct device_node *node; 186 struct device *dev = &pdev->dev; 187 struct cros_ec_platform *ec_platform = dev_get_platdata(dev); 188 struct cros_ec_dev *ec = kzalloc(sizeof(*ec), GFP_KERNEL); 189 struct ec_response_pchg_count pchg_count; 190 int i; 191 192 if (!ec) 193 return retval; 194 195 dev_set_drvdata(dev, ec); 196 ec->ec_dev = dev_get_drvdata(dev->parent); 197 ec->dev = dev; 198 ec->cmd_offset = ec_platform->cmd_offset; 199 ec->features.flags[0] = -1U; /* Not cached yet */ 200 ec->features.flags[1] = -1U; /* Not cached yet */ 201 device_initialize(&ec->class_dev); 202 203 for (i = 0; i < ARRAY_SIZE(cros_mcu_devices); i++) { 204 /* 205 * Check whether this is actually a dedicated MCU rather 206 * than an standard EC. 207 */ 208 if (cros_ec_check_features(ec, cros_mcu_devices[i].id)) { 209 dev_info(dev, "CrOS %s MCU detected\n", 210 cros_mcu_devices[i].desc); 211 /* 212 * Help userspace differentiating ECs from other MCU, 213 * regardless of the probing order. 214 */ 215 ec_platform->ec_name = cros_mcu_devices[i].name; 216 break; 217 } 218 } 219 220 /* 221 * Add the class device 222 */ 223 ec->class_dev.class = &cros_class; 224 ec->class_dev.parent = dev; 225 ec->class_dev.release = cros_ec_class_release; 226 227 retval = dev_set_name(&ec->class_dev, "%s", ec_platform->ec_name); 228 if (retval) { 229 dev_err(dev, "dev_set_name failed => %d\n", retval); 230 goto failed; 231 } 232 233 retval = device_add(&ec->class_dev); 234 if (retval) 235 goto failed; 236 237 /* check whether this EC is a sensor hub. */ 238 if (cros_ec_get_sensor_count(ec) > 0) { 239 retval = mfd_add_hotplug_devices(ec->dev, 240 cros_ec_sensorhub_cells, 241 ARRAY_SIZE(cros_ec_sensorhub_cells)); 242 if (retval) 243 dev_err(ec->dev, "failed to add %s subdevice: %d\n", 244 cros_ec_sensorhub_cells->name, retval); 245 } 246 247 /* 248 * The following subdevices can be detected by sending the 249 * EC_FEATURE_GET_CMD Embedded Controller device. 250 */ 251 for (i = 0; i < ARRAY_SIZE(cros_subdevices); i++) { 252 if (cros_ec_check_features(ec, cros_subdevices[i].id)) { 253 retval = mfd_add_hotplug_devices(ec->dev, 254 cros_subdevices[i].mfd_cells, 255 cros_subdevices[i].num_cells); 256 if (retval) 257 dev_err(ec->dev, 258 "failed to add %s subdevice: %d\n", 259 cros_subdevices[i].mfd_cells->name, 260 retval); 261 } 262 } 263 264 /* 265 * FW nodes can load cros_ec_ucsi, but early PDC devices did not define 266 * the required nodes. On PDC systems without FW nodes for cros_ec_ucsi, 267 * the driver should be added as an mfd subdevice. 268 */ 269 if (cros_ec_check_features(ec, EC_FEATURE_USB_PD) && 270 cros_ec_check_features(ec, EC_FEATURE_UCSI_PPM)) { 271 struct device *acpi_dev = device_find_child(ec->ec_dev->dev, 272 NULL, > 273 ucsi_acpi_match); 274 275 if (!!acpi_dev) { 276 put_device(acpi_dev); 277 } else if (!of_find_compatible_node(NULL, NULL, "google,cros-ec-ucsi")) { 278 retval = mfd_add_hotplug_devices(ec->dev, 279 cros_ec_ucsi_cells, 280 ARRAY_SIZE(cros_ec_ucsi_cells)); 281 if (retval) 282 dev_warn(ec->dev, 283 "failed to add cros_ec_ucsi: %d\n", 284 retval); 285 } 286 } 287 288 /* 289 * UCSI provides power supply information so we don't need to separately 290 * load the cros_usbpd_charger driver. 291 */ 292 if (cros_ec_check_features(ec, EC_FEATURE_USB_PD) && 293 !cros_ec_check_features(ec, EC_FEATURE_UCSI_PPM)) { 294 retval = mfd_add_hotplug_devices(ec->dev, 295 cros_usbpd_charger_cells, 296 ARRAY_SIZE(cros_usbpd_charger_cells)); 297 298 if (retval) 299 dev_warn(ec->dev, "failed to add usbpd-charger: %d\n", 300 retval); 301 } 302 303 /* 304 * Lightbar is a special case. Newer devices support autodetection, 305 * but older ones do not. 306 */ 307 if (cros_ec_check_features(ec, EC_FEATURE_LIGHTBAR) || 308 dmi_match(DMI_PRODUCT_NAME, "Link")) { 309 retval = mfd_add_hotplug_devices(ec->dev, 310 cros_ec_lightbar_cells, 311 ARRAY_SIZE(cros_ec_lightbar_cells)); 312 if (retval) 313 dev_warn(ec->dev, "failed to add lightbar: %d\n", 314 retval); 315 } 316 317 /* 318 * The PD notifier driver cell is separate since it only needs to be 319 * explicitly added on platforms that don't have the PD notifier ACPI 320 * device entry defined. 321 */ 322 if (IS_ENABLED(CONFIG_OF) && ec->ec_dev->dev->of_node) { 323 if (cros_ec_check_features(ec, EC_FEATURE_USB_PD)) { 324 retval = mfd_add_hotplug_devices(ec->dev, 325 cros_usbpd_notify_cells, 326 ARRAY_SIZE(cros_usbpd_notify_cells)); 327 if (retval) 328 dev_err(ec->dev, 329 "failed to add PD notify devices: %d\n", 330 retval); 331 } 332 } 333 334 /* 335 * The PCHG device cannot be detected by sending EC_FEATURE_GET_CMD, but 336 * it can be detected by querying the number of peripheral chargers. 337 */ 338 retval = cros_ec_cmd(ec->ec_dev, 0, EC_CMD_PCHG_COUNT, NULL, 0, 339 &pchg_count, sizeof(pchg_count)); 340 if (retval >= 0 && pchg_count.port_count) { 341 retval = mfd_add_hotplug_devices(ec->dev, 342 cros_ec_pchg_cells, 343 ARRAY_SIZE(cros_ec_pchg_cells)); 344 if (retval) 345 dev_warn(ec->dev, "failed to add pchg: %d\n", 346 retval); 347 } 348 349 /* 350 * The following subdevices cannot be detected by sending the 351 * EC_FEATURE_GET_CMD to the Embedded Controller device. 352 */ 353 retval = mfd_add_hotplug_devices(ec->dev, cros_ec_platform_cells, 354 ARRAY_SIZE(cros_ec_platform_cells)); 355 if (retval) 356 dev_warn(ec->dev, 357 "failed to add cros-ec platform devices: %d\n", 358 retval); 359 360 /* Check whether this EC instance has a VBC NVRAM */ 361 node = ec->ec_dev->dev->of_node; 362 if (of_property_read_bool(node, "google,has-vbc-nvram")) { 363 retval = mfd_add_hotplug_devices(ec->dev, cros_ec_vbc_cells, 364 ARRAY_SIZE(cros_ec_vbc_cells)); 365 if (retval) 366 dev_warn(ec->dev, "failed to add VBC devices: %d\n", 367 retval); 368 } 369 370 return 0; 371 372 failed: 373 put_device(&ec->class_dev); 374 return retval; 375 } 376
Hi Jameson, kernel test robot noticed the following build errors: [auto build test ERROR on 9fc83373f0ffb8834da48b1446a5c2fef9525bb1] url: https://github.com/intel-lab-lkp/linux/commits/Jameson-Thies/dt-bindings-Add-cros-ec-ucsi-to-cros-ec-typec-device-tree-documentation/20250313-040216 base: 9fc83373f0ffb8834da48b1446a5c2fef9525bb1 patch link: https://lore.kernel.org/r/20250312195951.1579682-4-jthies%40google.com patch subject: [PATCH v1 3/3] mfd: cros_ec: Don't add cros_ec_ucsi if it is defined in OF or ACPI config: x86_64-buildonly-randconfig-003-20250313 (https://download.01.org/0day-ci/archive/20250313/202503132357.RnlF5A0E-lkp@intel.com/config) compiler: clang version 19.1.7 (https://github.com/llvm/llvm-project cd708029e0b2869e80abe31ddb175f7c35361f90) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250313/202503132357.RnlF5A0E-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/202503132357.RnlF5A0E-lkp@intel.com/ All errors (new ones prefixed by >>): >> drivers/mfd/cros_ec_dev.c:273:12: error: incompatible function pointer types passing 'int (struct device *, void *)' to parameter of type 'device_match_t' (aka 'int (*)(struct device *, const void *)') [-Wincompatible-function-pointer-types] 273 | ucsi_acpi_match); | ^~~~~~~~~~~~~~~ include/linux/device.h:1101:21: note: passing argument to parameter 'match' here 1101 | device_match_t match); | ^ 1 error generated. vim +273 drivers/mfd/cros_ec_dev.c 181 182 static int ec_device_probe(struct platform_device *pdev) 183 { 184 int retval = -ENOMEM; 185 struct device_node *node; 186 struct device *dev = &pdev->dev; 187 struct cros_ec_platform *ec_platform = dev_get_platdata(dev); 188 struct cros_ec_dev *ec = kzalloc(sizeof(*ec), GFP_KERNEL); 189 struct ec_response_pchg_count pchg_count; 190 int i; 191 192 if (!ec) 193 return retval; 194 195 dev_set_drvdata(dev, ec); 196 ec->ec_dev = dev_get_drvdata(dev->parent); 197 ec->dev = dev; 198 ec->cmd_offset = ec_platform->cmd_offset; 199 ec->features.flags[0] = -1U; /* Not cached yet */ 200 ec->features.flags[1] = -1U; /* Not cached yet */ 201 device_initialize(&ec->class_dev); 202 203 for (i = 0; i < ARRAY_SIZE(cros_mcu_devices); i++) { 204 /* 205 * Check whether this is actually a dedicated MCU rather 206 * than an standard EC. 207 */ 208 if (cros_ec_check_features(ec, cros_mcu_devices[i].id)) { 209 dev_info(dev, "CrOS %s MCU detected\n", 210 cros_mcu_devices[i].desc); 211 /* 212 * Help userspace differentiating ECs from other MCU, 213 * regardless of the probing order. 214 */ 215 ec_platform->ec_name = cros_mcu_devices[i].name; 216 break; 217 } 218 } 219 220 /* 221 * Add the class device 222 */ 223 ec->class_dev.class = &cros_class; 224 ec->class_dev.parent = dev; 225 ec->class_dev.release = cros_ec_class_release; 226 227 retval = dev_set_name(&ec->class_dev, "%s", ec_platform->ec_name); 228 if (retval) { 229 dev_err(dev, "dev_set_name failed => %d\n", retval); 230 goto failed; 231 } 232 233 retval = device_add(&ec->class_dev); 234 if (retval) 235 goto failed; 236 237 /* check whether this EC is a sensor hub. */ 238 if (cros_ec_get_sensor_count(ec) > 0) { 239 retval = mfd_add_hotplug_devices(ec->dev, 240 cros_ec_sensorhub_cells, 241 ARRAY_SIZE(cros_ec_sensorhub_cells)); 242 if (retval) 243 dev_err(ec->dev, "failed to add %s subdevice: %d\n", 244 cros_ec_sensorhub_cells->name, retval); 245 } 246 247 /* 248 * The following subdevices can be detected by sending the 249 * EC_FEATURE_GET_CMD Embedded Controller device. 250 */ 251 for (i = 0; i < ARRAY_SIZE(cros_subdevices); i++) { 252 if (cros_ec_check_features(ec, cros_subdevices[i].id)) { 253 retval = mfd_add_hotplug_devices(ec->dev, 254 cros_subdevices[i].mfd_cells, 255 cros_subdevices[i].num_cells); 256 if (retval) 257 dev_err(ec->dev, 258 "failed to add %s subdevice: %d\n", 259 cros_subdevices[i].mfd_cells->name, 260 retval); 261 } 262 } 263 264 /* 265 * FW nodes can load cros_ec_ucsi, but early PDC devices did not define 266 * the required nodes. On PDC systems without FW nodes for cros_ec_ucsi, 267 * the driver should be added as an mfd subdevice. 268 */ 269 if (cros_ec_check_features(ec, EC_FEATURE_USB_PD) && 270 cros_ec_check_features(ec, EC_FEATURE_UCSI_PPM)) { 271 struct device *acpi_dev = device_find_child(ec->ec_dev->dev, 272 NULL, > 273 ucsi_acpi_match); 274 275 if (!!acpi_dev) { 276 put_device(acpi_dev); 277 } else if (!of_find_compatible_node(NULL, NULL, "google,cros-ec-ucsi")) { 278 retval = mfd_add_hotplug_devices(ec->dev, 279 cros_ec_ucsi_cells, 280 ARRAY_SIZE(cros_ec_ucsi_cells)); 281 if (retval) 282 dev_warn(ec->dev, 283 "failed to add cros_ec_ucsi: %d\n", 284 retval); 285 } 286 } 287 288 /* 289 * UCSI provides power supply information so we don't need to separately 290 * load the cros_usbpd_charger driver. 291 */ 292 if (cros_ec_check_features(ec, EC_FEATURE_USB_PD) && 293 !cros_ec_check_features(ec, EC_FEATURE_UCSI_PPM)) { 294 retval = mfd_add_hotplug_devices(ec->dev, 295 cros_usbpd_charger_cells, 296 ARRAY_SIZE(cros_usbpd_charger_cells)); 297 298 if (retval) 299 dev_warn(ec->dev, "failed to add usbpd-charger: %d\n", 300 retval); 301 } 302 303 /* 304 * Lightbar is a special case. Newer devices support autodetection, 305 * but older ones do not. 306 */ 307 if (cros_ec_check_features(ec, EC_FEATURE_LIGHTBAR) || 308 dmi_match(DMI_PRODUCT_NAME, "Link")) { 309 retval = mfd_add_hotplug_devices(ec->dev, 310 cros_ec_lightbar_cells, 311 ARRAY_SIZE(cros_ec_lightbar_cells)); 312 if (retval) 313 dev_warn(ec->dev, "failed to add lightbar: %d\n", 314 retval); 315 } 316 317 /* 318 * The PD notifier driver cell is separate since it only needs to be 319 * explicitly added on platforms that don't have the PD notifier ACPI 320 * device entry defined. 321 */ 322 if (IS_ENABLED(CONFIG_OF) && ec->ec_dev->dev->of_node) { 323 if (cros_ec_check_features(ec, EC_FEATURE_USB_PD)) { 324 retval = mfd_add_hotplug_devices(ec->dev, 325 cros_usbpd_notify_cells, 326 ARRAY_SIZE(cros_usbpd_notify_cells)); 327 if (retval) 328 dev_err(ec->dev, 329 "failed to add PD notify devices: %d\n", 330 retval); 331 } 332 } 333 334 /* 335 * The PCHG device cannot be detected by sending EC_FEATURE_GET_CMD, but 336 * it can be detected by querying the number of peripheral chargers. 337 */ 338 retval = cros_ec_cmd(ec->ec_dev, 0, EC_CMD_PCHG_COUNT, NULL, 0, 339 &pchg_count, sizeof(pchg_count)); 340 if (retval >= 0 && pchg_count.port_count) { 341 retval = mfd_add_hotplug_devices(ec->dev, 342 cros_ec_pchg_cells, 343 ARRAY_SIZE(cros_ec_pchg_cells)); 344 if (retval) 345 dev_warn(ec->dev, "failed to add pchg: %d\n", 346 retval); 347 } 348 349 /* 350 * The following subdevices cannot be detected by sending the 351 * EC_FEATURE_GET_CMD to the Embedded Controller device. 352 */ 353 retval = mfd_add_hotplug_devices(ec->dev, cros_ec_platform_cells, 354 ARRAY_SIZE(cros_ec_platform_cells)); 355 if (retval) 356 dev_warn(ec->dev, 357 "failed to add cros-ec platform devices: %d\n", 358 retval); 359 360 /* Check whether this EC instance has a VBC NVRAM */ 361 node = ec->ec_dev->dev->of_node; 362 if (of_property_read_bool(node, "google,has-vbc-nvram")) { 363 retval = mfd_add_hotplug_devices(ec->dev, cros_ec_vbc_cells, 364 ARRAY_SIZE(cros_ec_vbc_cells)); 365 if (retval) 366 dev_warn(ec->dev, "failed to add VBC devices: %d\n", 367 retval); 368 } 369 370 return 0; 371 372 failed: 373 put_device(&ec->class_dev); 374 return retval; 375 } 376
Echoing my response on patch 1/3, thank you for taking a look at this. I’ll follow up with a v2 series to address your comments and resolve the build issues. Additional responses below. > > devices will be created automatically. None of parent devices should > > ever check if the child exist to create a child - it makes no sense. > > This is still valid - none of parents should be poking around to see if > there is a child or not. The core handles it, DT handles it etc. > Understood, we can remove this for DT. We need to keep part of this check for ACPI because there are platforms which need this driver and haven't defined proper ACPI nodes in their FW to load cros_ec_ucsi. Thanks, Jameson
diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c index 9f84a52b48d6..5fb6cd20c5ec 100644 --- a/drivers/mfd/cros_ec_dev.c +++ b/drivers/mfd/cros_ec_dev.c @@ -5,6 +5,7 @@ * Copyright (C) 2014 Google, Inc. */ +#include <linux/acpi.h> #include <linux/dmi.h> #include <linux/kconfig.h> #include <linux/mfd/core.h> @@ -128,11 +129,6 @@ static const struct cros_feature_to_cells cros_subdevices[] = { .mfd_cells = cros_ec_rtc_cells, .num_cells = ARRAY_SIZE(cros_ec_rtc_cells), }, - { - .id = EC_FEATURE_UCSI_PPM, - .mfd_cells = cros_ec_ucsi_cells, - .num_cells = ARRAY_SIZE(cros_ec_ucsi_cells), - }, { .id = EC_FEATURE_HANG_DETECT, .mfd_cells = cros_ec_wdt_cells, @@ -169,6 +165,15 @@ static const struct mfd_cell cros_ec_vbc_cells[] = { { .name = "cros-ec-vbc", } }; + +static int ucsi_acpi_match(struct device *dev, void *data) +{ + struct acpi_device_id ucsi_acpi_device_ids[] = { + { "GOOG0021", 0 }, + }; + return !!acpi_match_device(ucsi_acpi_device_ids, dev); +} + static void cros_ec_class_release(struct device *dev) { kfree(to_cros_ec_dev(dev)); @@ -256,6 +261,30 @@ static int ec_device_probe(struct platform_device *pdev) } } + /* + * FW nodes can load cros_ec_ucsi, but early PDC devices did not define + * the required nodes. On PDC systems without FW nodes for cros_ec_ucsi, + * the driver should be added as an mfd subdevice. + */ + if (cros_ec_check_features(ec, EC_FEATURE_USB_PD) && + cros_ec_check_features(ec, EC_FEATURE_UCSI_PPM)) { + struct device *acpi_dev = device_find_child(ec->ec_dev->dev, + NULL, + ucsi_acpi_match); + + if (!!acpi_dev) { + put_device(acpi_dev); + } else if (!of_find_compatible_node(NULL, NULL, "google,cros-ec-ucsi")) { + retval = mfd_add_hotplug_devices(ec->dev, + cros_ec_ucsi_cells, + ARRAY_SIZE(cros_ec_ucsi_cells)); + if (retval) + dev_warn(ec->dev, + "failed to add cros_ec_ucsi: %d\n", + retval); + } + } + /* * UCSI provides power supply information so we don't need to separately * load the cros_usbpd_charger driver.
Check for cros_ec_ucsi to be defined in the OF device tree or an ACPI node. If it is defined by either OF or ACPI, it does not need to be added as a subdevice of cros_ec_dev. Signed-off-by: Jameson Thies <jthies@google.com> --- drivers/mfd/cros_ec_dev.c | 39 ++++++++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-)