On Thu, Jun 27, 2019 at 12:23 PM Charles Keepax
<ckeepax@xxxxxxxxxxxxxxxxxxxxx> 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@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.
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.
