From: "Rafael J. Wysocki" <rafael@kernel.org> To: Charles Keepax <ckeepax@opensource.cirrus.com> Cc: Mark Brown <broonie@kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "Rafael J. Wysocki" <rafael@kernel.org>, ACPI Devel Maling List <linux-acpi@vger.kernel.org>, Liam Girdwood <lgirdwood@gmail.com>, "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." <alsa-devel@alsa-project.org>, patches@opensource.cirrus.com Subject: Re: [PATCH 1/2] device property: Add new array helper Date: Thu, 27 Jun 2019 11:39:10 +0200 [thread overview] Message-ID: <CAJZ5v0hvN=8YmF+v6wKx9mQ=DRosAtK7QU=EWYf5PXEDsn4FEQ@mail.gmail.com> (raw) In-Reply-To: <20190626153611.10170-1-ckeepax@opensource.cirrus.com> 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.
WARNING: multiple messages have this Message-ID (diff)
From: "Rafael J. Wysocki" <rafael@kernel.org> To: Charles Keepax <ckeepax@opensource.cirrus.com> Cc: "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." <alsa-devel@alsa-project.org>, "Rafael J. Wysocki" <rafael@kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Liam Girdwood <lgirdwood@gmail.com>, ACPI Devel Maling List <linux-acpi@vger.kernel.org>, Mark Brown <broonie@kernel.org>, patches@opensource.cirrus.com Subject: Re: [PATCH 1/2] device property: Add new array helper Date: Thu, 27 Jun 2019 11:39:10 +0200 [thread overview] Message-ID: <CAJZ5v0hvN=8YmF+v6wKx9mQ=DRosAtK7QU=EWYf5PXEDsn4FEQ@mail.gmail.com> (raw) In-Reply-To: <20190626153611.10170-1-ckeepax@opensource.cirrus.com> 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.
next prev parent reply other threads:[~2019-06-27 9:39 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 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-26 15:36 ` [PATCH 2/2] ASoC: madera: Read device tree configuration Charles Keepax 2019-06-26 15:36 ` Charles Keepax 2019-06-27 0:15 ` Greg KH 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 0:15 ` Greg KH 2019-06-27 9:17 ` Rafael J. Wysocki 2019-06-27 9:17 ` Rafael J. Wysocki 2019-06-27 9:39 ` Rafael J. Wysocki [this message] 2019-06-27 9:39 ` Rafael J. Wysocki 2019-06-27 10:23 ` Charles Keepax 2019-06-27 10:23 ` Charles Keepax 2019-06-27 11:02 ` Rafael J. Wysocki 2019-06-27 11:02 ` Rafael J. Wysocki 2019-06-27 12:33 ` Charles Keepax 2019-06-27 12:33 ` Charles Keepax
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='CAJZ5v0hvN=8YmF+v6wKx9mQ=DRosAtK7QU=EWYf5PXEDsn4FEQ@mail.gmail.com' \ --to=rafael@kernel.org \ --cc=alsa-devel@alsa-project.org \ --cc=broonie@kernel.org \ --cc=ckeepax@opensource.cirrus.com \ --cc=gregkh@linuxfoundation.org \ --cc=lgirdwood@gmail.com \ --cc=linux-acpi@vger.kernel.org \ --cc=patches@opensource.cirrus.com \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.