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;
Date:   Sat, 21 Jan 2023 17:51:20 +0000
From:   Jonathan Cameron <jic23@kernel.org>
To:     Hans de Goede <hdegoede@redhat.com>
Cc:     Kai-Heng Feng <kai.heng.feng@canonical.com>,
        ktsai@capellamicro.com, lars@metafoo.de,
        Wahaj <wahajaved@protonmail.com>, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] iio: light: cm32181: Fix PM support on system with 2
 I2C resources
Message-ID: <20230121175120.526d3d16@jic23-huawei>
In-Reply-To: <3d51f766-bd80-870d-c939-99660526c654@redhat.com>
References: <20230118170422.339619-1-kai.heng.feng@canonical.com>
        <3d51f766-bd80-870d-c939-99660526c654@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 18 Jan 2023 18:07:45 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi,
> 
> On 1/18/23 18:04, 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 I2C
> > 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, use the I2C client
> > stored in the cm32181 private struct so the PM ops can get the correct
> > I2C client to really talk to the sensor device.
> > 
> > Fixes: 68c1b3dd5c48 ("iio: light: cm32181: Add PM support")
> > BugLink: https://bugs.launchpad.net/bugs/1988346
> > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2152281
> > Tested-by: Wahaj <wahajaved@protonmail.com>
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>  
> 
> Thanks, patch looks good to me:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> 
> Regards,
> 
> Hans
Applied to the fixes-togreg branch of iio.git and marked for stable.

Thanks

Jonathan

> 
> 
> 
> > ---
> > v2:
> >  - Removed setting drvdata to the dummy client.
> >  - Added bug links.
> >  - Wording.
> > 
> >  drivers/iio/light/cm32181.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iio/light/cm32181.c b/drivers/iio/light/cm32181.c
> > index 001055d097509..b1674a5bfa368 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);
> > +
> >  	/*
> >  	 * Some ACPI systems list 2 I2C resources for the CM3218 sensor, the
> >  	 * SMBus Alert Response Address (ARA, 0x0c) and the actual I2C address.
> > @@ -460,8 +462,6 @@ static int cm32181_probe(struct i2c_client *client)
> >  			return PTR_ERR(client);
> >  	}
> >  
> > -	i2c_set_clientdata(client, indio_dev);
> > -
> >  	cm32181 = iio_priv(indio_dev);
> >  	cm32181->client = client;
> >  	cm32181->dev = dev;
> > @@ -490,7 +490,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 +499,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]);  
>