From: Jonathan Cameron <jonathan.cameron@huawei.com>
To: "Ardelean, Alexandru" <alexandru.Ardelean@analog.com>
Cc: "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
"namhyung@kernel.org" <namhyung@kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"andriy.shevchenko@linux.intel.com"
<andriy.shevchenko@linux.intel.com>,
"linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
"mingo@kernel.org" <mingo@kernel.org>
Subject: Re: [PATCH 1/2] lib: add __sysfs_match_string_with_gaps() helper
Date: Wed, 24 Apr 2019 13:34:55 +0100 [thread overview]
Message-ID: <20190424133455.00002909@huawei.com> (raw)
In-Reply-To: <86ea407aaa891e50a3bdaf2c3653636a365076ee.camel@analog.com>
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
next prev parent reply other threads:[~2019-04-24 12:35 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-22 8:32 [PATCH 1/2] lib: add __sysfs_match_string_with_gaps() helper Alexandru Ardelean
2019-04-22 8:32 ` [PATCH 2/2] iio: Handle enumerated properties with gaps Alexandru Ardelean
2019-04-22 10:38 ` Jonathan Cameron
2019-04-22 14:03 ` [PATCH 2/2][V2] " Alexandru Ardelean
2019-04-22 10:37 ` [PATCH 1/2] lib: add __sysfs_match_string_with_gaps() helper Jonathan Cameron
2019-04-22 11:16 ` Ardelean, Alexandru
2019-04-22 14:02 ` [PATCH 1/2][V2] " Alexandru Ardelean
2019-04-22 21:06 ` [PATCH 1/2] " Greg KH
2019-04-23 6:38 ` Ardelean, Alexandru
2019-04-24 12:34 ` Jonathan Cameron [this message]
2019-04-25 19:37 ` gregkh
2019-04-26 9:29 ` Alexandru Ardelean
2019-04-26 14:27 ` andriy.shevchenko
2019-05-06 13:45 ` Alexandru Ardelean
2019-05-06 14:46 ` andriy.shevchenko
2019-05-08 11:34 ` Ardelean, Alexandru
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190424133455.00002909@huawei.com \
--to=jonathan.cameron@huawei.com \
--cc=alexandru.Ardelean@analog.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@kernel.org \
--cc=namhyung@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).