| Message ID | 20220214184318.409208-5-jacopo@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: imx: Destage imx7-mipi-csis with fixes on top | expand |
Hi Jacopo, Thank you for the patch. On Mon, Feb 14, 2022 at 07:43:14PM +0100, Jacopo Mondi wrote: > Define the imx-media-info structure which contains CSI configuration > parameter that depend on the SoC version the peripheral is integrated > in. > > Replace the existing 'model' field with the newly defined structure. > > Only define the SoC id and the supported pixel sampling modes for the > moment. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > drivers/staging/media/imx/imx-media.h | 44 ++++++++++++++++++++++ > drivers/staging/media/imx/imx7-media-csi.c | 44 ++++++++++++++-------- > 2 files changed, 73 insertions(+), 15 deletions(-) > > diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h > index f263fc3adbb9..1b0b660413cb 100644 > --- a/drivers/staging/media/imx/imx-media.h > +++ b/drivers/staging/media/imx/imx-media.h > @@ -18,6 +18,16 @@ > #define IMX_MEDIA_DEF_PIX_WIDTH 640 > #define IMX_MEDIA_DEF_PIX_HEIGHT 480 > > +/* > + * Enumeration of the SoC models the peripheral is integrated in. > + */ > +enum soc_id { > + IMX6UL, > + IMX7, > + IMX8MM, > + IMX8MQ, Those names are too generic. > +}; > + > /* > * Enumeration of the IPU internal sub-devices > */ > @@ -141,10 +151,44 @@ struct imx_media_pad_vdev { > struct list_head list; > }; > > +/* > + * enum sample_mode_id - Define the CSI Rx queue sample size > + * > + * The pixel sampling mode defines the possible sampling methods from the > + * CSI Rx queue to the next processing block of the capture pipeline. > + * > + * The supported methods depends on the SoC model and on synthesis time > + * configurations. > + * > + * @MODE_SINGLE: Single pixel mode sampling > + * @MODE_DUAL: Double pixel mode sampling > + * @MODE_QUAD: Quad pixel mode sampling > + */ > +enum sample_mode_id { > + MODE_SINGLE = BIT(0), > + MODE_DUAL = BIT(1), > + MODE_QUAD = BIT(2), Here too. > +}; Let's limit this to the imx7-media-csi driver, it's unrelated to the i.MX6 IPUv3 and should not be part of common helpers. It doesn't seem like any subsequent patch in this series use the sample mode or the soc_id in common helpers, so it should hopefully not be a bit issue. I would also like to see a comment somewhere (in this patch or one of the subsequent ones) that explains in more details how the CSIS and CSI bridge are connected, and how various bits affect data signal routing between the two. I can help if necessary. > +/* > + * Information and configurations dependent on the SoC the peripheral is > + * integrated in. > + * > + * @soc_id: The SoC identifier. See &enum soc_id. > + * @sample_modes: Mask of supported pixel modes. See &enum sample_mode_id. > + */ > +struct imx_media_info { > + enum soc_id soc_id; > + u8 sample_modes; > +}; > + > struct imx_media_dev { > struct media_device md; > struct v4l2_device v4l2_dev; > > + /* Per-model information. */ > + const struct imx_media_info *info; > + > /* the pipeline object */ > struct media_pipeline pipe; > > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c > index 59100e409709..112096774961 100644 > --- a/drivers/staging/media/imx/imx7-media-csi.c > +++ b/drivers/staging/media/imx/imx7-media-csi.c > @@ -159,12 +159,6 @@ > #define CSI_CSICR18 0x48 > #define CSI_CSICR19 0x4c > > -enum imx_csi_model { > - IMX7_CSI_IMX7 = 0, > - IMX7_CSI_IMX8MQ, > - IMX7_CSI_IMX8MM, > -}; I think you can keep this instead of soc_id. > - > struct imx7_csi { > struct device *dev; > struct v4l2_subdev sd; > @@ -200,8 +194,6 @@ struct imx7_csi { > bool is_csi2; > > struct completion last_eof_completion; > - > - enum imx_csi_model model; > }; > > static struct imx7_csi * > @@ -562,6 +554,8 @@ static void imx7_csi_baseaddr_switch_on_second_frame(struct imx7_csi *csi) > > static void imx7_csi_enable(struct imx7_csi *csi) > { > + struct imx_media_dev *imxmd = csi->imxmd; > + > /* Clear the Rx FIFO and reflash the DMA controller. */ > imx7_csi_rx_fifo_clear(csi); > imx7_csi_dma_reflash(csi); > @@ -576,7 +570,7 @@ static void imx7_csi_enable(struct imx7_csi *csi) > imx7_csi_dmareq_rff_enable(csi); > imx7_csi_hw_enable(csi); > > - if (csi->model == IMX7_CSI_IMX8MQ) > + if (imxmd->info->soc_id == IMX8MQ) > imx7_csi_baseaddr_switch_on_second_frame(csi); > } > > @@ -1181,8 +1175,6 @@ static int imx7_csi_probe(struct platform_device *pdev) > if (IS_ERR(csi->regbase)) > return PTR_ERR(csi->regbase); > > - csi->model = (enum imx_csi_model)(uintptr_t)of_device_get_match_data(&pdev->dev); > - > spin_lock_init(&csi->irqlock); > mutex_init(&csi->lock); > > @@ -1202,6 +1194,8 @@ static int imx7_csi_probe(struct platform_device *pdev) > } > platform_set_drvdata(pdev, &csi->sd); > > + imxmd->info = of_device_get_match_data(dev); > + > ret = imx_media_of_add_csi(imxmd, node); > if (ret < 0 && ret != -ENODEV && ret != -EEXIST) > goto cleanup; > @@ -1276,11 +1270,31 @@ static int imx7_csi_remove(struct platform_device *pdev) > return 0; > } > > +static const struct imx_media_info imx8mq_info = { > + .soc_id = IMX8MQ, > + .sample_modes = MODE_SINGLE, > +}; > + > +static const struct imx_media_info imx8mm_info = { > + .soc_id = IMX8MM, > + .sample_modes = MODE_SINGLE | MODE_DUAL, > +}; > + > +static const struct imx_media_info imx7_info = { > + .soc_id = IMX7, > + .sample_modes = MODE_SINGLE, > +}; > + > +static const struct imx_media_info imx6ul_info = { > + .soc_id = IMX6UL, > + .sample_modes = MODE_SINGLE, > +}; > + > static const struct of_device_id imx7_csi_of_match[] = { > - { .compatible = "fsl,imx8mq-csi", .data = (void *)IMX7_CSI_IMX8MQ }, > - { .compatible = "fsl,imx8mm-csi", .data = (void *)IMX7_CSI_IMX8MM }, > - { .compatible = "fsl,imx7-csi", .data = (void *)IMX7_CSI_IMX7 }, > - { .compatible = "fsl,imx6ul-csi", .data = (void *)IMX7_CSI_IMX7 }, > + { .compatible = "fsl,imx8mq-csi", .data = &imx8mq_info }, > + { .compatible = "fsl,imx8mm-csi", .data = &imx8mm_info }, > + { .compatible = "fsl,imx7-csi", .data = &imx7_info }, > + { .compatible = "fsl,imx6ul-csi", .data = &imx6ul_info }, > { }, > }; > MODULE_DEVICE_TABLE(of, imx7_csi_of_match);
Hi Laurent On Mon, Feb 14, 2022 at 09:20:41PM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Mon, Feb 14, 2022 at 07:43:14PM +0100, Jacopo Mondi wrote: > > Define the imx-media-info structure which contains CSI configuration > > parameter that depend on the SoC version the peripheral is integrated > > in. > > > > Replace the existing 'model' field with the newly defined structure. > > > > Only define the SoC id and the supported pixel sampling modes for the > > moment. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > drivers/staging/media/imx/imx-media.h | 44 ++++++++++++++++++++++ > > drivers/staging/media/imx/imx7-media-csi.c | 44 ++++++++++++++-------- > > 2 files changed, 73 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h > > index f263fc3adbb9..1b0b660413cb 100644 > > --- a/drivers/staging/media/imx/imx-media.h > > +++ b/drivers/staging/media/imx/imx-media.h > > @@ -18,6 +18,16 @@ > > #define IMX_MEDIA_DEF_PIX_WIDTH 640 > > #define IMX_MEDIA_DEF_PIX_HEIGHT 480 > > > > +/* > > + * Enumeration of the SoC models the peripheral is integrated in. > > + */ > > +enum soc_id { > > + IMX6UL, > > + IMX7, > > + IMX8MM, > > + IMX8MQ, > > Those names are too generic. > C++ scoped enums got me used to shorten the single enum items :) Of course this should be made specific. > > +}; > > + > > /* > > * Enumeration of the IPU internal sub-devices > > */ > > @@ -141,10 +151,44 @@ struct imx_media_pad_vdev { > > struct list_head list; > > }; > > > > +/* > > + * enum sample_mode_id - Define the CSI Rx queue sample size > > + * > > + * The pixel sampling mode defines the possible sampling methods from the > > + * CSI Rx queue to the next processing block of the capture pipeline. > > + * > > + * The supported methods depends on the SoC model and on synthesis time > > + * configurations. > > + * > > + * @MODE_SINGLE: Single pixel mode sampling > > + * @MODE_DUAL: Double pixel mode sampling > > + * @MODE_QUAD: Quad pixel mode sampling > > + */ > > +enum sample_mode_id { > > + MODE_SINGLE = BIT(0), > > + MODE_DUAL = BIT(1), > > + MODE_QUAD = BIT(2), > > Here too. > > > +}; > > Let's limit this to the imx7-media-csi driver, it's unrelated to the > i.MX6 IPUv3 and should not be part of common helpers. It doesn't seem Ok, I had no idea this header was used by i.MX6 too. What a mess. > like any subsequent patch in this series use the sample mode or the > soc_id in common helpers, so it should hopefully not be a bit issue. > No it would not. > I would also like to see a comment somewhere (in this patch or one of > the subsequent ones) that explains in more details how the CSIS and CSI > bridge are connected, and how various bits affect data signal routing > between the two. I can help if necessary. > I can try but I'm not sure I have the full picture in mind. > > +/* > > + * Information and configurations dependent on the SoC the peripheral is > > + * integrated in. > > + * > > + * @soc_id: The SoC identifier. See &enum soc_id. > > + * @sample_modes: Mask of supported pixel modes. See &enum sample_mode_id. > > + */ > > +struct imx_media_info { > > + enum soc_id soc_id; > > + u8 sample_modes; > > +}; > > + > > struct imx_media_dev { > > struct media_device md; > > struct v4l2_device v4l2_dev; > > > > + /* Per-model information. */ > > + const struct imx_media_info *info; > > + > > /* the pipeline object */ > > struct media_pipeline pipe; > > > > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c > > index 59100e409709..112096774961 100644 > > --- a/drivers/staging/media/imx/imx7-media-csi.c > > +++ b/drivers/staging/media/imx/imx7-media-csi.c > > @@ -159,12 +159,6 @@ > > #define CSI_CSICR18 0x48 > > #define CSI_CSICR19 0x4c > > > > -enum imx_csi_model { > > - IMX7_CSI_IMX7 = 0, > > - IMX7_CSI_IMX8MQ, > > - IMX7_CSI_IMX8MM, > > -}; > > I think you can keep this instead of soc_id. > Ahem. I develed this patches on a downstream where I could test and where these where not there yet. When I rebased on media-master I found out about these ones, and I replaced them without too much thinking. > > - > > struct imx7_csi { > > struct device *dev; > > struct v4l2_subdev sd; > > @@ -200,8 +194,6 @@ struct imx7_csi { > > bool is_csi2; > > > > struct completion last_eof_completion; > > - > > - enum imx_csi_model model; > > }; > > > > static struct imx7_csi * > > @@ -562,6 +554,8 @@ static void imx7_csi_baseaddr_switch_on_second_frame(struct imx7_csi *csi) > > > > static void imx7_csi_enable(struct imx7_csi *csi) > > { > > + struct imx_media_dev *imxmd = csi->imxmd; > > + > > /* Clear the Rx FIFO and reflash the DMA controller. */ > > imx7_csi_rx_fifo_clear(csi); > > imx7_csi_dma_reflash(csi); > > @@ -576,7 +570,7 @@ static void imx7_csi_enable(struct imx7_csi *csi) > > imx7_csi_dmareq_rff_enable(csi); > > imx7_csi_hw_enable(csi); > > > > - if (csi->model == IMX7_CSI_IMX8MQ) > > + if (imxmd->info->soc_id == IMX8MQ) > > imx7_csi_baseaddr_switch_on_second_frame(csi); > > } > > > > @@ -1181,8 +1175,6 @@ static int imx7_csi_probe(struct platform_device *pdev) > > if (IS_ERR(csi->regbase)) > > return PTR_ERR(csi->regbase); > > > > - csi->model = (enum imx_csi_model)(uintptr_t)of_device_get_match_data(&pdev->dev); > > - > > spin_lock_init(&csi->irqlock); > > mutex_init(&csi->lock); > > > > @@ -1202,6 +1194,8 @@ static int imx7_csi_probe(struct platform_device *pdev) > > } > > platform_set_drvdata(pdev, &csi->sd); > > > > + imxmd->info = of_device_get_match_data(dev); > > + > > ret = imx_media_of_add_csi(imxmd, node); > > if (ret < 0 && ret != -ENODEV && ret != -EEXIST) > > goto cleanup; > > @@ -1276,11 +1270,31 @@ static int imx7_csi_remove(struct platform_device *pdev) > > return 0; > > } > > > > +static const struct imx_media_info imx8mq_info = { > > + .soc_id = IMX8MQ, > > + .sample_modes = MODE_SINGLE, > > +}; > > + > > +static const struct imx_media_info imx8mm_info = { > > + .soc_id = IMX8MM, > > + .sample_modes = MODE_SINGLE | MODE_DUAL, > > +}; > > + > > +static const struct imx_media_info imx7_info = { > > + .soc_id = IMX7, > > + .sample_modes = MODE_SINGLE, > > +}; > > + > > +static const struct imx_media_info imx6ul_info = { > > + .soc_id = IMX6UL, > > + .sample_modes = MODE_SINGLE, > > +}; > > + > > static const struct of_device_id imx7_csi_of_match[] = { > > - { .compatible = "fsl,imx8mq-csi", .data = (void *)IMX7_CSI_IMX8MQ }, > > - { .compatible = "fsl,imx8mm-csi", .data = (void *)IMX7_CSI_IMX8MM }, > > - { .compatible = "fsl,imx7-csi", .data = (void *)IMX7_CSI_IMX7 }, > > - { .compatible = "fsl,imx6ul-csi", .data = (void *)IMX7_CSI_IMX7 }, > > + { .compatible = "fsl,imx8mq-csi", .data = &imx8mq_info }, > > + { .compatible = "fsl,imx8mm-csi", .data = &imx8mm_info }, > > + { .compatible = "fsl,imx7-csi", .data = &imx7_info }, > > + { .compatible = "fsl,imx6ul-csi", .data = &imx6ul_info }, > > { }, > > }; > > MODULE_DEVICE_TABLE(of, imx7_csi_of_match); > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h index f263fc3adbb9..1b0b660413cb 100644 --- a/drivers/staging/media/imx/imx-media.h +++ b/drivers/staging/media/imx/imx-media.h @@ -18,6 +18,16 @@ #define IMX_MEDIA_DEF_PIX_WIDTH 640 #define IMX_MEDIA_DEF_PIX_HEIGHT 480 +/* + * Enumeration of the SoC models the peripheral is integrated in. + */ +enum soc_id { + IMX6UL, + IMX7, + IMX8MM, + IMX8MQ, +}; + /* * Enumeration of the IPU internal sub-devices */ @@ -141,10 +151,44 @@ struct imx_media_pad_vdev { struct list_head list; }; +/* + * enum sample_mode_id - Define the CSI Rx queue sample size + * + * The pixel sampling mode defines the possible sampling methods from the + * CSI Rx queue to the next processing block of the capture pipeline. + * + * The supported methods depends on the SoC model and on synthesis time + * configurations. + * + * @MODE_SINGLE: Single pixel mode sampling + * @MODE_DUAL: Double pixel mode sampling + * @MODE_QUAD: Quad pixel mode sampling + */ +enum sample_mode_id { + MODE_SINGLE = BIT(0), + MODE_DUAL = BIT(1), + MODE_QUAD = BIT(2), +}; + +/* + * Information and configurations dependent on the SoC the peripheral is + * integrated in. + * + * @soc_id: The SoC identifier. See &enum soc_id. + * @sample_modes: Mask of supported pixel modes. See &enum sample_mode_id. + */ +struct imx_media_info { + enum soc_id soc_id; + u8 sample_modes; +}; + struct imx_media_dev { struct media_device md; struct v4l2_device v4l2_dev; + /* Per-model information. */ + const struct imx_media_info *info; + /* the pipeline object */ struct media_pipeline pipe; diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c index 59100e409709..112096774961 100644 --- a/drivers/staging/media/imx/imx7-media-csi.c +++ b/drivers/staging/media/imx/imx7-media-csi.c @@ -159,12 +159,6 @@ #define CSI_CSICR18 0x48 #define CSI_CSICR19 0x4c -enum imx_csi_model { - IMX7_CSI_IMX7 = 0, - IMX7_CSI_IMX8MQ, - IMX7_CSI_IMX8MM, -}; - struct imx7_csi { struct device *dev; struct v4l2_subdev sd; @@ -200,8 +194,6 @@ struct imx7_csi { bool is_csi2; struct completion last_eof_completion; - - enum imx_csi_model model; }; static struct imx7_csi * @@ -562,6 +554,8 @@ static void imx7_csi_baseaddr_switch_on_second_frame(struct imx7_csi *csi) static void imx7_csi_enable(struct imx7_csi *csi) { + struct imx_media_dev *imxmd = csi->imxmd; + /* Clear the Rx FIFO and reflash the DMA controller. */ imx7_csi_rx_fifo_clear(csi); imx7_csi_dma_reflash(csi); @@ -576,7 +570,7 @@ static void imx7_csi_enable(struct imx7_csi *csi) imx7_csi_dmareq_rff_enable(csi); imx7_csi_hw_enable(csi); - if (csi->model == IMX7_CSI_IMX8MQ) + if (imxmd->info->soc_id == IMX8MQ) imx7_csi_baseaddr_switch_on_second_frame(csi); } @@ -1181,8 +1175,6 @@ static int imx7_csi_probe(struct platform_device *pdev) if (IS_ERR(csi->regbase)) return PTR_ERR(csi->regbase); - csi->model = (enum imx_csi_model)(uintptr_t)of_device_get_match_data(&pdev->dev); - spin_lock_init(&csi->irqlock); mutex_init(&csi->lock); @@ -1202,6 +1194,8 @@ static int imx7_csi_probe(struct platform_device *pdev) } platform_set_drvdata(pdev, &csi->sd); + imxmd->info = of_device_get_match_data(dev); + ret = imx_media_of_add_csi(imxmd, node); if (ret < 0 && ret != -ENODEV && ret != -EEXIST) goto cleanup; @@ -1276,11 +1270,31 @@ static int imx7_csi_remove(struct platform_device *pdev) return 0; } +static const struct imx_media_info imx8mq_info = { + .soc_id = IMX8MQ, + .sample_modes = MODE_SINGLE, +}; + +static const struct imx_media_info imx8mm_info = { + .soc_id = IMX8MM, + .sample_modes = MODE_SINGLE | MODE_DUAL, +}; + +static const struct imx_media_info imx7_info = { + .soc_id = IMX7, + .sample_modes = MODE_SINGLE, +}; + +static const struct imx_media_info imx6ul_info = { + .soc_id = IMX6UL, + .sample_modes = MODE_SINGLE, +}; + static const struct of_device_id imx7_csi_of_match[] = { - { .compatible = "fsl,imx8mq-csi", .data = (void *)IMX7_CSI_IMX8MQ }, - { .compatible = "fsl,imx8mm-csi", .data = (void *)IMX7_CSI_IMX8MM }, - { .compatible = "fsl,imx7-csi", .data = (void *)IMX7_CSI_IMX7 }, - { .compatible = "fsl,imx6ul-csi", .data = (void *)IMX7_CSI_IMX7 }, + { .compatible = "fsl,imx8mq-csi", .data = &imx8mq_info }, + { .compatible = "fsl,imx8mm-csi", .data = &imx8mm_info }, + { .compatible = "fsl,imx7-csi", .data = &imx7_info }, + { .compatible = "fsl,imx6ul-csi", .data = &imx6ul_info }, { }, }; MODULE_DEVICE_TABLE(of, imx7_csi_of_match);
Define the imx-media-info structure which contains CSI configuration parameter that depend on the SoC version the peripheral is integrated in. Replace the existing 'model' field with the newly defined structure. Only define the SoC id and the supported pixel sampling modes for the moment. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- drivers/staging/media/imx/imx-media.h | 44 ++++++++++++++++++++++ drivers/staging/media/imx/imx7-media-csi.c | 44 ++++++++++++++-------- 2 files changed, 73 insertions(+), 15 deletions(-)