* [PATCH v2 1/3] ARM: dts: imx6ul: Add csi node @ 2019-06-06 15:38 Sébastien Szymanski 2019-06-06 15:38 ` [PATCH v2 2/3] media: imx7-media-csi: add i.MX6UL support Sébastien Szymanski 2019-06-06 15:38 ` [PATCH v2 3/3] media: dt-bindings: imx7-csi: add i.MX6UL/L support Sébastien Szymanski 0 siblings, 2 replies; 11+ messages in thread From: Sébastien Szymanski @ 2019-06-06 15:38 UTC (permalink / raw) To: devel, linux-kernel, linux-arm-kernel, devicetree, linux-media, Shawn Guo, Mauro Carvalho Chehab Cc: Greg Kroah-Hartman, Philipp Zabel, Steve Longerbeam, NXP Linux Team, Fabio Estevam, Pengutronix Kernel Team, Sascha Hauer, Mark Rutland, Rob Herring, Rui Miguel Silva, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/3] media: imx7-media-csi: add i.MX6UL support 2019-06-06 15:38 [PATCH v2 1/3] ARM: dts: imx6ul: Add csi node Sébastien Szymanski @ 2019-06-06 15:38 ` Sébastien Szymanski 2019-06-06 23:10 ` Randy Dunlap 2019-06-10 10:28 ` Rui Miguel Silva 2019-06-06 15:38 ` [PATCH v2 3/3] media: dt-bindings: imx7-csi: add i.MX6UL/L support Sébastien Szymanski 1 sibling, 2 replies; 11+ messages in thread From: Sébastien Szymanski @ 2019-06-06 15:38 UTC (permalink / raw) To: devel, linux-kernel, linux-arm-kernel, devicetree, linux-media, Shawn Guo, Mauro Carvalho Chehab Cc: Greg Kroah-Hartman, Philipp Zabel, Steve Longerbeam, NXP Linux Team, Fabio Estevam, Pengutronix Kernel Team, Sascha Hauer, Mark Rutland, Rob Herring, Rui Miguel Silva, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] media: imx7-media-csi: add i.MX6UL support 2019-06-06 15:38 ` [PATCH v2 2/3] media: imx7-media-csi: add i.MX6UL support Sébastien Szymanski @ 2019-06-06 23:10 ` Randy Dunlap 2019-06-10 10:32 ` Rui Miguel Silva 2019-06-10 10:28 ` Rui Miguel Silva 1 sibling, 1 reply; 11+ messages in thread From: Randy Dunlap @ 2019-06-06 23:10 UTC (permalink / raw) To: Sébastien Szymanski, devel, linux-kernel, linux-arm-kernel, devicetree, linux-media, Shawn Guo, Mauro Carvalho Chehab Cc: Mark Rutland, Philipp Zabel, Greg Kroah-Hartman, Sascha Hauer, Rob Herring, NXP Linux Team, Pengutronix Kernel Team, Steve Longerbeam 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] media: imx7-media-csi: add i.MX6UL support 2019-06-06 23:10 ` Randy Dunlap @ 2019-06-10 10:32 ` Rui Miguel Silva 0 siblings, 0 replies; 11+ messages in thread From: Rui Miguel Silva @ 2019-06-10 10:32 UTC (permalink / raw) To: driverdev-devel Cc: Sébastien Szymanski, devel, linux-kernel, linux-arm-kernel, devicetree, linux-media, Shawn Guo, Mauro Carvalho Chehab, Mark Rutland, Pengutronix Kernel Team, Greg Kroah-Hartman, Sascha Hauer, Rob Herring, NXP Linux Team, Philipp Zabel, Steve Longerbeam 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] media: imx7-media-csi: add i.MX6UL support 2019-06-06 15:38 ` [PATCH v2 2/3] media: imx7-media-csi: add i.MX6UL support Sébastien Szymanski 2019-06-06 23:10 ` Randy Dunlap @ 2019-06-10 10:28 ` Rui Miguel Silva 2019-06-11 8:16 ` Sébastien Szymanski 1 sibling, 1 reply; 11+ messages in thread From: Rui Miguel Silva @ 2019-06-10 10:28 UTC (permalink / raw) To: Sébastien Szymanski Cc: devel, linux-kernel, linux-arm-kernel, devicetree, linux-media, Shawn Guo, Mauro Carvalho Chehab, Greg Kroah-Hartman, Philipp Zabel, Steve Longerbeam, NXP Linux Team, Fabio Estevam, Pengutronix Kernel Team, Sascha Hauer, Mark Rutland, Rob Herring 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, ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] media: imx7-media-csi: add i.MX6UL support 2019-06-10 10:28 ` Rui Miguel Silva @ 2019-06-11 8:16 ` Sébastien Szymanski 2019-06-11 9:40 ` Rui Miguel Silva 0 siblings, 1 reply; 11+ messages in thread From: Sébastien Szymanski @ 2019-06-11 8:16 UTC (permalink / raw) To: Rui Miguel Silva Cc: devel, linux-kernel, linux-arm-kernel, devicetree, linux-media, Shawn Guo, Mauro Carvalho Chehab, Greg Kroah-Hartman, Philipp Zabel, Steve Longerbeam, NXP Linux Team, Fabio Estevam, Pengutronix Kernel Team, Sascha Hauer, Mark Rutland, Rob Herring 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] media: imx7-media-csi: add i.MX6UL support 2019-06-11 8:16 ` Sébastien Szymanski @ 2019-06-11 9:40 ` Rui Miguel Silva 2019-06-11 10:03 ` Sébastien Szymanski 0 siblings, 1 reply; 11+ messages in thread From: Rui Miguel Silva @ 2019-06-11 9:40 UTC (permalink / raw) To: Sébastien Szymanski Cc: devel, linux-kernel, linux-arm-kernel, devicetree, linux-media, Shawn Guo, Mauro Carvalho Chehab, Greg Kroah-Hartman, Philipp Zabel, Steve Longerbeam, NXP Linux Team, Fabio Estevam, Pengutronix Kernel Team, Sascha Hauer, Mark Rutland, Rob Herring 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, >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] media: imx7-media-csi: add i.MX6UL support 2019-06-11 9:40 ` Rui Miguel Silva @ 2019-06-11 10:03 ` Sébastien Szymanski 2019-06-11 11:56 ` Rui Miguel Silva 0 siblings, 1 reply; 11+ messages in thread From: Sébastien Szymanski @ 2019-06-11 10:03 UTC (permalink / raw) To: Rui Miguel Silva Cc: devel, linux-kernel, linux-arm-kernel, devicetree, linux-media, Shawn Guo, Mauro Carvalho Chehab, Greg Kroah-Hartman, Philipp Zabel, Steve Longerbeam, NXP Linux Team, Fabio Estevam, Pengutronix Kernel Team, Sascha Hauer, Mark Rutland, Rob Herring 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] media: imx7-media-csi: add i.MX6UL support 2019-06-11 10:03 ` Sébastien Szymanski @ 2019-06-11 11:56 ` Rui Miguel Silva 0 siblings, 0 replies; 11+ messages in thread From: Rui Miguel Silva @ 2019-06-11 11:56 UTC (permalink / raw) To: Sébastien Szymanski Cc: devel, linux-kernel, linux-arm-kernel, devicetree, linux-media, Shawn Guo, Mauro Carvalho Chehab, Greg Kroah-Hartman, Philipp Zabel, Steve Longerbeam, NXP Linux Team, Fabio Estevam, Pengutronix Kernel Team, Sascha Hauer, Mark Rutland, Rob Herring 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, ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] media: dt-bindings: imx7-csi: add i.MX6UL/L support 2019-06-06 15:38 [PATCH v2 1/3] ARM: dts: imx6ul: Add csi node Sébastien Szymanski 2019-06-06 15:38 ` [PATCH v2 2/3] media: imx7-media-csi: add i.MX6UL support Sébastien Szymanski @ 2019-06-06 15:38 ` Sébastien Szymanski 2019-07-09 1:52 ` Rob Herring 1 sibling, 1 reply; 11+ messages in thread From: Sébastien Szymanski @ 2019-06-06 15:38 UTC (permalink / raw) To: devel, linux-kernel, linux-arm-kernel, devicetree, linux-media, Shawn Guo, Mauro Carvalho Chehab Cc: Greg Kroah-Hartman, Philipp Zabel, Steve Longerbeam, NXP Linux Team, Fabio Estevam, Pengutronix Kernel Team, Sascha Hauer, Mark Rutland, Rob Herring, Rui Miguel Silva, 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] media: dt-bindings: imx7-csi: add i.MX6UL/L support 2019-06-06 15:38 ` [PATCH v2 3/3] media: dt-bindings: imx7-csi: add i.MX6UL/L support Sébastien Szymanski @ 2019-07-09 1:52 ` Rob Herring 0 siblings, 0 replies; 11+ messages in thread From: Rob Herring @ 2019-07-09 1:52 UTC (permalink / raw) To: Sébastien Szymanski Cc: devel, linux-kernel, linux-arm-kernel, devicetree, linux-media, Shawn Guo, Mauro Carvalho Chehab, Greg Kroah-Hartman, Philipp Zabel, Steve Longerbeam, NXP Linux Team, Fabio Estevam, Pengutronix Kernel Team, Sascha Hauer, Mark Rutland, Rui Miguel Silva, Sébastien Szymanski 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-07-09 1:52 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-06 15:38 [PATCH v2 1/3] ARM: dts: imx6ul: Add csi node Sébastien Szymanski 2019-06-06 15:38 ` [PATCH v2 2/3] media: imx7-media-csi: add i.MX6UL support Sébastien Szymanski 2019-06-06 23:10 ` Randy Dunlap 2019-06-10 10:32 ` Rui Miguel Silva 2019-06-10 10:28 ` Rui Miguel Silva 2019-06-11 8:16 ` Sébastien Szymanski 2019-06-11 9:40 ` Rui Miguel Silva 2019-06-11 10:03 ` Sébastien Szymanski 2019-06-11 11:56 ` Rui Miguel Silva 2019-06-06 15:38 ` [PATCH v2 3/3] media: dt-bindings: imx7-csi: add i.MX6UL/L support Sébastien Szymanski 2019-07-09 1:52 ` Rob Herring
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).