Message-ID: <6923fefa-0358-c496-fc97-cb84bbb42d9a@starfivetech.com>
Date:   Mon, 20 Mar 2023 19:54:49 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH v2 4/6] media: cadence: Add support for external dphy and
 JH7110 SoC
Content-Language: en-US
To:     Laurent Pinchart <laurent.pinchart@ideasonboard.com>
CC:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        Maxime Ripard <mripard@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Robert Foss <rfoss@kernel.org>,
        "Todor Tomov" <todor.too@gmail.com>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        "Jernej Skrabec" <jernej.skrabec@gmail.com>,
        Andrzej Pietrasiewicz <andrzejtp2010@gmail.com>,
        Hans Verkuil <hverkuil-cisco@xs4all.nl>,
        "Eugen Hristev" <eugen.hristev@collabora.com>,
        <linux-media@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <changhuang.liang@starfivetech.com>
References: <20230310120553.60586-1-jack.zhu@starfivetech.com>
 <20230310120553.60586-5-jack.zhu@starfivetech.com>
 <20230312113338.GC2545@pendragon.ideasonboard.com>
From:   Jack Zhu <jack.zhu@starfivetech.com>
In-Reply-To: <20230312113338.GC2545@pendragon.ideasonboard.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 2023/3/12 19:33, Laurent Pinchart wrote:
> Hi Jack,
> 
> Thank you for the patch.
> 
> On Fri, Mar 10, 2023 at 08:05:51PM +0800, Jack Zhu wrote:
>> Add support for external MIPI D-PHY and Starfive JH7110 SoC which
>> has the cadence csi2 receiver.
> 
> This patch bundles multiple changes that should be split into their own
> patches. I'll comment on that below.

Thank you for your suggestion and comments. I'll split the patch.

> 
>> Signed-off-by: Jack Zhu <jack.zhu@starfivetech.com>
>> ---
>>  drivers/media/platform/cadence/cdns-csi2rx.c | 273 ++++++++++++++++++-
>>  1 file changed, 263 insertions(+), 10 deletions(-)
>> 
>> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
>> index cc3ebb0d96f6..7e7b096869fc 100644
>> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
>> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
>> @@ -10,9 +10,11 @@
>>  #include <linux/io.h>
>>  #include <linux/module.h>
>>  #include <linux/of.h>
>> +#include <linux/of_device.h>
>>  #include <linux/of_graph.h>
>>  #include <linux/phy/phy.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/reset.h>
>>  #include <linux/slab.h>
>>  
>>  #include <media/v4l2-ctrls.h>
>> @@ -30,6 +32,12 @@
>>  #define CSI2RX_STATIC_CFG_DLANE_MAP(llane, plane)	((plane) << (16 + (llane) * 4))
>>  #define CSI2RX_STATIC_CFG_LANES_MASK			GENMASK(11, 8)
>>  
>> +#define CSI2RX_DPHY_LANE_CTRL_REG		0x40
>> +#define CSI2RX_DPHY_CL_RST			BIT(16)
>> +#define CSI2RX_DPHY_DL_RST(i)			BIT((i) + 12)
>> +#define CSI2RX_DPHY_CL_EN			BIT(4)
>> +#define CSI2RX_DPHY_DL_EN(i)			BIT(i)
>> +
>>  #define CSI2RX_STREAM_BASE(n)		(((n) + 1) * 0x100)
>>  
>>  #define CSI2RX_STREAM_CTRL_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x000)
>> @@ -37,6 +45,7 @@
>>  
>>  #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_EN_DATA_TYPE_0	BIT(7)
>>  #define CSI2RX_STREAM_DATA_CFG_VC_SELECT(n)		BIT((n) + 16)
>>  
>>  #define CSI2RX_STREAM_CFG_REG(n)		(CSI2RX_STREAM_BASE(n) + 0x00c)
>> @@ -54,8 +63,19 @@ enum csi2rx_pads {
>>  	CSI2RX_PAD_MAX,
>>  };
>>  
>> +struct csi2rx_fmt {
>> +	u32				code;
>> +	u8				bpp;
>> +	u32				dt;
>> +};
>> +
>> +struct csi2rx_platform_info {
>> +	unsigned long sys_clk_rate;
>> +};
>> +
>>  struct csi2rx_priv {
>>  	struct device			*dev;
>> +	unsigned int			power_count;
>>  	unsigned int			count;
>>  
>>  	/*
>> @@ -68,6 +88,9 @@ struct csi2rx_priv {
>>  	struct clk			*sys_clk;
>>  	struct clk			*p_clk;
>>  	struct clk			*pixel_clk[CSI2RX_STREAMS_MAX];
>> +	struct reset_control		*sys_rst;
>> +	struct reset_control		*p_rst;
>> +	struct reset_control		*pixel_rst[CSI2RX_STREAMS_MAX];
>>  	struct phy			*dphy;
>>  
>>  	u8				lanes[CSI2RX_LANES_MAX];
>> @@ -83,14 +106,100 @@ struct csi2rx_priv {
>>  	/* Remote source */
>>  	struct v4l2_subdev		*source_subdev;
>>  	int				source_pad;
>> +
>> +	const struct csi2rx_platform_info	*platform_info;
>> +};
>> +
>> +static const struct csi2rx_fmt formats[] = {
>> +	{
>> +		.code	= MEDIA_BUS_FMT_SRGGB10_1X10,
>> +		.bpp	= 10,
>> +		.dt	= 0x2b,
>> +	},
>> +	{
>> +		.code	= MEDIA_BUS_FMT_SGRBG10_1X10,
>> +		.bpp	= 10,
>> +		.dt	= 0x2b,
>> +	},
>> +	{
>> +		.code	= MEDIA_BUS_FMT_SGBRG10_1X10,
>> +		.bpp	= 10,
>> +		.dt	= 0x2b,
>> +	},
>> +	{
>> +		.code	= MEDIA_BUS_FMT_SBGGR10_1X10,
>> +		.bpp	= 10,
>> +		.dt	= 0x2b,
>> +	},
>>  };
>>  
>> +static u8 csi2rx_get_bpp(u32 code)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(formats); i++) {
>> +		if (formats[i].code == code)
>> +			return formats[i].bpp;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static u32 csi2rx_get_dt(u32 code)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(formats); i++) {
>> +		if (formats[i].code == code)
>> +			return formats[i].dt;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static s64 csi2rx_get_pixel_rate(struct csi2rx_priv *csi2rx)
>> +{
>> +	struct v4l2_ctrl *ctrl;
>> +
>> +	ctrl = v4l2_ctrl_find(csi2rx->source_subdev->ctrl_handler,
>> +			      V4L2_CID_PIXEL_RATE);
>> +	if (!ctrl) {
>> +		dev_err(csi2rx->dev, "no pixel rate control in subdev: %s\n",
>> +			csi2rx->source_subdev->name);
>> +		return -EINVAL;
>> +	}
>> +
>> +	return v4l2_ctrl_g_ctrl_int64(ctrl);
>> +}
>> +
>>  static inline
>>  struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev)
>>  {
>>  	return container_of(subdev, struct csi2rx_priv, subdev);
>>  }
>>  
>> +static int csi2rx_s_power(struct v4l2_subdev *subdev, int on)
> 
> .s_power() is deprecated, please drop it. You should use runtime PM
> instead (with autosuspend). In .s_stream(), you should call
> pm_runtime_resume_and_get() when starting streaming, and
> pm_runtime_put_autosuspend() when stopping streaming.
> 

OK, will drop .s_power(). The csi2rx will do not call phy_init()/
phy_exit(). The work of phy_init()/phy_exit() is integrated into PHY
driver.

>> +{
>> +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
>> +
>> +	mutex_lock(&csi2rx->lock);
>> +
>> +	if (on) {
>> +		if (!csi2rx->power_count)
>> +			phy_init(csi2rx->dphy);
>> +
>> +		csi2rx->power_count++;
>> +	} else {
>> +		csi2rx->power_count--;
>> +
>> +		if (!csi2rx->power_count)
>> +			phy_exit(csi2rx->dphy);
>> +	}
>> +
>> +	mutex_unlock(&csi2rx->lock);
>> +	return 0;
>> +}
>> +
>>  static void csi2rx_reset(struct csi2rx_priv *csi2rx)
>>  {
>>  	writel(CSI2RX_SOFT_RESET_PROTOCOL | CSI2RX_SOFT_RESET_FRONT,
>> @@ -101,17 +210,70 @@ static void csi2rx_reset(struct csi2rx_priv *csi2rx)
>>  	writel(0, csi2rx->base + CSI2RX_SOFT_RESET_REG);
>>  }
>>  
>> +static int csi2rx_configure_ext_dphy(struct csi2rx_priv *csi2rx)
> 
> Support for external DPHYs should be split to a patch of its own.

OK, will split the patch.

> 
>> +{
>> +	union phy_configure_opts opts = { };
>> +	struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy;
>> +	struct v4l2_subdev_format sd_fmt;
>> +	s64 pixel_rate;
>> +	int ret;
>> +	u8 bpp;
>> +
>> +	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>> +	sd_fmt.pad = 0;
>> +
>> +	ret = v4l2_subdev_call(csi2rx->source_subdev, pad, get_fmt, NULL,
>> +			       &sd_fmt);
> 
> Don't call .get_fmt() on the source. A subdev driver should only look at
> its own formats. As the format on the source pad of the source must
> match the format on the sink pad of this subdev when the pipeline is
> valid, you can use the format on the sink pad here.

OK, will drop. This step of work is not necessary.

> 
>> +	if (ret)
>> +		return ret;
>> +
>> +	bpp = csi2rx_get_bpp(sd_fmt.format.code);
>> +	if (!bpp)
>> +		return -EINVAL;
>> +
>> +	pixel_rate = csi2rx_get_pixel_rate(csi2rx);
>> +	if (pixel_rate < 0)
>> +		return pixel_rate;
>> +
>> +	ret = phy_mipi_dphy_get_default_config(pixel_rate, bpp,
>> +					       csi2rx->num_lanes, cfg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	phy_pm_runtime_get_sync(csi2rx->dphy);
>> +
>> +	ret = phy_power_on(csi2rx->dphy);
>> +	if (ret)
>> +		goto out;
>> +
>> +	ret = phy_configure(csi2rx->dphy, &opts);
>> +	if (ret) {
>> +		/* Can't do anything if it fails. Ignore the return value. */
>> +		phy_power_off(csi2rx->dphy);
>> +		goto out;
>> +	}
>> +
>> +out:
>> +	phy_pm_runtime_put_sync(csi2rx->dphy);
>> +
>> +	return ret;
>> +}
>> +
>>  static int csi2rx_start(struct csi2rx_priv *csi2rx)
>>  {
>> +	struct v4l2_subdev_format sd_fmt;
>>  	unsigned int i;
>>  	unsigned long lanes_used = 0;
>>  	u32 reg;
>> +	u32 dt = 0;
>>  	int ret;
>>  
>>  	ret = clk_prepare_enable(csi2rx->p_clk);
>>  	if (ret)
>>  		return ret;
>>  
>> +	reset_control_deassert(csi2rx->p_rst);
> 
> Support for reset controllers should also be split to a patch of its
> own.

OK, will split the patch according to logic change.

> 
>> +
>>  	csi2rx_reset(csi2rx);
>>  
>>  	reg = csi2rx->num_lanes << 8;
>> @@ -139,6 +301,29 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>>  	if (ret)
>>  		goto err_disable_pclk;
>>  
>> +	/* Enable DPHY clk and data lanes. */
>> +	if (csi2rx->dphy) {
>> +		reg = CSI2RX_DPHY_CL_EN | CSI2RX_DPHY_CL_RST;
>> +		for (i = 0; i < csi2rx->num_lanes; i++) {
>> +			reg |= CSI2RX_DPHY_DL_EN(csi2rx->lanes[i] - 1);
>> +			reg |= CSI2RX_DPHY_DL_RST(csi2rx->lanes[i] - 1);
>> +		}
>> +
>> +		writel(reg, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG);
>> +	}
>> +
>> +	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>> +	sd_fmt.pad = 0;
>> +
>> +	ret = v4l2_subdev_call(csi2rx->source_subdev, pad, get_fmt, NULL,
>> +			       &sd_fmt);
> 
> Same as above regarding the format.

OK, will fix.

> 
>> +	if (ret)
>> +		dev_warn(csi2rx->dev, "Couldn't get format\n");
>> +
>> +	dt = csi2rx_get_dt(sd_fmt.format.code);
>> +	if (!dt)
> 
> When you reach this point, the format should be guaranteed to be valid
> as .set_fmt() will have adjusted it. There should thus be no need to
> handle errors.
> 
>> +		dev_warn(csi2rx->dev, "Couldn't get dt\n");
>> +
>>  	/*
>>  	 * Create a static mapping between the CSI virtual channels
>>  	 * and the output stream.
>> @@ -154,6 +339,8 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>>  		if (ret)
>>  			goto err_disable_pixclk;
>>  
>> +		reset_control_deassert(csi2rx->pixel_rst[i]);
>> +
>>  		writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,
>>  		       csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
>>  
>> @@ -161,6 +348,11 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>>  		       CSI2RX_STREAM_DATA_CFG_VC_SELECT(i),
>>  		       csi2rx->base + CSI2RX_STREAM_DATA_CFG_REG(i));
>>  
>> +		if (dt)
>> +			writel(readl(csi2rx->base + CSI2RX_STREAM_DATA_CFG_REG(i)) |
>> +			       CSI2RX_STREAM_DATA_CFG_EN_DATA_TYPE_0 | dt,
>> +			       csi2rx->base + CSI2RX_STREAM_DATA_CFG_REG(i));
> 
> Support for CSI-2 DT should be split to a patch of its own.

CSI-2 DT is not necessary. I will drop the relevant code.

> 
>> +
>>  		writel(CSI2RX_STREAM_CTRL_START,
>>  		       csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
>>  	}
>> @@ -169,10 +361,27 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>>  	if (ret)
>>  		goto err_disable_pixclk;
>>  
>> +	if (csi2rx->platform_info && csi2rx->platform_info->sys_clk_rate > 0)
>> +		clk_set_rate(csi2rx->sys_clk,
>> +			     csi2rx->platform_info->sys_clk_rate);
>> +
>> +	reset_control_deassert(csi2rx->sys_rst);
>> +
>> +	if (csi2rx->dphy) {
>> +		ret = csi2rx_configure_ext_dphy(csi2rx);
>> +		if (ret) {
>> +			dev_err(csi2rx->dev,
>> +				"Failed to configure external DPHY: %d\n", ret);
>> +			goto err_disable_sysclk;
>> +		}
>> +	}
>> +
>>  	clk_disable_unprepare(csi2rx->p_clk);
>>  
>>  	return 0;
>>  
>> +err_disable_sysclk:
>> +	clk_disable_unprepare(csi2rx->sys_clk);
>>  err_disable_pixclk:
>>  	for (; i > 0; i--)
>>  		clk_disable_unprepare(csi2rx->pixel_clk[i - 1]);
>> @@ -188,18 +397,28 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
>>  	unsigned int i;
>>  
>>  	clk_prepare_enable(csi2rx->p_clk);
>> +	reset_control_assert(csi2rx->sys_rst);
>>  	clk_disable_unprepare(csi2rx->sys_clk);
>>  
>>  	for (i = 0; i < csi2rx->max_streams; i++) {
>>  		writel(0, csi2rx->base + CSI2RX_STREAM_CTRL_REG(i));
>>  
>> +		reset_control_assert(csi2rx->pixel_rst[i]);
>>  		clk_disable_unprepare(csi2rx->pixel_clk[i]);
>>  	}
>>  
>> +	reset_control_assert(csi2rx->p_rst);
>>  	clk_disable_unprepare(csi2rx->p_clk);
>>  
>>  	if (v4l2_subdev_call(csi2rx->source_subdev, video, s_stream, false))
>>  		dev_warn(csi2rx->dev, "Couldn't disable our subdev\n");
>> +
>> +	if (csi2rx->dphy) {
>> +		writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG);
>> +
>> +		if (phy_power_off(csi2rx->dphy))
>> +			dev_warn(csi2rx->dev, "Couldn't power off DPHY\n");
>> +	}
>>  }
>>  
>>  static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable)
>> @@ -236,11 +455,16 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable)
>>  	return ret;
>>  }
>>  
>> +static const struct v4l2_subdev_core_ops csi2rx_core_ops = {
>> +	.s_power = csi2rx_s_power,
>> +};
>> +
>>  static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
>>  	.s_stream	= csi2rx_s_stream,
>>  };
>>  
>>  static const struct v4l2_subdev_ops csi2rx_subdev_ops = {
>> +	.core		= &csi2rx_core_ops,
>>  	.video		= &csi2rx_video_ops,
>>  };
>>  
>> @@ -250,6 +474,8 @@ static int csi2rx_async_bound(struct v4l2_async_notifier *notifier,
>>  {
>>  	struct v4l2_subdev *subdev = notifier->sd;
>>  	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
>> +	struct v4l2_device *v4l2_dev;
>> +	int ret;
>>  
>>  	csi2rx->source_pad = media_entity_get_fwnode_pad(&s_subdev->entity,
>>  							 s_subdev->fwnode,
>> @@ -265,6 +491,15 @@ static int csi2rx_async_bound(struct v4l2_async_notifier *notifier,
>>  	dev_dbg(csi2rx->dev, "Bound %s pad: %d\n", s_subdev->name,
>>  		csi2rx->source_pad);
>>  
>> +	/* ensure source subdev register subdev node */
>> +	v4l2_dev = notifier->v4l2_dev ? notifier->v4l2_dev :
>> +		notifier->parent->v4l2_dev;
>> +	if (v4l2_dev) {
>> +		ret = v4l2_device_register_subdev_nodes(v4l2_dev);
> 
> This belongs to the main driver (the camss in your case), not this
> driver.

OK, will fix.

> 
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>>  	return media_create_pad_link(&csi2rx->source_subdev->entity,
>>  				     csi2rx->source_pad,
>>  				     &csi2rx->subdev.entity, 0,
>> @@ -299,21 +534,23 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx,
>>  		return PTR_ERR(csi2rx->p_clk);
>>  	}
>>  
>> +	csi2rx->sys_rst =
>> +		devm_reset_control_get_optional_exclusive(&pdev->dev,
>> +							  "sys_rst");
>> +	if (IS_ERR(csi2rx->sys_rst))
>> +		return PTR_ERR(csi2rx->sys_rst);
>> +
>> +	csi2rx->p_rst =
>> +		devm_reset_control_get_optional_exclusive(&pdev->dev, "p_rst");
>> +	if (IS_ERR(csi2rx->p_rst))
>> +		return PTR_ERR(csi2rx->p_rst);
>> +
>>  	csi2rx->dphy = devm_phy_optional_get(&pdev->dev, "dphy");
>>  	if (IS_ERR(csi2rx->dphy)) {
>>  		dev_err(&pdev->dev, "Couldn't get external D-PHY\n");
>>  		return PTR_ERR(csi2rx->dphy);
>>  	}
>>  
>> -	/*
>> -	 * FIXME: Once we'll have external D-PHY support, the check
>> -	 * will need to be removed.
>> -	 */
>> -	if (csi2rx->dphy) {
>> -		dev_err(&pdev->dev, "External D-PHY not supported yet\n");
>> -		return -EINVAL;
>> -	}
>> -
>>  	ret = clk_prepare_enable(csi2rx->p_clk);
>>  	if (ret) {
>>  		dev_err(&pdev->dev, "Couldn't prepare and enable P clock\n");
>> @@ -343,13 +580,14 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx,
>>  	 * FIXME: Once we'll have internal D-PHY support, the check
>>  	 * will need to be removed.
>>  	 */
>> -	if (csi2rx->has_internal_dphy) {
>> +	if (!csi2rx->dphy && csi2rx->has_internal_dphy) {
>>  		dev_err(&pdev->dev, "Internal D-PHY not supported yet\n");
>>  		return -EINVAL;
>>  	}
>>  
>>  	for (i = 0; i < csi2rx->max_streams; i++) {
>>  		char clk_name[16];
>> +		char rst_name[16];
>>  
>>  		snprintf(clk_name, sizeof(clk_name), "pixel_if%u_clk", i);
>>  		csi2rx->pixel_clk[i] = devm_clk_get(&pdev->dev, clk_name);
>> @@ -357,6 +595,13 @@ static int csi2rx_get_resources(struct csi2rx_priv *csi2rx,
>>  			dev_err(&pdev->dev, "Couldn't get clock %s\n", clk_name);
>>  			return PTR_ERR(csi2rx->pixel_clk[i]);
>>  		}
>> +
>> +		snprintf(rst_name, sizeof(rst_name), "pixel_if%u_rst", i);
>> +		csi2rx->pixel_rst[i] =
>> +			devm_reset_control_get_optional_exclusive(&pdev->dev,
>> +								  rst_name);
>> +		if (IS_ERR(csi2rx->pixel_rst[i]))
>> +			return PTR_ERR(csi2rx->pixel_rst[i]);
>>  	}
>>  
>>  	return 0;
>> @@ -425,6 +670,7 @@ static int csi2rx_probe(struct platform_device *pdev)
>>  	csi2rx = kzalloc(sizeof(*csi2rx), GFP_KERNEL);
>>  	if (!csi2rx)
>>  		return -ENOMEM;
>> +	csi2rx->platform_info = of_device_get_match_data(&pdev->dev);
>>  	platform_set_drvdata(pdev, csi2rx);
>>  	csi2rx->dev = &pdev->dev;
>>  	mutex_init(&csi2rx->lock);
>> @@ -441,6 +687,7 @@ static int csi2rx_probe(struct platform_device *pdev)
>>  	csi2rx->subdev.dev = &pdev->dev;
>>  	v4l2_subdev_init(&csi2rx->subdev, &csi2rx_subdev_ops);
>>  	v4l2_set_subdevdata(&csi2rx->subdev, &pdev->dev);
>> +	csi2rx->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>>  	snprintf(csi2rx->subdev.name, V4L2_SUBDEV_NAME_SIZE, "%s.%s",
>>  		 KBUILD_MODNAME, dev_name(&pdev->dev));
>>  
>> @@ -462,6 +709,7 @@ static int csi2rx_probe(struct platform_device *pdev)
>>  	dev_info(&pdev->dev,
>>  		 "Probed CSI2RX with %u/%u lanes, %u streams, %s D-PHY\n",
>>  		 csi2rx->num_lanes, csi2rx->max_lanes, csi2rx->max_streams,
>> +		 csi2rx->dphy ? "external" :
>>  		 csi2rx->has_internal_dphy ? "internal" : "no");
>>  
>>  	return 0;
>> @@ -483,8 +731,13 @@ static int csi2rx_remove(struct platform_device *pdev)
>>  	return 0;
>>  }
>>  
>> +static const struct csi2rx_platform_info stf_jh7110_info = {
>> +	.sys_clk_rate = 297000000,
> 
> This sounds like something that would be better handled through the
> assigned-clock-rates property in DT.

OK, will use assigned-clock-rates property in DT.

> 
>> +};
>> +
>>  static const struct of_device_id csi2rx_of_table[] = {
>>  	{ .compatible = "cdns,csi2rx" },
>> +	{ .compatible = "starfive,jh7110-csi2rx", .data = &stf_jh7110_info },
> 
> I would move SoC-specific compatible strings before the generic one.

OK, will move it before the generic one.

> Maxime, is there a need to keep the generic compatible string now that
> we have SoC-specific support ?

The existing bindings state

  - compatible: must be set to "cdns,csi2rx" and an SoC-specific compatible

It might be better to keep the generic compatible string.

> 
>>  	{ },
>>  };
>>  MODULE_DEVICE_TABLE(of, csi2rx_of_table);
>