On Tue, 23 Feb 2016 18:45:17 +0000
Mark Rutland <mark.rutland@xxxxxxx> wrote:
> On Tue, Feb 23, 2016 at 01:17:25PM -0500, David Rivshin (Allworx) wrote:
> > From: David Rivshin <drivshin@xxxxxxxxxxx>
> > +static int is31fl32xx_parse_child_dt(const struct device *dev,
> > + const struct device_node *child,
> > + struct is31fl32xx_led_data *led_data)
> > +{
> > + struct led_classdev *cdev = &led_data->cdev;
> > + int ret = 0;
> > + u32 reg;
> > +
> > + cdev->name = of_get_property(child, "label", NULL) ? : child->name;
>
> We really shouldn't be spreading of_get_property into more drivers. It's
> generally not what you want, and doesn't do appropriate sanity checking.
Sorry, I copied the basic DT parsing from some other led driver (leds-pwm,
I think), and did not check if there was a better way.
> Use of_property_read_string, though you may need to copy the result.
Will do.
As far as copying the result, this is an area I'm fuzzy on. If I
understand correctly the device_nodes are reference counted, and
of_property_read_string() returns a pointer to the data inside the
device_node. So it seems I'd either need to hold onto a reference,
or make a copy of the string.
devm_kstrdup() seems like it would be an easy way to go, although
maybe not the most efficient.
Using of_node_get() would seem to be more efficient, but then what
would be the right way to release that reference on remove()?
- Does that already happen in some infrastructure when using DT
probing?
- Is there a devm_*() helper function that should be used?
- Or would it be best to just keep a pointer to the device_node so
that of_node_put() can be called on remove()?
Also, I have yet to find an example of a driver which either copies
the string or does an of_node_get() on the node. In fact just by a
count of files, it seems very few have any hope of doing either:
$ find drivers/ -name '*.c' -exec grep of_property_read_string -l {} + \
| wc -l
197
$ find drivers/ -name '*.c' -exec grep of_property_read_string -l {} + \
| xargs grep -l of_node_get | wc -l
15
$ find drivers/ -name '*.c' -exec grep of_property_read_string -l {} + \
| xargs grep -l strdup | wc -l
11
So I'm very much hoping that there's some infrastructure automatically
handling the appropriate reference counting, so that the nodes stay
around (at least) as long as the driver is instantiated...
> > +
> > + ret = of_property_read_u32(child, "reg", ®);
> > + if (ret || reg < 1 || reg > led_data->priv->cdef->channels) {
> > + dev_err(dev,
> > + "Child node %s does not have a valid reg property\n",
> > + child->name);
> > + return -EINVAL;
> > + }
> > + led_data->channel = reg;
> > +
> > + cdev->default_trigger = of_get_property(child, "linux,default-trigger",
> > + NULL);
>
> Likewise.
Will take care of this same as above.
Thanks!
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
