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.ai...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
