| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3fdf8296-8839-4b44-8048-3720b0f45787@baylibre.com>
Date: Thu, 8 May 2025 11:24:08 -0500
From: David Lechner <dlechner@...libre.com>
To: Brajesh Patil <brajeshpatil11@...il.com>, jic23@...nel.org,
lars@...afoo.de
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
marcelo.schmitt1@...il.com
Subject: Re: [PATCH v2 5/5] iio: magnetometer: qmc5883l: add mount matrix,
control features and power management
On 5/8/25 7:09 AM, Brajesh Patil wrote:
> Signed-off-by: Brajesh Patil <brajeshpatil11@...il.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
>
Powered by blists - more mailing lists