Hi,
On 1/18/23 06:15, Kai-Heng Feng wrote:
> On Wed, Jan 18, 2023 at 11:29 AM Kai-Heng Feng
> <kai.heng.feng@xxxxxxxxxxxxx> wrote:
>>
>> Hi Hans,
>>
>> On Wed, Jan 18, 2023 at 1:21 AM Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxxxxxxx>
>>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
>>>
>>> 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.
>>
>> The intention is to save indio_dev as drvdata in the first (i.e.
>> original) i2c_client's dev.
>>
>>>
>>>> /*
>>>> * 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.
>>
>> Doesn't i2c_acpi_new_device() creates a new i2c_client (and its dev embedded)?
>>
>> So the intention is to save indio_dev for the second (ARA case) i2c_client too.
>>
>>>
>>>>
>>>> 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.
>>
>> Yes, it seems like those 2 hunks are not necessary. Let me send a new patch.
>
> if (ACPI_HANDLE(dev) && client->addr == SMBUS_ALERT_RESPONSE_ADDRESS) {
> ...
> client = i2c_acpi_new_device(dev, 1, &board_info);
> ...
> }
> i2c_set_clientdata(client, indio_dev);
>
> It means the indio_dev is only assigned to the new i2c_client->dev's
> drvdata, the original dev's drvdata remains NULL.
> So we need to assign it before the original client gets replaced by
> the new one, otherwise we can't get cm32181 in PM ops.
You are right, my bad. The original code has a bug where it indeed was
making the i2c_set_clientdata() call on the wrong client device.
So the i2c_set_clientdata() call needs to be moved up.
There is no need to also call i2c_set_clientdata() on the dummy
i2c-client though. That one does not have a driver attached.
The suspend/resume callbacks are made on the original client-dev,
not on the one of the dummy-client (which is the one which we
actually use to communicate).
>> But I do wonder what happens for the removing case? Will the second
>> i2c_client leak?
Yes it does, good point. That should probably also be fixed, but
that needs to be a different / second patch.
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]);
>>>
>
