| Message ID | 20230808162800.61651-3-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | iio: Introduce and use device_property_match_property_string() | expand |
On Tue, 8 Aug 2023 19:27:56 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > Sometimes the users want to match the single value string property > against an array of predefined strings. Create a helper for them. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/base/property.c | 35 +++++++++++++++++++++++++++++++++++ > include/linux/property.h | 12 ++++++++++++ > 2 files changed, 47 insertions(+) > > diff --git a/drivers/base/property.c b/drivers/base/property.c > index 3bb9505f1631..8f8e2a6816bc 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -498,6 +498,41 @@ int fwnode_property_match_string(const struct fwnode_handle *fwnode, > } > EXPORT_SYMBOL_GPL(fwnode_property_match_string); > > +/** > + * fwnode_property_match_property_string - find a property string value in an array and return index > + * @fwnode: Firmware node to get the property of > + * @propname: Name of the property holding the string value > + * @array: String array to search in > + * @n: Size of the @array > + * > + * Find a property string value in a given @array and if it is found return > + * the index back. > + * > + * Return: index, starting from %0, if the string value was found in the @array (success), > + * %-ENOENT when the string value was not found in the @array, > + * %-EINVAL if given arguments are not valid, > + * %-ENODATA if the property does not have a value, > + * %-EPROTO or %-EILSEQ if the property is not a string, > + * %-ENXIO if no suitable firmware interface is present. > + */ > +int fwnode_property_match_property_string(const struct fwnode_handle *fwnode, > + const char *propname, const char * const *array, size_t n) Hi Andy, Whilst I'm not 100% sold on adding ever increasing complexity to what we match, this one feels like a common enough thing to be worth providing. Looking at the usecases I wonder if it would be better to pass in an unsigned int *ret which is only updated on a match? That way the common properties approach of not checking the return value if we have an optional property would apply. e.g. patch 3 would end up with a block that looks like: st->input_mode = ADMV1014_IQ_MODE; device_property_match_property_string(&spi->dev, "adi,input-mode", input_mode_names, ARRAY_SIZE(input_mode_names), &st->input_mode); Only neat and tidy if the thing being optionally read into is an unsigned int though (otherwise you still need a local variable) Jonathan > +{ > + const char *string; > + int ret; > + > + ret = fwnode_property_read_string(fwnode, propname, &string); > + if (ret) > + return ret; > + > + ret = match_string(array, n, string); > + if (ret < 0) > + ret = -ENOENT; > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(fwnode_property_match_property_string); > + > /** > * fwnode_property_get_reference_args() - Find a reference with arguments > * @fwnode: Firmware node where to look for the reference > diff --git a/include/linux/property.h b/include/linux/property.h > index 8c3c6685a2ae..11f3ad6814f2 100644 > --- a/include/linux/property.h > +++ b/include/linux/property.h > @@ -97,6 +97,18 @@ static inline bool device_is_compatible(const struct device *dev, const char *co > return fwnode_device_is_compatible(dev_fwnode(dev), compat); > } > > +int fwnode_property_match_property_string(const struct fwnode_handle *fwnode, > + const char *propname, > + const char * const *array, size_t n); > + > +static inline > +int device_property_match_property_string(const struct device *dev, > + const char *propname, > + const char * const *array, size_t n) > +{ > + return fwnode_property_match_property_string(dev_fwnode(dev), propname, array, n); > +} > + > int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode, > const char *prop, const char *nargs_prop, > unsigned int nargs, unsigned int index,
On Wed, Aug 09, 2023 at 06:59:44PM +0100, Jonathan Cameron wrote: > On Tue, 8 Aug 2023 19:27:56 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: ... > > +int fwnode_property_match_property_string(const struct fwnode_handle *fwnode, > > + const char *propname, const char * const *array, size_t n) > > Hi Andy, > > Whilst I'm not 100% sold on adding ever increasing complexity to what we > match, this one feels like a common enough thing to be worth providing. Yep, that's why I considered it's good to add (and because of new comers). > Looking at the usecases I wonder if it would be better to pass in > an unsigned int *ret which is only updated on a match? So the question is here are we going to match (pun intended) the prototype to the device_property_match*() family of functions or to device_property_read_*() one. If the latter, this has to be renamed, but then it probably will contradict the semantics as we are _matching_ against something and not just _reading_ something. That said, do you agree that current implementation is (slightly) better from these aspects? Anyway, look at the below. > That way the common properties approach of not checking the return value > if we have an optional property would apply. > > e.g. patch 3 Only? > would end up with a block that looks like: > > st->input_mode = ADMV1014_IQ_MODE; > device_property_match_property_string(&spi->dev, "adi,input-mode", > input_mode_names, > ARRAY_SIZE(input_mode_names), > &st->input_mode); > > Only neat and tidy if the thing being optionally read into is an unsigned int > though (otherwise you still need a local variable) We also can have a hybrid variant, returning in both sides int device_property_match_property_string(..., size_t *index) { if (index) *index = ret; return ret; } (also note the correct return type as it has to match to @n). Would it be still okay or too over engineered?
On Thu, 10 Aug 2023 16:26:54 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Wed, Aug 09, 2023 at 06:59:44PM +0100, Jonathan Cameron wrote: > > On Tue, 8 Aug 2023 19:27:56 +0300 > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > ... > > > > +int fwnode_property_match_property_string(const struct fwnode_handle *fwnode, > > > + const char *propname, const char * const *array, size_t n) > > > > Hi Andy, > > > > Whilst I'm not 100% sold on adding ever increasing complexity to what we > > match, this one feels like a common enough thing to be worth providing. > > Yep, that's why I considered it's good to add (and because of new comers). > > > Looking at the usecases I wonder if it would be better to pass in > > an unsigned int *ret which is only updated on a match? > > So the question is here are we going to match (pun intended) the prototype to > the device_property_match*() family of functions or to device_property_read_*() > one. If the latter, this has to be renamed, but then it probably will contradict > the semantics as we are _matching_ against something and not just _reading_ > something. > > That said, do you agree that current implementation is (slightly) better from > these aspects? Anyway, look at the below. > > > That way the common properties approach of not checking the return value > > if we have an optional property would apply. > > > > e.g. patch 3 > > Only? I didn't look further :) > > > would end up with a block that looks like: > > > > st->input_mode = ADMV1014_IQ_MODE; > > device_property_match_property_string(&spi->dev, "adi,input-mode", > > input_mode_names, > > ARRAY_SIZE(input_mode_names), > > &st->input_mode); > > > > Only neat and tidy if the thing being optionally read into is an unsigned int > > though (otherwise you still need a local variable) > > We also can have a hybrid variant, returning in both sides > > int device_property_match_property_string(..., size_t *index) > { > if (index) > *index = ret; > return ret; > } > > (also note the correct return type as it has to match to @n). > > Would it be still okay or too over engineered? > Probably over engineered.... Lets stick to what you have. If various firmware folk are happy with the new function that's fine by me. Rafael? Jonathan
On Mon, Aug 28, 2023 at 8:00 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Thu, 10 Aug 2023 16:26:54 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > On Wed, Aug 09, 2023 at 06:59:44PM +0100, Jonathan Cameron wrote: > > > On Tue, 8 Aug 2023 19:27:56 +0300 > > > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > > > ... > > > > > > +int fwnode_property_match_property_string(const struct fwnode_handle *fwnode, > > > > + const char *propname, const char * const *array, size_t n) > > > > > > Hi Andy, > > > > > > Whilst I'm not 100% sold on adding ever increasing complexity to what we > > > match, this one feels like a common enough thing to be worth providing. > > > > Yep, that's why I considered it's good to add (and because of new comers). > > > > > Looking at the usecases I wonder if it would be better to pass in > > > an unsigned int *ret which is only updated on a match? > > > > So the question is here are we going to match (pun intended) the prototype to > > the device_property_match*() family of functions or to device_property_read_*() > > one. If the latter, this has to be renamed, but then it probably will contradict > > the semantics as we are _matching_ against something and not just _reading_ > > something. > > > > That said, do you agree that current implementation is (slightly) better from > > these aspects? Anyway, look at the below. > > > > > That way the common properties approach of not checking the return value > > > if we have an optional property would apply. > > > > > > e.g. patch 3 > > > > Only? > I didn't look further :) > > > > > > would end up with a block that looks like: > > > > > > st->input_mode = ADMV1014_IQ_MODE; > > > device_property_match_property_string(&spi->dev, "adi,input-mode", > > > input_mode_names, > > > ARRAY_SIZE(input_mode_names), > > > &st->input_mode); > > > > > > Only neat and tidy if the thing being optionally read into is an unsigned int > > > though (otherwise you still need a local variable) > > > > We also can have a hybrid variant, returning in both sides > > > > int device_property_match_property_string(..., size_t *index) > > { > > if (index) > > *index = ret; > > return ret; > > } > > > > (also note the correct return type as it has to match to @n). > > > > Would it be still okay or too over engineered? > > > Probably over engineered.... > > Lets stick to what you have. If various firmware folk are happy with > the new function that's fine by me. Rafael? Sorry for the delay, I've lost track of this. Honestly, I have no strong opinion, but I think that this is going to reduce some code duplication which is a valid purpose, so please feel free to add Acked-by: Rafael J. Wysocki <rafael@kernel.org> to this patch. Thanks!
On Tue, Oct 17, 2023 at 09:19:30PM +0200, Rafael J. Wysocki wrote: > On Mon, Aug 28, 2023 at 8:00 PM Jonathan Cameron <jic23@kernel.org> wrote: ... > Sorry for the delay, I've lost track of this. NP! > Honestly, I have no strong opinion, but I think that this is going to > reduce some code duplication which is a valid purpose, so please feel > free to add > > Acked-by: Rafael J. Wysocki <rafael@kernel.org> > > to this patch. Thank you! Jonathan, are we all set for applying this series?
On Tue, 17 Oct 2023 22:43:04 +0300 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Tue, Oct 17, 2023 at 09:19:30PM +0200, Rafael J. Wysocki wrote: > > On Mon, Aug 28, 2023 at 8:00 PM Jonathan Cameron <jic23@kernel.org> wrote: > > ... > > > Sorry for the delay, I've lost track of this. > > NP! > > > Honestly, I have no strong opinion, but I think that this is going to > > reduce some code duplication which is a valid purpose, so please feel > > free to add > > > > Acked-by: Rafael J. Wysocki <rafael@kernel.org> > > > > to this patch. > > Thank you! > > Jonathan, are we all set for applying this series? > Applied, but it might end up as 6.8 material depending on exactly how timing turns out. I have one pull request sent and I'm not sure I'll get another one in this cycle. Given I just applied some big drivers I'd like to, but not sure yet... Jonathan
On Wed, Oct 18, 2023 at 08:37:55PM +0100, Jonathan Cameron wrote: > On Tue, 17 Oct 2023 22:43:04 +0300 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Tue, Oct 17, 2023 at 09:19:30PM +0200, Rafael J. Wysocki wrote: > > > On Mon, Aug 28, 2023 at 8:00 PM Jonathan Cameron <jic23@kernel.org> wrote: ... > > > Sorry for the delay, I've lost track of this. > > > > NP! > > > > > Honestly, I have no strong opinion, but I think that this is going to > > > reduce some code duplication which is a valid purpose, so please feel > > > free to add > > > > > > Acked-by: Rafael J. Wysocki <rafael@kernel.org> > > > > > > to this patch. > > > > Thank you! > > > > Jonathan, are we all set for applying this series? > > > Applied, but it might end up as 6.8 material depending on exactly how > timing turns out. I have one pull request sent and I'm not sure I'll get > another one in this cycle. Given I just applied some big drivers I'd like to, but > not sure yet... It's fine, I'm not in hurry with this and thank you!
diff --git a/drivers/base/property.c b/drivers/base/property.c index 3bb9505f1631..8f8e2a6816bc 100644 --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -498,6 +498,41 @@ int fwnode_property_match_string(const struct fwnode_handle *fwnode, } EXPORT_SYMBOL_GPL(fwnode_property_match_string); +/** + * fwnode_property_match_property_string - find a property string value in an array and return index + * @fwnode: Firmware node to get the property of + * @propname: Name of the property holding the string value + * @array: String array to search in + * @n: Size of the @array + * + * Find a property string value in a given @array and if it is found return + * the index back. + * + * Return: index, starting from %0, if the string value was found in the @array (success), + * %-ENOENT when the string value was not found in the @array, + * %-EINVAL if given arguments are not valid, + * %-ENODATA if the property does not have a value, + * %-EPROTO or %-EILSEQ if the property is not a string, + * %-ENXIO if no suitable firmware interface is present. + */ +int fwnode_property_match_property_string(const struct fwnode_handle *fwnode, + const char *propname, const char * const *array, size_t n) +{ + const char *string; + int ret; + + ret = fwnode_property_read_string(fwnode, propname, &string); + if (ret) + return ret; + + ret = match_string(array, n, string); + if (ret < 0) + ret = -ENOENT; + + return ret; +} +EXPORT_SYMBOL_GPL(fwnode_property_match_property_string); + /** * fwnode_property_get_reference_args() - Find a reference with arguments * @fwnode: Firmware node where to look for the reference diff --git a/include/linux/property.h b/include/linux/property.h index 8c3c6685a2ae..11f3ad6814f2 100644 --- a/include/linux/property.h +++ b/include/linux/property.h @@ -97,6 +97,18 @@ static inline bool device_is_compatible(const struct device *dev, const char *co return fwnode_device_is_compatible(dev_fwnode(dev), compat); } +int fwnode_property_match_property_string(const struct fwnode_handle *fwnode, + const char *propname, + const char * const *array, size_t n); + +static inline +int device_property_match_property_string(const struct device *dev, + const char *propname, + const char * const *array, size_t n) +{ + return fwnode_property_match_property_string(dev_fwnode(dev), propname, array, n); +} + int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode, const char *prop, const char *nargs_prop, unsigned int nargs, unsigned int index,
Sometimes the users want to match the single value string property against an array of predefined strings. Create a helper for them. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/base/property.c | 35 +++++++++++++++++++++++++++++++++++ include/linux/property.h | 12 ++++++++++++ 2 files changed, 47 insertions(+)