Hi Mauro,
On Sun, Dec 13, 2015 at 10:54:57PM -0200, Mauro Carvalho Chehab wrote:
> > >> + e->idx_max = idx_max;
> > >> +
> > >> + return 0;
> > >> +}
> > >> +EXPORT_SYMBOL_GPL(__media_entity_enum_init);
> > >> +
> > >> +/**
> > >> + * media_entity_enum_cleanup - Release resources of an entity enumeration
> > >> + *
> > >> + * @e: Entity enumeration to be released
> > >> + */
> > >> +void media_entity_enum_cleanup(struct media_entity_enum *e)
> > >> +{
> > >> + if (e->e != e->__e)
> > >> + kfree(e->e);
> > >> + e->e = NULL;
> > >> +}
> > >> +EXPORT_SYMBOL_GPL(media_entity_enum_cleanup);
> > >> +
> > >> +/**
> > >> * media_entity_init - Initialize a media entity
> > >> *
> > >> * @num_pads: Total number of sink and source pads.
> > >> diff --git a/include/media/media-device.h b/include/media/media-device.h
> > >> index c0e1764..2d46c66 100644
> > >> --- a/include/media/media-device.h
> > >> +++ b/include/media/media-device.h
> > >> @@ -110,6 +110,20 @@ struct media_device {
> > >> /* media_devnode to media_device */
> > >> #define to_media_device(node) container_of(node, struct media_device, devnode)
> > >>
> > >> +/**
> > >> + * media_entity_enum_init - Initialise an entity enumeration
> > >> + *
> > >> + * @e: Entity enumeration to be initialised
> > >> + * @mdev: The related media device
> > >> + *
> > >> + * Returns zero on success or a negative error code.
> > >> + */
> > >> +static inline __must_check int media_entity_enum_init(
> > >> + struct media_entity_enum *e, struct media_device *mdev)
> > >> +{
> > >> + return __media_entity_enum_init(e, mdev->entity_internal_idx_max + 1);
> > >> +}
> > >> +
> > >> void media_device_init(struct media_device *mdev);
> > >> void media_device_cleanup(struct media_device *mdev);
> > >> int __must_check __media_device_register(struct media_device *mdev,
> > >> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > >> index d3d3a39..5a0339a 100644
> > >> --- a/include/media/media-entity.h
> > >> +++ b/include/media/media-entity.h
> > >> @@ -23,7 +23,7 @@
> > >> #ifndef _MEDIA_ENTITY_H
> > >> #define _MEDIA_ENTITY_H
> > >>
> > >> -#include <linux/bitops.h>
> > >> +#include <linux/bitmap.h>
> > >> #include <linux/kernel.h>
> > >> #include <linux/list.h>
> > >> #include <linux/media.h>
> > >> @@ -71,6 +71,30 @@ struct media_gobj {
> > >> struct list_head list;
> > >> };
> > >>
> > >> +#define MEDIA_ENTITY_ENUM_MAX_DEPTH 16
> > >> +#define MEDIA_ENTITY_ENUM_MAX_ID 64
> > >> +
> > >> +/*
> > >> + * The number of pads can't be bigger than the number of entities,
> > >> + * as the worse-case scenario is to have one entity linked up to
> > >> + * MEDIA_ENTITY_ENUM_MAX_ID - 1 entities.
> > >> + */
> > >> +#define MEDIA_ENTITY_MAX_PADS (MEDIA_ENTITY_ENUM_MAX_ID - 1)
> > >> +
> > >> +/* struct media_entity_enum - An enumeration of media entities.
> > >> + *
> > >> + * @__e: Pre-allocated space reserved for media entities if the total
> > >> + * number of entities does not exceed MEDIA_ENTITY_ENUM_MAX_ID.
> > >> + * @e: Bit mask in which each bit represents one entity at struct
> > >> + * media_entity->internal_idx.
> > >> + * @idx_max: Number of bits in the enum.
> > >> + */
> > >> +struct media_entity_enum {
> > >> + DECLARE_BITMAP(__e, MEDIA_ENTITY_ENUM_MAX_ID);
> > >> + unsigned long *e;
> > >
> > > As mentioned before, don't use single letter names, specially inside
> > > publicly used structures. There's no good reason for that, and the
> > > code become really obscure.
> > >
> > > Also, just declare one bitmap. having a "pre-allocated" hardcoded
> > > one here is very confusing.
> >
> > I prefer to keep it as allocating a few bytes of memory is silly. In the
> > vast majority of the cases the pre-allocated array will be sufficient.
>
> "vast majority" actually depends on the driver/subsystem. The fact that
> right now most drivers work fine with < 64 entities may not be
> true in the future.
One reason the pre-allocated bitmap shouldn't be removed yet is that by
doing that, i.e. allocating memory in media_entity_enum_init(), the user
must check the return code. The further patches in the set make drivers do
that add __must_check modifier to the prototype.
I will thus add another patch near the top to remove the pre-allocated
bitmap.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
--
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
