diff mbox series

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

Message ID 20240326113700.56725-3-hdegoede@redhat.com (mailing list archive)
State Accepted
Headers show
Series iio: accel: mxc4005: IRQ fixes + reset chip on probe() | expand

Commit Message

Hans de Goede March 26, 2024, 11:37 a.m. UTC 
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(+)

Comments

Jonathan Cameron March 28, 2024, 1:05 p.m. UTC | #1 
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,
diff mbox series

Patch

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,