| Message ID | 20230808162800.61651-1-andriy.shevchenko@linux.intel.com |
|---|---|
| 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 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!
At least 4 drivers in the kernel are open coding functionality of proposed device_property_match_property_string() helper. There are also more existing users, but I want to limit this to one (biggest user so far) subsystem. Hence, assuming Greg KH is okay this can be routed via IIO (and again via Greg :-). The +28 statistics in particular due to documentation. With more users converted we may actually get rid of some lines of code in the future. Andy Shevchenko (6): device property: Use fwnode_property_string_array_count() device property: Add fwnode_property_match_property_string() iio: frequency: adf4377: Switch to device_property_match_property_string() iio: frequency: admv1014: Switch to device_property_match_property_string() iio: magnetometer: tmag5273: Switch to device_property_match_property_string() iio: proximity: sx9324: Switch to device_property_match_property_string() drivers/base/property.c | 37 ++++++++++++++++++++++++++++- drivers/iio/frequency/adf4377.c | 16 +++++-------- drivers/iio/frequency/admv1014.c | 31 ++++++++++-------------- drivers/iio/magnetometer/tmag5273.c | 10 +++----- drivers/iio/proximity/sx9324.c | 24 ++++++++----------- include/linux/property.h | 12 ++++++++++ 6 files changed, 79 insertions(+), 51 deletions(-)