Add csi node for i.MX6UL SoC.
Reviewed-by: Fabio Estevam <[email protected]>
Signed-off-by: Sébastien Szymanski <[email protected]>
---
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
Document "fsl,imx6ul-csi" entry.
Signed-off-by: Sébastien Szymanski <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
>
> 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
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 <[email protected]>
> ---
>
> 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,
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 <[email protected]>
>> ---
>>
>> 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
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 <[email protected]>
>> ---
>>
>> 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
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 <[email protected]>
>>> ---
>>>
>>> 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,
>>
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 <[email protected]>
>>>> ---
>>>>
>>>> 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
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 <[email protected]>
>>>>> ---
>>>>>
>>>>> 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,
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 <[email protected]>
> ---
>
> 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 <[email protected]>