Hi Laurent
On Mon, Feb 14, 2022 at 09:15:07PM +0200, Laurent Pinchart wrote:
> 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@xxxxxxxxxx>
> > ---
> > 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";
No, I kind of superficially added the mq version where the mm was
already and went on :)
Care to explain why currently we have two const for the "8mm" and the
"imx7" versions ?
>
> 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.
I'm not aware of how how many revisions of the imx7 and imx6 versions
exists, nor how they differ, but the existing distinction feels a bit
weird.
The const items should be the compatible fallbacks, should them be
generic, why is 8mm among them ? Shouldn't we specify the precise SoC
version in the list of possible enum items only ?
Something like
oneOf:
- enum:
- fsl,imx8mq-csi
- fsl,imx8mm-csi
- fsl,imx6ul-csi
- const:
- fsl,imx7-csi
In example I see:
arch/arm64/boot/dts/freescale/imx8mq.dtsi: compatible = "fsl,imx8mq-csi", "fsl,imx7-csi";
Where this should either be
compatible = "fsl,imx8mq-csi"
or
compatible = "fsl,imx8mm-csi", "fsl,imx7-csi";
?
>
> > 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.
Sorry, but shouldn't:
compatible = "fsl,imx8mm-csi", fsl,imx7-csi"
allow me to match on imx8mm already, without the above change.
I think what I don't get is why imx8mm is a 'generic fallback' in
first place.
>
> > { .compatible = "fsl,imx7-csi", .data = (void *)IMX7_CSI_IMX7 },
> > { .compatible = "fsl,imx6ul-csi", .data = (void *)IMX7_CSI_IMX7 },
> > { },
>
> --
> Regards,
>
> Laurent Pinchart
