Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230117160951.282581-1-kai.heng.feng@canonical.com>
 <5c95d25b-ff26-053b-efc8-5f6fd979c7e2@redhat.com> <CAAd53p5DFUMjMNAyp6YVONwCpGs8rRVORj0=OSgj+Z0f3QpeCg@mail.gmail.com>
In-Reply-To: <CAAd53p5DFUMjMNAyp6YVONwCpGs8rRVORj0=OSgj+Z0f3QpeCg@mail.gmail.com>
From:   Kai-Heng Feng <kai.heng.feng@canonical.com>
Date:   Wed, 18 Jan 2023 13:15:05 +0800
Message-ID: <CAAd53p5XXCniBN7x4uhp4XW=qr2U72_UntgAR0BV2viRtd+8EA@mail.gmail.com>
Subject: Re: [PATCH] iio: light: cm32181: Fix PM support on system with 2 I2C resources
To:     Hans de Goede <hdegoede@redhat.com>
Cc:     ktsai@capellamicro.com, jic23@kernel.org, lars@metafoo.de,
        Wahaj <wahajaved@protonmail.com>, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jan 18, 2023 at 11:29 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> Hi Hans,
>
> On Wed, Jan 18, 2023 at 1:21 AM Hans de Goede <hdegoede@redhat.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@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.
>
> 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.

Kai-Heng

>
> 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]);
> >