* [PATCH v2 4/5] iio: magnetometer: qmc5883l: add extended sysfs attributes and configuration options
2025-05-08 12:08 [PATCH v2 3/5] iio: magnetometer: qmc5883l: Add initial driver support Brajesh Patil
@ 2025-05-08 12:08 ` Brajesh Patil
2025-05-08 16:18 ` David Lechner
2025-05-11 15:33 ` Jonathan Cameron
2025-05-08 12:09 ` [PATCH v2 5/5] iio: magnetometer: qmc5883l: add mount matrix, control features and power management Brajesh Patil
2025-05-08 16:03 ` [PATCH v2 3/5] iio: magnetometer: qmc5883l: Add initial driver support David Lechner
2 siblings, 2 replies; 8+ messages in thread
From: Brajesh Patil @ 2025-05-08 12:08 UTC (permalink / raw)
To: jic23, lars
Cc: linux-iio, linux-kernel, marcelo.schmitt1, dlechner,
Brajesh Patil
Signed-off-by: Brajesh Patil <brajeshpatil11@gmail.com>
---
drivers/iio/magnetometer/qmc5883l.c | 276 +++++++++++++++++++++++++++-
1 file changed, 274 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/magnetometer/qmc5883l.c b/drivers/iio/magnetometer/qmc5883l.c
index 68597cdd0ca8..07c65f193def 100644
--- a/drivers/iio/magnetometer/qmc5883l.c
+++ b/drivers/iio/magnetometer/qmc5883l.c
@@ -54,6 +54,20 @@
#define QMC5883L_OSR_MASK 0xC0
#define QMC5883L_OSR_SHIFT 6
+static const char *const qmc5883l_modes[] = {
+ "standby", "continuous"
+};
+
+static const int qmc5883l_odr_avail[][2] = {
+ {10, 0}, {50, 0}, {100, 0}, {200, 0}
+};
+
+static const char *const qmc5883l_osr_avail[] = {
+ "512", "256", "128", "64"
+};
+
+static const int qmc5883l_scale_avail[] = {2, 8};
+
static const int qmc5883l_odr_map[] = {
[QMC5883L_ODR_10HZ] = 10,
[QMC5883L_ODR_50HZ] = 50,
@@ -82,6 +96,12 @@ struct qmc5883l_data {
static int qmc5883l_init(struct qmc5883l_data *data);
static int qmc5883l_set_mode(struct qmc5883l_data *data, unsigned int mode);
+static ssize_t qmc5883l_show_odr_avail(struct device *dev,
+ struct device_attribute *attr, char *buf);
+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 int qmc5883l_buffer_preenable(struct iio_dev *indio_dev)
{
@@ -142,6 +162,102 @@ static const struct regmap_config qmc5883l_regmap_config = {
.cache_type = REGCACHE_RBTREE,
};
+static int qmc5883l_set_osr(struct qmc5883l_data *data, unsigned int osr)
+{
+ int ret;
+
+ mutex_lock(&data->lock);
+ ret = regmap_update_bits(data->regmap, QMC5883L_CONTROL_REG_1,
+ QMC5883L_OSR_MASK, osr << QMC5883L_OSR_SHIFT);
+ mutex_unlock(&data->lock);
+
+ return ret;
+}
+
+static int qmc5883l_get_osr(struct qmc5883l_data *data)
+{
+ unsigned int val;
+ int ret;
+
+ ret = regmap_read(data->regmap, QMC5883L_CONTROL_REG_1, &val);
+ if (ret < 0)
+ return ret;
+
+ return (val & QMC5883L_OSR_MASK) >> QMC5883L_OSR_SHIFT;
+}
+
+static int qmc5883l_get_osr_iio(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan)
+{
+ struct qmc5883l_data *data = iio_priv(indio_dev);
+
+ return qmc5883l_get_osr(data);
+}
+
+static int qmc5883l_set_osr_iio(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ unsigned int osr)
+{
+ struct qmc5883l_data *data = iio_priv(indio_dev);
+
+ return qmc5883l_set_osr(data, osr);
+}
+
+static s32 qmc5883l_set_rng(struct qmc5883l_data *data, u8 rng)
+{
+ int ret;
+
+ mutex_lock(&data->lock);
+ ret = regmap_update_bits(data->regmap, QMC5883L_CONTROL_REG_1,
+ QMC5883L_RNG_MASK, rng << QMC5883L_RNG_SHIFT);
+ mutex_unlock(&data->lock);
+ return ret;
+}
+
+static s32 qmc5883l_set_odr(struct qmc5883l_data *data, u8 odr)
+{
+ int ret;
+
+ mutex_lock(&data->lock);
+ ret = regmap_update_bits(data->regmap, QMC5883L_CONTROL_REG_1,
+ QMC5883L_ODR_MASK, odr << QMC5883L_ODR_SHIFT);
+ mutex_unlock(&data->lock);
+ return ret;
+}
+
+static int qmc5883l_get_odr(struct qmc5883l_data *data, unsigned int *odr)
+{
+ int ret;
+ unsigned int val;
+
+ ret = regmap_read(data->regmap, QMC5883L_CONTROL_REG_1, &val);
+ if (ret < 0)
+ return ret;
+
+ *odr = (val & QMC5883L_ODR_MASK) >> QMC5883L_ODR_SHIFT;
+ return 0;
+}
+
+static int qmc5883l_get_delay_based_on_odr(struct qmc5883l_data *data)
+{
+ unsigned int odr;
+ int ret;
+
+ ret = qmc5883l_get_odr(data, &odr);
+ if (ret < 0)
+ return ret;
+
+ switch (odr) {
+ case QMC5883L_ODR_10HZ: return 100000;
+ case QMC5883L_ODR_50HZ: return 20000;
+ case QMC5883L_ODR_100HZ: return 10000;
+ case QMC5883L_ODR_200HZ: return 5000;
+ default:
+ dev_err(&data->client->dev, "Invalid ODR value: %u\n", odr);
+ return -EINVAL;
+ }
+}
+
static int qmc5883l_set_mode(struct qmc5883l_data *data, unsigned int mode)
{
int ret;
@@ -154,11 +270,44 @@ static int qmc5883l_set_mode(struct qmc5883l_data *data, unsigned int mode)
return ret;
}
+static int qmc5883l_get_mode(struct qmc5883l_data *data)
+{
+ unsigned int val;
+ int ret;
+
+ ret = regmap_read(data->regmap, QMC5883L_CONTROL_REG_1, &val);
+ if (ret < 0)
+ return ret;
+
+ return (val & QMC5883L_MODE_MASK) >> QMC5883L_MODE_SHIFT;
+}
+
+static int qmc5883l_get_mode_iio(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan)
+{
+ struct qmc5883l_data *data = iio_priv(indio_dev);
+
+ return qmc5883l_get_mode(data);
+}
+
+static int qmc5883l_set_mode_iio(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ unsigned int mode)
+{
+ struct qmc5883l_data *data = iio_priv(indio_dev);
+
+ return qmc5883l_set_mode(data, mode);
+}
+
static int qmc5883l_wait_measurement(struct qmc5883l_data *data)
{
int tries = 150;
unsigned int val;
- int ret;
+ int ret, delay;
+
+ delay = qmc5883l_get_delay_based_on_odr(data);
+ if (delay < 0)
+ return delay;
while (tries-- > 0) {
ret = regmap_read(data->regmap, QMC5883L_STATUS_REG, &val);
@@ -172,7 +321,7 @@ static int qmc5883l_wait_measurement(struct qmc5883l_data *data)
if (val & QMC5883L_DRDY)
return 0;
- usleep_range(5000, 6000);
+ usleep_range(delay, delay + 1000);
}
dev_err(&data->client->dev, "data not ready\n");
@@ -208,6 +357,65 @@ static int qmc5883l_read_measurement(struct qmc5883l_data *data,
return IIO_VAL_INT;
}
+static const struct iio_enum qmc5883l_mode_enum = {
+ .items = qmc5883l_modes,
+ .num_items = ARRAY_SIZE(qmc5883l_modes),
+ .get = qmc5883l_get_mode_iio,
+ .set = qmc5883l_set_mode_iio,
+};
+
+static const struct iio_enum qmc5883l_osr_enum = {
+ .items = qmc5883l_osr_avail,
+ .num_items = ARRAY_SIZE(qmc5883l_osr_avail),
+ .get = qmc5883l_get_osr_iio,
+ .set = qmc5883l_set_osr_iio,
+};
+
+static const struct iio_chan_spec_ext_info qmc5883l_ext_info[] = {
+ IIO_ENUM("mode", IIO_SHARED_BY_TYPE, &qmc5883l_mode_enum),
+ 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),
+ { }
+};
+
+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 ssize_t qmc5883l_show_odr_avail(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return sprintf(buf, "10 50 100 200\n");
+}
+
+static ssize_t qmc5883l_show_scale_avail(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ return sprintf(buf, "2 8\n");
+}
+
+static ssize_t qmc5883l_show_status(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct qmc5883l_data *data = iio_priv(indio_dev);
+ unsigned int val;
+ int ret;
+
+ ret = regmap_read(data->regmap, QMC5883L_STATUS_REG, &val);
+ if (ret < 0)
+ return ret;
+
+ if (attr == (struct device_attribute *)&iio_dev_attr_data_ready.dev_attr.attr)
+ return sprintf(buf, "%d\n", !!(val & QMC5883L_DRDY));
+ else if (attr == (struct device_attribute *)&iio_dev_attr_overflow.dev_attr.attr)
+ return sprintf(buf, "%d\n", !!(val & QMC5883L_OVL));
+
+ return -EINVAL;
+}
+
static int qmc5883l_read_raw(struct iio_dev *indio_dev,
struct iio_chan_spec const *chan, int *val, int *val2, long mask)
{
@@ -275,6 +483,54 @@ static int qmc5883l_read_raw(struct iio_dev *indio_dev,
return -EINVAL;
}
+static int qmc5883l_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct qmc5883l_data *data = iio_priv(indio_dev);
+ int odr, range;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ if (val == 10 && val2 == 0)
+ odr = QMC5883L_ODR_10HZ;
+ else if (val == 50 && val2 == 0)
+ odr = QMC5883L_ODR_50HZ;
+ else if (val == 100 && val2 == 0)
+ odr = QMC5883L_ODR_100HZ;
+ else if (val == 200 && val2 == 0)
+ odr = QMC5883L_ODR_200HZ;
+ else
+ return -EINVAL;
+
+ return qmc5883l_set_odr(data, odr);
+ case IIO_CHAN_INFO_SCALE:
+ if (val == 2 && val2 == 0)
+ range = QMC5883L_RNG_2G;
+ else if (val == 8 && val2 == 0)
+ range = QMC5883L_RNG_8G;
+ else
+ return -EINVAL;
+
+ return qmc5883l_set_rng(data, range << QMC5883L_RNG_SHIFT);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int qmc5883l_write_raw_get_fmt(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, long mask)
+{
+ switch (mask) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SCALE:
+ return IIO_VAL_INT_PLUS_NANO;
+ default:
+ return -EINVAL;
+ }
+}
+
static irqreturn_t qmc5883l_trigger_handler(int irq, void *p)
{
struct iio_poll_func *pf = p;
@@ -321,6 +577,7 @@ static irqreturn_t qmc5883l_trigger_handler(int irq, void *p)
.storagebits = 16, \
.endianness = IIO_LE, \
}, \
+ .ext_info = qmc5883l_ext_info, \
}
static const struct iio_chan_spec qmc5883l_channels[] = {
@@ -337,6 +594,18 @@ static const struct iio_chan_spec qmc5883l_channels[] = {
IIO_CHAN_SOFT_TIMESTAMP(3),
};
+static struct attribute *qmc5883l_attributes[] = {
+ &iio_dev_attr_sampling_frequency_available.dev_attr.attr,
+ &iio_dev_attr_scale_available.dev_attr.attr,
+ &iio_dev_attr_data_ready.dev_attr.attr,
+ &iio_dev_attr_overflow.dev_attr.attr,
+ NULL
+};
+
+static const struct attribute_group qmc5883l_attribute_group = {
+ .attrs = qmc5883l_attributes,
+};
+
static int qmc5883l_init(struct qmc5883l_data *data)
{
int ret;
@@ -382,7 +651,10 @@ static int qmc5883l_init(struct qmc5883l_data *data)
}
static const struct iio_info qmc5883l_info = {
+ .attrs = &qmc5883l_attribute_group,
.read_raw = &qmc5883l_read_raw,
+ .write_raw = &qmc5883l_write_raw,
+ .write_raw_get_fmt = &qmc5883l_write_raw_get_fmt,
};
static const unsigned long qmc5883l_scan_masks[] = {0x7, 0};
--
2.39.5
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2 4/5] iio: magnetometer: qmc5883l: add extended sysfs attributes and configuration options
2025-05-08 12:08 ` [PATCH v2 4/5] iio: magnetometer: qmc5883l: add extended sysfs attributes and configuration options Brajesh Patil
@ 2025-05-08 16:18 ` David Lechner
2025-05-11 15:33 ` Jonathan Cameron
1 sibling, 0 replies; 8+ messages in thread
From: David Lechner @ 2025-05-08 16:18 UTC (permalink / raw)
To: Brajesh Patil, jic23, lars; +Cc: linux-iio, linux-kernel, marcelo.schmitt1
On 5/8/25 7:08 AM, Brajesh Patil wrote:
> Signed-off-by: Brajesh Patil <brajeshpatil11@gmail.com>
> ---
...
> +
> +static const struct iio_chan_spec_ext_info qmc5883l_ext_info[] = {
> + IIO_ENUM("mode", IIO_SHARED_BY_TYPE, &qmc5883l_mode_enum),
> + IIO_ENUM_AVAILABLE("mode", IIO_SHARED_BY_TYPE, &qmc5883l_mode_enum),
> + IIO_ENUM("oversampling_ratio", IIO_SHARED_BY_TYPE, &qmc5883l_osr_enum),
There is already IIO_CHAN_INFO_OVERSAMPLING_RATIO so we don't need to make this
a custom attribute.
> + IIO_ENUM_AVAILABLE("oversampling_ratio", IIO_SHARED_BY_TYPE, &qmc5883l_osr_enum),
> + { }
> +};
> +
> +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);
As far as I can tell, mode, data_ready and overflow are not standard attributes.
I don't see them in Documentation/ABI/testing/sysfs-bus-iio*. So if these really
are needed, we will need a justification of why these don't fit into any
existing attributes.
In the previous patch standby/continuous and data ready were handled internally
already so I don't think need those.
Overflow sounds like it could possibly be an event, but I didn't really look in
to it.
> +
> +static ssize_t qmc5883l_show_odr_avail(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return sprintf(buf, "10 50 100 200\n");
> +}
> +
> +static ssize_t qmc5883l_show_scale_avail(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + return sprintf(buf, "2 8\n");
> +}
> +
> +static ssize_t qmc5883l_show_status(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct qmc5883l_data *data = iio_priv(indio_dev);
> + unsigned int val;
> + int ret;
> +
> + ret = regmap_read(data->regmap, QMC5883L_STATUS_REG, &val);
> + if (ret < 0)
> + return ret;
> +
> + if (attr == (struct device_attribute *)&iio_dev_attr_data_ready.dev_attr.attr)
> + return sprintf(buf, "%d\n", !!(val & QMC5883L_DRDY));
> + else if (attr == (struct device_attribute *)&iio_dev_attr_overflow.dev_attr.attr)
> + return sprintf(buf, "%d\n", !!(val & QMC5883L_OVL));
> +
> + return -EINVAL;
> +}
> +
> static int qmc5883l_read_raw(struct iio_dev *indio_dev,
> struct iio_chan_spec const *chan, int *val, int *val2, long mask)
> {
> @@ -275,6 +483,54 @@ static int qmc5883l_read_raw(struct iio_dev *indio_dev,
> return -EINVAL;
> }
>
> +static int qmc5883l_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct qmc5883l_data *data = iio_priv(indio_dev);
> + int odr, range;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
Since the format for this is set to IIO_VAL_INT, val2 will always be 0 and can
be dropped. Then we can turn this into a switch statemnt.
> + if (val == 10 && val2 == 0)
> + odr = QMC5883L_ODR_10HZ;
> + else if (val == 50 && val2 == 0)
> + odr = QMC5883L_ODR_50HZ;
> + else if (val == 100 && val2 == 0)
> + odr = QMC5883L_ODR_100HZ;
> + else if (val == 200 && val2 == 0)
> + odr = QMC5883L_ODR_200HZ;
> + else
> + return -EINVAL;
> +
> + return qmc5883l_set_odr(data, odr);
> + case IIO_CHAN_INFO_SCALE:
If scale is always an integer value, then we should set that in
qmc5883l_write_raw_get_fmt() and we don't have to check val2 here either.
> + if (val == 2 && val2 == 0)
> + range = QMC5883L_RNG_2G;
> + else if (val == 8 && val2 == 0)
> + range = QMC5883L_RNG_8G;
> + else
> + return -EINVAL;
> +
> + return qmc5883l_set_rng(data, range << QMC5883L_RNG_SHIFT);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int qmc5883l_write_raw_get_fmt(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, long mask)
> +{
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + return IIO_VAL_INT_PLUS_NANO;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> static irqreturn_t qmc5883l_trigger_handler(int irq, void *p)
> {
> struct iio_poll_func *pf = p;
> @@ -321,6 +577,7 @@ static irqreturn_t qmc5883l_trigger_handler(int irq, void *p)
> .storagebits = 16, \
> .endianness = IIO_LE, \
> }, \
> + .ext_info = qmc5883l_ext_info, \
> }
>
> static const struct iio_chan_spec qmc5883l_channels[] = {
> @@ -337,6 +594,18 @@ static const struct iio_chan_spec qmc5883l_channels[] = {
> IIO_CHAN_SOFT_TIMESTAMP(3),
> };
>
> +static struct attribute *qmc5883l_attributes[] = {
> + &iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> + &iio_dev_attr_scale_available.dev_attr.attr,
> + &iio_dev_attr_data_ready.dev_attr.attr,
> + &iio_dev_attr_overflow.dev_attr.attr,
> + NULL
> +};
> +
> +static const struct attribute_group qmc5883l_attribute_group = {
> + .attrs = qmc5883l_attributes,
> +};
> +
> static int qmc5883l_init(struct qmc5883l_data *data)
> {
> int ret;
> @@ -382,7 +651,10 @@ static int qmc5883l_init(struct qmc5883l_data *data)
> }
>
> static const struct iio_info qmc5883l_info = {
> + .attrs = &qmc5883l_attribute_group,
> .read_raw = &qmc5883l_read_raw,
We can implement .read_avail here to avoid needing custom _available attributes.
> + .write_raw = &qmc5883l_write_raw,
> + .write_raw_get_fmt = &qmc5883l_write_raw_get_fmt,
> };
>
> static const unsigned long qmc5883l_scan_masks[] = {0x7, 0};
> --
> 2.39.5
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 4/5] iio: magnetometer: qmc5883l: add extended sysfs attributes and configuration options
2025-05-08 12:08 ` [PATCH v2 4/5] iio: magnetometer: qmc5883l: add extended sysfs attributes and configuration options Brajesh Patil
2025-05-08 16:18 ` David Lechner
@ 2025-05-11 15:33 ` Jonathan Cameron
1 sibling, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2025-05-11 15:33 UTC (permalink / raw)
To: Brajesh Patil; +Cc: lars, linux-iio, linux-kernel, marcelo.schmitt1, dlechner
On Thu, 8 May 2025 13:08:59 +0100
Brajesh Patil <brajeshpatil11@gmail.com> wrote:
> Signed-off-by: Brajesh Patil <brajeshpatil11@gmail.com>
> ---
> drivers/iio/magnetometer/qmc5883l.c | 276 +++++++++++++++++++++++++++-
> 1 file changed, 274 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/magnetometer/qmc5883l.c b/drivers/iio/magnetometer/qmc5883l.c
> index 68597cdd0ca8..07c65f193def 100644
> --- a/drivers/iio/magnetometer/qmc5883l.c
> +++ b/drivers/iio/magnetometer/qmc5883l.c
> @@ -54,6 +54,20 @@
> #define QMC5883L_OSR_MASK 0xC0
> #define QMC5883L_OSR_SHIFT 6
>
> +static const char *const qmc5883l_modes[] = {
> + "standby", "continuous"
We try to tie modes like this to what sort of read paterns
we are setting. Can be done with runtime pm or difference between buffered
support and sysfs accesses but we almost never expose the control to userspace
directly.
> +};
> +
> +static const int qmc5883l_odr_avail[][2] = {
> + {10, 0}, {50, 0}, {100, 0}, {200, 0}
{ 10 , 0 }, etc and trailing comma as not inherent to that last
value that it is a terminator or anything like that.
> +};
> +
> +static const char *const qmc5883l_osr_avail[] = {
> + "512", "256", "128", "64"
> +};
> +
> +static const int qmc5883l_scale_avail[] = {2, 8};
{ 2, 8, }
> +
> static const int qmc5883l_odr_map[] = {
> [QMC5883L_ODR_10HZ] = 10,
> [QMC5883L_ODR_50HZ] = 50,
> @@ -82,6 +96,12 @@ struct qmc5883l_data {
>
> static int qmc5883l_init(struct qmc5883l_data *data);
> static int qmc5883l_set_mode(struct qmc5883l_data *data, unsigned int mode);
> +static ssize_t qmc5883l_show_odr_avail(struct device *dev,
> + struct device_attribute *attr, char *buf);
> +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);
>
Generally reorganize your code to avoid need for forwards declarations.
>
> +static const struct iio_enum qmc5883l_mode_enum = {
> + .items = qmc5883l_modes,
> + .num_items = ARRAY_SIZE(qmc5883l_modes),
> + .get = qmc5883l_get_mode_iio,
> + .set = qmc5883l_set_mode_iio,
> +};
> +
> +static const struct iio_enum qmc5883l_osr_enum = {
> + .items = qmc5883l_osr_avail,
> + .num_items = ARRAY_SIZE(qmc5883l_osr_avail),
> + .get = qmc5883l_get_osr_iio,
> + .set = qmc5883l_set_osr_iio,
> +};
> +
> +static const struct iio_chan_spec_ext_info qmc5883l_ext_info[] = {
> + IIO_ENUM("mode", IIO_SHARED_BY_TYPE, &qmc5883l_mode_enum),
Whilst I haven't yet looked at details here, mode attributes are basically unusable by
generic software. If at all possible hid that in the driver by auto switching
modes as appropriate to what userspace is asking you to do.
> + IIO_ENUM_AVAILABLE("mode", IIO_SHARED_BY_TYPE, &qmc5883l_mode_enum),
> + IIO_ENUM("oversampling_ratio", IIO_SHARED_BY_TYPE, &qmc5883l_osr_enum),
standard ABI, so do that the normal way.
> + IIO_ENUM_AVAILABLE("oversampling_ratio", IIO_SHARED_BY_TYPE, &qmc5883l_osr_enum),
> + { }
> +};
> +
> +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 int qmc5883l_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct qmc5883l_data *data = iio_priv(indio_dev);
> + int odr, range;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + if (val == 10 && val2 == 0)
If val2 == 0 is required for all. Check that once only.
> + odr = QMC5883L_ODR_10HZ;
> + else if (val == 50 && val2 == 0)
> + odr = QMC5883L_ODR_50HZ;
> + else if (val == 100 && val2 == 0)
> + odr = QMC5883L_ODR_100HZ;
> + else if (val == 200 && val2 == 0)
> + odr = QMC5883L_ODR_200HZ;
> + else
> + return -EINVAL;
> +
> + return qmc5883l_set_odr(data, odr);
> + case IIO_CHAN_INFO_SCALE:
> + if (val == 2 && val2 == 0)
> + range = QMC5883L_RNG_2G;
> + else if (val == 8 && val2 == 0)
> + range = QMC5883L_RNG_8G;
> + else
> + return -EINVAL;
> +
> + return qmc5883l_set_rng(data, range << QMC5883L_RNG_SHIFT);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int qmc5883l_write_raw_get_fmt(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, long mask)
> +{
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + return IIO_VAL_INT_PLUS_NANO;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> static irqreturn_t qmc5883l_trigger_handler(int irq, void *p)
> {
> struct iio_poll_func *pf = p;
> @@ -321,6 +577,7 @@ static irqreturn_t qmc5883l_trigger_handler(int irq, void *p)
> .storagebits = 16, \
> .endianness = IIO_LE, \
> }, \
> + .ext_info = qmc5883l_ext_info, \
> }
>
> static const struct iio_chan_spec qmc5883l_channels[] = {
> @@ -337,6 +594,18 @@ static const struct iio_chan_spec qmc5883l_channels[] = {
> IIO_CHAN_SOFT_TIMESTAMP(3),
> };
>
> +static struct attribute *qmc5883l_attributes[] = {
> + &iio_dev_attr_sampling_frequency_available.dev_attr.attr,
Use the get_avail() callback and standard core handlign for this.
> + &iio_dev_attr_scale_available.dev_attr.attr,
and this.
> + &iio_dev_attr_data_ready.dev_attr.attr,
> + &iio_dev_attr_overflow.dev_attr.attr,
Seconding what David said about custom ABI. Fit with normal ABI where possible
and think very very hard about whether additional ABI is needed or whether you
can set the relevant stuff internally.
For example data_ready isn't something we'd normally expose to userspace
directly at all.
> + NULL
> +};
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 5/5] iio: magnetometer: qmc5883l: add mount matrix, control features and power management
2025-05-08 12:08 [PATCH v2 3/5] iio: magnetometer: qmc5883l: Add initial driver support Brajesh Patil
2025-05-08 12:08 ` [PATCH v2 4/5] iio: magnetometer: qmc5883l: add extended sysfs attributes and configuration options Brajesh Patil
@ 2025-05-08 12:09 ` Brajesh Patil
2025-05-08 16:24 ` David Lechner
2025-05-08 16:03 ` [PATCH v2 3/5] iio: magnetometer: qmc5883l: Add initial driver support David Lechner
2 siblings, 1 reply; 8+ messages in thread
From: Brajesh Patil @ 2025-05-08 12:09 UTC (permalink / raw)
To: jic23, lars
Cc: linux-iio, linux-kernel, marcelo.schmitt1, dlechner,
Brajesh Patil
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);
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);
+}
+
+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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v2 5/5] iio: magnetometer: qmc5883l: add mount matrix, control features and power management
2025-05-08 12:09 ` [PATCH v2 5/5] iio: magnetometer: qmc5883l: add mount matrix, control features and power management Brajesh Patil
@ 2025-05-08 16:24 ` David Lechner
0 siblings, 0 replies; 8+ messages in thread
From: David Lechner @ 2025-05-08 16:24 UTC (permalink / raw)
To: Brajesh Patil, jic23, lars; +Cc: linux-iio, linux-kernel, marcelo.schmitt1
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 3/5] iio: magnetometer: qmc5883l: Add initial driver support
2025-05-08 12:08 [PATCH v2 3/5] iio: magnetometer: qmc5883l: Add initial driver support Brajesh Patil
2025-05-08 12:08 ` [PATCH v2 4/5] iio: magnetometer: qmc5883l: add extended sysfs attributes and configuration options Brajesh Patil
2025-05-08 12:09 ` [PATCH v2 5/5] iio: magnetometer: qmc5883l: add mount matrix, control features and power management Brajesh Patil
@ 2025-05-08 16:03 ` David Lechner
2025-05-11 15:26 ` Jonathan Cameron
2 siblings, 1 reply; 8+ messages in thread
From: David Lechner @ 2025-05-08 16:03 UTC (permalink / raw)
To: Brajesh Patil, jic23, lars; +Cc: linux-iio, linux-kernel, marcelo.schmitt1
On 5/8/25 7:08 AM, Brajesh Patil wrote:
This needs a description that explains why we would want to add this to the
kernel.
> Signed-off-by: Brajesh Patil <brajeshpatil11@gmail.com>
> ---
> drivers/iio/magnetometer/Kconfig | 13 +
> drivers/iio/magnetometer/Makefile | 2 +
> drivers/iio/magnetometer/qmc5883l.c | 471 ++++++++++++++++++++++++++++
> 3 files changed, 486 insertions(+)
> create mode 100644 drivers/iio/magnetometer/qmc5883l.c
>
> diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
> index 3debf1320ad1..97f375c75ff8 100644
> --- a/drivers/iio/magnetometer/Kconfig
> +++ b/drivers/iio/magnetometer/Kconfig
> @@ -206,6 +206,19 @@ config SENSORS_HMC5843_SPI
> - hmc5843_core (core functions)
> - hmc5843_spi (support for HMC5983)
>
> +config SENSORS_QMC5883L
> + tristate "QST QMC5883L 3-Axis Magnetometer"
> + depends on I2C
> + select REGMAP_I2C
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + help
> + Say Y here to build support for the QST QMC5883L 3-axis magnetometer
> + through its I2C interface.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called qmc5883l.
> +
> config SENSORS_RM3100
> tristate
> select IIO_BUFFER
> diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
> index 9297723a97d8..f51e7595f5e3 100644
> --- a/drivers/iio/magnetometer/Makefile
> +++ b/drivers/iio/magnetometer/Makefile
> @@ -27,6 +27,8 @@ obj-$(CONFIG_SENSORS_HMC5843) += hmc5843_core.o
> obj-$(CONFIG_SENSORS_HMC5843_I2C) += hmc5843_i2c.o
> obj-$(CONFIG_SENSORS_HMC5843_SPI) += hmc5843_spi.o
>
> +obj-$(CONFIG_SENSORS_QMC5883L) += qmc5883l.c
> +
> obj-$(CONFIG_SENSORS_RM3100) += rm3100-core.o
> obj-$(CONFIG_SENSORS_RM3100_I2C) += rm3100-i2c.o
> obj-$(CONFIG_SENSORS_RM3100_SPI) += rm3100-spi.o
> diff --git a/drivers/iio/magnetometer/qmc5883l.c b/drivers/iio/magnetometer/qmc5883l.c
> new file mode 100644
> index 000000000000..68597cdd0ca8
> --- /dev/null
> +++ b/drivers/iio/magnetometer/qmc5883l.c
> @@ -0,0 +1,471 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +
> +/* Register Addresses */
> +#define QMC5883L_DATA_OUT_LSB_REG 0x00
> +#define QMC5883L_STATUS_REG 0x06
> +#define QMC5883L_TEMP_OUT_LSB_REG 0x07
> +#define QMC5883L_CONTROL_REG_1 0x09
> +#define QMC5883L_CONTROL_REG_2 0x0A
> +#define QMC5883L_FBR_REG 0x0B
> +#define QMC5883L_CHIP_ID_REG 0x0D
> +#define QMC5883L_CHIP_ID 0xFF
Usually, we add _REG_ in these names to make it obvious that it is a register
address.
> +
> +/* Status Register Bits */
> +#define QMC5883L_DRDY 0x01
> +#define QMC5883L_OVL 0x02
> +#define QMC5883L_DOR 0x04
Should use the BIT() macro for these.
> +
> +/* Control Register 1 Configuration Bits */
> +/* Mode (bits [1:0]) */
> +#define QMC5883L_MODE_STANDBY 0x00
> +#define QMC5883L_MODE_CONT 0x01
> +#define QMC5883L_MODE_MASK 0x03
Use GENMASK(1, 0) insted of 0x3.
> +#define QMC5883L_MODE_SHIFT 0
In the driver, we can use FIELD_PREP() with the _MASK value, then we don't need
any of the _SHIFT macros.
> +
> +/* Output Data Rate - ODR (bits [3:2]) */
> +#define QMC5883L_ODR_10HZ 0x00
> +#define QMC5883L_ODR_50HZ 0x01
> +#define QMC5883L_ODR_100HZ 0x02
> +#define QMC5883L_ODR_200HZ 0x03
> +#define QMC5883L_ODR_MASK 0x0C
> +#define QMC5883L_ODR_SHIFT 2
> +
> +/* Full Scale Range - RNG (bits [5:4]) */
> +#define QMC5883L_RNG_2G 0x00
> +#define QMC5883L_RNG_8G 0x01
> +#define QMC5883L_RNG_MASK 0x30
> +#define QMC5883L_RNG_SHIFT 4
> +
> +/* Oversampling Ratio - OSR (bits [7:6]) */
> +#define QMC5883L_OSR_512 0x00
> +#define QMC5883L_OSR_256 0x01
> +#define QMC5883L_OSR_128 0x02
> +#define QMC5883L_OSR_64 0x03
> +#define QMC5883L_OSR_MASK 0xC0
> +#define QMC5883L_OSR_SHIFT 6
Same comment applies to these, we can use GENMASK(), then we don't need the
"(bits [A:B])" in the comment and we don't need the _SHIFT macros.
> +
> +static const int qmc5883l_odr_map[] = {
> + [QMC5883L_ODR_10HZ] = 10,
> + [QMC5883L_ODR_50HZ] = 50,
> + [QMC5883L_ODR_100HZ] = 100,
> + [QMC5883L_ODR_200HZ] = 200,
> +};
> +
> +/**
> + * struct qmc5883l_data - device instance specific data
> + * @client: I2C client structure
> + * @lock: mutex to protect register access
> + * @regmap: register map of the device
> + * @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 {
> + __le16 chans[3];
> +
> + s64 timestamp __aligned(8);
aligned_s64 timestamp;
> + } scan;
> +};
> +
> +static int qmc5883l_init(struct qmc5883l_data *data);
> +static int qmc5883l_set_mode(struct qmc5883l_data *data, unsigned int mode);
Can we reorder things to avoid the forward declarations?
> +
> +static int qmc5883l_buffer_preenable(struct iio_dev *indio_dev)
> +{
> + struct qmc5883l_data *data = iio_priv(indio_dev);
> +
> + return qmc5883l_set_mode(data, QMC5883L_MODE_CONT);
> +}
> +
> +static int qmc5883l_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> + struct qmc5883l_data *data = iio_priv(indio_dev);
> +
> + return qmc5883l_set_mode(data, QMC5883L_MODE_STANDBY);
> +}
> +
> +static const struct iio_buffer_setup_ops qmc5883l_buffer_setup_ops = {
> + .preenable = qmc5883l_buffer_preenable,
> + .postdisable = qmc5883l_buffer_postdisable,
> +};
> +
> +/* Register map access tables */
> +static const struct regmap_range qmc5883l_readable_ranges[] = {
> + regmap_reg_range(QMC5883L_DATA_OUT_LSB_REG, QMC5883L_CHIP_ID_REG),
> +};
> +
> +static const struct regmap_access_table qmc5883l_readable_table = {
> + .yes_ranges = qmc5883l_readable_ranges,
> + .n_yes_ranges = ARRAY_SIZE(qmc5883l_readable_ranges),
> +};
> +
> +static const struct regmap_range qmc5883l_writable_ranges[] = {
> + regmap_reg_range(QMC5883L_CONTROL_REG_1, QMC5883L_FBR_REG),
> +};
> +
> +static const struct regmap_access_table qmc5883l_writable_table = {
> + .yes_ranges = qmc5883l_writable_ranges,
> + .n_yes_ranges = ARRAY_SIZE(qmc5883l_writable_ranges),
> +};
> +
> +static const struct regmap_range qmc5883l_volatile_ranges[] = {
> + regmap_reg_range(QMC5883L_DATA_OUT_LSB_REG, QMC5883L_TEMP_OUT_LSB_REG + 1),
> +};
> +
> +static const struct regmap_access_table qmc5883l_volatile_table = {
> + .yes_ranges = qmc5883l_volatile_ranges,
> + .n_yes_ranges = ARRAY_SIZE(qmc5883l_volatile_ranges),
> +};
> +
> +static const struct regmap_config qmc5883l_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = QMC5883L_CHIP_ID_REG,
> +
> + .rd_table = &qmc5883l_readable_table,
> + .wr_table = &qmc5883l_writable_table,
> + .volatile_table = &qmc5883l_volatile_table,
> +
> + .cache_type = REGCACHE_RBTREE,
Regmap docs say:
"Any new caches should usually use the maple tree cache unless they specifically
require that there are never any allocations at runtime and can't provide
defaults in which case they should use the flat cache."
So why using RBTREE?
> +};
> +
> +static int qmc5883l_set_mode(struct qmc5883l_data *data, unsigned int mode)
> +{
> + int ret;
> +
> + mutex_lock(&data->lock);
> + ret = regmap_update_bits(data->regmap, QMC5883L_CONTROL_REG_1,
> + QMC5883L_MODE_MASK, mode << QMC5883L_MODE_SHIFT);
> + mutex_unlock(&data->lock);
> +
> + return ret;
> +}
> +
> +static int qmc5883l_wait_measurement(struct qmc5883l_data *data)
> +{
> + int tries = 150;
> + unsigned int val;
> + int ret;
> +
> + while (tries-- > 0) {
> + ret = regmap_read(data->regmap, QMC5883L_STATUS_REG, &val);
> + if (ret < 0)
> + return ret;
> +
> + if (val & QMC5883L_OVL) {
> + dev_err(&data->client->dev, "data overflow\n");
> + return -EOVERFLOW;
> + }
> +
> + if (val & QMC5883L_DRDY)
> + return 0;
> + usleep_range(5000, 6000);
fsleep()
> + }
> +
> + dev_err(&data->client->dev, "data not ready\n");
> + return -EIO;
Would -ETIMEDOUT be more appropriate?
> +}
> +
> +static int qmc5883l_read_measurement(struct qmc5883l_data *data,
> + int idx, int *val)
> +{
> + __le16 values[3];
> + int ret;
> +
> + ret = qmc5883l_set_mode(data, QMC5883L_MODE_CONT);
> + if (ret < 0)
> + return ret;
> +
> + ret = qmc5883l_wait_measurement(data);
> + if (ret < 0) {
> + qmc5883l_set_mode(data, QMC5883L_MODE_STANDBY);
> + return ret;
> + }
> +
> + mutex_lock(&data->lock);
Lock isn't protecting all register access.
> + ret = regmap_bulk_read(data->regmap, QMC5883L_DATA_OUT_LSB_REG,
> + values, sizeof(values));
> + mutex_unlock(&data->lock);
> +
> + qmc5883l_set_mode(data, QMC5883L_MODE_STANDBY);
> + if (ret < 0)
> + return ret;
> +
> + *val = sign_extend32(le16_to_cpu(values[idx]), 15);
> + return IIO_VAL_INT;
> +}
> +
> +static int qmc5883l_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val, int *val2, long mask)
> +{
> + struct qmc5883l_data *data = iio_priv(indio_dev);
> + unsigned int rval;
> + __le16 temp_val;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + if (chan->type == IIO_TEMP) {
> + ret = qmc5883l_set_mode(data, QMC5883L_MODE_CONT);
> + if (ret < 0)
> + return ret;
> +
> + mutex_lock(&data->lock);
> + ret = regmap_bulk_read(data->regmap, QMC5883L_TEMP_OUT_LSB_REG,
> + &temp_val, sizeof(temp_val));
> + mutex_unlock(&data->lock);
> +
> + qmc5883l_set_mode(data, QMC5883L_MODE_STANDBY);
If someone did a raw read during a buffered read, this would take it out of
standby and the buffered read would stop working, no?
Using iio_device_claim_direct() can solve this by not allowing raw read during
buffered read. I think we could probably drop the mutex lock entirely from the
driver and just rely on iio_device_claim_direct() instead.
> +
> + if (!ret)
> + *val = sign_extend32(le16_to_cpu(temp_val), 15);
> +
> + return ret ? ret : IIO_VAL_INT;
> + }
> + return qmc5883l_read_measurement(data, chan->scan_index, val);
> + case IIO_CHAN_INFO_SCALE:
> + if (chan->type == IIO_TEMP) {
> + /* scale = 124 / 10000 = 0.0124 °C/LSB */
> + *val = 124;
> + *val2 = 10000;
> + return IIO_VAL_FRACTIONAL;
> + }
> + ret = regmap_read(data->regmap, QMC5883L_CONTROL_REG_1, &rval);
> + if (ret < 0)
> + return ret;
> + rval = (rval & QMC5883L_RNG_MASK) >> QMC5883L_RNG_SHIFT;
FIELD_GET() will simplify this.
> + *val = (rval == 0) ? 12000 : 3000; /* ±2G:12000, ±8G:3000 LSB/G */
> + *val2 = 0;
Don't need to set val2.
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_OFFSET:
> + if (chan->type == IIO_TEMP) {
> + /* offset = 287661 / 100 = 2876.61 °C */
> + *val = 287661;
> + *val2 = 100;
> + return IIO_VAL_FRACTIONAL;
> + }
> + return -EINVAL;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + ret = regmap_read(data->regmap, QMC5883L_CONTROL_REG_1, &rval);
> + if (ret < 0)
> + return ret;
> +
> + rval = (rval & QMC5883L_ODR_MASK) >> QMC5883L_ODR_SHIFT;
> +
> + if (rval >= ARRAY_SIZE(qmc5883l_odr_map) || !qmc5883l_odr_map[rval])
> + return -EINVAL;
> +
> + *val = qmc5883l_odr_map[rval];
> + *val2 = 0;
> + return IIO_VAL_INT;
> + }
> + return -EINVAL;
> +}
> +
> +static irqreturn_t qmc5883l_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct qmc5883l_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + mutex_lock(&data->lock);
If we do end up keeping the mutex lock, guard(mutex)(&data->lock); will help
simplify things a bit in many of the functions like this.
> + ret = qmc5883l_wait_measurement(data);
> + if (ret < 0) {
> + mutex_unlock(&data->lock);
> + goto done;
> + }
> +
> + ret = regmap_bulk_read(data->regmap, QMC5883L_DATA_OUT_LSB_REG,
> + data->scan.chans, sizeof(data->scan.chans));
> + mutex_unlock(&data->lock);
> +
> + if (ret < 0)
> + goto done;
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
> + iio_get_time_ns(indio_dev));
> +
> +done:
> + iio_trigger_notify_done(indio_dev->trig);
> + return IRQ_HANDLED;
> +}
> +
> +/* Channel definitions */
> +#define QMC5883L_CHANNEL(axis, idx) \
> +{ \
> + .type = IIO_MAGN, \
> + .modified = 1, \
> + .channel2 = IIO_MOD_##axis, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_type = \
> + BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> + .scan_index = idx, \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 16, \
> + .storagebits = 16, \
> + .endianness = IIO_LE, \
> + }, \
> +}
Use tabs so that all of the \ line up nicely on the right.
> +
> +static const struct iio_chan_spec qmc5883l_channels[] = {
> + QMC5883L_CHANNEL(X, 0),
> + QMC5883L_CHANNEL(Y, 1),
> + QMC5883L_CHANNEL(Z, 2),
> + {
> + .type = IIO_TEMP,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_OFFSET),
> + .scan_index = -1,
> + },
> + IIO_CHAN_SOFT_TIMESTAMP(3),
> +};
> +
> +static int qmc5883l_init(struct qmc5883l_data *data)
> +{
> + int ret;
> + u8 chip_id;
> + unsigned int chip_id_tmp;
> + unsigned int ctrl1;
> +
> + ret = regmap_read(data->regmap, QMC5883L_CHIP_ID_REG, &chip_id_tmp);
> + if (ret < 0) {
> + dev_err(&data->client->dev, "Failed to read chip ID\n");
> + return ret;
This is during probe, so return dev_err_probe() can be used here and for other
error returns in this function.
> + }
> +
> + chip_id = (u8)chip_id_tmp;
> + if (chip_id != QMC5883L_CHIP_ID) {
> + dev_err(&data->client->dev, "Invalid chip ID: 0x%02X (expected 0x%02X)\n",
> + chip_id, QMC5883L_CHIP_ID);
> + return -ENODEV;
Usually, we don't consider a wrong ID an error though. There may be some
comaptible chip that can use this driver but has a differet ID. Could change
this to info or warning.
> + }
> +
> + mutex_lock(&data->lock);
This happens during probe, so mutex isn't needed here.
> + ret = regmap_write(data->regmap, QMC5883L_FBR_REG, 0x01);
> + if (ret < 0)
> + goto unlock;
> +
> + ctrl1 = (QMC5883L_OSR_64 << QMC5883L_OSR_SHIFT) |
> + (QMC5883L_RNG_2G << QMC5883L_RNG_SHIFT) |
> + (QMC5883L_ODR_50HZ << QMC5883L_ODR_SHIFT) |
> + (QMC5883L_MODE_STANDBY << QMC5883L_MODE_SHIFT);
As mentioned above, we can use FIELD_PREP() for these.
> +
> + ret = regmap_write(data->regmap, QMC5883L_CONTROL_REG_1, ctrl1);
> + if (ret < 0)
> + goto unlock;
> +
> + mutex_unlock(&data->lock);
> + dev_dbg(&data->client->dev,
> + "Initialized with OSR=64, RNG=2G, ODR=50Hz, Mode=Standby\n");
> + return 0;
> +
> +unlock:
> + mutex_unlock(&data->lock);
> + return ret;
> +}
> +
> +static const struct iio_info qmc5883l_info = {
> + .read_raw = &qmc5883l_read_raw,
> +};
> +
> +static const unsigned long qmc5883l_scan_masks[] = {0x7, 0};
> +
> +static int qmc5883l_probe(struct i2c_client *client)
> +{
> + struct regmap *regmap;
> + struct qmc5883l_data *data;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + regmap = devm_regmap_init_i2c(client, &qmc5883l_regmap_config);
> + if (IS_ERR(regmap)) {
> + dev_err(&client->dev, "Failed to initialize regmap\n");
> + return PTR_ERR(regmap);
> + }
All error return paths can be simplified with return dev_err_probe().
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> + if (!indio_dev) {
> + dev_err(&client->dev, "Failed to allocate iio device\n");
> + return -ENOMEM;
> + }
> +
> + data = iio_priv(indio_dev);
> + data->client = client;
> + data->regmap = regmap;
> + mutex_init(&data->lock);
> +
> + indio_dev->name = "qmc5883l";
> + indio_dev->info = &qmc5883l_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = qmc5883l_channels;
> + indio_dev->num_channels = ARRAY_SIZE(qmc5883l_channels);
> + indio_dev->available_scan_masks = qmc5883l_scan_masks;
> +
> + 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;
> + }
> +
> + ret = qmc5883l_init(data);
> + if (ret < 0) {
> + dev_err(&client->dev, "Failed to initialize device: %d\n", ret);
> + return ret;
> + }
> +
> + ret = devm_iio_device_register(&client->dev, indio_dev);
> + if (ret < 0) {
> + dev_err(&client->dev, "Failed to register IIO device: %d\n", ret);
> + return ret;
> + }
> +
> + i2c_set_clientdata(client, indio_dev);
Is this actually needed if there is no i2c_get_clientdata()?
> + return 0;
> +}
> +
> +static const struct i2c_device_id qmc5883l_id[] = {
> + { "qmc5883l", 0 },
Can leave out the 0.
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, qmc5883l_id);
> +
> +static const struct of_device_id qmc5883l_of_match[] = {
> + { .compatible = "qst,qmc5883l" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, qmc5883l_of_match);
> +
> +static struct i2c_driver qmc5883l_driver = {
> + .driver = {
> + .name = "qmc5883l",
> + .of_match_table = qmc5883l_of_match,
> + },
> + .id_table = qmc5883l_id,
> + .probe = qmc5883l_probe,
> +};
> +
> +module_i2c_driver(qmc5883l_driver);
> +
> +MODULE_AUTHOR("Brajesh Patil <brajeshpatil11@gmail.com>");
> +MODULE_DESCRIPTION("QMC5883L Driver");
> +MODULE_LICENSE("GPL");
> +
> --
> 2.39.5
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2 3/5] iio: magnetometer: qmc5883l: Add initial driver support
2025-05-08 16:03 ` [PATCH v2 3/5] iio: magnetometer: qmc5883l: Add initial driver support David Lechner
@ 2025-05-11 15:26 ` Jonathan Cameron
0 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2025-05-11 15:26 UTC (permalink / raw)
To: David Lechner
Cc: Brajesh Patil, lars, linux-iio, linux-kernel, marcelo.schmitt1
On Thu, 8 May 2025 11:03:51 -0500
David Lechner <dlechner@baylibre.com> wrote:
> On 5/8/25 7:08 AM, Brajesh Patil wrote:
>
> This needs a description that explains why we would want to add this to the
> kernel.
>
> > Signed-off-by: Brajesh Patil <brajeshpatil11@gmail.com>
A jumped on top of David's review (note I cropped lots of good feedback so do
look at his reply!) and added a few things.
J
> > diff --git a/drivers/iio/magnetometer/qmc5883l.c b/drivers/iio/magnetometer/qmc5883l.c
> > new file mode 100644
> > index 000000000000..68597cdd0ca8
> > --- /dev/null
> > +++ b/drivers/iio/magnetometer/qmc5883l.c
> > @@ -0,0 +1,471 @@
> > + case IIO_CHAN_INFO_SAMP_FREQ:
> > + ret = regmap_read(data->regmap, QMC5883L_CONTROL_REG_1, &rval);
> > + if (ret < 0)
> > + return ret;
> > +
> > + rval = (rval & QMC5883L_ODR_MASK) >> QMC5883L_ODR_SHIFT;
> > +
> > + if (rval >= ARRAY_SIZE(qmc5883l_odr_map) || !qmc5883l_odr_map[rval])
> > + return -EINVAL;
> > +
> > + *val = qmc5883l_odr_map[rval];
> > + *val2 = 0;
> > + return IIO_VAL_INT;
> > + }
> > + return -EINVAL;
Probably can't get here - which is good, but if you can't drop this line.
> > +}
> > +static const struct iio_info qmc5883l_info = {
> > + .read_raw = &qmc5883l_read_raw,
> > +};
> > +
> > +static const unsigned long qmc5883l_scan_masks[] = {0x7, 0};
static const unsigned long qmc5883l_scan_masks[] = { 0x7, 0 };
> > +
> > +static int qmc5883l_probe(struct i2c_client *client)
> > +{
...
> > +
> > + data = iio_priv(indio_dev);
> > + data->client = client;
> > + data->regmap = regmap;
> > + mutex_init(&data->lock);
> > +
ret = devm_mutex_init(&data->lock);
if (ret)
return ret;
Cleaning up mutexes only does stuff in debug modes, so traditionally we didn't
bother but now we have devm_ handling it is a nice to have thing.
^ permalink raw reply [flat|nested] 8+ messages in thread