Hi Sakari,
On Friday 07 Apr 2017 13:36:34 Sakari Ailus wrote:
> On Fri, Apr 07, 2017 at 12:44:27PM +0300, Laurent Pinchart wrote:
> > On Thursday 06 Apr 2017 16:12:04 Sakari Ailus wrote:
> > > The fwnode_handle is a more generic way than OF device_node to describe
> > > firmware nodes. Instead of the OF API, use more generic fwnode API to
> > > obtain the same information.
> >
> > I would mention that this is a copy of v4l2-of.c with the OF API replaced
> > with the fwnode API.
>
> I'll add that to the description.
>
> > > As the V4L2 fwnode support will be required by a small minority of e.g.
> > > ACPI based systems (the same might actually go for OF), make this a
> > > module instead of embedding it in the videodev module.
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > ---
> > >
> > > drivers/media/v4l2-core/Kconfig | 3 +
> > > drivers/media/v4l2-core/Makefile | 1 +
> > > drivers/media/v4l2-core/v4l2-fwnode.c | 353 +++++++++++++++++++++++++++
> > > include/media/v4l2-fwnode.h | 104 ++++++++++
> > > 4 files changed, 461 insertions(+)
> > > create mode 100644 drivers/media/v4l2-core/v4l2-fwnode.c
> > > create mode 100644 include/media/v4l2-fwnode.h
[snip]
> > > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c
> > > b/drivers/media/v4l2-core/v4l2-fwnode.c new file mode 100644
> > > index 0000000..4f69b11
> > > --- /dev/null
> > > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > > @@ -0,0 +1,353 @@
> > > +/*
> > > + * V4L2 fwnode binding parsing library
> > > + *
> > > + * Copyright (c) 2016 Intel Corporation.
> > > + * Author: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > + *
> > > + * Copyright (C) 2012 - 2013 Samsung Electronics Co., Ltd.
> > > + * Author: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
> > > + *
> > > + * Copyright (C) 2012 Renesas Electronics Corp.
> > > + * Author: Guennadi Liakhovetski <g.liakhovetski@xxxxxx>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of version 2 of the GNU General Public License as
> > > + * published by the Free Software Foundation.
> > > + */
> > > +#include <linux/acpi.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/property.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/string.h>
> > > +#include <linux/types.h>
> > > +
> > > +#include <media/v4l2-fwnode.h>
> > > +
> > > +static int v4l2_fwnode_endpoint_parse_csi_bus(struct fwnode_handle
> > > *fwn,
> > > + struct v4l2_fwnode_endpoint
> > > *vfwn)
> > > +{
> > > + struct v4l2_fwnode_bus_mipi_csi2 *bus = &vfwn->bus.mipi_csi2;
> > > + bool have_clk_lane = false;
> > > + unsigned int flags = 0, lanes_used = 0;
> > > + unsigned int i;
> > > + u32 v;
> > > + int rval;
> >
> > I would have used "ret" instead of "rval" ;-)
>
> I know. But
>
> 1) there's no established convention in the file and
>
> 2) "rval" has the benefit is easier to look up; one doesn't find a plethora
> of "return something". Therefore it is better than "ret" for the purpose.
The solution to that is
/ret\>
(and, of course, switching to vim :-D)
[snip]
> > > +/*
> > > + * v4l2_fwnode_endpoint_free() - free the V4L2 fwnode acquired by
> > > + * v4l2_fwnode_endpoint_alloc_parse()
> > > + * @fwn - the V4L2 fwnode the resources of which are to be released
> >
> > Mayeb "the V4L2 fwnode whose resources are to be released" ?
> >
> > > + *
> > > + * It is safe to call this function with NULL argument or on an
> >
> > s/on an/on a/
>
> Yes.
>
> > > + * V4L2 fwnode the parsing of which failed.
> >
> > "whose parsing failed" ?
>
> Any particular reason? Do you like "whose"? :-)
"of which" sounds dubious in this context, but please consult a native English
speaker in case of doubt.
[snip]
> > > +/**
> > > + * v4l2_fwnode_endpoint_parse_link() - parse a link between two
> > > endpoints
> > > + * @node: pointer to the fwnode at the local end of the link
> >
> > The parameter is called __fwn. I believe you should rename it to fwn,
> > otherwise the documentation will look weird.
> >
> > As explained before, you should mention that this is an endpoint fwnode
> > handle.
>
> Agreed.
>
> > > + * @link: pointer to the V4L2 fwnode link data structure
> > > + *
> > > + * Fill the link structure with the local and remote nodes and port
> > > numbers.
> > > + * The local_node and remote_node fields are set to point to the local
> > > and
> > > + * remote port's parent nodes respectively (the port parent node being
> > > the
> > > + * parent node of the port node if that node isn't a 'ports' node, or
> > > the
> > > + * grand-parent node of the port node otherwise).
> > > + *
> > > + * A reference is taken to both the local and remote nodes, the caller
> > > + * must use v4l2_fwnode_endpoint_put_link() to drop the references
> > > + * when done with the link.
> >
> > Just curious, is there a reason to wrap earlier than the 80 columns limit
> > ?
> >
> > * A reference is taken to both the local and remote nodes, the caller
> > must use
> > * v4l2_fwnode_endpoint_put_link() to drop the references when done with
> > the
> > * link.
> >
> > would work.
>
> It is like a speed limit, you can perfectly legally drive slower than 80
> characters per line, right? :-)
That reminds me of a particular road trip in California that should probably
not be mentioned here, but whose participants certainly remember :-)
> Albeit the conditions seem pretty good in this file, I can try pushing it
> closer to the limit.
>
> > > + * Return: 0 on success, or -ENOLINK if the remote fwnode can't be
> > > found.
> >
> > Here too you should mention remote endpoint.
>
> Oh yes.
>
> > > + */
--
Regards,
Laurent Pinchart
