[PATCH v1 0/6] iio: Introduce and use device_property_match_property_string()

[PATCH v1 0/6] iio: Introduce and use device_property_match_property_string()

[Andy Shevchenko]

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(-)

-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 1/6] device property: Use fwnode_property_string_array_count()

[Andy Shevchenko]

Use fwnode_property_string_array_count() instead of open coded variant.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/base/property.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 8c40abed7852..3bb9505f1631 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -473,7 +473,7 @@ int fwnode_property_match_string(const struct fwnode_handle *fwnode,
 	const char **values;
 	int nval, ret;
 
-	nval = fwnode_property_read_string_array(fwnode, propname, NULL, 0);
+	nval = fwnode_property_string_array_count(fwnode, propname);
 	if (nval < 0)
 		return nval;
 
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 2/6] device property: Add fwnode_property_match_property_string()

[Andy Shevchenko]

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)
+{
+	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,
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 3/6] iio: frequency: adf4377: Switch to device_property_match_property_string()

[Andy Shevchenko]

Replace open coded device_property_match_property_string().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/frequency/adf4377.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/frequency/adf4377.c b/drivers/iio/frequency/adf4377.c
index 26abecbd51e0..9284c13f1abb 100644
--- a/drivers/iio/frequency/adf4377.c
+++ b/drivers/iio/frequency/adf4377.c
@@ -870,7 +870,6 @@ static const struct iio_chan_spec adf4377_channels[] = {
 static int adf4377_properties_parse(struct adf4377_state *st)
 {
 	struct spi_device *spi = st->spi;
-	const char *str;
 	int ret;
 
 	st->clkin = devm_clk_get_enabled(&spi->dev, "ref_in");
@@ -896,16 +895,13 @@ static int adf4377_properties_parse(struct adf4377_state *st)
 		return dev_err_probe(&spi->dev, PTR_ERR(st->gpio_enclk2),
 				     "failed to get the CE GPIO\n");
 
-	ret = device_property_read_string(&spi->dev, "adi,muxout-select", &str);
-	if (ret) {
-		st->muxout_select = ADF4377_MUXOUT_HIGH_Z;
-	} else {
-		ret = match_string(adf4377_muxout_modes, ARRAY_SIZE(adf4377_muxout_modes), str);
-		if (ret < 0)
-			return ret;
-
+	ret = device_property_match_property_string(&spi->dev, "adi,muxout-select",
+						    adf4377_muxout_modes,
+						    ARRAY_SIZE(adf4377_muxout_modes));
+	if (ret >= 0)
 		st->muxout_select = ret;
-	}
+	else
+		st->muxout_select = ADF4377_MUXOUT_HIGH_Z;
 
 	return 0;
 }
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 4/6] iio: frequency: admv1014: Switch to device_property_match_property_string()

[Andy Shevchenko]

Replace open coded device_property_match_property_string().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/frequency/admv1014.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/iio/frequency/admv1014.c b/drivers/iio/frequency/admv1014.c
index bb5e1feef42b..b46b73b89eb7 100644
--- a/drivers/iio/frequency/admv1014.c
+++ b/drivers/iio/frequency/admv1014.c
@@ -710,7 +710,6 @@ static int admv1014_init(struct admv1014_state *st)
 
 static int admv1014_properties_parse(struct admv1014_state *st)
 {
-	const char *str;
 	unsigned int i;
 	struct spi_device *spi = st->spi;
 	int ret;
@@ -719,27 +718,21 @@ static int admv1014_properties_parse(struct admv1014_state *st)
 
 	st->p1db_comp = device_property_read_bool(&spi->dev, "adi,p1db-compensation-enable");
 
-	ret = device_property_read_string(&spi->dev, "adi,input-mode", &str);
-	if (ret) {
-		st->input_mode = ADMV1014_IQ_MODE;
-	} else {
-		ret = match_string(input_mode_names, ARRAY_SIZE(input_mode_names), str);
-		if (ret < 0)
-			return ret;
-
+	ret = device_property_match_property_string(&spi->dev, "adi,input-mode",
+						    input_mode_names,
+						    ARRAY_SIZE(input_mode_names));
+	if (ret >= 0)
 		st->input_mode = ret;
-	}
-
-	ret = device_property_read_string(&spi->dev, "adi,quad-se-mode", &str);
-	if (ret) {
-		st->quad_se_mode = ADMV1014_SE_MODE_POS;
-	} else {
-		ret = match_string(quad_se_mode_names, ARRAY_SIZE(quad_se_mode_names), str);
-		if (ret < 0)
-			return ret;
+	else
+		st->input_mode = ADMV1014_IQ_MODE;
 
+	ret = device_property_match_property_string(&spi->dev, "adi,quad-se-mode",
+						    quad_se_mode_names,
+						    ARRAY_SIZE(quad_se_mode_names));
+	if (ret >= 0)
 		st->quad_se_mode = ADMV1014_SE_MODE_POS + (ret * 3);
-	}
+	else
+		st->quad_se_mode = ADMV1014_SE_MODE_POS;
 
 	for (i = 0; i < ADMV1014_NUM_REGULATORS; ++i)
 		st->regulators[i].supply = admv1014_reg_name[i];
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 5/6] iio: magnetometer: tmag5273: Switch to device_property_match_property_string()

[Andy Shevchenko]

Replace open coded device_property_match_property_string().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/magnetometer/tmag5273.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/magnetometer/tmag5273.c b/drivers/iio/magnetometer/tmag5273.c
index c5e5c4ad681e..bcd989d65059 100644
--- a/drivers/iio/magnetometer/tmag5273.c
+++ b/drivers/iio/magnetometer/tmag5273.c
@@ -497,17 +497,13 @@ static int tmag5273_set_operating_mode(struct tmag5273_data *data,
 static void tmag5273_read_device_property(struct tmag5273_data *data)
 {
 	struct device *dev = data->dev;
-	const char *str;
 	int ret;
 
 	data->angle_measurement = TMAG5273_ANGLE_EN_X_Y;
 
-	ret = device_property_read_string(dev, "ti,angle-measurement", &str);
-	if (ret)
-		return;
-
-	ret = match_string(tmag5273_angle_names,
-			   ARRAY_SIZE(tmag5273_angle_names), str);
+	ret = device_property_match_property_string(dev, "ti,angle-measurement",
+						    tmag5273_angle_names,
+						    ARRAY_SIZE(tmag5273_angle_names));
 	if (ret >= 0)
 		data->angle_measurement = ret;
 }
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 6/6] iio: proximity: sx9324: Switch to device_property_match_property_string()

[Andy Shevchenko]

Replace open coded device_property_match_property_string().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/iio/proximity/sx9324.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/proximity/sx9324.c b/drivers/iio/proximity/sx9324.c
index 438f9c9aba6e..ac2ed2da21cc 100644
--- a/drivers/iio/proximity/sx9324.c
+++ b/drivers/iio/proximity/sx9324.c
@@ -888,7 +888,6 @@ sx9324_get_default_reg(struct device *dev, int idx,
 	char prop[] = SX9324_PROXRAW_DEF;
 	u32 start = 0, raw = 0, pos = 0;
 	int ret, count, ph, pin;
-	const char *res;
 
 	memcpy(reg_def, &sx9324_default_regs[idx], sizeof(*reg_def));
 
@@ -915,24 +914,21 @@ sx9324_get_default_reg(struct device *dev, int idx,
 		reg_def->def = raw;
 		break;
 	case SX9324_REG_AFE_CTRL0:
-		ret = device_property_read_string(dev,
-				"semtech,cs-idle-sleep", &res);
-		if (!ret)
-			ret = match_string(sx9324_csidle, ARRAY_SIZE(sx9324_csidle), res);
+		ret = device_property_match_property_string(dev, "semtech,cs-idle-sleep",
+							    sx9324_csidle,
+							    ARRAY_SIZE(sx9324_csidle));
 		if (ret >= 0) {
 			reg_def->def &= ~SX9324_REG_AFE_CTRL0_CSIDLE_MASK;
 			reg_def->def |= ret << SX9324_REG_AFE_CTRL0_CSIDLE_SHIFT;
 		}
 
-		ret = device_property_read_string(dev,
-				"semtech,int-comp-resistor", &res);
-		if (ret)
-			break;
-		ret = match_string(sx9324_rints, ARRAY_SIZE(sx9324_rints), res);
-		if (ret < 0)
-			break;
-		reg_def->def &= ~SX9324_REG_AFE_CTRL0_RINT_MASK;
-		reg_def->def |= ret << SX9324_REG_AFE_CTRL0_RINT_SHIFT;
+		ret = device_property_match_property_string(dev, "semtech,int-comp-resistor",
+							    sx9324_rints,
+							    ARRAY_SIZE(sx9324_rints));
+		if (ret >= 0) {
+			reg_def->def &= ~SX9324_REG_AFE_CTRL0_RINT_MASK;
+			reg_def->def |= ret << SX9324_REG_AFE_CTRL0_RINT_SHIFT;
+		}
 		break;
 	case SX9324_REG_AFE_CTRL4:
 	case SX9324_REG_AFE_CTRL7:
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 2/6] device property: Add fwnode_property_match_property_string()

[Jonathan Cameron]

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,

Re: [PATCH v1 2/6] device property: Add fwnode_property_match_property_string()

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/6] device property: Add fwnode_property_match_property_string()

[Jonathan Cameron]

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

Re: [PATCH v1 1/6] device property: Use fwnode_property_string_array_count()

[Rafael J. Wysocki]

On Tue, Aug 8, 2023 at 6:31 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> Use fwnode_property_string_array_count() instead of open coded variant.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Acked-by: Rafael J. Wysocki <rafael@kernel.org>

> ---
>  drivers/base/property.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 8c40abed7852..3bb9505f1631 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -473,7 +473,7 @@ int fwnode_property_match_string(const struct fwnode_handle *fwnode,
>         const char **values;
>         int nval, ret;
>
> -       nval = fwnode_property_read_string_array(fwnode, propname, NULL, 0);
> +       nval = fwnode_property_string_array_count(fwnode, propname);
>         if (nval < 0)
>                 return nval;
>
> --
> 2.40.0.1.gaa8946217a0b
>

Re: [PATCH v1 2/6] device property: Add fwnode_property_match_property_string()

[Rafael J. Wysocki]

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!

Re: [PATCH v1 2/6] device property: Add fwnode_property_match_property_string()

[Andy Shevchenko]

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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/6] device property: Add fwnode_property_match_property_string()

[Jonathan Cameron]

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

Re: [PATCH v1 2/6] device property: Add fwnode_property_match_property_string()

[Andy Shevchenko]

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!

-- 
With Best Regards,
Andy Shevchenko