[alsa-devel] [PATCH 1/2] device property: Add new array helper

[Charles Keepax]

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 at 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