Re: [PATCH v3 1/8] iio:core: add a callback to allow drivers to provide _available attributes
On Nov 07, 2016 12:57, Peter Rosin wrote: > On 2016-11-07 12:37, Daniel Baluta wrote: > > On Mon, Oct 24, 2016 at 1:39 AM, Peter Rosin wrote: > >> From: Jonathan Cameron > >> > >> A large number of attributes can only take a limited range of values. > >> Currently in IIO this is handled by directly registering additional > >> *_available attributes thus providing this information to userspace. > >> > >> It is desirable to provide this information via the core for much the same > >> reason this was done for the actual channel information attributes in the > >> first place. If it isn't there, then it can only really be accessed from > >> userspace. Other in kernel IIO consumers have no access to what valid > >> parameters are. > >> > >> Two forms are currently supported: > >> * list of values in one particular IIO_VAL_* format. > >> e.g. 1.30 1.50 1.73 > >> * range specification with a step size: > >> e.g. [1.00 0.50 2.50] > >> equivalent to 1.00 1.500 2.00 2.50 > > > > Is there any driver using this format? :) > > Yes, soon. Hopefully. See patch 3/8 > iio: mcp4531: provide range of available raw values > https://patchwork.kernel.org/patch/9391283/ I would also like to add this to mcp4131.c and ds1803.c. -- Slawomir Stepien
Re: [PATCH v3 1/8] iio:core: add a callback to allow drivers to provide _available attributes
On 2016-11-07 12:37, Daniel Baluta wrote: > On Mon, Oct 24, 2016 at 1:39 AM, Peter Rosin wrote: >> From: Jonathan Cameron >> >> A large number of attributes can only take a limited range of values. >> Currently in IIO this is handled by directly registering additional >> *_available attributes thus providing this information to userspace. >> >> It is desirable to provide this information via the core for much the same >> reason this was done for the actual channel information attributes in the >> first place. If it isn't there, then it can only really be accessed from >> userspace. Other in kernel IIO consumers have no access to what valid >> parameters are. >> >> Two forms are currently supported: >> * list of values in one particular IIO_VAL_* format. >> e.g. 1.30 1.50 1.73 >> * range specification with a step size: >> e.g. [1.00 0.50 2.50] >> equivalent to 1.00 1.500 2.00 2.50 > > Is there any driver using this format? :) Yes, soon. Hopefully. See patch 3/8 iio: mcp4531: provide range of available raw values https://patchwork.kernel.org/patch/9391283/ >> >> An addition set of masks are used to allow different sharing rules for the >> *_available attributes generated. >> >> This allows for example: >> >> in_accel_x_offset >> in_accel_y_offset >> in_accel_offset_available. >> >> We could have gone with having a specification for each and every >> info_mask element but that would have meant changing the existing userspace >> ABI. This approach does not. >> >> Signed-off-by: Jonathan Cameron >> [forward ported, added some docs and fixed buffer overflows /peda] >> Signed-off-by: Peter Rosin > > The patch looks good to me at a first glance. Thanks, may I add your acked-by if/when I respin? Cheers, Peter
Re: [PATCH v3 1/8] iio:core: add a callback to allow drivers to provide _available attributes
On Mon, Nov 7, 2016 at 1:57 PM, Peter Rosin wrote: > On 2016-11-07 12:37, Daniel Baluta wrote: >> On Mon, Oct 24, 2016 at 1:39 AM, Peter Rosin wrote: >>> From: Jonathan Cameron >>> >>> A large number of attributes can only take a limited range of values. >>> Currently in IIO this is handled by directly registering additional >>> *_available attributes thus providing this information to userspace. >>> >>> It is desirable to provide this information via the core for much the same >>> reason this was done for the actual channel information attributes in the >>> first place. If it isn't there, then it can only really be accessed from >>> userspace. Other in kernel IIO consumers have no access to what valid >>> parameters are. >>> >>> Two forms are currently supported: >>> * list of values in one particular IIO_VAL_* format. >>> e.g. 1.30 1.50 1.73 >>> * range specification with a step size: >>> e.g. [1.00 0.50 2.50] >>> equivalent to 1.00 1.500 2.00 2.50 >> >> Is there any driver using this format? :) > > Yes, soon. Hopefully. See patch 3/8 > iio: mcp4531: provide range of available raw values > https://patchwork.kernel.org/patch/9391283/ > >>> >>> An addition set of masks are used to allow different sharing rules for the >>> *_available attributes generated. >>> >>> This allows for example: >>> >>> in_accel_x_offset >>> in_accel_y_offset >>> in_accel_offset_available. >>> >>> We could have gone with having a specification for each and every >>> info_mask element but that would have meant changing the existing userspace >>> ABI. This approach does not. >>> >>> Signed-off-by: Jonathan Cameron >>> [forward ported, added some docs and fixed buffer overflows /peda] >>> Signed-off-by: Peter Rosin >> >> The patch looks good to me at a first glance. > > Thanks, may I add your acked-by if/when I respin? Yes. You can have it from here: Acked-by: Daniel Baluta thanks, Daniel.
Re: [PATCH v3 1/8] iio:core: add a callback to allow drivers to provide _available attributes
On Mon, Oct 24, 2016 at 1:39 AM, Peter Rosin wrote: > From: Jonathan Cameron > > A large number of attributes can only take a limited range of values. > Currently in IIO this is handled by directly registering additional > *_available attributes thus providing this information to userspace. > > It is desirable to provide this information via the core for much the same > reason this was done for the actual channel information attributes in the > first place. If it isn't there, then it can only really be accessed from > userspace. Other in kernel IIO consumers have no access to what valid > parameters are. > > Two forms are currently supported: > * list of values in one particular IIO_VAL_* format. > e.g. 1.30 1.50 1.73 > * range specification with a step size: > e.g. [1.00 0.50 2.50] > equivalent to 1.00 1.500 2.00 2.50 Is there any driver using this format? :) > > An addition set of masks are used to allow different sharing rules for the > *_available attributes generated. > > This allows for example: > > in_accel_x_offset > in_accel_y_offset > in_accel_offset_available. > > We could have gone with having a specification for each and every > info_mask element but that would have meant changing the existing userspace > ABI. This approach does not. > > Signed-off-by: Jonathan Cameron > [forward ported, added some docs and fixed buffer overflows /peda] > Signed-off-by: Peter Rosin The patch looks good to me at a first glance.
Re: [PATCH v3 1/8] iio:core: add a callback to allow drivers to provide _available attributes
On 23/10/16 23:39, Peter Rosin wrote:
> From: Jonathan Cameron
>
> A large number of attributes can only take a limited range of values.
> Currently in IIO this is handled by directly registering additional
> *_available attributes thus providing this information to userspace.
>
> It is desirable to provide this information via the core for much the same
> reason this was done for the actual channel information attributes in the
> first place. If it isn't there, then it can only really be accessed from
> userspace. Other in kernel IIO consumers have no access to what valid
> parameters are.
>
> Two forms are currently supported:
> * list of values in one particular IIO_VAL_* format.
> e.g. 1.30 1.50 1.73
> * range specification with a step size:
> e.g. [1.00 0.50 2.50]
> equivalent to 1.00 1.500 2.00 2.50
>
> An addition set of masks are used to allow different sharing rules for the
> *_available attributes generated.
>
> This allows for example:
>
> in_accel_x_offset
> in_accel_y_offset
> in_accel_offset_available.
>
> We could have gone with having a specification for each and every
> info_mask element but that would have meant changing the existing userspace
> ABI. This approach does not.
>
> Signed-off-by: Jonathan Cameron
> [forward ported, added some docs and fixed buffer overflows /peda]
> Signed-off-by: Peter Rosin
Looks nicely cleaned up to me, but I'm really not going to be the best
person to review this. So would appreciate anyone else who has the time
taking a look at this.
Thanks,
Jonathan
> ---
> drivers/iio/industrialio-core.c | 259
> +++-
> include/linux/iio/iio.h | 29 +
> include/linux/iio/types.h | 5 +
> 3 files changed, 260 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index fc340ed3dca1..b35c61a31fa6 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -575,66 +575,82 @@ int of_iio_read_mount_matrix(const struct device *dev,
> #endif
> EXPORT_SYMBOL(of_iio_read_mount_matrix);
>
> -/**
> - * iio_format_value() - Formats a IIO value into its string representation
> - * @buf: The buffer to which the formatted value gets written
> - * @type:One of the IIO_VAL_... constants. This decides how the val
> - * and val2 parameters are formatted.
> - * @size:Number of IIO value entries contained in vals
> - * @vals:Pointer to the values, exact meaning depends on the
> - * type parameter.
> - *
> - * Return: 0 by default, a negative number on failure or the
> - * total number of characters written for a type that belongs
> - * to the IIO_VAL_... constant.
> - */
> -ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals)
> +static ssize_t __iio_format_value(char *buf, size_t len, unsigned int type,
> + int size, const int *vals)
> {
> unsigned long long tmp;
> + int tmp0, tmp1;
> bool scale_db = false;
>
> switch (type) {
> case IIO_VAL_INT:
> - return sprintf(buf, "%d\n", vals[0]);
> + return snprintf(buf, len, "%d", vals[0]);
> case IIO_VAL_INT_PLUS_MICRO_DB:
> scale_db = true;
> case IIO_VAL_INT_PLUS_MICRO:
> if (vals[1] < 0)
> - return sprintf(buf, "-%d.%06u%s\n", abs(vals[0]),
> --vals[1], scale_db ? " dB" : "");
> + return snprintf(buf, len, "-%d.%06u%s", abs(vals[0]),
> + -vals[1], scale_db ? " dB" : "");
> else
> - return sprintf(buf, "%d.%06u%s\n", vals[0], vals[1],
> - scale_db ? " dB" : "");
> + return snprintf(buf, len, "%d.%06u%s", vals[0], vals[1],
> + scale_db ? " dB" : "");
> case IIO_VAL_INT_PLUS_NANO:
> if (vals[1] < 0)
> - return sprintf(buf, "-%d.%09u\n", abs(vals[0]),
> --vals[1]);
> + return snprintf(buf, len, "-%d.%09u", abs(vals[0]),
> + -vals[1]);
> else
> - return sprintf(buf, "%d.%09u\n", vals[0], vals[1]);
> + return snprintf(buf, len, "%d.%09u", vals[0], vals[1]);
> case IIO_VAL_FRACTIONAL:
> tmp = div_s64((s64)vals[0] * 10LL, vals[1]);
> - vals[0] = (int)div_s64_rem(tmp, 10, &vals[1]);
> - return sprintf(buf, "%d.%09u\n", vals[0], abs(vals[1]));
> + tmp1 = vals[1];
> + tmp0 = (int)div_s64_rem(tmp, 10, &tmp1);
> + return snprintf(buf, len, "%d.%09u", tmp0, abs(tmp1));
> case IIO_VAL_FRACTIONAL_LOG2:
> tmp = (
[PATCH v3 1/8] iio:core: add a callback to allow drivers to provide _available attributes
From: Jonathan Cameron
A large number of attributes can only take a limited range of values.
Currently in IIO this is handled by directly registering additional
*_available attributes thus providing this information to userspace.
It is desirable to provide this information via the core for much the same
reason this was done for the actual channel information attributes in the
first place. If it isn't there, then it can only really be accessed from
userspace. Other in kernel IIO consumers have no access to what valid
parameters are.
Two forms are currently supported:
* list of values in one particular IIO_VAL_* format.
e.g. 1.30 1.50 1.73
* range specification with a step size:
e.g. [1.00 0.50 2.50]
equivalent to 1.00 1.500 2.00 2.50
An addition set of masks are used to allow different sharing rules for the
*_available attributes generated.
This allows for example:
in_accel_x_offset
in_accel_y_offset
in_accel_offset_available.
We could have gone with having a specification for each and every
info_mask element but that would have meant changing the existing userspace
ABI. This approach does not.
Signed-off-by: Jonathan Cameron
[forward ported, added some docs and fixed buffer overflows /peda]
Signed-off-by: Peter Rosin
---
drivers/iio/industrialio-core.c | 259 +++-
include/linux/iio/iio.h | 29 +
include/linux/iio/types.h | 5 +
3 files changed, 260 insertions(+), 33 deletions(-)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index fc340ed3dca1..b35c61a31fa6 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -575,66 +575,82 @@ int of_iio_read_mount_matrix(const struct device *dev,
#endif
EXPORT_SYMBOL(of_iio_read_mount_matrix);
-/**
- * iio_format_value() - Formats a IIO value into its string representation
- * @buf: The buffer to which the formatted value gets written
- * @type: One of the IIO_VAL_... constants. This decides how the val
- * and val2 parameters are formatted.
- * @size: Number of IIO value entries contained in vals
- * @vals: Pointer to the values, exact meaning depends on the
- * type parameter.
- *
- * Return: 0 by default, a negative number on failure or the
- *total number of characters written for a type that belongs
- *to the IIO_VAL_... constant.
- */
-ssize_t iio_format_value(char *buf, unsigned int type, int size, int *vals)
+static ssize_t __iio_format_value(char *buf, size_t len, unsigned int type,
+ int size, const int *vals)
{
unsigned long long tmp;
+ int tmp0, tmp1;
bool scale_db = false;
switch (type) {
case IIO_VAL_INT:
- return sprintf(buf, "%d\n", vals[0]);
+ return snprintf(buf, len, "%d", vals[0]);
case IIO_VAL_INT_PLUS_MICRO_DB:
scale_db = true;
case IIO_VAL_INT_PLUS_MICRO:
if (vals[1] < 0)
- return sprintf(buf, "-%d.%06u%s\n", abs(vals[0]),
- -vals[1], scale_db ? " dB" : "");
+ return snprintf(buf, len, "-%d.%06u%s", abs(vals[0]),
+ -vals[1], scale_db ? " dB" : "");
else
- return sprintf(buf, "%d.%06u%s\n", vals[0], vals[1],
- scale_db ? " dB" : "");
+ return snprintf(buf, len, "%d.%06u%s", vals[0], vals[1],
+ scale_db ? " dB" : "");
case IIO_VAL_INT_PLUS_NANO:
if (vals[1] < 0)
- return sprintf(buf, "-%d.%09u\n", abs(vals[0]),
- -vals[1]);
+ return snprintf(buf, len, "-%d.%09u", abs(vals[0]),
+ -vals[1]);
else
- return sprintf(buf, "%d.%09u\n", vals[0], vals[1]);
+ return snprintf(buf, len, "%d.%09u", vals[0], vals[1]);
case IIO_VAL_FRACTIONAL:
tmp = div_s64((s64)vals[0] * 10LL, vals[1]);
- vals[0] = (int)div_s64_rem(tmp, 10, &vals[1]);
- return sprintf(buf, "%d.%09u\n", vals[0], abs(vals[1]));
+ tmp1 = vals[1];
+ tmp0 = (int)div_s64_rem(tmp, 10, &tmp1);
+ return snprintf(buf, len, "%d.%09u", tmp0, abs(tmp1));
case IIO_VAL_FRACTIONAL_LOG2:
tmp = (s64)vals[0] * 10LL >> vals[1];
- vals[1] = do_div(tmp, 10LL);
- vals[0] = tmp;
- return sprintf(buf, "%d.%09u\n", vals[0], vals[1]);
+ tmp1 = do_div(tmp, 10LL);
+ tmp0 = tmp;
+ return snprintf(buf, len, "%d.%09u", tmp0, tmp1);
