Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752424AbbF3IJX (ORCPT ); Tue, 30 Jun 2015 04:09:23 -0400 Received: from out4-smtp.messagingengine.com ([66.111.4.28]:49602 "EHLO out4-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750803AbbF3IJN (ORCPT ); Tue, 30 Jun 2015 04:09:13 -0400 X-Sasl-enc: DmYcdOeuv1R64Alw7Qpu4vaqgVKqSUIDGHnWm5Ri7XOV 1435651751 Date: Tue, 30 Jun 2015 11:09:10 +0300 From: Sergei Zviagintsev To: "Dr. H. Nikolaus Schaller" Cc: Marek Belisko , Jiri Slaby , Greg Kroah-Hartman , Arnd Bergmann , Peter Hurley , Mark Rutland , One Thousand Gnomes , Sebastian Reichel , NeilBrown , Grant Likely , LKML , linux-serial@vger.kernel.org, Rob Herring , Pawel Moll Subject: Re: [PATCH RFC v2 1/3] tty: serial core: provide method to search uart by phandle Message-ID: <20150630080910.GC17917@localhost.localdomain> References: <1435520786-31867-1-git-send-email-marek@goldelico.com> <1435520786-31867-2-git-send-email-marek@goldelico.com> <20150628213434.GB21426@localhost.localdomain> <72C0781B-5DEC-474C-83B5-3CD6B8D68890@goldelico.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <72C0781B-5DEC-474C-83B5-3CD6B8D68890@goldelico.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2765 Lines: 71 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/