On Monday 26 July 2010 18:38:28 Laurent Pinchart wrote:
> Hi Hans,
>
> On Saturday 24 July 2010 14:18:11 Hans Verkuil wrote:
> > On Wednesday 21 July 2010 16:35:28 Laurent Pinchart wrote:
> >
> > <snip>
> >
> > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > > new file mode 100644
> > > index 0000000..fd44647
> > > --- /dev/null
> > > +++ b/include/media/media-entity.h
> > > @@ -0,0 +1,79 @@
> > > +#ifndef _MEDIA_ENTITY_H
> > > +#define _MEDIA_ENTITY_H
> > > +
> > > +#include <linux/list.h>
> > > +
> > > +#define MEDIA_ENTITY_TYPE_NODE 1
> > > +#define MEDIA_ENTITY_TYPE_SUBDEV 2
> > > +
> > > +#define MEDIA_ENTITY_SUBTYPE_NODE_V4L 1
> > > +#define MEDIA_ENTITY_SUBTYPE_NODE_FB 2
> > > +#define MEDIA_ENTITY_SUBTYPE_NODE_ALSA 3
> > > +#define MEDIA_ENTITY_SUBTYPE_NODE_DVB 4
> > > +
> > > +#define MEDIA_ENTITY_SUBTYPE_SUBDEV_VID_DECODER 1
> > > +#define MEDIA_ENTITY_SUBTYPE_SUBDEV_VID_ENCODER 2
> > > +#define MEDIA_ENTITY_SUBTYPE_SUBDEV_MISC 3
> >
> > These names are too awkward.
> >
> > I see two options:
> >
> > 1) Rename the type field to 'entity' and the macros to
> > MEDIA_ENTITY_NODE/SUBDEV. Also rename subtype to type and the macros to
> > MEDIA_ENTITY_TYPE_NODE_V4L and MEDIA_ENTITY_TYPE_SUBDEV_VID_DECODER. We
> > might even get away with dropping _TYPE from the macro name.
> >
> > 2) Merge type and subtype to a single entity field. The top 16 bits are the
> > entity type, the bottom 16 bits are the subtype. That way you end up with:
> >
> > #define MEDIA_ENTITY_NODE (1 << 16)
> > #define MEDIA_ENTITY_SUBDEV (2 << 16)
> >
> > #define MEDIA_ENTITY_NODE_V4L (MEDIA_ENTITY_NODE + 1)
> >
> > #define MEDIA_ENTITY_SUBDEV_VID_DECODER (MEDIA_ENTITY_SUBDEV + 1)
> >
> > I rather like this option myself.
>
> I like option 2 better, but I would keep the field name "type" instead of
> "entity". Constants could start with MEDIA_ENTITY_TYPE_, or just MEDIA_ENTITY_
> (I think I would prefer MEDIA_ENTITY_TYPE_).
Yes, I realized that later as well. Keep the 'type' field name.
I'm not sure about the macro name. I still think MEDIA_ENTITY_TYPE_SUBDEV_VID_DECODER
is too much of a mouthful.
>
> > > +
> > > +#define MEDIA_LINK_FLAG_ACTIVE (1 << 0)
> > > +#define MEDIA_LINK_FLAG_IMMUTABLE (1 << 1)
> > > +
> > > +#define MEDIA_PAD_DIR_INPUT 1
> > > +#define MEDIA_PAD_DIR_OUTPUT 2
> > > +
> > > +struct media_entity_link {
> > > + struct media_entity_pad *source;/* Source pad */
> > > + struct media_entity_pad *sink; /* Sink pad */
> > > + struct media_entity_link *other;/* Link in the reverse direction */
> > > + u32 flags; /* Link flags (MEDIA_LINK_FLAG_*) */
> > > +};
> > > +
> > > +struct media_entity_pad {
> > > + struct media_entity *entity; /* Entity this pad belongs to */
> > > + u32 direction; /* Pad direction (MEDIA_PAD_DIR_*) */
> > > + u8 index; /* Pad index in the entity pads array */
> >
> > We can use bitfields for direction and index. That way we can also easily
> > add other flags/attributes.
>
> You proposed to merge the direction field into a new flags field, I suppose
> that should be done here too for consistency. Having 16 flags might be a bit
> low though, 32 would be better. If you want to keep 16 bits for now, maybe we
> should have 2 reserved __u32 instead of one.
Yes, let's use a u32 flags field for this.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
