| 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: <20250511163310.01522d27@jic23-huawei>
Date: Sun, 11 May 2025 16:33:10 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Brajesh Patil <brajeshpatil11@...il.com>
Cc: lars@...afoo.de, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org, marcelo.schmitt1@...il.com,
dlechner@...libre.com
Subject: Re: [PATCH v2 4/5] iio: magnetometer: qmc5883l: add extended sysfs
attributes and configuration options
On Thu, 8 May 2025 13:08:59 +0100
Brajesh Patil <brajeshpatil11@...il.com> wrote:
> Signed-off-by: Brajesh Patil <brajeshpatil11@...il.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
> +};
Powered by blists - more mailing lists