Hi,
On Mon, Jun 29, 2015 at 06:44:23PM +0200, Dr. H. Nikolaus Schaller wrote:
[...]
> >> + list_for_each_entry(uart, &uart_list, head) {
> >> + if (node != uart->dev->of_node)
> >> + continue;
> >> +
> >> + return uart;
> >
> > We can easily save three lines here :)
>
> Hm. We have copied from here:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/phy/phy.c?id=refs/tags/v4.1#n65
>
> So please let us know how you want to save 3 lines.
Sorry for not being specific, I just meant that it could be written as
if (node == uart->dev->of_node)
return uart;
> >> +/**
> >> + * devm_serial_get_uart_by_phandle - find the uart by phandle
> >> + * @dev - device that requests this uart
> >> + * @phandle - name of the property holding the uart phandle value
> >> + * @index - the index of the uart
> >> + *
> >> + * Returns the uart_port associated with the given phandle value,
> >> + * after getting a refcount to it, -ENODEV if there is no such uart or
> >> + * -EPROBE_DEFER if there is a phandle to the uart, but the device is
> >> + * not yet loaded. While at that, it also associates the device with
> >> + * the uart using devres. On driver detach, release function is invoked
> >> + * on the devres data, then, devres data is freed.
> >
> > Add -ENOMEM and -EINVAL, remove -EPROBE_DEFER?
>
> Well, if the device is not loaded it means the caller must return
> -EPROBE_DEFER
> anyways since it can’t complete it’s probe function.
That was my mistake, from the first sight I hadn't found where
-EPROBE_DEFER is actually returned from the code and thus decided that
kernel-doc has an error. But now I see it, thanks.
> >> +
> >> + *ptr = uart;
> >> + devres_add(dev, ptr);
> >
> > What is the point of assigning value to *ptr?
>
> Good question. I think it is necessary to store a copy of the found uart/phy
> instead of a reference.
> Therefore the assignment to *ptr copies into the new memory area allocated
> above by
>
> ptr = devres_alloc(devm_serial_uart_release, sizeof(*ptr), GFP_KERNEL);
>
> This makes the dev the owner of the data - instead of unknown ownership
> before.
>
> It is the same as here (but it might be wrong/unnecessary there as well):
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/phy/phy.c?id=refs/tags/v4.1#n209
>
> Maybe it has something to do with the unfinished devm_serial_uart_release().
Indeed. I haven't noticed this through the first quick look into the
code. Thank you for explanation.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
