[PATCH v2 1/3] ARM: dts: imx6ul: Add csi node

[PATCH v2 1/3] ARM: dts: imx6ul: Add csi node

[Sébastien Szymanski]

Add csi node for i.MX6UL SoC.

Reviewed-by: Fabio Estevam <festevam@gmail.com>
Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
---

Changes for v2:
 - only "mclk" clock is required now.

 arch/arm/boot/dts/imx6ul.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/imx6ul.dtsi b/arch/arm/boot/dts/imx6ul.dtsi
index bbf010c73336..f10012de5eb6 100644
--- a/arch/arm/boot/dts/imx6ul.dtsi
+++ b/arch/arm/boot/dts/imx6ul.dtsi
@@ -951,6 +951,15 @@
 				};
 			};
 
+			csi: csi@21c4000 {
+				compatible = "fsl,imx6ul-csi", "fsl,imx7-csi";
+				reg = <0x021c4000 0x4000>;
+				interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
+				clocks = <&clks IMX6UL_CLK_CSI>;
+				clock-names = "mclk";
+				status = "disabled";
+			};
+
 			lcdif: lcdif@21c8000 {
 				compatible = "fsl,imx6ul-lcdif", "fsl,imx28-lcdif";
 				reg = <0x021c8000 0x4000>;
-- 
2.19.2

[PATCH v2 2/3] media: imx7-media-csi: add i.MX6UL support

[Sébastien Szymanski]

i.MX7 and i.MX6UL/L have the same CSI controller. So add i.MX6UL/L support
to imx7-media-csi driver.

Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
---

Changes for v2:
 - rebase on top of linuxtv/master
 - mention i.MX6UL/L in header and Kconfig help text
 - rename csi_type to csi_soc_id

 drivers/staging/media/imx/Kconfig          |  4 +-
 drivers/staging/media/imx/imx7-media-csi.c | 62 ++++++++++++++++------
 2 files changed, 49 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/media/imx/Kconfig b/drivers/staging/media/imx/Kconfig
index ad3d7df6bb3c..8b6dc42c39e0 100644
--- a/drivers/staging/media/imx/Kconfig
+++ b/drivers/staging/media/imx/Kconfig
@@ -22,11 +22,11 @@ config VIDEO_IMX_CSI
 	  A video4linux camera sensor interface driver for i.MX5/6.
 
 config VIDEO_IMX7_CSI
-	tristate "i.MX7 Camera Sensor Interface driver"
+	tristate "i.MX6UL/L / i.MX7 Camera Sensor Interface driver"
 	depends on VIDEO_IMX_MEDIA && VIDEO_DEV && I2C
 	default y
 	help
 	  Enable support for video4linux camera sensor interface driver for
-	  i.MX7.
+	  i.MX6UL/L or i.MX7.
 endmenu
 endif
diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
index 9101566f3f67..902bdce594cf 100644
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * V4L2 Capture CSI Subdev for Freescale i.MX7 SOC
+ * V4L2 Capture CSI Subdev for Freescale i.MX6UL/L / i.MX7 SOC
  *
  * Copyright (c) 2019 Linaro Ltd
  *
@@ -152,6 +152,11 @@
 #define CSI_CSICR18		0x48
 #define CSI_CSICR19		0x4c
 
+enum csi_soc_id {
+	IMX7,
+	IMX6UL
+};
+
 struct imx7_csi {
 	struct device *dev;
 	struct v4l2_subdev sd;
@@ -191,6 +196,7 @@ struct imx7_csi {
 	bool is_init;
 	bool is_streaming;
 	bool is_csi2;
+	enum csi_soc_id soc_id;
 
 	struct completion last_eof_completion;
 };
@@ -548,6 +554,14 @@ static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
 	if (ret)
 		return ret;
 
+	if (csi->soc_id == IMX6UL) {
+		mutex_lock(&csi->lock);
+		csi->is_csi2 = false;
+		mutex_unlock(&csi->lock);
+
+		return 0;
+	}
+
 	ret = imx7_csi_get_upstream_endpoint(csi, &upstream_ep, true);
 	if (ret) {
 		v4l2_err(&csi->sd, "failed to find upstream endpoint\n");
@@ -757,6 +771,7 @@ static int imx7_csi_configure(struct imx7_csi *csi)
 	struct v4l2_pix_format *out_pix = &vdev->fmt.fmt.pix;
 	__u32 in_code = csi->format_mbus[IMX7_CSI_PAD_SINK].code;
 	u32 cr1, cr18;
+	int width = out_pix->width;
 
 	if (out_pix->field == V4L2_FIELD_INTERLACED) {
 		imx7_csi_deinterlace_enable(csi, true);
@@ -766,15 +781,27 @@ static int imx7_csi_configure(struct imx7_csi *csi)
 		imx7_csi_buf_stride_set(csi, 0);
 	}
 
-	imx7_csi_set_imagpara(csi, out_pix->width, out_pix->height);
+	cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
+
+	if (!csi->is_csi2) {
+		if (out_pix->pixelformat == V4L2_PIX_FMT_UYVY ||
+		    out_pix->pixelformat == V4L2_PIX_FMT_YUYV)
+			width *= 2;
+
+		imx7_csi_set_imagpara(csi, width, out_pix->height);
+
+		cr18 |= (BIT_BASEADDR_SWITCH_EN | BIT_BASEADDR_SWITCH_SEL |
+			BIT_BASEADDR_CHG_ERR_EN);
+		imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
 
-	if (!csi->is_csi2)
 		return 0;
+	}
+
+	imx7_csi_set_imagpara(csi, width, out_pix->height);
 
 	cr1 = imx7_csi_reg_read(csi, CSI_CSICR1);
 	cr1 &= ~BIT_GCLK_MODE;
 
-	cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
 	cr18 &= BIT_MIPI_DATA_FORMAT_MASK;
 	cr18 |= BIT_DATA_FROM_MIPI;
 
@@ -809,11 +836,9 @@ static void imx7_csi_enable(struct imx7_csi *csi)
 {
 	imx7_csi_sw_reset(csi);
 
-	if (csi->is_csi2) {
-		imx7_csi_dmareq_rff_enable(csi);
-		imx7_csi_hw_enable_irq(csi);
-		imx7_csi_hw_enable(csi);
-	}
+	imx7_csi_dmareq_rff_enable(csi);
+	imx7_csi_hw_enable_irq(csi);
+	imx7_csi_hw_enable(csi);
 }
 
 static void imx7_csi_disable(struct imx7_csi *csi)
@@ -1166,19 +1191,32 @@ static int imx7_csi_parse_endpoint(struct device *dev,
 	return fwnode_device_is_available(asd->match.fwnode) ? 0 : -EINVAL;
 }
 
+static const struct of_device_id imx7_csi_of_match[] = {
+	{ .compatible = "fsl,imx7-csi", .data = (void *)IMX7 },
+	{ .compatible = "fsl,imx6ul-csi", .data = (void *)IMX6UL },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, imx7_csi_of_match);
+
 static int imx7_csi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *node = dev->of_node;
 	struct imx_media_dev *imxmd;
 	struct imx7_csi *csi;
+	const struct of_device_id *of_id;
 	int ret;
 
+	of_id = of_match_node(imx7_csi_of_match, node);
+	if (!of_id)
+		return -ENODEV;
+
 	csi = devm_kzalloc(&pdev->dev, sizeof(*csi), GFP_KERNEL);
 	if (!csi)
 		return -ENOMEM;
 
 	csi->dev = dev;
+	csi->soc_id = (enum csi_soc_id)of_id->data;
 
 	csi->mclk = devm_clk_get(&pdev->dev, "mclk");
 	if (IS_ERR(csi->mclk)) {
@@ -1294,12 +1332,6 @@ static int imx7_csi_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id imx7_csi_of_match[] = {
-	{ .compatible = "fsl,imx7-csi" },
-	{ },
-};
-MODULE_DEVICE_TABLE(of, imx7_csi_of_match);
-
 static struct platform_driver imx7_csi_driver = {
 	.probe = imx7_csi_probe,
 	.remove = imx7_csi_remove,
-- 
2.19.2

[PATCH v2 3/3] media: dt-bindings: imx7-csi: add i.MX6UL/L support

[Sébastien Szymanski]

Document "fsl,imx6ul-csi" entry.

Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
---

Changes for v2:
 - New patch to document new "fsl,imx6ul-csi" entry.

 Documentation/devicetree/bindings/media/imx7-csi.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/imx7-csi.txt b/Documentation/devicetree/bindings/media/imx7-csi.txt
index 3c07bc676bc3..49e9af19b3ea 100644
--- a/Documentation/devicetree/bindings/media/imx7-csi.txt
+++ b/Documentation/devicetree/bindings/media/imx7-csi.txt
@@ -9,7 +9,7 @@ to connect directly to external CMOS image sensors.
 
 Required properties:
 
-- compatible    : "fsl,imx7-csi";
+- compatible    : "fsl,imx7-csi" or "fsl,imx6ul-csi";
 - reg           : base address and length of the register set for the device;
 - interrupts    : should contain CSI interrupt;
 - clocks        : list of clock specifiers, see
-- 
2.19.2

Re: [PATCH v2 2/3] media: imx7-media-csi: add i.MX6UL support

[Randy Dunlap]

On 6/6/19 8:38 AM, Sébastien Szymanski wrote:
> i.MX7 and i.MX6UL/L have the same CSI controller. So add i.MX6UL/L support
> to imx7-media-csi driver.
> 
> Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
> ---
> 
> Changes for v2:
>  - rebase on top of linuxtv/master
>  - mention i.MX6UL/L in header and Kconfig help text
>  - rename csi_type to csi_soc_id
> 
>  drivers/staging/media/imx/Kconfig          |  4 +-
>  drivers/staging/media/imx/imx7-media-csi.c | 62 ++++++++++++++++------
>  2 files changed, 49 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/Kconfig b/drivers/staging/media/imx/Kconfig
> index ad3d7df6bb3c..8b6dc42c39e0 100644
> --- a/drivers/staging/media/imx/Kconfig
> +++ b/drivers/staging/media/imx/Kconfig
> @@ -22,11 +22,11 @@ config VIDEO_IMX_CSI
>  	  A video4linux camera sensor interface driver for i.MX5/6.
>  
>  config VIDEO_IMX7_CSI
> -	tristate "i.MX7 Camera Sensor Interface driver"
> +	tristate "i.MX6UL/L / i.MX7 Camera Sensor Interface driver"
>  	depends on VIDEO_IMX_MEDIA && VIDEO_DEV && I2C
>  	default y

Hi,
I realize that this "default y" is not part of this patch set, but we have
pretty strong guidance that a driver should not default to 'y' unless it is
needed for a system to boot.  If this driver is optional, then please drop
the 2 occurrences of "default y" in this Kconfig file.

thanks.
>  	help
>  	  Enable support for video4linux camera sensor interface driver for
> -	  i.MX7.
> +	  i.MX6UL/L or i.MX7.
>  endmenu
>  endif


-- 
~Randy

Re: [PATCH v2 2/3] media: imx7-media-csi: add i.MX6UL support

[Rui Miguel Silva]

Hi Sebastien,
Thanks for the patch.

On Thu 06 Jun 2019 at 16:38, Sébastien Szymanski wrote:
> i.MX7 and i.MX6UL/L have the same CSI controller. So add i.MX6UL/L support
> to imx7-media-csi driver.
>
> Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
> ---
>
> Changes for v2:
>  - rebase on top of linuxtv/master
>  - mention i.MX6UL/L in header and Kconfig help text
>  - rename csi_type to csi_soc_id
>
>  drivers/staging/media/imx/Kconfig          |  4 +-
>  drivers/staging/media/imx/imx7-media-csi.c | 62 ++++++++++++++++------
>  2 files changed, 49 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/staging/media/imx/Kconfig b/drivers/staging/media/imx/Kconfig
> index ad3d7df6bb3c..8b6dc42c39e0 100644
> --- a/drivers/staging/media/imx/Kconfig
> +++ b/drivers/staging/media/imx/Kconfig
> @@ -22,11 +22,11 @@ config VIDEO_IMX_CSI
>  	  A video4linux camera sensor interface driver for i.MX5/6.
>
>  config VIDEO_IMX7_CSI
> -	tristate "i.MX7 Camera Sensor Interface driver"
> +	tristate "i.MX6UL/L / i.MX7 Camera Sensor Interface driver"
>  	depends on VIDEO_IMX_MEDIA && VIDEO_DEV && I2C
>  	default y
>  	help
>  	  Enable support for video4linux camera sensor interface driver for
> -	  i.MX7.
> +	  i.MX6UL/L or i.MX7.
>  endmenu
>  endif
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index 9101566f3f67..902bdce594cf 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
> - * V4L2 Capture CSI Subdev for Freescale i.MX7 SOC
> + * V4L2 Capture CSI Subdev for Freescale i.MX6UL/L / i.MX7 SOC
>   *
>   * Copyright (c) 2019 Linaro Ltd
>   *
> @@ -152,6 +152,11 @@
>  #define CSI_CSICR18		0x48
>  #define CSI_CSICR19		0x4c
>
> +enum csi_soc_id {
> +	IMX7,
> +	IMX6UL
> +};
> +
>  struct imx7_csi {
>  	struct device *dev;
>  	struct v4l2_subdev sd;
> @@ -191,6 +196,7 @@ struct imx7_csi {
>  	bool is_init;
>  	bool is_streaming;
>  	bool is_csi2;
> +	enum csi_soc_id soc_id;
>
>  	struct completion last_eof_completion;
>  };
> @@ -548,6 +554,14 @@ static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
>  	if (ret)
>  		return ret;
>
> +	if (csi->soc_id == IMX6UL) {
> +		mutex_lock(&csi->lock);
> +		csi->is_csi2 = false;
> +		mutex_unlock(&csi->lock);
> +
> +		return 0;
> +	}
> +
>  	ret = imx7_csi_get_upstream_endpoint(csi, &upstream_ep, true);
>  	if (ret) {
>  		v4l2_err(&csi->sd, "failed to find upstream endpoint\n");
> @@ -757,6 +771,7 @@ static int imx7_csi_configure(struct imx7_csi *csi)
>  	struct v4l2_pix_format *out_pix = &vdev->fmt.fmt.pix;
>  	__u32 in_code = csi->format_mbus[IMX7_CSI_PAD_SINK].code;
>  	u32 cr1, cr18;
> +	int width = out_pix->width;
>
>  	if (out_pix->field == V4L2_FIELD_INTERLACED) {
>  		imx7_csi_deinterlace_enable(csi, true);
> @@ -766,15 +781,27 @@ static int imx7_csi_configure(struct imx7_csi *csi)
>  		imx7_csi_buf_stride_set(csi, 0);
>  	}
>
> -	imx7_csi_set_imagpara(csi, out_pix->width, out_pix->height);
> +	cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
> +
> +	if (!csi->is_csi2) {
> +		if (out_pix->pixelformat == V4L2_PIX_FMT_UYVY ||
> +		    out_pix->pixelformat == V4L2_PIX_FMT_YUYV)
> +			width *= 2;
> +
> +		imx7_csi_set_imagpara(csi, width, out_pix->height);
> +
> +		cr18 |= (BIT_BASEADDR_SWITCH_EN | BIT_BASEADDR_SWITCH_SEL |
> +			BIT_BASEADDR_CHG_ERR_EN);
> +		imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
>
> -	if (!csi->is_csi2)
>  		return 0;
> +	}
> +
> +	imx7_csi_set_imagpara(csi, width, out_pix->height);
>
>  	cr1 = imx7_csi_reg_read(csi, CSI_CSICR1);
>  	cr1 &= ~BIT_GCLK_MODE;
>
> -	cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
>  	cr18 &= BIT_MIPI_DATA_FORMAT_MASK;
>  	cr18 |= BIT_DATA_FROM_MIPI;
>
> @@ -809,11 +836,9 @@ static void imx7_csi_enable(struct imx7_csi *csi)
>  {
>  	imx7_csi_sw_reset(csi);
>
> -	if (csi->is_csi2) {
> -		imx7_csi_dmareq_rff_enable(csi);
> -		imx7_csi_hw_enable_irq(csi);
> -		imx7_csi_hw_enable(csi);
> -	}
> +	imx7_csi_dmareq_rff_enable(csi);
> +	imx7_csi_hw_enable_irq(csi);
> +	imx7_csi_hw_enable(csi);
>  }
>
>  static void imx7_csi_disable(struct imx7_csi *csi)
> @@ -1166,19 +1191,32 @@ static int imx7_csi_parse_endpoint(struct device *dev,
>  	return fwnode_device_is_available(asd->match.fwnode) ? 0 : -EINVAL;
>  }
>
> +static const struct of_device_id imx7_csi_of_match[] = {
> +	{ .compatible = "fsl,imx7-csi", .data = (void *)IMX7 },
> +	{ .compatible = "fsl,imx6ul-csi", .data = (void *)IMX6UL },

looking at this again I think we can do this is a different way.
Instead data being the soc_id, just set here if it is_csi2 or not.

This would avoid to add a soc_id  to the struct that it really it
is used only to setup the is_csi2 var. I think this will make this
patch a lot simpler.

> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, imx7_csi_of_match);
> +
>  static int imx7_csi_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct device_node *node = dev->of_node;
>  	struct imx_media_dev *imxmd;
>  	struct imx7_csi *csi;
> +	const struct of_device_id *of_id;
>  	int ret;
>
> +	of_id = of_match_node(imx7_csi_of_match, node);

With the above said, here I think we can use the of_match_device?

hope this makes sense also to you.

Once again thanks for the patches.

---
Cheers,
        Rui

> +	if (!of_id)
> +		return -ENODEV;
> +
>  	csi = devm_kzalloc(&pdev->dev, sizeof(*csi), GFP_KERNEL);
>  	if (!csi)
>  		return -ENOMEM;
>
>  	csi->dev = dev;
> +	csi->soc_id = (enum csi_soc_id)of_id->data;
>
>  	csi->mclk = devm_clk_get(&pdev->dev, "mclk");
>  	if (IS_ERR(csi->mclk)) {
> @@ -1294,12 +1332,6 @@ static int imx7_csi_remove(struct platform_device *pdev)
>  	return 0;
>  }
>
> -static const struct of_device_id imx7_csi_of_match[] = {
> -	{ .compatible = "fsl,imx7-csi" },
> -	{ },
> -};
> -MODULE_DEVICE_TABLE(of, imx7_csi_of_match);
> -
>  static struct platform_driver imx7_csi_driver = {
>  	.probe = imx7_csi_probe,
>  	.remove = imx7_csi_remove,

Re: [PATCH v2 2/3] media: imx7-media-csi: add i.MX6UL support

[Rui Miguel Silva]

Hi Randy,
On Fri 07 Jun 2019 at 00:10, Randy Dunlap wrote:
> On 6/6/19 8:38 AM, Sébastien Szymanski wrote:
>> i.MX7 and i.MX6UL/L have the same CSI controller. So add i.MX6UL/L support
>> to imx7-media-csi driver.
>>
>> Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
>> ---
>>
>> Changes for v2:
>>  - rebase on top of linuxtv/master
>>  - mention i.MX6UL/L in header and Kconfig help text
>>  - rename csi_type to csi_soc_id
>>
>>  drivers/staging/media/imx/Kconfig          |  4 +-
>>  drivers/staging/media/imx/imx7-media-csi.c | 62 ++++++++++++++++------
>>  2 files changed, 49 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/staging/media/imx/Kconfig b/drivers/staging/media/imx/Kconfig
>> index ad3d7df6bb3c..8b6dc42c39e0 100644
>> --- a/drivers/staging/media/imx/Kconfig
>> +++ b/drivers/staging/media/imx/Kconfig
>> @@ -22,11 +22,11 @@ config VIDEO_IMX_CSI
>>  	  A video4linux camera sensor interface driver for i.MX5/6.
>>
>>  config VIDEO_IMX7_CSI
>> -	tristate "i.MX7 Camera Sensor Interface driver"
>> +	tristate "i.MX6UL/L / i.MX7 Camera Sensor Interface driver"
>>  	depends on VIDEO_IMX_MEDIA && VIDEO_DEV && I2C
>>  	default y
>
> Hi,
> I realize that this "default y" is not part of this patch set, but we have
> pretty strong guidance that a driver should not default to 'y' unless it is
> needed for a system to boot.  If this driver is optional, then please drop
> the 2 occurrences of "default y" in this Kconfig file.

Yeah, even though both depends on imx_media, I agree that they
should not default to y. I will send a patch for this.
Thanks.

---
Cheers,
        Rui


>
> thanks.
>>  	help
>>  	  Enable support for video4linux camera sensor interface driver for
>> -	  i.MX7.
>> +	  i.MX6UL/L or i.MX7.
>>  endmenu
>>  endif

Re: [PATCH v2 2/3] media: imx7-media-csi: add i.MX6UL support

[Sébastien Szymanski]

Hi Rui,

thanks for the review!

On 6/10/19 12:28 PM, Rui Miguel Silva wrote:
> Hi Sebastien,
> Thanks for the patch.
> 
> On Thu 06 Jun 2019 at 16:38, Sébastien Szymanski wrote:
>> i.MX7 and i.MX6UL/L have the same CSI controller. So add i.MX6UL/L support
>> to imx7-media-csi driver.
>>
>> Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
>> ---
>>
>> Changes for v2:
>>  - rebase on top of linuxtv/master
>>  - mention i.MX6UL/L in header and Kconfig help text
>>  - rename csi_type to csi_soc_id
>>
>>  drivers/staging/media/imx/Kconfig          |  4 +-
>>  drivers/staging/media/imx/imx7-media-csi.c | 62 ++++++++++++++++------
>>  2 files changed, 49 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/staging/media/imx/Kconfig b/drivers/staging/media/imx/Kconfig
>> index ad3d7df6bb3c..8b6dc42c39e0 100644
>> --- a/drivers/staging/media/imx/Kconfig
>> +++ b/drivers/staging/media/imx/Kconfig
>> @@ -22,11 +22,11 @@ config VIDEO_IMX_CSI
>>  	  A video4linux camera sensor interface driver for i.MX5/6.
>>
>>  config VIDEO_IMX7_CSI
>> -	tristate "i.MX7 Camera Sensor Interface driver"
>> +	tristate "i.MX6UL/L / i.MX7 Camera Sensor Interface driver"
>>  	depends on VIDEO_IMX_MEDIA && VIDEO_DEV && I2C
>>  	default y
>>  	help
>>  	  Enable support for video4linux camera sensor interface driver for
>> -	  i.MX7.
>> +	  i.MX6UL/L or i.MX7.
>>  endmenu
>>  endif
>> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
>> index 9101566f3f67..902bdce594cf 100644
>> --- a/drivers/staging/media/imx/imx7-media-csi.c
>> +++ b/drivers/staging/media/imx/imx7-media-csi.c
>> @@ -1,6 +1,6 @@
>>  // SPDX-License-Identifier: GPL-2.0
>>  /*
>> - * V4L2 Capture CSI Subdev for Freescale i.MX7 SOC
>> + * V4L2 Capture CSI Subdev for Freescale i.MX6UL/L / i.MX7 SOC
>>   *
>>   * Copyright (c) 2019 Linaro Ltd
>>   *
>> @@ -152,6 +152,11 @@
>>  #define CSI_CSICR18		0x48
>>  #define CSI_CSICR19		0x4c
>>
>> +enum csi_soc_id {
>> +	IMX7,
>> +	IMX6UL
>> +};
>> +
>>  struct imx7_csi {
>>  	struct device *dev;
>>  	struct v4l2_subdev sd;
>> @@ -191,6 +196,7 @@ struct imx7_csi {
>>  	bool is_init;
>>  	bool is_streaming;
>>  	bool is_csi2;
>> +	enum csi_soc_id soc_id;
>>
>>  	struct completion last_eof_completion;
>>  };
>> @@ -548,6 +554,14 @@ static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
>>  	if (ret)
>>  		return ret;
>>
>> +	if (csi->soc_id == IMX6UL) {
>> +		mutex_lock(&csi->lock);
>> +		csi->is_csi2 = false;
>> +		mutex_unlock(&csi->lock);
>> +
>> +		return 0;
>> +	}
>> +
>>  	ret = imx7_csi_get_upstream_endpoint(csi, &upstream_ep, true);
>>  	if (ret) {
>>  		v4l2_err(&csi->sd, "failed to find upstream endpoint\n");
>> @@ -757,6 +771,7 @@ static int imx7_csi_configure(struct imx7_csi *csi)
>>  	struct v4l2_pix_format *out_pix = &vdev->fmt.fmt.pix;
>>  	__u32 in_code = csi->format_mbus[IMX7_CSI_PAD_SINK].code;
>>  	u32 cr1, cr18;
>> +	int width = out_pix->width;
>>
>>  	if (out_pix->field == V4L2_FIELD_INTERLACED) {
>>  		imx7_csi_deinterlace_enable(csi, true);
>> @@ -766,15 +781,27 @@ static int imx7_csi_configure(struct imx7_csi *csi)
>>  		imx7_csi_buf_stride_set(csi, 0);
>>  	}
>>
>> -	imx7_csi_set_imagpara(csi, out_pix->width, out_pix->height);
>> +	cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
>> +
>> +	if (!csi->is_csi2) {
>> +		if (out_pix->pixelformat == V4L2_PIX_FMT_UYVY ||
>> +		    out_pix->pixelformat == V4L2_PIX_FMT_YUYV)
>> +			width *= 2;
>> +
>> +		imx7_csi_set_imagpara(csi, width, out_pix->height);
>> +
>> +		cr18 |= (BIT_BASEADDR_SWITCH_EN | BIT_BASEADDR_SWITCH_SEL |
>> +			BIT_BASEADDR_CHG_ERR_EN);
>> +		imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
>>
>> -	if (!csi->is_csi2)
>>  		return 0;
>> +	}
>> +
>> +	imx7_csi_set_imagpara(csi, width, out_pix->height);
>>
>>  	cr1 = imx7_csi_reg_read(csi, CSI_CSICR1);
>>  	cr1 &= ~BIT_GCLK_MODE;
>>
>> -	cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
>>  	cr18 &= BIT_MIPI_DATA_FORMAT_MASK;
>>  	cr18 |= BIT_DATA_FROM_MIPI;
>>
>> @@ -809,11 +836,9 @@ static void imx7_csi_enable(struct imx7_csi *csi)
>>  {
>>  	imx7_csi_sw_reset(csi);
>>
>> -	if (csi->is_csi2) {
>> -		imx7_csi_dmareq_rff_enable(csi);
>> -		imx7_csi_hw_enable_irq(csi);
>> -		imx7_csi_hw_enable(csi);
>> -	}
>> +	imx7_csi_dmareq_rff_enable(csi);
>> +	imx7_csi_hw_enable_irq(csi);
>> +	imx7_csi_hw_enable(csi);
>>  }
>>
>>  static void imx7_csi_disable(struct imx7_csi *csi)
>> @@ -1166,19 +1191,32 @@ static int imx7_csi_parse_endpoint(struct device *dev,
>>  	return fwnode_device_is_available(asd->match.fwnode) ? 0 : -EINVAL;
>>  }
>>
>> +static const struct of_device_id imx7_csi_of_match[] = {
>> +	{ .compatible = "fsl,imx7-csi", .data = (void *)IMX7 },
>> +	{ .compatible = "fsl,imx6ul-csi", .data = (void *)IMX6UL },
> 
> looking at this again I think we can do this is a different way.
> Instead data being the soc_id, just set here if it is_csi2 or not.
> 
> This would avoid to add a soc_id  to the struct that it really it
> is used only to setup the is_csi2 var. I think this will make this
> patch a lot simpler.

Well, I have added this soc_id because imx7_csi_get_upstream_endpoint in
imx7_csi_pad_link_validate fails:

[  366.549768] csi: failed to find upstream endpoint
[  366.556274] csi: pipeline start failed with -19

My pipeline is:

Device topology
- entity 1: csi (2 pads, 2 links)
            type V4L2 subdev subtype Unknown flags 0
            device node name /dev/v4l-subdev0
        pad0: Sink
                [fmt:UYVY8_2X8/640x480 field:none colorspace:srgb
xfer:srgb ycbcr:601 quantization:full-range]
                <- "ov5640 1-003c":0 [ENABLED]
        pad1: Source
                [fmt:UYVY8_2X8/640x480 field:none colorspace:srgb
xfer:srgb ycbcr:601 quantization:full-range]
                -> "csi capture":0 [ENABLED]

- entity 4: csi capture (1 pad, 1 link)
            type Node subtype V4L flags 0
            device node name /dev/video1
        pad0: Sink
                <- "csi":1 [ENABLED]

- entity 10: ov5640 1-003c (1 pad, 1 link)
             type V4L2 subdev subtype Sensor flags 0
             device node name /dev/v4l-subdev1
        pad0: Source
                [fmt:UYVY8_2X8/640x480@1/30 field:none colorspace:srgb
xfer:srgb ycbcr:601 quantization:full-range]
                -> "csi":0 [ENABLED]


Maybe we should fix this ?

Regards,

> 
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, imx7_csi_of_match);
>> +
>>  static int imx7_csi_probe(struct platform_device *pdev)
>>  {
>>  	struct device *dev = &pdev->dev;
>>  	struct device_node *node = dev->of_node;
>>  	struct imx_media_dev *imxmd;
>>  	struct imx7_csi *csi;
>> +	const struct of_device_id *of_id;
>>  	int ret;
>>
>> +	of_id = of_match_node(imx7_csi_of_match, node);
> 
> With the above said, here I think we can use the of_match_device?
> 
> hope this makes sense also to you.
> 
> Once again thanks for the patches.
> 
> ---
> Cheers,
>         Rui
> 
>> +	if (!of_id)
>> +		return -ENODEV;
>> +
>>  	csi = devm_kzalloc(&pdev->dev, sizeof(*csi), GFP_KERNEL);
>>  	if (!csi)
>>  		return -ENOMEM;
>>
>>  	csi->dev = dev;
>> +	csi->soc_id = (enum csi_soc_id)of_id->data;
>>
>>  	csi->mclk = devm_clk_get(&pdev->dev, "mclk");
>>  	if (IS_ERR(csi->mclk)) {
>> @@ -1294,12 +1332,6 @@ static int imx7_csi_remove(struct platform_device *pdev)
>>  	return 0;
>>  }
>>
>> -static const struct of_device_id imx7_csi_of_match[] = {
>> -	{ .compatible = "fsl,imx7-csi" },
>> -	{ },
>> -};
>> -MODULE_DEVICE_TABLE(of, imx7_csi_of_match);
>> -
>>  static struct platform_driver imx7_csi_driver = {
>>  	.probe = imx7_csi_probe,
>>  	.remove = imx7_csi_remove,
> 


-- 
Sébastien Szymanski
Software engineer, Armadeus Systems
Tel: +33 (0)9 72 29 41 44
Fax: +33 (0)9 72 28 79 26

Re: [PATCH v2 2/3] media: imx7-media-csi: add i.MX6UL support

[Rui Miguel Silva]

Hi Sebastien,
On Tue 11 Jun 2019 at 09:16, Sébastien Szymanski wrote:
> Hi Rui,
>
> thanks for the review!
>
> On 6/10/19 12:28 PM, Rui Miguel Silva wrote:
>> Hi Sebastien,
>> Thanks for the patch.
>>
>> On Thu 06 Jun 2019 at 16:38, Sébastien Szymanski wrote:
>>> i.MX7 and i.MX6UL/L have the same CSI controller. So add i.MX6UL/L support
>>> to imx7-media-csi driver.
>>>
>>> Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
>>> ---
>>>
>>> Changes for v2:
>>>  - rebase on top of linuxtv/master
>>>  - mention i.MX6UL/L in header and Kconfig help text
>>>  - rename csi_type to csi_soc_id
>>>
>>>  drivers/staging/media/imx/Kconfig          |  4 +-
>>>  drivers/staging/media/imx/imx7-media-csi.c | 62 ++++++++++++++++------
>>>  2 files changed, 49 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/imx/Kconfig b/drivers/staging/media/imx/Kconfig
>>> index ad3d7df6bb3c..8b6dc42c39e0 100644
>>> --- a/drivers/staging/media/imx/Kconfig
>>> +++ b/drivers/staging/media/imx/Kconfig
>>> @@ -22,11 +22,11 @@ config VIDEO_IMX_CSI
>>>  	  A video4linux camera sensor interface driver for i.MX5/6.
>>>
>>>  config VIDEO_IMX7_CSI
>>> -	tristate "i.MX7 Camera Sensor Interface driver"
>>> +	tristate "i.MX6UL/L / i.MX7 Camera Sensor Interface driver"
>>>  	depends on VIDEO_IMX_MEDIA && VIDEO_DEV && I2C
>>>  	default y
>>>  	help
>>>  	  Enable support for video4linux camera sensor interface driver for
>>> -	  i.MX7.
>>> +	  i.MX6UL/L or i.MX7.
>>>  endmenu
>>>  endif
>>> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
>>> index 9101566f3f67..902bdce594cf 100644
>>> --- a/drivers/staging/media/imx/imx7-media-csi.c
>>> +++ b/drivers/staging/media/imx/imx7-media-csi.c
>>> @@ -1,6 +1,6 @@
>>>  // SPDX-License-Identifier: GPL-2.0
>>>  /*
>>> - * V4L2 Capture CSI Subdev for Freescale i.MX7 SOC
>>> + * V4L2 Capture CSI Subdev for Freescale i.MX6UL/L / i.MX7 SOC
>>>   *
>>>   * Copyright (c) 2019 Linaro Ltd
>>>   *
>>> @@ -152,6 +152,11 @@
>>>  #define CSI_CSICR18		0x48
>>>  #define CSI_CSICR19		0x4c
>>>
>>> +enum csi_soc_id {
>>> +	IMX7,
>>> +	IMX6UL
>>> +};
>>> +
>>>  struct imx7_csi {
>>>  	struct device *dev;
>>>  	struct v4l2_subdev sd;
>>> @@ -191,6 +196,7 @@ struct imx7_csi {
>>>  	bool is_init;
>>>  	bool is_streaming;
>>>  	bool is_csi2;
>>> +	enum csi_soc_id soc_id;
>>>
>>>  	struct completion last_eof_completion;
>>>  };
>>> @@ -548,6 +554,14 @@ static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
>>>  	if (ret)
>>>  		return ret;
>>>
>>> +	if (csi->soc_id == IMX6UL) {
>>> +		mutex_lock(&csi->lock);
>>> +		csi->is_csi2 = false;
>>> +		mutex_unlock(&csi->lock);
>>> +
>>> +		return 0;
>>> +	}
>>> +
>>>  	ret = imx7_csi_get_upstream_endpoint(csi, &upstream_ep, true);
>>>  	if (ret) {
>>>  		v4l2_err(&csi->sd, "failed to find upstream endpoint\n");
>>> @@ -757,6 +771,7 @@ static int imx7_csi_configure(struct imx7_csi *csi)
>>>  	struct v4l2_pix_format *out_pix = &vdev->fmt.fmt.pix;
>>>  	__u32 in_code = csi->format_mbus[IMX7_CSI_PAD_SINK].code;
>>>  	u32 cr1, cr18;
>>> +	int width = out_pix->width;
>>>
>>>  	if (out_pix->field == V4L2_FIELD_INTERLACED) {
>>>  		imx7_csi_deinterlace_enable(csi, true);
>>> @@ -766,15 +781,27 @@ static int imx7_csi_configure(struct imx7_csi *csi)
>>>  		imx7_csi_buf_stride_set(csi, 0);
>>>  	}
>>>
>>> -	imx7_csi_set_imagpara(csi, out_pix->width, out_pix->height);
>>> +	cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
>>> +
>>> +	if (!csi->is_csi2) {
>>> +		if (out_pix->pixelformat == V4L2_PIX_FMT_UYVY ||
>>> +		    out_pix->pixelformat == V4L2_PIX_FMT_YUYV)
>>> +			width *= 2;
>>> +
>>> +		imx7_csi_set_imagpara(csi, width, out_pix->height);
>>> +
>>> +		cr18 |= (BIT_BASEADDR_SWITCH_EN | BIT_BASEADDR_SWITCH_SEL |
>>> +			BIT_BASEADDR_CHG_ERR_EN);
>>> +		imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
>>>
>>> -	if (!csi->is_csi2)
>>>  		return 0;
>>> +	}
>>> +
>>> +	imx7_csi_set_imagpara(csi, width, out_pix->height);
>>>
>>>  	cr1 = imx7_csi_reg_read(csi, CSI_CSICR1);
>>>  	cr1 &= ~BIT_GCLK_MODE;
>>>
>>> -	cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
>>>  	cr18 &= BIT_MIPI_DATA_FORMAT_MASK;
>>>  	cr18 |= BIT_DATA_FROM_MIPI;
>>>
>>> @@ -809,11 +836,9 @@ static void imx7_csi_enable(struct imx7_csi *csi)
>>>  {
>>>  	imx7_csi_sw_reset(csi);
>>>
>>> -	if (csi->is_csi2) {
>>> -		imx7_csi_dmareq_rff_enable(csi);
>>> -		imx7_csi_hw_enable_irq(csi);
>>> -		imx7_csi_hw_enable(csi);
>>> -	}
>>> +	imx7_csi_dmareq_rff_enable(csi);
>>> +	imx7_csi_hw_enable_irq(csi);
>>> +	imx7_csi_hw_enable(csi);
>>>  }
>>>
>>>  static void imx7_csi_disable(struct imx7_csi *csi)
>>> @@ -1166,19 +1191,32 @@ static int imx7_csi_parse_endpoint(struct device *dev,
>>>  	return fwnode_device_is_available(asd->match.fwnode) ? 0 : -EINVAL;
>>>  }
>>>
>>> +static const struct of_device_id imx7_csi_of_match[] = {
>>> +	{ .compatible = "fsl,imx7-csi", .data = (void *)IMX7 },
>>> +	{ .compatible = "fsl,imx6ul-csi", .data = (void *)IMX6UL },
>>
>> looking at this again I think we can do this is a different way.
>> Instead data being the soc_id, just set here if it is_csi2 or not.
>>
>> This would avoid to add a soc_id  to the struct that it really it
>> is used only to setup the is_csi2 var. I think this will make this
>> patch a lot simpler.
>
> Well, I have added this soc_id because imx7_csi_get_upstream_endpoint in
> imx7_csi_pad_link_validate fails:
>
> [  366.549768] csi: failed to find upstream endpoint
> [  366.556274] csi: pipeline start failed with -19
>

I think this fails because you do not define any endpoint for the
csi in your board dts file. I see in patch 1/3 the setup of csi,
disabled, but not the endpoint connecting csi with the ov5640 in
your board file (see the connection between mipi imx7 and ov2680
in the imx7-warp.dts, or the ov5640.txt file).

---
Cheers,
        Rui


>
> My pipeline is:
>
> Device topology
> - entity 1: csi (2 pads, 2 links)
>             type V4L2 subdev subtype Unknown flags 0
>             device node name /dev/v4l-subdev0
>         pad0: Sink
>                 [fmt:UYVY8_2X8/640x480 field:none colorspace:srgb
> xfer:srgb ycbcr:601 quantization:full-range]
>                 <- "ov5640 1-003c":0 [ENABLED]
>         pad1: Source
>                 [fmt:UYVY8_2X8/640x480 field:none colorspace:srgb
> xfer:srgb ycbcr:601 quantization:full-range]
>                 -> "csi capture":0 [ENABLED]
>
> - entity 4: csi capture (1 pad, 1 link)
>             type Node subtype V4L flags 0
>             device node name /dev/video1
>         pad0: Sink
>                 <- "csi":1 [ENABLED]
>
> - entity 10: ov5640 1-003c (1 pad, 1 link)
>              type V4L2 subdev subtype Sensor flags 0
>              device node name /dev/v4l-subdev1
>         pad0: Source
>                 [fmt:UYVY8_2X8/640x480@1/30 field:none colorspace:srgb
> xfer:srgb ycbcr:601 quantization:full-range]
>                 -> "csi":0 [ENABLED]
>
>
> Maybe we should fix this ?
>
> Regards,
>
>>
>>> +	{ },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, imx7_csi_of_match);
>>> +
>>>  static int imx7_csi_probe(struct platform_device *pdev)
>>>  {
>>>  	struct device *dev = &pdev->dev;
>>>  	struct device_node *node = dev->of_node;
>>>  	struct imx_media_dev *imxmd;
>>>  	struct imx7_csi *csi;
>>> +	const struct of_device_id *of_id;
>>>  	int ret;
>>>
>>> +	of_id = of_match_node(imx7_csi_of_match, node);
>>
>> With the above said, here I think we can use the of_match_device?
>>
>> hope this makes sense also to you.
>>
>> Once again thanks for the patches.
>>
>> ---
>> Cheers,
>>         Rui
>>
>>> +	if (!of_id)
>>> +		return -ENODEV;
>>> +
>>>  	csi = devm_kzalloc(&pdev->dev, sizeof(*csi), GFP_KERNEL);
>>>  	if (!csi)
>>>  		return -ENOMEM;
>>>
>>>  	csi->dev = dev;
>>> +	csi->soc_id = (enum csi_soc_id)of_id->data;
>>>
>>>  	csi->mclk = devm_clk_get(&pdev->dev, "mclk");
>>>  	if (IS_ERR(csi->mclk)) {
>>> @@ -1294,12 +1332,6 @@ static int imx7_csi_remove(struct platform_device *pdev)
>>>  	return 0;
>>>  }
>>>
>>> -static const struct of_device_id imx7_csi_of_match[] = {
>>> -	{ .compatible = "fsl,imx7-csi" },
>>> -	{ },
>>> -};
>>> -MODULE_DEVICE_TABLE(of, imx7_csi_of_match);
>>> -
>>>  static struct platform_driver imx7_csi_driver = {
>>>  	.probe = imx7_csi_probe,
>>>  	.remove = imx7_csi_remove,
>>

Re: [PATCH v2 2/3] media: imx7-media-csi: add i.MX6UL support

[Sébastien Szymanski]

On 6/11/19 11:40 AM, Rui Miguel Silva wrote:
> Hi Sebastien,
> On Tue 11 Jun 2019 at 09:16, Sébastien Szymanski wrote:
>> Hi Rui,
>>
>> thanks for the review!
>>
>> On 6/10/19 12:28 PM, Rui Miguel Silva wrote:
>>> Hi Sebastien,
>>> Thanks for the patch.
>>>
>>> On Thu 06 Jun 2019 at 16:38, Sébastien Szymanski wrote:
>>>> i.MX7 and i.MX6UL/L have the same CSI controller. So add i.MX6UL/L support
>>>> to imx7-media-csi driver.
>>>>
>>>> Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
>>>> ---
>>>>
>>>> Changes for v2:
>>>>  - rebase on top of linuxtv/master
>>>>  - mention i.MX6UL/L in header and Kconfig help text
>>>>  - rename csi_type to csi_soc_id
>>>>
>>>>  drivers/staging/media/imx/Kconfig          |  4 +-
>>>>  drivers/staging/media/imx/imx7-media-csi.c | 62 ++++++++++++++++------
>>>>  2 files changed, 49 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/media/imx/Kconfig b/drivers/staging/media/imx/Kconfig
>>>> index ad3d7df6bb3c..8b6dc42c39e0 100644
>>>> --- a/drivers/staging/media/imx/Kconfig
>>>> +++ b/drivers/staging/media/imx/Kconfig
>>>> @@ -22,11 +22,11 @@ config VIDEO_IMX_CSI
>>>>  	  A video4linux camera sensor interface driver for i.MX5/6.
>>>>
>>>>  config VIDEO_IMX7_CSI
>>>> -	tristate "i.MX7 Camera Sensor Interface driver"
>>>> +	tristate "i.MX6UL/L / i.MX7 Camera Sensor Interface driver"
>>>>  	depends on VIDEO_IMX_MEDIA && VIDEO_DEV && I2C
>>>>  	default y
>>>>  	help
>>>>  	  Enable support for video4linux camera sensor interface driver for
>>>> -	  i.MX7.
>>>> +	  i.MX6UL/L or i.MX7.
>>>>  endmenu
>>>>  endif
>>>> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
>>>> index 9101566f3f67..902bdce594cf 100644
>>>> --- a/drivers/staging/media/imx/imx7-media-csi.c
>>>> +++ b/drivers/staging/media/imx/imx7-media-csi.c
>>>> @@ -1,6 +1,6 @@
>>>>  // SPDX-License-Identifier: GPL-2.0
>>>>  /*
>>>> - * V4L2 Capture CSI Subdev for Freescale i.MX7 SOC
>>>> + * V4L2 Capture CSI Subdev for Freescale i.MX6UL/L / i.MX7 SOC
>>>>   *
>>>>   * Copyright (c) 2019 Linaro Ltd
>>>>   *
>>>> @@ -152,6 +152,11 @@
>>>>  #define CSI_CSICR18		0x48
>>>>  #define CSI_CSICR19		0x4c
>>>>
>>>> +enum csi_soc_id {
>>>> +	IMX7,
>>>> +	IMX6UL
>>>> +};
>>>> +
>>>>  struct imx7_csi {
>>>>  	struct device *dev;
>>>>  	struct v4l2_subdev sd;
>>>> @@ -191,6 +196,7 @@ struct imx7_csi {
>>>>  	bool is_init;
>>>>  	bool is_streaming;
>>>>  	bool is_csi2;
>>>> +	enum csi_soc_id soc_id;
>>>>
>>>>  	struct completion last_eof_completion;
>>>>  };
>>>> @@ -548,6 +554,14 @@ static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
>>>>  	if (ret)
>>>>  		return ret;
>>>>
>>>> +	if (csi->soc_id == IMX6UL) {
>>>> +		mutex_lock(&csi->lock);
>>>> +		csi->is_csi2 = false;
>>>> +		mutex_unlock(&csi->lock);
>>>> +
>>>> +		return 0;
>>>> +	}
>>>> +
>>>>  	ret = imx7_csi_get_upstream_endpoint(csi, &upstream_ep, true);
>>>>  	if (ret) {
>>>>  		v4l2_err(&csi->sd, "failed to find upstream endpoint\n");
>>>> @@ -757,6 +771,7 @@ static int imx7_csi_configure(struct imx7_csi *csi)
>>>>  	struct v4l2_pix_format *out_pix = &vdev->fmt.fmt.pix;
>>>>  	__u32 in_code = csi->format_mbus[IMX7_CSI_PAD_SINK].code;
>>>>  	u32 cr1, cr18;
>>>> +	int width = out_pix->width;
>>>>
>>>>  	if (out_pix->field == V4L2_FIELD_INTERLACED) {
>>>>  		imx7_csi_deinterlace_enable(csi, true);
>>>> @@ -766,15 +781,27 @@ static int imx7_csi_configure(struct imx7_csi *csi)
>>>>  		imx7_csi_buf_stride_set(csi, 0);
>>>>  	}
>>>>
>>>> -	imx7_csi_set_imagpara(csi, out_pix->width, out_pix->height);
>>>> +	cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
>>>> +
>>>> +	if (!csi->is_csi2) {
>>>> +		if (out_pix->pixelformat == V4L2_PIX_FMT_UYVY ||
>>>> +		    out_pix->pixelformat == V4L2_PIX_FMT_YUYV)
>>>> +			width *= 2;
>>>> +
>>>> +		imx7_csi_set_imagpara(csi, width, out_pix->height);
>>>> +
>>>> +		cr18 |= (BIT_BASEADDR_SWITCH_EN | BIT_BASEADDR_SWITCH_SEL |
>>>> +			BIT_BASEADDR_CHG_ERR_EN);
>>>> +		imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
>>>>
>>>> -	if (!csi->is_csi2)
>>>>  		return 0;
>>>> +	}
>>>> +
>>>> +	imx7_csi_set_imagpara(csi, width, out_pix->height);
>>>>
>>>>  	cr1 = imx7_csi_reg_read(csi, CSI_CSICR1);
>>>>  	cr1 &= ~BIT_GCLK_MODE;
>>>>
>>>> -	cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
>>>>  	cr18 &= BIT_MIPI_DATA_FORMAT_MASK;
>>>>  	cr18 |= BIT_DATA_FROM_MIPI;
>>>>
>>>> @@ -809,11 +836,9 @@ static void imx7_csi_enable(struct imx7_csi *csi)
>>>>  {
>>>>  	imx7_csi_sw_reset(csi);
>>>>
>>>> -	if (csi->is_csi2) {
>>>> -		imx7_csi_dmareq_rff_enable(csi);
>>>> -		imx7_csi_hw_enable_irq(csi);
>>>> -		imx7_csi_hw_enable(csi);
>>>> -	}
>>>> +	imx7_csi_dmareq_rff_enable(csi);
>>>> +	imx7_csi_hw_enable_irq(csi);
>>>> +	imx7_csi_hw_enable(csi);
>>>>  }
>>>>
>>>>  static void imx7_csi_disable(struct imx7_csi *csi)
>>>> @@ -1166,19 +1191,32 @@ static int imx7_csi_parse_endpoint(struct device *dev,
>>>>  	return fwnode_device_is_available(asd->match.fwnode) ? 0 : -EINVAL;
>>>>  }
>>>>
>>>> +static const struct of_device_id imx7_csi_of_match[] = {
>>>> +	{ .compatible = "fsl,imx7-csi", .data = (void *)IMX7 },
>>>> +	{ .compatible = "fsl,imx6ul-csi", .data = (void *)IMX6UL },
>>>
>>> looking at this again I think we can do this is a different way.
>>> Instead data being the soc_id, just set here if it is_csi2 or not.
>>>
>>> This would avoid to add a soc_id  to the struct that it really it
>>> is used only to setup the is_csi2 var. I think this will make this
>>> patch a lot simpler.
>>
>> Well, I have added this soc_id because imx7_csi_get_upstream_endpoint in
>> imx7_csi_pad_link_validate fails:
>>
>> [  366.549768] csi: failed to find upstream endpoint
>> [  366.556274] csi: pipeline start failed with -19
>>
> 
> I think this fails because you do not define any endpoint for the
> csi in your board dts file. I see in patch 1/3 the setup of csi,
> disabled, but not the endpoint connecting csi with the ov5640 in
> your board file (see the connection between mipi imx7 and ov2680
> in the imx7-warp.dts, or the ov5640.txt file).

I actually do, in the device tree of my board I have:

&csi {
	pinctrl-names = "default";
	pinctrl-0 = <&pinctrl_csi>;
	status = "okay";

	port {
		csi_ep: endpoint {
			remote-endpoint = <&ov5640_ep>;
			bus-type = <5>; // V4L2_FWNODE_BUS_TYPE_PARALLEL
		};
	};
};

and

&i2c2 {
..
	ov5640: camera@3c {
		...
		port {
                        ov5640_ep: endpoint {
                                remote-endpoint = <&csi_ep>;
                                bus-width = <8>;
                                data-shift = <2>; /* lines 9:2 are used */
                                hsync-active = <0>;
                                vsync-active = <1>;
                                pclk-sample = <0>;
                        };
                };
	};
};

Regards,

> 
> ---
> Cheers,
>         Rui
> 
> 
>>
>> My pipeline is:
>>
>> Device topology
>> - entity 1: csi (2 pads, 2 links)
>>             type V4L2 subdev subtype Unknown flags 0
>>             device node name /dev/v4l-subdev0
>>         pad0: Sink
>>                 [fmt:UYVY8_2X8/640x480 field:none colorspace:srgb
>> xfer:srgb ycbcr:601 quantization:full-range]
>>                 <- "ov5640 1-003c":0 [ENABLED]
>>         pad1: Source
>>                 [fmt:UYVY8_2X8/640x480 field:none colorspace:srgb
>> xfer:srgb ycbcr:601 quantization:full-range]
>>                 -> "csi capture":0 [ENABLED]
>>
>> - entity 4: csi capture (1 pad, 1 link)
>>             type Node subtype V4L flags 0
>>             device node name /dev/video1
>>         pad0: Sink
>>                 <- "csi":1 [ENABLED]
>>
>> - entity 10: ov5640 1-003c (1 pad, 1 link)
>>              type V4L2 subdev subtype Sensor flags 0
>>              device node name /dev/v4l-subdev1
>>         pad0: Source
>>                 [fmt:UYVY8_2X8/640x480@1/30 field:none colorspace:srgb
>> xfer:srgb ycbcr:601 quantization:full-range]
>>                 -> "csi":0 [ENABLED]
>>
>>
>> Maybe we should fix this ?
>>
>> Regards,
>>
>>>
>>>> +	{ },
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, imx7_csi_of_match);
>>>> +
>>>>  static int imx7_csi_probe(struct platform_device *pdev)
>>>>  {
>>>>  	struct device *dev = &pdev->dev;
>>>>  	struct device_node *node = dev->of_node;
>>>>  	struct imx_media_dev *imxmd;
>>>>  	struct imx7_csi *csi;
>>>> +	const struct of_device_id *of_id;
>>>>  	int ret;
>>>>
>>>> +	of_id = of_match_node(imx7_csi_of_match, node);
>>>
>>> With the above said, here I think we can use the of_match_device?
>>>
>>> hope this makes sense also to you.
>>>
>>> Once again thanks for the patches.
>>>
>>> ---
>>> Cheers,
>>>         Rui
>>>
>>>> +	if (!of_id)
>>>> +		return -ENODEV;
>>>> +
>>>>  	csi = devm_kzalloc(&pdev->dev, sizeof(*csi), GFP_KERNEL);
>>>>  	if (!csi)
>>>>  		return -ENOMEM;
>>>>
>>>>  	csi->dev = dev;
>>>> +	csi->soc_id = (enum csi_soc_id)of_id->data;
>>>>
>>>>  	csi->mclk = devm_clk_get(&pdev->dev, "mclk");
>>>>  	if (IS_ERR(csi->mclk)) {
>>>> @@ -1294,12 +1332,6 @@ static int imx7_csi_remove(struct platform_device *pdev)
>>>>  	return 0;
>>>>  }
>>>>
>>>> -static const struct of_device_id imx7_csi_of_match[] = {
>>>> -	{ .compatible = "fsl,imx7-csi" },
>>>> -	{ },
>>>> -};
>>>> -MODULE_DEVICE_TABLE(of, imx7_csi_of_match);
>>>> -
>>>>  static struct platform_driver imx7_csi_driver = {
>>>>  	.probe = imx7_csi_probe,
>>>>  	.remove = imx7_csi_remove,
>>>
> 


-- 
Sébastien Szymanski
Software engineer, Armadeus Systems
Tel: +33 (0)9 72 29 41 44
Fax: +33 (0)9 72 28 79 26

Re: [PATCH v2 2/3] media: imx7-media-csi: add i.MX6UL support

[Rui Miguel Silva]

Hi Sebastien,
On Tue 11 Jun 2019 at 11:03, Sébastien Szymanski wrote:
> On 6/11/19 11:40 AM, Rui Miguel Silva wrote:
>> Hi Sebastien,
>> On Tue 11 Jun 2019 at 09:16, Sébastien Szymanski wrote:
>>> Hi Rui,
>>>
>>> thanks for the review!
>>>
>>> On 6/10/19 12:28 PM, Rui Miguel Silva wrote:
>>>> Hi Sebastien,
>>>> Thanks for the patch.
>>>>
>>>> On Thu 06 Jun 2019 at 16:38, Sébastien Szymanski wrote:
>>>>> i.MX7 and i.MX6UL/L have the same CSI controller. So add i.MX6UL/L support
>>>>> to imx7-media-csi driver.
>>>>>
>>>>> Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
>>>>> ---
>>>>>
>>>>> Changes for v2:
>>>>>  - rebase on top of linuxtv/master
>>>>>  - mention i.MX6UL/L in header and Kconfig help text
>>>>>  - rename csi_type to csi_soc_id
>>>>>
>>>>>  drivers/staging/media/imx/Kconfig          |  4 +-
>>>>>  drivers/staging/media/imx/imx7-media-csi.c | 62 ++++++++++++++++------
>>>>>  2 files changed, 49 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/drivers/staging/media/imx/Kconfig b/drivers/staging/media/imx/Kconfig
>>>>> index ad3d7df6bb3c..8b6dc42c39e0 100644
>>>>> --- a/drivers/staging/media/imx/Kconfig
>>>>> +++ b/drivers/staging/media/imx/Kconfig
>>>>> @@ -22,11 +22,11 @@ config VIDEO_IMX_CSI
>>>>>  	  A video4linux camera sensor interface driver for i.MX5/6.
>>>>>
>>>>>  config VIDEO_IMX7_CSI
>>>>> -	tristate "i.MX7 Camera Sensor Interface driver"
>>>>> +	tristate "i.MX6UL/L / i.MX7 Camera Sensor Interface driver"
>>>>>  	depends on VIDEO_IMX_MEDIA && VIDEO_DEV && I2C
>>>>>  	default y
>>>>>  	help
>>>>>  	  Enable support for video4linux camera sensor interface driver for
>>>>> -	  i.MX7.
>>>>> +	  i.MX6UL/L or i.MX7.
>>>>>  endmenu
>>>>>  endif
>>>>> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
>>>>> index 9101566f3f67..902bdce594cf 100644
>>>>> --- a/drivers/staging/media/imx/imx7-media-csi.c
>>>>> +++ b/drivers/staging/media/imx/imx7-media-csi.c
>>>>> @@ -1,6 +1,6 @@
>>>>>  // SPDX-License-Identifier: GPL-2.0
>>>>>  /*
>>>>> - * V4L2 Capture CSI Subdev for Freescale i.MX7 SOC
>>>>> + * V4L2 Capture CSI Subdev for Freescale i.MX6UL/L / i.MX7 SOC
>>>>>   *
>>>>>   * Copyright (c) 2019 Linaro Ltd
>>>>>   *
>>>>> @@ -152,6 +152,11 @@
>>>>>  #define CSI_CSICR18		0x48
>>>>>  #define CSI_CSICR19		0x4c
>>>>>
>>>>> +enum csi_soc_id {
>>>>> +	IMX7,
>>>>> +	IMX6UL
>>>>> +};
>>>>> +
>>>>>  struct imx7_csi {
>>>>>  	struct device *dev;
>>>>>  	struct v4l2_subdev sd;
>>>>> @@ -191,6 +196,7 @@ struct imx7_csi {
>>>>>  	bool is_init;
>>>>>  	bool is_streaming;
>>>>>  	bool is_csi2;
>>>>> +	enum csi_soc_id soc_id;
>>>>>
>>>>>  	struct completion last_eof_completion;
>>>>>  };
>>>>> @@ -548,6 +554,14 @@ static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
>>>>>  	if (ret)
>>>>>  		return ret;
>>>>>
>>>>> +	if (csi->soc_id == IMX6UL) {
>>>>> +		mutex_lock(&csi->lock);
>>>>> +		csi->is_csi2 = false;
>>>>> +		mutex_unlock(&csi->lock);
>>>>> +
>>>>> +		return 0;
>>>>> +	}
>>>>> +
>>>>>  	ret = imx7_csi_get_upstream_endpoint(csi, &upstream_ep, true);
>>>>>  	if (ret) {
>>>>>  		v4l2_err(&csi->sd, "failed to find upstream endpoint\n");
>>>>> @@ -757,6 +771,7 @@ static int imx7_csi_configure(struct imx7_csi *csi)
>>>>>  	struct v4l2_pix_format *out_pix = &vdev->fmt.fmt.pix;
>>>>>  	__u32 in_code = csi->format_mbus[IMX7_CSI_PAD_SINK].code;
>>>>>  	u32 cr1, cr18;
>>>>> +	int width = out_pix->width;
>>>>>
>>>>>  	if (out_pix->field == V4L2_FIELD_INTERLACED) {
>>>>>  		imx7_csi_deinterlace_enable(csi, true);
>>>>> @@ -766,15 +781,27 @@ static int imx7_csi_configure(struct imx7_csi *csi)
>>>>>  		imx7_csi_buf_stride_set(csi, 0);
>>>>>  	}
>>>>>
>>>>> -	imx7_csi_set_imagpara(csi, out_pix->width, out_pix->height);
>>>>> +	cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
>>>>> +
>>>>> +	if (!csi->is_csi2) {
>>>>> +		if (out_pix->pixelformat == V4L2_PIX_FMT_UYVY ||
>>>>> +		    out_pix->pixelformat == V4L2_PIX_FMT_YUYV)
>>>>> +			width *= 2;
>>>>> +
>>>>> +		imx7_csi_set_imagpara(csi, width, out_pix->height);
>>>>> +
>>>>> +		cr18 |= (BIT_BASEADDR_SWITCH_EN | BIT_BASEADDR_SWITCH_SEL |
>>>>> +			BIT_BASEADDR_CHG_ERR_EN);
>>>>> +		imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
>>>>>
>>>>> -	if (!csi->is_csi2)
>>>>>  		return 0;
>>>>> +	}
>>>>> +
>>>>> +	imx7_csi_set_imagpara(csi, width, out_pix->height);
>>>>>
>>>>>  	cr1 = imx7_csi_reg_read(csi, CSI_CSICR1);
>>>>>  	cr1 &= ~BIT_GCLK_MODE;
>>>>>
>>>>> -	cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
>>>>>  	cr18 &= BIT_MIPI_DATA_FORMAT_MASK;
>>>>>  	cr18 |= BIT_DATA_FROM_MIPI;
>>>>>
>>>>> @@ -809,11 +836,9 @@ static void imx7_csi_enable(struct imx7_csi *csi)
>>>>>  {
>>>>>  	imx7_csi_sw_reset(csi);
>>>>>
>>>>> -	if (csi->is_csi2) {
>>>>> -		imx7_csi_dmareq_rff_enable(csi);
>>>>> -		imx7_csi_hw_enable_irq(csi);
>>>>> -		imx7_csi_hw_enable(csi);
>>>>> -	}
>>>>> +	imx7_csi_dmareq_rff_enable(csi);
>>>>> +	imx7_csi_hw_enable_irq(csi);
>>>>> +	imx7_csi_hw_enable(csi);
>>>>>  }
>>>>>
>>>>>  static void imx7_csi_disable(struct imx7_csi *csi)
>>>>> @@ -1166,19 +1191,32 @@ static int imx7_csi_parse_endpoint(struct device *dev,
>>>>>  	return fwnode_device_is_available(asd->match.fwnode) ? 0 : -EINVAL;
>>>>>  }
>>>>>
>>>>> +static const struct of_device_id imx7_csi_of_match[] = {
>>>>> +	{ .compatible = "fsl,imx7-csi", .data = (void *)IMX7 },
>>>>> +	{ .compatible = "fsl,imx6ul-csi", .data = (void *)IMX6UL },
>>>>
>>>> looking at this again I think we can do this is a different way.
>>>> Instead data being the soc_id, just set here if it is_csi2 or not.
>>>>
>>>> This would avoid to add a soc_id  to the struct that it really it
>>>> is used only to setup the is_csi2 var. I think this will make this
>>>> patch a lot simpler.
>>>
>>> Well, I have added this soc_id because imx7_csi_get_upstream_endpoint in
>>> imx7_csi_pad_link_validate fails:
>>>
>>> [  366.549768] csi: failed to find upstream endpoint
>>> [  366.556274] csi: pipeline start failed with -19
>>>
>>
>> I think this fails because you do not define any endpoint for the
>> csi in your board dts file. I see in patch 1/3 the setup of csi,
>> disabled, but not the endpoint connecting csi with the ov5640 in
>> your board file (see the connection between mipi imx7 and ov2680
>> in the imx7-warp.dts, or the ov5640.txt file).
>
> I actually do, in the device tree of my board I have:

Yeah, I thought you did this, because if not it did not work in
the first place. I will take a look at why the fetch of the
upstream endpoint is not working. it should :).

Thanks for the feedback. I will let you know.

---
Cheers,
	Rui

>
> &csi {
> 	pinctrl-names = "default";
> 	pinctrl-0 = <&pinctrl_csi>;
> 	status = "okay";
>
> 	port {
> 		csi_ep: endpoint {
> 			remote-endpoint = <&ov5640_ep>;
> 			bus-type = <5>; // V4L2_FWNODE_BUS_TYPE_PARALLEL
> 		};
> 	};
> };
>
> and
>
> &i2c2 {
> ..
> 	ov5640: camera@3c {
> 		...
> 		port {
>                         ov5640_ep: endpoint {
>                                 remote-endpoint = <&csi_ep>;
>                                 bus-width = <8>;
>                                 data-shift = <2>; /* lines 9:2 are used */
>                                 hsync-active = <0>;
>                                 vsync-active = <1>;
>                                 pclk-sample = <0>;
>                         };
>                 };
> 	};
> };
>
> Regards,

Re: [PATCH v2 3/3] media: dt-bindings: imx7-csi: add i.MX6UL/L support

[Rob Herring]

On Thu,  6 Jun 2019 17:38:25 +0200, =?UTF-8?q?S=C3=A9bastien=20Szymanski?=          wrote:
> Document "fsl,imx6ul-csi" entry.
> 
> Signed-off-by: Sébastien Szymanski <sebastien.szymanski@armadeus.com>
> ---
> 
> Changes for v2:
>  - New patch to document new "fsl,imx6ul-csi" entry.
> 
>  Documentation/devicetree/bindings/media/imx7-csi.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>