Re: [PATCH 3/7] [media] vimc: Add vimc_ent_sd_init/cleanup helper functions
Hi Helen,
Thank you for the patch.
On Thursday 06 August 2015 17:26:10 Helen Fornazier wrote:
As all the subdevices in the topology will be initialized in the same
way, to avoid code repetition the vimc_ent_sd_init/cleanup helper functions
were created
Signed-off-by: Helen Fornazier helen.fornaz...@gmail.com
---
drivers/media/platform/vimc/vimc-core.c | 76 ++
drivers/media/platform/vimc/vimc-core.h | 17 +
drivers/media/platform/vimc/vimc-sensor.c | 104 --
3 files changed, 123 insertions(+), 74 deletions(-)
diff --git a/drivers/media/platform/vimc/vimc-core.c
b/drivers/media/platform/vimc/vimc-core.c index 3ef9b51..96d53fd 100644
--- a/drivers/media/platform/vimc/vimc-core.c
+++ b/drivers/media/platform/vimc/vimc-core.c
@@ -332,6 +332,82 @@ struct media_pad *vimc_pads_init(u16 num_pads, const
unsigned long *pads_flag) return pads;
}
+/* media operations */
+static const struct media_entity_operations vimc_ent_sd_mops = {
+ .link_validate = v4l2_subdev_link_validate,
+};
+
+void vimc_ent_sd_cleanup(struct vimc_ent_subdevice *vsd)
+{
+ media_entity_cleanup(vsd-ved.ent);
+ v4l2_device_unregister_subdev(vsd-sd);
v4l2_device_unregister_subdev() calls media_entity_remove_links() and
media_device_unregister_entity(), so it would be better to call
media_entity_cleanup() after v4l2_device_unregister_subdev().
+ kfree(vsd);
+}
+
+struct vimc_ent_subdevice *vimc_ent_sd_init(size_t struct_size,
+ struct v4l2_device *v4l2_dev,
+ const char *const name,
+ u16 num_pads,
+ const unsigned long *pads_flag,
+ const struct v4l2_subdev_ops *sd_ops,
+ void (*sd_destroy)(struct vimc_ent_device *))
+{
+ int ret;
+ struct vimc_ent_subdevice *vsd;
+
+ if (!v4l2_dev || !v4l2_dev-dev || !name || (num_pads !pads_flag))
+ return ERR_PTR(-EINVAL);
+
+ /* Allocate the vsd struct */
+ vsd = kzalloc(struct_size, GFP_KERNEL);
This requires vimc_ent_subdevice being embedded as the first field of the
parent structure, which restricts options for the code using this API. If you
were implementing a subsystem API I'd ask for the allocation to happen outside
of this function, with the vimc_ent_subdevice pointer being passed to the
function. However, as this is local to the vimc driver, if you believe it
won't be an issue in the foreseeable future then I'm fine with the
implementation.
By the way, such core functions should really be documented with a kerneldoc
comment block. Requirements such as the embedding field position isn't very
apparent when you glance at the code and would benefit from being documented.
+ if (!vsd)
+ return ERR_PTR(-ENOMEM);
+
+ /* Link the vimc_deb_device struct with the v4l2 parent */
+ vsd-v4l2_dev = v4l2_dev;
+ /* Link the vimc_deb_device struct with the dev parent */
+ vsd-dev = v4l2_dev-dev;
+
+ /* Allocate the pads */
+ vsd-ved.pads = vimc_pads_init(num_pads, pads_flag);
+ if (IS_ERR(vsd-ved.pads)) {
+ ret = PTR_ERR(vsd-ved.pads);
+ goto err_free_vsd;
+ }
+
+ /* Initialize the media entity */
+ vsd-sd.entity.name = name;
+ vsd-sd.entity.type = MEDIA_ENT_T_V4L2_SUBDEV;
+ ret = media_entity_init(vsd-sd.entity, num_pads,
+ vsd-ved.pads, num_pads);
+ if (ret)
+ goto err_clean_pads;
+
+ /* Fill the vimc_ent_device struct */
+ vsd-ved.destroy = sd_destroy;
+ vsd-ved.ent = vsd-sd.entity;
+
+ /* Initialize the subdev */
+ v4l2_subdev_init(vsd-sd, sd_ops);
+ vsd-sd.entity.ops = vimc_ent_sd_mops;
+ vsd-sd.owner = THIS_MODULE;
+ strlcpy(vsd-sd.name, name, sizeof(vsd-sd.name));
+ v4l2_set_subdevdata(vsd-sd, vsd);
+
+ /* Expose this subdev to user space */
+ vsd-sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
+
+ /* return the created vimc subdevice*/
+ return vsd;
+
+err_clean_pads:
+ vimc_pads_cleanup(vsd-ved.pads);
+err_free_vsd:
+ kfree(vsd);
+
+ return ERR_PTR(ret);
+}
+
/* TODO: remove this function when all the
* entities specific code are implemented */
static void vimc_raw_destroy(struct vimc_ent_device *ved)
diff --git a/drivers/media/platform/vimc/vimc-core.h
b/drivers/media/platform/vimc/vimc-core.h index be05469..295a554 100644
--- a/drivers/media/platform/vimc/vimc-core.h
+++ b/drivers/media/platform/vimc/vimc-core.h
@@ -37,6 +37,13 @@ struct vimc_ent_device {
struct media_pad *sink, const void *frame);
};
+struct vimc_ent_subdevice {
+ struct vimc_ent_device ved;
+ struct v4l2_subdev sd;
+ struct v4l2_device *v4l2_dev;
Given that struct v4l2_subdev stores a pointer to the same struct
[PATCH 3/7] [media] vimc: Add vimc_ent_sd_init/cleanup helper functions
As all the subdevices in the topology will be initialized in the same
way, to avoid code repetition the vimc_ent_sd_init/cleanup helper functions
were created
Signed-off-by: Helen Fornazier helen.fornaz...@gmail.com
---
drivers/media/platform/vimc/vimc-core.c | 76 ++
drivers/media/platform/vimc/vimc-core.h | 17 +
drivers/media/platform/vimc/vimc-sensor.c | 104 +-
3 files changed, 123 insertions(+), 74 deletions(-)
diff --git a/drivers/media/platform/vimc/vimc-core.c
b/drivers/media/platform/vimc/vimc-core.c
index 3ef9b51..96d53fd 100644
--- a/drivers/media/platform/vimc/vimc-core.c
+++ b/drivers/media/platform/vimc/vimc-core.c
@@ -332,6 +332,82 @@ struct media_pad *vimc_pads_init(u16 num_pads, const
unsigned long *pads_flag)
return pads;
}
+/* media operations */
+static const struct media_entity_operations vimc_ent_sd_mops = {
+ .link_validate = v4l2_subdev_link_validate,
+};
+
+void vimc_ent_sd_cleanup(struct vimc_ent_subdevice *vsd)
+{
+ media_entity_cleanup(vsd-ved.ent);
+ v4l2_device_unregister_subdev(vsd-sd);
+ kfree(vsd);
+}
+
+struct vimc_ent_subdevice *vimc_ent_sd_init(size_t struct_size,
+ struct v4l2_device *v4l2_dev,
+ const char *const name,
+ u16 num_pads,
+ const unsigned long *pads_flag,
+ const struct v4l2_subdev_ops *sd_ops,
+ void (*sd_destroy)(struct vimc_ent_device *))
+{
+ int ret;
+ struct vimc_ent_subdevice *vsd;
+
+ if (!v4l2_dev || !v4l2_dev-dev || !name || (num_pads !pads_flag))
+ return ERR_PTR(-EINVAL);
+
+ /* Allocate the vsd struct */
+ vsd = kzalloc(struct_size, GFP_KERNEL);
+ if (!vsd)
+ return ERR_PTR(-ENOMEM);
+
+ /* Link the vimc_deb_device struct with the v4l2 parent */
+ vsd-v4l2_dev = v4l2_dev;
+ /* Link the vimc_deb_device struct with the dev parent */
+ vsd-dev = v4l2_dev-dev;
+
+ /* Allocate the pads */
+ vsd-ved.pads = vimc_pads_init(num_pads, pads_flag);
+ if (IS_ERR(vsd-ved.pads)) {
+ ret = PTR_ERR(vsd-ved.pads);
+ goto err_free_vsd;
+ }
+
+ /* Initialize the media entity */
+ vsd-sd.entity.name = name;
+ vsd-sd.entity.type = MEDIA_ENT_T_V4L2_SUBDEV;
+ ret = media_entity_init(vsd-sd.entity, num_pads,
+ vsd-ved.pads, num_pads);
+ if (ret)
+ goto err_clean_pads;
+
+ /* Fill the vimc_ent_device struct */
+ vsd-ved.destroy = sd_destroy;
+ vsd-ved.ent = vsd-sd.entity;
+
+ /* Initialize the subdev */
+ v4l2_subdev_init(vsd-sd, sd_ops);
+ vsd-sd.entity.ops = vimc_ent_sd_mops;
+ vsd-sd.owner = THIS_MODULE;
+ strlcpy(vsd-sd.name, name, sizeof(vsd-sd.name));
+ v4l2_set_subdevdata(vsd-sd, vsd);
+
+ /* Expose this subdev to user space */
+ vsd-sd.flags = V4L2_SUBDEV_FL_HAS_DEVNODE;
+
+ /* return the created vimc subdevice*/
+ return vsd;
+
+err_clean_pads:
+ vimc_pads_cleanup(vsd-ved.pads);
+err_free_vsd:
+ kfree(vsd);
+
+ return ERR_PTR(ret);
+}
+
/* TODO: remove this function when all the
* entities specific code are implemented */
static void vimc_raw_destroy(struct vimc_ent_device *ved)
diff --git a/drivers/media/platform/vimc/vimc-core.h
b/drivers/media/platform/vimc/vimc-core.h
index be05469..295a554 100644
--- a/drivers/media/platform/vimc/vimc-core.h
+++ b/drivers/media/platform/vimc/vimc-core.h
@@ -37,6 +37,13 @@ struct vimc_ent_device {
struct media_pad *sink, const void *frame);
};
+struct vimc_ent_subdevice {
+ struct vimc_ent_device ved;
+ struct v4l2_subdev sd;
+ struct v4l2_device *v4l2_dev;
+ struct device *dev;
+};
+
int vimc_propagate_frame(struct device *dev,
struct media_pad *src, const void *frame);
@@ -48,6 +55,16 @@ static inline void vimc_pads_cleanup(struct media_pad *pads)
kfree(pads);
}
+/* Helper function to initialize/cleanup a subdevice used */
+struct vimc_ent_subdevice *vimc_ent_sd_init(size_t struct_size,
+ struct v4l2_device *v4l2_dev,
+ const char *const name,
+ u16 num_pads,
+ const unsigned long *pads_flag,
+ const struct v4l2_subdev_ops *sd_ops,
+ void (*sd_destroy)(struct vimc_ent_device *));
+void vimc_ent_sd_cleanup(struct vimc_ent_subdevice *vsd);
+
const struct vimc_pix_map *vimc_pix_map_by_code(u32 code);
const struct vimc_pix_map *vimc_pix_map_by_pixelformat(u32 pixelformat);
diff --git a/drivers/media/platform/vimc/vimc-sensor.c
