[PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1148 bytes --]

From: Sebastian Reichel <sre@kernel.org>

If v4l2_device_register_subdev_nodes() is called multiple times, it is
better to return early than corrupt memory.

Without this, exposure / gain controls do not work in the camera
application on N900.

Signed-off-by: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Signed-off-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/media/v4l2-core/v4l2-device.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index f364cc1..937c6de 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -235,6 +235,9 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
 		if (!(sd->flags & V4L2_SUBDEV_FL_HAS_DEVNODE))
 			continue;
 
+		if (sd->devnode)
+			continue;
+
 		vdev = kzalloc(sizeof(*vdev), GFP_KERNEL);
 		if (!vdev) {
 			err = -ENOMEM;
-- 
2.1.4


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH 2/4] Core changes for CCP2/CSI1 support.

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2365 bytes --]

From: Sakari Ailus <sakari.ailus@iki.fi>

CCP2, or CSI-1, is an older single data lane serial bus. Add core
support neccessary for it.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Signed-off-by: Pavel Machek <pavel@ucw.cz>
---
 include/media/v4l2-mediabus.h |  4 ++++
 include/media/v4l2-of.h       | 17 +++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
index 34cc99e..315c167 100644
--- a/include/media/v4l2-mediabus.h
+++ b/include/media/v4l2-mediabus.h
@@ -69,11 +69,15 @@
  * @V4L2_MBUS_PARALLEL:	parallel interface with hsync and vsync
  * @V4L2_MBUS_BT656:	parallel interface with embedded synchronisation, can
  *			also be used for BT.1120
+ * @V4L2_MBUS_CSI1:	MIPI CSI-1 serial interface
+ * @V4L2_MBUS_CCP2:	CCP2 (Compact Camera Port 2)
  * @V4L2_MBUS_CSI2:	MIPI CSI-2 serial interface
  */
 enum v4l2_mbus_type {
 	V4L2_MBUS_PARALLEL,
 	V4L2_MBUS_BT656,
+	V4L2_MBUS_CSI1,
+	V4L2_MBUS_CCP2,
 	V4L2_MBUS_CSI2,
 };
 
diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
index 4dc34b2..63a52ee 100644
--- a/include/media/v4l2-of.h
+++ b/include/media/v4l2-of.h
@@ -53,6 +53,22 @@ struct v4l2_of_bus_parallel {
 };
 
 /**
+ * struct v4l2_of_bus_csi1 - CSI-1/CCP2 data bus structure
+ * @clock_inv: polarity of clock/strobe signal
+ *	       false - not inverted, true - inverted
+ * @strobe: false - data/clock, true - data/strobe
+ * @data_lane: the number of the data lane
+ * @clock_lane: the number of the clock lane
+ */
+struct v4l2_of_bus_mipi_csi1 {
+	bool clock_inv;
+	bool strobe;
+	bool lane_polarity[2];
+	unsigned char data_lane;
+	unsigned char clock_lane;
+};
+
+/**
  * struct v4l2_of_endpoint - the endpoint data structure
  * @base: struct of_endpoint containing port, id, and local of_node
  * @bus_type: bus type
@@ -66,6 +82,7 @@ struct v4l2_of_endpoint {
 	enum v4l2_mbus_type bus_type;
 	union {
 		struct v4l2_of_bus_parallel parallel;
+		struct v4l2_of_bus_mipi_csi1 mipi_csi1;
 		struct v4l2_of_bus_mipi_csi2 mipi_csi2;
 	} bus;
 	u64 *link_frequencies;
-- 
2.1.4


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH 3/4] smiapp: add CCP2 support

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1929 bytes --]

Add support for CCP2 connected SMIA sensors as found
on the Nokia N900.

Signed-off-by: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Signed-off-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/media/i2c/smiapp/smiapp-core.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index f4e92bd..212293f 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -2807,13 +2807,19 @@ static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev)
 	switch (bus_cfg->bus_type) {
 	case V4L2_MBUS_CSI2:
 		hwcfg->csi_signalling_mode = SMIAPP_CSI_SIGNALLING_MODE_CSI2;
+		hwcfg->lanes = bus_cfg->bus.mipi_csi2.num_data_lanes;
+		break;
+	case V4L2_MBUS_CCP2:
+		hwcfg->csi_signalling_mode = (bus_cfg->bus.mipi_csi1.strobe) ?
+		SMIAPP_CSI_SIGNALLING_MODE_CCP2_DATA_STROBE :
+		SMIAPP_CSI_SIGNALLING_MODE_CCP2_DATA_CLOCK;
+		hwcfg->lanes = 1;
 		break;
-		/* FIXME: add CCP2 support. */
 	default:
+		dev_err(dev, "unknown bus protocol\n");
 		goto out_err;
 	}
 
-	hwcfg->lanes = bus_cfg->bus.mipi_csi2.num_data_lanes;
 	dev_dbg(dev, "lanes %u\n", hwcfg->lanes);
 
 	/* NVM size is not mandatory */
@@ -2827,8 +2833,8 @@ static struct smiapp_hwconfig *smiapp_get_hwconfig(struct device *dev)
 		goto out_err;
 	}
 
-	dev_dbg(dev, "nvm %d, clk %d, csi %d\n", hwcfg->nvm_size,
-		hwcfg->ext_clk, hwcfg->csi_signalling_mode);
+	dev_dbg(dev, "nvm %d, clk %d, mode %d\n",
+		hwcfg->nvm_size, hwcfg->ext_clk, hwcfg->csi_signalling_mode);
 
 	if (!bus_cfg->nr_of_link_frequencies) {
 		dev_warn(dev, "no link frequencies defined\n");
-- 
2.1.4


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH 4/4] v4l: split lane parsing code

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 7360 bytes --]

From: Sakari Ailus <sakari.ailus@iki.fi>

The function to parse CSI2 bus parameters was called
v4l2_of_parse_csi_bus(), rename it as v4l2_of_parse_csi2_bus() in
anticipation of CSI1/CCP2 support.

Obtain data bus type from bus-type property. Only try parsing bus
specific properties in this case.

Separate lane parsing from CSI-2 bus parameter parsing. The CSI-1 will
need these as well, separate them into a different
function. have_clk_lane and num_data_lanes arguments may be NULL; the
CSI-1 bus will have no use for them.

Add support for parsing of CSI-1 and CCP2 bus related properties
documented in video-interfaces.txt.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
Signed-off-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/media/v4l2-core/v4l2-of.c | 138 ++++++++++++++++++++++++++++++--------
 1 file changed, 111 insertions(+), 27 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c
index 4f59f44..85629de 100644
--- a/drivers/media/v4l2-core/v4l2-of.c
+++ b/drivers/media/v4l2-core/v4l2-of.c
@@ -20,64 +20,103 @@
 
 #include <media/v4l2-of.h>
 
-static int v4l2_of_parse_csi_bus(const struct device_node *node,
-				 struct v4l2_of_endpoint *endpoint)
+/* This has to match values in the device tree. */
+
+enum v4l2_of_bus_type {
+	V4L2_OF_BUS_TYPE_GUESS = 0, /* CSI-2 D-PHY, parallel or Bt.656 */
+	V4L2_OF_BUS_TYPE_CSI2_CPHY,
+	V4L2_OF_BUS_TYPE_CSI1,
+	V4L2_OF_BUS_TYPE_CCP2,
+};
+
+static int v4l2_of_parse_lanes(const struct device_node *node,
+			       unsigned char *clock_lane,
+			       bool *have_clk_lane,
+			       unsigned char *data_lanes,
+			       bool *lane_polarities,
+			       unsigned short *__num_data_lanes,
+			       unsigned int max_data_lanes)
 {
-	struct v4l2_of_bus_mipi_csi2 *bus = &endpoint->bus.mipi_csi2;
 	struct property *prop;
-	bool have_clk_lane = false;
-	unsigned int flags = 0, lanes_used = 0;
+	unsigned int lanes_used = 0;
+	short num_data_lanes = 0;
 	u32 v;
 
 	prop = of_find_property(node, "data-lanes", NULL);
 	if (prop) {
 		const __be32 *lane = NULL;
-		unsigned int i;
 
-		for (i = 0; i < ARRAY_SIZE(bus->data_lanes); i++) {
+		for (num_data_lanes = 0; num_data_lanes < max_data_lanes;
+		     num_data_lanes++) {
 			lane = of_prop_next_u32(prop, lane, &v);
 			if (!lane)
 				break;
+			data_lanes[num_data_lanes] = v;
 
 			if (lanes_used & BIT(v))
 				pr_warn("%s: duplicated lane %u in data-lanes\n",
 					node->full_name, v);
 			lanes_used |= BIT(v);
-
-			bus->data_lanes[i] = v;
 		}
-		bus->num_data_lanes = i;
 	}
+	if (__num_data_lanes)
+		*__num_data_lanes = num_data_lanes;
 
 	prop = of_find_property(node, "lane-polarities", NULL);
 	if (prop) {
 		const __be32 *polarity = NULL;
 		unsigned int i;
 
-		for (i = 0; i < ARRAY_SIZE(bus->lane_polarities); i++) {
+		for (i = 0; i < 1 + max_data_lanes; i++) {
 			polarity = of_prop_next_u32(prop, polarity, &v);
 			if (!polarity)
 				break;
-			bus->lane_polarities[i] = v;
+			lane_polarities[i] = v;
 		}
 
-		if (i < 1 + bus->num_data_lanes /* clock + data */) {
+		if (i < 1 + num_data_lanes /* clock + data */) {
 			pr_warn("%s: too few lane-polarities entries (need %u, got %u)\n",
-				node->full_name, 1 + bus->num_data_lanes, i);
+				node->full_name, 1 + num_data_lanes, i);
 			return -EINVAL;
 		}
 	}
 
+	if (have_clk_lane)
+		*have_clk_lane = false;
+
 	if (!of_property_read_u32(node, "clock-lanes", &v)) {
+		*clock_lane = v;
+		if (have_clk_lane)
+			*have_clk_lane = true;
+
 		if (lanes_used & BIT(v))
 			pr_warn("%s: duplicated lane %u in clock-lanes\n",
 				node->full_name, v);
 		lanes_used |= BIT(v);
-
-		bus->clock_lane = v;
-		have_clk_lane = true;
 	}
 
+	return 0;
+}
+
+static int v4l2_of_parse_csi2_bus(const struct device_node *node,
+				 struct v4l2_of_endpoint *endpoint)
+{
+	struct v4l2_of_bus_mipi_csi2 *bus = &endpoint->bus.mipi_csi2;
+	bool have_clk_lane = false;
+	unsigned int flags = 0;
+	int rval;
+	u32 v;
+
+	rval = v4l2_of_parse_lanes(node, &bus->clock_lane, &have_clk_lane,
+				   bus->data_lanes, bus->lane_polarities,
+				   &bus->num_data_lanes,
+				   ARRAY_SIZE(bus->data_lanes));
+	if (rval)
+		return rval;
+
+	BUILD_BUG_ON(1 + ARRAY_SIZE(bus->data_lanes)
+		       != ARRAY_SIZE(bus->lane_polarities));
+
 	if (of_get_property(node, "clock-noncontinuous", &v))
 		flags |= V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK;
 	else if (have_clk_lane || bus->num_data_lanes > 0)
@@ -139,6 +178,35 @@ static void v4l2_of_parse_parallel_bus(const struct device_node *node,
 
 }
 
+void v4l2_of_parse_csi1_bus(const struct device_node *node,
+			    struct v4l2_of_endpoint *endpoint,
+			    enum v4l2_of_bus_type bus_type)
+{
+	struct v4l2_of_bus_mipi_csi1 *bus = &endpoint->bus.mipi_csi1;
+	u32 v;
+
+	v4l2_of_parse_lanes(node, &bus->clock_lane, NULL,
+			    &bus->data_lane, bus->lane_polarity,
+			    NULL, 1);
+
+	if (!of_property_read_u32(node, "clock-inv", &v))
+		bus->clock_inv = v;
+
+	if (!of_property_read_u32(node, "strobe", &v))
+		bus->strobe = v;
+
+	if (!of_property_read_u32(node, "data-lane", &v))
+		bus->data_lane = v;
+
+	if (!of_property_read_u32(node, "clock-lane", &v))
+		bus->clock_lane = v;
+
+	if (bus_type == V4L2_OF_BUS_TYPE_CSI1)
+		endpoint->bus_type = V4L2_MBUS_CSI1;
+	else
+		endpoint->bus_type = V4L2_MBUS_CCP2;
+}
+
 /**
  * v4l2_of_parse_endpoint() - parse all endpoint node properties
  * @node: pointer to endpoint device_node
@@ -162,6 +230,7 @@ static void v4l2_of_parse_parallel_bus(const struct device_node *node,
 int v4l2_of_parse_endpoint(const struct device_node *node,
 			   struct v4l2_of_endpoint *endpoint)
 {
+	u32 bus_type = 0;
 	int rval;
 
 	of_graph_parse_endpoint(node, &endpoint->base);
@@ -169,17 +238,32 @@ int v4l2_of_parse_endpoint(const struct device_node *node,
 	memset(&endpoint->bus_type, 0, sizeof(*endpoint) -
 	       offsetof(typeof(*endpoint), bus_type));
 
-	rval = v4l2_of_parse_csi_bus(node, endpoint);
-	if (rval)
-		return rval;
-	/*
-	 * Parse the parallel video bus properties only if none
-	 * of the MIPI CSI-2 specific properties were found.
-	 */
-	if (endpoint->bus.mipi_csi2.flags == 0)
-		v4l2_of_parse_parallel_bus(node, endpoint);
+	rval = of_property_read_u32(node, "bus-type", &bus_type);
+	if (bus_type == 0) {
+		endpoint->bus_type = 0;
+		rval = v4l2_of_parse_csi2_bus(node, endpoint);
+		if (rval)
+			return rval;
+		/*
+		 * Parse the parallel video bus properties only if none
+		 * of the MIPI CSI-2 specific properties were found.
+		 */
+		if (endpoint->bus.mipi_csi2.flags == 0)
+			v4l2_of_parse_parallel_bus(node, endpoint);
+
+		return 0;
+	}
 
-	return 0;
+	switch (bus_type) {
+	case V4L2_OF_BUS_TYPE_CSI1:
+	case V4L2_OF_BUS_TYPE_CCP2:
+		v4l2_of_parse_csi1_bus(node, endpoint, bus_type);
+		return 0;
+	default:
+		pr_warn("bad bus-type %u, device_node \"%s\"\n",
+			bus_type, node->full_name);
+		return -EINVAL;
+	}
 }
 EXPORT_SYMBOL(v4l2_of_parse_endpoint);
 
-- 
2.1.4


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 765 bytes --]

Hi!

On Tue 2017-02-14 23:38:49, Pavel Machek wrote:
> From: Sebastian Reichel <sre@kernel.org>
> 
> If v4l2_device_register_subdev_nodes() is called multiple times, it is
> better to return early than corrupt memory.
> 
> Without this, exposure / gain controls do not work in the camera
> application on N900.
> 
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> Signed-off-by: Pavel Machek <pavel@ucw.cz>

Can I get some updates/feedback here?

You liked this one and whole series should be ready...

Best regards,
									Pavel



-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Sakari Ailus]

Hi Pavel,

On Mon, Feb 20, 2017 at 11:31:14AM +0100, Pavel Machek wrote:
> Hi!
> 
> On Tue 2017-02-14 23:38:49, Pavel Machek wrote:
> > From: Sebastian Reichel <sre@kernel.org>
> > 
> > If v4l2_device_register_subdev_nodes() is called multiple times, it is
> > better to return early than corrupt memory.
> > 
> > Without this, exposure / gain controls do not work in the camera
> > application on N900.
> > 
> > Signed-off-by: Sebastian Reichel <sre@kernel.org>
> > Signed-off-by: Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> Can I get some updates/feedback here?
> 
> You liked this one and whole series should be ready...

:-)

I was just rebasing the CCP2 support on the fwnode patchset.

I'm just pushing the result here:

<URL:https://git.linuxtv.org/sailus/media_tree.git/log/?h=ccp2>

I've tested ACPI, will test DT soon...

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Sakari Ailus]

On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote:
> I've tested ACPI, will test DT soon...

DT case works, too (Nokia N9).

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 610 bytes --]

On Mon 2017-02-20 15:56:36, Sakari Ailus wrote:
> On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote:
> > I've tested ACPI, will test DT soon...
> 
> DT case works, too (Nokia N9).

Hmm. Good to know. Now to figure out how to get N900 case to work...

AFAICT N9 has CSI2, not CSI1 support, right? Some of the core changes
seem to be in, so I'll need to figure out which, and will still need
omap3isp modifications...

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Sakari Ailus]

On Tue, Feb 21, 2017 at 12:07:21PM +0100, Pavel Machek wrote:
> On Mon 2017-02-20 15:56:36, Sakari Ailus wrote:
> > On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote:
> > > I've tested ACPI, will test DT soon...
> > 
> > DT case works, too (Nokia N9).
> 
> Hmm. Good to know. Now to figure out how to get N900 case to work...
> 
> AFAICT N9 has CSI2, not CSI1 support, right? Some of the core changes
> seem to be in, so I'll need to figure out which, and will still need
> omap3isp modifications...

Indeed, I've only tested for CSI-2 as I have no functional CSI-1 devices.

It's essentially the functionality in the four patches. The data-lane and
clock-name properties have been renamed as data-lanes and clock-lanes (i.e.
plural) to match the property documentation.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1369 bytes --]

Hi!

On Tue 2017-02-21 13:11:04, Sakari Ailus wrote:
> On Tue, Feb 21, 2017 at 12:07:21PM +0100, Pavel Machek wrote:
> > On Mon 2017-02-20 15:56:36, Sakari Ailus wrote:
> > > On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote:
> > > > I've tested ACPI, will test DT soon...
> > > 
> > > DT case works, too (Nokia N9).
> > 
> > Hmm. Good to know. Now to figure out how to get N900 case to work...
> > 
> > AFAICT N9 has CSI2, not CSI1 support, right? Some of the core changes
> > seem to be in, so I'll need to figure out which, and will still need
> > omap3isp modifications...
> 
> Indeed, I've only tested for CSI-2 as I have no functional CSI-1 devices.
> 
> It's essentially the functionality in the four patches. The data-lane and
> clock-name properties have been renamed as data-lanes and clock-lanes (i.e.
> plural) to match the property documentation.

Ok, thanks, I got CSI-1 support to compile.

I'm now fighting with subdevices support. Camera flash and autofocus
coil really should be subdevices of the ISP, right?

Do you have any solution for that? [I need it for my userspace to
work, and porting the old one looks like lot of fun (tm) :-(].

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1331 bytes --]

On Tue 2017-02-21 13:11:04, Sakari Ailus wrote:
> On Tue, Feb 21, 2017 at 12:07:21PM +0100, Pavel Machek wrote:
> > On Mon 2017-02-20 15:56:36, Sakari Ailus wrote:
> > > On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote:
> > > > I've tested ACPI, will test DT soon...
> > > 
> > > DT case works, too (Nokia N9).
> > 
> > Hmm. Good to know. Now to figure out how to get N900 case to work...
> > 
> > AFAICT N9 has CSI2, not CSI1 support, right? Some of the core changes
> > seem to be in, so I'll need to figure out which, and will still need
> > omap3isp modifications...
> 
> Indeed, I've only tested for CSI-2 as I have no functional CSI-1 devices.
> 
> It's essentially the functionality in the four patches. The data-lane and
> clock-name properties have been renamed as data-lanes and clock-lanes (i.e.
> plural) to match the property documentation.

Ok, I got the camera sensor to work. No subdevices support, so I don't
have focus (etc) working, but that's a start. I also had to remove
video-bus-switch support; but I guess it will be easier to use
video-multiplexer patches... 

I'll have patches over weekend.

Best regards,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Sakari Ailus]

On Sat, Feb 25, 2017 at 01:09:18AM +0100, Pavel Machek wrote:
> On Tue 2017-02-21 13:11:04, Sakari Ailus wrote:
> > On Tue, Feb 21, 2017 at 12:07:21PM +0100, Pavel Machek wrote:
> > > On Mon 2017-02-20 15:56:36, Sakari Ailus wrote:
> > > > On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote:
> > > > > I've tested ACPI, will test DT soon...
> > > > 
> > > > DT case works, too (Nokia N9).
> > > 
> > > Hmm. Good to know. Now to figure out how to get N900 case to work...
> > > 
> > > AFAICT N9 has CSI2, not CSI1 support, right? Some of the core changes
> > > seem to be in, so I'll need to figure out which, and will still need
> > > omap3isp modifications...
> > 
> > Indeed, I've only tested for CSI-2 as I have no functional CSI-1 devices.
> > 
> > It's essentially the functionality in the four patches. The data-lane and
> > clock-name properties have been renamed as data-lanes and clock-lanes (i.e.
> > plural) to match the property documentation.
> 
> Ok, I got the camera sensor to work. No subdevices support, so I don't
> have focus (etc) working, but that's a start. I also had to remove
> video-bus-switch support; but I guess it will be easier to use
> video-multiplexer patches... 
> 
> I'll have patches over weekend.

I briefly looked at what's there --- you do miss the video nodes for the
non-sensor sub-devices, and they also don't show up in the media graph,
right?

I guess they don't end up matching in the async list.

I think we need to make the non-sensor sub-device support more generic;
it's not just the OMAP 3 ISP that needs it. I think we need to document
the property for the flash phandle as well; I can write one, or refresh
an existing one that I believe already exists.

How about calling it either simply "flash" or "camera-flash"? Similarly
for lens: "lens" or "camera-lens". I have a vague feeling the "camera-"
prefix is somewhat redundant, so I'd just go for "flash" or "lens".

At the very least the property names must be generic (not hardware
dependent) as this kind of functionality should be present in the
framework rather than in individual drivers. That'll be for later though.

Making the sub-device bus configuration a pointer should be in a separate
patch. It makes sense since the entire configuration is not valid for all
sub-devices attached to the ISP anymore. I think it originally was a
separate patch, but they probably have been merged at some point. I can't
find it right now anyway.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

camera subdevice support was Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2081 bytes --]

Hi!

> > Ok, I got the camera sensor to work. No subdevices support, so I don't
> > have focus (etc) working, but that's a start. I also had to remove
> > video-bus-switch support; but I guess it will be easier to use
> > video-multiplexer patches... 
> > 
> > I'll have patches over weekend.
> 
> I briefly looked at what's there --- you do miss the video nodes for the
> non-sensor sub-devices, and they also don't show up in the media graph,
> right?

Yes.

> I guess they don't end up matching in the async list.

How should they get to the async list?

> I think we need to make the non-sensor sub-device support more generic;
> it's not just the OMAP 3 ISP that needs it. I think we need to document
> the property for the flash phandle as well; I can write one, or refresh
> an existing one that I believe already exists.
> 
> How about calling it either simply "flash" or "camera-flash"? Similarly
> for lens: "lens" or "camera-lens". I have a vague feeling the "camera-"
> prefix is somewhat redundant, so I'd just go for "flash" or "lens".

Actually, I'd go for "flash" and "focus-coil". There may be other
lens properties, such as zoom, mirror movement, lens identification,
...

> At the very least the property names must be generic (not hardware
> dependent) as this kind of functionality should be present in the
> framework rather than in individual drivers. That'll be for later
> though.

Agreed, that would be nice.

> Making the sub-device bus configuration a pointer should be in a separate
> patch. It makes sense since the entire configuration is not valid for all
> sub-devices attached to the ISP anymore. I think it originally was a
> separate patch, but they probably have been merged at some point. I can't
> find it right now anyway.

I believe I can find the patch. But I'm not sure if I can port it to
the fwnode infrastructure anytime soon...

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: camera subdevice support was Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 816 bytes --]

Hi!

> > Making the sub-device bus configuration a pointer should be in a separate
> > patch. It makes sense since the entire configuration is not valid for all
> > sub-devices attached to the ISP anymore. I think it originally was a
> > separate patch, but they probably have been merged at some point. I can't
> > find it right now anyway.
> 
> I believe I can find the patch. But I'm not sure if I can port it to
> the fwnode infrastructure anytime soon...

Here is the (unported) patch.

https://git.kernel.org/cgit/linux/kernel/git/pavel/linux-n900.git/commit/?h=camera-sa-5&id=e705712683b99fec282f87ed3e80bb58c95cf726

Maybe this helps,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: camera subdevice support was Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Sakari Ailus]

Moi! :-)

On Sat, Feb 25, 2017 at 10:53:22PM +0100, Pavel Machek wrote:
> Hi!
> 
> > > Ok, I got the camera sensor to work. No subdevices support, so I don't
> > > have focus (etc) working, but that's a start. I also had to remove
> > > video-bus-switch support; but I guess it will be easier to use
> > > video-multiplexer patches... 
> > > 
> > > I'll have patches over weekend.
> > 
> > I briefly looked at what's there --- you do miss the video nodes for the
> > non-sensor sub-devices, and they also don't show up in the media graph,
> > right?
> 
> Yes.
> 
> > I guess they don't end up matching in the async list.
> 
> How should they get to the async list?

The patch you referred to does that. The problem is, it does make the bus
configuration a pointer as well. There should be two patches. That's not a
lot of work to separate them though. But it should be done.

> 
> > I think we need to make the non-sensor sub-device support more generic;
> > it's not just the OMAP 3 ISP that needs it. I think we need to document
> > the property for the flash phandle as well; I can write one, or refresh
> > an existing one that I believe already exists.
> > 
> > How about calling it either simply "flash" or "camera-flash"? Similarly
> > for lens: "lens" or "camera-lens". I have a vague feeling the "camera-"
> > prefix is somewhat redundant, so I'd just go for "flash" or "lens".
> 
> Actually, I'd go for "flash" and "focus-coil". There may be other
> lens properties, such as zoom, mirror movement, lens identification,
> ...

Good point. Still there may be other ways to move the lens than the voice
coil (which sure is cheap), so how about "flash" and "lens-focus"?

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: camera subdevice support was Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2214 bytes --]

Ahoj! :-)

> > > > Ok, I got the camera sensor to work. No subdevices support, so I don't
> > > > have focus (etc) working, but that's a start. I also had to remove
> > > > video-bus-switch support; but I guess it will be easier to use
> > > > video-multiplexer patches... 
> > > > 
> > > > I'll have patches over weekend.
> > > 
> > > I briefly looked at what's there --- you do miss the video nodes for the
> > > non-sensor sub-devices, and they also don't show up in the media graph,
> > > right?
> > 
> > Yes.
> > 
> > > I guess they don't end up matching in the async list.
> > 
> > How should they get to the async list?
> 
> The patch you referred to does that. The problem is, it does make the bus
> configuration a pointer as well. There should be two patches. That's not a
> lot of work to separate them though. But it should be done.

Well... This is the line I'm fighting with:

+ of_parse_phandle(dev->of_node, "ti,camera-flashes",
+							flash++)

If someone told me its fwnode equivalent, I might be able to get it to
work. Knowing what group_id is and if I could ignore it would help a
bit, too :-).

(Also, I'll be glad to test patches :-))

> > > I think we need to make the non-sensor sub-device support more generic;
> > > it's not just the OMAP 3 ISP that needs it. I think we need to document
> > > the property for the flash phandle as well; I can write one, or refresh
> > > an existing one that I believe already exists.
> > > 
> > > How about calling it either simply "flash" or "camera-flash"? Similarly
> > > for lens: "lens" or "camera-lens". I have a vague feeling the "camera-"
> > > prefix is somewhat redundant, so I'd just go for "flash" or "lens".
> > 
> > Actually, I'd go for "flash" and "focus-coil". There may be other
> > lens properties, such as zoom, mirror movement, lens identification,
> > ...
> 
> Good point. Still there may be other ways to move the lens than the voice
> coil (which sure is cheap), so how about "flash" and "lens-focus"?

Sounds good to me.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: camera subdevice support was Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Sakari Ailus]

Hyvää iltaa!

On Sun, Feb 26, 2017 at 09:38:51AM +0100, Pavel Machek wrote:
> Ahoj! :-)
> 
> > > > > Ok, I got the camera sensor to work. No subdevices support, so I don't
> > > > > have focus (etc) working, but that's a start. I also had to remove
> > > > > video-bus-switch support; but I guess it will be easier to use
> > > > > video-multiplexer patches... 
> > > > > 
> > > > > I'll have patches over weekend.
> > > > 
> > > > I briefly looked at what's there --- you do miss the video nodes for the
> > > > non-sensor sub-devices, and they also don't show up in the media graph,
> > > > right?
> > > 
> > > Yes.
> > > 
> > > > I guess they don't end up matching in the async list.
> > > 
> > > How should they get to the async list?
> > 
> > The patch you referred to does that. The problem is, it does make the bus
> > configuration a pointer as well. There should be two patches. That's not a
> > lot of work to separate them though. But it should be done.
> 
> Well... This is the line I'm fighting with:
> 
> + of_parse_phandle(dev->of_node, "ti,camera-flashes",
> +							flash++)
> 
> If someone told me its fwnode equivalent, I might be able to get it to
> work. Knowing what group_id is and if I could ignore it would help a
> bit, too :-).

Right.

ACPI does not have equivalents for OF phandles. That's the background of the
problem. The port and endpoint references are a bit special: there'a a
device reference and indices of the port and the endpoint nodes.

I think you can just use the OF API for the time being until we define how
to describe flash devices with ACPI. The difference with ACPI will be
visible there almost no matter what do you do there, which is one more
reason to have that functionality in the framework (and not drivers).

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: camera subdevice support was Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 4732 bytes --]

Dobry den! :-)

> > > > Ok, I got the camera sensor to work. No subdevices support, so I don't
> > > > have focus (etc) working, but that's a start. I also had to remove
> > > > video-bus-switch support; but I guess it will be easier to use
> > > > video-multiplexer patches... 
> > > > 
> > > > I'll have patches over weekend.
> > > 
> > > I briefly looked at what's there --- you do miss the video nodes for the
> > > non-sensor sub-devices, and they also don't show up in the media graph,
> > > right?
> > 
> > Yes.
> > 
> > > I guess they don't end up matching in the async list.
> > 
> > How should they get to the async list?
> 
> The patch you referred to does that. The problem is, it does make the bus
> configuration a pointer as well. There should be two patches. That's not a
> lot of work to separate them though. But it should be done.
> 
> > 
> > > I think we need to make the non-sensor sub-device support more generic;
> > > it's not just the OMAP 3 ISP that needs it. I think we need to document
> > > the property for the flash phandle as well; I can write one, or refresh
> > > an existing one that I believe already exists.
> > > 
> > > How about calling it either simply "flash" or "camera-flash"? Similarly
> > > for lens: "lens" or "camera-lens". I have a vague feeling the "camera-"
> > > prefix is somewhat redundant, so I'd just go for "flash" or "lens".
> > 
> > Actually, I'd go for "flash" and "focus-coil". There may be other
> > lens properties, such as zoom, mirror movement, lens identification,
> > ...
> 
> Good point. Still there may be other ways to move the lens than the voice
> coil (which sure is cheap), so how about "flash" and "lens-focus"?

Ok, so something like this? (Yes, needs binding documentation and you
wanted it in the core.. can fix.)

BTW, fwnode_handle_put() seems to be missing in the success path of
isp_fwnodes_parse() -- can you check that?

Best regards,
								Pavel


diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index c80397a..6f6fbed 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2114,7 +2114,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
 			buscfg->bus.ccp2.lanecfg.data[0].pol =
 				vfwn.bus.mipi_csi1.lane_polarity[1];
 
-			dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
+			dev_dbg(dev, "data lane %u polarity %u, pos %u\n", 0,
 				buscfg->bus.ccp2.lanecfg.data[0].pol,
 				buscfg->bus.ccp2.lanecfg.data[0].pos);
 
@@ -2162,10 +2162,64 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
 	return 0;
 }
 
+static int camera_subdev_parse(struct device *dev, struct v4l2_async_notifier *notifier,
+			       const char *key)
+{
+	struct device_node *node;
+	struct isp_async_subdev *isd;
+
+	printk("Looking for %s\n", key);
+	
+	node = of_parse_phandle(dev->of_node, key, 0);
+	if (!node)
+		return 0;
+
+	printk("Having subdevice: %p\n", node);
+		
+	isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
+	if (!isd)
+		return -ENOMEM;
+
+	notifier->subdevs[notifier->num_subdevs] = &isd->asd;
+
+	isd->asd.match.of.node = node;
+	if (!isd->asd.match.of.node) {
+		dev_warn(dev, "bad remote port parent\n");
+		return -EIO;
+	}
+
+	isd->asd.match_type = V4L2_ASYNC_MATCH_OF;
+	notifier->num_subdevs++;
+
+	return 0;
+}
+
+static int camera_subdevs_parse(struct device *dev, struct v4l2_async_notifier *notifier,
+				int max)
+{
+	int res = 0;
+
+	printk("Going through camera-flashes\n");
+	if (notifier->num_subdevs < max) {
+		res = camera_subdev_parse(dev, notifier, "flash");
+		if (res)
+			return res;
+	}
+
+	if (notifier->num_subdevs < max) {
+		res = camera_subdev_parse(dev, notifier, "lens-focus");
+		if (res)
+			return res;
+	}
+	
+	return 0;
+}
+
 static int isp_fwnodes_parse(struct device *dev,
 			     struct v4l2_async_notifier *notifier)
 {
 	struct fwnode_handle *fwn = NULL;
+	int res = 0;
 
 	notifier->subdevs = devm_kcalloc(
 		dev, ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL);
@@ -2199,6 +2253,15 @@ static int isp_fwnodes_parse(struct device *dev,
 		notifier->num_subdevs++;
 	}
 
+	/* FIXME: missing put in the success path? */
+
+	res = camera_subdevs_parse(dev, notifier, ISP_MAX_SUBDEVS);
+	if (res)
+		goto error;
+
+	if (notifier->num_subdevs == ISP_MAX_SUBDEVS) {
+		printk("isp: Maybe too many devices?\n");
+	}
 	return notifier->num_subdevs;
 
 error:


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: camera subdevice support was Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Sakari Ailus]

On Sat, Mar 04, 2017 at 09:55:51AM +0100, Pavel Machek wrote:
> Dobry den! :-)

Huomenta! :-)

> 
> > > > > Ok, I got the camera sensor to work. No subdevices support, so I don't
> > > > > have focus (etc) working, but that's a start. I also had to remove
> > > > > video-bus-switch support; but I guess it will be easier to use
> > > > > video-multiplexer patches... 
> > > > > 
> > > > > I'll have patches over weekend.
> > > > 
> > > > I briefly looked at what's there --- you do miss the video nodes for the
> > > > non-sensor sub-devices, and they also don't show up in the media graph,
> > > > right?
> > > 
> > > Yes.
> > > 
> > > > I guess they don't end up matching in the async list.
> > > 
> > > How should they get to the async list?
> > 
> > The patch you referred to does that. The problem is, it does make the bus
> > configuration a pointer as well. There should be two patches. That's not a
> > lot of work to separate them though. But it should be done.
> > 
> > > 
> > > > I think we need to make the non-sensor sub-device support more generic;
> > > > it's not just the OMAP 3 ISP that needs it. I think we need to document
> > > > the property for the flash phandle as well; I can write one, or refresh
> > > > an existing one that I believe already exists.
> > > > 
> > > > How about calling it either simply "flash" or "camera-flash"? Similarly
> > > > for lens: "lens" or "camera-lens". I have a vague feeling the "camera-"
> > > > prefix is somewhat redundant, so I'd just go for "flash" or "lens".
> > > 
> > > Actually, I'd go for "flash" and "focus-coil". There may be other
> > > lens properties, such as zoom, mirror movement, lens identification,
> > > ...
> > 
> > Good point. Still there may be other ways to move the lens than the voice
> > coil (which sure is cheap), so how about "flash" and "lens-focus"?
> 
> Ok, so something like this? (Yes, needs binding documentation and you
> wanted it in the core.. can fix.)
> 
> BTW, fwnode_handle_put() seems to be missing in the success path of
> isp_fwnodes_parse() -- can you check that?

Where exactly? I noticed that if notifier->num_subdevs hits the limit the
last node isn't put properly. I'll fix that. Is that what you meant?

> 
> Best regards,
> 								Pavel
> 
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index c80397a..6f6fbed 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2114,7 +2114,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
>  			buscfg->bus.ccp2.lanecfg.data[0].pol =
>  				vfwn.bus.mipi_csi1.lane_polarity[1];
>  
> -			dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
> +			dev_dbg(dev, "data lane %u polarity %u, pos %u\n", 0,

Why?

>  				buscfg->bus.ccp2.lanecfg.data[0].pol,
>  				buscfg->bus.ccp2.lanecfg.data[0].pos);
>  
> @@ -2162,10 +2162,64 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
>  	return 0;
>  }
>  
> +static int camera_subdev_parse(struct device *dev, struct v4l2_async_notifier *notifier,
> +			       const char *key)
> +{
> +	struct device_node *node;
> +	struct isp_async_subdev *isd;
> +
> +	printk("Looking for %s\n", key);
> +	
> +	node = of_parse_phandle(dev->of_node, key, 0);

There may be more than one flash associated with a sensor. Speaking of which
--- how is it associated to the sensors?

One way to do this could be to simply move the flash property to the sensor
OF node. We could have it here, too, if the flash was not associated with
any sensor, but I doubt that will ever be needed.

This really calls fork moving this part to the framework away from drivers.

> +	if (!node)
> +		return 0;
> +
> +	printk("Having subdevice: %p\n", node);
> +		
> +	isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
> +	if (!isd)
> +		return -ENOMEM;
> +
> +	notifier->subdevs[notifier->num_subdevs] = &isd->asd;
> +
> +	isd->asd.match.of.node = node;
> +	if (!isd->asd.match.of.node) {

You should check node here first.

> +		dev_warn(dev, "bad remote port parent\n");
> +		return -EIO;
> +	}
> +

And then assign it here.

isd->asd.match.fwnode.fwn = of_fwnode_handle(node);

> +	isd->asd.match_type = V4L2_ASYNC_MATCH_OF;

V4L2_ASYNC_MATCH_FWNODE, please.

> +	notifier->num_subdevs++;
> +
> +	return 0;
> +}
> +
> +static int camera_subdevs_parse(struct device *dev, struct v4l2_async_notifier *notifier,
> +				int max)
> +{
> +	int res = 0;

No need to assign res here.

> +
> +	printk("Going through camera-flashes\n");
> +	if (notifier->num_subdevs < max) {
> +		res = camera_subdev_parse(dev, notifier, "flash");
> +		if (res)
> +			return res;
> +	}
> +
> +	if (notifier->num_subdevs < max) {
> +		res = camera_subdev_parse(dev, notifier, "lens-focus");
> +		if (res)
> +			return res;
> +	}
> +	
> +	return 0;
> +}
> +
>  static int isp_fwnodes_parse(struct device *dev,
>  			     struct v4l2_async_notifier *notifier)
>  {
>  	struct fwnode_handle *fwn = NULL;
> +	int res = 0;
>  
>  	notifier->subdevs = devm_kcalloc(
>  		dev, ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL);
> @@ -2199,6 +2253,15 @@ static int isp_fwnodes_parse(struct device *dev,
>  		notifier->num_subdevs++;
>  	}
>  
> +	/* FIXME: missing put in the success path? */
> +
> +	res = camera_subdevs_parse(dev, notifier, ISP_MAX_SUBDEVS);
> +	if (res)
> +		goto error;
> +
> +	if (notifier->num_subdevs == ISP_MAX_SUBDEVS) {
> +		printk("isp: Maybe too many devices?\n");
> +	}
>  	return notifier->num_subdevs;
>  
>  error:
> 
> 

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: camera subdevice support was Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3953 bytes --]

On Sat 2017-03-04 14:30:11, Sakari Ailus wrote:
> On Sat, Mar 04, 2017 at 09:55:51AM +0100, Pavel Machek wrote:
> > Dobry den! :-)
> 
> Huomenta! :-)

Dobry vecer! :-).

> > > Good point. Still there may be other ways to move the lens than the voice
> > > coil (which sure is cheap), so how about "flash" and "lens-focus"?
> > 
> > Ok, so something like this? (Yes, needs binding documentation and you
> > wanted it in the core.. can fix.)
> > 
> > BTW, fwnode_handle_put() seems to be missing in the success path of
> > isp_fwnodes_parse() -- can you check that?
> 
> Where exactly? I noticed that if notifier->num_subdevs hits the limit the
> last node isn't put properly. I'll fix that. Is that what you meant?

I guess I'm confused. I see no put on the success path. Maybe it is
put somewhere out of the function where I did not look... is the
reference held while the driver is running

> > --- a/drivers/media/platform/omap3isp/isp.c
> > +++ b/drivers/media/platform/omap3isp/isp.c
> > @@ -2114,7 +2114,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
> >  			buscfg->bus.ccp2.lanecfg.data[0].pol =
> >  				vfwn.bus.mipi_csi1.lane_polarity[1];
> >  
> > -			dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
> > +			dev_dbg(dev, "data lane %u polarity %u, pos %u\n", 0,
> 
> Why?

I was printing uninitialized / unused variable, which is a no-no (and
gcc complains). I guess I should do a separate patch.

> >  				buscfg->bus.ccp2.lanecfg.data[0].pol,
> >  				buscfg->bus.ccp2.lanecfg.data[0].pos);
> >  
> > @@ -2162,10 +2162,64 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
> >  	return 0;
> >  }
> >  
> > +static int camera_subdev_parse(struct device *dev, struct v4l2_async_notifier *notifier,
> > +			       const char *key)
> > +{
> > +	struct device_node *node;
> > +	struct isp_async_subdev *isd;
> > +
> > +	printk("Looking for %s\n", key);
> > +	
> > +	node = of_parse_phandle(dev->of_node, key, 0);
> 
> There may be more than one flash associated with a sensor. Speaking of which
> --- how is it associated to the sensors?

Ok, more then one flash I can understand (will fix). 

> One way to do this could be to simply move the flash property to the sensor
> OF node. We could have it here, too, if the flash was not associated with
> any sensor, but I doubt that will ever be needed.
> 
> This really calls fork moving this part to the framework away from
> drivers.

The rest I don't get :-(. The flash is likely connected over i2c, so
it should not become child node of omap3isp.

And yes, I agree we want to move it into the framework. Lets agree on
how it should work and where to put it, I'll debug it here then move it... 

> > +	if (!node)
> > +		return 0;
> > +
> > +	printk("Having subdevice: %p\n", node);
> > +		
> > +	isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
> > +	if (!isd)
> > +		return -ENOMEM;
> > +
> > +	notifier->subdevs[notifier->num_subdevs] = &isd->asd;
> > +
> > +	isd->asd.match.of.node = node;
> > +	if (!isd->asd.match.of.node) {
> 
> You should check node here first.

Umm. I did, above. This can't happen, AFAICT.

> > +		dev_warn(dev, "bad remote port parent\n");
> > +		return -EIO;
> > +	}
> > +
> 
> And then assign it here.
> 
> isd->asd.match.fwnode.fwn = of_fwnode_handle(node);
> 
> > +	isd->asd.match_type = V4L2_ASYNC_MATCH_OF;
> 
> V4L2_ASYNC_MATCH_FWNODE, please.

Ok. Lets see if it still works after the changes :-)... it does, good.

> > +static int camera_subdevs_parse(struct device *dev, struct v4l2_async_notifier *notifier,
> > +				int max)
> > +{
> > +	int res = 0;
> 
> No need to assign res here.

Ok.

Thanks and best regards,
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: camera subdevice support was Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3005 bytes --]

Čágo Belo Šílenci! :-)

> > +static int camera_subdev_parse(struct device *dev, struct v4l2_async_notifier *notifier,
> > +			       const char *key)
> > +{
> > +	struct device_node *node;
> > +	struct isp_async_subdev *isd;
> > +
> > +	printk("Looking for %s\n", key);
> > +	
> > +	node = of_parse_phandle(dev->of_node, key, 0);
> 
> There may be more than one flash associated with a sensor. Speaking of which
> --- how is it associated to the sensors?
> 
> One way to do this could be to simply move the flash property to the sensor
> OF node. We could have it here, too, if the flash was not associated with
> any sensor, but I doubt that will ever be needed.

I don't know what you mean here. Anyway, here's updated version.

Best regards,
								Pavel

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index c80397a..22d0e4a 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2162,10 +2162,57 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
 	return 0;
 }
 
+static int camera_subdev_parse(struct device *dev, struct v4l2_async_notifier *notifier,
+			       const char *key, int max)
+{
+	struct device_node *node;
+	struct isp_async_subdev *isd;
+	int num = 0;
+
+	printk("Looking for %s\n", key);
+
+	while (notifier->num_subdevs < max) {
+		node = of_parse_phandle(dev->of_node, key, num++);
+		if (!node)
+			return 0;
+
+		printk("Having subdevice: %p\n", node);
+		
+		isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
+		if (!isd)
+			return -ENOMEM;
+
+		notifier->subdevs[notifier->num_subdevs] = &isd->asd;
+
+		isd->asd.match.fwnode.fwn = of_fwnode_handle(node);
+		isd->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
+		notifier->num_subdevs++;
+	}
+
+	return 0;
+}
+
+static int camera_subdevs_parse(struct device *dev, struct v4l2_async_notifier *notifier,
+				int max)
+{
+	int res;
+
+	res = camera_subdev_parse(dev, notifier, "flash", max);
+	if (res)
+		return res;
+
+	res = camera_subdev_parse(dev, notifier, "lens-focus", max);
+	if (res)
+		return res;
+	
+	return 0;
+}
+
 static int isp_fwnodes_parse(struct device *dev,
 			     struct v4l2_async_notifier *notifier)
 {
 	struct fwnode_handle *fwn = NULL;
+	int res;
 
 	notifier->subdevs = devm_kcalloc(
 		dev, ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL);
@@ -2199,6 +2246,15 @@ static int isp_fwnodes_parse(struct device *dev,
 		notifier->num_subdevs++;
 	}
 
+	/* FIXME: missing put in the success path? */
+
+	res = camera_subdevs_parse(dev, notifier, ISP_MAX_SUBDEVS);
+	if (res)
+		goto error;
+
+	if (notifier->num_subdevs == ISP_MAX_SUBDEVS) {
+		printk("isp: Maybe too many devices?\n");
+	}
 	return notifier->num_subdevs;
 
 error:

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

subdevice config into pointer (was Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times)

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2775 bytes --]

Hi!

> Making the sub-device bus configuration a pointer should be in a separate
> patch. It makes sense since the entire configuration is not valid for all
> sub-devices attached to the ISP anymore. I think it originally was a
> separate patch, but they probably have been merged at some point. I can't
> find it right now anyway.

Something like this?
									Pavel

commit df9141c66678b549fac9d143bd55ed0b242cf36e
Author: Pavel <pavel@ucw.cz>
Date:   Wed Mar 1 13:27:56 2017 +0100

    Turn bus in struct isp_async_subdev into pointer; some of our subdevs
    (flash, focus) will not need bus configuration.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 8a456d4..36bd359 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2030,12 +2030,18 @@ enum isp_of_phy {
 static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
 			    struct isp_async_subdev *isd)
 {
-	struct isp_bus_cfg *buscfg = &isd->bus;
+	struct isp_bus_cfg *buscfg;
 	struct v4l2_fwnode_endpoint vfwn;
 	unsigned int i;
 	int ret;
 	bool csi1 = false;
 
+	buscfg = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);
+	if (!buscfg)
+		return -ENOMEM;
+
+	isd->bus = buscfg;
+
 	ret = v4l2_fwnode_endpoint_parse(fwn, &vfwn);
 	if (ret)
 		return ret;
@@ -2246,7 +2252,7 @@ static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
 		container_of(asd, struct isp_async_subdev, asd);
 
 	isd->sd = subdev;
-	isd->sd->host_priv = &isd->bus;
+	isd->sd->host_priv = isd->bus;
 
 	return 0;
 }
diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
index 7e6f663..c0b9d1d 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -228,7 +228,7 @@ struct isp_device {
 
 struct isp_async_subdev {
 	struct v4l2_subdev *sd;
-	struct isp_bus_cfg bus;
+	struct isp_bus_cfg *bus;
 	struct v4l2_async_subdev asd;
 };
 
diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
index f20abe8..be23408 100644
--- a/drivers/media/platform/omap3isp/ispcsiphy.c
+++ b/drivers/media/platform/omap3isp/ispcsiphy.c
@@ -202,7 +202,7 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
 		struct isp_async_subdev *isd =
 			container_of(pipe->external->asd,
 				     struct isp_async_subdev, asd);
-		buscfg = &isd->bus;
+		buscfg = isd->bus;
 	}
 
 	if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: subdevice config into pointer (was Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times)

[Sakari Ailus]

Hi Pavel,

On Thu, Mar 02, 2017 at 10:07:27AM +0100, Pavel Machek wrote:
> Hi!
> 
> > Making the sub-device bus configuration a pointer should be in a separate
> > patch. It makes sense since the entire configuration is not valid for all
> > sub-devices attached to the ISP anymore. I think it originally was a
> > separate patch, but they probably have been merged at some point. I can't
> > find it right now anyway.
> 
> Something like this?
> 									Pavel
> 
> commit df9141c66678b549fac9d143bd55ed0b242cf36e
> Author: Pavel <pavel@ucw.cz>
> Date:   Wed Mar 1 13:27:56 2017 +0100
> 
>     Turn bus in struct isp_async_subdev into pointer; some of our subdevs
>     (flash, focus) will not need bus configuration.
> 
> Signed-off-by: Pavel Machek <pavel@ucw.cz>

I applied this to the ccp2 branch with an improved patch description.

> 
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index 8a456d4..36bd359 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2030,12 +2030,18 @@ enum isp_of_phy {
>  static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
>  			    struct isp_async_subdev *isd)
>  {
> -	struct isp_bus_cfg *buscfg = &isd->bus;
> +	struct isp_bus_cfg *buscfg;
>  	struct v4l2_fwnode_endpoint vfwn;
>  	unsigned int i;
>  	int ret;
>  	bool csi1 = false;
>  
> +	buscfg = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);
> +	if (!buscfg)
> +		return -ENOMEM;
> +
> +	isd->bus = buscfg;
> +
>  	ret = v4l2_fwnode_endpoint_parse(fwn, &vfwn);
>  	if (ret)
>  		return ret;
> @@ -2246,7 +2252,7 @@ static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
>  		container_of(asd, struct isp_async_subdev, asd);
>  
>  	isd->sd = subdev;
> -	isd->sd->host_priv = &isd->bus;
> +	isd->sd->host_priv = isd->bus;
>  
>  	return 0;
>  }
> diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
> index 7e6f663..c0b9d1d 100644
> --- a/drivers/media/platform/omap3isp/isp.h
> +++ b/drivers/media/platform/omap3isp/isp.h
> @@ -228,7 +228,7 @@ struct isp_device {
>  
>  struct isp_async_subdev {
>  	struct v4l2_subdev *sd;
> -	struct isp_bus_cfg bus;
> +	struct isp_bus_cfg *bus;
>  	struct v4l2_async_subdev asd;
>  };
>  
> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c b/drivers/media/platform/omap3isp/ispcsiphy.c
> index f20abe8..be23408 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -202,7 +202,7 @@ static int omap3isp_csiphy_config(struct isp_csiphy *phy)
>  		struct isp_async_subdev *isd =
>  			container_of(pipe->external->asd,
>  				     struct isp_async_subdev, asd);
> -		buscfg = &isd->bus;
> +		buscfg = isd->bus;
>  	}
>  
>  	if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
> 
> 

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: subdevice config into pointer (was Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times)

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1147 bytes --]

Hi!

> > > Making the sub-device bus configuration a pointer should be in a separate
> > > patch. It makes sense since the entire configuration is not valid for all
> > > sub-devices attached to the ISP anymore. I think it originally was a
> > > separate patch, but they probably have been merged at some point. I can't
> > > find it right now anyway.
> > 
> > Something like this?
> > 
> > commit df9141c66678b549fac9d143bd55ed0b242cf36e
> > Author: Pavel <pavel@ucw.cz>
> > Date:   Wed Mar 1 13:27:56 2017 +0100
> > 
> >     Turn bus in struct isp_async_subdev into pointer; some of our subdevs
> >     (flash, focus) will not need bus configuration.
> > 
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> I applied this to the ccp2 branch with an improved patch
> description.

Thanks!

[But the important part is to get subdevices to work on ccp2 based
branch, and it still fails to work at all if I attempt to enable
them. I'd like to understand why...]

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: subdevice config into pointer (was Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times)

[Sakari Ailus]

Hi Pavel,

On Thu, Mar 02, 2017 at 03:58:08PM +0100, Pavel Machek wrote:
> Hi!
> 
> > > > Making the sub-device bus configuration a pointer should be in a separate
> > > > patch. It makes sense since the entire configuration is not valid for all
> > > > sub-devices attached to the ISP anymore. I think it originally was a
> > > > separate patch, but they probably have been merged at some point. I can't
> > > > find it right now anyway.
> > > 
> > > Something like this?
> > > 
> > > commit df9141c66678b549fac9d143bd55ed0b242cf36e
> > > Author: Pavel <pavel@ucw.cz>
> > > Date:   Wed Mar 1 13:27:56 2017 +0100
> > > 
> > >     Turn bus in struct isp_async_subdev into pointer; some of our subdevs
> > >     (flash, focus) will not need bus configuration.
> > > 
> > > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > 
> > I applied this to the ccp2 branch with an improved patch
> > description.
> 
> Thanks!
> 
> [But the important part is to get subdevices to work on ccp2 based
> branch, and it still fails to work at all if I attempt to enable
> them. I'd like to understand why...]

Did you add the flash / lens to the async list? The patches currently in the
ccp branch do not include that --- it should be in parsing the flash /
lens-focus properties in omap3isp device's node.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: subdevice config into pointer (was Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times)

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3353 bytes --]

Hi!

> > > > > Making the sub-device bus configuration a pointer should be in a separate
> > > > > patch. It makes sense since the entire configuration is not valid for all
> > > > > sub-devices attached to the ISP anymore. I think it originally was a
> > > > > separate patch, but they probably have been merged at some point. I can't
> > > > > find it right now anyway.
> > > > 
> > > > Something like this?
> > > > 
> > > > commit df9141c66678b549fac9d143bd55ed0b242cf36e
> > > > Author: Pavel <pavel@ucw.cz>
> > > > Date:   Wed Mar 1 13:27:56 2017 +0100
> > > > 
> > > >     Turn bus in struct isp_async_subdev into pointer; some of our subdevs
> > > >     (flash, focus) will not need bus configuration.
> > > > 
> > > > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > > 
> > > I applied this to the ccp2 branch with an improved patch
> > > description.
> > 
> > Thanks!
> > 
> > [But the important part is to get subdevices to work on ccp2 based
> > branch, and it still fails to work at all if I attempt to enable
> > them. I'd like to understand why...]
> 
> Did you add the flash / lens to the async list? The patches currently in the
> ccp branch do not include that --- it should be in parsing the flash /
> lens-focus properties in omap3isp device's node.

I retried, and it fails different way than I assumed. I might be able
to debug this one as sensor (and mplayer) still works.

Best regards,
								Pavel

--

    This is what subdevs support should look like, I guess; but I don't
    know fwnode stuff well enough.
    
diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index c80397a..36bd359 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2166,6 +2166,8 @@ static int isp_fwnodes_parse(struct device *dev,
 			     struct v4l2_async_notifier *notifier)
 {
 	struct fwnode_handle *fwn = NULL;
+	struct device_node *node;
+	int flash = 0;
 
 	notifier->subdevs = devm_kcalloc(
 		dev, ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs), GFP_KERNEL);
@@ -2199,6 +2201,42 @@ static int isp_fwnodes_parse(struct device *dev,
 		notifier->num_subdevs++;
 	}
 
+	printk("Going through camera-flashes\n");
+	while (notifier->num_subdevs < ISP_MAX_SUBDEVS) {
+	       /* FIXME: fwnode_graph_get_remote_endpoint()
+	       (fwn = fwnode_graph_get_next_endpoint(device_fwnode_handle(dev), fwn, ))  */
+		struct isp_async_subdev *isd;
+
+		node = of_parse_phandle(dev->of_node, "ti,camera-flashes", flash++);
+		flash++;
+		if (!node)
+			break;
+
+		printk("Having subdevice: %p\n", node);
+		
+#if 1
+		isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
+		if (!isd)
+			goto error;
+
+		notifier->subdevs[notifier->num_subdevs] = &isd->asd;
+
+
+		isd->asd.match.of.node = node;
+		if (!isd->asd.match.of.node) {
+			dev_warn(dev, "bad remote port parent\n");
+			goto error;
+		}
+
+		isd->asd.match_type = V4L2_ASYNC_MATCH_OF;
+		notifier->num_subdevs++;
+#endif
+	}
+
+
+	if (notifier->num_subdevs == ISP_MAX_SUBDEVS) {
+		printk("isp: Maybe too many devices?\n");
+	}
 	return notifier->num_subdevs;
 
 error:


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: subdevice config into pointer (was Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times)

[Laurent Pinchart]

Hi Sakari,

On Thursday 02 Mar 2017 16:16:17 Sakari Ailus wrote:
> On Thu, Mar 02, 2017 at 10:07:27AM +0100, Pavel Machek wrote:
> > Hi!
> > 
> > > Making the sub-device bus configuration a pointer should be in a
> > > separate patch. It makes sense since the entire configuration is not
> > > valid for all sub-devices attached to the ISP anymore. I think it
> > > originally was a separate patch, but they probably have been merged at
> > > some point. I can'tfind it right now anyway.
> > 
> > Something like this?
> > 
> > 									Pavel
> > 
> > commit df9141c66678b549fac9d143bd55ed0b242cf36e
> > Author: Pavel <pavel@ucw.cz>
> > Date:   Wed Mar 1 13:27:56 2017 +0100
> > 
> >     Turn bus in struct isp_async_subdev into pointer; some of our subdevs
> >     (flash, focus) will not need bus configuration.
> > 
> > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> I applied this to the ccp2 branch with an improved patch description.
> 
> > diff --git a/drivers/media/platform/omap3isp/isp.c
> > b/drivers/media/platform/omap3isp/isp.c index 8a456d4..36bd359 100644
> > --- a/drivers/media/platform/omap3isp/isp.c
> > +++ b/drivers/media/platform/omap3isp/isp.c
> > @@ -2030,12 +2030,18 @@ enum isp_of_phy {
> > 
> >  static int isp_fwnode_parse(struct device *dev, struct fwnode_handle
> >  *fwn,
> >  
> >  			    struct isp_async_subdev *isd)
> >  
> >  {
> > 
> > -	struct isp_bus_cfg *buscfg = &isd->bus;
> > +	struct isp_bus_cfg *buscfg;
> > 
> >  	struct v4l2_fwnode_endpoint vfwn;
> >  	unsigned int i;
> >  	int ret;
> >  	bool csi1 = false;
> > 
> > +	buscfg = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);

Given that you recently get rid of devm_kzalloc() in the driver, let's not 
introduce a new one here.

> > +	if (!buscfg)
> > +		return -ENOMEM;
> > +
> > +	isd->bus = buscfg;
> > +
> >  	ret = v4l2_fwnode_endpoint_parse(fwn, &vfwn);
> >  	if (ret)
> >  	
> >  		return ret;
> > 

[snip]

-- 
Regards,

Laurent Pinchart

Re: subdevice config into pointer (was Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times)

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 768 bytes --]

Hi!

> > >  static int isp_fwnode_parse(struct device *dev, struct fwnode_handle
> > >  *fwn,
> > >  
> > >  			    struct isp_async_subdev *isd)
> > >  
> > >  {
> > > 
> > > -	struct isp_bus_cfg *buscfg = &isd->bus;
> > > +	struct isp_bus_cfg *buscfg;
> > > 
> > >  	struct v4l2_fwnode_endpoint vfwn;
> > >  	unsigned int i;
> > >  	int ret;
> > >  	bool csi1 = false;
> > > 
> > > +	buscfg = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);
> 
> Given that you recently get rid of devm_kzalloc() in the driver, let's not 
> introduce a new one here.

What is wrong with devm_kzalloc()?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: subdevice config into pointer (was Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times)

[Sakari Ailus]

Hi Laurent,

On Thu, Mar 02, 2017 at 08:39:51PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Thursday 02 Mar 2017 16:16:17 Sakari Ailus wrote:
> > On Thu, Mar 02, 2017 at 10:07:27AM +0100, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > Making the sub-device bus configuration a pointer should be in a
> > > > separate patch. It makes sense since the entire configuration is not
> > > > valid for all sub-devices attached to the ISP anymore. I think it
> > > > originally was a separate patch, but they probably have been merged at
> > > > some point. I can'tfind it right now anyway.
> > > 
> > > Something like this?
> > > 
> > > 									Pavel
> > > 
> > > commit df9141c66678b549fac9d143bd55ed0b242cf36e
> > > Author: Pavel <pavel@ucw.cz>
> > > Date:   Wed Mar 1 13:27:56 2017 +0100
> > > 
> > >     Turn bus in struct isp_async_subdev into pointer; some of our subdevs
> > >     (flash, focus) will not need bus configuration.
> > > 
> > > Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > 
> > I applied this to the ccp2 branch with an improved patch description.
> > 
> > > diff --git a/drivers/media/platform/omap3isp/isp.c
> > > b/drivers/media/platform/omap3isp/isp.c index 8a456d4..36bd359 100644
> > > --- a/drivers/media/platform/omap3isp/isp.c
> > > +++ b/drivers/media/platform/omap3isp/isp.c
> > > @@ -2030,12 +2030,18 @@ enum isp_of_phy {
> > > 
> > >  static int isp_fwnode_parse(struct device *dev, struct fwnode_handle
> > >  *fwn,
> > >  
> > >  			    struct isp_async_subdev *isd)
> > >  
> > >  {
> > > 
> > > -	struct isp_bus_cfg *buscfg = &isd->bus;
> > > +	struct isp_bus_cfg *buscfg;
> > > 
> > >  	struct v4l2_fwnode_endpoint vfwn;
> > >  	unsigned int i;
> > >  	int ret;
> > >  	bool csi1 = false;
> > > 
> > > +	buscfg = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);
> 
> Given that you recently get rid of devm_kzalloc() in the driver, let's not 
> introduce a new one here.

That's certainly a valid point.

Still, the entire async sub-devices array is allocated with devm_()
allocation functions still; that part wasn't addressed by the patchset
mostly removing devm_() memory allocation, so this patch does actually not
change how the memory is allocated.

Beyond that, I'm not entirely sure whether this is a problem to begin with:
devm resources are released after remove() callback and access to this data
structure should only happen as a direct result of user IOCTL. IOCTLs may
only be in progress as long as there are open file handles on a device ---
and such file handles must be closed until the remove() callback may finish.
(Referring to the Oslo meeting notes.)

Some of the above must be still verified; either way, but the options are
clear: either devm must be removed here as well (with the rest) or that it's
fine to use it here: from this point of view this patch makes no difference.

> 
> > > +	if (!buscfg)
> > > +		return -ENOMEM;
> > > +
> > > +	isd->bus = buscfg;
> > > +
> > >  	ret = v4l2_fwnode_endpoint_parse(fwn, &vfwn);
> > >  	if (ret)
> > >  	
> > >  		return ret;
> > > 
> 
> [snip]
> 

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 5338 bytes --]

Hi!

> > On Mon 2017-02-20 15:56:36, Sakari Ailus wrote:
> > > On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote:
> > > > I've tested ACPI, will test DT soon...
> > > 
> > > DT case works, too (Nokia N9).
> > 
> > Hmm. Good to know. Now to figure out how to get N900 case to work...
> > 
> > AFAICT N9 has CSI2, not CSI1 support, right? Some of the core changes
> > seem to be in, so I'll need to figure out which, and will still need
> > omap3isp modifications...
> 
> Indeed, I've only tested for CSI-2 as I have no functional CSI-1 devices.
> 
> It's essentially the functionality in the four patches. The data-lane and
> clock-name properties have been renamed as data-lanes and clock-lanes (i.e.
> plural) to match the property documentation.

Yes, it seems to work.

Here's a patch. It has checkpatch issues, I can fix them.  More
support is needed on the ispcsiphy.c side... Could you take (fixed)
version of this to your fwnode branch?

Thanks,
									Pavel




---

omap3isp: add support for CSI1 bus
    
Signed-off-by: Pavel Machek <pavel@ucw.cz>
    
diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 245225a..4b10cfe 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2032,6 +2034,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
 	struct v4l2_fwnode_endpoint vfwn;
 	unsigned int i;
 	int ret;
+	int csi1 = 0;
 
 	ret = v4l2_fwnode_endpoint_parse(fwn, &vfwn);
 	if (ret)
@@ -2059,38 +2062,82 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
 
 	case ISP_OF_PHY_CSIPHY1:
 	case ISP_OF_PHY_CSIPHY2:
-		/* FIXME: always assume CSI-2 for now. */
+		switch (vfwn.bus_type) {
+		case V4L2_MBUS_CSI2:
+			dev_dbg(dev, "csi2 configuration\n");
+			csi1 = 0;
+			break;
+		case V4L2_MBUS_CCP2:
+		case V4L2_MBUS_CSI1:
+			dev_dbg(dev, "csi1 configuration\n");
+			csi1 = 1;
+			break;
+		default:
+			dev_err(dev, "unkonwn bus type\n");
+		}
+
 		switch (vfwn.base.port) {
 		case ISP_OF_PHY_CSIPHY1:
-			buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
+			if (csi1)
+				buscfg->interface = ISP_INTERFACE_CCP2B_PHY1;
+			else
+				buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
 			break;
 		case ISP_OF_PHY_CSIPHY2:
-			buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
+			if (csi1)
+				buscfg->interface = ISP_INTERFACE_CCP2B_PHY2;
+			else
+				buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
 			break;
+		default:
+			dev_err(dev, "bad port\n");
 		}
-		buscfg->bus.csi2.lanecfg.clk.pos = vfwn.bus.mipi_csi2.clock_lane;
-		buscfg->bus.csi2.lanecfg.clk.pol =
-			vfwn.bus.mipi_csi2.lane_polarities[0];
-		dev_dbg(dev, "clock lane polarity %u, pos %u\n",
-			buscfg->bus.csi2.lanecfg.clk.pol,
-			buscfg->bus.csi2.lanecfg.clk.pos);
-
-		for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
-			buscfg->bus.csi2.lanecfg.data[i].pos =
-				vfwn.bus.mipi_csi2.data_lanes[i];
-			buscfg->bus.csi2.lanecfg.data[i].pol =
-				vfwn.bus.mipi_csi2.lane_polarities[i + 1];
+		if (csi1) {
+			buscfg->bus.ccp2.lanecfg.clk.pos = vfwn.bus.mipi_csi1.clock_lane;
+			buscfg->bus.ccp2.lanecfg.clk.pol =
+				vfwn.bus.mipi_csi1.lane_polarity[0];
+			dev_dbg(dev, "clock lane polarity %u, pos %u\n",
+				buscfg->bus.ccp2.lanecfg.clk.pol,
+				buscfg->bus.ccp2.lanecfg.clk.pos);
+
+			buscfg->bus.ccp2.lanecfg.data[0].pos = 1;
+			buscfg->bus.ccp2.lanecfg.data[0].pol = 0;
+
 			dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
-				buscfg->bus.csi2.lanecfg.data[i].pol,
-				buscfg->bus.csi2.lanecfg.data[i].pos);
+				buscfg->bus.ccp2.lanecfg.data[0].pol,
+				buscfg->bus.ccp2.lanecfg.data[0].pos);
+
+			buscfg->bus.ccp2.strobe_clk_pol = vfwn.bus.mipi_csi1.clock_inv;
+			buscfg->bus.ccp2.phy_layer = vfwn.bus.mipi_csi1.strobe;
+			buscfg->bus.ccp2.ccp2_mode = vfwn.bus_type == V4L2_MBUS_CCP2;
+			buscfg->bus.ccp2.vp_clk_pol = 1;
+			
+			buscfg->bus.ccp2.crc = 1;		
+		} else {
+			buscfg->bus.csi2.lanecfg.clk.pos = vfwn.bus.mipi_csi2.clock_lane;
+			buscfg->bus.csi2.lanecfg.clk.pol =
+				vfwn.bus.mipi_csi2.lane_polarities[0];
+			dev_dbg(dev, "clock lane polarity %u, pos %u\n",
+				buscfg->bus.csi2.lanecfg.clk.pol,
+				buscfg->bus.csi2.lanecfg.clk.pos);
+
+			for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
+				buscfg->bus.csi2.lanecfg.data[i].pos =
+					vfwn.bus.mipi_csi2.data_lanes[i];
+				buscfg->bus.csi2.lanecfg.data[i].pol =
+					vfwn.bus.mipi_csi2.lane_polarities[i + 1];
+				dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
+					buscfg->bus.csi2.lanecfg.data[i].pol,
+					buscfg->bus.csi2.lanecfg.data[i].pos);
+			}
+			/*
+			 * FIXME: now we assume the CRC is always there.
+			 * Implement a way to obtain this information from the
+			 * sensor. Frame descriptors, perhaps?
+			 */
+
+			buscfg->bus.csi2.crc = 1;
 		}
-
-		/*
-		 * FIXME: now we assume the CRC is always there.
-		 * Implement a way to obtain this information from the
-		 * sensor. Frame descriptors, perhaps?
-		 */
-		buscfg->bus.csi2.crc = 1;
 		break;
 
 	default:


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 5655 bytes --]

Hi!

> > > On Mon 2017-02-20 15:56:36, Sakari Ailus wrote:
> > > > On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote:
> > > > > I've tested ACPI, will test DT soon...
> > > > 
> > > > DT case works, too (Nokia N9).
> > > 
> > > Hmm. Good to know. Now to figure out how to get N900 case to work...
> > > 
> > > AFAICT N9 has CSI2, not CSI1 support, right? Some of the core changes
> > > seem to be in, so I'll need to figure out which, and will still need
> > > omap3isp modifications...
> > 
> > Indeed, I've only tested for CSI-2 as I have no functional CSI-1 devices.
> > 
> > It's essentially the functionality in the four patches. The data-lane and
> > clock-name properties have been renamed as data-lanes and clock-lanes (i.e.
> > plural) to match the property documentation.
> 
> Yes, it seems to work.
> 
> Here's a patch. It has checkpatch issues, I can fix them.  More
> support is needed on the ispcsiphy.c side... Could you take (fixed)
> version of this to your fwnode branch?

Any feedback would be welcome :-)
									Pavel


> omap3isp: add support for CSI1 bus
>     
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
>     
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index 245225a..4b10cfe 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2032,6 +2034,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
>  	struct v4l2_fwnode_endpoint vfwn;
>  	unsigned int i;
>  	int ret;
> +	int csi1 = 0;
>  
>  	ret = v4l2_fwnode_endpoint_parse(fwn, &vfwn);
>  	if (ret)
> @@ -2059,38 +2062,82 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
>  
>  	case ISP_OF_PHY_CSIPHY1:
>  	case ISP_OF_PHY_CSIPHY2:
> -		/* FIXME: always assume CSI-2 for now. */
> +		switch (vfwn.bus_type) {
> +		case V4L2_MBUS_CSI2:
> +			dev_dbg(dev, "csi2 configuration\n");
> +			csi1 = 0;
> +			break;
> +		case V4L2_MBUS_CCP2:
> +		case V4L2_MBUS_CSI1:
> +			dev_dbg(dev, "csi1 configuration\n");
> +			csi1 = 1;
> +			break;
> +		default:
> +			dev_err(dev, "unkonwn bus type\n");
> +		}
> +
>  		switch (vfwn.base.port) {
>  		case ISP_OF_PHY_CSIPHY1:
> -			buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
> +			if (csi1)
> +				buscfg->interface = ISP_INTERFACE_CCP2B_PHY1;
> +			else
> +				buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
>  			break;
>  		case ISP_OF_PHY_CSIPHY2:
> -			buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
> +			if (csi1)
> +				buscfg->interface = ISP_INTERFACE_CCP2B_PHY2;
> +			else
> +				buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
>  			break;
> +		default:
> +			dev_err(dev, "bad port\n");
>  		}
> -		buscfg->bus.csi2.lanecfg.clk.pos = vfwn.bus.mipi_csi2.clock_lane;
> -		buscfg->bus.csi2.lanecfg.clk.pol =
> -			vfwn.bus.mipi_csi2.lane_polarities[0];
> -		dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> -			buscfg->bus.csi2.lanecfg.clk.pol,
> -			buscfg->bus.csi2.lanecfg.clk.pos);
> -
> -		for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
> -			buscfg->bus.csi2.lanecfg.data[i].pos =
> -				vfwn.bus.mipi_csi2.data_lanes[i];
> -			buscfg->bus.csi2.lanecfg.data[i].pol =
> -				vfwn.bus.mipi_csi2.lane_polarities[i + 1];
> +		if (csi1) {
> +			buscfg->bus.ccp2.lanecfg.clk.pos = vfwn.bus.mipi_csi1.clock_lane;
> +			buscfg->bus.ccp2.lanecfg.clk.pol =
> +				vfwn.bus.mipi_csi1.lane_polarity[0];
> +			dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> +				buscfg->bus.ccp2.lanecfg.clk.pol,
> +				buscfg->bus.ccp2.lanecfg.clk.pos);
> +
> +			buscfg->bus.ccp2.lanecfg.data[0].pos = 1;
> +			buscfg->bus.ccp2.lanecfg.data[0].pol = 0;
> +
>  			dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
> -				buscfg->bus.csi2.lanecfg.data[i].pol,
> -				buscfg->bus.csi2.lanecfg.data[i].pos);
> +				buscfg->bus.ccp2.lanecfg.data[0].pol,
> +				buscfg->bus.ccp2.lanecfg.data[0].pos);
> +
> +			buscfg->bus.ccp2.strobe_clk_pol = vfwn.bus.mipi_csi1.clock_inv;
> +			buscfg->bus.ccp2.phy_layer = vfwn.bus.mipi_csi1.strobe;
> +			buscfg->bus.ccp2.ccp2_mode = vfwn.bus_type == V4L2_MBUS_CCP2;
> +			buscfg->bus.ccp2.vp_clk_pol = 1;
> +			
> +			buscfg->bus.ccp2.crc = 1;		
> +		} else {
> +			buscfg->bus.csi2.lanecfg.clk.pos = vfwn.bus.mipi_csi2.clock_lane;
> +			buscfg->bus.csi2.lanecfg.clk.pol =
> +				vfwn.bus.mipi_csi2.lane_polarities[0];
> +			dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> +				buscfg->bus.csi2.lanecfg.clk.pol,
> +				buscfg->bus.csi2.lanecfg.clk.pos);
> +
> +			for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
> +				buscfg->bus.csi2.lanecfg.data[i].pos =
> +					vfwn.bus.mipi_csi2.data_lanes[i];
> +				buscfg->bus.csi2.lanecfg.data[i].pol =
> +					vfwn.bus.mipi_csi2.lane_polarities[i + 1];
> +				dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
> +					buscfg->bus.csi2.lanecfg.data[i].pol,
> +					buscfg->bus.csi2.lanecfg.data[i].pos);
> +			}
> +			/*
> +			 * FIXME: now we assume the CRC is always there.
> +			 * Implement a way to obtain this information from the
> +			 * sensor. Frame descriptors, perhaps?
> +			 */
> +
> +			buscfg->bus.csi2.crc = 1;
>  		}
> -
> -		/*
> -		 * FIXME: now we assume the CRC is always there.
> -		 * Implement a way to obtain this information from the
> -		 * sensor. Frame descriptors, perhaps?
> -		 */
> -		buscfg->bus.csi2.crc = 1;
>  		break;
>  
>  	default:
> 
> 



-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Sakari Ailus]

Hi Pavel,

Please find my comments below.

On Sat, Feb 25, 2017 at 11:12:55PM +0100, Pavel Machek wrote:
> Hi!
> 
> > > On Mon 2017-02-20 15:56:36, Sakari Ailus wrote:
> > > > On Mon, Feb 20, 2017 at 03:09:13PM +0200, Sakari Ailus wrote:
> > > > > I've tested ACPI, will test DT soon...
> > > > 
> > > > DT case works, too (Nokia N9).
> > > 
> > > Hmm. Good to know. Now to figure out how to get N900 case to work...
> > > 
> > > AFAICT N9 has CSI2, not CSI1 support, right? Some of the core changes
> > > seem to be in, so I'll need to figure out which, and will still need
> > > omap3isp modifications...
> > 
> > Indeed, I've only tested for CSI-2 as I have no functional CSI-1 devices.
> > 
> > It's essentially the functionality in the four patches. The data-lane and
> > clock-name properties have been renamed as data-lanes and clock-lanes (i.e.
> > plural) to match the property documentation.
> 
> Yes, it seems to work.
> 
> Here's a patch. It has checkpatch issues, I can fix them.  More
> support is needed on the ispcsiphy.c side... Could you take (fixed)
> version of this to your fwnode branch?
> 
> Thanks,
> 									Pavel
> 
> 
> 
> 
> ---
> 
> omap3isp: add support for CSI1 bus
>     
> Signed-off-by: Pavel Machek <pavel@ucw.cz>
>     
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index 245225a..4b10cfe 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2032,6 +2034,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
>  	struct v4l2_fwnode_endpoint vfwn;
>  	unsigned int i;
>  	int ret;
> +	int csi1 = 0;
>  
>  	ret = v4l2_fwnode_endpoint_parse(fwn, &vfwn);
>  	if (ret)
> @@ -2059,38 +2062,82 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
>  
>  	case ISP_OF_PHY_CSIPHY1:
>  	case ISP_OF_PHY_CSIPHY2:
> -		/* FIXME: always assume CSI-2 for now. */
> +		switch (vfwn.bus_type) {
> +		case V4L2_MBUS_CSI2:
> +			dev_dbg(dev, "csi2 configuration\n");
> +			csi1 = 0;
> +			break;
> +		case V4L2_MBUS_CCP2:
> +		case V4L2_MBUS_CSI1:
> +			dev_dbg(dev, "csi1 configuration\n");
> +			csi1 = 1;
> +			break;
> +		default:
> +			dev_err(dev, "unkonwn bus type\n");
> +		}
> +
>  		switch (vfwn.base.port) {
>  		case ISP_OF_PHY_CSIPHY1:
> -			buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
> +			if (csi1)

You could compare vfwn.bus_type == V4L2_MBUS_CSI2 for this. But if you
choose the local variable, please make it bool instead.

> +				buscfg->interface = ISP_INTERFACE_CCP2B_PHY1;
> +			else
> +				buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
>  			break;
>  		case ISP_OF_PHY_CSIPHY2:
> -			buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
> +			if (csi1)
> +				buscfg->interface = ISP_INTERFACE_CCP2B_PHY2;
> +			else
> +				buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
>  			break;
> +		default:
> +			dev_err(dev, "bad port\n");
>  		}
> -		buscfg->bus.csi2.lanecfg.clk.pos = vfwn.bus.mipi_csi2.clock_lane;
> -		buscfg->bus.csi2.lanecfg.clk.pol =
> -			vfwn.bus.mipi_csi2.lane_polarities[0];
> -		dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> -			buscfg->bus.csi2.lanecfg.clk.pol,
> -			buscfg->bus.csi2.lanecfg.clk.pos);
> -
> -		for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
> -			buscfg->bus.csi2.lanecfg.data[i].pos =
> -				vfwn.bus.mipi_csi2.data_lanes[i];
> -			buscfg->bus.csi2.lanecfg.data[i].pol =
> -				vfwn.bus.mipi_csi2.lane_polarities[i + 1];
> +		if (csi1) {
> +			buscfg->bus.ccp2.lanecfg.clk.pos = vfwn.bus.mipi_csi1.clock_lane;

Wrap after "="?

> +			buscfg->bus.ccp2.lanecfg.clk.pol =
> +				vfwn.bus.mipi_csi1.lane_polarity[0];
> +			dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> +				buscfg->bus.ccp2.lanecfg.clk.pol,
> +				buscfg->bus.ccp2.lanecfg.clk.pos);
> +
> +			buscfg->bus.ccp2.lanecfg.data[0].pos = 1;

Shouldn't this be vfwn.bus.mipi_csi1.data_lane ?

> +			buscfg->bus.ccp2.lanecfg.data[0].pol = 0;

And this one is vfwn.bus.mipi_csi1.lane_polarity[1] .

> +
>  			dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
> -				buscfg->bus.csi2.lanecfg.data[i].pol,
> -				buscfg->bus.csi2.lanecfg.data[i].pos);
> +				buscfg->bus.ccp2.lanecfg.data[0].pol,
> +				buscfg->bus.ccp2.lanecfg.data[0].pos);
> +
> +			buscfg->bus.ccp2.strobe_clk_pol = vfwn.bus.mipi_csi1.clock_inv;
> +			buscfg->bus.ccp2.phy_layer = vfwn.bus.mipi_csi1.strobe;
> +			buscfg->bus.ccp2.ccp2_mode = vfwn.bus_type == V4L2_MBUS_CCP2;

The lines over 80 characters should be wrapped.

> +			buscfg->bus.ccp2.vp_clk_pol = 1;
> +			
> +			buscfg->bus.ccp2.crc = 1;		
> +		} else {
> +			buscfg->bus.csi2.lanecfg.clk.pos = vfwn.bus.mipi_csi2.clock_lane;
> +			buscfg->bus.csi2.lanecfg.clk.pol =
> +				vfwn.bus.mipi_csi2.lane_polarities[0];
> +			dev_dbg(dev, "clock lane polarity %u, pos %u\n",
> +				buscfg->bus.csi2.lanecfg.clk.pol,
> +				buscfg->bus.csi2.lanecfg.clk.pos);
> +
> +			for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
> +				buscfg->bus.csi2.lanecfg.data[i].pos =
> +					vfwn.bus.mipi_csi2.data_lanes[i];
> +				buscfg->bus.csi2.lanecfg.data[i].pol =
> +					vfwn.bus.mipi_csi2.lane_polarities[i + 1];
> +				dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
> +					buscfg->bus.csi2.lanecfg.data[i].pol,
> +					buscfg->bus.csi2.lanecfg.data[i].pos);
> +			}
> +			/*
> +			 * FIXME: now we assume the CRC is always there.
> +			 * Implement a way to obtain this information from the
> +			 * sensor. Frame descriptors, perhaps?
> +			 */
> +
> +			buscfg->bus.csi2.crc = 1;
>  		}
> -
> -		/*
> -		 * FIXME: now we assume the CRC is always there.
> -		 * Implement a way to obtain this information from the
> -		 * sensor. Frame descriptors, perhaps?
> -		 */
> -		buscfg->bus.csi2.crc = 1;
>  		break;
>  
>  	default:
> 
> 

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 1/4] v4l2: device_register_subdev_nodes: allow calling multiple times

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 858 bytes --]

Hi!

> Please find my comments below.

Thanks for quick review, will fix.

> >  		switch (vfwn.base.port) {
> >  		case ISP_OF_PHY_CSIPHY1:
> > -			buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
> > +			if (csi1)
> 
> You could compare vfwn.bus_type == V4L2_MBUS_CSI2 for this. But if you
> choose the local variable, please make it bool instead.

I prefer variable, will switch to bool.

> > +
> > +			buscfg->bus.ccp2.lanecfg.data[0].pos = 1;
> 
> Shouldn't this be vfwn.bus.mipi_csi1.data_lane ?
> 
> > +			buscfg->bus.ccp2.lanecfg.data[0].pol = 0;
> 
> And this one is vfwn.bus.mipi_csi1.lane_polarity[1] .

Thanks for catching this.

Checkpatch issues will be fixed.

								 Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH] omap3isp: Parse CSI1 configuration from the device tree.

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 5053 bytes --]

Add support for parsing CSI1 configuration.

Signed-off-by: Pavel Machek <pavel@ucw.cz>

---

> Please find my comments below.

Thanks for comments. They are fixed now, plus I fixed the checkpatch
stuff that was possible.

It should be ready to apply to the right branch.

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 245225a..b8eef2f 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2032,6 +2032,7 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
 	struct v4l2_fwnode_endpoint vfwn;
 	unsigned int i;
 	int ret;
+	bool csi1 = false;
 
 	ret = v4l2_fwnode_endpoint_parse(fwn, &vfwn);
 	if (ret)
@@ -2059,38 +2060,88 @@ static int isp_fwnode_parse(struct device *dev, struct fwnode_handle *fwn,
 
 	case ISP_OF_PHY_CSIPHY1:
 	case ISP_OF_PHY_CSIPHY2:
-		/* FIXME: always assume CSI-2 for now. */
+		switch (vfwn.bus_type) {
+		case V4L2_MBUS_CCP2:
+		case V4L2_MBUS_CSI1:
+			dev_dbg(dev, "csi1 configuration\n");
+			csi1 = true;
+			break;
+		case V4L2_MBUS_CSI2:
+			dev_dbg(dev, "csi2 configuration\n");
+			csi1 = false;
+			break;
+		default:
+			dev_err(dev, "unkonwn bus type\n");
+		}
+
 		switch (vfwn.base.port) {
 		case ISP_OF_PHY_CSIPHY1:
-			buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
+			if (csi1)
+				buscfg->interface = ISP_INTERFACE_CCP2B_PHY1;
+			else
+				buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
 			break;
 		case ISP_OF_PHY_CSIPHY2:
-			buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
+			if (csi1)
+				buscfg->interface = ISP_INTERFACE_CCP2B_PHY2;
+			else
+				buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
 			break;
+		default:
+			dev_err(dev, "bad port\n");
 		}
-		buscfg->bus.csi2.lanecfg.clk.pos = vfwn.bus.mipi_csi2.clock_lane;
-		buscfg->bus.csi2.lanecfg.clk.pol =
-			vfwn.bus.mipi_csi2.lane_polarities[0];
-		dev_dbg(dev, "clock lane polarity %u, pos %u\n",
-			buscfg->bus.csi2.lanecfg.clk.pol,
-			buscfg->bus.csi2.lanecfg.clk.pos);
-
-		for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
-			buscfg->bus.csi2.lanecfg.data[i].pos =
-				vfwn.bus.mipi_csi2.data_lanes[i];
-			buscfg->bus.csi2.lanecfg.data[i].pol =
-				vfwn.bus.mipi_csi2.lane_polarities[i + 1];
+		if (csi1) {
+			buscfg->bus.ccp2.lanecfg.clk.pos =
+				vfwn.bus.mipi_csi1.clock_lane;
+			buscfg->bus.ccp2.lanecfg.clk.pol =
+				vfwn.bus.mipi_csi1.lane_polarity[0];
+			dev_dbg(dev, "clock lane polarity %u, pos %u\n",
+				buscfg->bus.ccp2.lanecfg.clk.pol,
+				buscfg->bus.ccp2.lanecfg.clk.pos);
+
+			buscfg->bus.ccp2.lanecfg.data[0].pos =
+				vfwn.bus.mipi_csi1.data_lane;
+			buscfg->bus.ccp2.lanecfg.data[0].pol =
+				vfwn.bus.mipi_csi1.lane_polarity[1];
+
 			dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
-				buscfg->bus.csi2.lanecfg.data[i].pol,
-				buscfg->bus.csi2.lanecfg.data[i].pos);
+				buscfg->bus.ccp2.lanecfg.data[0].pol,
+				buscfg->bus.ccp2.lanecfg.data[0].pos);
+
+			buscfg->bus.ccp2.strobe_clk_pol =
+				vfwn.bus.mipi_csi1.clock_inv;
+			buscfg->bus.ccp2.phy_layer = vfwn.bus.mipi_csi1.strobe;
+			buscfg->bus.ccp2.ccp2_mode =
+				vfwn.bus_type == V4L2_MBUS_CCP2;
+			buscfg->bus.ccp2.vp_clk_pol = 1;
+
+			buscfg->bus.ccp2.crc = 1;
+		} else {
+			buscfg->bus.csi2.lanecfg.clk.pos =
+				vfwn.bus.mipi_csi2.clock_lane;
+			buscfg->bus.csi2.lanecfg.clk.pol =
+				vfwn.bus.mipi_csi2.lane_polarities[0];
+			dev_dbg(dev, "clock lane polarity %u, pos %u\n",
+				buscfg->bus.csi2.lanecfg.clk.pol,
+				buscfg->bus.csi2.lanecfg.clk.pos);
+
+			for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
+				buscfg->bus.csi2.lanecfg.data[i].pos =
+					vfwn.bus.mipi_csi2.data_lanes[i];
+				buscfg->bus.csi2.lanecfg.data[i].pol =
+					vfwn.bus.mipi_csi2.lane_polarities[i + 1];
+				dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
+					buscfg->bus.csi2.lanecfg.data[i].pol,
+					buscfg->bus.csi2.lanecfg.data[i].pos);
+			}
+			/*
+			 * FIXME: now we assume the CRC is always there.
+			 * Implement a way to obtain this information from the
+			 * sensor. Frame descriptors, perhaps?
+			 */
+
+			buscfg->bus.csi2.crc = 1;
 		}
-
-		/*
-		 * FIXME: now we assume the CRC is always there.
-		 * Implement a way to obtain this information from the
-		 * sensor. Frame descriptors, perhaps?
-		 */
-		buscfg->bus.csi2.crc = 1;
 		break;
 
 	default:
diff --git a/drivers/media/platform/omap3isp/omap3isp.h b/drivers/media/platform/omap3isp/omap3isp.h
index 443e8f7..f6d1d0d 100644
--- a/drivers/media/platform/omap3isp/omap3isp.h
+++ b/drivers/media/platform/omap3isp/omap3isp.h
@@ -108,6 +108,7 @@ struct isp_ccp2_cfg {
 	unsigned int ccp2_mode:1;
 	unsigned int phy_layer:1;
 	unsigned int vpclk_div:2;
+	unsigned int vp_clk_pol:1;
 	struct isp_csiphy_lanes_cfg lanecfg;
 };
 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]