This helper is similar to __sysfs_match_string() with the exception that it ignores NULL elements within the array. It takes an extra parameter (called `gaps`) which when true will ignore the NULL elements. When false, this function behaves exactly like `__sysfs_match_string()`. Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- include/linux/string.h | 9 ++++++++- lib/string.c | 14 ++++++++++---- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/include/linux/string.h b/include/linux/string.h index 7927b875f80c..30595ed483dc 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -189,7 +189,14 @@ static inline int strtobool(const char *s, bool *res) } int match_string(const char * const *array, size_t n, const char *string); -int __sysfs_match_string(const char * const *array, size_t n, const char *s); +int __sysfs_match_string_with_gaps(const char * const *array, size_t n, + const char *str, bool gaps); + +static inline int __sysfs_match_string(const char * const *array, size_t n, + const char *str) +{ + return __sysfs_match_string_with_gaps(array, n, str, false); +} /** * sysfs_match_string - matches given string in an array diff --git a/lib/string.c b/lib/string.c index 38e4ca08e757..8ddac3cd292a 100644 --- a/lib/string.c +++ b/lib/string.c @@ -659,30 +659,36 @@ int match_string(const char * const *array, size_t n, const char *string) EXPORT_SYMBOL(match_string); /** - * __sysfs_match_string - matches given string in an array + * __sysfs_match_string_with_gaps - matches string in array ignoring NULLs * @array: array of strings * @n: number of strings in the array or -1 for NULL terminated arrays * @str: string to match with + * @gaps: boolean to ignore NULL elements within the array * * Returns index of @str in the @array or -EINVAL, just like match_string(). * Uses sysfs_streq instead of strcmp for matching. + * Ignores NULL pointers within the @array if @gaps is true. */ -int __sysfs_match_string(const char * const *array, size_t n, const char *str) +int __sysfs_match_string_with_gaps(const char * const *array, size_t n, + const char *str, bool gaps) { const char *item; int index; for (index = 0; index < n; index++) { item = array[index]; - if (!item) + if (!item) { + if (gaps) + continue; break; + } if (sysfs_streq(item, str)) return index; } return -EINVAL; } -EXPORT_SYMBOL(__sysfs_match_string); +EXPORT_SYMBOL(__sysfs_match_string_with_gaps); #ifndef __HAVE_ARCH_MEMSET /** -- 2.17.1
From: Lars-Peter Clausen <lars@metafoo.de> Some enums might have gaps or reserved values in the middle of their value range. E.g. consider a 2-bit enum where the values 0, 1 and 3 have a meaning, but 2 is a reserved value and can not be used. Add support for such enums to the IIO enum helper functions. A reserved values is marked by setting its entry in the items array to NULL rather than the normal descriptive string value. Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- drivers/iio/industrialio-core.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index f2ebca65f964..c141a29bf446 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -447,8 +447,11 @@ ssize_t iio_enum_available_read(struct iio_dev *indio_dev, if (!e->num_items) return 0; - for (i = 0; i < e->num_items; ++i) + for (i = 0; i < e->num_items; ++i) { + if (!e->items[i]) + continue; len += scnprintf(buf + len, PAGE_SIZE - len, "%s ", e->items[i]); + } /* replace last space with a newline */ buf[len - 1] = '\n'; @@ -469,7 +472,7 @@ ssize_t iio_enum_read(struct iio_dev *indio_dev, i = e->get(indio_dev, chan); if (i < 0) return i; - else if (i >= e->num_items) + else if (i >= e->num_items || !e->items[i]) return -EINVAL; return snprintf(buf, PAGE_SIZE, "%s\n", e->items[i]); @@ -486,7 +489,8 @@ ssize_t iio_enum_write(struct iio_dev *indio_dev, if (!e->set) return -EINVAL; - ret = __sysfs_match_string(e->items, e->num_items, buf); + ret = __sysfs_match_string_with_gaps(e->items, e->num_items, + buf, true); if (ret < 0) return ret; -- 2.17.1
On Mon, 22 Apr 2019 11:32:56 +0300 Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > This helper is similar to __sysfs_match_string() with the exception that it > ignores NULL elements within the array. > It takes an extra parameter (called `gaps`) which when true will ignore > the NULL elements. When false, this function behaves exactly like > `__sysfs_match_string()`. > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> Hi Alex, The existence of that gaps parameter does seem a little odd. Why not just have the exposed _gaps form wrap a version that takes that parameter and always set it to true? Otherwise looks good to me. Jonathan > --- > include/linux/string.h | 9 ++++++++- > lib/string.c | 14 ++++++++++---- > 2 files changed, 18 insertions(+), 5 deletions(-) > > diff --git a/include/linux/string.h b/include/linux/string.h > index 7927b875f80c..30595ed483dc 100644 > --- a/include/linux/string.h > +++ b/include/linux/string.h > @@ -189,7 +189,14 @@ static inline int strtobool(const char *s, bool *res) > } > > int match_string(const char * const *array, size_t n, const char *string); > -int __sysfs_match_string(const char * const *array, size_t n, const char *s); > +int __sysfs_match_string_with_gaps(const char * const *array, size_t n, > + const char *str, bool gaps); > + > +static inline int __sysfs_match_string(const char * const *array, size_t n, > + const char *str) > +{ > + return __sysfs_match_string_with_gaps(array, n, str, false); > +} > > /** > * sysfs_match_string - matches given string in an array > diff --git a/lib/string.c b/lib/string.c > index 38e4ca08e757..8ddac3cd292a 100644 > --- a/lib/string.c > +++ b/lib/string.c > @@ -659,30 +659,36 @@ int match_string(const char * const *array, size_t n, const char *string) > EXPORT_SYMBOL(match_string); > > /** > - * __sysfs_match_string - matches given string in an array > + * __sysfs_match_string_with_gaps - matches string in array ignoring NULLs > * @array: array of strings > * @n: number of strings in the array or -1 for NULL terminated arrays > * @str: string to match with > + * @gaps: boolean to ignore NULL elements within the array > * > * Returns index of @str in the @array or -EINVAL, just like match_string(). > * Uses sysfs_streq instead of strcmp for matching. > + * Ignores NULL pointers within the @array if @gaps is true. > */ > -int __sysfs_match_string(const char * const *array, size_t n, const char *str) > +int __sysfs_match_string_with_gaps(const char * const *array, size_t n, > + const char *str, bool gaps) > { > const char *item; > int index; > > for (index = 0; index < n; index++) { > item = array[index]; > - if (!item) > + if (!item) { > + if (gaps) > + continue; > break; > + } > if (sysfs_streq(item, str)) > return index; > } > > return -EINVAL; > } > -EXPORT_SYMBOL(__sysfs_match_string); > +EXPORT_SYMBOL(__sysfs_match_string_with_gaps); > > #ifndef __HAVE_ARCH_MEMSET > /**
On Mon, 22 Apr 2019 11:32:57 +0300 Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > From: Lars-Peter Clausen <lars@metafoo.de> > > Some enums might have gaps or reserved values in the middle of their value > range. E.g. consider a 2-bit enum where the values 0, 1 and 3 have a > meaning, but 2 is a reserved value and can not be used. > > Add support for such enums to the IIO enum helper functions. A reserved > values is marked by setting its entry in the items array to NULL rather > than the normal descriptive string value. > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> Looks good, subject to the whole gaps parameter discussion on patch 1. Thanks, Jonathan > --- > drivers/iio/industrialio-core.c | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c > index f2ebca65f964..c141a29bf446 100644 > --- a/drivers/iio/industrialio-core.c > +++ b/drivers/iio/industrialio-core.c > @@ -447,8 +447,11 @@ ssize_t iio_enum_available_read(struct iio_dev *indio_dev, > if (!e->num_items) > return 0; > > - for (i = 0; i < e->num_items; ++i) > + for (i = 0; i < e->num_items; ++i) { > + if (!e->items[i]) > + continue; > len += scnprintf(buf + len, PAGE_SIZE - len, "%s ", e->items[i]); > + } > > /* replace last space with a newline */ > buf[len - 1] = '\n'; > @@ -469,7 +472,7 @@ ssize_t iio_enum_read(struct iio_dev *indio_dev, > i = e->get(indio_dev, chan); > if (i < 0) > return i; > - else if (i >= e->num_items) > + else if (i >= e->num_items || !e->items[i]) > return -EINVAL; > > return snprintf(buf, PAGE_SIZE, "%s\n", e->items[i]); > @@ -486,7 +489,8 @@ ssize_t iio_enum_write(struct iio_dev *indio_dev, > if (!e->set) > return -EINVAL; > > - ret = __sysfs_match_string(e->items, e->num_items, buf); > + ret = __sysfs_match_string_with_gaps(e->items, e->num_items, > + buf, true); > if (ret < 0) > return ret; >
On Mon, 2019-04-22 at 11:37 +0100, Jonathan Cameron wrote: > [External] > > > On Mon, 22 Apr 2019 11:32:56 +0300 > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > This helper is similar to __sysfs_match_string() with the exception > > that it > > ignores NULL elements within the array. > > It takes an extra parameter (called `gaps`) which when true will ignore > > the NULL elements. When false, this function behaves exactly like > > `__sysfs_match_string()`. > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > Hi Alex, > > The existence of that gaps parameter does seem a little odd. > Why not just have the exposed _gaps form wrap a version that > takes that parameter and always set it to true? > That was one way I thought about doing it. I was thinking about maybe not exporting too many symbols, since that can add some size-overhead to the kernel during linking. I'll send a V2. Thanks Alex > Otherwise looks good to me. > > Jonathan > > > --- > > include/linux/string.h | 9 ++++++++- > > lib/string.c | 14 ++++++++++---- > > 2 files changed, 18 insertions(+), 5 deletions(-) > > > > diff --git a/include/linux/string.h b/include/linux/string.h > > index 7927b875f80c..30595ed483dc 100644 > > --- a/include/linux/string.h > > +++ b/include/linux/string.h > > @@ -189,7 +189,14 @@ static inline int strtobool(const char *s, bool > > *res) > > } > > > > int match_string(const char * const *array, size_t n, const char > > *string); > > -int __sysfs_match_string(const char * const *array, size_t n, const > > char *s); > > +int __sysfs_match_string_with_gaps(const char * const *array, size_t > > n, > > + const char *str, bool gaps); > > + > > +static inline int __sysfs_match_string(const char * const *array, > > size_t n, > > + const char *str) > > +{ > > + return __sysfs_match_string_with_gaps(array, n, str, false); > > +} > > > > /** > > * sysfs_match_string - matches given string in an array > > diff --git a/lib/string.c b/lib/string.c > > index 38e4ca08e757..8ddac3cd292a 100644 > > --- a/lib/string.c > > +++ b/lib/string.c > > @@ -659,30 +659,36 @@ int match_string(const char * const *array, > > size_t n, const char *string) > > EXPORT_SYMBOL(match_string); > > > > /** > > - * __sysfs_match_string - matches given string in an array > > + * __sysfs_match_string_with_gaps - matches string in array ignoring > > NULLs > > * @array: array of strings > > * @n: number of strings in the array or -1 for NULL terminated arrays > > * @str: string to match with > > + * @gaps: boolean to ignore NULL elements within the array > > * > > * Returns index of @str in the @array or -EINVAL, just like > > match_string(). > > * Uses sysfs_streq instead of strcmp for matching. > > + * Ignores NULL pointers within the @array if @gaps is true. > > */ > > -int __sysfs_match_string(const char * const *array, size_t n, const > > char *str) > > +int __sysfs_match_string_with_gaps(const char * const *array, size_t > > n, > > + const char *str, bool gaps) > > { > > const char *item; > > int index; > > > > for (index = 0; index < n; index++) { > > item = array[index]; > > - if (!item) > > + if (!item) { > > + if (gaps) > > + continue; > > break; > > + } > > if (sysfs_streq(item, str)) > > return index; > > } > > > > return -EINVAL; > > } > > -EXPORT_SYMBOL(__sysfs_match_string); > > +EXPORT_SYMBOL(__sysfs_match_string_with_gaps); > > > > #ifndef __HAVE_ARCH_MEMSET > > /** > >
This helper is similar to __sysfs_match_string() with the exception that it ignores NULL elements within the array. It takes an extra parameter (called `gaps`) which when true will ignore the NULL elements. When false, this function behaves exactly like `__sysfs_match_string()`. Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- include/linux/string.h | 2 ++ lib/string.c | 48 ++++++++++++++++++++++++++++++++---------- 2 files changed, 39 insertions(+), 11 deletions(-) diff --git a/include/linux/string.h b/include/linux/string.h index 7927b875f80c..08e4a33b5f29 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -190,6 +190,8 @@ static inline int strtobool(const char *s, bool *res) int match_string(const char * const *array, size_t n, const char *string); int __sysfs_match_string(const char * const *array, size_t n, const char *s); +int __sysfs_match_string_with_gaps(const char * const *array, size_t n, + const char *str); /** * sysfs_match_string - matches given string in an array diff --git a/lib/string.c b/lib/string.c index 38e4ca08e757..2ae3b7e5ff3d 100644 --- a/lib/string.c +++ b/lib/string.c @@ -658,32 +658,58 @@ int match_string(const char * const *array, size_t n, const char *string) } EXPORT_SYMBOL(match_string); -/** - * __sysfs_match_string - matches given string in an array - * @array: array of strings - * @n: number of strings in the array or -1 for NULL terminated arrays - * @str: string to match with - * - * Returns index of @str in the @array or -EINVAL, just like match_string(). - * Uses sysfs_streq instead of strcmp for matching. - */ -int __sysfs_match_string(const char * const *array, size_t n, const char *str) +static int __sysfs_match_string_common(const char * const *array, size_t n, + const char *str, bool gaps) { const char *item; int index; for (index = 0; index < n; index++) { item = array[index]; - if (!item) + if (!item) { + if (gaps) + continue; break; + } if (sysfs_streq(item, str)) return index; } return -EINVAL; } + +/** + * __sysfs_match_string - matches given string in an array + * @array: array of strings + * @n: number of strings in the array or -1 for NULL terminated arrays + * @str: string to match with + * + * Returns index of @str in the @array or -EINVAL, just like match_string(). + * Uses sysfs_streq instead of strcmp for matching. + */ +int __sysfs_match_string(const char * const *array, size_t n, const char *str) +{ + return __sysfs_match_string_common(array, n, str, false); +} EXPORT_SYMBOL(__sysfs_match_string); +/** + * __sysfs_match_string_with_gaps - matches string in array ignoring NULLs + * @array: array of strings + * @n: number of strings in the array or -1 for NULL terminated arrays + * @str: string to match with + * + * Returns index of @str in the @array or -EINVAL, just like match_string(). + * Uses sysfs_streq instead of strcmp for matching. + * Ignores NULL entries within the @array. + */ +int __sysfs_match_string_with_gaps(const char * const *array, size_t n, + const char *str) +{ + return __sysfs_match_string_common(array, n, str, true); +} +EXPORT_SYMBOL(__sysfs_match_string_with_gaps); + #ifndef __HAVE_ARCH_MEMSET /** * memset - Fill a region of memory with the given value -- 2.17.1
From: Lars-Peter Clausen <lars@metafoo.de> Some enums might have gaps or reserved values in the middle of their value range. E.g. consider a 2-bit enum where the values 0, 1 and 3 have a meaning, but 2 is a reserved value and can not be used. Add support for such enums to the IIO enum helper functions. A reserved values is marked by setting its entry in the items array to NULL rather than the normal descriptive string value. Signed-off-by: Lars-Peter Clausen <lars@metafoo.de> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- drivers/iio/industrialio-core.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c index f2ebca65f964..e848b35bd9c0 100644 --- a/drivers/iio/industrialio-core.c +++ b/drivers/iio/industrialio-core.c @@ -447,8 +447,11 @@ ssize_t iio_enum_available_read(struct iio_dev *indio_dev, if (!e->num_items) return 0; - for (i = 0; i < e->num_items; ++i) + for (i = 0; i < e->num_items; ++i) { + if (!e->items[i]) + continue; len += scnprintf(buf + len, PAGE_SIZE - len, "%s ", e->items[i]); + } /* replace last space with a newline */ buf[len - 1] = '\n'; @@ -469,7 +472,7 @@ ssize_t iio_enum_read(struct iio_dev *indio_dev, i = e->get(indio_dev, chan); if (i < 0) return i; - else if (i >= e->num_items) + else if (i >= e->num_items || !e->items[i]) return -EINVAL; return snprintf(buf, PAGE_SIZE, "%s\n", e->items[i]); @@ -486,7 +489,7 @@ ssize_t iio_enum_write(struct iio_dev *indio_dev, if (!e->set) return -EINVAL; - ret = __sysfs_match_string(e->items, e->num_items, buf); + ret = __sysfs_match_string_with_gaps(e->items, e->num_items, buf); if (ret < 0) return ret; -- 2.17.1
On Mon, Apr 22, 2019 at 11:32:56AM +0300, Alexandru Ardelean wrote:
> This helper is similar to __sysfs_match_string() with the exception that it
> ignores NULL elements within the array.
sysfs is "one value per file", why are you trying to write multiple
things on a single line to a single sysfs file?
Is IIO really that messy? :)
thanks,
greg k-h
On Mon, 2019-04-22 at 23:06 +0200, Greg KH wrote: > > > On Mon, Apr 22, 2019 at 11:32:56AM +0300, Alexandru Ardelean wrote: > > This helper is similar to __sysfs_match_string() with the exception > > that it > > ignores NULL elements within the array. > > sysfs is "one value per file", why are you trying to write multiple > things on a single line to a single sysfs file? > > Is IIO really that messy? :) > Hmm, I don't think I understood the comment/question, or maybe I did not formulate the comment properly. Maybe Jonathan can pitch-in here if I'm saying something wrong. So, in IIO there is `struct iio_enum` which is essentially a sysfs wrapper for exposing an "enum" type to userspace via sysfs (which takes only one value). This iio_enum type is basically a string-to-int mapping. Some example in C: enum { ENUM0, ENUM1, ENUM5 = 5, ENUM6, ENUM7 }; /* Notice the gaps in the elements */ static const char * const item_strings[] = { [ENUM0] = "mode0", [ENUM1] = "mode1", [ENUM5] = "mode5", [ENUM6] = "mode6", [ENUM7] = "mode7", }; static const struct iio_enum iio_enum1 = { .items = item_strings, .num_items = ARRAY_SIZE(item_strings), .set = iio_enum1_set, .get = iio_enum1_get, }; The signature of the iio_enum1_set / iio_enum1_get is below: static int iio_enum1_set(struct iio_dev *indio_dev, const struct iio_chan_spec *chan, unsigned int val); static int iio_enum1_get(struct iio_dev *indio_dev, const struct iio_chan_spec *chan) IIO core resolves the string-to-int mapping. It uses __sysfs_match_string() to do that, but it requires that the list of strings (and C enums) be contiguous. This change [and V2 of this patch] introduces a __sysfs_match_string_with_gaps() helper that ignores gaps (represented as NULLs). For reference, __sysfs_match_string() returns -EINVAL on the first NULL in the array of strings (regardless of the given array size). __sysfs_match_string_with_gaps() is typically helpful when C enums refer to bitfields, or have some equivalence in HW. Thanks Alex > thanks, > > greg k-h
On Tue, 23 Apr 2019 06:38:44 +0000 "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote: > On Mon, 2019-04-22 at 23:06 +0200, Greg KH wrote: > > > > > > On Mon, Apr 22, 2019 at 11:32:56AM +0300, Alexandru Ardelean wrote: > > > This helper is similar to __sysfs_match_string() with the exception > > > that it > > > ignores NULL elements within the array. > > > > sysfs is "one value per file", why are you trying to write multiple > > things on a single line to a single sysfs file? > > > > Is IIO really that messy? :) > > > > Hmm, I don't think I understood the comment/question, or maybe I did not > formulate the comment properly. > > Maybe Jonathan can pitch-in here if I'm saying something wrong. > > So, in IIO there is `struct iio_enum` which is essentially a sysfs wrapper > for exposing an "enum" type to userspace via sysfs (which takes only one > value). This iio_enum type is basically a string-to-int mapping. > > Some example in C: > > enum { > ENUM0, > ENUM1, > ENUM5 = 5, > ENUM6, > ENUM7 > }; > > > /* Notice the gaps in the elements */ > static const char * const item_strings[] = { > [ENUM0] = "mode0", > [ENUM1] = "mode1", > [ENUM5] = "mode5", > [ENUM6] = "mode6", > [ENUM7] = "mode7", > }; > > static const struct iio_enum iio_enum1 = { > .items = item_strings, > .num_items = ARRAY_SIZE(item_strings), > .set = iio_enum1_set, > .get = iio_enum1_get, > }; > > > The signature of the iio_enum1_set / iio_enum1_get is below: > > static int iio_enum1_set(struct iio_dev *indio_dev, > const struct iio_chan_spec *chan, unsigned int val); > > static int iio_enum1_get(struct iio_dev *indio_dev, > const struct iio_chan_spec *chan) > > > IIO core resolves the string-to-int mapping. > It uses __sysfs_match_string() to do that, but it requires that the list of > strings (and C enums) be contiguous. > This change [and V2 of this patch] introduces a > __sysfs_match_string_with_gaps() helper that ignores gaps (represented as > NULLs). > > For reference, __sysfs_match_string() returns -EINVAL on the first NULL in > the array of strings (regardless of the given array size). > > __sysfs_match_string_with_gaps() is typically helpful when C enums refer to > bitfields, or have some equivalence in HW. > You have described it well. Perhaps the issue is in the naming? Or more description is needed for the original patch. It's worth highlighting that the current help text for __sysfs_match_string has a description that says: /** * __sysfs_match_string - matches given string in an array * @array: array of strings * @n: number of strings in the array or -1 for NULL terminated arrays * @str: string to match with * * Returns index of @str in the @array or -EINVAL, just like match_string(). * Uses sysfs_streq instead of strcmp for matching. */ so one could argue that if you pass a value of n which is not -1 the function should not assume that any NULL terminates the array... So basically this new function is implementing what I would have assumed __sysfs_match_string would do, but doesn't. Jonathan > Thanks > Alex > > > thanks, > > > > greg k-h
On Wed, Apr 24, 2019 at 01:34:55PM +0100, Jonathan Cameron wrote:
> On Tue, 23 Apr 2019 06:38:44 +0000
> "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
>
> > On Mon, 2019-04-22 at 23:06 +0200, Greg KH wrote:
> > >
> > >
> > > On Mon, Apr 22, 2019 at 11:32:56AM +0300, Alexandru Ardelean wrote:
> > > > This helper is similar to __sysfs_match_string() with the exception
> > > > that it
> > > > ignores NULL elements within the array.
> > >
> > > sysfs is "one value per file", why are you trying to write multiple
> > > things on a single line to a single sysfs file?
> > >
> > > Is IIO really that messy? :)
> > >
> >
> > Hmm, I don't think I understood the comment/question, or maybe I did not
> > formulate the comment properly.
> >
> > Maybe Jonathan can pitch-in here if I'm saying something wrong.
> >
> > So, in IIO there is `struct iio_enum` which is essentially a sysfs wrapper
> > for exposing an "enum" type to userspace via sysfs (which takes only one
> > value). This iio_enum type is basically a string-to-int mapping.
>
> >
> > Some example in C:
> >
> > enum {
> > ENUM0,
> > ENUM1,
> > ENUM5 = 5,
> > ENUM6,
> > ENUM7
> > };
> >
> >
> > /* Notice the gaps in the elements */
> > static const char * const item_strings[] = {
> > [ENUM0] = "mode0",
> > [ENUM1] = "mode1",
> > [ENUM5] = "mode5",
> > [ENUM6] = "mode6",
> > [ENUM7] = "mode7",
> > };
> >
> > static const struct iio_enum iio_enum1 = {
> > .items = item_strings,
> > .num_items = ARRAY_SIZE(item_strings),
> > .set = iio_enum1_set,
> > .get = iio_enum1_get,
> > };
> >
> >
> > The signature of the iio_enum1_set / iio_enum1_get is below:
> >
> > static int iio_enum1_set(struct iio_dev *indio_dev,
> > const struct iio_chan_spec *chan, unsigned int val);
> >
> > static int iio_enum1_get(struct iio_dev *indio_dev,
> > const struct iio_chan_spec *chan)
> >
> >
> > IIO core resolves the string-to-int mapping.
> > It uses __sysfs_match_string() to do that, but it requires that the list of
> > strings (and C enums) be contiguous.
> > This change [and V2 of this patch] introduces a
> > __sysfs_match_string_with_gaps() helper that ignores gaps (represented as
> > NULLs).
> >
> > For reference, __sysfs_match_string() returns -EINVAL on the first NULL in
> > the array of strings (regardless of the given array size).
> >
> > __sysfs_match_string_with_gaps() is typically helpful when C enums refer to
> > bitfields, or have some equivalence in HW.
> >
>
> You have described it well.
> Perhaps the issue is in the naming? Or more description is needed for the original
> patch.
>
> It's worth highlighting that the current help text for
> __sysfs_match_string has a description that says:
>
> /**
> * __sysfs_match_string - matches given string in an array
> * @array: array of strings
> * @n: number of strings in the array or -1 for NULL terminated arrays
> * @str: string to match with
> *
> * Returns index of @str in the @array or -EINVAL, just like match_string().
> * Uses sysfs_streq instead of strcmp for matching.
> */
>
> so one could argue that if you pass a value of n which is not -1 the function
> should not assume that any NULL terminates the array...
>
> So basically this new function is implementing what I would have assumed
> __sysfs_match_string would do, but doesn't.
Ok, yeah, I'm confused, I also thought this is what the original
function did.
Nevermind, no objection from me on this:
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Thu, Apr 25, 2019 at 10:38 PM gregkh@linuxfoundation.org
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Apr 24, 2019 at 01:34:55PM +0100, Jonathan Cameron wrote:
> > On Tue, 23 Apr 2019 06:38:44 +0000
> > "Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:
> >
> > > On Mon, 2019-04-22 at 23:06 +0200, Greg KH wrote:
> > > >
> > > >
> > > > On Mon, Apr 22, 2019 at 11:32:56AM +0300, Alexandru Ardelean wrote:
> > > > > This helper is similar to __sysfs_match_string() with the exception
> > > > > that it
> > > > > ignores NULL elements within the array.
> > > >
> > > > sysfs is "one value per file", why are you trying to write multiple
> > > > things on a single line to a single sysfs file?
> > > >
> > > > Is IIO really that messy? :)
> > > >
> > >
> > > Hmm, I don't think I understood the comment/question, or maybe I did not
> > > formulate the comment properly.
> > >
> > > Maybe Jonathan can pitch-in here if I'm saying something wrong.
> > >
> > > So, in IIO there is `struct iio_enum` which is essentially a sysfs wrapper
> > > for exposing an "enum" type to userspace via sysfs (which takes only one
> > > value). This iio_enum type is basically a string-to-int mapping.
> >
> > >
> > > Some example in C:
> > >
> > > enum {
> > > ENUM0,
> > > ENUM1,
> > > ENUM5 = 5,
> > > ENUM6,
> > > ENUM7
> > > };
> > >
> > >
> > > /* Notice the gaps in the elements */
> > > static const char * const item_strings[] = {
> > > [ENUM0] = "mode0",
> > > [ENUM1] = "mode1",
> > > [ENUM5] = "mode5",
> > > [ENUM6] = "mode6",
> > > [ENUM7] = "mode7",
> > > };
> > >
> > > static const struct iio_enum iio_enum1 = {
> > > .items = item_strings,
> > > .num_items = ARRAY_SIZE(item_strings),
> > > .set = iio_enum1_set,
> > > .get = iio_enum1_get,
> > > };
> > >
> > >
> > > The signature of the iio_enum1_set / iio_enum1_get is below:
> > >
> > > static int iio_enum1_set(struct iio_dev *indio_dev,
> > > const struct iio_chan_spec *chan, unsigned int val);
> > >
> > > static int iio_enum1_get(struct iio_dev *indio_dev,
> > > const struct iio_chan_spec *chan)
> > >
> > >
> > > IIO core resolves the string-to-int mapping.
> > > It uses __sysfs_match_string() to do that, but it requires that the list of
> > > strings (and C enums) be contiguous.
> > > This change [and V2 of this patch] introduces a
> > > __sysfs_match_string_with_gaps() helper that ignores gaps (represented as
> > > NULLs).
> > >
> > > For reference, __sysfs_match_string() returns -EINVAL on the first NULL in
> > > the array of strings (regardless of the given array size).
> > >
> > > __sysfs_match_string_with_gaps() is typically helpful when C enums refer to
> > > bitfields, or have some equivalence in HW.
> > >
> >
> > You have described it well.
> > Perhaps the issue is in the naming? Or more description is needed for the original
> > patch.
> >
> > It's worth highlighting that the current help text for
> > __sysfs_match_string has a description that says:
> >
> > /**
> > * __sysfs_match_string - matches given string in an array
> > * @array: array of strings
> > * @n: number of strings in the array or -1 for NULL terminated arrays
> > * @str: string to match with
> > *
> > * Returns index of @str in the @array or -EINVAL, just like match_string().
> > * Uses sysfs_streq instead of strcmp for matching.
> > */
> >
> > so one could argue that if you pass a value of n which is not -1 the function
> > should not assume that any NULL terminates the array...
> >
> > So basically this new function is implementing what I would have assumed
> > __sysfs_match_string would do, but doesn't.
>
> Ok, yeah, I'm confused, I also thought this is what the original
> function did.
>
> Nevermind, no objection from me on this:
>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Hmm, I actually did not give much thought to that -1.
I'll check into this and see about a V3.
It may make more sense to just fix the original
`__sysfs_match_string()`, but I'll need to go through the users of
this function and see.
Thanks
Alex
On Fri, Apr 26, 2019 at 12:29:11PM +0300, Alexandru Ardelean wrote:
> Hmm, I actually did not give much thought to that -1.
> I'll check into this and see about a V3.
> It may make more sense to just fix the original
> `__sysfs_match_string()`, but I'll need to go through the users of
> this function and see.
I was thinking about existing users of such (with "gaps") cases.
Not all of them have NULL there and would like to avoid some members.
Though, I think that we may ignore NULL items if -1 is supplied.
Think as well about ARRAY_SIZE() as given to that.
And consider to fix match_string() accordingly.
--
With Best Regards,
Andy Shevchenko
On Fri, Apr 26, 2019 at 5:27 PM andriy.shevchenko@linux.intel.com <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Apr 26, 2019 at 12:29:11PM +0300, Alexandru Ardelean wrote: > > > Hmm, I actually did not give much thought to that -1. > > I'll check into this and see about a V3. > > It may make more sense to just fix the original > > `__sysfs_match_string()`, but I'll need to go through the users of > > this function and see. > > I was thinking about existing users of such (with "gaps") cases. > Not all of them have NULL there and would like to avoid some members. > Though, I think that we may ignore NULL items if -1 is supplied. > > Think as well about ARRAY_SIZE() as given to that. > I am a bit vague on what you are proposing. Is it: a) Leave __sysfs_match_string() as-is and introduce a new `__sysfs_match_string_with_gaps()` helper/variant ? b) Fix __sysfs_match_string() to break/exit on the first NULL, only if -1 is provided ? Either is fine, but I wanted to clarify. Thanks Alex > And consider to fix match_string() accordingly. > > -- > With Best Regards, > Andy Shevchenko > >
On Mon, May 06, 2019 at 04:45:43PM +0300, Alexandru Ardelean wrote: > On Fri, Apr 26, 2019 at 5:27 PM andriy.shevchenko@linux.intel.com > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Fri, Apr 26, 2019 at 12:29:11PM +0300, Alexandru Ardelean wrote: > > > > > Hmm, I actually did not give much thought to that -1. > > > I'll check into this and see about a V3. > > > It may make more sense to just fix the original > > > `__sysfs_match_string()`, but I'll need to go through the users of > > > this function and see. > > > > I was thinking about existing users of such (with "gaps") cases. > > Not all of them have NULL there and would like to avoid some members. > > Though, I think that we may ignore NULL items if -1 is supplied. > > > > Think as well about ARRAY_SIZE() as given to that. > > > > I am a bit vague on what you are proposing. > Is it: > > a) Leave __sysfs_match_string() as-is and introduce a new > `__sysfs_match_string_with_gaps()` helper/variant ? > b) Fix __sysfs_match_string() to break/exit on the first NULL, only if > -1 is provided ? > > Either is fine, but I wanted to clarify. The current logic something like "-1 to go till first NULL" and ARRAY_SIZE() in *some* cases is basically the synonym to above. What I meant is to check if there is *any* case where ARRAY_SIZE() behaves in the same way as -1. Those cases should be fixed accordingly. Otherwise, the b) is what would be preferred according to the discussion. > > And consider to fix match_string() accordingly. -- With Best Regards, Andy Shevchenko
On Mon, 2019-05-06 at 17:46 +0300, andriy.shevchenko@linux.intel.com wrote: > [External] > > > On Mon, May 06, 2019 at 04:45:43PM +0300, Alexandru Ardelean wrote: > > On Fri, Apr 26, 2019 at 5:27 PM andriy.shevchenko@linux.intel.com > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > On Fri, Apr 26, 2019 at 12:29:11PM +0300, Alexandru Ardelean wrote: > > > > > > > Hmm, I actually did not give much thought to that -1. > > > > I'll check into this and see about a V3. > > > > It may make more sense to just fix the original > > > > `__sysfs_match_string()`, but I'll need to go through the users of > > > > this function and see. > > > > > > I was thinking about existing users of such (with "gaps") cases. > > > Not all of them have NULL there and would like to avoid some members. > > > Though, I think that we may ignore NULL items if -1 is supplied. > > > > > > Think as well about ARRAY_SIZE() as given to that. > > > > > > > I am a bit vague on what you are proposing. > > Is it: > > > > a) Leave __sysfs_match_string() as-is and introduce a new > > `__sysfs_match_string_with_gaps()` helper/variant ? > > b) Fix __sysfs_match_string() to break/exit on the first NULL, only if > > -1 is provided ? > > > > Either is fine, but I wanted to clarify. > > The current logic something like "-1 to go till first NULL" and > ARRAY_SIZE() in > *some* cases is basically the synonym to above. > > What I meant is to check if there is *any* case where ARRAY_SIZE() > behaves in > the same way as -1. Those cases should be fixed accordingly. > > Otherwise, the b) is what would be preferred according to the discussion. > Ack. I sent a series. I guess this is the noisiest I've ever been. And I feel a bit bad/guilty [for generating that much noise], but I'll probably grab a beer later to treat it. I'll probably learn something from this. Guilt-tripped experiences are pretty learnful. Thanks Alex > > > And consider to fix match_string() accordingly. > > -- > With Best Regards, > Andy Shevchenko > >