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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Feb 15, 2022 at 10:45:41AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Tue, Feb 15, 2022 at 09:36:31AM +0100, Jacopo Mondi wrote:
> > On Mon, Feb 14, 2022 at 09:15:07PM +0200, Laurent Pinchart wrote:
> > > 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 ?
>
> Because the imx8mm version was considered compatible with the imx7, so
>
>        - items:
>            - const: fsl,imx8mm-csi
>            - const: fsl,imx7-csi
>
> will validate
>
> 	compatible = "fsl,imx8mm-csi", "fsl,imx7-csi";
>
> The first imx8mm compatible string is ignored by the driver, but
> documented to support future drivers changes that would require
> differentiating between the two versions.
>

ah that's why the driver doesn't match on mm, yet

> > > 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 ?
>
> No, the const items are not the compatible fallbacks. imx8mm isn't a
> generic fallback. "items" requires *all* items to be present in order to
> validate.
>
> > Something like
> >
> >       oneOf:
> >         - enum:
> >           - fsl,imx8mq-csi
> >           - fsl,imx8mm-csi
> >           - fsl,imx6ul-csi
> >         - const:
> >           - fsl,imx7-csi
>
> That's not a valid schema.
>

I meant

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

But I now understand why 8mm and imx7 were there.

And the oneOf indeed doesn't allow to use imx7 as a fallback but
rather as an alternative to the precise SoC identifiers.

> >
> > 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.
>
> See above.
>
> > > >  	{ .compatible = "fsl,imx7-csi", .data = (void *)IMX7_CSI_IMX7 },
> > > >  	{ .compatible = "fsl,imx6ul-csi", .data = (void *)IMX7_CSI_IMX7 },
> > > >  	{ },
>
> --
> Regards,
>
> Laurent Pinchart




[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux