Re: [PATCH v2 4/5] iio: magnetometer: qmc5883l: add extended sysfs attributes and configuration options

[David Lechner <dlechner@baylibre.com>]

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
>