diff mbox

[v2,2/2] v4l: cadence: Add Cadence MIPI-CSI2 RX driver

Message ID 20170720092302.2982-3-maxime.ripard@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Maxime Ripard July 20, 2017, 9:23 a.m. UTC
The Cadence CSI-2 RX Controller is an hardware block meant to be used as a
bridge between a CSI-2 bus and pixel grabbers.

It supports operating with internal or external D-PHY, with up to 4 lanes,
or without any D-PHY. The current code only supports the former case.

It also support dynamic mapping of the CSI-2 virtual channels to the
associated pixel grabbers, but that isn't allowed at the moment either.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/media/platform/Kconfig               |   1 +
 drivers/media/platform/Makefile              |   2 +
 drivers/media/platform/cadence/Kconfig       |  12 +
 drivers/media/platform/cadence/Makefile      |   1 +
 drivers/media/platform/cadence/cdns-csi2rx.c | 413 +++++++++++++++++++++++++++
 5 files changed, 429 insertions(+)
 create mode 100644 drivers/media/platform/cadence/Kconfig
 create mode 100644 drivers/media/platform/cadence/Makefile
 create mode 100644 drivers/media/platform/cadence/cdns-csi2rx.c

Comments

kernel test robot July 23, 2017, 1:09 a.m. UTC | #1
Hi Maxime,

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.13-rc1 next-20170721]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Maxime-Ripard/media-v4l-Add-support-for-the-Cadence-MIPI-CSI2-RX/20170723-083419
base:   git://linuxtv.org/media_tree.git master
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 6.2.0
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=ia64 

All error/warnings (new ones prefixed by >>):

   drivers/media/platform/cadence/cdns-csi2rx.c: In function 'csi2rx_async_bound':
>> drivers/media/platform/cadence/cdns-csi2rx.c:169:31: error: implicit declaration of function 'subnotifier_to_v4l2_subdev' [-Werror=implicit-function-declaration]
     struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/media/platform/cadence/cdns-csi2rx.c:169:31: warning: initialization makes pointer from integer without a cast [-Wint-conversion]
   drivers/media/platform/cadence/cdns-csi2rx.c: In function 'csi2rx_async_complete':
   drivers/media/platform/cadence/cdns-csi2rx.c:191:31: warning: initialization makes pointer from integer without a cast [-Wint-conversion]
     struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/media/platform/cadence/cdns-csi2rx.c: In function 'csi2rx_async_unbind':
   drivers/media/platform/cadence/cdns-csi2rx.c:205:31: warning: initialization makes pointer from integer without a cast [-Wint-conversion]
     struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/media/platform/cadence/cdns-csi2rx.c: In function 'csi2rx_parse_dt':
>> drivers/media/platform/cadence/cdns-csi2rx.c:324:8: error: implicit declaration of function 'v4l2_async_subdev_notifier_register' [-Werror=implicit-function-declaration]
     ret = v4l2_async_subdev_notifier_register(&csi2rx->subdev, 1, subdevs,
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/subnotifier_to_v4l2_subdev +169 drivers/media/platform/cadence/cdns-csi2rx.c

   164	
   165	static int csi2rx_async_bound(struct v4l2_async_notifier *notifier,
   166				      struct v4l2_subdev *s_subdev,
   167				      struct v4l2_async_subdev *asd)
   168	{
 > 169		struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
   170		struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
   171	
   172		csi2rx->sensor_pad = media_entity_get_fwnode_pad(&s_subdev->entity,
   173								 &csi2rx->sensor_node->fwnode,
   174								 MEDIA_PAD_FL_SOURCE);
   175		if (csi2rx->sensor_pad < 0) {
   176			dev_err(csi2rx->dev, "Couldn't find output pad for subdev %s\n",
   177				s_subdev->name);
   178			return csi2rx->sensor_pad;
   179		}
   180	
   181		csi2rx->sensor_subdev = s_subdev;
   182	
   183		dev_dbg(csi2rx->dev, "Bound %s pad: %d\n", s_subdev->name,
   184			csi2rx->sensor_pad);
   185	
   186		return 0;
   187	}
   188	
   189	static int csi2rx_async_complete(struct v4l2_async_notifier *notifier)
   190	{
 > 191		struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
   192		struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
   193	
   194		return media_create_pad_link(&csi2rx->sensor_subdev->entity,
   195					     csi2rx->sensor_pad,
   196					     &csi2rx->subdev.entity, 0,
   197					     MEDIA_LNK_FL_ENABLED |
   198					     MEDIA_LNK_FL_IMMUTABLE);
   199	}
   200	
   201	static void csi2rx_async_unbind(struct v4l2_async_notifier *notifier,
   202					struct v4l2_subdev *s_subdev,
   203					struct v4l2_async_subdev *asd)
   204	{
   205		struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
   206		struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
   207	
   208		dev_dbg(csi2rx->dev, "Unbound %s pad: %d\n", s_subdev->name,
   209			csi2rx->sensor_pad);
   210	
   211		csi2rx->sensor_subdev = NULL;
   212		csi2rx->sensor_pad = -EINVAL;
   213	}
   214	
   215	static int csi2rx_get_resources(struct csi2rx_priv *csi2rx,
   216					struct platform_device *pdev)
   217	{
   218		struct resource *res;
   219		u32 reg;
   220		int i;
   221	
   222		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
   223		csi2rx->base = devm_ioremap_resource(&pdev->dev, res);
   224		if (IS_ERR(csi2rx->base)) {
   225			dev_err(&pdev->dev, "Couldn't map our registers\n");
   226			return PTR_ERR(csi2rx->base);
   227		}
   228	
   229		reg = readl(csi2rx->base + CSI2RX_DEVICE_CFG_REG);
   230		csi2rx->max_lanes = (reg & 7) + 1;
   231		csi2rx->max_streams = ((reg >> 4) & 7);
   232		csi2rx->cdns_dphy = reg & BIT(3);
   233	
   234		csi2rx->sys_clk = devm_clk_get(&pdev->dev, "sys_clk");
   235		if (IS_ERR(csi2rx->sys_clk)) {
   236			dev_err(&pdev->dev, "Couldn't get sys clock\n");
   237			return PTR_ERR(csi2rx->sys_clk);
   238		}
   239	
   240		csi2rx->p_clk = devm_clk_get(&pdev->dev, "p_clk");
   241		if (IS_ERR(csi2rx->p_clk)) {
   242			dev_err(&pdev->dev, "Couldn't get P clock\n");
   243			return PTR_ERR(csi2rx->p_clk);
   244		}
   245	
   246		csi2rx->p_free_clk = devm_clk_get(&pdev->dev, "p_free_clk");
   247		if (IS_ERR(csi2rx->p_free_clk)) {
   248			dev_err(&pdev->dev, "Couldn't get free running P clock\n");
   249			return PTR_ERR(csi2rx->p_free_clk);
   250		}
   251	
   252		for (i = 0; i < csi2rx->max_streams; i++) {
   253			char clk_name[16];
   254	
   255			snprintf(clk_name, sizeof(clk_name), "pixel_if%u_clk", i);
   256			csi2rx->pixel_clk[i] = devm_clk_get(&pdev->dev, clk_name);
   257			if (IS_ERR(csi2rx->pixel_clk[i])) {
   258				dev_err(&pdev->dev, "Couldn't get clock %s\n", clk_name);
   259				return PTR_ERR(csi2rx->pixel_clk[i]);
   260			}
   261		}
   262	
   263		if (csi2rx->cdns_dphy) {
   264			csi2rx->dphy_rx_clk = devm_clk_get(&pdev->dev, "dphy_rx_clk");
   265			if (IS_ERR(csi2rx->dphy_rx_clk)) {
   266				dev_err(&pdev->dev, "Couldn't get D-PHY RX clock\n");
   267				return PTR_ERR(csi2rx->dphy_rx_clk);
   268			}
   269		}
   270	
   271		return 0;
   272	}
   273	
   274	static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)
   275	{
   276		struct v4l2_fwnode_endpoint v4l2_ep;
   277		struct v4l2_async_subdev **subdevs;
   278		struct device_node *ep, *remote;
   279		int ret = 0;
   280	
   281		ep = of_graph_get_endpoint_by_regs(csi2rx->dev->of_node, 0, 0);
   282		if (!ep)
   283			return -EINVAL;
   284	
   285		ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
   286		if (ret) {
   287			dev_err(csi2rx->dev, "Could not parse v4l2 endpoint\n");
   288			goto out;
   289		}
   290	
   291		if (v4l2_ep.bus_type != V4L2_MBUS_CSI2) {
   292			dev_err(csi2rx->dev, "Unsupported media bus type: 0x%x\n",
   293				v4l2_ep.bus_type);
   294			ret = -EINVAL;
   295			goto out;
   296		}
   297	
   298		csi2rx->lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes;
   299		if (csi2rx->lanes > csi2rx->max_lanes) {
   300			dev_err(csi2rx->dev, "Unsupported number of data-lanes: %d\n",
   301				csi2rx->lanes);
   302			ret = -EINVAL;
   303			goto out;
   304		}
   305	
   306		remote = of_graph_get_remote_port_parent(ep);
   307		if (!remote) {
   308			dev_err(csi2rx->dev, "No device found for endpoint %pOF\n", ep);
   309			ret = -EINVAL;
   310			goto out;
   311		}
   312	
   313		dev_dbg(csi2rx->dev, "Found remote device %pOF\n", remote);
   314	
   315		csi2rx->sensor_node = remote;
   316		csi2rx->asd.match.fwnode.fwnode = &remote->fwnode;
   317		csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
   318	
   319		subdevs = devm_kzalloc(csi2rx->dev, sizeof(*subdevs), GFP_KERNEL);
   320		if (subdevs == NULL)
   321			return -ENOMEM;
   322		subdevs[0] = &csi2rx->asd;
   323	
 > 324		ret = v4l2_async_subdev_notifier_register(&csi2rx->subdev, 1, subdevs,
   325							  csi2rx_async_bound,
   326							  csi2rx_async_complete,
   327							  csi2rx_async_unbind);
   328		if (ret < 0) {
   329			dev_err(csi2rx->dev, "Failed to register our notifier\n");
   330			return ret;
   331		}
   332	
   333	out:
   334		of_node_put(ep);
   335		return ret;
   336	}
   337	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Benoit Parrot Aug. 7, 2017, 7:24 p.m. UTC | #2
Maxime Ripard <maxime.ripard@free-electrons.com> wrote on Thu [2017-Jul-20 11:23:02 +0200]:
> The Cadence CSI-2 RX Controller is an hardware block meant to be used as a
> bridge between a CSI-2 bus and pixel grabbers.
> 
> It supports operating with internal or external D-PHY, with up to 4 lanes,
> or without any D-PHY. The current code only supports the former case.
> 
> It also support dynamic mapping of the CSI-2 virtual channels to the
> associated pixel grabbers, but that isn't allowed at the moment either.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/media/platform/Kconfig               |   1 +
>  drivers/media/platform/Makefile              |   2 +
>  drivers/media/platform/cadence/Kconfig       |  12 +
>  drivers/media/platform/cadence/Makefile      |   1 +
>  drivers/media/platform/cadence/cdns-csi2rx.c | 413 +++++++++++++++++++++++++++
>  5 files changed, 429 insertions(+)
>  create mode 100644 drivers/media/platform/cadence/Kconfig
>  create mode 100644 drivers/media/platform/cadence/Makefile
>  create mode 100644 drivers/media/platform/cadence/cdns-csi2rx.c
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 1313cd533436..a79d96e9b723 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -26,6 +26,7 @@ config VIDEO_VIA_CAMERA
>  #
>  # Platform multimedia device configuration
>  #
> +source "drivers/media/platform/cadence/Kconfig"
>  
>  source "drivers/media/platform/davinci/Kconfig"
>  
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 9beadc760467..1d31eb51e9bb 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -2,6 +2,8 @@
>  # Makefile for the video capture/playback device drivers.
>  #
>  
> +obj-$(CONFIG_VIDEO_CADENCE)		+= cadence/
> +
>  obj-$(CONFIG_VIDEO_M32R_AR_M64278) += arv.o
>  
>  obj-$(CONFIG_VIDEO_VIA_CAMERA) += via-camera.o
> diff --git a/drivers/media/platform/cadence/Kconfig b/drivers/media/platform/cadence/Kconfig
> new file mode 100644
> index 000000000000..d1b6bbb6a0eb
> --- /dev/null
> +++ b/drivers/media/platform/cadence/Kconfig
> @@ -0,0 +1,12 @@
> +config VIDEO_CADENCE
> +	bool "Cadence Video Devices"
> +
> +if VIDEO_CADENCE
> +
> +config VIDEO_CADENCE_CSI2RX
> +	tristate "Cadence MIPI-CSI2 RX Controller v1.3"
> +	depends on MEDIA_CONTROLLER
> +	depends on VIDEO_V4L2_SUBDEV_API
> +	select V4L2_FWNODE
> +
> +endif
> diff --git a/drivers/media/platform/cadence/Makefile b/drivers/media/platform/cadence/Makefile
> new file mode 100644
> index 000000000000..99a4086b7448
> --- /dev/null
> +++ b/drivers/media/platform/cadence/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_VIDEO_CADENCE_CSI2RX)	+= cdns-csi2rx.o
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> new file mode 100644
> index 000000000000..9a58f275f53c
> --- /dev/null
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -0,0 +1,413 @@
> +/*
> + * Driver for Cadence MIPI-CSI2 RX Controller v1.3
> + *
> + * Copyright (C) 2017 Cadence Design Systems Inc.
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-subdev.h>
> +
> +#define CSI2RX_DEVICE_CFG_REG			0x000
> +
> +#define CSI2RX_SOFT_RESET_REG			0x004
> +#define CSI2RX_SOFT_RESET_PROTOCOL			BIT(1)
> +#define CSI2RX_SOFT_RESET_FRONT				BIT(0)
> +
> +#define CSI2RX_STATIC_CFG_REG			0x008
> +
> +#define CSI2RX_STREAM_BASE(n)		(((n) + 1) * 0x100)
> +
> +#define CSI2RX_STREAM_CTRL_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x000)
> +#define CSI2RX_STREAM_CTRL_START			BIT(0)
> +
> +#define CSI2RX_STREAM_DATA_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x008)
> +#define CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT		BIT(31)
> +#define CSI2RX_STREAM_DATA_CFG_VC_SELECT(n)		BIT((n) + 16)
> +
> +#define CSI2RX_STREAM_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x00c)
> +#define CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF		(1 << 8)
> +
> +#define CSI2RX_STREAMS_MAX	4
> +

Just to confirm here that "streams" in this case are equivalent to
"Virtual Channel", correct?

> +enum csi2rx_pads {
> +	CSI2RX_PAD_SINK,
> +	CSI2RX_PAD_SOURCE_VC0,
> +	CSI2RX_PAD_SOURCE_VC1,
> +	CSI2RX_PAD_SOURCE_VC2,
> +	CSI2RX_PAD_SOURCE_VC3,
> +	CSI2RX_PAD_MAX,
> +};
> +
> +struct csi2rx_priv {
> +	struct device		*dev;
> +
> +	void __iomem		*base;
> +	struct clk		*sys_clk;
> +	struct clk		*p_clk;
> +	struct clk		*p_free_clk;
> +	struct clk		*pixel_clk[CSI2RX_STREAMS_MAX];
> +	struct clk		*dphy_rx_clk;
> +
> +	u8			lanes;
> +	u8			max_lanes;
> +	u8			max_streams;
> +	bool			cdns_dphy;
> +
> +	struct v4l2_subdev	subdev;
> +	struct media_pad	pads[CSI2RX_PAD_MAX];
> +
> +	/* Remote sensor */
> +	struct v4l2_async_subdev	asd;
> +	struct device_node		*sensor_node;
> +	struct v4l2_subdev		*sensor_subdev;
> +	int				sensor_pad;
> +};
> +
> +static inline
> +struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev)
> +{
> +	return container_of(subdev, struct csi2rx_priv, subdev);
> +}
> +
> +static void csi2rx_reset(struct csi2rx_priv *csi2rx)
> +{
> +	writel(CSI2RX_SOFT_RESET_PROTOCOL | CSI2RX_SOFT_RESET_FRONT,
> +	       csi2rx->base + CSI2RX_SOFT_RESET_REG);
> +
> +	udelay(10);

Shouldn't we use usleep_range() instead?

> +
> +	writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG);
> +}
> +
> +static int csi2rx_start(struct csi2rx_priv *csi2rx)
> +{
> +	u32 reg;
> +	int i;
> +
> +	csi2rx_reset(csi2rx);
> +
> +	// TODO: modify the mapping of the DPHY lanes?
> +	reg = readl(csi2rx->base + CSI2RX_STATIC_CFG_REG);
> +	reg &= ~GENMASK(11, 8);
> +	writel(reg | (csi2rx->lanes << 8),
> +	       csi2rx->base + CSI2RX_STATIC_CFG_REG);
> +
> +	/*
> +	 * Create a static mapping between the CSI virtual channels
> +	 * and the output stream.
> +	 *
> +	 * This should be enhanced, but v4l2 lacks the support for
> +	 * changing that mapping dynamically.
> +	 */
> +	for (i = 0; i < csi2rx->max_streams; i++) {
> +		writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,
> +		       csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
> +
> +		writel(CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT |
> +		       CSI2RX_STREAM_DATA_CFG_VC_SELECT(i),
> +		       csi2rx->base + CSI2RX_STREAM_DATA_CFG_REG(i));
> +
> +		writel(CSI2RX_STREAM_CTRL_START,
> +		       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> +	}
> +
> +	return 0;
> +}
> +
> +static int csi2rx_stop(struct csi2rx_priv *csi2rx)
> +{
> +	int i;
> +
> +	for (i = 0; i < csi2rx->max_streams; i++)
> +		writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> +
> +	return 0;
> +}
> +

Here, it is entirely possible that the "dma/buffer" engine which will
make use of this receiver creates separates video nodes for each streams.
In which case, you could theoretically have multiple user space capture
on-going.
But the "start" and "stop" method above would disrupt any of the other
stream. Unless you start and stop all 4 capture streams in lock step.

Eventually, the sub device might be a port aggregator which has up to
4 sensors on the source pad and feed each camera traffic on its own
Virtual Channel.

I know there isn't support in the framework for this currently but it
is something to think about.

Regards,
Benoit

> +static int csi2rx_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(sd);
> +
> +	v4l2_subdev_call(csi2rx->sensor_subdev, video, s_stream,
> +			 enable);
> +
> +	if (enable)
> +		csi2rx_start(csi2rx);
> +	else
> +		csi2rx_stop(csi2rx);
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
> +	.s_stream	= csi2rx_s_stream,
> +};
> +
> +static struct v4l2_subdev_ops csi2rx_subdev_ops = {
> +	.video		= &csi2rx_video_ops,
> +};
> +
> +static int csi2rx_async_bound(struct v4l2_async_notifier *notifier,
> +			      struct v4l2_subdev *s_subdev,
> +			      struct v4l2_async_subdev *asd)
> +{
> +	struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
> +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> +	csi2rx->sensor_pad = media_entity_get_fwnode_pad(&s_subdev->entity,
> +							 &csi2rx->sensor_node->fwnode,
> +							 MEDIA_PAD_FL_SOURCE);
> +	if (csi2rx->sensor_pad < 0) {
> +		dev_err(csi2rx->dev, "Couldn't find output pad for subdev %s\n",
> +			s_subdev->name);
> +		return csi2rx->sensor_pad;
> +	}
> +
> +	csi2rx->sensor_subdev = s_subdev;
> +
> +	dev_dbg(csi2rx->dev, "Bound %s pad: %d\n", s_subdev->name,
> +		csi2rx->sensor_pad);
> +
> +	return 0;
> +}
> +
> +static int csi2rx_async_complete(struct v4l2_async_notifier *notifier)
> +{
> +	struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
> +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> +	return media_create_pad_link(&csi2rx->sensor_subdev->entity,
> +				     csi2rx->sensor_pad,
> +				     &csi2rx->subdev.entity, 0,
> +				     MEDIA_LNK_FL_ENABLED |
> +				     MEDIA_LNK_FL_IMMUTABLE);
> +}
> +
> +static void csi2rx_async_unbind(struct v4l2_async_notifier *notifier,
> +				struct v4l2_subdev *s_subdev,
> +				struct v4l2_async_subdev *asd)
> +{
> +	struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
> +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> +	dev_dbg(csi2rx->dev, "Unbound %s pad: %d\n", s_subdev->name,
> +		csi2rx->sensor_pad);
> +
> +	csi2rx->sensor_subdev = NULL;
> +	csi2rx->sensor_pad = -EINVAL;
> +}
> +
> +static int csi2rx_get_resources(struct csi2rx_priv *csi2rx,
> +				struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	u32 reg;
> +	int i;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	csi2rx->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(csi2rx->base)) {
> +		dev_err(&pdev->dev, "Couldn't map our registers\n");
> +		return PTR_ERR(csi2rx->base);
> +	}
> +
> +	reg = readl(csi2rx->base + CSI2RX_DEVICE_CFG_REG);
> +	csi2rx->max_lanes = (reg & 7) + 1;
> +	csi2rx->max_streams = ((reg >> 4) & 7);
> +	csi2rx->cdns_dphy = reg & BIT(3);
> +
> +	csi2rx->sys_clk = devm_clk_get(&pdev->dev, "sys_clk");
> +	if (IS_ERR(csi2rx->sys_clk)) {
> +		dev_err(&pdev->dev, "Couldn't get sys clock\n");
> +		return PTR_ERR(csi2rx->sys_clk);
> +	}
> +
> +	csi2rx->p_clk = devm_clk_get(&pdev->dev, "p_clk");
> +	if (IS_ERR(csi2rx->p_clk)) {
> +		dev_err(&pdev->dev, "Couldn't get P clock\n");
> +		return PTR_ERR(csi2rx->p_clk);
> +	}
> +
> +	csi2rx->p_free_clk = devm_clk_get(&pdev->dev, "p_free_clk");
> +	if (IS_ERR(csi2rx->p_free_clk)) {
> +		dev_err(&pdev->dev, "Couldn't get free running P clock\n");
> +		return PTR_ERR(csi2rx->p_free_clk);
> +	}
> +
> +	for (i = 0; i < csi2rx->max_streams; i++) {
> +		char clk_name[16];
> +
> +		snprintf(clk_name, sizeof(clk_name), "pixel_if%u_clk", i);
> +		csi2rx->pixel_clk[i] = devm_clk_get(&pdev->dev, clk_name);
> +		if (IS_ERR(csi2rx->pixel_clk[i])) {
> +			dev_err(&pdev->dev, "Couldn't get clock %s\n", clk_name);
> +			return PTR_ERR(csi2rx->pixel_clk[i]);
> +		}
> +	}
> +
> +	if (csi2rx->cdns_dphy) {
> +		csi2rx->dphy_rx_clk = devm_clk_get(&pdev->dev, "dphy_rx_clk");
> +		if (IS_ERR(csi2rx->dphy_rx_clk)) {
> +			dev_err(&pdev->dev, "Couldn't get D-PHY RX clock\n");
> +			return PTR_ERR(csi2rx->dphy_rx_clk);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)
> +{
> +	struct v4l2_fwnode_endpoint v4l2_ep;
> +	struct v4l2_async_subdev **subdevs;
> +	struct device_node *ep, *remote;
> +	int ret = 0;
> +
> +	ep = of_graph_get_endpoint_by_regs(csi2rx->dev->of_node, 0, 0);
> +	if (!ep)
> +		return -EINVAL;
> +
> +	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
> +	if (ret) {
> +		dev_err(csi2rx->dev, "Could not parse v4l2 endpoint\n");
> +		goto out;
> +	}
> +
> +	if (v4l2_ep.bus_type != V4L2_MBUS_CSI2) {
> +		dev_err(csi2rx->dev, "Unsupported media bus type: 0x%x\n",
> +			v4l2_ep.bus_type);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	csi2rx->lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes;
> +	if (csi2rx->lanes > csi2rx->max_lanes) {
> +		dev_err(csi2rx->dev, "Unsupported number of data-lanes: %d\n",
> +			csi2rx->lanes);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	remote = of_graph_get_remote_port_parent(ep);
> +	if (!remote) {
> +		dev_err(csi2rx->dev, "No device found for endpoint %pOF\n", ep);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	dev_dbg(csi2rx->dev, "Found remote device %pOF\n", remote);
> +
> +	csi2rx->sensor_node = remote;
> +	csi2rx->asd.match.fwnode.fwnode = &remote->fwnode;
> +	csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> +
> +	subdevs = devm_kzalloc(csi2rx->dev, sizeof(*subdevs), GFP_KERNEL);
> +	if (subdevs == NULL)
> +		return -ENOMEM;
> +	subdevs[0] = &csi2rx->asd;
> +
> +	ret = v4l2_async_subdev_notifier_register(&csi2rx->subdev, 1, subdevs,
> +						  csi2rx_async_bound,
> +						  csi2rx_async_complete,
> +						  csi2rx_async_unbind);
> +	if (ret < 0) {
> +		dev_err(csi2rx->dev, "Failed to register our notifier\n");
> +		return ret;
> +	}
> +
> +out:
> +	of_node_put(ep);
> +	return ret;
> +}
> +
> +static int csi2rx_probe(struct platform_device *pdev)
> +{
> +	struct csi2rx_priv *csi2rx;
> +	int i, ret;
> +
> +	csi2rx = devm_kzalloc(&pdev->dev, sizeof(*csi2rx), GFP_KERNEL);
> +	if (!csi2rx)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, csi2rx);
> +	csi2rx->dev = &pdev->dev;
> +
> +	ret = csi2rx_get_resources(csi2rx, pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to get our resources\n");
> +		return ret;
> +	}
> +
> +	ret = csi2rx_parse_dt(csi2rx);
> +	if (ret)
> +		return ret;
> +
> +	csi2rx->subdev.owner = THIS_MODULE;
> +	csi2rx->subdev.dev = &pdev->dev;
> +	v4l2_subdev_init(&csi2rx->subdev, &csi2rx_subdev_ops);
> +	v4l2_set_subdevdata(&csi2rx->subdev, &pdev->dev);
> +	snprintf(csi2rx->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s.%s",
> +		 KBUILD_MODNAME, dev_name(&pdev->dev));
> +
> +	/* Create our media pads */
> +	csi2rx->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> +	csi2rx->pads[CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> +	for (i = CSI2RX_PAD_SOURCE_VC0; i < CSI2RX_PAD_MAX; i++)
> +		csi2rx->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> +
> +	ret = media_entity_pads_init(&csi2rx->subdev.entity, CSI2RX_PAD_MAX,
> +				     csi2rx->pads);
> +	if (ret)
> +		return ret;
> +
> +	ret = v4l2_async_register_subdev(&csi2rx->subdev);
> +	if (ret < 0)
> +		return ret;
> +
> +	dev_info(&pdev->dev,
> +		 "Probed CSI2RX with %d/%d lanes, %d streams, %s D-PHY\n",
> +		 csi2rx->lanes, csi2rx->max_lanes, csi2rx->max_streams,
> +		 csi2rx->cdns_dphy ? "Cadence" : "no");
> +
> +	return 0;
> +}
> +
> +static int csi2rx_remove(struct platform_device *pdev)
> +{
> +	struct csi2rx_priv *csi2rx = platform_get_drvdata(pdev);
> +
> +	v4l2_async_unregister_subdev(&csi2rx->subdev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id csi2rx_of_table[] = {
> +	{ .compatible = "cdns,csi2rx" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, csi2rx_of_table);
> +
> +static struct platform_driver csi2rx_driver = {
> +	.probe	= csi2rx_probe,
> +	.remove	= csi2rx_remove,
> +
> +	.driver	= {
> +		.name		= "cdns-csi2rx",
> +		.of_match_table	= csi2rx_of_table,
> +	},
> +};
> +module_platform_driver(csi2rx_driver);
> -- 
> 2.13.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laurent Pinchart Aug. 7, 2017, 8:42 p.m. UTC | #3
Hi Maxime,

Thank you for the patch.

On Thursday 20 Jul 2017 11:23:02 Maxime Ripard wrote:
> The Cadence CSI-2 RX Controller is an hardware block meant to be used as a
> bridge between a CSI-2 bus and pixel grabbers.
> 
> It supports operating with internal or external D-PHY, with up to 4 lanes,
> or without any D-PHY. The current code only supports the former case.
> 
> It also support dynamic mapping of the CSI-2 virtual channels to the
> associated pixel grabbers, but that isn't allowed at the moment either.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  drivers/media/platform/Kconfig               |   1 +
>  drivers/media/platform/Makefile              |   2 +
>  drivers/media/platform/cadence/Kconfig       |  12 +
>  drivers/media/platform/cadence/Makefile      |   1 +
>  drivers/media/platform/cadence/cdns-csi2rx.c | 413 ++++++++++++++++++++++++
>  5 files changed, 429 insertions(+)
>  create mode 100644 drivers/media/platform/cadence/Kconfig
>  create mode 100644 drivers/media/platform/cadence/Makefile
>  create mode 100644 drivers/media/platform/cadence/cdns-csi2rx.c

[snip]

> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c
> b/drivers/media/platform/cadence/cdns-csi2rx.c new file mode 100644
> index 000000000000..9a58f275f53c
> --- /dev/null
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -0,0 +1,413 @@
> +/*
> + * Driver for Cadence MIPI-CSI2 RX Controller v1.3
> + *
> + * Copyright (C) 2017 Cadence Design Systems Inc.
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by
> the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-subdev.h>
> +
> +#define CSI2RX_DEVICE_CFG_REG			0x000
> +
> +#define CSI2RX_SOFT_RESET_REG			0x004
> +#define CSI2RX_SOFT_RESET_PROTOCOL			BIT(1)
> +#define CSI2RX_SOFT_RESET_FRONT				BIT(0)
> +
> +#define CSI2RX_STATIC_CFG_REG			0x008
> +
> +#define CSI2RX_STREAM_BASE(n)		(((n) + 1) * 0x100)
> +
> +#define CSI2RX_STREAM_CTRL_REG(n)		(CSI2RX_STREAM_BASE(n) + 
0x000)
> +#define CSI2RX_STREAM_CTRL_START			BIT(0)
> +
> +#define CSI2RX_STREAM_DATA_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) 
+ 0x008)
> +#define CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT		BIT(31)
> +#define CSI2RX_STREAM_DATA_CFG_VC_SELECT(n)		BIT((n) + 16)
> +
> +#define CSI2RX_STREAM_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) + 
0x00c)
> +#define CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF		(1 << 8)
> +
> +#define CSI2RX_STREAMS_MAX	4
> +
> +enum csi2rx_pads {
> +	CSI2RX_PAD_SINK,
> +	CSI2RX_PAD_SOURCE_VC0,
> +	CSI2RX_PAD_SOURCE_VC1,
> +	CSI2RX_PAD_SOURCE_VC2,
> +	CSI2RX_PAD_SOURCE_VC3,
> +	CSI2RX_PAD_MAX,
> +};
> +
> +struct csi2rx_priv {
> +	struct device		*dev;
> +
> +	void __iomem		*base;
> +	struct clk		*sys_clk;
> +	struct clk		*p_clk;
> +	struct clk		*p_free_clk;
> +	struct clk		*pixel_clk[CSI2RX_STREAMS_MAX];
> +	struct clk		*dphy_rx_clk;
> +
> +	u8			lanes;
> +	u8			max_lanes;
> +	u8			max_streams;
> +	bool			cdns_dphy;
> +
> +	struct v4l2_subdev	subdev;
> +	struct media_pad	pads[CSI2RX_PAD_MAX];
> +
> +	/* Remote sensor */
> +	struct v4l2_async_subdev	asd;
> +	struct device_node		*sensor_node;
> +	struct v4l2_subdev		*sensor_subdev;
> +	int				sensor_pad;

The remove subdev might not be a sensor. I would rename those three fields to 
source_* (don't forget to update the comment accordingly).

> +};
> +
> +static inline
> +struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev)
> +{
> +	return container_of(subdev, struct csi2rx_priv, subdev);
> +}
> +
> +static void csi2rx_reset(struct csi2rx_priv *csi2rx)
> +{
> +	writel(CSI2RX_SOFT_RESET_PROTOCOL | CSI2RX_SOFT_RESET_FRONT,
> +	       csi2rx->base + CSI2RX_SOFT_RESET_REG);
> +
> +	udelay(10);
> +
> +	writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG);
> +}
> +
> +static int csi2rx_start(struct csi2rx_priv *csi2rx)
> +{
> +	u32 reg;
> +	int i;

i is never negative, you can make it an unsigned int.

> +	csi2rx_reset(csi2rx);
> +
> +	// TODO: modify the mapping of the DPHY lanes?

The mapping should be read from DT and applied here.

> +	reg = readl(csi2rx->base + CSI2RX_STATIC_CFG_REG);
> +	reg &= ~GENMASK(11, 8);

Could you define a macro for this register field ?

> +	writel(reg | (csi2rx->lanes << 8),
> +	       csi2rx->base + CSI2RX_STATIC_CFG_REG);
> +
> +	/*
> +	 * Create a static mapping between the CSI virtual channels
> +	 * and the output stream.
> +	 *
> +	 * This should be enhanced, but v4l2 lacks the support for
> +	 * changing that mapping dynamically.

V4L2 also lacks the ability to start/stop streams independently. That will 
hopefully be implemented at some point.

> +	 */
> +	for (i = 0; i < csi2rx->max_streams; i++) {
> +		writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,
> +		       csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
> +
> +		writel(CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT |
> +		       CSI2RX_STREAM_DATA_CFG_VC_SELECT(i),
> +		       csi2rx->base + CSI2RX_STREAM_DATA_CFG_REG(i));
> +
> +		writel(CSI2RX_STREAM_CTRL_START,
> +		       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> +	}
> +
> +	return 0;
> +}
> +
> +static int csi2rx_stop(struct csi2rx_priv *csi2rx)
> +{
> +	int i;

i is never negative, you can make it an unsigned int.

> +	for (i = 0; i < csi2rx->max_streams; i++)
> +		writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> +
> +	return 0;
> +}
> +
> +static int csi2rx_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(sd);
> +
> +	v4l2_subdev_call(csi2rx->sensor_subdev, video, s_stream,
> +			 enable);

Shouldn't you handle errors here ?

I'm not familiar with this IP core, but most D-PHYs need to synchronize to the 
input and must be started before the source.

> +	if (enable)
> +		csi2rx_start(csi2rx);
> +	else
> +		csi2rx_stop(csi2rx);
> +
> +	return 0;
> +}
> +
> +static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
> +	.s_stream	= csi2rx_s_stream,
> +};
> +
> +static struct v4l2_subdev_ops csi2rx_subdev_ops = {

This structure can be made const.

> +	.video		= &csi2rx_video_ops,
> +};
> +
> +static int csi2rx_async_bound(struct v4l2_async_notifier *notifier,
> +			      struct v4l2_subdev *s_subdev,
> +			      struct v4l2_async_subdev *asd)
> +{
> +	struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
> +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> +	csi2rx->sensor_pad = media_entity_get_fwnode_pad(&s_subdev->entity,
> +							 &csi2rx->sensor_node-
>fwnode,
> +							 MEDIA_PAD_FL_SOURCE);
> +	if (csi2rx->sensor_pad < 0) {
> +		dev_err(csi2rx->dev, "Couldn't find output pad for subdev 
%s\n",
> +			s_subdev->name);
> +		return csi2rx->sensor_pad;
> +	}
> +
> +	csi2rx->sensor_subdev = s_subdev;
> +
> +	dev_dbg(csi2rx->dev, "Bound %s pad: %d\n", s_subdev->name,
> +		csi2rx->sensor_pad);
> +
> +	return 0;
> +}
> +
> +static int csi2rx_async_complete(struct v4l2_async_notifier *notifier)
> +{
> +	struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
> +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> +	return media_create_pad_link(&csi2rx->sensor_subdev->entity,
> +				     csi2rx->sensor_pad,
> +				     &csi2rx->subdev.entity, 0,
> +				     MEDIA_LNK_FL_ENABLED |
> +				     MEDIA_LNK_FL_IMMUTABLE);
> +}
> +
> +static void csi2rx_async_unbind(struct v4l2_async_notifier *notifier,
> +				struct v4l2_subdev *s_subdev,
> +				struct v4l2_async_subdev *asd)
> +{
> +	struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
> +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> +
> +	dev_dbg(csi2rx->dev, "Unbound %s pad: %d\n", s_subdev->name,
> +		csi2rx->sensor_pad);
> +
> +	csi2rx->sensor_subdev = NULL;
> +	csi2rx->sensor_pad = -EINVAL;
> +}
> +
> +static int csi2rx_get_resources(struct csi2rx_priv *csi2rx,
> +				struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	u32 reg;
> +	int i;

i is never negative, you can make it an unsigned int.

> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	csi2rx->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(csi2rx->base)) {
> +		dev_err(&pdev->dev, "Couldn't map our registers\n");

devm_ioremap_resource() prints an error message, no need to duplicate it.

> +		return PTR_ERR(csi2rx->base);
> +	}
> +
> +	reg = readl(csi2rx->base + CSI2RX_DEVICE_CFG_REG);

Shouldn't you enable the register access clock(s) before reading the register 
?

> +	csi2rx->max_lanes = (reg & 7) + 1;

Up to 8 lanes ? Shouldn't this be (reg & 3) + 1 ?

> +	csi2rx->max_streams = ((reg >> 4) & 7);

Should you validate the value as you use it as an array access index ?

> +	csi2rx->cdns_dphy = reg & BIT(3);
> +
> +	csi2rx->sys_clk = devm_clk_get(&pdev->dev, "sys_clk");
> +	if (IS_ERR(csi2rx->sys_clk)) {
> +		dev_err(&pdev->dev, "Couldn't get sys clock\n");

If you wrote this as

		dev_err(&pdev->dev, "Couldn't get %s clock\n", "sys");

and the next messages in a similar fashion, most of the message wouldn't be 
duplicated, resulting in smaller code size.

> +		return PTR_ERR(csi2rx->sys_clk);
> +	}
> +
> +	csi2rx->p_clk = devm_clk_get(&pdev->dev, "p_clk");
> +	if (IS_ERR(csi2rx->p_clk)) {
> +		dev_err(&pdev->dev, "Couldn't get P clock\n");
> +		return PTR_ERR(csi2rx->p_clk);
> +	}
> +
> +	csi2rx->p_free_clk = devm_clk_get(&pdev->dev, "p_free_clk");
> +	if (IS_ERR(csi2rx->p_free_clk)) {
> +		dev_err(&pdev->dev, "Couldn't get free running P clock\n");
> +		return PTR_ERR(csi2rx->p_free_clk);
> +	}
> +
> +	for (i = 0; i < csi2rx->max_streams; i++) {
> +		char clk_name[16];

Isn't 13 enough ?

> +
> +		snprintf(clk_name, sizeof(clk_name), "pixel_if%u_clk", i);
> +		csi2rx->pixel_clk[i] = devm_clk_get(&pdev->dev, clk_name);
> +		if (IS_ERR(csi2rx->pixel_clk[i])) {
> +			dev_err(&pdev->dev, "Couldn't get clock %s\n", 
clk_name);
> +			return PTR_ERR(csi2rx->pixel_clk[i]);
> +		}
> +	}
> +
> +	if (csi2rx->cdns_dphy) {
> +		csi2rx->dphy_rx_clk = devm_clk_get(&pdev->dev, "dphy_rx_clk");
> +		if (IS_ERR(csi2rx->dphy_rx_clk)) {
> +			dev_err(&pdev->dev, "Couldn't get D-PHY RX clock\n");
> +			return PTR_ERR(csi2rx->dphy_rx_clk);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)
> +{
> +	struct v4l2_fwnode_endpoint v4l2_ep;
> +	struct v4l2_async_subdev **subdevs;
> +	struct device_node *ep, *remote;
> +	int ret = 0;

No need to initialize ret to 0.

> +
> +	ep = of_graph_get_endpoint_by_regs(csi2rx->dev->of_node, 0, 0);
> +	if (!ep)
> +		return -EINVAL;
> +
> +	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
> +	if (ret) {
> +		dev_err(csi2rx->dev, "Could not parse v4l2 endpoint\n");
> +		goto out;
> +	}
> +
> +	if (v4l2_ep.bus_type != V4L2_MBUS_CSI2) {
> +		dev_err(csi2rx->dev, "Unsupported media bus type: 0x%x\n",
> +			v4l2_ep.bus_type);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	csi2rx->lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes;
> +	if (csi2rx->lanes > csi2rx->max_lanes) {
> +		dev_err(csi2rx->dev, "Unsupported number of data-lanes: %d\n",
> +			csi2rx->lanes);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	remote = of_graph_get_remote_port_parent(ep);
> +	if (!remote) {
> +		dev_err(csi2rx->dev, "No device found for endpoint %pOF\n", 
ep);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	dev_dbg(csi2rx->dev, "Found remote device %pOF\n", remote);
> +
> +	csi2rx->sensor_node = remote;
> +	csi2rx->asd.match.fwnode.fwnode = &remote->fwnode;
> +	csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> +
> +	subdevs = devm_kzalloc(csi2rx->dev, sizeof(*subdevs), GFP_KERNEL);

Could you store this in the csi2rx_priv structure to avoid a dynamic 
allocation ?

> +	if (subdevs == NULL)
> +		return -ENOMEM;
> +	subdevs[0] = &csi2rx->asd;
> +
> +	ret = v4l2_async_subdev_notifier_register(&csi2rx->subdev, 1, subdevs,
> +						  csi2rx_async_bound,
> +						  csi2rx_async_complete,
> +						  csi2rx_async_unbind);
> +	if (ret < 0) {
> +		dev_err(csi2rx->dev, "Failed to register our notifier\n");
> +		return ret;
> +	}

I would register the notifier in the probe function, as this is not DT 
parsing.

> +out:
> +	of_node_put(ep);
> +	return ret;
> +}
> +
> +static int csi2rx_probe(struct platform_device *pdev)
> +{
> +	struct csi2rx_priv *csi2rx;
> +	int i, ret;

i is never negative, you can make it an unsigned int.

> +
> +	csi2rx = devm_kzalloc(&pdev->dev, sizeof(*csi2rx), GFP_KERNEL);

Please don't use devm_kzalloc() to allocate structures that can be accessed 
from userspace (in this case because it embeds the subdev structure, 
accessible at least through the subdevs ioctls). This is incompatible with 
proper unplug handling. There are many other issues that we will need to solve 
in the V4L2 core to handling unplugging properly, but let's not add a new one.

> +	if (!csi2rx)
> +		return -ENOMEM;
> +	platform_set_drvdata(pdev, csi2rx);
> +	csi2rx->dev = &pdev->dev;
> +
> +	ret = csi2rx_get_resources(csi2rx, pdev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to get our resources\n");

You already print error messages in the csi2rx_get_resources() function, 
there's no need to add a new one.

> +		return ret;
> +	}
> +
> +	ret = csi2rx_parse_dt(csi2rx);
> +	if (ret)
> +		return ret;
> +
> +	csi2rx->subdev.owner = THIS_MODULE;
> +	csi2rx->subdev.dev = &pdev->dev;
> +	v4l2_subdev_init(&csi2rx->subdev, &csi2rx_subdev_ops);
> +	v4l2_set_subdevdata(&csi2rx->subdev, &pdev->dev);
> +	snprintf(csi2rx->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s.%s",
> +		 KBUILD_MODNAME, dev_name(&pdev->dev));
> +
> +	/* Create our media pads */
> +	csi2rx->subdev.entity.function = 
MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> +	csi2rx->pads[CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> +	for (i = CSI2RX_PAD_SOURCE_VC0; i < CSI2RX_PAD_MAX; i++)
> +		csi2rx->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> +
> +	ret = media_entity_pads_init(&csi2rx->subdev.entity, CSI2RX_PAD_MAX,
> +				     csi2rx->pads);
> +	if (ret)
> +		return ret;
> +
> +	ret = v4l2_async_register_subdev(&csi2rx->subdev);
> +	if (ret < 0)
> +		return ret;
> +
> +	dev_info(&pdev->dev,
> +		 "Probed CSI2RX with %d/%d lanes, %d streams, %s D-PHY\n",

Please use %u for unsigned integers.

> +		 csi2rx->lanes, csi2rx->max_lanes, csi2rx->max_streams,
> +		 csi2rx->cdns_dphy ? "Cadence" : "no");

As the driver currently doesn't support external PHYs, should you treat the 
presence of an external PHY in DT as an error ?

> +	return 0;
> +}
> +
> +static int csi2rx_remove(struct platform_device *pdev)
> +{
> +	struct csi2rx_priv *csi2rx = platform_get_drvdata(pdev);
> +
> +	v4l2_async_unregister_subdev(&csi2rx->subdev);
> +
> +	return 0;
> +}

[snip]
Maxime Ripard Aug. 25, 2017, 2:03 p.m. UTC | #4
Hi Benoit,

On Mon, Aug 07, 2017 at 02:24:24PM -0500, Benoit Parrot wrote:
> > +#define CSI2RX_STREAMS_MAX	4
> > +
> 
> Just to confirm here that "streams" in this case are equivalent to
> "Virtual Channel", correct?

Kind of, yes, but it's a bit more complicated than that. The streams
are the output of the CSI2-RX block, so the interface to the capture
devices.

You can associate any CSI-2 virtual channel (in input) to any stream
(in output). However, as we don't have a way to change that routing at
runtime using v4l2 (yet [1]), the driver currently hardcode that
routing to have CSI-2 VC0 sent to stream 0, VC1 to stream 1, etc.

> > +static void csi2rx_reset(struct csi2rx_priv *csi2rx)
> > +{
> > +	writel(CSI2RX_SOFT_RESET_PROTOCOL | CSI2RX_SOFT_RESET_FRONT,
> > +	       csi2rx->base + CSI2RX_SOFT_RESET_REG);
> > +
> > +	udelay(10);
> 
> Shouldn't we use usleep_range() instead?

We totally should.

> > +static int csi2rx_stop(struct csi2rx_priv *csi2rx)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < csi2rx->max_streams; i++)
> > +		writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
> > +
> > +	return 0;
> > +}
> > +
> 
> Here, it is entirely possible that the "dma/buffer" engine which will
> make use of this receiver creates separates video nodes for each streams.
> In which case, you could theoretically have multiple user space capture
> on-going.
> But the "start" and "stop" method above would disrupt any of the other
> stream. Unless you start and stop all 4 capture streams in lock step.
> 
> Eventually, the sub device might be a port aggregator which has up to
> 4 sensors on the source pad and feed each camera traffic on its own
> Virtual Channel.
> 
> I know there isn't support in the framework for this currently but it
> is something to think about.

I guess you could make the same argument if you have several engines,
each one connected to a stream.

One way to work around that could be to add some reference counting in
the start and stop functions, and keep enabling all the outputs.

This wouldn't solve the underlying issue that all the stream would be
enabled and we don't really have a way to tell which one we want to
enable, but at least we wouldn't disrupt active streams when the first
one goes away.

Maxime

1: https://www.mail-archive.com/linux-media@vger.kernel.org/msg116955.html
Maxime Ripard Aug. 25, 2017, 2:30 p.m. UTC | #5
Hi Laurent,

I've removed the comments I'll take into account

On Mon, Aug 07, 2017 at 11:42:01PM +0300, Laurent Pinchart wrote:
> > +	csi2rx_reset(csi2rx);
> > +
> > +	// TODO: modify the mapping of the DPHY lanes?
> 
> The mapping should be read from DT and applied here.

As far as I understand it, this is a mapping between logical and
physical lanes before forwarding it to the D-PHY. Would data-lanes
apply for such a case?

> > +static int csi2rx_s_stream(struct v4l2_subdev *sd, int enable)
> > +{
> > +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(sd);
> > +
> > +	v4l2_subdev_call(csi2rx->sensor_subdev, video, s_stream,
> > +			 enable);
> 
> Shouldn't you handle errors here ?

Yes.

> I'm not familiar with this IP core, but most D-PHYs need to synchronize to the 
> input and must be started before the source.

We don't have any kind of D-PHY support at the moment, but I guess we
should put it there once we do.

> > +		return PTR_ERR(csi2rx->base);
> > +	}
> > +
> > +	reg = readl(csi2rx->base + CSI2RX_DEVICE_CFG_REG);
> 
> Shouldn't you enable the register access clock(s) before reading the
> register ?

Argh, you're right.

> 
> > +	csi2rx->max_lanes = (reg & 7) + 1;
> 
> Up to 8 lanes ? Shouldn't this be (reg & 3) + 1 ?

The bit field is indeed documented 3 bits wide, hence the 7.

> > +	csi2rx->max_streams = ((reg >> 4) & 7);
> 
> Should you validate the value as you use it as an array access index ?

I should, yes.

> > +	csi2rx->cdns_dphy = reg & BIT(3);
> > +
> > +	csi2rx->sys_clk = devm_clk_get(&pdev->dev, "sys_clk");
> > +	if (IS_ERR(csi2rx->sys_clk)) {
> > +		dev_err(&pdev->dev, "Couldn't get sys clock\n");
> 
> If you wrote this as
> 
> 		dev_err(&pdev->dev, "Couldn't get %s clock\n", "sys");
> 
> and the next messages in a similar fashion, most of the message wouldn't be 
> duplicated, resulting in smaller code size.

I'm usually not a big fan of this, since one side effect is also that
you can't grep / search it anymore, and I'm not sure it's worth the
hassle for a couple dozen bytes.

> 
> > +		return PTR_ERR(csi2rx->sys_clk);
> > +	}
> > +
> > +	csi2rx->p_clk = devm_clk_get(&pdev->dev, "p_clk");
> > +	if (IS_ERR(csi2rx->p_clk)) {
> > +		dev_err(&pdev->dev, "Couldn't get P clock\n");
> > +		return PTR_ERR(csi2rx->p_clk);
> > +	}
> > +
> > +	csi2rx->p_free_clk = devm_clk_get(&pdev->dev, "p_free_clk");
> > +	if (IS_ERR(csi2rx->p_free_clk)) {
> > +		dev_err(&pdev->dev, "Couldn't get free running P clock\n");
> > +		return PTR_ERR(csi2rx->p_free_clk);
> > +	}
> > +
> > +	for (i = 0; i < csi2rx->max_streams; i++) {
> > +		char clk_name[16];
> 
> Isn't 13 enough ?

It is, if you have an int short enough.

> > +	csi2rx->sensor_node = remote;
> > +	csi2rx->asd.match.fwnode.fwnode = &remote->fwnode;
> > +	csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > +
> > +	subdevs = devm_kzalloc(csi2rx->dev, sizeof(*subdevs), GFP_KERNEL);
> 
> Could you store this in the csi2rx_priv structure to avoid a dynamic 
> allocation ?

It's used only in this function, so it seems a bit overkill to do
that. But I guess I could just allocate it on the stack.

> > +
> > +	csi2rx = devm_kzalloc(&pdev->dev, sizeof(*csi2rx), GFP_KERNEL);
> 
> Please don't use devm_kzalloc() to allocate structures that can be accessed 
> from userspace (in this case because it embeds the subdev structure, 
> accessible at least through the subdevs ioctls). This is incompatible with 
> proper unplug handling. There are many other issues that we will need to solve 
> in the V4L2 core to handling unplugging properly, but let's not add a new one.

What's wrong with kzalloc in such a case? As far as I know, the
userspace can access such a memory, as long as you use copy_to_user,
right? Or is it because of the life-cycle of the allocation that would
be gone while the userspace might not be yet? I'm not sure what would
be a proper fix for it though.

Thanks for your review!
Maxime
diff mbox

Patch

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 1313cd533436..a79d96e9b723 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -26,6 +26,7 @@  config VIDEO_VIA_CAMERA
 #
 # Platform multimedia device configuration
 #
+source "drivers/media/platform/cadence/Kconfig"
 
 source "drivers/media/platform/davinci/Kconfig"
 
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 9beadc760467..1d31eb51e9bb 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -2,6 +2,8 @@ 
 # Makefile for the video capture/playback device drivers.
 #
 
+obj-$(CONFIG_VIDEO_CADENCE)		+= cadence/
+
 obj-$(CONFIG_VIDEO_M32R_AR_M64278) += arv.o
 
 obj-$(CONFIG_VIDEO_VIA_CAMERA) += via-camera.o
diff --git a/drivers/media/platform/cadence/Kconfig b/drivers/media/platform/cadence/Kconfig
new file mode 100644
index 000000000000..d1b6bbb6a0eb
--- /dev/null
+++ b/drivers/media/platform/cadence/Kconfig
@@ -0,0 +1,12 @@ 
+config VIDEO_CADENCE
+	bool "Cadence Video Devices"
+
+if VIDEO_CADENCE
+
+config VIDEO_CADENCE_CSI2RX
+	tristate "Cadence MIPI-CSI2 RX Controller v1.3"
+	depends on MEDIA_CONTROLLER
+	depends on VIDEO_V4L2_SUBDEV_API
+	select V4L2_FWNODE
+
+endif
diff --git a/drivers/media/platform/cadence/Makefile b/drivers/media/platform/cadence/Makefile
new file mode 100644
index 000000000000..99a4086b7448
--- /dev/null
+++ b/drivers/media/platform/cadence/Makefile
@@ -0,0 +1 @@ 
+obj-$(CONFIG_VIDEO_CADENCE_CSI2RX)	+= cdns-csi2rx.o
diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
new file mode 100644
index 000000000000..9a58f275f53c
--- /dev/null
+++ b/drivers/media/platform/cadence/cdns-csi2rx.c
@@ -0,0 +1,413 @@ 
+/*
+ * Driver for Cadence MIPI-CSI2 RX Controller v1.3
+ *
+ * Copyright (C) 2017 Cadence Design Systems Inc.
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/platform_device.h>
+
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
+#include <media/v4l2-subdev.h>
+
+#define CSI2RX_DEVICE_CFG_REG			0x000
+
+#define CSI2RX_SOFT_RESET_REG			0x004
+#define CSI2RX_SOFT_RESET_PROTOCOL			BIT(1)
+#define CSI2RX_SOFT_RESET_FRONT				BIT(0)
+
+#define CSI2RX_STATIC_CFG_REG			0x008
+
+#define CSI2RX_STREAM_BASE(n)		(((n) + 1) * 0x100)
+
+#define CSI2RX_STREAM_CTRL_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x000)
+#define CSI2RX_STREAM_CTRL_START			BIT(0)
+
+#define CSI2RX_STREAM_DATA_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x008)
+#define CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT		BIT(31)
+#define CSI2RX_STREAM_DATA_CFG_VC_SELECT(n)		BIT((n) + 16)
+
+#define CSI2RX_STREAM_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x00c)
+#define CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF		(1 << 8)
+
+#define CSI2RX_STREAMS_MAX	4
+
+enum csi2rx_pads {
+	CSI2RX_PAD_SINK,
+	CSI2RX_PAD_SOURCE_VC0,
+	CSI2RX_PAD_SOURCE_VC1,
+	CSI2RX_PAD_SOURCE_VC2,
+	CSI2RX_PAD_SOURCE_VC3,
+	CSI2RX_PAD_MAX,
+};
+
+struct csi2rx_priv {
+	struct device		*dev;
+
+	void __iomem		*base;
+	struct clk		*sys_clk;
+	struct clk		*p_clk;
+	struct clk		*p_free_clk;
+	struct clk		*pixel_clk[CSI2RX_STREAMS_MAX];
+	struct clk		*dphy_rx_clk;
+
+	u8			lanes;
+	u8			max_lanes;
+	u8			max_streams;
+	bool			cdns_dphy;
+
+	struct v4l2_subdev	subdev;
+	struct media_pad	pads[CSI2RX_PAD_MAX];
+
+	/* Remote sensor */
+	struct v4l2_async_subdev	asd;
+	struct device_node		*sensor_node;
+	struct v4l2_subdev		*sensor_subdev;
+	int				sensor_pad;
+};
+
+static inline
+struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev)
+{
+	return container_of(subdev, struct csi2rx_priv, subdev);
+}
+
+static void csi2rx_reset(struct csi2rx_priv *csi2rx)
+{
+	writel(CSI2RX_SOFT_RESET_PROTOCOL | CSI2RX_SOFT_RESET_FRONT,
+	       csi2rx->base + CSI2RX_SOFT_RESET_REG);
+
+	udelay(10);
+
+	writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG);
+}
+
+static int csi2rx_start(struct csi2rx_priv *csi2rx)
+{
+	u32 reg;
+	int i;
+
+	csi2rx_reset(csi2rx);
+
+	// TODO: modify the mapping of the DPHY lanes?
+	reg = readl(csi2rx->base + CSI2RX_STATIC_CFG_REG);
+	reg &= ~GENMASK(11, 8);
+	writel(reg | (csi2rx->lanes << 8),
+	       csi2rx->base + CSI2RX_STATIC_CFG_REG);
+
+	/*
+	 * Create a static mapping between the CSI virtual channels
+	 * and the output stream.
+	 *
+	 * This should be enhanced, but v4l2 lacks the support for
+	 * changing that mapping dynamically.
+	 */
+	for (i = 0; i < csi2rx->max_streams; i++) {
+		writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,
+		       csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
+
+		writel(CSI2RX_STREAM_DATA_CFG_EN_VC_SELECT |
+		       CSI2RX_STREAM_DATA_CFG_VC_SELECT(i),
+		       csi2rx->base + CSI2RX_STREAM_DATA_CFG_REG(i));
+
+		writel(CSI2RX_STREAM_CTRL_START,
+		       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
+	}
+
+	return 0;
+}
+
+static int csi2rx_stop(struct csi2rx_priv *csi2rx)
+{
+	int i;
+
+	for (i = 0; i < csi2rx->max_streams; i++)
+		writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
+
+	return 0;
+}
+
+static int csi2rx_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(sd);
+
+	v4l2_subdev_call(csi2rx->sensor_subdev, video, s_stream,
+			 enable);
+
+	if (enable)
+		csi2rx_start(csi2rx);
+	else
+		csi2rx_stop(csi2rx);
+
+	return 0;
+}
+
+static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
+	.s_stream	= csi2rx_s_stream,
+};
+
+static struct v4l2_subdev_ops csi2rx_subdev_ops = {
+	.video		= &csi2rx_video_ops,
+};
+
+static int csi2rx_async_bound(struct v4l2_async_notifier *notifier,
+			      struct v4l2_subdev *s_subdev,
+			      struct v4l2_async_subdev *asd)
+{
+	struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
+	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
+
+	csi2rx->sensor_pad = media_entity_get_fwnode_pad(&s_subdev->entity,
+							 &csi2rx->sensor_node->fwnode,
+							 MEDIA_PAD_FL_SOURCE);
+	if (csi2rx->sensor_pad < 0) {
+		dev_err(csi2rx->dev, "Couldn't find output pad for subdev %s\n",
+			s_subdev->name);
+		return csi2rx->sensor_pad;
+	}
+
+	csi2rx->sensor_subdev = s_subdev;
+
+	dev_dbg(csi2rx->dev, "Bound %s pad: %d\n", s_subdev->name,
+		csi2rx->sensor_pad);
+
+	return 0;
+}
+
+static int csi2rx_async_complete(struct v4l2_async_notifier *notifier)
+{
+	struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
+	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
+
+	return media_create_pad_link(&csi2rx->sensor_subdev->entity,
+				     csi2rx->sensor_pad,
+				     &csi2rx->subdev.entity, 0,
+				     MEDIA_LNK_FL_ENABLED |
+				     MEDIA_LNK_FL_IMMUTABLE);
+}
+
+static void csi2rx_async_unbind(struct v4l2_async_notifier *notifier,
+				struct v4l2_subdev *s_subdev,
+				struct v4l2_async_subdev *asd)
+{
+	struct v4l2_subdev *subdev = subnotifier_to_v4l2_subdev(notifier);
+	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
+
+	dev_dbg(csi2rx->dev, "Unbound %s pad: %d\n", s_subdev->name,
+		csi2rx->sensor_pad);
+
+	csi2rx->sensor_subdev = NULL;
+	csi2rx->sensor_pad = -EINVAL;
+}
+
+static int csi2rx_get_resources(struct csi2rx_priv *csi2rx,
+				struct platform_device *pdev)
+{
+	struct resource *res;
+	u32 reg;
+	int i;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	csi2rx->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(csi2rx->base)) {
+		dev_err(&pdev->dev, "Couldn't map our registers\n");
+		return PTR_ERR(csi2rx->base);
+	}
+
+	reg = readl(csi2rx->base + CSI2RX_DEVICE_CFG_REG);
+	csi2rx->max_lanes = (reg & 7) + 1;
+	csi2rx->max_streams = ((reg >> 4) & 7);
+	csi2rx->cdns_dphy = reg & BIT(3);
+
+	csi2rx->sys_clk = devm_clk_get(&pdev->dev, "sys_clk");
+	if (IS_ERR(csi2rx->sys_clk)) {
+		dev_err(&pdev->dev, "Couldn't get sys clock\n");
+		return PTR_ERR(csi2rx->sys_clk);
+	}
+
+	csi2rx->p_clk = devm_clk_get(&pdev->dev, "p_clk");
+	if (IS_ERR(csi2rx->p_clk)) {
+		dev_err(&pdev->dev, "Couldn't get P clock\n");
+		return PTR_ERR(csi2rx->p_clk);
+	}
+
+	csi2rx->p_free_clk = devm_clk_get(&pdev->dev, "p_free_clk");
+	if (IS_ERR(csi2rx->p_free_clk)) {
+		dev_err(&pdev->dev, "Couldn't get free running P clock\n");
+		return PTR_ERR(csi2rx->p_free_clk);
+	}
+
+	for (i = 0; i < csi2rx->max_streams; i++) {
+		char clk_name[16];
+
+		snprintf(clk_name, sizeof(clk_name), "pixel_if%u_clk", i);
+		csi2rx->pixel_clk[i] = devm_clk_get(&pdev->dev, clk_name);
+		if (IS_ERR(csi2rx->pixel_clk[i])) {
+			dev_err(&pdev->dev, "Couldn't get clock %s\n", clk_name);
+			return PTR_ERR(csi2rx->pixel_clk[i]);
+		}
+	}
+
+	if (csi2rx->cdns_dphy) {
+		csi2rx->dphy_rx_clk = devm_clk_get(&pdev->dev, "dphy_rx_clk");
+		if (IS_ERR(csi2rx->dphy_rx_clk)) {
+			dev_err(&pdev->dev, "Couldn't get D-PHY RX clock\n");
+			return PTR_ERR(csi2rx->dphy_rx_clk);
+		}
+	}
+
+	return 0;
+}
+
+static int csi2rx_parse_dt(struct csi2rx_priv *csi2rx)
+{
+	struct v4l2_fwnode_endpoint v4l2_ep;
+	struct v4l2_async_subdev **subdevs;
+	struct device_node *ep, *remote;
+	int ret = 0;
+
+	ep = of_graph_get_endpoint_by_regs(csi2rx->dev->of_node, 0, 0);
+	if (!ep)
+		return -EINVAL;
+
+	ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
+	if (ret) {
+		dev_err(csi2rx->dev, "Could not parse v4l2 endpoint\n");
+		goto out;
+	}
+
+	if (v4l2_ep.bus_type != V4L2_MBUS_CSI2) {
+		dev_err(csi2rx->dev, "Unsupported media bus type: 0x%x\n",
+			v4l2_ep.bus_type);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	csi2rx->lanes = v4l2_ep.bus.mipi_csi2.num_data_lanes;
+	if (csi2rx->lanes > csi2rx->max_lanes) {
+		dev_err(csi2rx->dev, "Unsupported number of data-lanes: %d\n",
+			csi2rx->lanes);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	remote = of_graph_get_remote_port_parent(ep);
+	if (!remote) {
+		dev_err(csi2rx->dev, "No device found for endpoint %pOF\n", ep);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	dev_dbg(csi2rx->dev, "Found remote device %pOF\n", remote);
+
+	csi2rx->sensor_node = remote;
+	csi2rx->asd.match.fwnode.fwnode = &remote->fwnode;
+	csi2rx->asd.match_type = V4L2_ASYNC_MATCH_FWNODE;
+
+	subdevs = devm_kzalloc(csi2rx->dev, sizeof(*subdevs), GFP_KERNEL);
+	if (subdevs == NULL)
+		return -ENOMEM;
+	subdevs[0] = &csi2rx->asd;
+
+	ret = v4l2_async_subdev_notifier_register(&csi2rx->subdev, 1, subdevs,
+						  csi2rx_async_bound,
+						  csi2rx_async_complete,
+						  csi2rx_async_unbind);
+	if (ret < 0) {
+		dev_err(csi2rx->dev, "Failed to register our notifier\n");
+		return ret;
+	}
+
+out:
+	of_node_put(ep);
+	return ret;
+}
+
+static int csi2rx_probe(struct platform_device *pdev)
+{
+	struct csi2rx_priv *csi2rx;
+	int i, ret;
+
+	csi2rx = devm_kzalloc(&pdev->dev, sizeof(*csi2rx), GFP_KERNEL);
+	if (!csi2rx)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, csi2rx);
+	csi2rx->dev = &pdev->dev;
+
+	ret = csi2rx_get_resources(csi2rx, pdev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to get our resources\n");
+		return ret;
+	}
+
+	ret = csi2rx_parse_dt(csi2rx);
+	if (ret)
+		return ret;
+
+	csi2rx->subdev.owner = THIS_MODULE;
+	csi2rx->subdev.dev = &pdev->dev;
+	v4l2_subdev_init(&csi2rx->subdev, &csi2rx_subdev_ops);
+	v4l2_set_subdevdata(&csi2rx->subdev, &pdev->dev);
+	snprintf(csi2rx->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s.%s",
+		 KBUILD_MODNAME, dev_name(&pdev->dev));
+
+	/* Create our media pads */
+	csi2rx->subdev.entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
+	csi2rx->pads[CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
+	for (i = CSI2RX_PAD_SOURCE_VC0; i < CSI2RX_PAD_MAX; i++)
+		csi2rx->pads[i].flags = MEDIA_PAD_FL_SOURCE;
+
+	ret = media_entity_pads_init(&csi2rx->subdev.entity, CSI2RX_PAD_MAX,
+				     csi2rx->pads);
+	if (ret)
+		return ret;
+
+	ret = v4l2_async_register_subdev(&csi2rx->subdev);
+	if (ret < 0)
+		return ret;
+
+	dev_info(&pdev->dev,
+		 "Probed CSI2RX with %d/%d lanes, %d streams, %s D-PHY\n",
+		 csi2rx->lanes, csi2rx->max_lanes, csi2rx->max_streams,
+		 csi2rx->cdns_dphy ? "Cadence" : "no");
+
+	return 0;
+}
+
+static int csi2rx_remove(struct platform_device *pdev)
+{
+	struct csi2rx_priv *csi2rx = platform_get_drvdata(pdev);
+
+	v4l2_async_unregister_subdev(&csi2rx->subdev);
+
+	return 0;
+}
+
+static const struct of_device_id csi2rx_of_table[] = {
+	{ .compatible = "cdns,csi2rx" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, csi2rx_of_table);
+
+static struct platform_driver csi2rx_driver = {
+	.probe	= csi2rx_probe,
+	.remove	= csi2rx_remove,
+
+	.driver	= {
+		.name		= "cdns-csi2rx",
+		.of_match_table	= csi2rx_of_table,
+	},
+};
+module_platform_driver(csi2rx_driver);