Re: [PATCH 1/2] lib: add __sysfs_match_string_with_gaps() helper

[Jonathan Cameron]

On Tue, 23 Apr 2019 06:38:44 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@xxxxxxxxxx> 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