linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] media: vimc: various fixes
@ 2020-03-28  7:52 Dafna Hirschfeld
  2020-03-28  7:52 ` [PATCH 1/5] media: vimc: remove the function vimc_unregister Dafna Hirschfeld
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Dafna Hirschfeld @ 2020-03-28  7:52 UTC (permalink / raw)
  To: linux-media, dafna.hirschfeld, helen.koike, ezequiel, hverkuil,
	kernel, dafna3, mchehab, laurent.pinchart

Various fixes and improvements for vimc driver.

Patches summary:

patch 1: remove the function vimc_unregister
patch 2: if vimc_add_subdevs fails then the cleanup should be inside the function
patch 3: return the error code if a callback fails instead of NULL
patch 4: fix some typos in the docs
patch 5: adds a struct vimc_cap_type that holds the callbacks of each vimc entity type,
since for each type the set of callbacks is always the same.

Dafna Hirschfeld (5):
  media: vimc: remove the function vimc_unregister
  media: vimc: handle error in vimc_add_subdevs
  media: vimc: keep the error value when adding an entity fails
  media: vimc: fix issues in documentation in vimc-common.h
  media: vimc: add vimc_ent_type struct for the callbacks of entities

 drivers/media/platform/vimc/vimc-capture.c | 10 ++-
 drivers/media/platform/vimc/vimc-common.h  | 53 ++++++-------
 drivers/media/platform/vimc/vimc-core.c    | 90 ++++++++++------------
 drivers/media/platform/vimc/vimc-debayer.c |  9 ++-
 drivers/media/platform/vimc/vimc-scaler.c  |  9 ++-
 drivers/media/platform/vimc/vimc-sensor.c  |  9 ++-
 6 files changed, 93 insertions(+), 87 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/5] media: vimc: remove the function vimc_unregister
  2020-03-28  7:52 [PATCH 0/5] media: vimc: various fixes Dafna Hirschfeld
@ 2020-03-28  7:52 ` Dafna Hirschfeld
  2020-03-28  7:52 ` [PATCH 2/5] media: vimc: handle error in vimc_add_subdevs Dafna Hirschfeld
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Dafna Hirschfeld @ 2020-03-28  7:52 UTC (permalink / raw)
  To: linux-media, dafna.hirschfeld, helen.koike, ezequiel, hverkuil,
	kernel, dafna3, mchehab, laurent.pinchart

The function vimc_unregister is called only from
one place in the code and has only 3 lines so it
has no justification.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/media/platform/vimc/vimc-core.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
index 339126e565dc..1f8498837646 100644
--- a/drivers/media/platform/vimc/vimc-core.c
+++ b/drivers/media/platform/vimc/vimc-core.c
@@ -268,13 +268,6 @@ static int vimc_register_devices(struct vimc_device *vimc)
 	return ret;
 }
 
-static void vimc_unregister(struct vimc_device *vimc)
-{
-	vimc_unregister_subdevs(vimc);
-	media_device_unregister(&vimc->mdev);
-	v4l2_device_unregister(&vimc->v4l2_dev);
-}
-
 static int vimc_probe(struct platform_device *pdev)
 {
 	struct vimc_device *vimc;
@@ -321,7 +314,9 @@ static int vimc_remove(struct platform_device *pdev)
 
 	dev_dbg(&pdev->dev, "remove");
 
-	vimc_unregister(vimc);
+	vimc_unregister_subdevs(vimc);
+	media_device_unregister(&vimc->mdev);
+	v4l2_device_unregister(&vimc->v4l2_dev);
 	v4l2_device_put(&vimc->v4l2_dev);
 
 	return 0;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/5] media: vimc: handle error in vimc_add_subdevs
  2020-03-28  7:52 [PATCH 0/5] media: vimc: various fixes Dafna Hirschfeld
  2020-03-28  7:52 ` [PATCH 1/5] media: vimc: remove the function vimc_unregister Dafna Hirschfeld
@ 2020-03-28  7:52 ` Dafna Hirschfeld
  2020-03-28  7:52 ` [PATCH 3/5] media: vimc: keep the error value when adding an entity fails Dafna Hirschfeld
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Dafna Hirschfeld @ 2020-03-28  7:52 UTC (permalink / raw)
  To: linux-media, dafna.hirschfeld, helen.koike, ezequiel, hverkuil,
	kernel, dafna3, mchehab, laurent.pinchart

In case the 'add' callback of an entity fails,
then all other entities should unregister and released.
This should be done inside vimc_add_subdevs so that
the function handles its own failure.

In order to call vimc_unregister_subdevs and vimc_release_subdevs from
vimc_add_subdevs, the order of the function should change.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/media/platform/vimc/vimc-core.c | 42 +++++++++++++------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
index 1f8498837646..dab01cbc31d2 100644
--- a/drivers/media/platform/vimc/vimc-core.c
+++ b/drivers/media/platform/vimc/vimc-core.c
@@ -160,24 +160,6 @@ static int vimc_create_links(struct vimc_device *vimc)
 	return ret;
 }
 
-static int vimc_add_subdevs(struct vimc_device *vimc)
-{
-	unsigned int i;
-
-	for (i = 0; i < vimc->pipe_cfg->num_ents; i++) {
-		dev_dbg(vimc->mdev.dev, "new entity for %s\n",
-			vimc->pipe_cfg->ents[i].name);
-		vimc->ent_devs[i] = vimc->pipe_cfg->ents[i].add(vimc,
-					vimc->pipe_cfg->ents[i].name);
-		if (!vimc->ent_devs[i]) {
-			dev_err(vimc->mdev.dev, "add new entity for %s\n",
-				vimc->pipe_cfg->ents[i].name);
-			return -EINVAL;
-		}
-	}
-	return 0;
-}
-
 static void vimc_release_subdevs(struct vimc_device *vimc)
 {
 	unsigned int i;
@@ -196,6 +178,26 @@ static void vimc_unregister_subdevs(struct vimc_device *vimc)
 			vimc->pipe_cfg->ents[i].unregister(vimc->ent_devs[i]);
 }
 
+static int vimc_add_subdevs(struct vimc_device *vimc)
+{
+	unsigned int i;
+
+	for (i = 0; i < vimc->pipe_cfg->num_ents; i++) {
+		dev_dbg(vimc->mdev.dev, "new entity for %s\n",
+			vimc->pipe_cfg->ents[i].name);
+		vimc->ent_devs[i] = vimc->pipe_cfg->ents[i].add(vimc,
+					vimc->pipe_cfg->ents[i].name);
+		if (!vimc->ent_devs[i]) {
+			dev_err(vimc->mdev.dev, "add new entity for %s\n",
+				vimc->pipe_cfg->ents[i].name);
+			vimc_unregister_subdevs(vimc);
+			vimc_release_subdevs(vimc);
+			return -EINVAL;
+		}
+	}
+	return 0;
+}
+
 static void vimc_v4l2_dev_release(struct v4l2_device *v4l2_dev)
 {
 	struct vimc_device *vimc =
@@ -229,8 +231,7 @@ static int vimc_register_devices(struct vimc_device *vimc)
 	/* Invoke entity config hooks to initialize and register subdevs */
 	ret = vimc_add_subdevs(vimc);
 	if (ret)
-		/* remove sundevs that got added */
-		goto err_rm_subdevs;
+		goto err_free_ent_devs;
 
 	/* Initialize links */
 	ret = vimc_create_links(vimc);
@@ -261,6 +262,7 @@ static int vimc_register_devices(struct vimc_device *vimc)
 err_rm_subdevs:
 	vimc_unregister_subdevs(vimc);
 	vimc_release_subdevs(vimc);
+err_free_ent_devs:
 	kfree(vimc->ent_devs);
 err_v4l2_unregister:
 	v4l2_device_unregister(&vimc->v4l2_dev);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/5] media: vimc: keep the error value when adding an entity fails
  2020-03-28  7:52 [PATCH 0/5] media: vimc: various fixes Dafna Hirschfeld
  2020-03-28  7:52 ` [PATCH 1/5] media: vimc: remove the function vimc_unregister Dafna Hirschfeld
  2020-03-28  7:52 ` [PATCH 2/5] media: vimc: handle error in vimc_add_subdevs Dafna Hirschfeld
@ 2020-03-28  7:52 ` Dafna Hirschfeld
  2020-03-28  7:52 ` [PATCH 4/5] media: vimc: fix issues in documentation in vimc-common.h Dafna Hirschfeld
  2020-03-28  7:52 ` [PATCH 5/5] media: vimc: add vimc_ent_type struct for the callbacks of entities Dafna Hirschfeld
  4 siblings, 0 replies; 9+ messages in thread
From: Dafna Hirschfeld @ 2020-03-28  7:52 UTC (permalink / raw)
  To: linux-media, dafna.hirschfeld, helen.koike, ezequiel, hverkuil,
	kernel, dafna3, mchehab, laurent.pinchart

Currently when the 'add' callback of an entity fails, a
NULL is returned. This hides the error code of the failure
and always returns -EINVAL.

Replace return NULL with return ERR_PTR(ret) to improve debugging.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/media/platform/vimc/vimc-capture.c |  4 ++--
 drivers/media/platform/vimc/vimc-core.c    | 11 +++++++----
 drivers/media/platform/vimc/vimc-debayer.c |  4 ++--
 drivers/media/platform/vimc/vimc-scaler.c  |  4 ++--
 drivers/media/platform/vimc/vimc-sensor.c  |  4 ++--
 5 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index 23e740c1c5c0..8bafbf90cbf5 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -395,7 +395,7 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
 	/* Allocate the vimc_cap_device struct */
 	vcap = kzalloc(sizeof(*vcap), GFP_KERNEL);
 	if (!vcap)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	/* Initialize the media entity */
 	vcap->vdev.entity.name = vcfg_name;
@@ -476,5 +476,5 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
 err_free_vcap:
 	kfree(vcap);
 
-	return NULL;
+	return ERR_PTR(ret);
 }
diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
index dab01cbc31d2..edac998f58d5 100644
--- a/drivers/media/platform/vimc/vimc-core.c
+++ b/drivers/media/platform/vimc/vimc-core.c
@@ -187,12 +187,15 @@ static int vimc_add_subdevs(struct vimc_device *vimc)
 			vimc->pipe_cfg->ents[i].name);
 		vimc->ent_devs[i] = vimc->pipe_cfg->ents[i].add(vimc,
 					vimc->pipe_cfg->ents[i].name);
-		if (!vimc->ent_devs[i]) {
-			dev_err(vimc->mdev.dev, "add new entity for %s\n",
-				vimc->pipe_cfg->ents[i].name);
+		if (IS_ERR(vimc->ent_devs[i])) {
+			int err = PTR_ERR(vimc->ent_devs[i]);
+
+			dev_err(vimc->mdev.dev, "adding entity %s failed (%d)\n",
+				vimc->pipe_cfg->ents[i].name, err);
+			vimc->ent_devs[i] = NULL;
 			vimc_unregister_subdevs(vimc);
 			vimc_release_subdevs(vimc);
-			return -EINVAL;
+			return err;
 		}
 	}
 	return 0;
diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
index baf6bf9f65b5..d591b6a6bcf0 100644
--- a/drivers/media/platform/vimc/vimc-debayer.c
+++ b/drivers/media/platform/vimc/vimc-debayer.c
@@ -532,7 +532,7 @@ struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
 	/* Allocate the vdeb struct */
 	vdeb = kzalloc(sizeof(*vdeb), GFP_KERNEL);
 	if (!vdeb)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	/* Create controls: */
 	v4l2_ctrl_handler_init(&vdeb->hdl, 2);
@@ -577,5 +577,5 @@ struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
 err_free_vdeb:
 	kfree(vdeb);
 
-	return NULL;
+	return ERR_PTR(ret);
 }
diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
index 7521439747c5..6ea05dcebc31 100644
--- a/drivers/media/platform/vimc/vimc-scaler.c
+++ b/drivers/media/platform/vimc/vimc-scaler.c
@@ -483,7 +483,7 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
 	/* Allocate the vsca struct */
 	vsca = kzalloc(sizeof(*vsca), GFP_KERNEL);
 	if (!vsca)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	/* Initialize ved and sd */
 	vsca->pads[0].flags = MEDIA_PAD_FL_SINK;
@@ -495,7 +495,7 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
 				   vsca->pads, &vimc_sca_ops);
 	if (ret) {
 		kfree(vsca);
-		return NULL;
+		return ERR_PTR(ret);
 	}
 
 	vsca->ved.process_frame = vimc_sca_process_frame;
diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
index 92daee58209e..24ce5b70b97e 100644
--- a/drivers/media/platform/vimc/vimc-sensor.c
+++ b/drivers/media/platform/vimc/vimc-sensor.c
@@ -317,7 +317,7 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
 	/* Allocate the vsen struct */
 	vsen = kzalloc(sizeof(*vsen), GFP_KERNEL);
 	if (!vsen)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	v4l2_ctrl_handler_init(&vsen->hdl, 4);
 
@@ -372,5 +372,5 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
 err_free_vsen:
 	kfree(vsen);
 
-	return NULL;
+	return ERR_PTR(ret);
 }
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 4/5] media: vimc: fix issues in documentation in vimc-common.h
  2020-03-28  7:52 [PATCH 0/5] media: vimc: various fixes Dafna Hirschfeld
                   ` (2 preceding siblings ...)
  2020-03-28  7:52 ` [PATCH 3/5] media: vimc: keep the error value when adding an entity fails Dafna Hirschfeld
@ 2020-03-28  7:52 ` Dafna Hirschfeld
  2020-03-28  7:52 ` [PATCH 5/5] media: vimc: add vimc_ent_type struct for the callbacks of entities Dafna Hirschfeld
  4 siblings, 0 replies; 9+ messages in thread
From: Dafna Hirschfeld @ 2020-03-28  7:52 UTC (permalink / raw)
  To: linux-media, dafna.hirschfeld, helen.koike, ezequiel, hverkuil,
	kernel, dafna3, mchehab, laurent.pinchart

There are some missing and extra fields and typos in
structs documentations in vimc-common.h. Fix it.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/media/platform/vimc/vimc-common.h | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
index 616d5a6b0754..40a58f1cb18f 100644
--- a/drivers/media/platform/vimc/vimc-common.h
+++ b/drivers/media/platform/vimc/vimc-common.h
@@ -63,7 +63,8 @@ do {									\
  *
  * @code:		media bus format code defined by MEDIA_BUS_FMT_* macros
  * @bbp:		number of bytes each pixel occupies
- * @pixelformat:	pixel format devined by V4L2_PIX_FMT_* macros
+ * @pixelformat:	pixel format defined by V4L2_PIX_FMT_* macros
+ * @bayer		true if this is a bayer format
  *
  * Struct which matches the MEDIA_BUS_FMT_* codes with the corresponding
  * V4L2_PIX_FMT_* fourcc pixelformat and its bytes per pixel (bpp)
@@ -90,7 +91,7 @@ struct vimc_pix_map {
  * the node it will be of an instance of v4l2_subdev or video_device struct
  * where both contains a struct media_entity.
  * Those structures should embedded the vimc_ent_device struct through
- * v4l2_set_subdevdata() and video_set_drvdata() respectivaly, allowing the
+ * v4l2_set_subdevdata() and video_set_drvdata() respectively, allowing the
  * vimc_ent_device struct to be retrieved from the corresponding struct
  * media_entity
  */
@@ -123,10 +124,8 @@ struct vimc_device {
  *				configuration for each entity
  *
  * @name			entity name
- * @ved				pointer to vimc_ent_device (a node in the
- *					topology)
  * @add				initializes and registers
- *					vim entity - called from vimc-core
+ *					vimc entity - called from vimc-core
  * @unregister			unregisters vimc entity - called from vimc-core
  * @release			releases vimc entity - called from the v4l2_dev
  *					release callback
@@ -182,7 +181,7 @@ const struct vimc_pix_map *vimc_pix_map_by_code(u32 code);
 /**
  * vimc_pix_map_by_pixelformat - get vimc_pix_map struct by v4l2 pixel format
  *
- * @pixelformat:	pixel format devined by V4L2_PIX_FMT_* macros
+ * @pixelformat:	pixel format defined by V4L2_PIX_FMT_* macros
  */
 const struct vimc_pix_map *vimc_pix_map_by_pixelformat(u32 pixelformat);
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 5/5] media: vimc: add vimc_ent_type struct for the callbacks of entities
  2020-03-28  7:52 [PATCH 0/5] media: vimc: various fixes Dafna Hirschfeld
                   ` (3 preceding siblings ...)
  2020-03-28  7:52 ` [PATCH 4/5] media: vimc: fix issues in documentation in vimc-common.h Dafna Hirschfeld
@ 2020-03-28  7:52 ` Dafna Hirschfeld
  2020-03-30 12:10   ` Hans Verkuil
  4 siblings, 1 reply; 9+ messages in thread
From: Dafna Hirschfeld @ 2020-03-28  7:52 UTC (permalink / raw)
  To: linux-media, dafna.hirschfeld, helen.koike, ezequiel, hverkuil,
	kernel, dafna3, mchehab, laurent.pinchart

Since each vimc entity type is defined by the
callbacks implementation, it is a good idea to
add a struct to hold these callbacks.

Each vimc entity then declare its type in the file for
the entity.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/media/platform/vimc/vimc-capture.c |  6 ++++
 drivers/media/platform/vimc/vimc-common.h  | 42 +++++++++++-----------
 drivers/media/platform/vimc/vimc-core.c    | 38 +++++++-------------
 drivers/media/platform/vimc/vimc-debayer.c |  5 +++
 drivers/media/platform/vimc/vimc-scaler.c  |  5 +++
 drivers/media/platform/vimc/vimc-sensor.c  |  5 +++
 6 files changed, 54 insertions(+), 47 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index 8bafbf90cbf5..230f024ff896 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -478,3 +478,9 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
 
 	return ERR_PTR(ret);
 }
+
+struct vimc_ent_type vimc_cap_type = {
+	.add = vimc_cap_add,
+	.unregister = vimc_cap_unregister,
+	.release = vimc_cap_release
+};
diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
index 40a58f1cb18f..b2fa3b793916 100644
--- a/drivers/media/platform/vimc/vimc-common.h
+++ b/drivers/media/platform/vimc/vimc-common.h
@@ -120,24 +120,35 @@ struct vimc_device {
 };
 
 /**
- * struct vimc_ent_config	Structure which describes individual
- *				configuration for each entity
+ * struct vimc_ent_type		Structure for the callbacks of the entity types
+ *
  *
- * @name			entity name
  * @add				initializes and registers
  *					vimc entity - called from vimc-core
  * @unregister			unregisters vimc entity - called from vimc-core
  * @release			releases vimc entity - called from the v4l2_dev
  *					release callback
  */
-struct vimc_ent_config {
-	const char *name;
+struct vimc_ent_type {
 	struct vimc_ent_device *(*add)(struct vimc_device *vimc,
 				       const char *vcfg_name);
 	void (*unregister)(struct vimc_ent_device *ved);
 	void (*release)(struct vimc_ent_device *ved);
 };
 
+/**
+ * struct vimc_ent_config	Structure which describes individual
+ *				configuration for each entity
+ *
+ * @name			entity name
+ * @type			contain the callbacks of this entity type
+ *
+ */
+struct vimc_ent_config {
+	const char *name;
+	struct vimc_ent_type *type;
+};
+
 /**
  * vimc_is_source - returns true if the entity has only source pads
  *
@@ -146,23 +157,10 @@ struct vimc_ent_config {
  */
 bool vimc_is_source(struct media_entity *ent);
 
-/* prototypes for vimc_ent_config hooks */
-struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
-				     const char *vcfg_name);
-void vimc_cap_unregister(struct vimc_ent_device *ved);
-void vimc_cap_release(struct vimc_ent_device *ved);
-
-struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
-				     const char *vcfg_name);
-void vimc_deb_release(struct vimc_ent_device *ved);
-
-struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
-				     const char *vcfg_name);
-void vimc_sca_release(struct vimc_ent_device *ved);
-
-struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
-				     const char *vcfg_name);
-void vimc_sen_release(struct vimc_ent_device *ved);
+extern struct vimc_ent_type vimc_sen_type;
+extern struct vimc_ent_type vimc_deb_type;
+extern struct vimc_ent_type vimc_sca_type;
+extern struct vimc_ent_type vimc_cap_type;
 
 /**
  * vimc_pix_map_by_index - get vimc_pix_map struct by its index
diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
index edac998f58d5..11210aaa2551 100644
--- a/drivers/media/platform/vimc/vimc-core.c
+++ b/drivers/media/platform/vimc/vimc-core.c
@@ -47,52 +47,40 @@ struct vimc_pipeline_config {
 static struct vimc_ent_config ent_config[] = {
 	{
 		.name = "Sensor A",
-		.add = vimc_sen_add,
-		.release = vimc_sen_release,
+		.type = &vimc_sen_type
 	},
 	{
 		.name = "Sensor B",
-		.add = vimc_sen_add,
-		.release = vimc_sen_release,
+		.type = &vimc_sen_type
 	},
 	{
 		.name = "Debayer A",
-		.add = vimc_deb_add,
-		.release = vimc_deb_release,
+		.type = &vimc_deb_type
 	},
 	{
 		.name = "Debayer B",
-		.add = vimc_deb_add,
-		.release = vimc_deb_release,
+		.type = &vimc_deb_type
 	},
 	{
 		.name = "Raw Capture 0",
-		.add = vimc_cap_add,
-		.unregister = vimc_cap_unregister,
-		.release = vimc_cap_release,
+		.type = &vimc_cap_type
 	},
 	{
 		.name = "Raw Capture 1",
-		.add = vimc_cap_add,
-		.unregister = vimc_cap_unregister,
-		.release = vimc_cap_release,
+		.type = &vimc_cap_type
 	},
 	{
 		/* TODO: change this to vimc-input when it is implemented */
 		.name = "RGB/YUV Input",
-		.add = vimc_sen_add,
-		.release = vimc_sen_release,
+		.type = &vimc_sen_type
 	},
 	{
 		.name = "Scaler",
-		.add = vimc_sca_add,
-		.release = vimc_sca_release,
+		.type = &vimc_sca_type
 	},
 	{
 		.name = "RGB/YUV Capture",
-		.add = vimc_cap_add,
-		.unregister = vimc_cap_unregister,
-		.release = vimc_cap_release,
+		.type = &vimc_cap_type
 	},
 };
 
@@ -166,7 +154,7 @@ static void vimc_release_subdevs(struct vimc_device *vimc)
 
 	for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
 		if (vimc->ent_devs[i])
-			vimc->pipe_cfg->ents[i].release(vimc->ent_devs[i]);
+			vimc->pipe_cfg->ents[i].type->release(vimc->ent_devs[i]);
 }
 
 static void vimc_unregister_subdevs(struct vimc_device *vimc)
@@ -174,8 +162,8 @@ static void vimc_unregister_subdevs(struct vimc_device *vimc)
 	unsigned int i;
 
 	for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
-		if (vimc->ent_devs[i] && vimc->pipe_cfg->ents[i].unregister)
-			vimc->pipe_cfg->ents[i].unregister(vimc->ent_devs[i]);
+		if (vimc->ent_devs[i] && vimc->pipe_cfg->ents[i].type->unregister)
+			vimc->pipe_cfg->ents[i].type->unregister(vimc->ent_devs[i]);
 }
 
 static int vimc_add_subdevs(struct vimc_device *vimc)
@@ -185,7 +173,7 @@ static int vimc_add_subdevs(struct vimc_device *vimc)
 	for (i = 0; i < vimc->pipe_cfg->num_ents; i++) {
 		dev_dbg(vimc->mdev.dev, "new entity for %s\n",
 			vimc->pipe_cfg->ents[i].name);
-		vimc->ent_devs[i] = vimc->pipe_cfg->ents[i].add(vimc,
+		vimc->ent_devs[i] = vimc->pipe_cfg->ents[i].type->add(vimc,
 					vimc->pipe_cfg->ents[i].name);
 		if (IS_ERR(vimc->ent_devs[i])) {
 			int err = PTR_ERR(vimc->ent_devs[i]);
diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
index d591b6a6bcf0..ddaf223ddde5 100644
--- a/drivers/media/platform/vimc/vimc-debayer.c
+++ b/drivers/media/platform/vimc/vimc-debayer.c
@@ -579,3 +579,8 @@ struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
 
 	return ERR_PTR(ret);
 }
+
+struct vimc_ent_type vimc_deb_type = {
+	.add = vimc_deb_add,
+	.release = vimc_deb_release
+};
diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
index 6ea05dcebc31..c6c31e8ce2e6 100644
--- a/drivers/media/platform/vimc/vimc-scaler.c
+++ b/drivers/media/platform/vimc/vimc-scaler.c
@@ -509,3 +509,8 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
 
 	return &vsca->ved;
 }
+
+struct vimc_ent_type vimc_sca_type = {
+	.add = vimc_sca_add,
+	.release = vimc_sca_release
+};
diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
index 24ce5b70b97e..3adf601e38e2 100644
--- a/drivers/media/platform/vimc/vimc-sensor.c
+++ b/drivers/media/platform/vimc/vimc-sensor.c
@@ -374,3 +374,8 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
 
 	return ERR_PTR(ret);
 }
+
+struct vimc_ent_type vimc_sen_type = {
+	.add = vimc_sen_add,
+	.release = vimc_sen_release
+};
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 5/5] media: vimc: add vimc_ent_type struct for the callbacks of entities
  2020-03-28  7:52 ` [PATCH 5/5] media: vimc: add vimc_ent_type struct for the callbacks of entities Dafna Hirschfeld
@ 2020-03-30 12:10   ` Hans Verkuil
  2020-03-30 12:11     ` Hans Verkuil
  2020-03-30 15:57     ` Dafna Hirschfeld
  0 siblings, 2 replies; 9+ messages in thread
From: Hans Verkuil @ 2020-03-30 12:10 UTC (permalink / raw)
  To: Dafna Hirschfeld, linux-media, helen.koike, ezequiel, kernel,
	dafna3, mchehab, laurent.pinchart

On 3/28/20 8:52 AM, Dafna Hirschfeld wrote:
> Since each vimc entity type is defined by the
> callbacks implementation, it is a good idea to
> add a struct to hold these callbacks.
> 
> Each vimc entity then declare its type in the file for
> the entity.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

I get these warnings:

drivers/media/platform/vimc/vimc-capture.c:328:6: warning: no previous prototype for ‘vimc_cap_release’ [-Wmissing-prototypes]

  328 | void vimc_cap_release(struct vimc_ent_device *ved)
      |      ^~~~~~~~~~~~~~~~
drivers/media/platform/vimc/vimc-capture.c:337:6: warning: no previous prototype for ‘vimc_cap_unregister’ [-Wmissing-prototypes]

  337 | void vimc_cap_unregister(struct vimc_ent_device *ved)
      |      ^~~~~~~~~~~~~~~~~~~
drivers/media/platform/vimc/vimc-capture.c:385:25: warning: no previous prototype for ‘vimc_cap_add’ [-Wmissing-prototypes]

  385 | struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
      |                         ^~~~~~~~~~~~
  CC      drivers/media/platform/vimc/vimc-scaler.o
drivers/media/platform/vimc/vimc-debayer.c:497:6: warning: no previous prototype for ‘vimc_deb_release’ [-Wmissing-prototypes]

  497 | void vimc_deb_release(struct vimc_ent_device *ved)
      |      ^~~~~~~~~~~~~~~~
drivers/media/platform/vimc/vimc-debayer.c:525:25: warning: no previous prototype for ‘vimc_deb_add’ [-Wmissing-prototypes]

  525 | struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
      |                         ^~~~~~~~~~~~
  CC      drivers/media/platform/vimc/vimc-sensor.o
drivers/media/platform/vimc/vimc-scaler.c:467:6: warning: no previous prototype for ‘vimc_sca_release’ [-Wmissing-prototypes]

  467 | void vimc_sca_release(struct vimc_ent_device *ved)
      |      ^~~~~~~~~~~~~~~~
drivers/media/platform/vimc/vimc-scaler.c:476:25: warning: no previous prototype for ‘vimc_sca_add’ [-Wmissing-prototypes]

  476 | struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
      |                         ^~~~~~~~~~~~
drivers/media/platform/vimc/vimc-sensor.c:282:6: warning: no previous prototype for ‘vimc_sen_release’ [-Wmissing-prototypes]

  282 | void vimc_sen_release(struct vimc_ent_device *ved)
      |      ^~~~~~~~~~~~~~~~
drivers/media/platform/vimc/vimc-sensor.c:310:25: warning: no previous prototype for ‘vimc_sen_add’ [-Wmissing-prototypes]

  310 | struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
      |                         ^~~~~~~~~~~~


I think that all these functions should become static.

Regards,

	Hans

> ---
>  drivers/media/platform/vimc/vimc-capture.c |  6 ++++
>  drivers/media/platform/vimc/vimc-common.h  | 42 +++++++++++-----------
>  drivers/media/platform/vimc/vimc-core.c    | 38 +++++++-------------
>  drivers/media/platform/vimc/vimc-debayer.c |  5 +++
>  drivers/media/platform/vimc/vimc-scaler.c  |  5 +++
>  drivers/media/platform/vimc/vimc-sensor.c  |  5 +++
>  6 files changed, 54 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index 8bafbf90cbf5..230f024ff896 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -478,3 +478,9 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
>  
>  	return ERR_PTR(ret);
>  }
> +
> +struct vimc_ent_type vimc_cap_type = {
> +	.add = vimc_cap_add,
> +	.unregister = vimc_cap_unregister,
> +	.release = vimc_cap_release
> +};
> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
> index 40a58f1cb18f..b2fa3b793916 100644
> --- a/drivers/media/platform/vimc/vimc-common.h
> +++ b/drivers/media/platform/vimc/vimc-common.h
> @@ -120,24 +120,35 @@ struct vimc_device {
>  };
>  
>  /**
> - * struct vimc_ent_config	Structure which describes individual
> - *				configuration for each entity
> + * struct vimc_ent_type		Structure for the callbacks of the entity types
> + *
>   *
> - * @name			entity name
>   * @add				initializes and registers
>   *					vimc entity - called from vimc-core
>   * @unregister			unregisters vimc entity - called from vimc-core
>   * @release			releases vimc entity - called from the v4l2_dev
>   *					release callback
>   */
> -struct vimc_ent_config {
> -	const char *name;
> +struct vimc_ent_type {
>  	struct vimc_ent_device *(*add)(struct vimc_device *vimc,
>  				       const char *vcfg_name);
>  	void (*unregister)(struct vimc_ent_device *ved);
>  	void (*release)(struct vimc_ent_device *ved);
>  };
>  
> +/**
> + * struct vimc_ent_config	Structure which describes individual
> + *				configuration for each entity
> + *
> + * @name			entity name
> + * @type			contain the callbacks of this entity type
> + *
> + */
> +struct vimc_ent_config {
> +	const char *name;
> +	struct vimc_ent_type *type;
> +};
> +
>  /**
>   * vimc_is_source - returns true if the entity has only source pads
>   *
> @@ -146,23 +157,10 @@ struct vimc_ent_config {
>   */
>  bool vimc_is_source(struct media_entity *ent);
>  
> -/* prototypes for vimc_ent_config hooks */
> -struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
> -				     const char *vcfg_name);
> -void vimc_cap_unregister(struct vimc_ent_device *ved);
> -void vimc_cap_release(struct vimc_ent_device *ved);
> -
> -struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
> -				     const char *vcfg_name);
> -void vimc_deb_release(struct vimc_ent_device *ved);
> -
> -struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
> -				     const char *vcfg_name);
> -void vimc_sca_release(struct vimc_ent_device *ved);
> -
> -struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
> -				     const char *vcfg_name);
> -void vimc_sen_release(struct vimc_ent_device *ved);
> +extern struct vimc_ent_type vimc_sen_type;
> +extern struct vimc_ent_type vimc_deb_type;
> +extern struct vimc_ent_type vimc_sca_type;
> +extern struct vimc_ent_type vimc_cap_type;
>  
>  /**
>   * vimc_pix_map_by_index - get vimc_pix_map struct by its index
> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
> index edac998f58d5..11210aaa2551 100644
> --- a/drivers/media/platform/vimc/vimc-core.c
> +++ b/drivers/media/platform/vimc/vimc-core.c
> @@ -47,52 +47,40 @@ struct vimc_pipeline_config {
>  static struct vimc_ent_config ent_config[] = {
>  	{
>  		.name = "Sensor A",
> -		.add = vimc_sen_add,
> -		.release = vimc_sen_release,
> +		.type = &vimc_sen_type
>  	},
>  	{
>  		.name = "Sensor B",
> -		.add = vimc_sen_add,
> -		.release = vimc_sen_release,
> +		.type = &vimc_sen_type
>  	},
>  	{
>  		.name = "Debayer A",
> -		.add = vimc_deb_add,
> -		.release = vimc_deb_release,
> +		.type = &vimc_deb_type
>  	},
>  	{
>  		.name = "Debayer B",
> -		.add = vimc_deb_add,
> -		.release = vimc_deb_release,
> +		.type = &vimc_deb_type
>  	},
>  	{
>  		.name = "Raw Capture 0",
> -		.add = vimc_cap_add,
> -		.unregister = vimc_cap_unregister,
> -		.release = vimc_cap_release,
> +		.type = &vimc_cap_type
>  	},
>  	{
>  		.name = "Raw Capture 1",
> -		.add = vimc_cap_add,
> -		.unregister = vimc_cap_unregister,
> -		.release = vimc_cap_release,
> +		.type = &vimc_cap_type
>  	},
>  	{
>  		/* TODO: change this to vimc-input when it is implemented */
>  		.name = "RGB/YUV Input",
> -		.add = vimc_sen_add,
> -		.release = vimc_sen_release,
> +		.type = &vimc_sen_type
>  	},
>  	{
>  		.name = "Scaler",
> -		.add = vimc_sca_add,
> -		.release = vimc_sca_release,
> +		.type = &vimc_sca_type
>  	},
>  	{
>  		.name = "RGB/YUV Capture",
> -		.add = vimc_cap_add,
> -		.unregister = vimc_cap_unregister,
> -		.release = vimc_cap_release,
> +		.type = &vimc_cap_type
>  	},
>  };
>  
> @@ -166,7 +154,7 @@ static void vimc_release_subdevs(struct vimc_device *vimc)
>  
>  	for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
>  		if (vimc->ent_devs[i])
> -			vimc->pipe_cfg->ents[i].release(vimc->ent_devs[i]);
> +			vimc->pipe_cfg->ents[i].type->release(vimc->ent_devs[i]);
>  }
>  
>  static void vimc_unregister_subdevs(struct vimc_device *vimc)
> @@ -174,8 +162,8 @@ static void vimc_unregister_subdevs(struct vimc_device *vimc)
>  	unsigned int i;
>  
>  	for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
> -		if (vimc->ent_devs[i] && vimc->pipe_cfg->ents[i].unregister)
> -			vimc->pipe_cfg->ents[i].unregister(vimc->ent_devs[i]);
> +		if (vimc->ent_devs[i] && vimc->pipe_cfg->ents[i].type->unregister)
> +			vimc->pipe_cfg->ents[i].type->unregister(vimc->ent_devs[i]);
>  }
>  
>  static int vimc_add_subdevs(struct vimc_device *vimc)
> @@ -185,7 +173,7 @@ static int vimc_add_subdevs(struct vimc_device *vimc)
>  	for (i = 0; i < vimc->pipe_cfg->num_ents; i++) {
>  		dev_dbg(vimc->mdev.dev, "new entity for %s\n",
>  			vimc->pipe_cfg->ents[i].name);
> -		vimc->ent_devs[i] = vimc->pipe_cfg->ents[i].add(vimc,
> +		vimc->ent_devs[i] = vimc->pipe_cfg->ents[i].type->add(vimc,
>  					vimc->pipe_cfg->ents[i].name);
>  		if (IS_ERR(vimc->ent_devs[i])) {
>  			int err = PTR_ERR(vimc->ent_devs[i]);
> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
> index d591b6a6bcf0..ddaf223ddde5 100644
> --- a/drivers/media/platform/vimc/vimc-debayer.c
> +++ b/drivers/media/platform/vimc/vimc-debayer.c
> @@ -579,3 +579,8 @@ struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
>  
>  	return ERR_PTR(ret);
>  }
> +
> +struct vimc_ent_type vimc_deb_type = {
> +	.add = vimc_deb_add,
> +	.release = vimc_deb_release
> +};
> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> index 6ea05dcebc31..c6c31e8ce2e6 100644
> --- a/drivers/media/platform/vimc/vimc-scaler.c
> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> @@ -509,3 +509,8 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
>  
>  	return &vsca->ved;
>  }
> +
> +struct vimc_ent_type vimc_sca_type = {
> +	.add = vimc_sca_add,
> +	.release = vimc_sca_release
> +};
> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
> index 24ce5b70b97e..3adf601e38e2 100644
> --- a/drivers/media/platform/vimc/vimc-sensor.c
> +++ b/drivers/media/platform/vimc/vimc-sensor.c
> @@ -374,3 +374,8 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
>  
>  	return ERR_PTR(ret);
>  }
> +
> +struct vimc_ent_type vimc_sen_type = {
> +	.add = vimc_sen_add,
> +	.release = vimc_sen_release
> +};
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 5/5] media: vimc: add vimc_ent_type struct for the callbacks of entities
  2020-03-30 12:10   ` Hans Verkuil
@ 2020-03-30 12:11     ` Hans Verkuil
  2020-03-30 15:57     ` Dafna Hirschfeld
  1 sibling, 0 replies; 9+ messages in thread
From: Hans Verkuil @ 2020-03-30 12:11 UTC (permalink / raw)
  To: Dafna Hirschfeld, linux-media, helen.koike, ezequiel, kernel,
	dafna3, mchehab, laurent.pinchart

On 3/30/20 2:10 PM, Hans Verkuil wrote:
> On 3/28/20 8:52 AM, Dafna Hirschfeld wrote:
>> Since each vimc entity type is defined by the
>> callbacks implementation, it is a good idea to
>> add a struct to hold these callbacks.
>>
>> Each vimc entity then declare its type in the file for
>> the entity.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> 
> I get these warnings:
> 
> drivers/media/platform/vimc/vimc-capture.c:328:6: warning: no previous prototype for ‘vimc_cap_release’ [-Wmissing-prototypes]
> 
>   328 | void vimc_cap_release(struct vimc_ent_device *ved)
>       |      ^~~~~~~~~~~~~~~~
> drivers/media/platform/vimc/vimc-capture.c:337:6: warning: no previous prototype for ‘vimc_cap_unregister’ [-Wmissing-prototypes]
> 
>   337 | void vimc_cap_unregister(struct vimc_ent_device *ved)
>       |      ^~~~~~~~~~~~~~~~~~~
> drivers/media/platform/vimc/vimc-capture.c:385:25: warning: no previous prototype for ‘vimc_cap_add’ [-Wmissing-prototypes]
> 
>   385 | struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
>       |                         ^~~~~~~~~~~~
>   CC      drivers/media/platform/vimc/vimc-scaler.o
> drivers/media/platform/vimc/vimc-debayer.c:497:6: warning: no previous prototype for ‘vimc_deb_release’ [-Wmissing-prototypes]
> 
>   497 | void vimc_deb_release(struct vimc_ent_device *ved)
>       |      ^~~~~~~~~~~~~~~~
> drivers/media/platform/vimc/vimc-debayer.c:525:25: warning: no previous prototype for ‘vimc_deb_add’ [-Wmissing-prototypes]
> 
>   525 | struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
>       |                         ^~~~~~~~~~~~
>   CC      drivers/media/platform/vimc/vimc-sensor.o
> drivers/media/platform/vimc/vimc-scaler.c:467:6: warning: no previous prototype for ‘vimc_sca_release’ [-Wmissing-prototypes]
> 
>   467 | void vimc_sca_release(struct vimc_ent_device *ved)
>       |      ^~~~~~~~~~~~~~~~
> drivers/media/platform/vimc/vimc-scaler.c:476:25: warning: no previous prototype for ‘vimc_sca_add’ [-Wmissing-prototypes]
> 
>   476 | struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
>       |                         ^~~~~~~~~~~~
> drivers/media/platform/vimc/vimc-sensor.c:282:6: warning: no previous prototype for ‘vimc_sen_release’ [-Wmissing-prototypes]
> 
>   282 | void vimc_sen_release(struct vimc_ent_device *ved)
>       |      ^~~~~~~~~~~~~~~~
> drivers/media/platform/vimc/vimc-sensor.c:310:25: warning: no previous prototype for ‘vimc_sen_add’ [-Wmissing-prototypes]
> 
>   310 | struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
>       |                         ^~~~~~~~~~~~
> 
> 
> I think that all these functions should become static.

Forgot to mention: the other patches in this series look fine, so I'll take
those and wait for a v2 of this last patch.

Regards,

	Hans

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 5/5] media: vimc: add vimc_ent_type struct for the callbacks of entities
  2020-03-30 12:10   ` Hans Verkuil
  2020-03-30 12:11     ` Hans Verkuil
@ 2020-03-30 15:57     ` Dafna Hirschfeld
  1 sibling, 0 replies; 9+ messages in thread
From: Dafna Hirschfeld @ 2020-03-30 15:57 UTC (permalink / raw)
  To: Hans Verkuil, linux-media, helen.koike, ezequiel, kernel, dafna3,
	mchehab, laurent.pinchart



On 30.03.20 14:10, Hans Verkuil wrote:
> On 3/28/20 8:52 AM, Dafna Hirschfeld wrote:
>> Since each vimc entity type is defined by the
>> callbacks implementation, it is a good idea to
>> add a struct to hold these callbacks.
>>
>> Each vimc entity then declare its type in the file for
>> the entity.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> 
> I get these warnings:
> 
> drivers/media/platform/vimc/vimc-capture.c:328:6: warning: no previous prototype for ‘vimc_cap_release’ [-Wmissing-prototypes]
> 
>    328 | void vimc_cap_release(struct vimc_ent_device *ved)
>        |      ^~~~~~~~~~~~~~~~
> drivers/media/platform/vimc/vimc-capture.c:337:6: warning: no previous prototype for ‘vimc_cap_unregister’ [-Wmissing-prototypes]
> 
>    337 | void vimc_cap_unregister(struct vimc_ent_device *ved)
>        |      ^~~~~~~~~~~~~~~~~~~
> drivers/media/platform/vimc/vimc-capture.c:385:25: warning: no previous prototype for ‘vimc_cap_add’ [-Wmissing-prototypes]
> 
>    385 | struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
>        |                         ^~~~~~~~~~~~
>    CC      drivers/media/platform/vimc/vimc-scaler.o
> drivers/media/platform/vimc/vimc-debayer.c:497:6: warning: no previous prototype for ‘vimc_deb_release’ [-Wmissing-prototypes]
> 
>    497 | void vimc_deb_release(struct vimc_ent_device *ved)
>        |      ^~~~~~~~~~~~~~~~
> drivers/media/platform/vimc/vimc-debayer.c:525:25: warning: no previous prototype for ‘vimc_deb_add’ [-Wmissing-prototypes]
> 
>    525 | struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
>        |                         ^~~~~~~~~~~~
>    CC      drivers/media/platform/vimc/vimc-sensor.o
> drivers/media/platform/vimc/vimc-scaler.c:467:6: warning: no previous prototype for ‘vimc_sca_release’ [-Wmissing-prototypes]
> 
>    467 | void vimc_sca_release(struct vimc_ent_device *ved)
>        |      ^~~~~~~~~~~~~~~~
> drivers/media/platform/vimc/vimc-scaler.c:476:25: warning: no previous prototype for ‘vimc_sca_add’ [-Wmissing-prototypes]
> 
>    476 | struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
>        |                         ^~~~~~~~~~~~
> drivers/media/platform/vimc/vimc-sensor.c:282:6: warning: no previous prototype for ‘vimc_sen_release’ [-Wmissing-prototypes]
> 
>    282 | void vimc_sen_release(struct vimc_ent_device *ved)
>        |      ^~~~~~~~~~~~~~~~
> drivers/media/platform/vimc/vimc-sensor.c:310:25: warning: no previous prototype for ‘vimc_sen_add’ [-Wmissing-prototypes]
> 
>    310 | struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
>        |                         ^~~~~~~~~~~~
> 
> 
> I think that all these functions should become static.

Hi, ok, I wonder why I don't get these warnings when compiling.

> 
> Regards,
> 
> 	Hans
> 
>> ---
>>   drivers/media/platform/vimc/vimc-capture.c |  6 ++++
>>   drivers/media/platform/vimc/vimc-common.h  | 42 +++++++++++-----------
>>   drivers/media/platform/vimc/vimc-core.c    | 38 +++++++-------------
>>   drivers/media/platform/vimc/vimc-debayer.c |  5 +++
>>   drivers/media/platform/vimc/vimc-scaler.c  |  5 +++
>>   drivers/media/platform/vimc/vimc-sensor.c  |  5 +++
>>   6 files changed, 54 insertions(+), 47 deletions(-)
>>
>> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
>> index 8bafbf90cbf5..230f024ff896 100644
>> --- a/drivers/media/platform/vimc/vimc-capture.c
>> +++ b/drivers/media/platform/vimc/vimc-capture.c
>> @@ -478,3 +478,9 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
>>   
>>   	return ERR_PTR(ret);
>>   }
>> +
>> +struct vimc_ent_type vimc_cap_type = {
>> +	.add = vimc_cap_add,
>> +	.unregister = vimc_cap_unregister,
>> +	.release = vimc_cap_release
>> +};
>> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
>> index 40a58f1cb18f..b2fa3b793916 100644
>> --- a/drivers/media/platform/vimc/vimc-common.h
>> +++ b/drivers/media/platform/vimc/vimc-common.h
>> @@ -120,24 +120,35 @@ struct vimc_device {
>>   };
>>   
>>   /**
>> - * struct vimc_ent_config	Structure which describes individual
>> - *				configuration for each entity
>> + * struct vimc_ent_type		Structure for the callbacks of the entity types
>> + *
>>    *
>> - * @name			entity name
>>    * @add				initializes and registers
>>    *					vimc entity - called from vimc-core
>>    * @unregister			unregisters vimc entity - called from vimc-core
>>    * @release			releases vimc entity - called from the v4l2_dev
>>    *					release callback
>>    */
>> -struct vimc_ent_config {
>> -	const char *name;
>> +struct vimc_ent_type {
>>   	struct vimc_ent_device *(*add)(struct vimc_device *vimc,
>>   				       const char *vcfg_name);
>>   	void (*unregister)(struct vimc_ent_device *ved);
>>   	void (*release)(struct vimc_ent_device *ved);
>>   };
>>   
>> +/**
>> + * struct vimc_ent_config	Structure which describes individual
>> + *				configuration for each entity
>> + *
>> + * @name			entity name
>> + * @type			contain the callbacks of this entity type
>> + *
>> + */
>> +struct vimc_ent_config {
>> +	const char *name;
>> +	struct vimc_ent_type *type;
>> +};
>> +
>>   /**
>>    * vimc_is_source - returns true if the entity has only source pads
>>    *
>> @@ -146,23 +157,10 @@ struct vimc_ent_config {
>>    */
>>   bool vimc_is_source(struct media_entity *ent);
>>   
>> -/* prototypes for vimc_ent_config hooks */
>> -struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
>> -				     const char *vcfg_name);
>> -void vimc_cap_unregister(struct vimc_ent_device *ved);
>> -void vimc_cap_release(struct vimc_ent_device *ved);
>> -
>> -struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
>> -				     const char *vcfg_name);
>> -void vimc_deb_release(struct vimc_ent_device *ved);
>> -
>> -struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
>> -				     const char *vcfg_name);
>> -void vimc_sca_release(struct vimc_ent_device *ved);
>> -
>> -struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
>> -				     const char *vcfg_name);
>> -void vimc_sen_release(struct vimc_ent_device *ved);
>> +extern struct vimc_ent_type vimc_sen_type;
>> +extern struct vimc_ent_type vimc_deb_type;
>> +extern struct vimc_ent_type vimc_sca_type;
>> +extern struct vimc_ent_type vimc_cap_type;
>>   
>>   /**
>>    * vimc_pix_map_by_index - get vimc_pix_map struct by its index
>> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
>> index edac998f58d5..11210aaa2551 100644
>> --- a/drivers/media/platform/vimc/vimc-core.c
>> +++ b/drivers/media/platform/vimc/vimc-core.c
>> @@ -47,52 +47,40 @@ struct vimc_pipeline_config {
>>   static struct vimc_ent_config ent_config[] = {
>>   	{
>>   		.name = "Sensor A",
>> -		.add = vimc_sen_add,
>> -		.release = vimc_sen_release,
>> +		.type = &vimc_sen_type
>>   	},
>>   	{
>>   		.name = "Sensor B",
>> -		.add = vimc_sen_add,
>> -		.release = vimc_sen_release,
>> +		.type = &vimc_sen_type
>>   	},
>>   	{
>>   		.name = "Debayer A",
>> -		.add = vimc_deb_add,
>> -		.release = vimc_deb_release,
>> +		.type = &vimc_deb_type
>>   	},
>>   	{
>>   		.name = "Debayer B",
>> -		.add = vimc_deb_add,
>> -		.release = vimc_deb_release,
>> +		.type = &vimc_deb_type
>>   	},
>>   	{
>>   		.name = "Raw Capture 0",
>> -		.add = vimc_cap_add,
>> -		.unregister = vimc_cap_unregister,
>> -		.release = vimc_cap_release,
>> +		.type = &vimc_cap_type
>>   	},
>>   	{
>>   		.name = "Raw Capture 1",
>> -		.add = vimc_cap_add,
>> -		.unregister = vimc_cap_unregister,
>> -		.release = vimc_cap_release,
>> +		.type = &vimc_cap_type
>>   	},
>>   	{
>>   		/* TODO: change this to vimc-input when it is implemented */
>>   		.name = "RGB/YUV Input",
>> -		.add = vimc_sen_add,
>> -		.release = vimc_sen_release,
>> +		.type = &vimc_sen_type
>>   	},
>>   	{
>>   		.name = "Scaler",
>> -		.add = vimc_sca_add,
>> -		.release = vimc_sca_release,
>> +		.type = &vimc_sca_type
>>   	},
>>   	{
>>   		.name = "RGB/YUV Capture",
>> -		.add = vimc_cap_add,
>> -		.unregister = vimc_cap_unregister,
>> -		.release = vimc_cap_release,
>> +		.type = &vimc_cap_type
>>   	},
>>   };
>>   
>> @@ -166,7 +154,7 @@ static void vimc_release_subdevs(struct vimc_device *vimc)
>>   
>>   	for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
>>   		if (vimc->ent_devs[i])
>> -			vimc->pipe_cfg->ents[i].release(vimc->ent_devs[i]);
>> +			vimc->pipe_cfg->ents[i].type->release(vimc->ent_devs[i]);
>>   }
>>   
>>   static void vimc_unregister_subdevs(struct vimc_device *vimc)
>> @@ -174,8 +162,8 @@ static void vimc_unregister_subdevs(struct vimc_device *vimc)
>>   	unsigned int i;
>>   
>>   	for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
>> -		if (vimc->ent_devs[i] && vimc->pipe_cfg->ents[i].unregister)
>> -			vimc->pipe_cfg->ents[i].unregister(vimc->ent_devs[i]);
>> +		if (vimc->ent_devs[i] && vimc->pipe_cfg->ents[i].type->unregister)
>> +			vimc->pipe_cfg->ents[i].type->unregister(vimc->ent_devs[i]);
>>   }
>>   
>>   static int vimc_add_subdevs(struct vimc_device *vimc)
>> @@ -185,7 +173,7 @@ static int vimc_add_subdevs(struct vimc_device *vimc)
>>   	for (i = 0; i < vimc->pipe_cfg->num_ents; i++) {
>>   		dev_dbg(vimc->mdev.dev, "new entity for %s\n",
>>   			vimc->pipe_cfg->ents[i].name);
>> -		vimc->ent_devs[i] = vimc->pipe_cfg->ents[i].add(vimc,
>> +		vimc->ent_devs[i] = vimc->pipe_cfg->ents[i].type->add(vimc,
>>   					vimc->pipe_cfg->ents[i].name);
>>   		if (IS_ERR(vimc->ent_devs[i])) {
>>   			int err = PTR_ERR(vimc->ent_devs[i]);
>> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
>> index d591b6a6bcf0..ddaf223ddde5 100644
>> --- a/drivers/media/platform/vimc/vimc-debayer.c
>> +++ b/drivers/media/platform/vimc/vimc-debayer.c
>> @@ -579,3 +579,8 @@ struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
>>   
>>   	return ERR_PTR(ret);
>>   }
>> +
>> +struct vimc_ent_type vimc_deb_type = {
>> +	.add = vimc_deb_add,
>> +	.release = vimc_deb_release
>> +};
>> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
>> index 6ea05dcebc31..c6c31e8ce2e6 100644
>> --- a/drivers/media/platform/vimc/vimc-scaler.c
>> +++ b/drivers/media/platform/vimc/vimc-scaler.c
>> @@ -509,3 +509,8 @@ struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
>>   
>>   	return &vsca->ved;
>>   }
>> +
>> +struct vimc_ent_type vimc_sca_type = {
>> +	.add = vimc_sca_add,
>> +	.release = vimc_sca_release
>> +};
>> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
>> index 24ce5b70b97e..3adf601e38e2 100644
>> --- a/drivers/media/platform/vimc/vimc-sensor.c
>> +++ b/drivers/media/platform/vimc/vimc-sensor.c
>> @@ -374,3 +374,8 @@ struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
>>   
>>   	return ERR_PTR(ret);
>>   }
>> +
>> +struct vimc_ent_type vimc_sen_type = {
>> +	.add = vimc_sen_add,
>> +	.release = vimc_sen_release
>> +};
>>
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-03-30 15:57 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-28  7:52 [PATCH 0/5] media: vimc: various fixes Dafna Hirschfeld
2020-03-28  7:52 ` [PATCH 1/5] media: vimc: remove the function vimc_unregister Dafna Hirschfeld
2020-03-28  7:52 ` [PATCH 2/5] media: vimc: handle error in vimc_add_subdevs Dafna Hirschfeld
2020-03-28  7:52 ` [PATCH 3/5] media: vimc: keep the error value when adding an entity fails Dafna Hirschfeld
2020-03-28  7:52 ` [PATCH 4/5] media: vimc: fix issues in documentation in vimc-common.h Dafna Hirschfeld
2020-03-28  7:52 ` [PATCH 5/5] media: vimc: add vimc_ent_type struct for the callbacks of entities Dafna Hirschfeld
2020-03-30 12:10   ` Hans Verkuil
2020-03-30 12:11     ` Hans Verkuil
2020-03-30 15:57     ` Dafna Hirschfeld

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).