[{"id":1767279,"web_url":"http://patchwork.ozlabs.org/comment/1767279/","msgid":"<20170912182339.GA27713@ti.com>","list_archive_url":null,"date":"2017-09-12T18:23:39","subject":"Re: [PATCH v3 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver","submitter":{"id":64914,"url":"http://patchwork.ozlabs.org/api/people/64914/","name":"Benoit Parrot","email":"bparrot@ti.com"},"content":"Maxime,\n\nThanks for the patch.\n\nMaxime Ripard <maxime.ripard@free-electrons.com> wrote on Mon [2017-Sep-04 15:03:35 +0200]:\n> The Cadence CSI-2 RX Controller is an hardware block meant to be used as a\n> bridge between a CSI-2 bus and pixel grabbers.\n> \n> It supports operating with internal or external D-PHY, with up to 4 lanes,\n> or without any D-PHY. The current code only supports the former case.\n> \n> It also support dynamic mapping of the CSI-2 virtual channels to the\n> associated pixel grabbers, but that isn't allowed at the moment either.\n> \n> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>\n> ---\n>  drivers/media/platform/Kconfig               |   1 +\n>  drivers/media/platform/Makefile              |   2 +\n>  drivers/media/platform/cadence/Kconfig       |  12 +\n>  drivers/media/platform/cadence/Makefile      |   1 +\n>  drivers/media/platform/cadence/cdns-csi2rx.c | 494 +++++++++++++++++++++++++++\n>  5 files changed, 510 insertions(+)\n>  create mode 100644 drivers/media/platform/cadence/Kconfig\n>  create mode 100644 drivers/media/platform/cadence/Makefile\n>  create mode 100644 drivers/media/platform/cadence/cdns-csi2rx.c\n>\n\n<snip>\n\n> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c\n> new file mode 100644\n> index 000000000000..e662b1890bba\n> --- /dev/null\n> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c\n> @@ -0,0 +1,494 @@\n> +/*\n> + * Driver for Cadence MIPI-CSI2 RX Controller v1.3\n> + *\n> + * Copyright (C) 2017 Cadence Design Systems Inc.\n> + *\n> + * This program is free software; you can redistribute  it and/or modify it\n> + * under  the terms of  the GNU General  Public License as published by the\n> + * Free Software Foundation;  either version 2 of the  License, or (at your\n> + * option) any later version.\n> + */\n> +\n> +#include <linux/atomic.h>\n> +#include <linux/clk.h>\n> +#include <linux/delay.h>\n> +#include <linux/io.h>\n> +#include <linux/module.h>\n> +#include <linux/of.h>\n> +#include <linux/of_graph.h>\n> +#include <linux/phy/phy.h>\n> +#include <linux/platform_device.h>\n> +#include <linux/slab.h>\n> +\n> +#include <media/v4l2-ctrls.h>\n> +#include <media/v4l2-device.h>\n> +#include <media/v4l2-fwnode.h>\n> +#include <media/v4l2-subdev.h>\n> +\n> +#define CSI2RX_DEVICE_CFG_REG\t\t\t0x000\n> +\n> +#define CSI2RX_SOFT_RESET_REG\t\t\t0x004\n> +#define CSI2RX_SOFT_RESET_PROTOCOL\t\t\tBIT(1)\n> +#define CSI2RX_SOFT_RESET_FRONT\t\t\t\tBIT(0)\n> +\n> +#define CSI2RX_STATIC_CFG_REG\t\t\t0x008\n> +#define CSI2RX_STATIC_CFG_DLANE_MAP(llane, plane)\t((plane) << (16 + (llane) * 4))\n> +#define CSI2RX_STATIC_CFG_LANES_MASK\t\t\tGENMASK(11, 8)\n> +\n> +#define CSI2RX_STREAM_BASE(n)\t\t(((n) + 1) * 0x100)\n> +\n> +#define CSI2RX_STREAM_CTRL_REG(n)\t\t(CSI2RX_STREAM_BASE(n) + 0x000)\n> +#define CSI2RX_STREAM_CTRL_START\t\t\tBIT(0)\n> +\n> +#define CSI2RX_STREAM_DATA_CFG_REG(n)\t\t(CSI2RX_STREAM_BASE(n) + 0x008)\n> +#define CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT\t\tBIT(31)\n> +#define CSI2RX_STREAM_DATA_CFG_VC_SELECT(n)\t\tBIT((n) + 16)\n> +\n> +#define CSI2RX_STREAM_CFG_REG(n)\t\t(CSI2RX_STREAM_BASE(n) + 0x00c)\n> +#define CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF\t\t(1 << 8)\n> +\n> +#define CSI2RX_STREAMS_MAX\t4\n> +\n> +enum csi2rx_pads {\n> +\tCSI2RX_PAD_SINK,\n> +\tCSI2RX_PAD_SOURCE_STREAM0,\n> +\tCSI2RX_PAD_SOURCE_STREAM1,\n> +\tCSI2RX_PAD_SOURCE_STREAM2,\n> +\tCSI2RX_PAD_SOURCE_STREAM3,\n> +\tCSI2RX_PAD_MAX,\n> +};\n> +\n> +struct csi2rx_priv {\n> +\tstruct device\t\t\t*dev;\n> +\tatomic_t\t\t\tcount;\n> +\n> +\tvoid __iomem\t\t\t*base;\n> +\tstruct clk\t\t\t*sys_clk;\n> +\tstruct clk\t\t\t*p_clk;\n> +\tstruct clk\t\t\t*pixel_clk[CSI2RX_STREAMS_MAX];\n> +\tstruct phy\t\t\t*dphy;\n> +\n> +\tu8\t\t\t\tlanes[4];\n> +\tu8\t\t\t\tnum_lanes;\n> +\tu8\t\t\t\tmax_lanes;\n> +\tu8\t\t\t\tmax_streams;\n> +\tbool\t\t\t\thas_internal_dphy;\n> +\n> +\tstruct v4l2_subdev\t\tsubdev;\n> +\tstruct media_pad\t\tpads[CSI2RX_PAD_MAX];\n> +\n> +\t/* Remote source */\n> +\tstruct v4l2_async_subdev\tasd;\n> +\tstruct device_node\t\t*source_node;\n> +\tstruct v4l2_subdev\t\t*source_subdev;\n> +\tint\t\t\t\tsource_pad;\n> +};\n> +\n> +static inline\n> +struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev)\n> +{\n> +\treturn container_of(subdev, struct csi2rx_priv, subdev);\n> +}\n> +\n> +static void csi2rx_reset(struct csi2rx_priv *csi2rx)\n> +{\n> +\twritel(CSI2RX_SOFT_RESET_PROTOCOL | CSI2RX_SOFT_RESET_FRONT,\n> +\t       csi2rx->base + CSI2RX_SOFT_RESET_REG);\n> +\n> +\tusleep_range(10, 20);\n> +\n> +\twritel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG);\n> +}\n> +\n> +static int csi2rx_start(struct csi2rx_priv *csi2rx)\n> +{\n> +\tunsigned int i;\n> +\tu32 reg;\n> +\tint ret;\n> +\n> +\t/*\n> +\t * We're not the first users, there's no need to enable the\n> +\t * whole controller.\n> +\t */\n> +\tif (atomic_inc_return(&csi2rx->count) > 1)\n> +\t\treturn 0;\n> +\n> +\tclk_prepare_enable(csi2rx->p_clk);\n> +\n> +\tprintk(\"%s %d\\n\", __func__, __LINE__);\n\nSome left over debug...\n\n> +\n> +\tcsi2rx_reset(csi2rx);\n> +\n> +\treg = csi2rx->num_lanes << 8;\n> +\tfor (i = 0; i < csi2rx->num_lanes; i++)\n> +\t\treg |= CSI2RX_STATIC_CFG_DLANE_MAP(i, csi2rx->lanes[i]);\n> +\n> +\twritel(reg, csi2rx->base + CSI2RX_STATIC_CFG_REG);\n> +\n> +\tret = v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, true);\n> +\tif (ret)\n> +\t\treturn ret;\n> +\n> +\t/*\n> +\t * Create a static mapping between the CSI virtual channels\n> +\t * and the output stream.\n> +\t *\n> +\t * This should be enhanced, but v4l2 lacks the support for\n> +\t * changing that mapping dynamically.\n> +\t *\n> +\t * We also cannot enable and disable independant streams here,\n> +\t * hence the reference counting.\n> +\t */\n> +\tfor (i = 0; i < csi2rx->max_streams; i++) {\n> +\t\tclk_prepare_enable(csi2rx->pixel_clk[i]);\n> +\n> +\t\twritel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,\n> +\t\t       csi2rx->base + CSI2RX_STREAM_CFG_REG(i));\n> +\n> +\t\twritel(CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT |\n> +\t\t       CSI2RX_STREAM_DATA_CFG_VC_SELECT(i),\n> +\t\t       csi2rx->base + CSI2RX_STREAM_DATA_CFG_REG(i));\n> +\n> +\t\twritel(CSI2RX_STREAM_CTRL_START,\n> +\t\t       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));\n> +\t}\n> +\n> +\tclk_prepare_enable(csi2rx->sys_clk);\n> +\n> +\tclk_disable_unprepare(csi2rx->p_clk);\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int csi2rx_stop(struct csi2rx_priv *csi2rx)\n> +{\n> +\tunsigned int i;\n> +\n> +\t/*\n> +\t * Let the last user turn off the lights\n> +\t */\n> +\tif (!atomic_dec_and_test(&csi2rx->count))\n> +\t\treturn 0;\n> +\n> +\tprintk(\"%s %d\\n\", __func__, __LINE__);\n\nSame here... dev_dbg perhaps? \n\n> +\n> +\tclk_prepare_enable(csi2rx->p_clk);\n> +\n> +\tfor (i = 0; i < csi2rx->max_streams; i++) {\n> +\t\twritel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));\n> +\n> +\t\tclk_disable_unprepare(csi2rx->pixel_clk[i]);\n> +\t}\n> +\n> +\tclk_disable_unprepare(csi2rx->p_clk);\n> +\n> +\treturn v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false);\n> +}\n> +\n> +static int csi2rx_s_stream(struct v4l2_subdev *sd, int enable)\n> +{\n> +\tstruct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(sd);\n> +\n> +\tif (enable)\n> +\t\tcsi2rx_start(csi2rx);\n> +\telse\n> +\t\tcsi2rx_stop(csi2rx);\n> +\n> +\treturn 0;\n\nSince we added the error checking in both csi2rx_start/csi2rx_stop\nwe should also propagate the error as well.\nSomething like:\n\n\tif (enable)\n\t\treturn csi2rx_start(csi2rx);\n\n\treturn csi2rx_stop(csi2rx);\n...\n\n> +}\n> +\n> +static const struct v4l2_subdev_video_ops csi2rx_video_ops = {\n> +\t.s_stream\t= csi2rx_s_stream,\n> +};\n> +\n> +static const struct v4l2_subdev_ops csi2rx_subdev_ops = {\n> +\t.video\t\t= &csi2rx_video_ops,\n> +};\n> +\n> +static int csi2rx_async_bound(struct v4l2_async_notifier *notifier,\n> +\t\t\t      struct v4l2_subdev *s_subdev,\n> +\t\t\t      struct v4l2_async_subdev *asd)\n> +{\n> +\tstruct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);\n> +\tstruct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);\n> +\n> +\tcsi2rx->source_pad = media_entity_get_fwnode_pad(&s_subdev->entity,\n> +\t\t\t\t\t\t\t &csi2rx->source_node->fwnode,\n> +\t\t\t\t\t\t\t MEDIA_PAD_FL_SOURCE);\n> +\tif (csi2rx->source_pad < 0) {\n> +\t\tdev_err(csi2rx->dev, \"Couldn't find output pad for subdev %s\\n\",\n> +\t\t\ts_subdev->name);\n> +\t\treturn csi2rx->source_pad;\n> +\t}\n> +\n> +\tcsi2rx->source_subdev = s_subdev;\n> +\n> +\tdev_dbg(csi2rx->dev, \"Bound %s pad: %d\\n\", s_subdev->name,\n> +\t\tcsi2rx->source_pad);\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int csi2rx_async_complete(struct v4l2_async_notifier *notifier)\n> +{\n> +\tstruct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);\n> +\tstruct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);\n> +\n> +\treturn media_create_pad_link(&csi2rx->source_subdev->entity,\n> +\t\t\t\t     csi2rx->source_pad,\n> +\t\t\t\t     &csi2rx->subdev.entity, 0,\n> +\t\t\t\t     MEDIA_LNK_FL_ENABLED |\n> +\t\t\t\t     MEDIA_LNK_FL_IMMUTABLE);\n> +}\n> +\n> +static void csi2rx_async_unbind(struct v4l2_async_notifier *notifier,\n> +\t\t\t\tstruct v4l2_subdev *s_subdev,\n> +\t\t\t\tstruct v4l2_async_subdev *asd)\n> +{\n> +\tstruct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);\n> +\tstruct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);\n> +\n> +\tdev_dbg(csi2rx->dev, \"Unbound %s pad: %d\\n\", s_subdev->name,\n> +\t\tcsi2rx->source_pad);\n> +\n> +\tcsi2rx->source_subdev = NULL;\n> +\tcsi2rx->source_pad = -EINVAL;\n> +}\n> +\n> +static int csi2rx_get_resources(struct csi2rx_priv *csi2rx,\n> +\t\t\t\tstruct platform_device *pdev)\n> +{\n> +\tstruct resource *res;\n> +\tunsigned char i;\n> +\tu32 reg;\n> +\n> +\tres = platform_get_resource(pdev, IORESOURCE_MEM, 0);\n> +\tcsi2rx->base = devm_ioremap_resource(&pdev->dev, res);\n> +\tif (IS_ERR(csi2rx->base))\n> +\t\treturn PTR_ERR(csi2rx->base);\n> +\n> +\tcsi2rx->sys_clk = devm_clk_get(&pdev->dev, \"sys_clk\");\n> +\tif (IS_ERR(csi2rx->sys_clk)) {\n> +\t\tdev_err(&pdev->dev, \"Couldn't get sys clock\\n\");\n> +\t\treturn PTR_ERR(csi2rx->sys_clk);\n> +\t}\n> +\n> +\tcsi2rx->p_clk = devm_clk_get(&pdev->dev, \"p_clk\");\n> +\tif (IS_ERR(csi2rx->p_clk)) {\n> +\t\tdev_err(&pdev->dev, \"Couldn't get P clock\\n\");\n> +\t\treturn PTR_ERR(csi2rx->p_clk);\n> +\t}\n> +\n> +\tcsi2rx->dphy = devm_phy_optional_get(&pdev->dev, \"dphy\");\n> +\tif (IS_ERR(csi2rx->dphy)) {\n> +\t\tdev_err(&pdev->dev, \"Couldn't get external D-PHY\\n\");\n> +\t\treturn PTR_ERR(csi2rx->dphy);\n> +\t}\n> +\n> +\t/*\n> +\t * FIXME: Once we'll have external D-PHY support, the check\n> +\t * will need to be removed.\n> +\t */\n> +\tif (csi2rx->dphy) {\n> +\t\tdev_err(&pdev->dev, \"External D-PHY not supported yet\\n\");\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tclk_prepare_enable(csi2rx->p_clk);\n> +\treg = readl(csi2rx->base + CSI2RX_DEVICE_CFG_REG);\n> +\tclk_disable_unprepare(csi2rx->p_clk);\n> +\n> +\tcsi2rx->max_lanes = (reg & 7);\n> +\tif (csi2rx->max_lanes > 4) {\n\nShould use a macro here.\n\n> +\t\tdev_err(&pdev->dev, \"Invalid number of lanes: %u\\n\",\n> +\t\t\tcsi2rx->max_lanes);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tcsi2rx->max_streams = ((reg >> 4) & 7);\n> +\tif (csi2rx->max_streams > CSI2RX_STREAMS_MAX) {\n> +\t\tdev_err(&pdev->dev, \"Invalid number of streams: %u\\n\",\n> +\t\t\tcsi2rx->max_streams);\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tcsi2rx->has_internal_dphy = (reg & BIT(3)) ? true : false;\n> +\n> +\t/*\n> +\t * FIXME: Once we'll have internal D-PHY support, the check\n> +\t * will need to be removed.\n> +\t */\n> +\tif (csi2rx->has_internal_dphy) {\n> +\t\tdev_err(&pdev->dev, \"Internal D-PHY not supported yet\\n\");\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tfor (i = 0; i < csi2rx->max_streams; i++) {\n> +\t\tchar clk_name[16];\n> +\n> +\t\tsnprintf(clk_name, sizeof(clk_name), \"pixel_if%u_clk\", i);\n> +\t\tcsi2rx->pixel_clk[i] = devm_clk_get(&pdev->dev, clk_name);\n> +\t\tif (IS_ERR(csi2rx->pixel_clk[i])) {\n> +\t\t\tdev_err(&pdev->dev, \"Couldn't get clock %s\\n\", clk_name);\n> +\t\t\treturn PTR_ERR(csi2rx->pixel_clk[i]);\n> +\t\t}\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)\n> +{\n> +\tstruct v4l2_fwnode_endpoint v4l2_ep;\n> +\tstruct device_node *ep, *remote;\n> +\tint ret;\n> +\n> +\tep = of_graph_get_endpoint_by_regs(csi2rx->dev->of_node, 0, 0);\n> +\tif (!ep)\n> +\t\treturn -EINVAL;\n> +\n> +\tret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);\n> +\tif (ret) {\n> +\t\tdev_err(csi2rx->dev, \"Could not parse v4l2 endpoint\\n\");\n> +\t\tgoto out;\n> +\t}\n> +\n> +\tif (v4l2_ep.bus_type != V4L2_MBUS_CSI2) {\n> +\t\tdev_err(csi2rx->dev, \"Unsupported media bus type: 0x%x\\n\",\n> +\t\t\tv4l2_ep.bus_type);\n> +\t\tret = -EINVAL;\n> +\t\tgoto out;\n> +\t}\n> +\n> +\tmemcpy(csi2rx->lanes, v4l2_ep.bus.mipi_csi2.data_lanes,\n> +\t       sizeof(csi2rx->lanes));\n> +\tcsi2rx->num_lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes;\n> +\tif (csi2rx->num_lanes > csi2rx->max_lanes) {\n> +\t\tdev_err(csi2rx->dev, \"Unsupported number of data-lanes: %d\\n\",\n> +\t\t\tcsi2rx->num_lanes);\n> +\t\tret = -EINVAL;\n> +\t\tgoto out;\n> +\t}\n> +\n> +\tremote = of_graph_get_remote_port_parent(ep);\n> +\tif (!remote) {\n> +\t\tdev_err(csi2rx->dev, \"No device found for endpoint %pOF\\n\", ep);\n> +\t\tret = -EINVAL;\n> +\t\tgoto out;\n> +\t}\n> +\n> +\tdev_dbg(csi2rx->dev, \"Found remote device %pOF\\n\", remote);\n> +\n> +\tcsi2rx->source_node = remote;\n> +\tcsi2rx->asd.match.fwnode.fwnode = &remote->fwnode;\n> +\tcsi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;\n> +\n> +out:\n> +\tof_node_put(ep);\n> +\treturn ret;\n> +}\n> +\n> +static int csi2rx_probe(struct platform_device *pdev)\n> +{\n> +\tstruct v4l2_async_subdev **subdevs;\n> +\tstruct csi2rx_priv *csi2rx;\n> +\tunsigned int i;\n> +\tint ret;\n> +\n> +\t/*\n> +\t * Since the v4l2_subdev structure is embedded in our\n> +\t * csi2rx_priv structure, and that the structure is exposed to\n> +\t * the user-space, we cannot just use the devm_variant\n> +\t * here. Indeed, that would lead to a use-after-free in a\n> +\t * open() - unbind - close() pattern.\n> +\t */\n> +\tcsi2rx = kzalloc(sizeof(*csi2rx), GFP_KERNEL);\n> +\tif (!csi2rx)\n> +\t\treturn -ENOMEM;\n> +\tplatform_set_drvdata(pdev, csi2rx);\n> +\tcsi2rx->dev = &pdev->dev;\n> +\n> +\tret = csi2rx_get_resources(csi2rx, pdev);\n> +\tif (ret)\n> +\t\tgoto err_free_priv;\n> +\n> +\tret = csi2rx_parse_dt(csi2rx);\n> +\tif (ret)\n> +\t\tgoto err_free_priv;\n> +\n> +\tcsi2rx->subdev.owner = THIS_MODULE;\n> +\tcsi2rx->subdev.dev = &pdev->dev;\n> +\tv4l2_subdev_init(&csi2rx->subdev, &csi2rx_subdev_ops);\n> +\tv4l2_set_subdevdata(&csi2rx->subdev, &pdev->dev);\n> +\tsnprintf(csi2rx->subdev.name, V4L2_SUBDEV_NAME_SIZE, \"%s.%s\",\n> +\t\t KBUILD_MODNAME, dev_name(&pdev->dev));\n> +\n> +\t/* Create our media pads */\n> +\tcsi2rx->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;\n> +\tcsi2rx->pads[CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;\n> +\tfor (i = CSI2RX_PAD_SOURCE_STREAM0; i < CSI2RX_PAD_MAX; i++)\n> +\t\tcsi2rx->pads[i].flags = MEDIA_PAD_FL_SOURCE;\n> +\n> +\tsubdevs = devm_kzalloc(csi2rx->dev, sizeof(*subdevs), GFP_KERNEL);\n> +\tif (!subdevs) {\n> +\t\tret = -ENOMEM;\n> +\t\tgoto err_free_priv;\n> +\t}\n> +\tsubdevs[0] = &csi2rx->asd;\n> +\n\nShouldn't the comment related to lifetime of memory allocation be also applied here?\nA reference to the \"subdevs\" pointer is taken internally so it might suffer the same fate.\nNot sure how many \"struct v4l2_async_subdev **subdevs\" we would end up needing\nbut since here we are only dealing with one, why not just make it a member of the\nstruct csi2rx_priv object.\n\n> +\tret = v4l2_async_subdev_notifier_register(&csi2rx->subdev, 1, subdevs,\n> +\t\t\t\t\t\t  csi2rx_async_bound,\n> +\t\t\t\t\t\t  csi2rx_async_complete,\n> +\t\t\t\t\t\t  csi2rx_async_unbind);\n> +\tif (ret < 0) {\n> +\t\tdev_err(csi2rx->dev, \"Failed to register our notifier\\n\");\n> +\t\tgoto err_free_priv;\n> +\t}\n> +\n> +\tret = media_entity_pads_init(&csi2rx->subdev.entity, CSI2RX_PAD_MAX,\n> +\t\t\t\t     csi2rx->pads);\n> +\tif (ret)\n> +\t\tgoto err_free_priv;\n> +\n> +\tret = v4l2_async_register_subdev(&csi2rx->subdev);\n> +\tif (ret < 0)\n> +\t\tgoto err_free_priv;\n> +\n> +\tdev_info(&pdev->dev,\n> +\t\t \"Probed CSI2RX with %u/%u lanes, %u streams, %s D-PHY\\n\",\n> +\t\t csi2rx->num_lanes, csi2rx->max_lanes, csi2rx->max_streams,\n> +\t\t csi2rx->has_internal_dphy ? \"internal\" : \"no\");\n> +\n> +\treturn 0;\n> +\n> +err_free_priv:\n> +\tkfree(csi2rx);\n> +\treturn ret;\n> +}\n> +\n> +static int csi2rx_remove(struct platform_device *pdev)\n> +{\n> +\tstruct csi2rx_priv *csi2rx = platform_get_drvdata(pdev);\n> +\n> +\tv4l2_async_unregister_subdev(&csi2rx->subdev);\n> +\tkfree(csi2rx);\n> +\n> +\treturn 0;\n> +}\n> +\n> +static const struct of_device_id csi2rx_of_table[] = {\n> +\t{ .compatible = \"cdns,csi2rx\" },\n> +\t{ },\n> +};\n> +MODULE_DEVICE_TABLE(of, csi2rx_of_table);\n> +\n> +static struct platform_driver csi2rx_driver = {\n> +\t.probe\t= csi2rx_probe,\n> +\t.remove\t= csi2rx_remove,\n> +\n> +\t.driver\t= {\n> +\t\t.name\t\t= \"cdns-csi2rx\",\n> +\t\t.of_match_table\t= csi2rx_of_table,\n> +\t},\n> +};\n> +module_platform_driver(csi2rx_driver);\n> -- \n> 2.13.5\n> \n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at  http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"<devicetree-owner@vger.kernel.org>","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=<UNKNOWN>)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ti.com header.i=@ti.com header.b=\"lsxCjKBw\"; \n\tdkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xsCqH70NXz9t2Q\n\tfor <incoming-dt@patchwork.ozlabs.org>;\n\tWed, 13 Sep 2017 04:23:55 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751413AbdILSXy (ORCPT\n\t<rfc822;incoming-dt@patchwork.ozlabs.org>);\n\tTue, 12 Sep 2017 14:23:54 -0400","from fllnx210.ext.ti.com ([198.47.19.17]:23951 \"EHLO\n\tfllnx210.ext.ti.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751380AbdILSXx (ORCPT\n\t<rfc822; devicetree@vger.kernel.org>); Tue, 12 Sep 2017 14:23:53 -0400","from dlelxv90.itg.ti.com ([172.17.2.17])\n\tby fllnx210.ext.ti.com (8.15.1/8.15.1) with ESMTP id v8CINdMI002668; \n\tTue, 12 Sep 2017 13:23:39 -0500","from DFLE103.ent.ti.com (dfle103.ent.ti.com [10.64.6.24])\n\tby dlelxv90.itg.ti.com (8.14.3/8.13.8) with ESMTP id v8CINdxJ001503; \n\tTue, 12 Sep 2017 13:23:39 -0500","from DFLE103.ent.ti.com (10.64.6.24) by DFLE103.ent.ti.com\n\t(10.64.6.24) with Microsoft SMTP Server (version=TLS1_2,\n\tcipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.1.845.34;\n\tTue, 12 Sep 2017 13:23:39 -0500","from dflp33.itg.ti.com (10.64.6.16) by DFLE103.ent.ti.com\n\t(10.64.6.24) with Microsoft SMTP Server (version=TLS1_0,\n\tcipher=TLS_RSA_WITH_AES_256_CBC_SHA) id 15.1.845.34 via Frontend\n\tTransport; Tue, 12 Sep 2017 13:23:39 -0500","from ti.com (ileax41-snat.itg.ti.com [10.172.224.153])\n\tby dflp33.itg.ti.com (8.14.3/8.13.8) with ESMTP id v8CINdjw029334;\n\tTue, 12 Sep 2017 13:23:39 -0500"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ti.com;\n\ts=ti-com-17Q1; t=1505240619;\n\tbh=GXnc9spb/edvXIyXFxF7QMaA6cty86+oV/NrKsJs8Ro=;\n\th=Date:From:To:CC:Subject:References:In-Reply-To;\n\tb=lsxCjKBwUF7hXTmUlbeZUNOkqvkrgn4TJUu+5Mf0YvlLj3J8UQWMb+EIZjvEYv6Hp\n\tlEOQ3gS2SQOEzGzSsMenu24GWemJk5s3NqauU+pZy7mLa+Fe9Vz4Xh+aSH2xPjwBOV\n\tuZv+nmUFRlc4xRUIftXOFbfmH0rKR3nEgduieSng=","Date":"Tue, 12 Sep 2017 13:23:39 -0500","From":"Benoit Parrot <bparrot@ti.com>","To":"Maxime Ripard <maxime.ripard@free-electrons.com>","CC":"Mauro Carvalho Chehab <mchehab@kernel.org>, Mark Rutland\n\t<mark.rutland@arm.com>, Rob Herring <robh+dt@kernel.org>, Laurent\n\tPinchart <laurent.pinchart@ideasonboard.com>, \n\t<linux-media@vger.kernel.org>, <devicetree@vger.kernel.org>, Cyprian\n\tWronka <cwronka@cadence.com>, Neil Webb <neilw@cadence.com>, Richard\n\tSproul <sproul@cadence.com>, Alan Douglas <adouglas@cadence.com>, \n\tSteve Creaney <screaney@cadence.com>, Thomas Petazzoni\n\t<thomas.petazzoni@free-electrons.com>, Boris Brezillon\n\t<boris.brezillon@free-electrons.com>, Niklas =?iso-8859-1?q?S=F6derlun?=\n\t=?iso-8859-1?q?d?= <niklas.soderlund@ragnatech.se>,\n\tHans Verkuil <hans.verkuil@cisco.com>, Sakari Ailus\n\t<sakari.ailus@linux.intel.com>","Subject":"Re: [PATCH v3 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver","Message-ID":"<20170912182339.GA27713@ti.com>","References":"<20170904130335.23280-1-maxime.ripard@free-electrons.com>\n\t<20170904130335.23280-3-maxime.ripard@free-electrons.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Disposition":"inline","In-Reply-To":"<20170904130335.23280-3-maxime.ripard@free-electrons.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","X-EXCLAIMER-MD-CONFIG":"e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<devicetree.vger.kernel.org>","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1767294,"web_url":"http://patchwork.ozlabs.org/comment/1767294/","msgid":"<20170912191312.GB27713@ti.com>","list_archive_url":null,"date":"2017-09-12T19:13:12","subject":"Re: [PATCH v3 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver","submitter":{"id":64914,"url":"http://patchwork.ozlabs.org/api/people/64914/","name":"Benoit Parrot","email":"bparrot@ti.com"},"content":"Benoit Parrot <bparrot@ti.com> wrote on Tue [2017-Sep-12 13:23:39 -0500]:\n\n<snip>\n\n> > +static int csi2rx_start(struct csi2rx_priv *csi2rx)\n> > +{\n> > +\tunsigned int i;\n> > +\tu32 reg;\n> > +\tint ret;\n> > +\n> > +\t/*\n> > +\t * We're not the first users, there's no need to enable the\n> > +\t * whole controller.\n> > +\t */\n> > +\tif (atomic_inc_return(&csi2rx->count) > 1)\n> > +\t\treturn 0;\n> > +\n> > +\tclk_prepare_enable(csi2rx->p_clk);\n> > +\n> > +\tprintk(\"%s %d\\n\", __func__, __LINE__);\n> \n> Some left over debug...\n> \n> > +\n> > +\tcsi2rx_reset(csi2rx);\n> > +\n> > +\treg = csi2rx->num_lanes << 8;\n> > +\tfor (i = 0; i < csi2rx->num_lanes; i++)\n> > +\t\treg |= CSI2RX_STATIC_CFG_DLANE_MAP(i, csi2rx->lanes[i]);\n> > +\n> > +\twritel(reg, csi2rx->base + CSI2RX_STATIC_CFG_REG);\n> > +\n> > +\tret = v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, true);\n> > +\tif (ret)\n> > +\t\treturn ret;\n> > +\n> > +\t/*\n> > +\t * Create a static mapping between the CSI virtual channels\n> > +\t * and the output stream.\n> > +\t *\n> > +\t * This should be enhanced, but v4l2 lacks the support for\n> > +\t * changing that mapping dynamically.\n> > +\t *\n> > +\t * We also cannot enable and disable independant streams here,\n> > +\t * hence the reference counting.\n> > +\t */\n> > +\tfor (i = 0; i < csi2rx->max_streams; i++) {\n> > +\t\tclk_prepare_enable(csi2rx->pixel_clk[i]);\n> > +\n> > +\t\twritel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,\n> > +\t\t       csi2rx->base + CSI2RX_STREAM_CFG_REG(i));\n> > +\n> > +\t\twritel(CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT |\n> > +\t\t       CSI2RX_STREAM_DATA_CFG_VC_SELECT(i),\n> > +\t\t       csi2rx->base + CSI2RX_STREAM_DATA_CFG_REG(i));\n\nI see here that we are setting the data_type to 0 (as we are not setting\nit) so effectively capturing everything on the channel(s).\nWill we be adding a method to select/filter specific data type?\nFor instance if we only want to grab YUV data in one stream and only\nRGB24 in another. Of course that would not be possible here as is...\n\nBenoit\n\n> > +\n> > +\t\twritel(CSI2RX_STREAM_CTRL_START,\n> > +\t\t       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));\n> > +\t}\n> > +\n> > +\tclk_prepare_enable(csi2rx->sys_clk);\n> > +\n> > +\tclk_disable_unprepare(csi2rx->p_clk);\n> > +\n> > +\treturn 0;\n> > +}\n\n<snip>\n--\nTo unsubscribe from this list: send the line \"unsubscribe devicetree\" in\nthe body of a message to majordomo@vger.kernel.org\nMore majordomo info at  http://vger.kernel.org/majordomo-info.html","headers":{"Return-Path":"<devicetree-owner@vger.kernel.org>","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=<UNKNOWN>)","ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ti.com header.i=@ti.com header.b=\"sCBB8ubY\"; \n\tdkim-atps=neutral"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xsDwl6mKqz9sNw\n\tfor <incoming-dt@patchwork.ozlabs.org>;\n\tWed, 13 Sep 2017 05:13:43 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1750869AbdILTNm (ORCPT\n\t<rfc822;incoming-dt@patchwork.ozlabs.org>);\n\tTue, 12 Sep 2017 15:13:42 -0400","from lelnx193.ext.ti.com ([198.47.27.77]:45866 \"EHLO\n\tlelnx193.ext.ti.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1750742AbdILTNl (ORCPT\n\t<rfc822; devicetree@vger.kernel.org>); Tue, 12 Sep 2017 15:13:41 -0400","from dflxv15.itg.ti.com ([128.247.5.124])\n\tby lelnx193.ext.ti.com (8.15.1/8.15.1) with ESMTP id v8CJDDZW004385; \n\tTue, 12 Sep 2017 14:13:13 -0500","from DLEE101.ent.ti.com (dlee101.ent.ti.com [157.170.170.31])\n\tby dflxv15.itg.ti.com (8.14.3/8.13.8) with ESMTP id v8CJDDAQ025004;\n\tTue, 12 Sep 2017 14:13:13 -0500","from DLEE104.ent.ti.com (157.170.170.34) by DLEE101.ent.ti.com\n\t(157.170.170.31) with Microsoft SMTP Server (version=TLS1_2,\n\tcipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.1.845.34;\n\tTue, 12 Sep 2017 14:13:13 -0500","from dflp33.itg.ti.com (10.64.6.16) by DLEE104.ent.ti.com\n\t(157.170.170.34) with Microsoft SMTP Server (version=TLS1_0,\n\tcipher=TLS_RSA_WITH_AES_256_CBC_SHA) id 15.1.845.34 via Frontend\n\tTransport; Tue, 12 Sep 2017 14:13:12 -0500","from ti.com (ileax41-snat.itg.ti.com [10.172.224.153])\n\tby dflp33.itg.ti.com (8.14.3/8.13.8) with ESMTP id v8CJDCsg014299;\n\tTue, 12 Sep 2017 14:13:12 -0500"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ti.com;\n\ts=ti-com-17Q1; t=1505243593;\n\tbh=x9JcORGTsR1Y06S0wZtle1DihKB7ZIp+o8UYu5r10yM=;\n\th=Date:From:To:CC:Subject:References:In-Reply-To;\n\tb=sCBB8ubYVrrOsYkRIOM2WEAqUepjuGizrUfLx5T8ihxYmqJLDdYht4HInBX8q85NI\n\tHLPy7c8niwP2Mgl7UyXHTMylzqoJlV6vm/tiYizlQbBou6xBCgiRcGWPT9/UiwHIVi\n\tbtF/mzsWVc1H/kkb7RLboGbMBoenQT6rS0cN5dHQ=","Date":"Tue, 12 Sep 2017 14:13:12 -0500","From":"Benoit Parrot <bparrot@ti.com>","To":"Maxime Ripard <maxime.ripard@free-electrons.com>","CC":"Mauro Carvalho Chehab <mchehab@kernel.org>, Mark Rutland\n\t<mark.rutland@arm.com>, Rob Herring <robh+dt@kernel.org>, Laurent\n\tPinchart <laurent.pinchart@ideasonboard.com>, \n\t<linux-media@vger.kernel.org>, <devicetree@vger.kernel.org>, Cyprian\n\tWronka <cwronka@cadence.com>, Neil Webb <neilw@cadence.com>, Richard\n\tSproul <sproul@cadence.com>, Alan Douglas <adouglas@cadence.com>, \n\tSteve Creaney <screaney@cadence.com>, Thomas Petazzoni\n\t<thomas.petazzoni@free-electrons.com>, Boris Brezillon\n\t<boris.brezillon@free-electrons.com>, Niklas =?iso-8859-1?q?S=F6derlun?=\n\t=?iso-8859-1?q?d?= <niklas.soderlund@ragnatech.se>,\n\tHans Verkuil <hans.verkuil@cisco.com>, Sakari Ailus\n\t<sakari.ailus@linux.intel.com>","Subject":"Re: [PATCH v3 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver","Message-ID":"<20170912191312.GB27713@ti.com>","References":"<20170904130335.23280-1-maxime.ripard@free-electrons.com>\n\t<20170904130335.23280-3-maxime.ripard@free-electrons.com>\n\t<20170912182339.GA27713@ti.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Disposition":"inline","In-Reply-To":"<20170912182339.GA27713@ti.com>","User-Agent":"Mutt/1.5.23 (2014-03-12)","X-EXCLAIMER-MD-CONFIG":"e1e8a2fd-e40a-4ac6-ac9b-f7e9cc9ee180","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<devicetree.vger.kernel.org>","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1768538,"web_url":"http://patchwork.ozlabs.org/comment/1768538/","msgid":"<20170914115429.cjulb2s74xsppx5j@flea.lan>","list_archive_url":null,"date":"2017-09-14T11:54:29","subject":"Re: [PATCH v3 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver","submitter":{"id":12916,"url":"http://patchwork.ozlabs.org/api/people/12916/","name":"Maxime Ripard","email":"maxime.ripard@free-electrons.com"},"content":"Hi Benoit,\n\nThanks for your comments,\n\nOn Tue, Sep 12, 2017 at 01:23:39PM -0500, Benoit Parrot wrote:\n> > +static int csi2rx_probe(struct platform_device *pdev)\n> > +{\n> > +\tstruct v4l2_async_subdev **subdevs;\n> > +\tstruct csi2rx_priv *csi2rx;\n> > +\tunsigned int i;\n> > +\tint ret;\n> > +\n> > +\t/*\n> > +\t * Since the v4l2_subdev structure is embedded in our\n> > +\t * csi2rx_priv structure, and that the structure is exposed to\n> > +\t * the user-space, we cannot just use the devm_variant\n> > +\t * here. Indeed, that would lead to a use-after-free in a\n> > +\t * open() - unbind - close() pattern.\n> > +\t */\n> > +\tcsi2rx = kzalloc(sizeof(*csi2rx), GFP_KERNEL);\n> > +\tif (!csi2rx)\n> > +\t\treturn -ENOMEM;\n> > +\tplatform_set_drvdata(pdev, csi2rx);\n> > +\tcsi2rx->dev = &pdev->dev;\n\n[snip]\n\n> > +\n> > +\tsubdevs = devm_kzalloc(csi2rx->dev, sizeof(*subdevs), GFP_KERNEL);\n> > +\tif (!subdevs) {\n> > +\t\tret = -ENOMEM;\n> > +\t\tgoto err_free_priv;\n> > +\t}\n> > +\tsubdevs[0] = &csi2rx->asd;\n> > +\n> \n> Shouldn't the comment related to lifetime of memory allocation be\n> also applied here?  A reference to the \"subdevs\" pointer is taken\n> internally so it might suffer the same fate.  Not sure how many\n> \"struct v4l2_async_subdev **subdevs\" we would end up needing but\n> since here we are only dealing with one, why not just make it a\n> member of the struct csi2rx_priv object.\n\nAs far as I know, only the notifier will use that array. The notifier\nwill be removed before that array is de-allocated, and the user-space\nnever has access to it, so I'm not sure the same issue arises here.\n\nMaxime","headers":{"Return-Path":"<devicetree-owner@vger.kernel.org>","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=<UNKNOWN>)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xtH5504Rwz9ryv\n\tfor <incoming-dt@patchwork.ozlabs.org>;\n\tThu, 14 Sep 2017 21:54:32 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751535AbdINLyb (ORCPT\n\t<rfc822;incoming-dt@patchwork.ozlabs.org>);\n\tThu, 14 Sep 2017 07:54:31 -0400","from mail.free-electrons.com ([62.4.15.54]:59423 \"EHLO\n\tmail.free-electrons.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751471AbdINLyb (ORCPT\n\t<rfc822; devicetree@vger.kernel.org>); Thu, 14 Sep 2017 07:54:31 -0400","by mail.free-electrons.com (Postfix, from userid 110)\n\tid 2200F2096A; Thu, 14 Sep 2017 13:54:29 +0200 (CEST)","from localhost (LStLambert-657-1-97-87.w90-63.abo.wanadoo.fr\n\t[90.63.216.87])\n\tby mail.free-electrons.com (Postfix) with ESMTPSA id ED7132090D;\n\tThu, 14 Sep 2017 13:54:28 +0200 (CEST)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on\n\tmail.free-electrons.com","X-Spam-Level":"","X-Spam-Status":"No, score=-1.0 required=5.0 tests=ALL_TRUSTED,SHORTCIRCUIT,\n\tURIBL_BLOCKED shortcircuit=ham autolearn=disabled version=3.4.0","Date":"Thu, 14 Sep 2017 13:54:29 +0200","From":"Maxime Ripard <maxime.ripard@free-electrons.com>","To":"Benoit Parrot <bparrot@ti.com>","Cc":"Mauro Carvalho Chehab <mchehab@kernel.org>, Mark Rutland\n\t<mark.rutland@arm.com>, Rob Herring <robh+dt@kernel.org>, Laurent\n\tPinchart <laurent.pinchart@ideasonboard.com>, \n\tlinux-media@vger.kernel.org, devicetree@vger.kernel.org, Cyprian Wronka\n\t<cwronka@cadence.com>, Neil Webb <neilw@cadence.com>, Richard Sproul\n\t<sproul@cadence.com>, Alan Douglas <adouglas@cadence.com>, Steve Creaney\n\t<screaney@cadence.com>, Thomas Petazzoni\n\t<thomas.petazzoni@free-electrons.com>, Boris Brezillon\n\t<boris.brezillon@free-electrons.com>, Niklas =?iso-8859-1?q?S=F6derlun?=\n\t=?iso-8859-1?q?d?= <niklas.soderlund@ragnatech.se>,\n\tHans Verkuil <hans.verkuil@cisco.com>, Sakari Ailus\n\t<sakari.ailus@linux.intel.com>","Subject":"Re: [PATCH v3 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver","Message-ID":"<20170914115429.cjulb2s74xsppx5j@flea.lan>","References":"<20170904130335.23280-1-maxime.ripard@free-electrons.com>\n\t<20170904130335.23280-3-maxime.ripard@free-electrons.com>\n\t<20170912182339.GA27713@ti.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"4julbau6b7y4g5px\"","Content-Disposition":"inline","In-Reply-To":"<20170912182339.GA27713@ti.com>","User-Agent":"NeoMutt/20170714 (1.8.3)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<devicetree.vger.kernel.org>","X-Mailing-List":"devicetree@vger.kernel.org"}},{"id":1768543,"web_url":"http://patchwork.ozlabs.org/comment/1768543/","msgid":"<20170914115758.7phdpeagiyvo4bf3@flea.lan>","list_archive_url":null,"date":"2017-09-14T11:57:58","subject":"Re: [PATCH v3 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver","submitter":{"id":12916,"url":"http://patchwork.ozlabs.org/api/people/12916/","name":"Maxime Ripard","email":"maxime.ripard@free-electrons.com"},"content":"On Tue, Sep 12, 2017 at 02:13:12PM -0500, Benoit Parrot wrote:\n> > > +\t/*\n> > > +\t * Create a static mapping between the CSI virtual channels\n> > > +\t * and the output stream.\n> > > +\t *\n> > > +\t * This should be enhanced, but v4l2 lacks the support for\n> > > +\t * changing that mapping dynamically.\n> > > +\t *\n> > > +\t * We also cannot enable and disable independant streams here,\n> > > +\t * hence the reference counting.\n> > > +\t */\n> > > +\tfor (i = 0; i < csi2rx->max_streams; i++) {\n> > > +\t\tclk_prepare_enable(csi2rx->pixel_clk[i]);\n> > > +\n> > > +\t\twritel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,\n> > > +\t\t       csi2rx->base + CSI2RX_STREAM_CFG_REG(i));\n> > > +\n> > > +\t\twritel(CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT |\n> > > +\t\t       CSI2RX_STREAM_DATA_CFG_VC_SELECT(i),\n> > > +\t\t       csi2rx->base + CSI2RX_STREAM_DATA_CFG_REG(i));\n> \n> I see here that we are setting the data_type to 0 (as we are not\n> setting it) so effectively capturing everything on the channel(s).\n> Will we be adding a method to select/filter specific data type?  For\n> instance if we only want to grab YUV data in one stream and only\n> RGB24 in another. Of course that would not be possible here as is...\n\nAh, right, I forgot about that. I've actually started that discussion\non another thread for a transceiver, without much success though:\nhttps://www.mail-archive.com/linux-media@vger.kernel.org/msg117920.html\n\nMaxime","headers":{"Return-Path":"<devicetree-owner@vger.kernel.org>","X-Original-To":"incoming-dt@patchwork.ozlabs.org","Delivered-To":"patchwork-incoming-dt@bilbo.ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=devicetree-owner@vger.kernel.org; receiver=<UNKNOWN>)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xtH956qZnz9ryv\n\tfor <incoming-dt@patchwork.ozlabs.org>;\n\tThu, 14 Sep 2017 21:58:01 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751561AbdINL6A (ORCPT\n\t<rfc822;incoming-dt@patchwork.ozlabs.org>);\n\tThu, 14 Sep 2017 07:58:00 -0400","from mail.free-electrons.com ([62.4.15.54]:59466 \"EHLO\n\tmail.free-electrons.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751537AbdINL6A (ORCPT\n\t<rfc822; devicetree@vger.kernel.org>); Thu, 14 Sep 2017 07:58:00 -0400","by mail.free-electrons.com (Postfix, from userid 110)\n\tid 9BE5B20971; Thu, 14 Sep 2017 13:57:58 +0200 (CEST)","from localhost (LStLambert-657-1-97-87.w90-63.abo.wanadoo.fr\n\t[90.63.216.87])\n\tby mail.free-electrons.com (Postfix) with ESMTPSA id 755562090F;\n\tThu, 14 Sep 2017 13:57:58 +0200 (CEST)"],"X-Spam-Checker-Version":"SpamAssassin 3.4.0 (2014-02-07) on\n\tmail.free-electrons.com","X-Spam-Level":"","X-Spam-Status":"No, score=-1.0 required=5.0 tests=ALL_TRUSTED,SHORTCIRCUIT,\n\tURIBL_BLOCKED shortcircuit=ham autolearn=disabled version=3.4.0","Date":"Thu, 14 Sep 2017 13:57:58 +0200","From":"Maxime Ripard <maxime.ripard@free-electrons.com>","To":"Benoit Parrot <bparrot@ti.com>","Cc":"Mauro Carvalho Chehab <mchehab@kernel.org>, Mark Rutland\n\t<mark.rutland@arm.com>, Rob Herring <robh+dt@kernel.org>, Laurent\n\tPinchart <laurent.pinchart@ideasonboard.com>, \n\tlinux-media@vger.kernel.org, devicetree@vger.kernel.org, Cyprian Wronka\n\t<cwronka@cadence.com>, Neil Webb <neilw@cadence.com>, Richard Sproul\n\t<sproul@cadence.com>, Alan Douglas <adouglas@cadence.com>, Steve Creaney\n\t<screaney@cadence.com>, Thomas Petazzoni\n\t<thomas.petazzoni@free-electrons.com>, Boris Brezillon\n\t<boris.brezillon@free-electrons.com>, Niklas =?iso-8859-1?q?S=F6derlun?=\n\t=?iso-8859-1?q?d?= <niklas.soderlund@ragnatech.se>,\n\tHans Verkuil <hans.verkuil@cisco.com>, Sakari Ailus\n\t<sakari.ailus@linux.intel.com>","Subject":"Re: [PATCH v3 2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver","Message-ID":"<20170914115758.7phdpeagiyvo4bf3@flea.lan>","References":"<20170904130335.23280-1-maxime.ripard@free-electrons.com>\n\t<20170904130335.23280-3-maxime.ripard@free-electrons.com>\n\t<20170912182339.GA27713@ti.com> <20170912191312.GB27713@ti.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha1;\n\tprotocol=\"application/pgp-signature\"; boundary=\"w7553w5onksf67sf\"","Content-Disposition":"inline","In-Reply-To":"<20170912191312.GB27713@ti.com>","User-Agent":"NeoMutt/20170714 (1.8.3)","Sender":"devicetree-owner@vger.kernel.org","Precedence":"bulk","List-ID":"<devicetree.vger.kernel.org>","X-Mailing-List":"devicetree@vger.kernel.org"}}]