Thanks Rafael. This looks mostly good but I have a doubt on the error
handling, see below.
> +static int sdw_acpi_check_duplicate(struct acpi_device *adev, void *data)
> +{
> + struct sdw_acpi_child_walk_data *cwd = data;
> + struct sdw_bus *bus = cwd->bus;
> + struct sdw_slave_id id;
> +
> + if (adev == cwd->adev)
> + return 0;
> +
> + if (!find_slave(bus, adev, &id))
> + return 0;
> +
> + if (cwd->id.sdw_version != id.sdw_version || cwd->id.mfg_id != id.mfg_id ||
> + cwd->id.part_id != id.part_id || cwd->id.class_id != id.class_id)
> + return 0;
> +
> + if (cwd->id.unique_id != id.unique_id) {
> + dev_dbg(bus->dev,
> + "Valid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
> + cwd->id.unique_id, id.unique_id, cwd->id.mfg_id,
> + cwd->id.part_id);
> + cwd->ignore_unique_id = false;
> + return 0;
> + }
> +
> + dev_err(bus->dev,
> + "Invalid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
> + cwd->id.unique_id, id.unique_id, cwd->id.mfg_id, cwd->id.part_id);
> + return -ENODEV;
if this error happens, I would guess it's reported ....
> +}
> +
> +static int sdw_acpi_find_one(struct acpi_device *adev, void *data)
> +{
> + struct sdw_bus *bus = data;
> + struct sdw_acpi_child_walk_data cwd = {
> + .bus = bus,
> + .adev = adev,
> + .ignore_unique_id = true,
> + };
> + int ret;
> +
> + if (!find_slave(bus, adev, &cwd.id))
> + return 0;
> +
> + /* Brute-force O(N^2) search for duplicates. */
> + ret = acpi_dev_for_each_child(ACPI_COMPANION(bus->dev),
> + sdw_acpi_check_duplicate, &cwd);
> + if (ret)
> + return ret;
... here, but I don't see this being propagated further...
> +
> + if (cwd.ignore_unique_id)
> + cwd.id.unique_id = SDW_IGNORED_UNIQUE_ID;
> +
> + /* Ignore errors and continue. */
> + sdw_slave_add(bus, &cwd.id, acpi_fwnode_handle(adev));
> + return 0;
> +}
> +
> /*
> * sdw_acpi_find_slaves() - Find Slave devices in Master ACPI node
> * @bus: SDW bus instance
> @@ -135,8 +200,7 @@ static bool find_slave(struct sdw_bus *b
> */
> int sdw_acpi_find_slaves(struct sdw_bus *bus)
> {
> - struct acpi_device *adev, *parent;
> - struct acpi_device *adev2, *parent2;
> + struct acpi_device *parent;
>
> parent = ACPI_COMPANION(bus->dev);
> if (!parent) {
> @@ -144,52 +208,7 @@ int sdw_acpi_find_slaves(struct sdw_bus
> return -ENODEV;
> }
>
> - list_for_each_entry(adev, &parent->children, node) {
> - struct sdw_slave_id id;
> - struct sdw_slave_id id2;
> - bool ignore_unique_id = true;
> -
> - if (!find_slave(bus, adev, &id))
> - continue;
> -
> - /* brute-force O(N^2) search for duplicates */
> - parent2 = parent;
> - list_for_each_entry(adev2, &parent2->children, node) {
> -
> - if (adev == adev2)
> - continue;
> -
> - if (!find_slave(bus, adev2, &id2))
> - continue;
> -
> - if (id.sdw_version != id2.sdw_version ||
> - id.mfg_id != id2.mfg_id ||
> - id.part_id != id2.part_id ||
> - id.class_id != id2.class_id)
> - continue;
> -
> - if (id.unique_id != id2.unique_id) {
> - dev_dbg(bus->dev,
> - "Valid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
> - id.unique_id, id2.unique_id, id.mfg_id, id.part_id);
> - ignore_unique_id = false;
> - } else {
> - dev_err(bus->dev,
> - "Invalid unique IDs 0x%x 0x%x for Slave mfg_id 0x%04x, part_id 0x%04x\n",
> - id.unique_id, id2.unique_id, id.mfg_id, id.part_id);
> - return -ENODEV;
> - }
> - }
> -
> - if (ignore_unique_id)
> - id.unique_id = SDW_IGNORED_UNIQUE_ID;
> -
> - /*
> - * don't error check for sdw_slave_add as we want to continue
> - * adding Slaves
> - */
> - sdw_slave_add(bus, &id, acpi_fwnode_handle(adev));
> - }
> + acpi_dev_for_each_child(parent, sdw_acpi_find_one, bus);
... here?
It looks like a change in the error handling flow where
sdw_acpi_find_slaves() is now returning 0 (success) always?
Shouldn't the return of sdw_acpi_find_one() be trapped, e.g. with
return acpi_dev_for_each_child(parent, sdw_acpi_find_one, bus);
>
> return 0;
> }
>
>
>
