Message-ID: <a9db708f-59a6-586a-8728-81622a93ef3e@redhat.com>
Date:   Wed, 18 Jan 2023 11:52:48 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Kai-Heng Feng <kai.heng.feng@...onical.com>
Cc:     ktsai@...ellamicro.com, jic23@...nel.org, lars@...afoo.de,
        Wahaj <wahajaved@...tonmail.com>, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: light: cm32181: Fix PM support on system with 2 I2C
 resources

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@...onical.com> wrote:
>>
>> Hi Hans,
>>
>> On Wed, Jan 18, 2023 at 1:21 AM Hans de Goede <hdegoede@...hat.com> 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@...tonmail.com>
>>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@...onical.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.
>>
>> 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]);
>>>
> 

Powered by blists - more mailing lists