Re: [PATCH v2 5/5] iio: magnetometer: qmc5883l: add mount matrix, control features and power management

[David Lechner <dlechner@baylibre.com>]

On 5/8/25 7:09 AM, Brajesh Patil wrote:
> Signed-off-by: Brajesh Patil <brajeshpatil11@gmail.com>
> ---
>  drivers/iio/magnetometer/qmc5883l.c | 89 +++++++++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
> 
> diff --git a/drivers/iio/magnetometer/qmc5883l.c b/drivers/iio/magnetometer/qmc5883l.c
> index 07c65f193def..d26f959ab8c5 100644
> --- a/drivers/iio/magnetometer/qmc5883l.c
> +++ b/drivers/iio/magnetometer/qmc5883l.c
> @@ -7,6 +7,7 @@
>  #include <linux/iio/trigger.h>
>  #include <linux/iio/trigger_consumer.h>
>  #include <linux/iio/triggered_buffer.h>
> +#include <linux/pm.h>
>  #include <linux/regmap.h>
>  #include <linux/types.h>
> 
> @@ -54,6 +55,10 @@
>  #define QMC5883L_OSR_MASK           0xC0
>  #define QMC5883L_OSR_SHIFT          6
> 
> +#define QMC5883L_SOFT_RST           0x80
> +#define QMC5883L_ROL_PNT            0x40
> +#define QMC5883L_INT_ENB            0x01
> +
>  static const char *const qmc5883l_modes[] = {
>  	"standby", "continuous"
>  };
> @@ -80,12 +85,14 @@ static const int qmc5883l_odr_map[] = {
>   * @client: I2C client structure
>   * @lock: mutex to protect register access
>   * @regmap: register map of the device
> + * @orientation: Sensor mounting orientation matrix
>   * @scan: buffer for triggered data reading
>   */
>  struct qmc5883l_data {
>  	struct i2c_client *client;
>  	struct mutex lock; /* Protects sensor read/write operations */
>  	struct regmap *regmap;
> +	struct iio_mount_matrix orientation;
> 
>  	struct {
>  		__le16 chans[3];
> @@ -102,6 +109,9 @@ static ssize_t qmc5883l_show_scale_avail(struct device *dev,
>  					 struct device_attribute *attr, char *buf);
>  static ssize_t qmc5883l_show_status(struct device *dev,
>  				    struct device_attribute *attr, char *buf);
> +static ssize_t qmc5883l_store_control(struct device *dev,
> +				      struct device_attribute *attr,
> +				      const char *buf, size_t count);
> 
>  static int qmc5883l_buffer_preenable(struct iio_dev *indio_dev)
>  {
> @@ -357,6 +367,15 @@ static int qmc5883l_read_measurement(struct qmc5883l_data *data,
>  	return IIO_VAL_INT;
>  }
> 
> +static const struct iio_mount_matrix *
> +qmc5883l_get_mount_matrix(const struct iio_dev *indio_dev,
> +			  const struct iio_chan_spec *chan)
> +{
> +	struct qmc5883l_data *data = iio_priv(indio_dev);
> +
> +	return &data->orientation;
> +}
> +
>  static const struct iio_enum qmc5883l_mode_enum = {
>  	.items = qmc5883l_modes,
>  	.num_items = ARRAY_SIZE(qmc5883l_modes),
> @@ -376,6 +395,7 @@ static const struct iio_chan_spec_ext_info qmc5883l_ext_info[] = {
>  	IIO_ENUM_AVAILABLE("mode", IIO_SHARED_BY_TYPE, &qmc5883l_mode_enum),
>  	IIO_ENUM("oversampling_ratio", IIO_SHARED_BY_TYPE, &qmc5883l_osr_enum),
>  	IIO_ENUM_AVAILABLE("oversampling_ratio", IIO_SHARED_BY_TYPE, &qmc5883l_osr_enum),
> +	IIO_MOUNT_MATRIX(IIO_SHARED_BY_DIR, qmc5883l_get_mount_matrix),
>  	{ }
>  };
> 
> @@ -383,6 +403,8 @@ static IIO_DEV_ATTR_SAMP_FREQ_AVAIL(qmc5883l_show_odr_avail);
>  static IIO_DEVICE_ATTR(scale_available, 0444, qmc5883l_show_scale_avail, NULL, 0);
>  static IIO_DEVICE_ATTR(data_ready, 0444, qmc5883l_show_status, NULL, 0);
>  static IIO_DEVICE_ATTR(overflow, 0444, qmc5883l_show_status, NULL, 0);
> +static IIO_DEVICE_ATTR(soft_reset, 0200, NULL, qmc5883l_store_control, 0);
> +static IIO_DEVICE_ATTR(pointer_rollover, 0200, NULL, qmc5883l_store_control, 0);

More custom attribute that probably aren't needed or need some justification.

A reset is usually only done on driver probe. Not sure what pointer rollover is.
> 
>  static ssize_t qmc5883l_show_odr_avail(struct device *dev,
>  				       struct device_attribute *attr, char *buf)
> @@ -416,6 +438,44 @@ static ssize_t qmc5883l_show_status(struct device *dev,
>  	return -EINVAL;
>  }
> 
> +/* Control attribute writes:
> + * - soft_reset: performs device reset and re-init
> + * - pointer_rollover: enables/disables rollover pointer
> + */
> +static ssize_t qmc5883l_store_control(struct device *dev, struct device_attribute *attr,
> +				      const char *buf, size_t count)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct qmc5883l_data *data = iio_priv(indio_dev);
> +	bool val;
> +	int ret = 0;
> +
> +	ret = kstrtobool(buf, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (attr == &iio_dev_attr_soft_reset.dev_attr && val) {
> +		mutex_lock(&data->lock);
> +		ret = regmap_write(data->regmap, QMC5883L_CONTROL_REG_2,
> +				   QMC5883L_SOFT_RST);
> +		mutex_unlock(&data->lock);
> +		msleep(50);
> +
> +		ret = qmc5883l_init(data);
> +		if (ret < 0) {
> +			dev_err(&data->client->dev, "Sensor Reinitialization Failed\n");
> +			return ret;
> +		}
> +		dev_info(&data->client->dev, "Sensor successfully reinitialized\n");
> +	} else if (attr == &iio_dev_attr_pointer_rollover.dev_attr) {
> +		mutex_lock(&data->lock);
> +		ret = regmap_update_bits(data->regmap, QMC5883L_CONTROL_REG_2,
> +					 QMC5883L_ROL_PNT, val ? QMC5883L_ROL_PNT : 0);
> +		mutex_unlock(&data->lock);
> +	}
> +	return ret ? ret : count;
> +}
> +
>  static int qmc5883l_read_raw(struct iio_dev *indio_dev,
>  			     struct iio_chan_spec const *chan, int *val, int *val2, long mask)
>  {
> @@ -599,6 +659,8 @@ static struct attribute *qmc5883l_attributes[] = {
>  	&iio_dev_attr_scale_available.dev_attr.attr,
>  	&iio_dev_attr_data_ready.dev_attr.attr,
>  	&iio_dev_attr_overflow.dev_attr.attr,
> +	&iio_dev_attr_soft_reset.dev_attr.attr,
> +	&iio_dev_attr_pointer_rollover.dev_attr.attr,
>  	NULL
>  };
> 
> @@ -659,6 +721,27 @@ static const struct iio_info qmc5883l_info = {
> 
>  static const unsigned long qmc5883l_scan_masks[] = {0x7, 0};
> 
> +static int qmc5883l_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct qmc5883l_data *data = iio_priv(indio_dev);
> +
> +	return qmc5883l_set_mode(data, QMC5883L_MODE_STANDBY);
> +}
> +
> +static int qmc5883l_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct qmc5883l_data *data = iio_priv(indio_dev);
> +
> +	return qmc5883l_set_mode(data, QMC5883L_MODE_CONT);
> +}

The driver is currently only seting CONT mode when reading data, so having
this in the suspend/resume doesn't make sense to me.

> +
> +static const struct dev_pm_ops qmc5883l_pm_ops = {
> +	.suspend = qmc5883l_suspend,
> +	.resume = qmc5883l_resume,
> +};
> +
>  static int qmc5883l_probe(struct i2c_client *client)
>  {
>  	struct regmap *regmap;
> @@ -683,6 +766,10 @@ static int qmc5883l_probe(struct i2c_client *client)
>  	data->regmap = regmap;
>  	mutex_init(&data->lock);
> 
> +	ret = iio_read_mount_matrix(&client->dev, &data->orientation);
> +	if (ret)
> +		dev_warn(&client->dev, "Failed to read mount matrix: %d\n", ret);
> +
>  	indio_dev->name = "qmc5883l";
>  	indio_dev->info = &qmc5883l_info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> @@ -693,6 +780,7 @@ static int qmc5883l_probe(struct i2c_client *client)
>  	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev,
>  					      NULL, &qmc5883l_trigger_handler,
>  					      &qmc5883l_buffer_setup_ops);
> +
>  	if (ret < 0) {
>  		dev_err(&client->dev, "Failed to setup triggered buffer: %d\n", ret);
>  		return ret;
> @@ -730,6 +818,7 @@ static struct i2c_driver qmc5883l_driver = {
>  	.driver = {
>  		.name = "qmc5883l",
>  		.of_match_table = qmc5883l_of_match,
> +		.pm = pm_sleep_ptr(&qmc5883l_pm_ops),
>  	},
>  	.id_table = qmc5883l_id,
>  	.probe = qmc5883l_probe,
> --
> 2.39.5
>