linux-acpi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] device property: Add new array helper
@ 2019-06-26 15:36 Charles Keepax
  2019-06-26 15:36 ` [PATCH 2/2] ASoC: madera: Read device tree configuration Charles Keepax
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Charles Keepax @ 2019-06-26 15:36 UTC (permalink / raw)
  To: broonie; +Cc: gregkh, rafael, linux-acpi, lgirdwood, alsa-devel, patches

It is fairly common to want to read an integer array property
that is composed of an unknown number of fixed size integer
groups. For example, say each group consists of three values
which correspond to the settings for one input on the device
and the driver supports several chips with different numbers
of inputs.

Add a new helper function to provide this functionality, it
differs for the existing helpers in that it allows reading a
smaller number of values than the full array size and checks
that the number of values read is a multiple of the group size.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 drivers/base/property.c  | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/property.h |  2 ++
 2 files changed, 50 insertions(+)

diff --git a/drivers/base/property.c b/drivers/base/property.c
index 348b37e64944c..656d21e01a648 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -133,6 +133,54 @@ int device_property_read_u32_array(struct device *dev, const char *propname,
 EXPORT_SYMBOL_GPL(device_property_read_u32_array);
 
 /**
+ * device_property_read_u32_2darray - return a 2d u32 array property of a device
+ * @dev: Device to get the property of
+ * @propname: Name of the property
+ * @val: The values are stored here or %NULL to return the number of values
+ * @nval: Size of the @val array
+ * @multiple: Number of entries in each block of data
+ *
+ * Function reads an array of u32 properties split up into fixed size
+ * sub-groups, with @propname from the device firmware description and
+ * stores them to @val if found.
+ *
+ * Return: Number of values read
+ *	   %0 if the property was not found,
+ *	   %-EINVAL if given arguments are not valid,
+ *	   %-ENODATA if the property does not have a value,
+ *	   %-EPROTO if the property is not an array of numbers,
+ *	   %-EOVERFLOW if the size of the property is not as expected.
+ *	   %-ENXIO if no suitable firmware interface is present.
+ */
+int device_property_read_u32_2darray(struct device *dev, const char *propname,
+				     u32 *val, size_t nval, int multiple)
+{
+	int n, ret;
+
+	n = device_property_read_u32_array(dev, propname, NULL, 0);
+	if (n == -EINVAL) {
+		return 0;	/* missing, ignore */
+	} else if (n < 0) {
+		dev_warn(dev, "%s malformed (%d)\n", propname, n);
+		return n;
+	} else if ((n % multiple) != 0) {
+		dev_warn(dev, "%s not a multiple of %d entries\n",
+			 propname, multiple);
+		return -EOVERFLOW;
+	}
+
+	if (n > nval)
+		n = nval;
+
+	ret = device_property_read_u32_array(dev, propname, val, n);
+	if (ret < 0)
+		return ret;
+	else
+		return n;
+}
+EXPORT_SYMBOL_GPL(device_property_read_u32_2darray);
+
+/**
  * device_property_read_u64_array - return a u64 array property of a device
  * @dev: Device to get the property of
  * @propname: Name of the property
diff --git a/include/linux/property.h b/include/linux/property.h
index e9caa290cda52..5ab0b4a7d34a2 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -40,6 +40,8 @@ int device_property_read_u16_array(struct device *dev, const char *propname,
 				   u16 *val, size_t nval);
 int device_property_read_u32_array(struct device *dev, const char *propname,
 				   u32 *val, size_t nval);
+int device_property_read_u32_2darray(struct device *dev, const char *propname,
+				     u32 *val, size_t nval, int multiple);
 int device_property_read_u64_array(struct device *dev, const char *propname,
 				   u64 *val, size_t nval);
 int device_property_read_string_array(struct device *dev, const char *propname,
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/2] ASoC: madera: Read device tree configuration
  2019-06-26 15:36 [PATCH 1/2] device property: Add new array helper Charles Keepax
@ 2019-06-26 15:36 ` Charles Keepax
  2019-06-27  0:15   ` Greg KH
  2019-06-27  0:15 ` [PATCH 1/2] device property: Add new array helper Greg KH
  2019-06-27  9:39 ` Rafael J. Wysocki
  2 siblings, 1 reply; 9+ messages in thread
From: Charles Keepax @ 2019-06-26 15:36 UTC (permalink / raw)
  To: broonie; +Cc: gregkh, rafael, linux-acpi, lgirdwood, alsa-devel, patches

Read the configuration of the Madera sound driver from device tree using
the new device tree helper function.

Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

Note this patch depends on patches that are currently going through Mark's
tree so probably best if this one also goes through there.

Thanks,
Charles

 sound/soc/codecs/madera.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/sound/soc/codecs/madera.c b/sound/soc/codecs/madera.c
index 1b1be19a2f991..8fe16e4e19c7b 100644
--- a/sound/soc/codecs/madera.c
+++ b/sound/soc/codecs/madera.c
@@ -300,6 +300,68 @@ int madera_free_overheat(struct madera_priv *priv)
 }
 EXPORT_SYMBOL_GPL(madera_free_overheat);
 
+static void madera_prop_get_inmode(struct madera_priv *priv)
+{
+	struct madera *madera = priv->madera;
+	struct madera_codec_pdata *pdata = &madera->pdata.codec;
+	u32 tmp[MADERA_MAX_INPUT * MADERA_MAX_MUXED_CHANNELS];
+	int n, i, in_idx, ch_idx;
+
+	BUILD_BUG_ON(ARRAY_SIZE(pdata->inmode) != MADERA_MAX_INPUT);
+	BUILD_BUG_ON(ARRAY_SIZE(pdata->inmode[0]) != MADERA_MAX_MUXED_CHANNELS);
+
+	n = device_property_read_u32_2darray(madera->dev, "cirrus,inmode",
+					     tmp, ARRAY_SIZE(tmp),
+					     MADERA_MAX_MUXED_CHANNELS);
+	if (n < 0)
+		return;
+
+	in_idx = 0;
+	ch_idx = 0;
+	for (i = 0; i < n; ++i) {
+		pdata->inmode[in_idx][ch_idx] = tmp[i];
+
+		if (++ch_idx == MADERA_MAX_MUXED_CHANNELS) {
+			ch_idx = 0;
+			++in_idx;
+		}
+	}
+}
+
+static void madera_prop_get_pdata(struct madera_priv *priv)
+{
+	struct madera *madera = priv->madera;
+	struct madera_codec_pdata *pdata = &madera->pdata.codec;
+	u32 out_mono[ARRAY_SIZE(pdata->out_mono)];
+	int i, n;
+
+	madera_prop_get_inmode(priv);
+
+	n = device_property_read_u32_2darray(madera->dev, "cirrus,out-mono",
+					     out_mono, ARRAY_SIZE(out_mono), 1);
+	if (n > 0)
+		for (i = 0; i < n; ++i)
+			pdata->out_mono[i] = !!out_mono[i];
+
+	device_property_read_u32_2darray(madera->dev,
+					 "cirrus,max-channels-clocked",
+					 pdata->max_channels_clocked,
+					 ARRAY_SIZE(pdata->max_channels_clocked),
+					 1);
+
+	device_property_read_u32_2darray(madera->dev, "cirrus,pdm-fmt",
+					 pdata->pdm_fmt,
+					 ARRAY_SIZE(pdata->pdm_fmt), 1);
+
+	device_property_read_u32_2darray(madera->dev, "cirrus,pdm-mute",
+					 pdata->pdm_mute,
+					 ARRAY_SIZE(pdata->pdm_mute), 1);
+
+	device_property_read_u32_2darray(madera->dev, "cirrus,dmic-ref",
+					 pdata->dmic_ref,
+					 ARRAY_SIZE(pdata->dmic_ref), 1);
+}
+
 int madera_core_init(struct madera_priv *priv)
 {
 	int i;
@@ -308,6 +370,9 @@ int madera_core_init(struct madera_priv *priv)
 	BUILD_BUG_ON(!madera_mixer_texts[MADERA_NUM_MIXER_INPUTS - 1]);
 	BUILD_BUG_ON(!madera_mixer_values[MADERA_NUM_MIXER_INPUTS - 1]);
 
+	if (!dev_get_platdata(priv->madera->dev))
+		madera_prop_get_pdata(priv);
+
 	mutex_init(&priv->rate_lock);
 
 	for (i = 0; i < MADERA_MAX_HP_OUTPUT; i++)
-- 
2.11.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] device property: Add new array helper
  2019-06-26 15:36 [PATCH 1/2] device property: Add new array helper Charles Keepax
  2019-06-26 15:36 ` [PATCH 2/2] ASoC: madera: Read device tree configuration Charles Keepax
@ 2019-06-27  0:15 ` Greg KH
  2019-06-27  9:17   ` Rafael J. Wysocki
  2019-06-27  9:39 ` Rafael J. Wysocki
  2 siblings, 1 reply; 9+ messages in thread
From: Greg KH @ 2019-06-27  0:15 UTC (permalink / raw)
  To: Charles Keepax
  Cc: broonie, rafael, linux-acpi, lgirdwood, alsa-devel, patches

On Wed, Jun 26, 2019 at 04:36:10PM +0100, Charles Keepax wrote:
> It is fairly common to want to read an integer array property
> that is composed of an unknown number of fixed size integer
> groups. For example, say each group consists of three values
> which correspond to the settings for one input on the device
> and the driver supports several chips with different numbers
> of inputs.
> 
> Add a new helper function to provide this functionality, it
> differs for the existing helpers in that it allows reading a
> smaller number of values than the full array size and checks
> that the number of values read is a multiple of the group size.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
>  drivers/base/property.c  | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/property.h |  2 ++
>  2 files changed, 50 insertions(+)

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] ASoC: madera: Read device tree configuration
  2019-06-26 15:36 ` [PATCH 2/2] ASoC: madera: Read device tree configuration Charles Keepax
@ 2019-06-27  0:15   ` Greg KH
  0 siblings, 0 replies; 9+ messages in thread
From: Greg KH @ 2019-06-27  0:15 UTC (permalink / raw)
  To: Charles Keepax
  Cc: broonie, rafael, linux-acpi, lgirdwood, alsa-devel, patches

On Wed, Jun 26, 2019 at 04:36:11PM +0100, Charles Keepax wrote:
> Read the configuration of the Madera sound driver from device tree using
> the new device tree helper function.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
> 
> Note this patch depends on patches that are currently going through Mark's
> tree so probably best if this one also goes through there.

No objection from me for him to take the previous one with this one in
his tree.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] device property: Add new array helper
  2019-06-27  0:15 ` [PATCH 1/2] device property: Add new array helper Greg KH
@ 2019-06-27  9:17   ` Rafael J. Wysocki
  0 siblings, 0 replies; 9+ messages in thread
From: Rafael J. Wysocki @ 2019-06-27  9:17 UTC (permalink / raw)
  To: Greg KH
  Cc: Charles Keepax, Mark Brown, Rafael J. Wysocki,
	ACPI Devel Maling List, Liam Girdwood,
	moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
	patches

On Thu, Jun 27, 2019 at 2:15 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Jun 26, 2019 at 04:36:10PM +0100, Charles Keepax wrote:
> > It is fairly common to want to read an integer array property
> > that is composed of an unknown number of fixed size integer
> > groups. For example, say each group consists of three values
> > which correspond to the settings for one input on the device
> > and the driver supports several chips with different numbers
> > of inputs.
> >
> > Add a new helper function to provide this functionality, it
> > differs for the existing helpers in that it allows reading a
> > smaller number of values than the full array size and checks
> > that the number of values read is a multiple of the group size.
> >
> > Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > ---
> >  drivers/base/property.c  | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/property.h |  2 ++
> >  2 files changed, 50 insertions(+)
>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Well, I'm kind of not convinced about this one.

More comments in direct reply to the patch message.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] device property: Add new array helper
  2019-06-26 15:36 [PATCH 1/2] device property: Add new array helper Charles Keepax
  2019-06-26 15:36 ` [PATCH 2/2] ASoC: madera: Read device tree configuration Charles Keepax
  2019-06-27  0:15 ` [PATCH 1/2] device property: Add new array helper Greg KH
@ 2019-06-27  9:39 ` Rafael J. Wysocki
  2019-06-27 10:23   ` Charles Keepax
  2 siblings, 1 reply; 9+ messages in thread
From: Rafael J. Wysocki @ 2019-06-27  9:39 UTC (permalink / raw)
  To: Charles Keepax
  Cc: Mark Brown, Greg Kroah-Hartman, Rafael J. Wysocki,
	ACPI Devel Maling List, Liam Girdwood,
	moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
	patches

On Wed, Jun 26, 2019 at 5:36 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> It is fairly common to want to read an integer array property
> that is composed of an unknown number of fixed size integer
> groups. For example, say each group consists of three values
> which correspond to the settings for one input on the device
> and the driver supports several chips with different numbers
> of inputs.
>
> Add a new helper function to provide this functionality, it
> differs for the existing helpers in that it allows reading a
> smaller number of values than the full array size and checks
> that the number of values read is a multiple of the group size.

I'm not convinced.

> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
>  drivers/base/property.c  | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/property.h |  2 ++
>  2 files changed, 50 insertions(+)
>
> diff --git a/drivers/base/property.c b/drivers/base/property.c
> index 348b37e64944c..656d21e01a648 100644
> --- a/drivers/base/property.c
> +++ b/drivers/base/property.c
> @@ -133,6 +133,54 @@ int device_property_read_u32_array(struct device *dev, const char *propname,
>  EXPORT_SYMBOL_GPL(device_property_read_u32_array);
>
>  /**
> + * device_property_read_u32_2darray - return a 2d u32 array property of a device
> + * @dev: Device to get the property of
> + * @propname: Name of the property
> + * @val: The values are stored here or %NULL to return the number of values
> + * @nval: Size of the @val array
> + * @multiple: Number of entries in each block of data
> + *
> + * Function reads an array of u32 properties split up into fixed size
> + * sub-groups, with @propname from the device firmware description and
> + * stores them to @val if found.
> + *
> + * Return: Number of values read
> + *        %0 if the property was not found,
> + *        %-EINVAL if given arguments are not valid,
> + *        %-ENODATA if the property does not have a value,
> + *        %-EPROTO if the property is not an array of numbers,
> + *        %-EOVERFLOW if the size of the property is not as expected.
> + *        %-ENXIO if no suitable firmware interface is present.
> + */
> +int device_property_read_u32_2darray(struct device *dev, const char *propname,
> +                                    u32 *val, size_t nval, int multiple)
> +{
> +       int n, ret;
> +
> +       n = device_property_read_u32_array(dev, propname, NULL, 0);
> +       if (n == -EINVAL) {
> +               return 0;       /* missing, ignore */

Why can't the caller use the (scheduled for merging in the 5.3 cycle)
new device_property_count_u32() to get the size of the array?

> +       } else if (n < 0) {
> +               dev_warn(dev, "%s malformed (%d)\n", propname, n);

Why dev_warn()?  Is there any reason real for anything higher-level
that dev_dbg() here?

> +               return n;
> +       } else if ((n % multiple) != 0) {

I guess the reason why this matters is that the caller expects a
certain number of full "rows" and n values are read.  Why not to
discard the extra values instead of returning an error here?

> +               dev_warn(dev, "%s not a multiple of %d entries\n",
> +                        propname, multiple);
> +               return -EOVERFLOW;

Why this error code?

> +       }
> +
> +       if (n > nval)
> +               n = nval;
> +
> +       ret = device_property_read_u32_array(dev, propname, val, n);

So this reads "copy at most nval values from the array property".

If that's really what you need, it can be done in two lines of code in
prospective callers of this wrapper.

> +       if (ret < 0)
> +               return ret;
> +       else
> +               return n;
> +}
> +EXPORT_SYMBOL_GPL(device_property_read_u32_2darray);
> +
> +/**
>   * device_property_read_u64_array - return a u64 array property of a device
>   * @dev: Device to get the property of
>   * @propname: Name of the property
> diff --git a/include/linux/property.h b/include/linux/property.h
> index e9caa290cda52..5ab0b4a7d34a2 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -40,6 +40,8 @@ int device_property_read_u16_array(struct device *dev, const char *propname,
>                                    u16 *val, size_t nval);
>  int device_property_read_u32_array(struct device *dev, const char *propname,
>                                    u32 *val, size_t nval);
> +int device_property_read_u32_2darray(struct device *dev, const char *propname,
> +                                    u32 *val, size_t nval, int multiple);
>  int device_property_read_u64_array(struct device *dev, const char *propname,
>                                    u64 *val, size_t nval);
>  int device_property_read_string_array(struct device *dev, const char *propname,
> --

I don't see much value in this change, sorry.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] device property: Add new array helper
  2019-06-27  9:39 ` Rafael J. Wysocki
@ 2019-06-27 10:23   ` Charles Keepax
  2019-06-27 11:02     ` Rafael J. Wysocki
  0 siblings, 1 reply; 9+ messages in thread
From: Charles Keepax @ 2019-06-27 10:23 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Mark Brown, Greg Kroah-Hartman, ACPI Devel Maling List,
	Liam Girdwood,
	moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
	patches

On Thu, Jun 27, 2019 at 11:39:10AM +0200, Rafael J. Wysocki wrote:
> On Wed, Jun 26, 2019 at 5:36 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> > +       n = device_property_read_u32_array(dev, propname, NULL, 0);
> > +       if (n == -EINVAL) {
> > +               return 0;       /* missing, ignore */
> 
> Why can't the caller use the (scheduled for merging in the 5.3 cycle)
> new device_property_count_u32() to get the size of the array?
> 

I wasn't aware of it, am now.

> > +       } else if (n < 0) {
> > +               dev_warn(dev, "%s malformed (%d)\n", propname, n);
> 
> Why dev_warn()?  Is there any reason real for anything higher-level
> that dev_dbg() here?
> 

Nice to know that your DT wasn't valid, but could be left to the
caller I guess.

> > +               return n;
> > +       } else if ((n % multiple) != 0) {
> 
> I guess the reason why this matters is that the caller expects a
> certain number of full "rows" and n values are read.  Why not to
> discard the extra values instead of returning an error here?
>

No reason really why it couldn't. Although my expectation would
generally be this helper is for reading a variable number of
fixed size groups. As in each group represents a "whole" item but
you don't know how many of those you have.

> > +               dev_warn(dev, "%s not a multiple of %d entries\n",
> > +                        propname, multiple);
> > +               return -EOVERFLOW;
> 
> Why this error code?
> 

As that is the error code all the device_property functions
return when the size is not as expected.

> > +       if (n > nval)
> > +               n = nval;
> > +
> > +       ret = device_property_read_u32_array(dev, propname, val, n);
> 
> So this reads "copy at most nval values from the array property".
> 
> If that's really what you need, it can be done in two lines of code in
> prospective callers of this wrapper.
> 

Indeed the helper here is basically exactly what would be done in
the caller if no helper existed.

> > +int device_property_read_u32_2darray(struct device *dev, const char *propname,
> > +                                    u32 *val, size_t nval, int multiple);
> >  int device_property_read_u64_array(struct device *dev, const char *propname,
> >                                    u64 *val, size_t nval);
> >  int device_property_read_string_array(struct device *dev, const char *propname,
> > --
> 
> I don't see much value in this change, sorry.

That is fine, I don't have any problem with the helper living
within our driver instead. Basically the issue from my side is I
need to read 6 different device tree properties all of which
require this behaviour, ie. read a variable number of fixed
groups and check I have whole groups. Open coding this for each
call is indeed only going to be 5-10 lines of code for each one
but since there are 6 of them it makes sense to put those 5-10
lines into a helper and have 5-10 lines not 30-60 lines. Seemed
the helper might be generally more useful, but if it is not then
it can go back into the driver.

Thanks,
Charles

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] device property: Add new array helper
  2019-06-27 10:23   ` Charles Keepax
@ 2019-06-27 11:02     ` Rafael J. Wysocki
  2019-06-27 12:33       ` Charles Keepax
  0 siblings, 1 reply; 9+ messages in thread
From: Rafael J. Wysocki @ 2019-06-27 11:02 UTC (permalink / raw)
  To: Charles Keepax
  Cc: Rafael J. Wysocki, Mark Brown, Greg Kroah-Hartman,
	ACPI Devel Maling List, Liam Girdwood,
	moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
	patches

On Thu, Jun 27, 2019 at 12:23 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> On Thu, Jun 27, 2019 at 11:39:10AM +0200, Rafael J. Wysocki wrote:
> > On Wed, Jun 26, 2019 at 5:36 PM Charles Keepax
> > <ckeepax@opensource.cirrus.com> wrote:
> > > +       n = device_property_read_u32_array(dev, propname, NULL, 0);
> > > +       if (n == -EINVAL) {
> > > +               return 0;       /* missing, ignore */
> >
> > Why can't the caller use the (scheduled for merging in the 5.3 cycle)
> > new device_property_count_u32() to get the size of the array?
> >
>
> I wasn't aware of it, am now.
>
> > > +       } else if (n < 0) {
> > > +               dev_warn(dev, "%s malformed (%d)\n", propname, n);
> >
> > Why dev_warn()?  Is there any reason real for anything higher-level
> > that dev_dbg() here?
> >
>
> Nice to know that your DT wasn't valid, but could be left to the
> caller I guess.

Right.  And only print the message when the caller really is going to fail.

> > > +               return n;
> > > +       } else if ((n % multiple) != 0) {
> >
> > I guess the reason why this matters is that the caller expects a
> > certain number of full "rows" and n values are read.  Why not to
> > discard the extra values instead of returning an error here?
> >
>
> No reason really why it couldn't. Although my expectation would
> generally be this helper is for reading a variable number of
> fixed size groups. As in each group represents a "whole" item but
> you don't know how many of those you have.

That really depends on how the property is defined and which is what
the caller knows.

> > > +               dev_warn(dev, "%s not a multiple of %d entries\n",
> > > +                        propname, multiple);
> > > +               return -EOVERFLOW;
> >
> > Why this error code?
> >
>
> As that is the error code all the device_property functions
> return when the size is not as expected.

To be precise, when there is a mismatch between the storage space
supplied by the caller and the property size.  That does not seem to
be the case here.  It is rather about the property format being not as
expected and that would be -EPROTO.

> > > +       if (n > nval)
> > > +               n = nval;
> > > +
> > > +       ret = device_property_read_u32_array(dev, propname, val, n);
> >
> > So this reads "copy at most nval values from the array property".
> >
> > If that's really what you need, it can be done in two lines of code in
> > prospective callers of this wrapper.
> >
>
> Indeed the helper here is basically exactly what would be done in
> the caller if no helper existed.
>
> > > +int device_property_read_u32_2darray(struct device *dev, const char *propname,
> > > +                                    u32 *val, size_t nval, int multiple);
> > >  int device_property_read_u64_array(struct device *dev, const char *propname,
> > >                                    u64 *val, size_t nval);
> > >  int device_property_read_string_array(struct device *dev, const char *propname,
> > > --
> >
> > I don't see much value in this change, sorry.
>
> That is fine, I don't have any problem with the helper living
> within our driver instead. Basically the issue from my side is I
> need to read 6 different device tree properties all of which
> require this behaviour, ie. read a variable number of fixed
> groups and check I have whole groups. Open coding this for each
> call is indeed only going to be 5-10 lines of code

Exactly two:

n = device_property_count_u32(dev, name);
ret = device_property_read_u32_array(dev, propname, val, n > nval ? nval : n);

And I would be fine with adding wrappers like this (and for the other
data types too for that matter).

It would take more lines if you wanted to complain about the format,
but as pointed out above, that would need to be done in the caller
anyway.

> for each one  but since there are 6 of them it makes sense to put those 5-10
> lines into a helper and have 5-10 lines not 30-60 lines. Seemed
> the helper might be generally more useful, but if it is not then
> it can go back into the driver.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] device property: Add new array helper
  2019-06-27 11:02     ` Rafael J. Wysocki
@ 2019-06-27 12:33       ` Charles Keepax
  0 siblings, 0 replies; 9+ messages in thread
From: Charles Keepax @ 2019-06-27 12:33 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Mark Brown, Greg Kroah-Hartman, ACPI Devel Maling List,
	Liam Girdwood,
	moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM...,
	patches

On Thu, Jun 27, 2019 at 01:02:32PM +0200, Rafael J. Wysocki wrote:
> On Thu, Jun 27, 2019 at 12:23 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> > On Thu, Jun 27, 2019 at 11:39:10AM +0200, Rafael J. Wysocki wrote:
> > > On Wed, Jun 26, 2019 at 5:36 PM Charles Keepax
> > > <ckeepax@opensource.cirrus.com> wrote:
> > That is fine, I don't have any problem with the helper living
> > within our driver instead. Basically the issue from my side is I
> > need to read 6 different device tree properties all of which
> > require this behaviour, ie. read a variable number of fixed
> > groups and check I have whole groups. Open coding this for each
> > call is indeed only going to be 5-10 lines of code
> 
> Exactly two:
> 
> n = device_property_count_u32(dev, name);
> ret = device_property_read_u32_array(dev, propname, val, n > nval ? nval : n);
> 
> And I would be fine with adding wrappers like this (and for the other
> data types too for that matter).
> 
> It would take more lines if you wanted to complain about the format,
> but as pointed out above, that would need to be done in the caller
> anyway.
> 

Ok I think that helps me to follow the situation. If
device_property_count cuts down the code required and leaves the
majority of the code as printing the messages which then wants to
live in the end driver anyways it probably isn't worth adding a
core helper for this.

Thank you for the review and the explanation. I will update the
driver patches to use the new function and resend those.

Thanks,
Charles

> > for each one  but since there are 6 of them it makes sense to put those 5-10
> > lines into a helper and have 5-10 lines not 30-60 lines. Seemed
> > the helper might be generally more useful, but if it is not then
> > it can go back into the driver.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-06-27 12:33 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-26 15:36 [PATCH 1/2] device property: Add new array helper Charles Keepax
2019-06-26 15:36 ` [PATCH 2/2] ASoC: madera: Read device tree configuration Charles Keepax
2019-06-27  0:15   ` Greg KH
2019-06-27  0:15 ` [PATCH 1/2] device property: Add new array helper Greg KH
2019-06-27  9:17   ` Rafael J. Wysocki
2019-06-27  9:39 ` Rafael J. Wysocki
2019-06-27 10:23   ` Charles Keepax
2019-06-27 11:02     ` Rafael J. Wysocki
2019-06-27 12:33       ` Charles Keepax

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