On Mon, Nov 08, 2021 at 02:12:38PM +0100, Hans de Goede wrote:
> On 11/2/21 00:43, Daniel Scally wrote:
> Does this sound reasonable / like I'm heading in the right direction?
It is up to you folks, since I have no time to participate in this with
a full dive right now. Below just some comments on the patches in case
they will go.
...
> - struct acpi_device *adev = ACPI_COMPANION(dev);
> + struct acpi_device *adev = to_acpi_device_node(fwnode);
> struct i2c_acpi_lookup lookup;
> struct i2c_adapter *adapter;
> LIST_HEAD(resource_list);
> int ret;
Make sense to move assignment here.
adev = to_acpi_device_node(fwnode);
> + if (!adev)
> + return ERR_PTR(-ENODEV);
...
> +static inline struct i2c_client *i2c_acpi_new_device(struct device *dev,
> + int index,
> + struct i2c_board_info *info)
> +{
> + return i2c_acpi_new_device_by_fwnode(dev->fwnode, index, info);
dev_fwnode(dev)
> +}
...
> +int cio2_bridge_sensors_are_ready(void)
> +{
> + struct acpi_device *adev;
> + bool ready = true;
Redundant. See below.
> + unsigned int i;
> +
> + for (i = 0; i < ARRAY_SIZE(cio2_supported_sensors); i++) {
> + const struct cio2_sensor_config *cfg =
> + &cio2_supported_sensors[i];
> +
> + for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
> + if (!adev->status.enabled)
> + continue;
> + if (!acpi_dev_ready_for_enumeration(adev))
> + ready = false;
You may put the adev here and return false.
> + }
> + }
> + return ready;
So return true.
> +}
...
> + if (sensor->ssdb.vcmtype)
> + nodes[SWNODE_VCM] = NODE_VCM(
> + cio2_vcm_types[sensor->ssdb.vcmtype - 1]);
Wouldn't be better
nodes[SWNODE_VCM] =
NODE_VCM(cio2_vcm_types[sensor->ssdb.vcmtype - 1]);
?
...
> + sensor->vcm_i2c_client = i2c_acpi_new_device_by_fwnode(
> + acpi_fwnode_handle(sensor->adev),
> + 1, &board_info);
Ditto.
--
With Best Regards,
Andy Shevchenko
