Re: Fwd: Surface Go VCM type (was: Need to pass acpi_enforce_resources=lax on the Surface Go (version1))

[Hans de Goede <hdegoede@redhat.com>]

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