Re: [PATCH] iio: light: cm32181: Fix PM support on system with 2 I2C resources

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

Hi,

On 1/17/23 18:19, Hans de Goede wrote:
> Hi,
> 
> On 1/17/23 17:09, Kai-Heng Feng wrote:
>> Commit c1e62062ff54 ("iio: light: cm32181: Handle CM3218 ACPI devices
>> with 2 I2C resources") creates a second client for the actual I2C
>> address, but the "struct device" passed to PM ops is the first client
>> that can't talk to the sensor.
>>
>> That means the I2C transfers in both suspend and resume routines can
>> fail and blocking the whole suspend process.
>>
>> Instead of using the first client for I2C transfer, store the cm32181
>> private struct on both cases so the PM ops can get the correct I2C
>> client to perfrom suspend and resume.
>>
>> Fixes: 68c1b3dd5c48 ("iio: light: cm32181: Add PM support")
>> Tested-by: Wahaj <wahajaved@protonmail.com>
>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> 
> Thank you for this fix. I had looking into this on my todo list,
> since I have been seeing some bug reports about this too.
> 
> One remark inline:
> 
>> ---
>>  drivers/iio/light/cm32181.c | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
>> index 001055d097509..0f319c891353c 100644
>> --- a/drivers/iio/light/cm32181.c
>> +++ b/drivers/iio/light/cm32181.c
>> @@ -440,6 +440,8 @@ static int cm32181_probe(struct i2c_client *client)
>>  	if (!indio_dev)
>>  		return -ENOMEM;
>>  
>> +	i2c_set_clientdata(client, indio_dev);
>> +
> 
> Why move this up, the suspend/resume callbacks cannot run until
> probe() completes, so no need for this change.
> 
>>  	/*
>>  	 * Some ACPI systems list 2 I2C resources for the CM3218 sensor, the
>>  	 * SMBus Alert Response Address (ARA, 0x0c) and the actual I2C address.
>> @@ -458,9 +460,9 @@ static int cm32181_probe(struct i2c_client *client)
>>  		client = i2c_acpi_new_device(dev, 1, &board_info);
>>  		if (IS_ERR(client))
>>  			return PTR_ERR(client);
>> -	}
>>  
>> -	i2c_set_clientdata(client, indio_dev);
>> +		i2c_set_clientdata(client, indio_dev);
>> +	}
> 
> And moving it inside the if block here (instead of just dropping it)
> is also weird. I guess you meant to just delete it since you moved it up.
> 
>>  
>>  	cm32181 = iio_priv(indio_dev);
>>  	cm32181->client = client;
> 
> Also note that the ->client used in suspend/resume now is not set until
> here, so moving the i2c_set_clientdata() up really does not do anything.
> 
> I beleive it would be best to just these 2 hunks from the patch and
> only keep the changes to the suspend/resume callbacks.

p.s.

I believe that this will likely also fix:
https://bugzilla.redhat.com/show_bug.cgi?id=2152281

Can you please add a:

Link: https://bugzilla.redhat.com/show_bug.cgi?id=2152281

to the next version.

Regards,

Hans



> 
> 
>> @@ -490,7 +492,8 @@ static int cm32181_probe(struct i2c_client *client)
>>  
>>  static int cm32181_suspend(struct device *dev)
>>  {
>> -	struct i2c_client *client = to_i2c_client(dev);
>> +	struct cm32181_chip *cm32181 = iio_priv(dev_get_drvdata(dev));
>> +	struct i2c_client *client = cm32181->client;
>>  
>>  	return i2c_smbus_write_word_data(client, CM32181_REG_ADDR_CMD,
>>  					 CM32181_CMD_ALS_DISABLE);
>> @@ -498,8 +501,8 @@ static int cm32181_suspend(struct device *dev)
>>  
>>  static int cm32181_resume(struct device *dev)
>>  {
>> -	struct i2c_client *client = to_i2c_client(dev);
>>  	struct cm32181_chip *cm32181 = iio_priv(dev_get_drvdata(dev));
>> +	struct i2c_client *client = cm32181->client;
>>  
>>  	return i2c_smbus_write_word_data(client, CM32181_REG_ADDR_CMD,
>>  					 cm32181->conf_regs[CM32181_REG_ADDR_CMD]);
>