Hi,
On 11/8/21 15:12, Andy Shevchenko wrote:
> 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.
Ack, will fix.
>
> 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)
Ack, will fix.
>
>> +}
>
> ...
>
>> +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.
I actually did it this way deliberately making use of
for_each_acpi_dev_match() not "leaking" a ref when you let
it run to the end.
I find this clearer because this way all the ref handling
is abstracted away in for_each_acpi_dev_match(), where as with
a put in the middle of the loop a causal reader of the code
is going to wonder there the put ref is coming from.
>
>> +}
>
> ...
>
>> + 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.
Ack, will fix both for the next version.
Regards,
Hans
