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

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Rob Herring <robh@kernel.org>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>,
	devicetree@vger.kernel.org, linux-media@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Jacopo Mondi <jacopo@jmondi.org>,
	Eugen Hristev <eugen.hristev@microchip.com>,
	Hugues Fruchet <hugues.fruchet@foss.st.com>,
	Maxime Coquelin <mcoquelin.stm32@gmail.com>,
	Alexandre Torgue <alexandre.torgue@foss.st.com>
Subject: Re: [PATCH 1/2] dt-bindings: media: Add macros for video interface bus types
Date: Sun, 6 Mar 2022 18:54:10 +0200	[thread overview]
Message-ID: <YiTnMq+czvLzEtnf@pendragon.ideasonboard.com> (raw)
In-Reply-To: <Yh416qrZr32PzMtJ@robh.at.kernel.org>

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__ */

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel