Re: [PATCH v2 01/10] [media] Move mediabus format definition to a more standard place

[Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

On Fri, 7 Nov 2014 13:43:59 +0200
Sakari Ailus <sakari.ailus-X3B1VOXEql0@public.gmane.org> wrote:

> Hi Boris,
> 
> Thank you for the update.
> 
> On Thu, Nov 06, 2014 at 10:56:59AM +0100, Boris Brezillon wrote:
> > Rename mediabus formats and move the enum into a separate header file so
> > that it can be used by DRM/KMS subsystem without any reference to the V4L2
> > subsystem.
> > 
> > Old v4l2_mbus_pixelcode now points to media_bus_format.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > Acked-by: Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org>
> > ---
> >  include/uapi/linux/Kbuild             |   1 +
> >  include/uapi/linux/media-bus-format.h | 131 ++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/v4l2-mediabus.h    | 114 +----------------------------
> >  3 files changed, 134 insertions(+), 112 deletions(-)
> >  create mode 100644 include/uapi/linux/media-bus-format.h
> > 
> > diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
> > index b70237e..b2c23f8 100644
> > --- a/include/uapi/linux/Kbuild
> > +++ b/include/uapi/linux/Kbuild
> > @@ -414,6 +414,7 @@ header-y += veth.h
> >  header-y += vfio.h
> >  header-y += vhost.h
> >  header-y += videodev2.h
> > +header-y += media-bus-format.h
> 
> Could you arrange this to the list alphabetically, please?
> 
> >  header-y += virtio_9p.h
> >  header-y += virtio_balloon.h
> >  header-y += virtio_blk.h
> > diff --git a/include/uapi/linux/media-bus-format.h b/include/uapi/linux/media-bus-format.h
> > new file mode 100644
> > index 0000000..251a902
> > --- /dev/null
> > +++ b/include/uapi/linux/media-bus-format.h
> > @@ -0,0 +1,131 @@
> > +/*
> > + * Media Bus API header
> > + *
> > + * Copyright (C) 2009, Guennadi Liakhovetski <g.liakhovetski-Mmb7MZpHnFY@public.gmane.org>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#ifndef __LINUX_MEDIA_BUS_FORMAT_H
> > +#define __LINUX_MEDIA_BUS_FORMAT_H
> > +
> > +/*
> > + * These bus formats uniquely identify data formats on the data bus. Format 0
> > + * is reserved, MEDIA_BUS_FMT_FIXED shall be used by host-client pairs, where
> > + * the data format is fixed. Additionally, "2X8" means that one pixel is
> > + * transferred in two 8-bit samples, "BE" or "LE" specify in which order those
> > + * samples are transferred over the bus: "LE" means that the least significant
> > + * bits are transferred first, "BE" means that the most significant bits are
> > + * transferred first, and "PADHI" and "PADLO" define which bits - low or high,
> > + * in the incomplete high byte, are filled with padding bits.
> > + *
> > + * The bus formats are grouped by type, bus_width, bits per component, samples
> > + * per pixel and order of subsamples. Numerical values are sorted using generic
> > + * numerical sort order (8 thus comes before 10).
> > + *
> > + * As their value can't change when a new bus format is inserted in the
> > + * enumeration, the bus formats are explicitly given a numerical value. The next
> > + * free values for each category are listed below, update them when inserting
> > + * new pixel codes.
> > + */
> > +
> > +#define MEDIA_BUS_FMT_ENTRY(name, val)	\
> > +	MEDIA_BUS_FMT_ ## name = val,	\
> > +	V4L2_MBUS_FMT_ ## name = val
> > +
> > +enum media_bus_format {
> 
> There's no really a need to keep the definitions inside the enum. It looks a
> little bit confusing to me. That made me realise something I missed
> yesterday.
> 
> There's a difference: the enum in C++ is a different thing than in C, and
> the enum type isn't able to contain any other values than those defined in
> the enumeration.
> 
> So what I propose is the following. Keep enum v4l2_mbus_pixelcode around,
> including the enum values. Define new values for MEDIA_BUS_* equivalents
> using preprocessor macros, as you've done below. Drop the definition of enum
> media_bus_format, and use u32 (or uint32_t) type for the variables.
> 
> This way the enum stays intact for existing C++ applications, and new
> applications will have to use a 32-bit type.
> 

Fair enough. If Hans agree I'll rework the series and drop the
media_bus_format enum.

Thanks,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com