Re: [PATCH v1.1 14/15] omap3isp: Add support for the Device Tree

2015-03-25 Thread Sakari Ailus 
Hi Laurent,

Thanks again for the comments!

On Sun, Mar 22, 2015 at 10:26:39PM +0200, Laurent Pinchart wrote:
 Hi Sakari,
 
 Thank you for the patch. This looks good to me, except that there's one last 
 bug I've spotted. Please see below.
 
 On Saturday 21 March 2015 00:05:04 Sakari Ailus wrote:
  Add the ISP device to omap3 DT include file and add support to the driver to
  use it.
  
  Also obtain information on the external entities and the ISP configuration
  related to them through the Device Tree in addition to the platform data.
  
  Signed-off-by: Sakari Ailus sakari.ai...@iki.fi
  ---
  since v1:
  
  - Print endpoint name in debug message when parsing an endpoint.
  
  - Rename lane-polarity property as lane-polarities.
  
  - Print endpoint name with the interface if the interface is invalid.
  
  - Remove assignment to two variables at the same time.
  
  - Fix multiple sub-device support in isp_of_parse_nodes().
  
  - Put of_node properly in isp_of_parse_nodes() (requires Philipp Zabel's
patch of: Decrement refcount of previous endpoint in
of_graph_get_next_endpoint.
  
  - Rename return value variable rval as ret to be consistent with the rest of
  the driver.
  
  - Read the register offset from the syscom property's first argument.
  
   drivers/media/platform/omap3isp/isp.c   |  218 ++--
   drivers/media/platform/omap3isp/isp.h   |   11 ++
   drivers/media/platform/omap3isp/ispcsiphy.c |7 +
   3 files changed, 224 insertions(+), 12 deletions(-)
  
  diff --git a/drivers/media/platform/omap3isp/isp.c
  b/drivers/media/platform/omap3isp/isp.c index 992e74c..92a859e 100644
  --- a/drivers/media/platform/omap3isp/isp.c
  +++ b/drivers/media/platform/omap3isp/isp.c
 
 [snip]
 
  +static int isp_of_parse_nodes(struct device *dev,
  + struct v4l2_async_notifier *notifier)
  +{
  +   struct device_node *node;
  +
  +   notifier-subdevs = devm_kcalloc(
  +   dev, ISP_MAX_SUBDEVS, sizeof(*notifier-subdevs), GFP_KERNEL);
  +   if (!notifier-subdevs)
  +   return -ENOMEM;
  +
  +   while ((node = of_graph_get_next_endpoint(dev-of_node, node)) 
  +  notifier-num_subdevs  ISP_MAX_SUBDEVS) {
 
 If the first condition evaluates to true and the second one to false, the 
 loop 
 will be exited without releasing the reference to the DT node. You could just 
 switch the two conditions to fix this.

Oh, I missed this. I'll resend the set.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1.1 14/15] omap3isp: Add support for the Device Tree

2015-03-23 Thread Laurent Pinchart 
Hi Sakari,

Thank you for the patch. This looks good to me, except that there's one last 
bug I've spotted. Please see below.

On Saturday 21 March 2015 00:05:04 Sakari Ailus wrote:
 Add the ISP device to omap3 DT include file and add support to the driver to
 use it.
 
 Also obtain information on the external entities and the ISP configuration
 related to them through the Device Tree in addition to the platform data.
 
 Signed-off-by: Sakari Ailus sakari.ai...@iki.fi
 ---
 since v1:
 
 - Print endpoint name in debug message when parsing an endpoint.
 
 - Rename lane-polarity property as lane-polarities.
 
 - Print endpoint name with the interface if the interface is invalid.
 
 - Remove assignment to two variables at the same time.
 
 - Fix multiple sub-device support in isp_of_parse_nodes().
 
 - Put of_node properly in isp_of_parse_nodes() (requires Philipp Zabel's
   patch of: Decrement refcount of previous endpoint in
   of_graph_get_next_endpoint.
 
 - Rename return value variable rval as ret to be consistent with the rest of
 the driver.
 
 - Read the register offset from the syscom property's first argument.
 
  drivers/media/platform/omap3isp/isp.c   |  218 ++--
  drivers/media/platform/omap3isp/isp.h   |   11 ++
  drivers/media/platform/omap3isp/ispcsiphy.c |7 +
  3 files changed, 224 insertions(+), 12 deletions(-)
 
 diff --git a/drivers/media/platform/omap3isp/isp.c
 b/drivers/media/platform/omap3isp/isp.c index 992e74c..92a859e 100644
 --- a/drivers/media/platform/omap3isp/isp.c
 +++ b/drivers/media/platform/omap3isp/isp.c

[snip]

 +static int isp_of_parse_nodes(struct device *dev,
 +   struct v4l2_async_notifier *notifier)
 +{
 + struct device_node *node;
 +
 + notifier-subdevs = devm_kcalloc(
 + dev, ISP_MAX_SUBDEVS, sizeof(*notifier-subdevs), GFP_KERNEL);
 + if (!notifier-subdevs)
 + return -ENOMEM;
 +
 + while ((node = of_graph_get_next_endpoint(dev-of_node, node)) 
 +notifier-num_subdevs  ISP_MAX_SUBDEVS) {

If the first condition evaluates to true and the second one to false, the loop 
will be exited without releasing the reference to the DT node. You could just 
switch the two conditions to fix this.

 + struct isp_async_subdev *isd;
 +
 + isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
 + if (!isd) {
 + of_node_put(node);
 + return -ENOMEM;
 + }
 +
 + notifier-subdevs[notifier-num_subdevs] = isd-asd;
 +
 + if (isp_of_parse_node(dev, node, isd)) {
 + of_node_put(node);
 + return -EINVAL;
 + }
 +
 + isd-asd.match.of.node = of_graph_get_remote_port_parent(node);
 + of_node_put(node);
 + if (!isd-asd.match.of.node) {
 + dev_warn(dev, bad remote port parent\n);
 + return -EINVAL;
 + }
 +
 + isd-asd.match_type = V4L2_ASYNC_MATCH_OF;
 + notifier-num_subdevs++;
 + }
 +
 + return notifier-num_subdevs;
 +}

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1.1 14/15] omap3isp: Add support for the Device Tree

2015-03-22 Thread Sakari Ailus 
Hi Laurent,

On Sat, Mar 21, 2015 at 12:05:04AM +0200, Sakari Ailus wrote:
 Add the ISP device to omap3 DT include file and add support to the driver to
 use it.
 
 Also obtain information on the external entities and the ISP configuration
 related to them through the Device Tree in addition to the platform data.
 
 Signed-off-by: Sakari Ailus sakari.ai...@iki.fi

If you're happy with this version, please add the set to your tree. The
patches can be also found in here, on top of your histogram DMA changes:

ssh://linuxtv.org/git/sailus/media_tree.git rm696-054-media

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1.1 14/15] omap3isp: Add support for the Device Tree

2015-03-20 Thread Sakari Ailus 
Add the ISP device to omap3 DT include file and add support to the driver to
use it.

Also obtain information on the external entities and the ISP configuration
related to them through the Device Tree in addition to the platform data.

Signed-off-by: Sakari Ailus sakari.ai...@iki.fi
---
since v1:

- Print endpoint name in debug message when parsing an endpoint.

- Rename lane-polarity property as lane-polarities.

- Print endpoint name with the interface if the interface is invalid.

- Remove assignment to two variables at the same time.

- Fix multiple sub-device support in isp_of_parse_nodes().

- Put of_node properly in isp_of_parse_nodes() (requires Philipp Zabel's
  patch of: Decrement refcount of previous endpoint in
  of_graph_get_next_endpoint.

- Rename return value variable rval as ret to be consistent with the rest of
  the driver.

- Read the register offset from the syscom property's first argument.

 drivers/media/platform/omap3isp/isp.c   |  218 +--
 drivers/media/platform/omap3isp/isp.h   |   11 ++
 drivers/media/platform/omap3isp/ispcsiphy.c |7 +
 3 files changed, 224 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c 
b/drivers/media/platform/omap3isp/isp.c
index 992e74c..92a859e 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -64,6 +64,7 @@
 
 #include media/v4l2-common.h
 #include media/v4l2-device.h
+#include media/v4l2-of.h
 
 #include isp.h
 #include ispreg.h
@@ -1991,6 +1992,13 @@ static int isp_register_entities(struct isp_device *isp)
if (ret  0)
goto done;
 
+   /*
+* Device Tree --- the external sub-devices will be registered
+* later. The same goes for the sub-device node registration.
+*/
+   if (isp-dev-of_node)
+   return 0;
+
/* Register external entities */
for (isp_subdev = pdata ? pdata-subdevs : NULL;
 isp_subdev  isp_subdev-board_info; isp_subdev++) {
@@ -2016,8 +2024,10 @@ static int isp_register_entities(struct isp_device *isp)
ret = v4l2_device_register_subdev_nodes(isp-v4l2_dev);
 
 done:
-   if (ret  0)
+   if (ret  0) {
isp_unregister_entities(isp);
+   v4l2_async_notifier_unregister(isp-notifier);
+   }
 
return ret;
 }
@@ -2232,6 +2242,7 @@ static int isp_remove(struct platform_device *pdev)
 {
struct isp_device *isp = platform_get_drvdata(pdev);
 
+   v4l2_async_notifier_unregister(isp-notifier);
isp_unregister_entities(isp);
isp_cleanup_modules(isp);
isp_xclk_cleanup(isp);
@@ -2243,6 +2254,156 @@ static int isp_remove(struct platform_device *pdev)
return 0;
 }
 
+enum isp_of_phy {
+   ISP_OF_PHY_PARALLEL = 0,
+   ISP_OF_PHY_CSIPHY1,
+   ISP_OF_PHY_CSIPHY2,
+};
+
+static int isp_of_parse_node(struct device *dev, struct device_node *node,
+struct isp_async_subdev *isd)
+{
+   struct isp_bus_cfg *buscfg = isd-bus;
+   struct v4l2_of_endpoint vep;
+   unsigned int i;
+
+   v4l2_of_parse_endpoint(node, vep);
+
+   dev_dbg(dev, parsing endpoint %s, interface %u\n, node-full_name,
+   vep.base.port);
+
+   switch (vep.base.port) {
+   case ISP_OF_PHY_PARALLEL:
+   buscfg-interface = ISP_INTERFACE_PARALLEL;
+   buscfg-bus.parallel.data_lane_shift =
+   vep.bus.parallel.data_shift;
+   buscfg-bus.parallel.clk_pol =
+   !!(vep.bus.parallel.flags
+   V4L2_MBUS_PCLK_SAMPLE_FALLING);
+   buscfg-bus.parallel.hs_pol =
+   !!(vep.bus.parallel.flags  V4L2_MBUS_VSYNC_ACTIVE_LOW);
+   buscfg-bus.parallel.vs_pol =
+   !!(vep.bus.parallel.flags  V4L2_MBUS_HSYNC_ACTIVE_LOW);
+   buscfg-bus.parallel.fld_pol =
+   !!(vep.bus.parallel.flags  V4L2_MBUS_FIELD_EVEN_LOW);
+   buscfg-bus.parallel.data_pol =
+   !!(vep.bus.parallel.flags  V4L2_MBUS_DATA_ACTIVE_LOW);
+   break;
+
+   case ISP_OF_PHY_CSIPHY1:
+   case ISP_OF_PHY_CSIPHY2:
+   /* FIXME: always assume CSI-2 for now. */
+   switch (vep.base.port) {
+   case ISP_OF_PHY_CSIPHY1:
+   buscfg-interface = ISP_INTERFACE_CSI2C_PHY1;
+   break;
+   case ISP_OF_PHY_CSIPHY2:
+   buscfg-interface = ISP_INTERFACE_CSI2A_PHY2;
+   break;
+   }
+   buscfg-bus.csi2.lanecfg.clk.pos = vep.bus.mipi_csi2.clock_lane;
+   buscfg-bus.csi2.lanecfg.clk.pol =
+   vep.bus.mipi_csi2.lane_polarities[0];
+   dev_dbg(dev, clock lane polarity %u, pos %u\n,
+   buscfg-bus.csi2.lanecfg.clk.pol,
+