[RFC 0/2] iio: accel: mxc4005: IRQ fixes + reset chip on probe()

[RFC 0/2] iio: accel: mxc4005: IRQ fixes + reset chip on probe()

[Hans de Goede]

Hi All,

As reported here:
https://bugzilla.kernel.org/show_bug.cgi?id=218578

The MXC6655 found on several Chuwi tablets models works sometimes instead
of all the time. The problem seems to be that the power-sequencing done
on the board causes the chip to not reliably reset leaving it in a random
state at boot (and after suspend/resume).

The second patch in this set fixes this by using the sw-reset feature to
explicitly reset the chip on probe() and resume().

While working on this I also noticed an issue with the interrupt mask
handling, this is fixed in the first patch of the set.

This is marked as a RFC for now because this is untested atm. I'll
provide a test kernel to the reporter of:
https://bugzilla.kernel.org/show_bug.cgi?id=218578
so that this can be tested.

Regards,

Hans


Hans de Goede (2):
  iio: accel: mxc4005: Interrupt handling fixes
  iio: accel: mxc4005: Reset chip on probe() and resume()

 drivers/iio/accel/mxc4005.c | 82 +++++++++++++++++++++++++++++++++----
 1 file changed, 75 insertions(+), 7 deletions(-)

-- 
2.44.0

[RFC 1/2] iio: accel: mxc4005: Interrupt handling fixes

[Hans de Goede]

There are 2 issues with interrupt handling in the mxc4005 driver:

1. mxc4005_set_trigger_state() writes MXC4005_REG_INT_MASK1_BIT_DRDYE
(0x01) to INT_MASK1 to enable the interrupt, but to disable the interrupt
it writes ~MXC4005_REG_INT_MASK1_BIT_DRDYE which is 0xfe, so it enables
all other interrupt sources in the INT_SRC1 register. On the MXC4005 this
is not an issue because only bit 0 of the register is used. On the MXC6655
OTOH this is a problem since bit7 is used as TC (Temperature Compensation)
disable bit and writing 1 to this disables Temperature Compensation which
should only be done when running self-tests on the chip.

Write 0 instead of ~MXC4005_REG_INT_MASK1_BIT_DRDYE to disable
the interrupts to fix this.

2. The datasheets for the MXC4005 / MXC6655 do not state what the reset
value for the INT_MASK0 and INT_MASK1 registers is and since these are
write only we also cannot learn this from the hw. Presumably the reset
value for both is all 0, which means all interrupts disabled.

Explicitly set both registers to 0 from mxc4005_chip_init() to ensure
both masks are actually set to 0.

Fixes: 79846e33aac1 ("iio: accel: mxc4005: add support for mxc6655")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/iio/accel/mxc4005.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/accel/mxc4005.c b/drivers/iio/accel/mxc4005.c
index 61839be501c2..111f4bcf24ad 100644
--- a/drivers/iio/accel/mxc4005.c
+++ b/drivers/iio/accel/mxc4005.c
@@ -27,9 +27,13 @@
 #define MXC4005_REG_ZOUT_UPPER		0x07
 #define MXC4005_REG_ZOUT_LOWER		0x08
 
+#define MXC4005_REG_INT_MASK0		0x0A
+
 #define MXC4005_REG_INT_MASK1		0x0B
 #define MXC4005_REG_INT_MASK1_BIT_DRDYE	0x01
 
+#define MXC4005_REG_INT_CLR0		0x00
+
 #define MXC4005_REG_INT_CLR1		0x01
 #define MXC4005_REG_INT_CLR1_BIT_DRDYC	0x01
 
@@ -113,7 +117,9 @@ static bool mxc4005_is_readable_reg(struct device *dev, unsigned int reg)
 static bool mxc4005_is_writeable_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
+	case MXC4005_REG_INT_CLR0:
 	case MXC4005_REG_INT_CLR1:
+	case MXC4005_REG_INT_MASK0:
 	case MXC4005_REG_INT_MASK1:
 	case MXC4005_REG_CONTROL:
 		return true;
@@ -330,17 +336,13 @@ static int mxc4005_set_trigger_state(struct iio_trigger *trig,
 {
 	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
 	struct mxc4005_data *data = iio_priv(indio_dev);
+	unsigned int val;
 	int ret;
 
 	mutex_lock(&data->mutex);
-	if (state) {
-		ret = regmap_write(data->regmap, MXC4005_REG_INT_MASK1,
-				   MXC4005_REG_INT_MASK1_BIT_DRDYE);
-	} else {
-		ret = regmap_write(data->regmap, MXC4005_REG_INT_MASK1,
-				   ~MXC4005_REG_INT_MASK1_BIT_DRDYE);
-	}
 
+	val = state ? MXC4005_REG_INT_MASK1_BIT_DRDYE : 0;
+	ret = regmap_write(data->regmap, MXC4005_REG_INT_MASK1, val);
 	if (ret < 0) {
 		mutex_unlock(&data->mutex);
 		dev_err(data->dev, "failed to update reg_int_mask1");
@@ -382,6 +384,14 @@ static int mxc4005_chip_init(struct mxc4005_data *data)
 
 	dev_dbg(data->dev, "MXC4005 chip id %02x\n", reg);
 
+	ret = regmap_write(data->regmap, MXC4005_REG_INT_MASK0, 0);
+	if (ret < 0)
+		return dev_err_probe(data->dev, ret, "writing INT_MASK0\n");
+
+	ret = regmap_write(data->regmap, MXC4005_REG_INT_MASK1, 0);
+	if (ret < 0)
+		return dev_err_probe(data->dev, ret, "writing INT_MASK1\n");
+
 	return 0;
 }
 
-- 
2.44.0

[RFC 2/2] iio: accel: mxc4005: Reset chip on probe() and resume()

[Hans de Goede]

On some designs the chip is not properly reset when powered up at boot or
after a suspend/resume cycle.

Use the sw-reset feature to ensure that the chip is in a clean state
after probe() / resume() and in the case of resume() restore the settings
(scale, trigger-enabled).

Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218578
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/iio/accel/mxc4005.c | 58 +++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/iio/accel/mxc4005.c b/drivers/iio/accel/mxc4005.c
index 111f4bcf24ad..9f38d3a08299 100644
--- a/drivers/iio/accel/mxc4005.c
+++ b/drivers/iio/accel/mxc4005.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2014, Intel Corporation.
  */
 
+#include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/i2c.h>
 #include <linux/iio/iio.h>
@@ -36,6 +37,7 @@
 
 #define MXC4005_REG_INT_CLR1		0x01
 #define MXC4005_REG_INT_CLR1_BIT_DRDYC	0x01
+#define MXC4005_REG_INT_CLR1_SW_RST	0x10
 
 #define MXC4005_REG_CONTROL		0x0D
 #define MXC4005_REG_CONTROL_MASK_FSR	GENMASK(6, 5)
@@ -43,6 +45,9 @@
 
 #define MXC4005_REG_DEVICE_ID		0x0E
 
+/* Datasheet does not specify a reset time, this is a conservative guess */
+#define MXC4005_RESET_TIME_US		2000
+
 enum mxc4005_axis {
 	AXIS_X,
 	AXIS_Y,
@@ -66,6 +71,8 @@ struct mxc4005_data {
 		s64 timestamp __aligned(8);
 	} scan;
 	bool trigger_enabled;
+	unsigned int control;
+	unsigned int int_mask1;
 };
 
 /*
@@ -349,6 +356,7 @@ static int mxc4005_set_trigger_state(struct iio_trigger *trig,
 		return ret;
 	}
 
+	data->int_mask1 = val;
 	data->trigger_enabled = state;
 	mutex_unlock(&data->mutex);
 
@@ -384,6 +392,13 @@ static int mxc4005_chip_init(struct mxc4005_data *data)
 
 	dev_dbg(data->dev, "MXC4005 chip id %02x\n", reg);
 
+	ret = regmap_write(data->regmap, MXC4005_REG_INT_CLR1,
+			   MXC4005_REG_INT_CLR1_SW_RST);
+	if (ret < 0)
+		return dev_err_probe(data->dev, ret, "resetting chip\n");
+
+	fsleep(MXC4005_RESET_TIME_US);
+
 	ret = regmap_write(data->regmap, MXC4005_REG_INT_MASK0, 0);
 	if (ret < 0)
 		return dev_err_probe(data->dev, ret, "writing INT_MASK0\n");
@@ -479,6 +494,48 @@ static int mxc4005_probe(struct i2c_client *client)
 	return devm_iio_device_register(&client->dev, indio_dev);
 }
 
+static int mxc4005_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct mxc4005_data *data = iio_priv(indio_dev);
+	int ret;
+
+	/* Save control to restore it on resume */
+	ret = regmap_read(data->regmap, MXC4005_REG_CONTROL, &data->control);
+	if (ret < 0)
+		dev_err(data->dev, "failed to read reg_control\n");
+
+	return ret;
+}
+
+static int mxc4005_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct mxc4005_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = regmap_write(data->regmap, MXC4005_REG_INT_CLR1,
+			   MXC4005_REG_INT_CLR1_SW_RST);
+	if (ret) {
+		dev_err(data->dev, "failed to reset chip: %d\n", ret);
+		return ret;
+	}
+
+	fsleep(MXC4005_RESET_TIME_US);
+
+	ret = regmap_write(data->regmap, MXC4005_REG_CONTROL, data->control);
+	ret |= regmap_write(data->regmap, MXC4005_REG_INT_MASK0, 0);
+	ret |= regmap_write(data->regmap, MXC4005_REG_INT_MASK1, data->int_mask1);
+	if (ret) {
+		dev_err(data->dev, "failed to restore registers\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(mxc4005_pm_ops, mxc4005_suspend, mxc4005_resume);
+
 static const struct acpi_device_id mxc4005_acpi_match[] = {
 	{"MXC4005",	0},
 	{"MXC6655",	0},
@@ -506,6 +563,7 @@ static struct i2c_driver mxc4005_driver = {
 		.name = MXC4005_DRV_NAME,
 		.acpi_match_table = mxc4005_acpi_match,
 		.of_match_table = mxc4005_of_match,
+		.pm = pm_sleep_ptr(&mxc4005_pm_ops),
 	},
 	.probe		= mxc4005_probe,
 	.id_table	= mxc4005_id,
-- 
2.44.0

Re: [RFC 0/2] iio: accel: mxc4005: IRQ fixes + reset chip on probe()

[Hans de Goede]

Hi,

On 3/26/24 12:36 PM, Hans de Goede wrote:
> Hi All,
> 
> As reported here:
> https://bugzilla.kernel.org/show_bug.cgi?id=218578
> 
> The MXC6655 found on several Chuwi tablets models works sometimes instead
> of all the time. The problem seems to be that the power-sequencing done
> on the board causes the chip to not reliably reset leaving it in a random
> state at boot (and after suspend/resume).
> 
> The second patch in this set fixes this by using the sw-reset feature to
> explicitly reset the chip on probe() and resume().
> 
> While working on this I also noticed an issue with the interrupt mask
> handling, this is fixed in the first patch of the set.
> 
> This is marked as a RFC for now because this is untested atm. I'll
> provide a test kernel to the reporter of:
> https://bugzilla.kernel.org/show_bug.cgi?id=218578
> so that this can be tested.

This has been tested now and works as advertised so from
my POV this is ready for merging now and no longer has
RFC status.

Regards,

Hans



> Hans de Goede (2):
>   iio: accel: mxc4005: Interrupt handling fixes
>   iio: accel: mxc4005: Reset chip on probe() and resume()
> 
>  drivers/iio/accel/mxc4005.c | 82 +++++++++++++++++++++++++++++++++----
>  1 file changed, 75 insertions(+), 7 deletions(-)
> 

Re: [RFC 2/2] iio: accel: mxc4005: Reset chip on probe() and resume()

[Jonathan Cameron]

On Tue, 26 Mar 2024 12:37:00 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> On some designs the chip is not properly reset when powered up at boot or
> after a suspend/resume cycle.
> 
> Use the sw-reset feature to ensure that the chip is in a clean state
> after probe() / resume() and in the case of resume() restore the settings
> (scale, trigger-enabled).
> 
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218578
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Applied but with a change to the error handling in resume.
It has the result that a failure may result in later commands not running
but given we are probably dead anyway if we fail there, I'm not that
worried by that tweak.  Plus point is we get the most informative error
code possible.

Applied to the fixes-togreg branch of iio.git and marked for stable.

Thanks,

Jonathan


> ---
>  drivers/iio/accel/mxc4005.c | 58 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
> 
> diff --git a/drivers/iio/accel/mxc4005.c b/drivers/iio/accel/mxc4005.c
> index 111f4bcf24ad..9f38d3a08299 100644
> --- a/drivers/iio/accel/mxc4005.c
> +++ b/drivers/iio/accel/mxc4005.c
> @@ -5,6 +5,7 @@
>   * Copyright (c) 2014, Intel Corporation.
>   */
>  
> +#include <linux/delay.h>
>  #include <linux/module.h>
>  #include <linux/i2c.h>
>  #include <linux/iio/iio.h>
> @@ -36,6 +37,7 @@
>  
>  #define MXC4005_REG_INT_CLR1		0x01
>  #define MXC4005_REG_INT_CLR1_BIT_DRDYC	0x01
> +#define MXC4005_REG_INT_CLR1_SW_RST	0x10
>  
>  #define MXC4005_REG_CONTROL		0x0D
>  #define MXC4005_REG_CONTROL_MASK_FSR	GENMASK(6, 5)
> @@ -43,6 +45,9 @@
>  
>  #define MXC4005_REG_DEVICE_ID		0x0E
>  
> +/* Datasheet does not specify a reset time, this is a conservative guess */
> +#define MXC4005_RESET_TIME_US		2000
> +
>  enum mxc4005_axis {
>  	AXIS_X,
>  	AXIS_Y,
> @@ -66,6 +71,8 @@ struct mxc4005_data {
>  		s64 timestamp __aligned(8);
>  	} scan;
>  	bool trigger_enabled;
> +	unsigned int control;
> +	unsigned int int_mask1;
>  };
>  
>  /*
> @@ -349,6 +356,7 @@ static int mxc4005_set_trigger_state(struct iio_trigger *trig,
>  		return ret;
>  	}
>  
> +	data->int_mask1 = val;
>  	data->trigger_enabled = state;
>  	mutex_unlock(&data->mutex);
>  
> @@ -384,6 +392,13 @@ static int mxc4005_chip_init(struct mxc4005_data *data)
>  
>  	dev_dbg(data->dev, "MXC4005 chip id %02x\n", reg);
>  
> +	ret = regmap_write(data->regmap, MXC4005_REG_INT_CLR1,
> +			   MXC4005_REG_INT_CLR1_SW_RST);
> +	if (ret < 0)
> +		return dev_err_probe(data->dev, ret, "resetting chip\n");
> +
> +	fsleep(MXC4005_RESET_TIME_US);
> +
>  	ret = regmap_write(data->regmap, MXC4005_REG_INT_MASK0, 0);
>  	if (ret < 0)
>  		return dev_err_probe(data->dev, ret, "writing INT_MASK0\n");
> @@ -479,6 +494,48 @@ static int mxc4005_probe(struct i2c_client *client)
>  	return devm_iio_device_register(&client->dev, indio_dev);
>  }
>  
> +static int mxc4005_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct mxc4005_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	/* Save control to restore it on resume */
> +	ret = regmap_read(data->regmap, MXC4005_REG_CONTROL, &data->control);
> +	if (ret < 0)
> +		dev_err(data->dev, "failed to read reg_control\n");
> +
> +	return ret;
> +}
> +
> +static int mxc4005_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct mxc4005_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = regmap_write(data->regmap, MXC4005_REG_INT_CLR1,
> +			   MXC4005_REG_INT_CLR1_SW_RST);
> +	if (ret) {
> +		dev_err(data->dev, "failed to reset chip: %d\n", ret);
> +		return ret;
> +	}
> +
> +	fsleep(MXC4005_RESET_TIME_US);
> +
> +	ret = regmap_write(data->regmap, MXC4005_REG_CONTROL, data->control);
> +	ret |= regmap_write(data->regmap, MXC4005_REG_INT_MASK0, 0);
> +	ret |= regmap_write(data->regmap, MXC4005_REG_INT_MASK1, data->int_mask1);
> +	if (ret) {
I'm really not a fan of doing this for return handling because you are eating the
useful values (to avoid corrupting them).  Better to have separate error
checks for each one. 
Given that's trivial I can make that change whilst applying.

Change made is: 
diff --git a/drivers/iio/accel/mxc4005.c b/drivers/iio/accel/mxc4005.c
index 9f38d3a08299..63c3566a533b 100644
--- a/drivers/iio/accel/mxc4005.c
+++ b/drivers/iio/accel/mxc4005.c
@@ -524,11 +524,21 @@ static int mxc4005_resume(struct device *dev)
        fsleep(MXC4005_RESET_TIME_US);
 
        ret = regmap_write(data->regmap, MXC4005_REG_CONTROL, data->control);
-       ret |= regmap_write(data->regmap, MXC4005_REG_INT_MASK0, 0);
-       ret |= regmap_write(data->regmap, MXC4005_REG_INT_MASK1, data->int_mask1);
        if (ret) {
-               dev_err(data->dev, "failed to restore registers\n");
-               return -EIO;
+               dev_err(data->dev, "failed to restore control register\n");
+               return ret;
+       }
+
+       ret = regmap_write(data->regmap, MXC4005_REG_INT_MASK0, 0);
+       if (ret) {
+               dev_err(data->dev, "failed to restore interrupt 0 mask\n");
+               return ret;
+       }
+
+       ret = regmap_write(data->regmap, MXC4005_REG_INT_MASK1, data->int_mask1);
+       if (ret) {
+               dev_err(data->dev, "failed to restore interrupt 1 mask\n");
+               return ret;
        }
 
        return 0;



> +		dev_err(data->dev, "failed to restore registers\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(mxc4005_pm_ops, mxc4005_suspend, mxc4005_resume);
> +
>  static const struct acpi_device_id mxc4005_acpi_match[] = {
>  	{"MXC4005",	0},
>  	{"MXC6655",	0},
> @@ -506,6 +563,7 @@ static struct i2c_driver mxc4005_driver = {
>  		.name = MXC4005_DRV_NAME,
>  		.acpi_match_table = mxc4005_acpi_match,
>  		.of_match_table = mxc4005_of_match,
> +		.pm = pm_sleep_ptr(&mxc4005_pm_ops),
>  	},
>  	.probe		= mxc4005_probe,
>  	.id_table	= mxc4005_id,