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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Sakari,

Thank you for the patch.

On Monday 16 March 2015 02:26:09 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.ailus@iki.fi>
> ---
>  drivers/media/platform/omap3isp/isp.c       |  209 ++++++++++++++++++++++--
>  drivers/media/platform/omap3isp/isp.h       |   11 ++
>  drivers/media/platform/omap3isp/ispcsiphy.c |    7 +
>  3 files changed, 215 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c
> b/drivers/media/platform/omap3isp/isp.c index 992e74c..0d6012a 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,14 @@ static int isp_register_entities(struct isp_device
> *isp) if (ret < 0)
>  		goto done;
> 
> +	/*
> +	 * Device Tree --- the external of the sub-devices will be

What do you mean by "the external of the sub-devices" ?

> +	 * registered later. 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 +2025,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 +2243,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 +2255,151 @@ 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, "interface %u\n", vep.base.port);

The message seems a bit terse to me, I would also print the endpoint node name 
to give a bit of context.

	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:

I'd use an if - else here, but that's up to you.

> +			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_polarity[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 =
> +				vep.bus.mipi_csi2.data_lanes[i];
> +			buscfg->bus.csi2.lanecfg.data[i].pol =
> +				vep.bus.mipi_csi2.lane_polarity[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;
> +		break;
> +
> +	default:
> +		dev_warn(dev, "invalid interface %d\n\n", vep.base.port);

Double \n ? I would also print the node name to add a bit of context.

> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int isp_of_parse_nodes(struct device *dev,
> +			      struct v4l2_async_notifier *notifier)
> +{
> +	struct device_node *node = NULL;

No need to initialize node to NULL.

> +	struct v4l2_async_subdev **asds;
> +
> +	asds = notifier->subdevs =

Could you use a single assignment per line, please ?

> +		devm_kcalloc(dev, ISP_MAX_SUBDEVS, sizeof(*notifier->subdevs),
> +			     GFP_KERNEL);
> +	if (!asds)
> +		return -ENOMEM;
> +
> +	while ((node = of_graph_get_next_endpoint(dev->of_node, node)) &&
> +	       notifier->num_subdevs < ISP_MAX_SUBDEVS) {
> +		struct isp_async_subdev *isd;
> +
> +		isd = devm_kzalloc(dev, sizeof(*isd), GFP_KERNEL);
> +		if (!isd)
> +			return -ENOMEM;
> +
> +		*asds = &isd->asd;
> +
> +		if (isp_of_parse_node(dev, node, isd))
> +			return -EINVAL;
> +
> +		isd->asd.match.of.node = of_graph_get_remote_port_parent(node);

An of_node_put() is needed somewhere in the exit/cleanup paths.

> +
> +		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;
> +}
> +
> +static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
> +				     struct v4l2_subdev *subdev,
> +				     struct v4l2_async_subdev *asd)
> +{
> +	struct isp_device *isp = container_of(async, struct isp_device,
> +					      notifier);
> +	struct isp_async_subdev *isd =
> +		container_of(asd, struct isp_async_subdev, asd);
> +	int rval;

The coding style in the omap3isp driver mostly uses ret, sorry :-)

> +
> +	rval = isp_link_entity(isp, &subdev->entity, isd->bus.interface);
> +	if (rval < 0)
> +		return rval;
> +
> +	isd->sd = subdev;
> +	isd->sd->host_priv = &isd->bus;
> +
> +	return rval;
> +}
> +
> +static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
> +{
> +	struct isp_device *isp = container_of(async, struct isp_device,
> +					      notifier);
> +
> +	return v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
> +}
> +
>  /*
>   * isp_probe - Probe ISP platform device
>   * @pdev: Pointer to ISP platform device
> @@ -2256,7 +2413,6 @@ static int isp_remove(struct platform_device *pdev)
>   */
>  static int isp_probe(struct platform_device *pdev)
>  {
> -	struct isp_platform_data *pdata = pdev->dev.platform_data;
>  	struct isp_device *isp;
>  	struct resource *mem;
>  	int ret;
> @@ -2268,13 +2424,37 @@ static int isp_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	}
> 
> +	if (IS_ENABLED(CONFIG_OF) && pdev->dev.of_node) {
> +		ret = of_property_read_u32(pdev->dev.of_node, "ti,phy-type",
> +					   &isp->phy_type);
> +		if (ret)
> +			return ret;
> +
> +		isp->syscon = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +							      "syscon");
> +		if (IS_ERR(isp->syscon))
> +			return PTR_ERR(isp->syscon);

isp->syscon_offset isn't set anywhere in the DT case, am I missing something ?

> +
> +		ret = isp_of_parse_nodes(&pdev->dev, &isp->notifier);
> +		if (ret < 0)
> +			return ret;
> +		ret = v4l2_async_notifier_register(&isp->v4l2_dev,
> +						   &isp->notifier);
> +		if (ret)
> +			return ret;
> +	} else {
> +		isp->pdata = pdev->dev.platform_data;
> +		isp->syscon = syscon_regmap_lookup_by_pdevname("syscon.0");
> +		if (IS_ERR(isp->syscon))
> +			return PTR_ERR(isp->syscon);
> +	}
> +
>  	isp->autoidle = autoidle;
> 
>  	mutex_init(&isp->isp_mutex);
>  	spin_lock_init(&isp->stat_lock);
> 
>  	isp->dev = &pdev->dev;
> -	isp->pdata = pdata;
>  	isp->ref_count = 0;
> 
>  	ret = dma_coerce_mask_and_coherent(isp->dev, DMA_BIT_MASK(32));
> @@ -2346,6 +2526,11 @@ static int isp_probe(struct platform_device *pdev)
>  		goto error_isp;
>  	}
> 
> +	if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node) {
> +		isp->syscon_offset = isp_res_maps[m].syscon_offset;
> +		isp->phy_type = isp_res_maps[m].phy_type;
> +	}
> +
>  	for (i = 1; i < OMAP3_ISP_IOMEM_CSI2A_REGS1; i++)
>  		isp->mmio_base[i] =
>  			isp->mmio_base[0] + isp_res_maps[m].offset[i];
> @@ -2358,15 +2543,6 @@ static int isp_probe(struct platform_device *pdev)
>  	isp->mmio_hist_base_phys =
>  		mem->start + isp_res_maps[m].offset[OMAP3_ISP_IOMEM_HIST];
> 
> -	isp->syscon = syscon_regmap_lookup_by_pdevname("syscon.0");
> -	if (IS_ERR(isp->syscon)) {
> -		ret = PTR_ERR(isp->syscon);
> -		goto error_isp;
> -	}
> -
> -	isp->syscon_offset = isp_res_maps[m].syscon_offset;
> -	isp->phy_type = isp_res_maps[m].phy_type;
> -
>  	/* IOMMU */
>  	ret = isp_attach_iommu(isp);
>  	if (ret < 0) {
> @@ -2394,6 +2570,9 @@ static int isp_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto error_iommu;
> 
> +	isp->notifier.bound = isp_subdev_notifier_bound;
> +	isp->notifier.complete = isp_subdev_notifier_complete;
> +
>  	ret = isp_register_entities(isp);
>  	if (ret < 0)
>  		goto error_modules;
> @@ -2429,6 +2608,11 @@ static struct platform_device_id omap3isp_id_table[]
> = { };
>  MODULE_DEVICE_TABLE(platform, omap3isp_id_table);
> 
> +static const struct of_device_id omap3isp_of_table[] = {
> +	{ .compatible = "ti,omap3-isp" },
> +	{ },
> +};
> +
>  static struct platform_driver omap3isp_driver = {
>  	.probe = isp_probe,
>  	.remove = isp_remove,
> @@ -2436,6 +2620,7 @@ static struct platform_driver omap3isp_driver = {
>  	.driver = {
>  		.name = "omap3isp",
>  		.pm	= &omap3isp_pm_ops,
> +		.of_match_table = omap3isp_of_table,
>  	},
>  };
> 
> diff --git a/drivers/media/platform/omap3isp/isp.h
> b/drivers/media/platform/omap3isp/isp.h index dcb7d20..431224e 100644
> --- a/drivers/media/platform/omap3isp/isp.h
> +++ b/drivers/media/platform/omap3isp/isp.h
> @@ -18,6 +18,7 @@
>  #define OMAP3_ISP_CORE_H
> 
>  #include <media/omap3isp.h>
> +#include <media/v4l2-async.h>
>  #include <media/v4l2-device.h>
>  #include <linux/clk-provider.h>
>  #include <linux/device.h>
> @@ -178,6 +179,7 @@ struct isp_xclk {
>   */
>  struct isp_device {
>  	struct v4l2_device v4l2_dev;
> +	struct v4l2_async_notifier notifier;
>  	struct media_device media_dev;
>  	struct device *dev;
>  	u32 revision;
> @@ -224,6 +226,15 @@ struct isp_device {
> 
>  	unsigned int sbl_resources;
>  	unsigned int subclk_resources;
> +
> +#define ISP_MAX_SUBDEVS		8
> +	struct v4l2_subdev *subdevs[ISP_MAX_SUBDEVS];
> +};
> +
> +struct isp_async_subdev {
> +	struct v4l2_subdev *sd;
> +	struct isp_bus_cfg bus;
> +	struct v4l2_async_subdev asd;
>  };
> 
>  #define v4l2_dev_to_isp_device(dev) \
> diff --git a/drivers/media/platform/omap3isp/ispcsiphy.c
> b/drivers/media/platform/omap3isp/ispcsiphy.c index d91dde1..495447d 100644
> --- a/drivers/media/platform/omap3isp/ispcsiphy.c
> +++ b/drivers/media/platform/omap3isp/ispcsiphy.c
> @@ -173,6 +173,13 @@ static int omap3isp_csiphy_config(struct isp_csiphy
> *phy) unsigned int i;
>  	u32 reg;
> 
> +	if (!buscfg) {
> +		struct isp_async_subdev *isd =
> +			container_of(pipe->external->asd,
> +				     struct isp_async_subdev, asd);
> +		buscfg = &isd->bus;
> +	}
> +
>  	if (buscfg->interface == ISP_INTERFACE_CCP2B_PHY1
> 
>  	    || buscfg->interface == ISP_INTERFACE_CCP2B_PHY2)
> 
>  		lanes = &buscfg->bus.ccp2.lanecfg;

-- 
Regards,

Laurent Pinchart