Re: [PATCH 05/14] media: add a V4L2 OF parser

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sylwester

On Mon, 1 Oct 2012, Sylwester Nawrocki wrote:

> On 09/27/2012 04:07 PM, Guennadi Liakhovetski wrote:
> > Add a V4L2 OF parser, implementing bindings, documented in
> > Documentation/devicetree/bindings/media/v4l2.txt.
> > 
> > Signed-off-by: Guennadi Liakhovetski<g.liakhovetski@xxxxxx>
> > ---
> >   drivers/media/v4l2-core/Makefile  |    3 +
> >   drivers/media/v4l2-core/v4l2-of.c |  190 +++++++++++++++++++++++++++++++++++++
> >   include/media/v4l2-of.h           |   62 ++++++++++++
> >   3 files changed, 255 insertions(+), 0 deletions(-)
> >   create mode 100644 drivers/media/v4l2-core/v4l2-of.c
> >   create mode 100644 include/media/v4l2-of.h
> > 
> > diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
> > index c2d61d4..00f64d6 100644
> > --- a/drivers/media/v4l2-core/Makefile
> > +++ b/drivers/media/v4l2-core/Makefile
> > @@ -9,6 +9,9 @@ videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
> >   ifeq ($(CONFIG_COMPAT),y)
> >     videodev-objs += v4l2-compat-ioctl32.o
> >   endif
> > +ifeq ($(CONFIG_OF),y)
> > +  videodev-objs += v4l2-of.o
> > +endif
> > 
> >   obj-$(CONFIG_VIDEO_DEV) += videodev.o v4l2-int-device.o
> >   obj-$(CONFIG_VIDEO_V4L2) += v4l2-common.o
> > diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c
> > new file mode 100644
> > index 0000000..f45d64b
> > --- /dev/null
> > +++ b/drivers/media/v4l2-core/v4l2-of.c
> > @@ -0,0 +1,190 @@
> > +/*
> > + * V4L2 OF binding parsing library
> > + *
> > + * 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/kernel.h>
> > +#include<linux/module.h>
> > +#include<linux/of.h>
> > +#include<linux/types.h>
> > +
> > +#include<media/v4l2-of.h>
> > +
> > +/*
> > + * All properties are optional. If none are found, we don't set any flags. This
> > + * means, the port has a static configuration and no properties have to be
> > + * specified explicitly.
> > + * If any properties are found, that identify the bus as parallel, and
> > + * slave-mode isn't set, we set V4L2_MBUS_MASTER. Similarly, if we recognise the
> > + * bus as serial CSI-2 and clock-noncontinuous isn't set, we set the
> > + * V4L2_MBUS_CSI2_CONTINUOUS_CLOCK flag.
> > + * The caller should hold a reference to "node."
> > + */
> > +void v4l2_of_parse_link(const struct device_node *node,
> > +			struct v4l2_of_link *link)
> > +{
> > +	const struct device_node *port_node = of_get_parent(node);
> > +	int size;
> > +	unsigned int v;
> > +	u32 data_lanes[ARRAY_SIZE(link->mipi_csi_2.data_lanes)];
> > +	bool data_lanes_present;
> > +
> > +	memset(link, 0, sizeof(*link));
> > +
> > +	link->local_node = node;
> > +
> > +	/* Doesn't matter, whether the below two calls succeed */
> > +	of_property_read_u32(port_node, "reg",&link->port);
> > +	of_property_read_u32(node, "reg",&link->addr);
> > +
> > +	if (!of_property_read_u32(node, "bus-width",&v))
> > +		link->parallel.bus_width = v;
> > +
> > +	if (!of_property_read_u32(node, "data-shift",&v))
> > +		link->parallel.data_shift = v;
> > +
> > +	if (!of_property_read_u32(node, "hsync-active",&v))
> > +		link->mbus_flags |= v ? V4L2_MBUS_HSYNC_ACTIVE_HIGH :
> > +			V4L2_MBUS_HSYNC_ACTIVE_LOW;
> > +
> > +	if (!of_property_read_u32(node, "vsync-active",&v))
> > +		link->mbus_flags |= v ? V4L2_MBUS_VSYNC_ACTIVE_HIGH :
> > +			V4L2_MBUS_VSYNC_ACTIVE_LOW;
> > +
> > +	if (!of_property_read_u32(node, "data-active",&v))
> > +		link->mbus_flags |= v ? V4L2_MBUS_DATA_ACTIVE_HIGH :
> > +			V4L2_MBUS_DATA_ACTIVE_LOW;
> > +
> > +	if (!of_property_read_u32(node, "pclk-sample",&v))
> > +		link->mbus_flags |= v ? V4L2_MBUS_PCLK_SAMPLE_RISING :
> > +			V4L2_MBUS_PCLK_SAMPLE_FALLING;
> > +
> > +	if (!of_property_read_u32(node, "field-even-active",&v))
> > +		link->mbus_flags |= v ? V4L2_MBUS_FIELD_EVEN_HIGH :
> > +			V4L2_MBUS_FIELD_EVEN_LOW;
> > +
> > +	if (of_get_property(node, "slave-mode",&size))
> > +		link->mbus_flags |= V4L2_MBUS_SLAVE;
> > +
> > +	/* If any parallel-bus properties have been found, skip serial ones */
> > +	if (link->parallel.bus_width || link->parallel.data_shift ||
> > +	    link->mbus_flags) {
> > +		/* Default parallel bus-master */
> > +		if (!(link->mbus_flags&  V4L2_MBUS_SLAVE))
> > +			link->mbus_flags |= V4L2_MBUS_MASTER;
> > +		return;
> > +	}
> > +
> > +	if (!of_property_read_u32(node, "clock-lanes",&v))
> > +		link->mipi_csi_2.clock_lane = v;
> > +
> > +	if (!of_property_read_u32_array(node, "data-lanes", data_lanes,
> > +					ARRAY_SIZE(data_lanes))) {
> > +		int i;
> > +		for (i = 0; i<  ARRAY_SIZE(data_lanes); i++)
> > +			link->mipi_csi_2.data_lanes[i] = data_lanes[i];
> 
> It doesn't look like what we aimed for. The data-lanes array is supposed
> to be of variable length, thus I don't think it can be parsed like that. 
> Or am I missing something ? I think we need more something like below 
> (based on of_property_read_u32_array(), not tested):

Ok, you're right, that my version only was suitable for fixed-size arrays, 
which wasn't our goal. But I don't think we should go down to manually 
parsing property data. How about (tested;-))

	data = of_find_property(node, "data-lanes", NULL);
	if (data) {
		int i = 0;
		const __be32 *lane = NULL;
		do {
			lane = of_prop_next_u32(data, lane, &data_lanes[i]);
		} while (lane && i++ < ARRAY_SIZE(data_lanes));
		link->mipi_csi_2.num_data_lanes = i;
		while (i--)
			link->mipi_csi_2.data_lanes[i] = data_lanes[i];
	}

> void v4l2_of_parse_mipi_csi2(const struct device_node *node,
> 				struct mipi_csi2 *mipi_csi2)
> {
> 	struct property *prop = of_find_property(node, "data-lanes", NULL);
> 	u8 *out_data_lanes = mipi_csi_2->data_lanes;
> 	int num_lanes;
> 	const __be32 *val;
> 
> 	if (!prop)
> 		return;
> 
> 	mipi_csi2->num_lanes = 0;
> 
> 	if (WARN (!prop->value, "Empty data-lanes property\n"))
> 		return;
> 
> 	num_lanes = prop->length / sizeof(u32);
> 	if (WARN_ON(num_lanes > ARRAY_SIZE(mipi_csi_2->data_lanes))
> 		num_lanes = ARRAY_SIZE(mipi_csi_2->data_lanes);
> 
> 	val = prop->value;
> 	while (num_lanes--)
> 		*out_data_lanes++ = be32_to_cpup(val++);
> 
> 	mipi_csi2->num_lanes = num_lanes;
> }
> 
> 	v4l2_of_parse_mipi_csi2(node, &link->mipi_csi2);

[snip]

> > +struct v4l2_of_link {
> > +	unsigned int port;
> > +	unsigned int addr;
> > +	struct list_head head;
> > +	const struct device_node *local_node;
> > +	const __be32 *remote;
> > +	unsigned int mbus_flags;
> > +	union {
> > +		struct {
> > +			unsigned char bus_width;
> > +			unsigned char data_shift;
> > +		} parallel;
> > +		struct {
> > +			unsigned char data_lanes[4];
> 
> Some devices are interested only in absolute number of lanes.
> I can't see how we could specify the number of data lanes here.
> Shouldn't something like 'unsigned char num_data_lanes;' be
> added to this structure ?

Yes, good point. As you see above, I've added it.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux