Re: [PATCH 3/8] staging: media: imx: Add more compatible strings

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jacopo,

Thank you for the patch.

On Mon, Feb 14, 2022 at 07:43:13PM +0100, Jacopo Mondi wrote:
> The imx7-media-csi driver controls the CSI (CMOS Sensor Interface)
> peripheral available on several SoC of different generations.
> 
> The current situation when it comes to compatible strings is rather
> confused:
> - Bindings document imx6ul, imx7 and imx8mm
> - Driver supports imx6ul, imx7 and imx8mq
> - The IMX8MM and IMX8MQ DTS use the correct compatible strings with a
>   fallback to the generic "imx7-csi" identifier:
>   arch/arm64/boot/dts/freescale/imx8mq.dtsi: compatible = "fsl,imx8mq-csi",
>   arch/arm64/boot/dts/freescale/imx8mm.dtsi: compatible = "fsl,imx8mm-csi",
> 
> Tidy-up the situation by adding the IMX8MQ compatible string to the
> bindings documentation andathe IMX8MM identifier to the driver, to allow
> to distinguish the SoC the CSI peripheral is integrated on in the
> following patches.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml | 1 +
>  drivers/staging/media/imx/imx7-media-csi.c                | 2 ++

I think Rob would prefer this being split in two patches, and I think it
would make sense, as you're fixing two separate issues.

>  2 files changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> index 4f7b78265336..0f1505d85111 100644
> --- a/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> +++ b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml
> @@ -21,6 +21,7 @@ properties:
>            - fsl,imx7-csi
>            - fsl,imx6ul-csi
>        - items:
> +          - const: fsl,imx8mq-csi
>            - const: fsl,imx8mm-csi
>            - const: fsl,imx7-csi

I don't think you intended to require the following:

	compatible = "fsl,imx8mq-csi", "fsl,imx8mm-csi", "fsl,imx7-csi";

You probably want

 properties:
   compatible:
     oneOf:
       - enum:
+          - fsl,imx8mq-csi
           - fsl,imx7-csi
           - fsl,imx6ul-csi
       - items:
           - const: fsl,imx8mm-csi
           - const: fsl,imx7-csi

instead.

> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index 32311fc0e2a4..59100e409709 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -162,6 +162,7 @@
>  enum imx_csi_model {
>  	IMX7_CSI_IMX7 = 0,
>  	IMX7_CSI_IMX8MQ,
> +	IMX7_CSI_IMX8MM,
>  };
>  
>  struct imx7_csi {
> @@ -1277,6 +1278,7 @@ static int imx7_csi_remove(struct platform_device *pdev)
>  
>  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 },

This isn't needed, as the i.MX8MM CSI bridgge is considered fully
backward-compatible with the i.MX7 version. I'd introduce this change in
the patch where you start using IMX7_CSI_IMX8MM, and I would then add
the following to the DT binding:

 properties:
   compatible:
     oneOf:
       - enum:
           - fsl,imx8mq-csi
+          - fsl,imx8mm-csi
           - fsl,imx7-csi
           - fsl,imx6ul-csi
       - items:
           - const: fsl,imx8mm-csi
           - const: fsl,imx7-csi

to allow setting

	compatible = "fsl,imx8mm-csi";

without the imx7 fallback if we consider the i.MX8MM version different.
If the driver can operate correctly on the i.MX8MM when using the i.MX7
fallback code paths (possibly minor issues that are not considered
fatal, such as missing features) then you could skip this binding
change.

>  	{ .compatible = "fsl,imx7-csi", .data = (void *)IMX7_CSI_IMX7 },
>  	{ .compatible = "fsl,imx6ul-csi", .data = (void *)IMX7_CSI_IMX7 },
>  	{ },

-- 
Regards,

Laurent Pinchart