From mboxrd@z Thu Jan 1 00:00:00 1970 From: Al Stone Subject: Re: [PATCH v2 15/16] device property: Add fwnode_get_next_parent() Date: Wed, 8 Feb 2017 09:49:09 -0700 Message-ID: References: <1486052546-19257-1-git-send-email-sakari.ailus@linux.intel.com> <9a481aec-92e6-ac01-b825-167bf73dfb11@redhat.com> <6293579.fEEljmGD4Z@aspire.rjw.lan> <7605668.5ZxNsBFnEG@aspire.rjw.lan> Reply-To: ahs3-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <7605668.5ZxNsBFnEG-yvgW3jdyMHm1GS7QM15AGw@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: "Rafael J. Wysocki" , Sakari Ailus , mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org Cc: linux-acpi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, sudeep.holla-5wv7dgnIgG8@public.gmane.org, lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org, rafael-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, mark.rutland-5wv7dgnIgG8@public.gmane.org, broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org List-Id: linux-acpi@vger.kernel.org On 02/08/2017 05:19 AM, Rafael J. Wysocki wrote: > On Wednesday, February 08, 2017 12:50:25 PM Rafael J. Wysocki wrote: >> On Tuesday, February 07, 2017 11:57:55 AM Al Stone wrote: >>> On 02/02/2017 09:22 AM, Sakari Ailus wrote: >>>> In order to differentiate the functionality between dropping a reference >>>> to the node (or not) for the benefit of OF, introduce >>>> fwnode_get_next_parent(). >>>> >>>> Signed-off-by: Sakari Ailus >>>> --- >>>> drivers/base/property.c | 29 +++++++++++++++++++++++++++++ >>>> include/linux/property.h | 1 + >>>> 2 files changed, 30 insertions(+) >>>> >>>> diff --git a/drivers/base/property.c b/drivers/base/property.c >>>> index 28125a5..8e4398f 100644 >>>> --- a/drivers/base/property.c >>>> +++ b/drivers/base/property.c >>>> @@ -868,6 +868,35 @@ int device_add_properties(struct device *dev, struct property_entry *properties) >>>> EXPORT_SYMBOL_GPL(device_add_properties); >>>> >>>> /** >>>> + * fwnode_get_next_parent - Iterate to the node's parent >>>> + * @fwnode: Firmware whose parent is retrieved >>>> + * >>>> + * This is like fwnode_get_parent() except that it drops the refcount >>>> + * on the passed node, making it suitable for iterating through a >>>> + * node's parents. >>>> + * >>>> + * Returns a node pointer with refcount incremented, use >>>> + * fwnode_handle_node() on it when done. >>>> + */ >>>> +struct fwnode_handle *fwnode_get_next_parent(struct fwnode_handle *fwnode) >>>> +{ >>>> + struct fwnode_handle *parent = NULL; >>>> + >>>> + if (is_of_node(fwnode)) { >>>> + struct device_node *node; >>>> + >>>> + node = of_get_next_parent(to_of_node(fwnode)); >>>> + if (node) >>>> + parent = &node->fwnode; >>>> + } else if (is_acpi_node(fwnode)) { >>>> + parent = acpi_node_get_parent(fwnode); >>>> + } >>>> + >>>> + return parent; >>>> +} >>>> +EXPORT_SYMBOL_GPL(fwnode_get_next_parent); >>> >>> I think I agree with Rob's prior comments about making an ops struct for DT >>> vs ACPI. Out of the 16 patches, 2/16, 3/16, 5/16 (multiple times), and this >>> patch all end up using the same construct. Maybe it needs to be a separate >>> refactoring effort, but if it's happening this often just in this patch set, >>> it seems like it's getting time to clean things up. >> >> As long as there are two cases only (ACPI vs DT), an ops struct wouldn't >> really make things simpler and it would make the code more difficult to >> follow. >> >> But we do have a third case (static or built-in properties) and it doesn't >> seem to be covered at all. > > That said the ops struct could be introduced on top of this series just fine. > It even might be cleaner to do it this way, so I'm not asking for a redesign > here. Right. I don't think it necessarily needs to be covered in this series, but it sure does feel like the time has come. The number of "if (dt) then x else if (acpi) then y;" really stood out for me in this series. > I'd like the built-in properties to be covered too, however. Yeah, good point. I was focused on ACPI and DT, and forgot about those. Not sure if I have time, but I can start looking into this. -- ciao, al ----------------------------------- Al Stone Software Engineer Red Hat, Inc. ahs3-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org ----------------------------------- -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html