From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans Verkuil Subject: Re: [PATCH v10 18/24] v4l: fwnode: Add a helper function to obtain device / interger references Date: Mon, 11 Sep 2017 14:38:23 +0200 Message-ID: <2e2eba02-39bc-11e1-d9f1-b83a6f580667@xs4all.nl> References: <20170911080008.21208-1-sakari.ailus@linux.intel.com> <20170911080008.21208-19-sakari.ailus@linux.intel.com> <11c951eb-0315-0149-829e-ed73d748e783@xs4all.nl> <20170911122820.fkbd2rnaddiestab@valkosipuli.retiisi.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20170911122820.fkbd2rnaddiestab@valkosipuli.retiisi.org.uk> Content-Language: en-US Sender: linux-media-owner@vger.kernel.org To: Sakari Ailus Cc: Sakari Ailus , linux-media@vger.kernel.org, niklas.soderlund@ragnatech.se, robh@kernel.org, laurent.pinchart@ideasonboard.com, linux-acpi@vger.kernel.org, mika.westerberg@intel.com, devicetree@vger.kernel.org, pavel@ucw.cz, sre@kernel.org List-Id: linux-acpi@vger.kernel.org On 09/11/2017 02:28 PM, Sakari Ailus wrote: > Hi Hans, > > Thanks for the review. > > On Mon, Sep 11, 2017 at 11:38:58AM +0200, Hans Verkuil wrote: >> Typo in subject: interger -> integer >> >> On 09/11/2017 10:00 AM, Sakari Ailus wrote: >>> v4l2_fwnode_reference_parse_int_prop() will find an fwnode such that under >>> the device's own fwnode, >> >> Sorry, you lost me here. Which device are we talking about? > > The fwnode related a struct device, in other words what dev_fwnode(dev) > gives you. This is either struct device.fwnode or struct > device.of_node.fwnode, depending on which firmware interface was used to > create the device. > > I'll add a note of this. > >> >>> it will follow child fwnodes with the given >>> property -- value pair and return the resulting fwnode. >> >> property-value pair (easier readable that way). >> >> You only describe v4l2_fwnode_reference_parse_int_prop(), not >> v4l2_fwnode_reference_parse_int_props(). > > Yes, I think I changed the naming but forgot to update the commit. I'll do > that now. > >> >>> >>> Signed-off-by: Sakari Ailus >>> --- >>> drivers/media/v4l2-core/v4l2-fwnode.c | 93 +++++++++++++++++++++++++++++++++++ >>> 1 file changed, 93 insertions(+) >>> >>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c >>> index 4821c4989119..56eee5bbd3b5 100644 >>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c >>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c >>> @@ -496,6 +496,99 @@ static int v4l2_fwnode_reference_parse( >>> return ret; >>> } >>> >>> +static struct fwnode_handle *v4l2_fwnode_reference_get_int_prop( >>> + struct fwnode_handle *fwnode, const char *prop, unsigned int index, >>> + const char **props, unsigned int nprops) >> >> Need comments describing what this does. > > Yes. I'll also rename it (get -> read) for consistency with the async > changes. Which async changes? Since the fwnode_handle that's returned is refcounted I wonder if 'get' isn't the right name in this case. > >> >>> +{ >>> + struct fwnode_reference_args fwnode_args; >>> + unsigned int *args = fwnode_args.args; >>> + struct fwnode_handle *child; >>> + int ret; >>> + >>> + ret = fwnode_property_get_reference_args(fwnode, prop, NULL, nprops, >>> + index, &fwnode_args); >>> + if (ret) >>> + return ERR_PTR(ret == -EINVAL ? -ENOENT : ret); >> >> Why map EINVAL to ENOENT? Needs a comment, either here or in the function description. > > fwnode_property_get_reference_args() returns currently a little bit > different error codes in ACPI / DT. This is worth documenting there and > fixing as well. > >> >>> + >>> + for (fwnode = fwnode_args.fwnode; >>> + nprops; nprops--, fwnode = child, props++, args++) { >> >> I think you cram too much in this for-loop: fwnode, nprops, fwnode, props, args... >> It's hard to parse. > > Hmm. I'm not sure if that really helps; the function is just handling each > entry in the array and related array pointers are changed accordingly. The > fwnode = child assignment is there to move to the child node. I.e. what you > need for handling the loop itself. > > I can change this though if you think it really makes a difference for > better. I think so, yes. I noticed you like complex for-loops :-) > >> >> I would make this a 'while (nprops)' and write out all the other assignments, >> increments and decrements. >> >>> + u32 val; >>> + >>> + fwnode_for_each_child_node(fwnode, child) { >>> + if (fwnode_property_read_u32(child, *props, &val)) >>> + continue; >>> + >>> + if (val == *args) >>> + break; >> >> I'm lost. This really needs comments and perhaps even an DT or ACPI example >> so you can see what exactly it is we're doing here. > > I'll add comments to the code. A good example will be ACPI documentation > for LEDs, see 17th patch in v9. That will go through the linux-pm tree so > it won't be available in the same tree for a while. Ideally an ACPI and an equivalent DT example would be nice to have, but I might be asking too much. I'm not that familiar with ACPI, so for me a DT example is easier. > >> >>> + } >>> + >>> + fwnode_handle_put(fwnode); >>> + >>> + if (!child) { >>> + fwnode = ERR_PTR(-ENOENT); >>> + break; >>> + } >>> + } >>> + >>> + return fwnode; >>> +} >>> + >>> +static int v4l2_fwnode_reference_parse_int_props( >>> + struct device *dev, struct v4l2_async_notifier *notifier, >>> + const char *prop, const char **props, unsigned int nprops) >> >> Needs comments describing what this does. > > Will add. > >> >>> +{ >>> + struct fwnode_handle *fwnode; >>> + unsigned int index = 0; >>> + int ret; >>> + >>> + while (!IS_ERR((fwnode = v4l2_fwnode_reference_get_int_prop( >>> + dev_fwnode(dev), prop, index, props, >>> + nprops)))) { >>> + fwnode_handle_put(fwnode); >>> + index++; >>> + } >>> + >>> + if (PTR_ERR(fwnode) != -ENOENT) >>> + return PTR_ERR(fwnode); >> >> Missing 'if (index == 0)'? > > Yes, will add. > >> >>> + >>> + ret = v4l2_async_notifier_realloc(notifier, >>> + notifier->num_subdevs + index); >>> + if (ret) >>> + return -ENOMEM; >>> + >>> + for (index = 0; !IS_ERR((fwnode = v4l2_fwnode_reference_get_int_prop( >>> + dev_fwnode(dev), prop, index, props, >>> + nprops))); ) { >> >> I'd add 'index++' in this for-loop. It's weird that it is missing. > > Agreed, I'll move it there. > >> >>> + struct v4l2_async_subdev *asd; >>> + >>> + if (WARN_ON(notifier->num_subdevs >= notifier->max_subdevs)) { >>> + ret = -EINVAL; >>> + goto error; >>> + } >>> + >>> + asd = kzalloc(sizeof(struct v4l2_async_subdev), GFP_KERNEL); >>> + if (!asd) { >>> + ret = -ENOMEM; >>> + goto error; >>> + } >>> + >>> + notifier->subdevs[notifier->num_subdevs] = asd; >>> + asd->match.fwnode.fwnode = fwnode; >>> + asd->match_type = V4L2_ASYNC_MATCH_FWNODE; >>> + notifier->num_subdevs++; >>> + >>> + fwnode_handle_put(fwnode); >>> + >>> + index++; >>> + } >>> + >>> + return PTR_ERR(fwnode) == -ENOENT ? 0 : PTR_ERR(fwnode); >>> + >>> +error: >>> + fwnode_handle_put(fwnode); >>> + return ret; >>> +} >>> + >>> MODULE_LICENSE("GPL"); >>> MODULE_AUTHOR("Sakari Ailus "); >>> MODULE_AUTHOR("Sylwester Nawrocki "); >>> > Regards, Hans