[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5c95d25b-ff26-053b-efc8-5f6fd979c7e2@redhat.com>
Date: Tue, 17 Jan 2023 18:19:19 +0100
From: Hans de Goede <hdegoede@...hat.com>
To: Kai-Heng Feng <kai.heng.feng@...onical.com>,
ktsai@...ellamicro.com, jic23@...nel.org, lars@...afoo.de
Cc: 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/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.
> /*
> * 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.
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