Hi Sebastien,
Many thanks for this patch.

Please note that there is another series on top of this code [0],
that turns my patch that you mention below obsolete, maybe you may
want to rebase on top of that. and also a different patch from me
[1].

[0]: 
https://lore.kernel.org/lkml/20190430225018.30252-2-slongerb...@gmail.com/#b
[1]: 
https://lore.kernel.org/linux-media/20190430222523.22814-1-rui.si...@linaro.org/

On Tue 30 Apr 2019 at 08:49, 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.szyman...@armadeus.com>

Can you also add i.MX6UL/L to the header of this file and maybe in
the help string in the Kconfig. That would make it clear that this
is supported also by this driver.

> ---
>  This patch needs the following patch from Rui Miguel Silva:
>  https://patchwork.linuxtv.org/patch/55657/
>
>  I have tested this patch with a OV5640 sensor (8-bit parallel). The 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:smpte170m 
> xfer:709 ycbcr:601 quantization:lim-range]
>                 <- "ov5640 1-003c":0 [ENABLED]
>         pad1: Source
>                 [fmt:UYVY8_2X8/640x480 field:none colorspace:smpte170m 
> xfer:709 ycbcr:601 quantization:lim-range]
>                 -> "csi capture":0 [ENABLED]
>
>  - entity 4: csi capture (1 pad, 1 link)
>             type Node subtype V4L flags 0
>             device node name /dev/video0
>         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]
>
>  drivers/staging/media/imx/imx7-media-csi.c | 61 ++++++++++++++++------
>  1 file changed, 46 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c 
> b/drivers/staging/media/imx/imx7-media-csi.c
> index a708a0340eb1..ef4534a96fa0 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -154,6 +154,11 @@
>  
>  static const char * const imx7_csi_clk_id[] = {"axi", "dcic", "mclk"};
>  
> +enum csi_type {

maybe here the name could be something along enum csi_device_id, I
don't know, type does not sound good to me??

> +     IMX7,
> +     IMX6UL
> +};
> +
>  struct imx7_csi {
>       struct device *dev;
>       struct v4l2_subdev sd;
> @@ -195,6 +200,7 @@ struct imx7_csi {
>       bool is_init;
>       bool is_streaming;
>       bool is_csi2;
> +     enum csi_type type;

here the same, instead type maybe dev_id?? or dev_type??

>  
>       struct completion last_eof_completion;
>  };
> @@ -554,6 +560,14 @@ static int imx7_csi_pad_link_validate(struct v4l2_subdev 
> *sd,
>       if (ret)
>               return ret;
>  
> +     if (csi->type == 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");
> @@ -763,6 +777,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);
> @@ -772,15 +787,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;
>  
> @@ -815,12 +842,9 @@ static int 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);
> -             return 0;
> -     }
> +     imx7_csi_dmareq_rff_enable(csi);
> +     imx7_csi_hw_enable_irq(csi);
> +     imx7_csi_hw_enable(csi);
>  
>       return 0;
>  }
> @@ -1218,20 +1242,33 @@ static int imx7_csi_clocks_get(struct imx7_csi *csi)
>       return devm_clk_bulk_get(dev, csi->num_clks, csi->clks);
>  }
>  
> +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;
>       struct resource *res;
>       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->type = (enum csi_type)of_id->data;
>  
>       ret = imx7_csi_clocks_get(csi);
>       if (ret < 0) {
> @@ -1349,12 +1386,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,

_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Reply via email to