[PATCH] iio: accel: mma8452: remove the reset operation during driver probe

[PATCH] iio: accel: mma8452: remove the reset operation during driver probe

[haibo.chen]

From: Haibo Chen <haibo.chen@nxp.com>

Though Sensor Datasheet define this reset bit in it's CTRL_REG2
register, but seems the actual hardware behavior do not align with
the doc expect. Once the reset bit is set, sensor can’t give back
an I2C ack, which will cause the probe fail. So just remove this
reset operation.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/iio/accel/mma8452.c | 28 ----------------------------
 1 file changed, 28 deletions(-)

diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
index 0016bb947c10..ec9e26fdfb2a 100644
--- a/drivers/iio/accel/mma8452.c
+++ b/drivers/iio/accel/mma8452.c
@@ -1481,30 +1481,6 @@ static void mma8452_trigger_cleanup(struct iio_dev *indio_dev)
 		iio_trigger_unregister(indio_dev->trig);
 }
 
-static int mma8452_reset(struct i2c_client *client)
-{
-	int i;
-	int ret;
-
-	ret = i2c_smbus_write_byte_data(client,	MMA8452_CTRL_REG2,
-					MMA8452_CTRL_REG2_RST);
-	if (ret < 0)
-		return ret;
-
-	for (i = 0; i < 10; i++) {
-		usleep_range(100, 200);
-		ret = i2c_smbus_read_byte_data(client, MMA8452_CTRL_REG2);
-		if (ret == -EIO)
-			continue; /* I2C comm reset */
-		if (ret < 0)
-			return ret;
-		if (!(ret & MMA8452_CTRL_REG2_RST))
-			return 0;
-	}
-
-	return -ETIMEDOUT;
-}
-
 static const struct of_device_id mma8452_dt_ids[] = {
 	{ .compatible = "fsl,mma8451", .data = &mma_chip_info_table[mma8451] },
 	{ .compatible = "fsl,mma8452", .data = &mma_chip_info_table[mma8452] },
@@ -1591,10 +1567,6 @@ static int mma8452_probe(struct i2c_client *client,
 	indio_dev->num_channels = data->chip_info->num_channels;
 	indio_dev->available_scan_masks = mma8452_scan_masks;
 
-	ret = mma8452_reset(client);
-	if (ret < 0)
-		goto disable_regulators;
-
 	data->data_cfg = MMA8452_DATA_CFG_FS_2G;
 	ret = i2c_smbus_write_byte_data(client, MMA8452_DATA_CFG,
 					data->data_cfg);
-- 
2.25.1

Re: [PATCH] iio: accel: mma8452: remove the reset operation during driver probe

[Jonathan Cameron]

On Tue, 22 Feb 2022 12:45:51 +0800
<haibo.chen@nxp.com> wrote:

> From: Haibo Chen <haibo.chen@nxp.com>
> 
> Though Sensor Datasheet define this reset bit in it's CTRL_REG2
> register, but seems the actual hardware behavior do not align with
> the doc expect. Once the reset bit is set, sensor can’t give back
> an I2C ack, which will cause the probe fail. So just remove this
> reset operation.
> 
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>

Hi Haibo,

I'm not really that keen to remove reset on probe as it's normally
a good way to ensure we get a device into a sane state as we have no
idea what has run before we load the driver.

Wolfram is there a standard way to work around missing ACK in cases
like this?  Would just ignoring the return value be fine or are their
i2c masters that will get stuck if they don't get the expected ack?

Jonathan

> ---
>  drivers/iio/accel/mma8452.c | 28 ----------------------------
>  1 file changed, 28 deletions(-)
> 
> diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> index 0016bb947c10..ec9e26fdfb2a 100644
> --- a/drivers/iio/accel/mma8452.c
> +++ b/drivers/iio/accel/mma8452.c
> @@ -1481,30 +1481,6 @@ static void mma8452_trigger_cleanup(struct iio_dev *indio_dev)
>  		iio_trigger_unregister(indio_dev->trig);
>  }
>  
> -static int mma8452_reset(struct i2c_client *client)
> -{
> -	int i;
> -	int ret;
> -
> -	ret = i2c_smbus_write_byte_data(client,	MMA8452_CTRL_REG2,
> -					MMA8452_CTRL_REG2_RST);
> -	if (ret < 0)
> -		return ret;
> -
> -	for (i = 0; i < 10; i++) {
> -		usleep_range(100, 200);
> -		ret = i2c_smbus_read_byte_data(client, MMA8452_CTRL_REG2);
> -		if (ret == -EIO)
> -			continue; /* I2C comm reset */
> -		if (ret < 0)
> -			return ret;
> -		if (!(ret & MMA8452_CTRL_REG2_RST))
> -			return 0;
> -	}
> -
> -	return -ETIMEDOUT;
> -}
> -
>  static const struct of_device_id mma8452_dt_ids[] = {
>  	{ .compatible = "fsl,mma8451", .data = &mma_chip_info_table[mma8451] },
>  	{ .compatible = "fsl,mma8452", .data = &mma_chip_info_table[mma8452] },
> @@ -1591,10 +1567,6 @@ static int mma8452_probe(struct i2c_client *client,
>  	indio_dev->num_channels = data->chip_info->num_channels;
>  	indio_dev->available_scan_masks = mma8452_scan_masks;
>  
> -	ret = mma8452_reset(client);
> -	if (ret < 0)
> -		goto disable_regulators;
> -
>  	data->data_cfg = MMA8452_DATA_CFG_FS_2G;
>  	ret = i2c_smbus_write_byte_data(client, MMA8452_DATA_CFG,
>  					data->data_cfg);

RE: [PATCH] iio: accel: mma8452: remove the reset operation during driver probe

[Bough Chen]

[-- Attachment #1: Type: text/plain, Size: 3396 bytes --]

> -----Original Message-----
> From: Jonathan Cameron [mailto:Jonathan.Cameron@Huawei.com]
> Sent: 2022年2月23日 0:44
> To: Bough Chen <haibo.chen@nxp.com>
> Cc: jic23@kernel.org; lars@metafoo.de; linux-iio@vger.kernel.org;
> pmeerw@pmeerw.net; dl-linux-imx <linux-imx@nxp.com>; Wolfram Sang
> <wsa@kernel.org>
> Subject: Re: [PATCH] iio: accel: mma8452: remove the reset operation
> during driver probe
> 
> On Tue, 22 Feb 2022 12:45:51 +0800
> <haibo.chen@nxp.com> wrote:
> 
> > From: Haibo Chen <haibo.chen@nxp.com>
> >
> > Though Sensor Datasheet define this reset bit in it's CTRL_REG2
> > register, but seems the actual hardware behavior do not align with the
> > doc expect. Once the reset bit is set, sensor can’t give back an I2C
> > ack, which will cause the probe fail. So just remove this reset
> > operation.
> >
> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> 
> Hi Haibo,
> 
> I'm not really that keen to remove reset on probe as it's normally a good
> way to ensure we get a device into a sane state as we have no idea what has
> run before we load the driver.
> 
> Wolfram is there a standard way to work around missing ACK in cases like
> this?  Would just ignoring the return value be fine or are their i2c masters
> that will get stuck if they don't get the expected ack?

Currently, i2c masters will not get stuck. Only the sensor driver probe failed.
Let me do more test about this, currently I find this issue on fxls8471/mma8452, and this driver cover many sensors
Not sure whether other sensors has the same behavior.


Best Regards
Haibo chen
> 
> Jonathan
> 
> > ---
> >  drivers/iio/accel/mma8452.c | 28 ----------------------------
> >  1 file changed, 28 deletions(-)
> >
> > diff --git a/drivers/iio/accel/mma8452.c b/drivers/iio/accel/mma8452.c
> > index 0016bb947c10..ec9e26fdfb2a 100644
> > --- a/drivers/iio/accel/mma8452.c
> > +++ b/drivers/iio/accel/mma8452.c
> > @@ -1481,30 +1481,6 @@ static void mma8452_trigger_cleanup(struct
> iio_dev *indio_dev)
> >  		iio_trigger_unregister(indio_dev->trig);
> >  }
> >
> > -static int mma8452_reset(struct i2c_client *client) -{
> > -	int i;
> > -	int ret;
> > -
> > -	ret = i2c_smbus_write_byte_data(client,	MMA8452_CTRL_REG2,
> > -					MMA8452_CTRL_REG2_RST);
> > -	if (ret < 0)
> > -		return ret;
> > -
> > -	for (i = 0; i < 10; i++) {
> > -		usleep_range(100, 200);
> > -		ret = i2c_smbus_read_byte_data(client, MMA8452_CTRL_REG2);
> > -		if (ret == -EIO)
> > -			continue; /* I2C comm reset */
> > -		if (ret < 0)
> > -			return ret;
> > -		if (!(ret & MMA8452_CTRL_REG2_RST))
> > -			return 0;
> > -	}
> > -
> > -	return -ETIMEDOUT;
> > -}
> > -
> >  static const struct of_device_id mma8452_dt_ids[] = {
> >  	{ .compatible = "fsl,mma8451", .data =
> &mma_chip_info_table[mma8451] },
> >  	{ .compatible = "fsl,mma8452", .data =
> &mma_chip_info_table[mma8452]
> > }, @@ -1591,10 +1567,6 @@ static int mma8452_probe(struct i2c_client
> *client,
> >  	indio_dev->num_channels = data->chip_info->num_channels;
> >  	indio_dev->available_scan_masks = mma8452_scan_masks;
> >
> > -	ret = mma8452_reset(client);
> > -	if (ret < 0)
> > -		goto disable_regulators;
> > -
> >  	data->data_cfg = MMA8452_DATA_CFG_FS_2G;
> >  	ret = i2c_smbus_write_byte_data(client, MMA8452_DATA_CFG,
> >  					data->data_cfg);


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 9551 bytes --]

Re: [PATCH] iio: accel: mma8452: remove the reset operation during driver probe

[Wolfram Sang]

> > Wolfram is there a standard way to work around missing ACK in cases like
> > this?  Would just ignoring the return value be fine or are their i2c masters
> > that will get stuck if they don't get the expected ack?

Did I get this right: the reset procedures terminates the ACK and STOP?
And the client expects a new START condition for communication?

Re: [PATCH] iio: accel: mma8452: remove the reset operation during driver probe

[Jonathan Cameron]

On Thu, 24 Feb 2022 18:33:26 +0100
Wolfram Sang <wsa@kernel.org> wrote:

> > > Wolfram is there a standard way to work around missing ACK in cases like
> > > this?  Would just ignoring the return value be fine or are their i2c masters
> > > that will get stuck if they don't get the expected ack?  
> 
> Did I get this right: the reset procedures terminates the ACK and STOP?
> And the client expects a new START condition for communication?
> 

@Bough Chen,

I'm assuming this is still an issue for you?  If so can you reply to
Wolfram so we can hopefully move this forwards.

Found this because it's still listed as needing an action in the IIO
patchwork.

Thanks,

Jonathan

RE: [PATCH] iio: accel: mma8452: remove the reset operation during driver probe

[Bough Chen]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: 2022年6月5日 0:17
> To: Wolfram Sang <wsa@kernel.org>
> Cc: Bough Chen <haibo.chen@nxp.com>; Jonathan Cameron
> <Jonathan.Cameron@huawei.com>; lars@metafoo.de;
> linux-iio@vger.kernel.org; pmeerw@pmeerw.net; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH] iio: accel: mma8452: remove the reset operation during
> driver probe
> 
> On Thu, 24 Feb 2022 18:33:26 +0100
> Wolfram Sang <wsa@kernel.org> wrote:
> 
> > > > Wolfram is there a standard way to work around missing ACK in
> > > > cases like this?  Would just ignoring the return value be fine or
> > > > are their i2c masters that will get stuck if they don't get the expected ack?
> >
> > Did I get this right: the reset procedures terminates the ACK and STOP?
> > And the client expects a new START condition for communication?
> >
> 
> @Bough Chen,
> 
> I'm assuming this is still an issue for you?  If so can you reply to Wolfram so we
> can hopefully move this forwards.
> 
> Found this because it's still listed as needing an action in the IIO patchwork.

Hi Jonathan,

Give me few days to switch my current work to this patch work.

Thanks
Bough Chen
> 
> Thanks,
> 
> Jonathan

RE: [PATCH] iio: accel: mma8452: remove the reset operation during driver probe

[Bough Chen]

> -----Original Message-----
> From: Jonathan Cameron <jic23@kernel.org>
> Sent: 2022年6月5日 0:17
> To: Wolfram Sang <wsa@kernel.org>
> Cc: Bough Chen <haibo.chen@nxp.com>; Jonathan Cameron
> <Jonathan.Cameron@huawei.com>; lars@metafoo.de;
> linux-iio@vger.kernel.org; pmeerw@pmeerw.net; dl-linux-imx
> <linux-imx@nxp.com>
> Subject: Re: [PATCH] iio: accel: mma8452: remove the reset operation during
> driver probe
> 
> On Thu, 24 Feb 2022 18:33:26 +0100
> Wolfram Sang <wsa@kernel.org> wrote:
> 
> > > > Wolfram is there a standard way to work around missing ACK in
> > > > cases like this?  Would just ignoring the return value be fine or
> > > > are their i2c masters that will get stuck if they don't get the expected ack?
> >
> > Did I get this right: the reset procedures terminates the ACK and STOP?
> > And the client expects a new START condition for communication?
> >
> 
Hi

I use a I2C logic analyzer and find this reset just terminate ACK, and I2C bus controller then detect this NACK, and give a STOP. After that, I2C bus work normal.
If just ignore this return, everything continue will be fine. 
Seems for this sensor, after the reset bit is set, it works immediately, so will not give ACK.

I will send a v2 patch, just ignore this return rather than remove the whole reset operation.

Best Regards
Bough Chen

> @Bough Chen,
> 
> I'm assuming this is still an issue for you?  If so can you reply to Wolfram so we
> can hopefully move this forwards.
> 
> Found this because it's still listed as needing an action in the IIO patchwork.
> 
> Thanks,
> 
> Jonathan