Re: [PATCH 1/2] dt-bindings: media: Add macros for video interface bus types

Laurent Pinchart
2022-03-06T16:56:08Z

Hi Rob,

On Tue, Mar 01, 2022 at 09:04:10AM -0600, Rob Herring wrote:
> On Sun, Feb 27, 2022 at 11:16:28PM +0200, Laurent Pinchart wrote:
> > On Sun, Feb 27, 2022 at 11:07:23PM +0200, Sakari Ailus wrote:
> > > On Sun, Feb 27, 2022 at 10:33:51PM +0200, Laurent Pinchart wrote:
> > > > Add a new dt-bindings/media/video-interfaces.h header that defines
> > > > macros corresponding to the bus types from media/video-interfaces.yaml.
> > > > This allows avoiding hardcoded constants in device tree sources.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > > include/dt-bindings/media/video-interfaces.h | 16 ++++++++++++++++
> > > > 1 file changed, 16 insertions(+)
> > > > create mode 100644 include/dt-bindings/media/video-interfaces.h
> > > >
> > > > diff --git a/include/dt-bindings/media/video-interfaces.h b/include/dt-bindings/media/video-interfaces.h
> > > > new file mode 100644
> > > > index 000000000000..e38058e1cca7
> > > > --- /dev/null
> > > > +++ b/include/dt-bindings/media/video-interfaces.h
> > > > @@ -0,0 +1,16 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0-only */
> > > > +/*
> > > > + * Copyright (C) 2022 Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > + */
> > > > +
> > > > +#ifndef __DT_BINDINGS_MEDIA_VIDEO_INTERFACES_H__
> > > > +#define __DT_BINDINGS_MEDIA_VIDEO_INTERFACES_H__
> > > > +
> > > > +#define MEDIA_BUS_TYPE_CSI2_CPHY 1
> > > > +#define MEDIA_BUS_TYPE_CSI1 2
> > > > +#define MEDIA_BUS_TYPE_CCP2 3
> > > > +#define MEDIA_BUS_TYPE_CSI2_DPHY 4
> > > > +#define MEDIA_BUS_TYPE_PARALLEL 5
> > >
> > > I've been long thinkin of renaming "PARALLEL" as "BT.601" which it really
> > > is. I don't mind postponing that, but I think you could as well start here.
> > > Up to you.
> >
> > I think it's a good idea, but we then need to decide what to do with
> > other types of parallel buses. Let's start this discussion now, and
> > implement it in a patch on top of this series.
>
> 5 and what it means is an ABI. If it is ambiguous and needs to be more
> specific, then you need new numbers for all of those specific types.
>
> If it is just a rename, I prefer it is done from the start.

It's both :-) It's ambiguous, but only used to refer to BT.601-liked
buses today in mainline, so I'll rename it. The number may be used to
refer to different types of parallel buses out-of-tree, and we can add
new types for that in mainline later when/if needed.

> > > Should this be somehow visible in video-interfaces.yaml?
> >
> > I wish we could use macros in .yaml files instead of numerical values,
> > but I don't think that's possible. I can do this:
> >
> > bus-type:
> > $ref: /schemas/types.yaml#/definitions/uint32
> > enum:
> > - - 1 # MIPI CSI-2 C-PHY
> > - - 2 # MIPI CSI1
> > - - 3 # CCP2
> > - - 4 # MIPI CSI-2 D-PHY
> > - - 5 # Parallel
> > - - 6 # BT.656
> > + - 1 # MIPI CSI-2 C-PHY (MEDIA_BUS_TYPE_CSI2_CPHY)
> > + - 2 # MIPI CSI1 (MEDIA_BUS_TYPE_CSI1)
> > + - 3 # CCP2 (MEDIA_BUS_TYPE_CCP2)
> > + - 4 # MIPI CSI-2 D-PHY (MEDIA_BUS_TYPE_CSI2_DPHY)
> > + - 5 # Parallel (MEDIA_BUS_TYPE_PARALLEL)
> > + - 6 # BT.656 (MEDIA_BUS_TYPE_BT656)
>
> Seems a bit redundant to have both comment text and define. The only
> part missing from the defines is 'MIPI'.

I agree. I'll use the macros.

It would be nice if macros could be used instead of numerical values in
the YAML schema, but that's certainly not high on the wishlist.

> > description:
> > - Data bus type.
> > + Data bus type. Use the macros listed above (defined in
> > + dt-bindings/video-interfaces.h) instead of numerical values.
> >
> > Any better proposal ?
> >
> > > > +#define MEDIA_BUS_TYPE_BT656 6
> > > > +
> > > > +#endif /* __DT_BINDINGS_MEDIA_VIDEO_INTERFACES_H__ */