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.
But I do wonder what happens for the removing case? Will the second
i2c_client leak?
Kai-Heng
>
> 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]);
>
