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@xxxxxxxxxxxxxxxxxxxxx> 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
