[PATCH v4 0/5] Collapse vimc into single monolithic driver

[PATCH v4 0/5] Collapse vimc into single monolithic driver

[Shuah Khan]

vimc uses Component API to split the driver into functional components.
The real hardware resembles a monolith structure than component and
component structure added a level of complexity making it hard to
maintain without adding any real benefit.

The sensor is one vimc component that would makes sense to be a separate
module to closely align with the real hardware. It would be easier to
collapse vimc into single monolithic driver first and then split the
sensor off as a separate module.

This patch series removes the component API and makes minimal changes to
the code base preserving the functional division of the code structure.
Preserving the functional structure allows us to split the sensor off
as a separate module in the future.

The following configure and stream test works on all devices.

    media-ctl -d platform:vimc -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
    media-ctl -d platform:vimc -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
    media-ctl -d platform:vimc -V '"Sensor B":0[fmt:SBGGR8_1X8/640x480]'
    media-ctl -d platform:vimc -V '"Debayer B":0[fmt:SBGGR8_1X8/640x480]'

    v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=1920,height=1440
    v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v pixelformat=BA81
    v4l2-ctl -z platform:vimc -d "Raw Capture 1" -v pixelformat=BA81

    v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video1
    v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video2
    v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video3

The second patch in the series fixes a general protection fault found
when rmmod is done while stream is active.

- rmmod while streaming returns vimc is in use
- rmmod without active stream works correctly

Changed since v3: (5 patches)
Patch 1:
- Main change is adding an array of struct vimc_ent_device(s) to save
  vimc_ent_device(s), subdevs create in their "add" hooks. These
  get used to create links and removing the subdevs. vimc-core allocates
  this array which sized to number of entries in the topology defined in
  the vimc_pipeline_config structure.
- Remove links when register fails before registering the media device.
  In this case, entity links need to be removed from error paths.
Patch 2: no change.
New patch 3:
- Cleaning up duplicated IS_SRC and IS_SINK defines and moving them to
  common header.
New patch 4:
- Documentation module parameter usage updates.
New patch 5:
- Lastly adding myself as a reviewers to MAINTAINERS file for vimc

Changes since v2:
- Rebase to media master on top of vimc reverts
- Rename "vent" variable to "vcfg" to reflect config and standout
  from "ved"
- Error handling when adding subdevs fails to remove already added
  subdevs, do other clean-up and bail out.

Changes since v1:
Patch 1 & 2: (patch 1 in this series)
- Collapsed the two patches into one
- Added common defines (vimc_device and vimc_ent_config) to vimc-common.h
  based on our discussion.
- Addressed review comments from Helen and Laurent
- Use vimc-common.h instead of creating a new file.
- Other minor comments from Helen on int vs. unsigned int and
  not needing to initialize ret in vimc_add_subdevs()
Patch 3 (patch 2 in this series):
- The second patch is the fix for gpf. Updated the patch after looking
  at the test results from Andre and Helen. This problem is in a common
  code and impacts all subdevs. The fix addresses the core problem and
  fixes it. Fix removes pads release from v4l2_device_unregister_subdev()
  and pads are now released from the sd release handler with all other
  resources.

Outstanding:
- Update documentation with the correct topology.
- There is one outstanding gpf remaining in the unbind path. I will
  fix that in a separate patch. This is an existing problem and will
  be easier to fix on top of this patch series.

vimc_print_dot (--print-dot) topology after this change: (no change
compared to media master)
digraph board {
        rankdir=TB
        n00000001 [label="{{} | Sensor A\n/dev/v4l-subdev0 | {<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green]
        n00000001:port0 -> n00000005:port0 [style=bold]
        n00000001:port0 -> n0000000b [style=bold]
        n00000003 [label="{{} | Sensor B\n/dev/v4l-subdev1 | {<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green]
        n00000003:port0 -> n00000008:port0 [style=bold]
        n00000003:port0 -> n0000000f [style=bold]
        n00000005 [label="{{<port0> 0} | Debayer A\n/dev/v4l-subdev2 | {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green]
        n00000005:port1 -> n00000015:port0
        n00000008 [label="{{<port0> 0} | Debayer B\n/dev/v4l-subdev3 | {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green]
        n00000008:port1 -> n00000015:port0 [style=dashed]
        n0000000b [label="Raw Capture 0\n/dev/video0", shape=box, style=filled, fillcolor=yellow]
        n0000000f [label="Raw Capture 1\n/dev/video1", shape=box, style=filled, fillcolor=yellow]
        n00000013 [label="{{} | RGB/YUV Input\n/dev/v4l-subdev4 | {<port0> 0}}", shape=Mrecord, style=filled, fillcolor=green]
        n00000013:port0 -> n00000015:port0 [style=dashed]
        n00000015 [label="{{<port0> 0} | Scaler\n/dev/v4l-subdev5 | {<port1> 1}}", shape=Mrecord, style=filled, fillcolor=green]
        n00000015:port1 -> n00000018 [style=bold]
        n00000018 [label="RGB/YUV Capture\n/dev/video2", shape=box, style=filled, fillcolor=yellow]
}

Shuah Khan (5):
  media: vimc: Collapse component structure into a single monolithic
    driver
  media: vimc: Fix gpf in rmmod path when stream is active
  vimc: move duplicated IS_SRC and IS_SINK to common header
  doc: media: vimc: Update module parameter usage information
  MAINTAINERS: Add reviewer to vimc driver

 Documentation/media/v4l-drivers/vimc.rst   |  12 +-
 MAINTAINERS                                |   1 +
 drivers/media/platform/vimc/Makefile       |   7 +-
 drivers/media/platform/vimc/vimc-capture.c |  81 ++------
 drivers/media/platform/vimc/vimc-common.c  |   3 +-
 drivers/media/platform/vimc/vimc-common.h  |  58 ++++++
 drivers/media/platform/vimc/vimc-core.c    | 216 +++++++++------------
 drivers/media/platform/vimc/vimc-debayer.c |  81 ++------
 drivers/media/platform/vimc/vimc-scaler.c  |  82 ++------
 drivers/media/platform/vimc/vimc-sensor.c  |  74 ++-----
 10 files changed, 215 insertions(+), 400 deletions(-)

-- 
2.20.1

[PATCH v4 1/5] media: vimc: Collapse component structure into a single monolithic driver

[Shuah Khan]

vimc uses Component API to split the driver into functional components.
The real hardware resembles a monolith structure than component and
component structure added a level of complexity making it hard to
maintain without adding any real benefit.

The sensor is one vimc component that would makes sense to be a separate
module to closely align with the real hardware. It would be easier to
collapse vimc into single monolithic driver first and then split the
sensor off as a separate module.

Collapse it into a single monolithic driver removing the Component API.
This patch removes the component API and makes minimal changes to the
code base preserving the functional division of the code structure.
Preserving the functional structure allows us to split the sensor off
as a separate module in the future.

Major design elements in this change are:
- Use existing struct vimc_ent_config and struct vimc_pipeline_config
  to drive the initialization of the functional components.
- Make vimc_device and vimc_ent_config global by moving them to
  vimc-common.h
- Add two new hooks add and rm to initialize and register, unregister
  and free subdevs.
- All component API is now gone and bind and unbind hooks are modified
  to do "add" and "rm" with minimal changes to just add and rm subdevs.
- vimc-core's bind and unbind are now register and unregister.
- Add a new field to vimc_device structure for saving the pointers to
  struct vimc_ent_device(s) subdevs create in their "add" hooks. These
  get used to create links and removing the subdevs. vimc-core allocates
  this array which sized to number of entries in the topology defined in
  the vimc_pipeline_config structure.
- vimc-core invokes "add" hooks from its vimc_register_devices().
  The "add" hooks remain the same and register subdevs. They don't
  create platform devices of their own and use vimc's pdev.dev as
  their reference device. Each "add" hook returns pointer to its struct
  vimc_ent_device. This is saved in the vimc_device ent_devs array.
- vimc-core invokes "rm" hooks from its unregister to unregister subdevs
  and cleanup.
- vimc-core invokes "add" and "rm" hooks with pointer to struct vimc_device
  and the corresponding vimc_ent_device saved in the ent_devs.

The following configure and stream test works on all devices.

media-ctl -d platform:vimc -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
media-ctl -d platform:vimc -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
media-ctl -d platform:vimc -V '"Sensor B":0[fmt:SBGGR8_1X8/640x480]'
media-ctl -d platform:vimc -V '"Debayer B":0[fmt:SBGGR8_1X8/640x480]'

v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=1920,height=1440
v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v pixelformat=BA81
v4l2-ctl -z platform:vimc -d "Raw Capture 1" -v pixelformat=BA81

v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video1
v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video2
v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video3

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/media/platform/vimc/Makefile       |   7 +-
 drivers/media/platform/vimc/vimc-capture.c |  81 ++------
 drivers/media/platform/vimc/vimc-common.h  |  54 ++++++
 drivers/media/platform/vimc/vimc-core.c    | 216 +++++++++------------
 drivers/media/platform/vimc/vimc-debayer.c |  69 ++-----
 drivers/media/platform/vimc/vimc-scaler.c  |  73 ++-----
 drivers/media/platform/vimc/vimc-sensor.c  |  73 ++-----
 7 files changed, 193 insertions(+), 380 deletions(-)

diff --git a/drivers/media/platform/vimc/Makefile b/drivers/media/platform/vimc/Makefile
index 96d06f030c31..a53b2b532e9f 100644
--- a/drivers/media/platform/vimc/Makefile
+++ b/drivers/media/platform/vimc/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
-vimc-y := vimc-core.o vimc-common.o vimc-streamer.o
+vimc-y := vimc-core.o vimc-common.o vimc-streamer.o vimc-capture.o \
+		vimc-debayer.o vimc-scaler.o vimc-sensor.o
+
+obj-$(CONFIG_VIDEO_VIMC) += vimc.o
 
-obj-$(CONFIG_VIDEO_VIMC) += vimc.o vimc-capture.o vimc-debayer.o \
-                vimc-scaler.o vimc-sensor.o
diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
index 1d56b91830ba..602f80323031 100644
--- a/drivers/media/platform/vimc/vimc-capture.c
+++ b/drivers/media/platform/vimc/vimc-capture.c
@@ -5,10 +5,6 @@
  * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com>
  */
 
-#include <linux/component.h>
-#include <linux/module.h>
-#include <linux/mod_devicetable.h>
-#include <linux/platform_device.h>
 #include <media/v4l2-ioctl.h>
 #include <media/videobuf2-core.h>
 #include <media/videobuf2-vmalloc.h>
@@ -16,8 +12,6 @@
 #include "vimc-common.h"
 #include "vimc-streamer.h"
 
-#define VIMC_CAP_DRV_NAME "vimc-capture"
-
 struct vimc_cap_device {
 	struct vimc_ent_device ved;
 	struct video_device vdev;
@@ -340,13 +334,11 @@ static void vimc_cap_release(struct video_device *vdev)
 	kfree(vcap);
 }
 
-static void vimc_cap_comp_unbind(struct device *comp, struct device *master,
-				 void *master_data)
+void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
 {
-	struct vimc_ent_device *ved = dev_get_drvdata(comp);
-	struct vimc_cap_device *vcap = container_of(ved, struct vimc_cap_device,
-						    ved);
+	struct vimc_cap_device *vcap;
 
+	vcap = container_of(ved, struct vimc_cap_device, ved);
 	vb2_queue_release(&vcap->queue);
 	media_entity_cleanup(ved->ent);
 	video_unregister_device(&vcap->vdev);
@@ -391,11 +383,10 @@ static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
 	return NULL;
 }
 
-static int vimc_cap_comp_bind(struct device *comp, struct device *master,
-			      void *master_data)
+struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
+				     const char *vcfg_name)
 {
-	struct v4l2_device *v4l2_dev = master_data;
-	struct vimc_platform_data *pdata = comp->platform_data;
+	struct v4l2_device *v4l2_dev = &vimc->v4l2_dev;
 	const struct vimc_pix_map *vpix;
 	struct vimc_cap_device *vcap;
 	struct video_device *vdev;
@@ -405,7 +396,7 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
 	/* Allocate the vimc_cap_device struct */
 	vcap = kzalloc(sizeof(*vcap), GFP_KERNEL);
 	if (!vcap)
-		return -ENOMEM;
+		return NULL;
 
 	/* Allocate the pads */
 	vcap->ved.pads =
@@ -416,7 +407,7 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
 	}
 
 	/* Initialize the media entity */
-	vcap->vdev.entity.name = pdata->entity_name;
+	vcap->vdev.entity.name = vcfg_name;
 	vcap->vdev.entity.function = MEDIA_ENT_F_IO_V4L;
 	ret = media_entity_pads_init(&vcap->vdev.entity,
 				     1, vcap->ved.pads);
@@ -440,8 +431,8 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
 
 	ret = vb2_queue_init(q);
 	if (ret) {
-		dev_err(comp, "%s: vb2 queue init failed (err=%d)\n",
-			pdata->entity_name, ret);
+		dev_err(&vimc->pdev.dev, "%s: vb2 queue init failed (err=%d)\n",
+			vcfg_name, ret);
 		goto err_clean_m_ent;
 	}
 
@@ -460,8 +451,7 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
 	vcap->ved.ent = &vcap->vdev.entity;
 	vcap->ved.process_frame = vimc_cap_process_frame;
 	vcap->ved.vdev_get_format = vimc_cap_get_format;
-	dev_set_drvdata(comp, &vcap->ved);
-	vcap->dev = comp;
+	vcap->dev = &vimc->pdev.dev;
 
 	/* Initialize the video_device struct */
 	vdev = &vcap->vdev;
@@ -474,18 +464,18 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
 	vdev->queue = q;
 	vdev->v4l2_dev = v4l2_dev;
 	vdev->vfl_dir = VFL_DIR_RX;
-	strscpy(vdev->name, pdata->entity_name, sizeof(vdev->name));
+	strscpy(vdev->name, vcfg_name, sizeof(vdev->name));
 	video_set_drvdata(vdev, &vcap->ved);
 
 	/* Register the video_device with the v4l2 and the media framework */
 	ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
 	if (ret) {
-		dev_err(comp, "%s: video register failed (err=%d)\n",
+		dev_err(&vimc->pdev.dev, "%s: video register failed (err=%d)\n",
 			vcap->vdev.name, ret);
 		goto err_release_queue;
 	}
 
-	return 0;
+	return &vcap->ved;
 
 err_release_queue:
 	vb2_queue_release(q);
@@ -496,46 +486,5 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
 err_free_vcap:
 	kfree(vcap);
 
-	return ret;
-}
-
-static const struct component_ops vimc_cap_comp_ops = {
-	.bind = vimc_cap_comp_bind,
-	.unbind = vimc_cap_comp_unbind,
-};
-
-static int vimc_cap_probe(struct platform_device *pdev)
-{
-	return component_add(&pdev->dev, &vimc_cap_comp_ops);
-}
-
-static int vimc_cap_remove(struct platform_device *pdev)
-{
-	component_del(&pdev->dev, &vimc_cap_comp_ops);
-
-	return 0;
+	return NULL;
 }
-
-static const struct platform_device_id vimc_cap_driver_ids[] = {
-	{
-		.name           = VIMC_CAP_DRV_NAME,
-	},
-	{ }
-};
-
-static struct platform_driver vimc_cap_pdrv = {
-	.probe		= vimc_cap_probe,
-	.remove		= vimc_cap_remove,
-	.id_table	= vimc_cap_driver_ids,
-	.driver		= {
-		.name	= VIMC_CAP_DRV_NAME,
-	},
-};
-
-module_platform_driver(vimc_cap_pdrv);
-
-MODULE_DEVICE_TABLE(platform, vimc_cap_driver_ids);
-
-MODULE_DESCRIPTION("Virtual Media Controller Driver (VIMC) Capture");
-MODULE_AUTHOR("Helen Mae Koike Fornazier <helen.fornazier@gmail.com>");
-MODULE_LICENSE("GPL");
diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
index 9c2e0e216c6b..87ee84f78322 100644
--- a/drivers/media/platform/vimc/vimc-common.h
+++ b/drivers/media/platform/vimc/vimc-common.h
@@ -9,6 +9,7 @@
 #define _VIMC_COMMON_H_
 
 #include <linux/slab.h>
+#include <linux/platform_device.h>
 #include <media/media-device.h>
 #include <media/v4l2-device.h>
 
@@ -111,6 +112,59 @@ struct vimc_ent_device {
 			      struct v4l2_pix_format *fmt);
 };
 
+/**
+ * struct vimc_device - main device for vimc driver
+ *
+ * @pdev	pointer to the platform device
+ * @pipe_cfg	pointer to the vimc pipeline configuration structure
+ * @ent_devs	arry of vimc_ent_device pointers
+ * @mdev	the associated media_device parent
+ * @v4l2_dev	Internal v4l2 parent device
+ */
+struct vimc_device {
+	struct platform_device pdev;
+	const struct vimc_pipeline_config *pipe_cfg;
+	struct vimc_ent_device **ent_devs;
+	struct media_device mdev;
+	struct v4l2_device v4l2_dev;
+};
+
+/**
+ * struct vimc_ent_config	Structure which describes individual
+ *				configuration for each entity
+ *
+ * @name			entity name
+ * @ved				pointer to vimc_ent_device (a node in the
+ *					topology)
+ * @add				subdev add hook - initializes and registers
+ *					subdev called from vimc-core
+ * @rm				subdev rm hook - unregisters and frees
+ *					subdev called from vimc-core
+ */
+struct vimc_ent_config {
+	const char *name;
+	struct vimc_ent_device *(*add)(struct vimc_device *vimc,
+				       const char *vcfg_name);
+	void (*rm)(struct vimc_device *vimc, struct vimc_ent_device *ved);
+};
+
+/* prototypes for vimc_ent_config add and rm hooks */
+struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
+				     const char *vcfg_name);
+void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
+
+struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
+				     const char *vcfg_name);
+void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
+
+struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
+				     const char *vcfg_name);
+void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
+
+struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
+				     const char *vcfg_name);
+void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
+
 /**
  * vimc_pads_init - initialize pads
  *
diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
index 571c55aa0e16..a1218578cb9a 100644
--- a/drivers/media/platform/vimc/vimc-core.c
+++ b/drivers/media/platform/vimc/vimc-core.c
@@ -5,7 +5,6 @@
  * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com>
  */
 
-#include <linux/component.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
@@ -24,29 +23,6 @@
 	.flags = link_flags,					\
 }
 
-struct vimc_device {
-	/* The platform device */
-	struct platform_device pdev;
-
-	/* The pipeline configuration */
-	const struct vimc_pipeline_config *pipe_cfg;
-
-	/* The Associated media_device parent */
-	struct media_device mdev;
-
-	/* Internal v4l2 parent device*/
-	struct v4l2_device v4l2_dev;
-
-	/* Subdevices */
-	struct platform_device **subdevs;
-};
-
-/* Structure which describes individual configuration for each entity */
-struct vimc_ent_config {
-	const char *name;
-	const char *drv;
-};
-
 /* Structure which describes links between entities */
 struct vimc_ent_link {
 	unsigned int src_ent;
@@ -68,43 +44,52 @@ struct vimc_pipeline_config {
  * Topology Configuration
  */
 
-static const struct vimc_ent_config ent_config[] = {
+static struct vimc_ent_config ent_config[] = {
 	{
 		.name = "Sensor A",
-		.drv = "vimc-sensor",
+		.add = vimc_sen_add,
+		.rm = vimc_sen_rm,
 	},
 	{
 		.name = "Sensor B",
-		.drv = "vimc-sensor",
+		.add = vimc_sen_add,
+		.rm = vimc_sen_rm,
 	},
 	{
 		.name = "Debayer A",
-		.drv = "vimc-debayer",
+		.add = vimc_deb_add,
+		.rm = vimc_deb_rm,
 	},
 	{
 		.name = "Debayer B",
-		.drv = "vimc-debayer",
+		.add = vimc_deb_add,
+		.rm = vimc_deb_rm,
 	},
 	{
 		.name = "Raw Capture 0",
-		.drv = "vimc-capture",
+		.add = vimc_cap_add,
+		.rm = vimc_cap_rm,
 	},
 	{
 		.name = "Raw Capture 1",
-		.drv = "vimc-capture",
+		.add = vimc_cap_add,
+		.rm = vimc_cap_rm,
 	},
 	{
-		.name = "RGB/YUV Input",
 		/* TODO: change this to vimc-input when it is implemented */
-		.drv = "vimc-sensor",
+		.name = "RGB/YUV Input",
+		.add = vimc_sen_add,
+		.rm = vimc_sen_rm,
 	},
 	{
 		.name = "Scaler",
-		.drv = "vimc-scaler",
+		.add = vimc_sca_add,
+		.rm = vimc_sca_rm,
 	},
 	{
 		.name = "RGB/YUV Capture",
-		.drv = "vimc-capture",
+		.add = vimc_cap_add,
+		.rm = vimc_cap_rm,
 	},
 };
 
@@ -127,7 +112,7 @@ static const struct vimc_ent_link ent_links[] = {
 	VIMC_ENT_LINK(7, 1, 8, 0, MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE),
 };
 
-static const struct vimc_pipeline_config pipe_cfg = {
+static struct vimc_pipeline_config pipe_cfg = {
 	.ents		= ent_config,
 	.num_ents	= ARRAY_SIZE(ent_config),
 	.links		= ent_links,
@@ -136,6 +121,14 @@ static const struct vimc_pipeline_config pipe_cfg = {
 
 /* -------------------------------------------------------------------------- */
 
+static void vimc_rm_links(struct vimc_device *vimc)
+{
+	unsigned int i;
+
+	for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
+		media_entity_remove_links(vimc->ent_devs[i]->ent);
+}
+
 static int vimc_create_links(struct vimc_device *vimc)
 {
 	unsigned int i;
@@ -144,32 +137,58 @@ static int vimc_create_links(struct vimc_device *vimc)
 	/* Initialize the links between entities */
 	for (i = 0; i < vimc->pipe_cfg->num_links; i++) {
 		const struct vimc_ent_link *link = &vimc->pipe_cfg->links[i];
-		/*
-		 * TODO: Check another way of retrieving ved struct without
-		 * relying on platform_get_drvdata
-		 */
+
 		struct vimc_ent_device *ved_src =
-			platform_get_drvdata(vimc->subdevs[link->src_ent]);
+			vimc->ent_devs[link->src_ent];
 		struct vimc_ent_device *ved_sink =
-			platform_get_drvdata(vimc->subdevs[link->sink_ent]);
+			vimc->ent_devs[link->sink_ent];
 
 		ret = media_create_pad_link(ved_src->ent, link->src_pad,
 					    ved_sink->ent, link->sink_pad,
 					    link->flags);
 		if (ret)
-			return ret;
+			goto err_rm_links;
 	}
 
 	return 0;
+
+err_rm_links:
+	vimc_rm_links(vimc);
+	return ret;
 }
 
-static int vimc_comp_bind(struct device *master)
+static int vimc_add_subdevs(struct vimc_device *vimc)
 {
-	struct vimc_device *vimc = container_of(to_platform_device(master),
-						struct vimc_device, pdev);
-	int ret;
+	unsigned int i;
+	struct vimc_ent_device *ved;
+
+	for (i = 0; i < vimc->pipe_cfg->num_ents; i++) {
+		dev_dbg(&vimc->pdev.dev, "new entity for %s\n",
+			vimc->pipe_cfg->ents[i].name);
+		ved = vimc->pipe_cfg->ents[i].add(vimc,
+					vimc->pipe_cfg->ents[i].name);
+		if (!ved) {
+			dev_err(&vimc->pdev.dev, "add new entity for %s\n",
+				vimc->pipe_cfg->ents[i].name);
+			return -1;
+		}
+		vimc->ent_devs[i] = ved;
+	}
+	return 0;
+}
+
+static void vimc_rm_subdevs(struct vimc_device *vimc)
+{
+	unsigned int i;
 
-	dev_dbg(master, "bind");
+	for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
+		if (vimc->ent_devs[i])
+			vimc->pipe_cfg->ents[i].rm(vimc, vimc->ent_devs[i]);
+}
+
+static int vimc_register_devices(struct vimc_device *vimc)
+{
+	int ret;
 
 	/* Register the v4l2 struct */
 	ret = v4l2_device_register(vimc->mdev.dev, &vimc->v4l2_dev);
@@ -179,22 +198,30 @@ static int vimc_comp_bind(struct device *master)
 		return ret;
 	}
 
-	/* Bind subdevices */
-	ret = component_bind_all(master, &vimc->v4l2_dev);
-	if (ret)
+	/* allocate ent_devs */
+	vimc->ent_devs = kmalloc_array(vimc->pipe_cfg->num_ents,
+				       sizeof(*vimc->ent_devs),
+				       GFP_KERNEL);
+	if (!vimc->ent_devs)
 		goto err_v4l2_unregister;
 
+	/* 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;
+
 	/* Initialize links */
 	ret = vimc_create_links(vimc);
 	if (ret)
-		goto err_comp_unbind_all;
+		goto err_rm_subdevs;
 
 	/* Register the media device */
 	ret = media_device_register(&vimc->mdev);
 	if (ret) {
 		dev_err(vimc->mdev.dev,
 			"media device register failed (err=%d)\n", ret);
-		goto err_comp_unbind_all;
+		goto err_rm_subdevs;
 	}
 
 	/* Expose all subdev's nodes*/
@@ -211,98 +238,32 @@ static int vimc_comp_bind(struct device *master)
 err_mdev_unregister:
 	media_device_unregister(&vimc->mdev);
 	media_device_cleanup(&vimc->mdev);
-err_comp_unbind_all:
-	component_unbind_all(master, NULL);
+err_rm_subdevs:
+	vimc_rm_subdevs(vimc);
+	kfree(vimc->ent_devs);
 err_v4l2_unregister:
 	v4l2_device_unregister(&vimc->v4l2_dev);
 
 	return ret;
 }
 
-static void vimc_comp_unbind(struct device *master)
+static void vimc_unregister(struct vimc_device *vimc)
 {
-	struct vimc_device *vimc = container_of(to_platform_device(master),
-						struct vimc_device, pdev);
-
-	dev_dbg(master, "unbind");
-
 	media_device_unregister(&vimc->mdev);
 	media_device_cleanup(&vimc->mdev);
-	component_unbind_all(master, NULL);
 	v4l2_device_unregister(&vimc->v4l2_dev);
+	kfree(vimc->ent_devs);
 }
 
-static int vimc_comp_compare(struct device *comp, void *data)
-{
-	return comp == data;
-}
-
-static struct component_match *vimc_add_subdevs(struct vimc_device *vimc)
-{
-	struct component_match *match = NULL;
-	struct vimc_platform_data pdata;
-	int i;
-
-	for (i = 0; i < vimc->pipe_cfg->num_ents; i++) {
-		dev_dbg(&vimc->pdev.dev, "new pdev for %s\n",
-			vimc->pipe_cfg->ents[i].drv);
-
-		strscpy(pdata.entity_name, vimc->pipe_cfg->ents[i].name,
-			sizeof(pdata.entity_name));
-
-		vimc->subdevs[i] = platform_device_register_data(&vimc->pdev.dev,
-						vimc->pipe_cfg->ents[i].drv,
-						PLATFORM_DEVID_AUTO,
-						&pdata,
-						sizeof(pdata));
-		if (IS_ERR(vimc->subdevs[i])) {
-			match = ERR_CAST(vimc->subdevs[i]);
-			while (--i >= 0)
-				platform_device_unregister(vimc->subdevs[i]);
-
-			return match;
-		}
-
-		component_match_add(&vimc->pdev.dev, &match, vimc_comp_compare,
-				    &vimc->subdevs[i]->dev);
-	}
-
-	return match;
-}
-
-static void vimc_rm_subdevs(struct vimc_device *vimc)
-{
-	unsigned int i;
-
-	for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
-		platform_device_unregister(vimc->subdevs[i]);
-}
-
-static const struct component_master_ops vimc_comp_ops = {
-	.bind = vimc_comp_bind,
-	.unbind = vimc_comp_unbind,
-};
-
 static int vimc_probe(struct platform_device *pdev)
 {
 	struct vimc_device *vimc = container_of(pdev, struct vimc_device, pdev);
-	struct component_match *match = NULL;
 	int ret;
 
 	dev_dbg(&pdev->dev, "probe");
 
 	memset(&vimc->mdev, 0, sizeof(vimc->mdev));
 
-	/* Create platform_device for each entity in the topology*/
-	vimc->subdevs = devm_kcalloc(&vimc->pdev.dev, vimc->pipe_cfg->num_ents,
-				     sizeof(*vimc->subdevs), GFP_KERNEL);
-	if (!vimc->subdevs)
-		return -ENOMEM;
-
-	match = vimc_add_subdevs(vimc);
-	if (IS_ERR(match))
-		return PTR_ERR(match);
-
 	/* Link the media device within the v4l2_device */
 	vimc->v4l2_dev.mdev = &vimc->mdev;
 
@@ -314,12 +275,9 @@ static int vimc_probe(struct platform_device *pdev)
 	vimc->mdev.dev = &pdev->dev;
 	media_device_init(&vimc->mdev);
 
-	/* Add self to the component system */
-	ret = component_master_add_with_match(&pdev->dev, &vimc_comp_ops,
-					      match);
+	ret = vimc_register_devices(vimc);
 	if (ret) {
 		media_device_cleanup(&vimc->mdev);
-		vimc_rm_subdevs(vimc);
 		return ret;
 	}
 
@@ -332,8 +290,8 @@ static int vimc_remove(struct platform_device *pdev)
 
 	dev_dbg(&pdev->dev, "remove");
 
-	component_master_del(&pdev->dev, &vimc_comp_ops);
 	vimc_rm_subdevs(vimc);
+	vimc_unregister(vimc);
 
 	return 0;
 }
diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
index b72b8385067b..00850f2501ad 100644
--- a/drivers/media/platform/vimc/vimc-debayer.c
+++ b/drivers/media/platform/vimc/vimc-debayer.c
@@ -15,8 +15,6 @@
 
 #include "vimc-common.h"
 
-#define VIMC_DEB_DRV_NAME "vimc-debayer"
-
 static unsigned int deb_mean_win_size = 3;
 module_param(deb_mean_win_size, uint, 0000);
 MODULE_PARM_DESC(deb_mean_win_size, " the window size to calculate the mean.\n"
@@ -491,44 +489,40 @@ static const struct v4l2_subdev_internal_ops vimc_deb_int_ops = {
 	.release = vimc_deb_release,
 };
 
-static void vimc_deb_comp_unbind(struct device *comp, struct device *master,
-				 void *master_data)
+void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
 {
-	struct vimc_ent_device *ved = dev_get_drvdata(comp);
-	struct vimc_deb_device *vdeb = container_of(ved, struct vimc_deb_device,
-						    ved);
+	struct vimc_deb_device *vdeb;
 
+	vdeb = container_of(ved, struct vimc_deb_device, ved);
 	vimc_ent_sd_unregister(ved, &vdeb->sd);
 }
 
-static int vimc_deb_comp_bind(struct device *comp, struct device *master,
-			      void *master_data)
+struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
+				     const char *vcfg_name)
 {
-	struct v4l2_device *v4l2_dev = master_data;
-	struct vimc_platform_data *pdata = comp->platform_data;
+	struct v4l2_device *v4l2_dev = &vimc->v4l2_dev;
 	struct vimc_deb_device *vdeb;
 	int ret;
 
 	/* Allocate the vdeb struct */
 	vdeb = kzalloc(sizeof(*vdeb), GFP_KERNEL);
 	if (!vdeb)
-		return -ENOMEM;
+		return NULL;
 
 	/* Initialize ved and sd */
 	ret = vimc_ent_sd_register(&vdeb->ved, &vdeb->sd, v4l2_dev,
-				   pdata->entity_name,
+				   vcfg_name,
 				   MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV, 2,
 				   (const unsigned long[2]) {MEDIA_PAD_FL_SINK,
 				   MEDIA_PAD_FL_SOURCE},
 				   &vimc_deb_int_ops, &vimc_deb_ops);
 	if (ret) {
 		kfree(vdeb);
-		return ret;
+		return NULL;
 	}
 
 	vdeb->ved.process_frame = vimc_deb_process_frame;
-	dev_set_drvdata(comp, &vdeb->ved);
-	vdeb->dev = comp;
+	vdeb->dev = &vimc->pdev.dev;
 
 	/* Initialize the frame format */
 	vdeb->sink_fmt = sink_fmt_default;
@@ -541,46 +535,5 @@ static int vimc_deb_comp_bind(struct device *comp, struct device *master,
 	vdeb->src_code = MEDIA_BUS_FMT_RGB888_1X24;
 	vdeb->set_rgb_src = vimc_deb_set_rgb_mbus_fmt_rgb888_1x24;
 
-	return 0;
+	return &vdeb->ved;
 }
-
-static const struct component_ops vimc_deb_comp_ops = {
-	.bind = vimc_deb_comp_bind,
-	.unbind = vimc_deb_comp_unbind,
-};
-
-static int vimc_deb_probe(struct platform_device *pdev)
-{
-	return component_add(&pdev->dev, &vimc_deb_comp_ops);
-}
-
-static int vimc_deb_remove(struct platform_device *pdev)
-{
-	component_del(&pdev->dev, &vimc_deb_comp_ops);
-
-	return 0;
-}
-
-static const struct platform_device_id vimc_deb_driver_ids[] = {
-	{
-		.name           = VIMC_DEB_DRV_NAME,
-	},
-	{ }
-};
-
-static struct platform_driver vimc_deb_pdrv = {
-	.probe		= vimc_deb_probe,
-	.remove		= vimc_deb_remove,
-	.id_table	= vimc_deb_driver_ids,
-	.driver		= {
-		.name	= VIMC_DEB_DRV_NAME,
-	},
-};
-
-module_platform_driver(vimc_deb_pdrv);
-
-MODULE_DEVICE_TABLE(platform, vimc_deb_driver_ids);
-
-MODULE_DESCRIPTION("Virtual Media Controller Driver (VIMC) Debayer");
-MODULE_AUTHOR("Helen Mae Koike Fornazier <helen.fornazier@gmail.com>");
-MODULE_LICENSE("GPL");
diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
index 49ab8d9dd9c9..28a947950a0b 100644
--- a/drivers/media/platform/vimc/vimc-scaler.c
+++ b/drivers/media/platform/vimc/vimc-scaler.c
@@ -5,18 +5,13 @@
  * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com>
  */
 
-#include <linux/component.h>
 #include <linux/module.h>
-#include <linux/mod_devicetable.h>
-#include <linux/platform_device.h>
 #include <linux/vmalloc.h>
 #include <linux/v4l2-mediabus.h>
 #include <media/v4l2-subdev.h>
 
 #include "vimc-common.h"
 
-#define VIMC_SCA_DRV_NAME "vimc-scaler"
-
 static unsigned int sca_mult = 3;
 module_param(sca_mult, uint, 0000);
 MODULE_PARM_DESC(sca_mult, " the image size multiplier");
@@ -350,89 +345,43 @@ static const struct v4l2_subdev_internal_ops vimc_sca_int_ops = {
 	.release = vimc_sca_release,
 };
 
-static void vimc_sca_comp_unbind(struct device *comp, struct device *master,
-				 void *master_data)
+void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
 {
-	struct vimc_ent_device *ved = dev_get_drvdata(comp);
-	struct vimc_sca_device *vsca = container_of(ved, struct vimc_sca_device,
-						    ved);
+	struct vimc_sca_device *vsca;
 
+	vsca = container_of(ved, struct vimc_sca_device, ved);
 	vimc_ent_sd_unregister(ved, &vsca->sd);
 }
 
-
-static int vimc_sca_comp_bind(struct device *comp, struct device *master,
-			      void *master_data)
+struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
+				     const char *vcfg_name)
 {
-	struct v4l2_device *v4l2_dev = master_data;
-	struct vimc_platform_data *pdata = comp->platform_data;
+	struct v4l2_device *v4l2_dev = &vimc->v4l2_dev;
 	struct vimc_sca_device *vsca;
 	int ret;
 
 	/* Allocate the vsca struct */
 	vsca = kzalloc(sizeof(*vsca), GFP_KERNEL);
 	if (!vsca)
-		return -ENOMEM;
+		return NULL;
 
 	/* Initialize ved and sd */
 	ret = vimc_ent_sd_register(&vsca->ved, &vsca->sd, v4l2_dev,
-				   pdata->entity_name,
+				   vcfg_name,
 				   MEDIA_ENT_F_PROC_VIDEO_SCALER, 2,
 				   (const unsigned long[2]) {MEDIA_PAD_FL_SINK,
 				   MEDIA_PAD_FL_SOURCE},
 				   &vimc_sca_int_ops, &vimc_sca_ops);
 	if (ret) {
 		kfree(vsca);
-		return ret;
+		return NULL;
 	}
 
 	vsca->ved.process_frame = vimc_sca_process_frame;
-	dev_set_drvdata(comp, &vsca->ved);
-	vsca->dev = comp;
+	vsca->dev = &vimc->pdev.dev;
 
 	/* Initialize the frame format */
 	vsca->sink_fmt = sink_fmt_default;
 
-	return 0;
-}
-
-static const struct component_ops vimc_sca_comp_ops = {
-	.bind = vimc_sca_comp_bind,
-	.unbind = vimc_sca_comp_unbind,
-};
-
-static int vimc_sca_probe(struct platform_device *pdev)
-{
-	return component_add(&pdev->dev, &vimc_sca_comp_ops);
+	return &vsca->ved;
 }
-
-static int vimc_sca_remove(struct platform_device *pdev)
-{
-	component_del(&pdev->dev, &vimc_sca_comp_ops);
-
-	return 0;
-}
-
-static const struct platform_device_id vimc_sca_driver_ids[] = {
-	{
-		.name           = VIMC_SCA_DRV_NAME,
-	},
-	{ }
-};
-
-static struct platform_driver vimc_sca_pdrv = {
-	.probe		= vimc_sca_probe,
-	.remove		= vimc_sca_remove,
-	.id_table	= vimc_sca_driver_ids,
-	.driver		= {
-		.name	= VIMC_SCA_DRV_NAME,
-	},
-};
-
-module_platform_driver(vimc_sca_pdrv);
-
-MODULE_DEVICE_TABLE(platform, vimc_sca_driver_ids);
-
-MODULE_DESCRIPTION("Virtual Media Controller Driver (VIMC) Scaler");
-MODULE_AUTHOR("Helen Mae Koike Fornazier <helen.fornazier@gmail.com>");
-MODULE_LICENSE("GPL");
diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
index 6c53b9fc1617..1f15637ca8bb 100644
--- a/drivers/media/platform/vimc/vimc-sensor.c
+++ b/drivers/media/platform/vimc/vimc-sensor.c
@@ -5,10 +5,6 @@
  * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com>
  */
 
-#include <linux/component.h>
-#include <linux/module.h>
-#include <linux/mod_devicetable.h>
-#include <linux/platform_device.h>
 #include <linux/v4l2-mediabus.h>
 #include <linux/vmalloc.h>
 #include <media/v4l2-ctrls.h>
@@ -18,8 +14,6 @@
 
 #include "vimc-common.h"
 
-#define VIMC_SEN_DRV_NAME "vimc-sensor"
-
 struct vimc_sen_device {
 	struct vimc_ent_device ved;
 	struct v4l2_subdev sd;
@@ -304,13 +298,11 @@ static const struct v4l2_subdev_internal_ops vimc_sen_int_ops = {
 	.release = vimc_sen_release,
 };
 
-static void vimc_sen_comp_unbind(struct device *comp, struct device *master,
-				 void *master_data)
+void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
 {
-	struct vimc_ent_device *ved = dev_get_drvdata(comp);
-	struct vimc_sen_device *vsen =
-				container_of(ved, struct vimc_sen_device, ved);
+	struct vimc_sen_device *vsen;
 
+	vsen = container_of(ved, struct vimc_sen_device, ved);
 	vimc_ent_sd_unregister(ved, &vsen->sd);
 }
 
@@ -331,18 +323,17 @@ static const struct v4l2_ctrl_config vimc_sen_ctrl_test_pattern = {
 	.qmenu = tpg_pattern_strings,
 };
 
-static int vimc_sen_comp_bind(struct device *comp, struct device *master,
-			      void *master_data)
+struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
+				     const char *vcfg_name)
 {
-	struct v4l2_device *v4l2_dev = master_data;
-	struct vimc_platform_data *pdata = comp->platform_data;
+	struct v4l2_device *v4l2_dev = &vimc->v4l2_dev;
 	struct vimc_sen_device *vsen;
 	int ret;
 
 	/* Allocate the vsen struct */
 	vsen = kzalloc(sizeof(*vsen), GFP_KERNEL);
 	if (!vsen)
-		return -ENOMEM;
+		return NULL;
 
 	v4l2_ctrl_handler_init(&vsen->hdl, 4);
 
@@ -368,7 +359,7 @@ static int vimc_sen_comp_bind(struct device *comp, struct device *master,
 
 	/* Initialize ved and sd */
 	ret = vimc_ent_sd_register(&vsen->ved, &vsen->sd, v4l2_dev,
-				   pdata->entity_name,
+				   vcfg_name,
 				   MEDIA_ENT_F_CAM_SENSOR, 1,
 				   (const unsigned long[1]) {MEDIA_PAD_FL_SOURCE},
 				   &vimc_sen_int_ops, &vimc_sen_ops);
@@ -376,8 +367,7 @@ static int vimc_sen_comp_bind(struct device *comp, struct device *master,
 		goto err_free_hdl;
 
 	vsen->ved.process_frame = vimc_sen_process_frame;
-	dev_set_drvdata(comp, &vsen->ved);
-	vsen->dev = comp;
+	vsen->dev = &vimc->pdev.dev;
 
 	/* Initialize the frame format */
 	vsen->mbus_format = fmt_default;
@@ -389,7 +379,7 @@ static int vimc_sen_comp_bind(struct device *comp, struct device *master,
 	if (ret)
 		goto err_unregister_ent_sd;
 
-	return 0;
+	return &vsen->ved;
 
 err_unregister_ent_sd:
 	vimc_ent_sd_unregister(&vsen->ved,  &vsen->sd);
@@ -398,46 +388,5 @@ static int vimc_sen_comp_bind(struct device *comp, struct device *master,
 err_free_vsen:
 	kfree(vsen);
 
-	return ret;
+	return NULL;
 }
-
-static const struct component_ops vimc_sen_comp_ops = {
-	.bind = vimc_sen_comp_bind,
-	.unbind = vimc_sen_comp_unbind,
-};
-
-static int vimc_sen_probe(struct platform_device *pdev)
-{
-	return component_add(&pdev->dev, &vimc_sen_comp_ops);
-}
-
-static int vimc_sen_remove(struct platform_device *pdev)
-{
-	component_del(&pdev->dev, &vimc_sen_comp_ops);
-
-	return 0;
-}
-
-static const struct platform_device_id vimc_sen_driver_ids[] = {
-	{
-		.name           = VIMC_SEN_DRV_NAME,
-	},
-	{ }
-};
-
-static struct platform_driver vimc_sen_pdrv = {
-	.probe		= vimc_sen_probe,
-	.remove		= vimc_sen_remove,
-	.id_table	= vimc_sen_driver_ids,
-	.driver		= {
-		.name	= VIMC_SEN_DRV_NAME,
-	},
-};
-
-module_platform_driver(vimc_sen_pdrv);
-
-MODULE_DEVICE_TABLE(platform, vimc_sen_driver_ids);
-
-MODULE_DESCRIPTION("Virtual Media Controller Driver (VIMC) Sensor");
-MODULE_AUTHOR("Helen Mae Koike Fornazier <helen.fornazier@gmail.com>");
-MODULE_LICENSE("GPL");
-- 
2.20.1

Re: [PATCH v4 1/5] media: vimc: Collapse component structure into a single monolithic driver

[Helen Koike]

Hi Shuah,

Thanks for the patch.

On 9/6/19 11:42 PM, Shuah Khan wrote:
> vimc uses Component API to split the driver into functional components.
> The real hardware resembles a monolith structure than component and
> component structure added a level of complexity making it hard to
> maintain without adding any real benefit.
> 
> The sensor is one vimc component that would makes sense to be a separate
> module to closely align with the real hardware. It would be easier to
> collapse vimc into single monolithic driver first and then split the
> sensor off as a separate module.
> 
> Collapse it into a single monolithic driver removing the Component API.
> This patch removes the component API and makes minimal changes to the
> code base preserving the functional division of the code structure.
> Preserving the functional structure allows us to split the sensor off
> as a separate module in the future.
> 
> Major design elements in this change are:
> - Use existing struct vimc_ent_config and struct vimc_pipeline_config
>   to drive the initialization of the functional components.
> - Make vimc_device and vimc_ent_config global by moving them to
>   vimc-common.h
> - Add two new hooks add and rm to initialize and register, unregister
>   and free subdevs.
> - All component API is now gone and bind and unbind hooks are modified
>   to do "add" and "rm" with minimal changes to just add and rm subdevs.
> - vimc-core's bind and unbind are now register and unregister.
> - Add a new field to vimc_device structure for saving the pointers to
>   struct vimc_ent_device(s) subdevs create in their "add" hooks. These
>   get used to create links and removing the subdevs. vimc-core allocates
>   this array which sized to number of entries in the topology defined in
>   the vimc_pipeline_config structure.
> - vimc-core invokes "add" hooks from its vimc_register_devices().
>   The "add" hooks remain the same and register subdevs. They don't
>   create platform devices of their own and use vimc's pdev.dev as
>   their reference device. Each "add" hook returns pointer to its struct
>   vimc_ent_device. This is saved in the vimc_device ent_devs array.
> - vimc-core invokes "rm" hooks from its unregister to unregister subdevs
>   and cleanup.
> - vimc-core invokes "add" and "rm" hooks with pointer to struct vimc_device
>   and the corresponding vimc_ent_device saved in the ent_devs.
> 
> The following configure and stream test works on all devices.
> 
> media-ctl -d platform:vimc -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
> media-ctl -d platform:vimc -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
> media-ctl -d platform:vimc -V '"Sensor B":0[fmt:SBGGR8_1X8/640x480]'
> media-ctl -d platform:vimc -V '"Debayer B":0[fmt:SBGGR8_1X8/640x480]'
> 
> v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=1920,height=1440
> v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v pixelformat=BA81
> v4l2-ctl -z platform:vimc -d "Raw Capture 1" -v pixelformat=BA81
> 
> v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video1
> v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video2
> v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video3
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> ---
>  drivers/media/platform/vimc/Makefile       |   7 +-
>  drivers/media/platform/vimc/vimc-capture.c |  81 ++------
>  drivers/media/platform/vimc/vimc-common.h  |  54 ++++++
>  drivers/media/platform/vimc/vimc-core.c    | 216 +++++++++------------
>  drivers/media/platform/vimc/vimc-debayer.c |  69 ++-----
>  drivers/media/platform/vimc/vimc-scaler.c  |  73 ++-----
>  drivers/media/platform/vimc/vimc-sensor.c  |  73 ++-----
>  7 files changed, 193 insertions(+), 380 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/Makefile b/drivers/media/platform/vimc/Makefile
> index 96d06f030c31..a53b2b532e9f 100644
> --- a/drivers/media/platform/vimc/Makefile
> +++ b/drivers/media/platform/vimc/Makefile
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
> -vimc-y := vimc-core.o vimc-common.o vimc-streamer.o
> +vimc-y := vimc-core.o vimc-common.o vimc-streamer.o vimc-capture.o \
> +		vimc-debayer.o vimc-scaler.o vimc-sensor.o
> +
> +obj-$(CONFIG_VIDEO_VIMC) += vimc.o
>  
> -obj-$(CONFIG_VIDEO_VIMC) += vimc.o vimc-capture.o vimc-debayer.o \
> -                vimc-scaler.o vimc-sensor.o
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index 1d56b91830ba..602f80323031 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -5,10 +5,6 @@
>   * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com>
>   */
>  
> -#include <linux/component.h>
> -#include <linux/module.h>
> -#include <linux/mod_devicetable.h>
> -#include <linux/platform_device.h>
>  #include <media/v4l2-ioctl.h>
>  #include <media/videobuf2-core.h>
>  #include <media/videobuf2-vmalloc.h>
> @@ -16,8 +12,6 @@
>  #include "vimc-common.h"
>  #include "vimc-streamer.h"
>  
> -#define VIMC_CAP_DRV_NAME "vimc-capture"
> -
>  struct vimc_cap_device {
>  	struct vimc_ent_device ved;
>  	struct video_device vdev;
> @@ -340,13 +334,11 @@ static void vimc_cap_release(struct video_device *vdev)
>  	kfree(vcap);
>  }
>  
> -static void vimc_cap_comp_unbind(struct device *comp, struct device *master,
> -				 void *master_data)
> +void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
>  {
> -	struct vimc_ent_device *ved = dev_get_drvdata(comp);
> -	struct vimc_cap_device *vcap = container_of(ved, struct vimc_cap_device,
> -						    ved);
> +	struct vimc_cap_device *vcap;
>  
> +	vcap = container_of(ved, struct vimc_cap_device, ved);
>  	vb2_queue_release(&vcap->queue);
>  	media_entity_cleanup(ved->ent);
>  	video_unregister_device(&vcap->vdev);
> @@ -391,11 +383,10 @@ static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
>  	return NULL;
>  }
>  
> -static int vimc_cap_comp_bind(struct device *comp, struct device *master,
> -			      void *master_data)
> +struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
> +				     const char *vcfg_name)
>  {
> -	struct v4l2_device *v4l2_dev = master_data;
> -	struct vimc_platform_data *pdata = comp->platform_data;
> +	struct v4l2_device *v4l2_dev = &vimc->v4l2_dev;
>  	const struct vimc_pix_map *vpix;
>  	struct vimc_cap_device *vcap;
>  	struct video_device *vdev;
> @@ -405,7 +396,7 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
>  	/* Allocate the vimc_cap_device struct */
>  	vcap = kzalloc(sizeof(*vcap), GFP_KERNEL);
>  	if (!vcap)
> -		return -ENOMEM;
> +		return NULL;
>  
>  	/* Allocate the pads */
>  	vcap->ved.pads =
> @@ -416,7 +407,7 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
>  	}
>  
>  	/* Initialize the media entity */
> -	vcap->vdev.entity.name = pdata->entity_name;
> +	vcap->vdev.entity.name = vcfg_name;
>  	vcap->vdev.entity.function = MEDIA_ENT_F_IO_V4L;
>  	ret = media_entity_pads_init(&vcap->vdev.entity,
>  				     1, vcap->ved.pads);
> @@ -440,8 +431,8 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
>  
>  	ret = vb2_queue_init(q);
>  	if (ret) {
> -		dev_err(comp, "%s: vb2 queue init failed (err=%d)\n",
> -			pdata->entity_name, ret);
> +		dev_err(&vimc->pdev.dev, "%s: vb2 queue init failed (err=%d)\n",
> +			vcfg_name, ret);
>  		goto err_clean_m_ent;
>  	}
>  
> @@ -460,8 +451,7 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
>  	vcap->ved.ent = &vcap->vdev.entity;
>  	vcap->ved.process_frame = vimc_cap_process_frame;
>  	vcap->ved.vdev_get_format = vimc_cap_get_format;
> -	dev_set_drvdata(comp, &vcap->ved);
> -	vcap->dev = comp;
> +	vcap->dev = &vimc->pdev.dev;
>  
>  	/* Initialize the video_device struct */
>  	vdev = &vcap->vdev;
> @@ -474,18 +464,18 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
>  	vdev->queue = q;
>  	vdev->v4l2_dev = v4l2_dev;
>  	vdev->vfl_dir = VFL_DIR_RX;
> -	strscpy(vdev->name, pdata->entity_name, sizeof(vdev->name));
> +	strscpy(vdev->name, vcfg_name, sizeof(vdev->name));
>  	video_set_drvdata(vdev, &vcap->ved);
>  
>  	/* Register the video_device with the v4l2 and the media framework */
>  	ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
>  	if (ret) {
> -		dev_err(comp, "%s: video register failed (err=%d)\n",
> +		dev_err(&vimc->pdev.dev, "%s: video register failed (err=%d)\n",
>  			vcap->vdev.name, ret);
>  		goto err_release_queue;
>  	}
>  
> -	return 0;
> +	return &vcap->ved;
>  
>  err_release_queue:
>  	vb2_queue_release(q);
> @@ -496,46 +486,5 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
>  err_free_vcap:
>  	kfree(vcap);
>  
> -	return ret;
> -}
> -
> -static const struct component_ops vimc_cap_comp_ops = {
> -	.bind = vimc_cap_comp_bind,
> -	.unbind = vimc_cap_comp_unbind,
> -};
> -
> -static int vimc_cap_probe(struct platform_device *pdev)
> -{
> -	return component_add(&pdev->dev, &vimc_cap_comp_ops);
> -}
> -
> -static int vimc_cap_remove(struct platform_device *pdev)
> -{
> -	component_del(&pdev->dev, &vimc_cap_comp_ops);
> -
> -	return 0;
> +	return NULL;
>  }
> -
> -static const struct platform_device_id vimc_cap_driver_ids[] = {
> -	{
> -		.name           = VIMC_CAP_DRV_NAME,
> -	},
> -	{ }
> -};
> -
> -static struct platform_driver vimc_cap_pdrv = {
> -	.probe		= vimc_cap_probe,
> -	.remove		= vimc_cap_remove,
> -	.id_table	= vimc_cap_driver_ids,
> -	.driver		= {
> -		.name	= VIMC_CAP_DRV_NAME,
> -	},
> -};
> -
> -module_platform_driver(vimc_cap_pdrv);
> -
> -MODULE_DEVICE_TABLE(platform, vimc_cap_driver_ids);
> -
> -MODULE_DESCRIPTION("Virtual Media Controller Driver (VIMC) Capture");
> -MODULE_AUTHOR("Helen Mae Koike Fornazier <helen.fornazier@gmail.com>");
> -MODULE_LICENSE("GPL");
> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
> index 9c2e0e216c6b..87ee84f78322 100644
> --- a/drivers/media/platform/vimc/vimc-common.h
> +++ b/drivers/media/platform/vimc/vimc-common.h
> @@ -9,6 +9,7 @@
>  #define _VIMC_COMMON_H_
>  
>  #include <linux/slab.h>
> +#include <linux/platform_device.h>

This should go before linux/slab.h to keep in alphabetical order.

>  #include <media/media-device.h>
>  #include <media/v4l2-device.h>
>  
> @@ -111,6 +112,59 @@ struct vimc_ent_device {
>  			      struct v4l2_pix_format *fmt);
>  };
>  
> +/**
> + * struct vimc_device - main device for vimc driver
> + *
> + * @pdev	pointer to the platform device
> + * @pipe_cfg	pointer to the vimc pipeline configuration structure
> + * @ent_devs	arry of vimc_ent_device pointers

s/arry/array

> + * @mdev	the associated media_device parent
> + * @v4l2_dev	Internal v4l2 parent device
> + */
> +struct vimc_device {
> +	struct platform_device pdev;
> +	const struct vimc_pipeline_config *pipe_cfg;
> +	struct vimc_ent_device **ent_devs;
> +	struct media_device mdev;
> +	struct v4l2_device v4l2_dev;
> +};
> +
> +/**
> + * struct vimc_ent_config	Structure which describes individual
> + *				configuration for each entity
> + *
> + * @name			entity name
> + * @ved				pointer to vimc_ent_device (a node in the
> + *					topology)
> + * @add				subdev add hook - initializes and registers
> + *					subdev called from vimc-core
> + * @rm				subdev rm hook - unregisters and frees
> + *					subdev called from vimc-core
> + */
> +struct vimc_ent_config {
> +	const char *name;
> +	struct vimc_ent_device *(*add)(struct vimc_device *vimc,
> +				       const char *vcfg_name);
> +	void (*rm)(struct vimc_device *vimc, struct vimc_ent_device *ved);
> +};
> +
> +/* prototypes for vimc_ent_config add and rm hooks */
> +struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
> +				     const char *vcfg_name);
> +void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
> +
> +struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
> +				     const char *vcfg_name);
> +void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
> +
> +struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
> +				     const char *vcfg_name);
> +void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
> +
> +struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
> +				     const char *vcfg_name);
> +void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
> +
>  /**
>   * vimc_pads_init - initialize pads
>   *
> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
> index 571c55aa0e16..a1218578cb9a 100644
> --- a/drivers/media/platform/vimc/vimc-core.c
> +++ b/drivers/media/platform/vimc/vimc-core.c
> @@ -5,7 +5,6 @@
>   * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com>
>   */
>  
> -#include <linux/component.h>
>  #include <linux/init.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> @@ -24,29 +23,6 @@
>  	.flags = link_flags,					\
>  }
>  
> -struct vimc_device {
> -	/* The platform device */
> -	struct platform_device pdev;
> -
> -	/* The pipeline configuration */
> -	const struct vimc_pipeline_config *pipe_cfg;
> -
> -	/* The Associated media_device parent */
> -	struct media_device mdev;
> -
> -	/* Internal v4l2 parent device*/
> -	struct v4l2_device v4l2_dev;
> -
> -	/* Subdevices */
> -	struct platform_device **subdevs;
> -};
> -
> -/* Structure which describes individual configuration for each entity */
> -struct vimc_ent_config {
> -	const char *name;
> -	const char *drv;
> -};
> -
>  /* Structure which describes links between entities */
>  struct vimc_ent_link {
>  	unsigned int src_ent;
> @@ -68,43 +44,52 @@ struct vimc_pipeline_config {
>   * Topology Configuration
>   */
>  
> -static const struct vimc_ent_config ent_config[] = {
> +static struct vimc_ent_config ent_config[] = {
>  	{
>  		.name = "Sensor A",
> -		.drv = "vimc-sensor",
> +		.add = vimc_sen_add,
> +		.rm = vimc_sen_rm,
>  	},
>  	{
>  		.name = "Sensor B",
> -		.drv = "vimc-sensor",
> +		.add = vimc_sen_add,
> +		.rm = vimc_sen_rm,
>  	},
>  	{
>  		.name = "Debayer A",
> -		.drv = "vimc-debayer",
> +		.add = vimc_deb_add,
> +		.rm = vimc_deb_rm,
>  	},
>  	{
>  		.name = "Debayer B",
> -		.drv = "vimc-debayer",
> +		.add = vimc_deb_add,
> +		.rm = vimc_deb_rm,
>  	},
>  	{
>  		.name = "Raw Capture 0",
> -		.drv = "vimc-capture",
> +		.add = vimc_cap_add,
> +		.rm = vimc_cap_rm,
>  	},
>  	{
>  		.name = "Raw Capture 1",
> -		.drv = "vimc-capture",
> +		.add = vimc_cap_add,
> +		.rm = vimc_cap_rm,
>  	},
>  	{
> -		.name = "RGB/YUV Input",
>  		/* TODO: change this to vimc-input when it is implemented */
> -		.drv = "vimc-sensor",
> +		.name = "RGB/YUV Input",
> +		.add = vimc_sen_add,
> +		.rm = vimc_sen_rm,
>  	},
>  	{
>  		.name = "Scaler",
> -		.drv = "vimc-scaler",
> +		.add = vimc_sca_add,
> +		.rm = vimc_sca_rm,
>  	},
>  	{
>  		.name = "RGB/YUV Capture",
> -		.drv = "vimc-capture",
> +		.add = vimc_cap_add,
> +		.rm = vimc_cap_rm,
>  	},
>  };
>  
> @@ -127,7 +112,7 @@ static const struct vimc_ent_link ent_links[] = {
>  	VIMC_ENT_LINK(7, 1, 8, 0, MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE),
>  };
>  
> -static const struct vimc_pipeline_config pipe_cfg = {
> +static struct vimc_pipeline_config pipe_cfg = {
>  	.ents		= ent_config,
>  	.num_ents	= ARRAY_SIZE(ent_config),
>  	.links		= ent_links,
> @@ -136,6 +121,14 @@ static const struct vimc_pipeline_config pipe_cfg = {
>  
>  /* -------------------------------------------------------------------------- */
>  
> +static void vimc_rm_links(struct vimc_device *vimc)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
> +		media_entity_remove_links(vimc->ent_devs[i]->ent);
> +}
> +
>  static int vimc_create_links(struct vimc_device *vimc)
>  {
>  	unsigned int i;
> @@ -144,32 +137,58 @@ static int vimc_create_links(struct vimc_device *vimc)
>  	/* Initialize the links between entities */
>  	for (i = 0; i < vimc->pipe_cfg->num_links; i++) {
>  		const struct vimc_ent_link *link = &vimc->pipe_cfg->links[i];
> -		/*
> -		 * TODO: Check another way of retrieving ved struct without
> -		 * relying on platform_get_drvdata
> -		 */
> +
>  		struct vimc_ent_device *ved_src =
> -			platform_get_drvdata(vimc->subdevs[link->src_ent]);
> +			vimc->ent_devs[link->src_ent];
>  		struct vimc_ent_device *ved_sink =
> -			platform_get_drvdata(vimc->subdevs[link->sink_ent]);
> +			vimc->ent_devs[link->sink_ent];
>  
>  		ret = media_create_pad_link(ved_src->ent, link->src_pad,
>  					    ved_sink->ent, link->sink_pad,
>  					    link->flags);
>  		if (ret)
> -			return ret;
> +			goto err_rm_links;
>  	}
>  
>  	return 0;
> +
> +err_rm_links:
> +	vimc_rm_links(vimc);
> +	return ret;
>  }
>  
> -static int vimc_comp_bind(struct device *master)
> +static int vimc_add_subdevs(struct vimc_device *vimc)
>  {
> -	struct vimc_device *vimc = container_of(to_platform_device(master),
> -						struct vimc_device, pdev);
> -	int ret;
> +	unsigned int i;
> +	struct vimc_ent_device *ved;
> +
> +	for (i = 0; i < vimc->pipe_cfg->num_ents; i++) {
> +		dev_dbg(&vimc->pdev.dev, "new entity for %s\n",
> +			vimc->pipe_cfg->ents[i].name);
> +		ved = vimc->pipe_cfg->ents[i].add(vimc,
> +					vimc->pipe_cfg->ents[i].name);
> +		if (!ved) {
> +			dev_err(&vimc->pdev.dev, "add new entity for %s\n",
> +				vimc->pipe_cfg->ents[i].name);
> +			return -1;

return -EINVAL

> +		}
> +		vimc->ent_devs[i] = ved;
> +	}
> +	return 0;
> +}
> +
> +static void vimc_rm_subdevs(struct vimc_device *vimc)
> +{
> +	unsigned int i;
>  
> -	dev_dbg(master, "bind");
> +	for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
> +		if (vimc->ent_devs[i])
> +			vimc->pipe_cfg->ents[i].rm(vimc, vimc->ent_devs[i]);
> +}
> +
> +static int vimc_register_devices(struct vimc_device *vimc)
> +{
> +	int ret;
>  
>  	/* Register the v4l2 struct */
>  	ret = v4l2_device_register(vimc->mdev.dev, &vimc->v4l2_dev);
> @@ -179,22 +198,30 @@ static int vimc_comp_bind(struct device *master)
>  		return ret;
>  	}
>  
> -	/* Bind subdevices */
> -	ret = component_bind_all(master, &vimc->v4l2_dev);
> -	if (ret)
> +	/* allocate ent_devs */
> +	vimc->ent_devs = kmalloc_array(vimc->pipe_cfg->num_ents,
> +				       sizeof(*vimc->ent_devs),
> +				       GFP_KERNEL);
> +	if (!vimc->ent_devs)
>  		goto err_v4l2_unregister;
>  
> +	/* 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;
> +
>  	/* Initialize links */
>  	ret = vimc_create_links(vimc);
>  	if (ret)
> -		goto err_comp_unbind_all;
> +		goto err_rm_subdevs;
>  
>  	/* Register the media device */
>  	ret = media_device_register(&vimc->mdev);
>  	if (ret) {
>  		dev_err(vimc->mdev.dev,
>  			"media device register failed (err=%d)\n", ret);
> -		goto err_comp_unbind_all;
> +		goto err_rm_subdevs;
>  	}
>  
>  	/* Expose all subdev's nodes*/
> @@ -211,98 +238,32 @@ static int vimc_comp_bind(struct device *master)
>  err_mdev_unregister:
>  	media_device_unregister(&vimc->mdev);
>  	media_device_cleanup(&vimc->mdev);
> -err_comp_unbind_all:
> -	component_unbind_all(master, NULL);
> +err_rm_subdevs:
> +	vimc_rm_subdevs(vimc);
> +	kfree(vimc->ent_devs);
>  err_v4l2_unregister:
>  	v4l2_device_unregister(&vimc->v4l2_dev);
>  
>  	return ret;
>  }
>  
> -static void vimc_comp_unbind(struct device *master)
> +static void vimc_unregister(struct vimc_device *vimc)
>  {
> -	struct vimc_device *vimc = container_of(to_platform_device(master),
> -						struct vimc_device, pdev);
> -
> -	dev_dbg(master, "unbind");
> -
>  	media_device_unregister(&vimc->mdev);
>  	media_device_cleanup(&vimc->mdev);
> -	component_unbind_all(master, NULL);
>  	v4l2_device_unregister(&vimc->v4l2_dev);
> +	kfree(vimc->ent_devs);
>  }
>  
> -static int vimc_comp_compare(struct device *comp, void *data)
> -{
> -	return comp == data;
> -}
> -
> -static struct component_match *vimc_add_subdevs(struct vimc_device *vimc)
> -{
> -	struct component_match *match = NULL;
> -	struct vimc_platform_data pdata;
> -	int i;
> -
> -	for (i = 0; i < vimc->pipe_cfg->num_ents; i++) {
> -		dev_dbg(&vimc->pdev.dev, "new pdev for %s\n",
> -			vimc->pipe_cfg->ents[i].drv);
> -
> -		strscpy(pdata.entity_name, vimc->pipe_cfg->ents[i].name,
> -			sizeof(pdata.entity_name));
> -
> -		vimc->subdevs[i] = platform_device_register_data(&vimc->pdev.dev,
> -						vimc->pipe_cfg->ents[i].drv,
> -						PLATFORM_DEVID_AUTO,
> -						&pdata,
> -						sizeof(pdata));
> -		if (IS_ERR(vimc->subdevs[i])) {
> -			match = ERR_CAST(vimc->subdevs[i]);
> -			while (--i >= 0)
> -				platform_device_unregister(vimc->subdevs[i]);
> -
> -			return match;
> -		}
> -
> -		component_match_add(&vimc->pdev.dev, &match, vimc_comp_compare,
> -				    &vimc->subdevs[i]->dev);
> -	}
> -
> -	return match;
> -}
> -
> -static void vimc_rm_subdevs(struct vimc_device *vimc)
> -{
> -	unsigned int i;
> -
> -	for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
> -		platform_device_unregister(vimc->subdevs[i]);
> -}
> -
> -static const struct component_master_ops vimc_comp_ops = {
> -	.bind = vimc_comp_bind,
> -	.unbind = vimc_comp_unbind,
> -};
> -
>  static int vimc_probe(struct platform_device *pdev)
>  {
>  	struct vimc_device *vimc = container_of(pdev, struct vimc_device, pdev);
> -	struct component_match *match = NULL;
>  	int ret;
>  
>  	dev_dbg(&pdev->dev, "probe");
>  
>  	memset(&vimc->mdev, 0, sizeof(vimc->mdev));
>  
> -	/* Create platform_device for each entity in the topology*/
> -	vimc->subdevs = devm_kcalloc(&vimc->pdev.dev, vimc->pipe_cfg->num_ents,
> -				     sizeof(*vimc->subdevs), GFP_KERNEL);
> -	if (!vimc->subdevs)
> -		return -ENOMEM;
> -
> -	match = vimc_add_subdevs(vimc);
> -	if (IS_ERR(match))
> -		return PTR_ERR(match);
> -
>  	/* Link the media device within the v4l2_device */
>  	vimc->v4l2_dev.mdev = &vimc->mdev;
>  
> @@ -314,12 +275,9 @@ static int vimc_probe(struct platform_device *pdev)
>  	vimc->mdev.dev = &pdev->dev;
>  	media_device_init(&vimc->mdev);
>  
> -	/* Add self to the component system */
> -	ret = component_master_add_with_match(&pdev->dev, &vimc_comp_ops,
> -					      match);
> +	ret = vimc_register_devices(vimc);
>  	if (ret) {
>  		media_device_cleanup(&vimc->mdev);
> -		vimc_rm_subdevs(vimc);
>  		return ret;
>  	}
>  
> @@ -332,8 +290,8 @@ static int vimc_remove(struct platform_device *pdev)
>  
>  	dev_dbg(&pdev->dev, "remove");
>  
> -	component_master_del(&pdev->dev, &vimc_comp_ops);
>  	vimc_rm_subdevs(vimc);
> +	vimc_unregister(vimc);
>  
>  	return 0;
>  }
> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
> index b72b8385067b..00850f2501ad 100644
> --- a/drivers/media/platform/vimc/vimc-debayer.c
> +++ b/drivers/media/platform/vimc/vimc-debayer.c
> @@ -15,8 +15,6 @@
>  

Could you also remove the following headers?

#include·<linux/component.h> 
#include·<linux/module.h> 
#include·<linux/mod_devicetable.h> 
#include·<linux/platform_device.h> 

>  #include "vimc-common.h"
>  
> -#define VIMC_DEB_DRV_NAME "vimc-debayer"
> -
>  static unsigned int deb_mean_win_size = 3;
>  module_param(deb_mean_win_size, uint, 0000);
>  MODULE_PARM_DESC(deb_mean_win_size, " the window size to calculate the mean.\n"
> @@ -491,44 +489,40 @@ static const struct v4l2_subdev_internal_ops vimc_deb_int_ops = {
>  	.release = vimc_deb_release,
>  };
>  
> -static void vimc_deb_comp_unbind(struct device *comp, struct device *master,
> -				 void *master_data)
> +void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
>  {
> -	struct vimc_ent_device *ved = dev_get_drvdata(comp);
> -	struct vimc_deb_device *vdeb = container_of(ved, struct vimc_deb_device,
> -						    ved);
> +	struct vimc_deb_device *vdeb;
>  
> +	vdeb = container_of(ved, struct vimc_deb_device, ved);
>  	vimc_ent_sd_unregister(ved, &vdeb->sd);
>  }
>  
> -static int vimc_deb_comp_bind(struct device *comp, struct device *master,
> -			      void *master_data)
> +struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
> +				     const char *vcfg_name)
>  {
> -	struct v4l2_device *v4l2_dev = master_data;
> -	struct vimc_platform_data *pdata = comp->platform_data;
> +	struct v4l2_device *v4l2_dev = &vimc->v4l2_dev;
>  	struct vimc_deb_device *vdeb;
>  	int ret;
>  
>  	/* Allocate the vdeb struct */
>  	vdeb = kzalloc(sizeof(*vdeb), GFP_KERNEL);
>  	if (!vdeb)
> -		return -ENOMEM;
> +		return NULL;
>  
>  	/* Initialize ved and sd */
>  	ret = vimc_ent_sd_register(&vdeb->ved, &vdeb->sd, v4l2_dev,
> -				   pdata->entity_name,
> +				   vcfg_name,
>  				   MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV, 2,
>  				   (const unsigned long[2]) {MEDIA_PAD_FL_SINK,
>  				   MEDIA_PAD_FL_SOURCE},
>  				   &vimc_deb_int_ops, &vimc_deb_ops);
>  	if (ret) {
>  		kfree(vdeb);
> -		return ret;
> +		return NULL;
>  	}
>  
>  	vdeb->ved.process_frame = vimc_deb_process_frame;
> -	dev_set_drvdata(comp, &vdeb->ved);
> -	vdeb->dev = comp;
> +	vdeb->dev = &vimc->pdev.dev;
>  
>  	/* Initialize the frame format */
>  	vdeb->sink_fmt = sink_fmt_default;
> @@ -541,46 +535,5 @@ static int vimc_deb_comp_bind(struct device *comp, struct device *master,
>  	vdeb->src_code = MEDIA_BUS_FMT_RGB888_1X24;
>  	vdeb->set_rgb_src = vimc_deb_set_rgb_mbus_fmt_rgb888_1x24;
>  
> -	return 0;
> +	return &vdeb->ved;
>  }
> -
> -static const struct component_ops vimc_deb_comp_ops = {
> -	.bind = vimc_deb_comp_bind,
> -	.unbind = vimc_deb_comp_unbind,
> -};
> -
> -static int vimc_deb_probe(struct platform_device *pdev)
> -{
> -	return component_add(&pdev->dev, &vimc_deb_comp_ops);
> -}
> -
> -static int vimc_deb_remove(struct platform_device *pdev)
> -{
> -	component_del(&pdev->dev, &vimc_deb_comp_ops);
> -
> -	return 0;
> -}
> -
> -static const struct platform_device_id vimc_deb_driver_ids[] = {
> -	{
> -		.name           = VIMC_DEB_DRV_NAME,
> -	},
> -	{ }
> -};
> -
> -static struct platform_driver vimc_deb_pdrv = {
> -	.probe		= vimc_deb_probe,
> -	.remove		= vimc_deb_remove,
> -	.id_table	= vimc_deb_driver_ids,
> -	.driver		= {
> -		.name	= VIMC_DEB_DRV_NAME,
> -	},
> -};
> -
> -module_platform_driver(vimc_deb_pdrv);
> -
> -MODULE_DEVICE_TABLE(platform, vimc_deb_driver_ids);
> -
> -MODULE_DESCRIPTION("Virtual Media Controller Driver (VIMC) Debayer");
> -MODULE_AUTHOR("Helen Mae Koike Fornazier <helen.fornazier@gmail.com>");
> -MODULE_LICENSE("GPL");
> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> index 49ab8d9dd9c9..28a947950a0b 100644
> --- a/drivers/media/platform/vimc/vimc-scaler.c
> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> @@ -5,18 +5,13 @@
>   * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com>
>   */
>  
> -#include <linux/component.h>
>  #include <linux/module.h>

You can remove this include as well.

> -#include <linux/mod_devicetable.h>
> -#include <linux/platform_device.h>
>  #include <linux/vmalloc.h>
>  #include <linux/v4l2-mediabus.h>
>  #include <media/v4l2-subdev.h>
>  
>  #include "vimc-common.h"
>  
> -#define VIMC_SCA_DRV_NAME "vimc-scaler"
> -
>  static unsigned int sca_mult = 3;
>  module_param(sca_mult, uint, 0000);
>  MODULE_PARM_DESC(sca_mult, " the image size multiplier");
> @@ -350,89 +345,43 @@ static const struct v4l2_subdev_internal_ops vimc_sca_int_ops = {
>  	.release = vimc_sca_release,
>  };
>  
> -static void vimc_sca_comp_unbind(struct device *comp, struct device *master,
> -				 void *master_data)
> +void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
>  {
> -	struct vimc_ent_device *ved = dev_get_drvdata(comp);
> -	struct vimc_sca_device *vsca = container_of(ved, struct vimc_sca_device,
> -						    ved);
> +	struct vimc_sca_device *vsca;
>  
> +	vsca = container_of(ved, struct vimc_sca_device, ved);
>  	vimc_ent_sd_unregister(ved, &vsca->sd);
>  }
>  
> -
> -static int vimc_sca_comp_bind(struct device *comp, struct device *master,
> -			      void *master_data)
> +struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
> +				     const char *vcfg_name)
>  {
> -	struct v4l2_device *v4l2_dev = master_data;
> -	struct vimc_platform_data *pdata = comp->platform_data;
> +	struct v4l2_device *v4l2_dev = &vimc->v4l2_dev;
>  	struct vimc_sca_device *vsca;
>  	int ret;
>  
>  	/* Allocate the vsca struct */
>  	vsca = kzalloc(sizeof(*vsca), GFP_KERNEL);
>  	if (!vsca)
> -		return -ENOMEM;
> +		return NULL;
>  
>  	/* Initialize ved and sd */
>  	ret = vimc_ent_sd_register(&vsca->ved, &vsca->sd, v4l2_dev,
> -				   pdata->entity_name,
> +				   vcfg_name,
>  				   MEDIA_ENT_F_PROC_VIDEO_SCALER, 2,
>  				   (const unsigned long[2]) {MEDIA_PAD_FL_SINK,
>  				   MEDIA_PAD_FL_SOURCE},
>  				   &vimc_sca_int_ops, &vimc_sca_ops);
>  	if (ret) {
>  		kfree(vsca);
> -		return ret;
> +		return NULL;
>  	}
>  
>  	vsca->ved.process_frame = vimc_sca_process_frame;
> -	dev_set_drvdata(comp, &vsca->ved);
> -	vsca->dev = comp;
> +	vsca->dev = &vimc->pdev.dev;
>  
>  	/* Initialize the frame format */
>  	vsca->sink_fmt = sink_fmt_default;
>  
> -	return 0;
> -}
> -
> -static const struct component_ops vimc_sca_comp_ops = {
> -	.bind = vimc_sca_comp_bind,
> -	.unbind = vimc_sca_comp_unbind,
> -};
> -
> -static int vimc_sca_probe(struct platform_device *pdev)
> -{
> -	return component_add(&pdev->dev, &vimc_sca_comp_ops);
> +	return &vsca->ved;
>  }
> -
> -static int vimc_sca_remove(struct platform_device *pdev)
> -{
> -	component_del(&pdev->dev, &vimc_sca_comp_ops);
> -
> -	return 0;
> -}
> -
> -static const struct platform_device_id vimc_sca_driver_ids[] = {
> -	{
> -		.name           = VIMC_SCA_DRV_NAME,
> -	},
> -	{ }
> -};
> -
> -static struct platform_driver vimc_sca_pdrv = {
> -	.probe		= vimc_sca_probe,
> -	.remove		= vimc_sca_remove,
> -	.id_table	= vimc_sca_driver_ids,
> -	.driver		= {
> -		.name	= VIMC_SCA_DRV_NAME,
> -	},
> -};
> -
> -module_platform_driver(vimc_sca_pdrv);
> -
> -MODULE_DEVICE_TABLE(platform, vimc_sca_driver_ids);
> -
> -MODULE_DESCRIPTION("Virtual Media Controller Driver (VIMC) Scaler");
> -MODULE_AUTHOR("Helen Mae Koike Fornazier <helen.fornazier@gmail.com>");
> -MODULE_LICENSE("GPL");
> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
> index 6c53b9fc1617..1f15637ca8bb 100644
> --- a/drivers/media/platform/vimc/vimc-sensor.c
> +++ b/drivers/media/platform/vimc/vimc-sensor.c
> @@ -5,10 +5,6 @@
>   * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com>
>   */
>  
> -#include <linux/component.h>
> -#include <linux/module.h>
> -#include <linux/mod_devicetable.h>
> -#include <linux/platform_device.h>
>  #include <linux/v4l2-mediabus.h>
>  #include <linux/vmalloc.h>
>  #include <media/v4l2-ctrls.h>
> @@ -18,8 +14,6 @@
>  
>  #include "vimc-common.h"
>  
> -#define VIMC_SEN_DRV_NAME "vimc-sensor"
> -
>  struct vimc_sen_device {
>  	struct vimc_ent_device ved;
>  	struct v4l2_subdev sd;
> @@ -304,13 +298,11 @@ static const struct v4l2_subdev_internal_ops vimc_sen_int_ops = {
>  	.release = vimc_sen_release,
>  };
>  
> -static void vimc_sen_comp_unbind(struct device *comp, struct device *master,
> -				 void *master_data)
> +void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
>  {
> -	struct vimc_ent_device *ved = dev_get_drvdata(comp);
> -	struct vimc_sen_device *vsen =
> -				container_of(ved, struct vimc_sen_device, ved);
> +	struct vimc_sen_device *vsen;
>  
> +	vsen = container_of(ved, struct vimc_sen_device, ved);
>  	vimc_ent_sd_unregister(ved, &vsen->sd);
>  }
>  
> @@ -331,18 +323,17 @@ static const struct v4l2_ctrl_config vimc_sen_ctrl_test_pattern = {
>  	.qmenu = tpg_pattern_strings,
>  };
>  
> -static int vimc_sen_comp_bind(struct device *comp, struct device *master,
> -			      void *master_data)
> +struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
> +				     const char *vcfg_name)
>  {
> -	struct v4l2_device *v4l2_dev = master_data;
> -	struct vimc_platform_data *pdata = comp->platform_data;
> +	struct v4l2_device *v4l2_dev = &vimc->v4l2_dev;
>  	struct vimc_sen_device *vsen;
>  	int ret;
>  
>  	/* Allocate the vsen struct */
>  	vsen = kzalloc(sizeof(*vsen), GFP_KERNEL);
>  	if (!vsen)
> -		return -ENOMEM;
> +		return NULL;
>  
>  	v4l2_ctrl_handler_init(&vsen->hdl, 4);
>  
> @@ -368,7 +359,7 @@ static int vimc_sen_comp_bind(struct device *comp, struct device *master,
>  
>  	/* Initialize ved and sd */
>  	ret = vimc_ent_sd_register(&vsen->ved, &vsen->sd, v4l2_dev,
> -				   pdata->entity_name,
> +				   vcfg_name,
>  				   MEDIA_ENT_F_CAM_SENSOR, 1,
>  				   (const unsigned long[1]) {MEDIA_PAD_FL_SOURCE},
>  				   &vimc_sen_int_ops, &vimc_sen_ops);
> @@ -376,8 +367,7 @@ static int vimc_sen_comp_bind(struct device *comp, struct device *master,
>  		goto err_free_hdl;
>  
>  	vsen->ved.process_frame = vimc_sen_process_frame;
> -	dev_set_drvdata(comp, &vsen->ved);
> -	vsen->dev = comp;
> +	vsen->dev = &vimc->pdev.dev;
>  
>  	/* Initialize the frame format */
>  	vsen->mbus_format = fmt_default;
> @@ -389,7 +379,7 @@ static int vimc_sen_comp_bind(struct device *comp, struct device *master,
>  	if (ret)
>  		goto err_unregister_ent_sd;
>  
> -	return 0;
> +	return &vsen->ved;
>  
>  err_unregister_ent_sd:
>  	vimc_ent_sd_unregister(&vsen->ved,  &vsen->sd);
> @@ -398,46 +388,5 @@ static int vimc_sen_comp_bind(struct device *comp, struct device *master,
>  err_free_vsen:
>  	kfree(vsen);
>  
> -	return ret;
> +	return NULL;
>  }
> -
> -static const struct component_ops vimc_sen_comp_ops = {
> -	.bind = vimc_sen_comp_bind,
> -	.unbind = vimc_sen_comp_unbind,
> -};
> -
> -static int vimc_sen_probe(struct platform_device *pdev)
> -{
> -	return component_add(&pdev->dev, &vimc_sen_comp_ops);
> -}
> -
> -static int vimc_sen_remove(struct platform_device *pdev)
> -{
> -	component_del(&pdev->dev, &vimc_sen_comp_ops);
> -
> -	return 0;
> -}
> -
> -static const struct platform_device_id vimc_sen_driver_ids[] = {
> -	{
> -		.name           = VIMC_SEN_DRV_NAME,
> -	},
> -	{ }
> -};
> -
> -static struct platform_driver vimc_sen_pdrv = {
> -	.probe		= vimc_sen_probe,
> -	.remove		= vimc_sen_remove,
> -	.id_table	= vimc_sen_driver_ids,
> -	.driver		= {
> -		.name	= VIMC_SEN_DRV_NAME,
> -	},
> -};
> -
> -module_platform_driver(vimc_sen_pdrv);
> -
> -MODULE_DEVICE_TABLE(platform, vimc_sen_driver_ids);
> -
> -MODULE_DESCRIPTION("Virtual Media Controller Driver (VIMC) Sensor");
> -MODULE_AUTHOR("Helen Mae Koike Fornazier <helen.fornazier@gmail.com>");
> -MODULE_LICENSE("GPL");
> 

With these changes:

Acked-by: Helen Koike <helen.koike@collabora.com>

Re: [PATCH v4 1/5] media: vimc: Collapse component structure into a single monolithic driver

[Shuah Khan]

On 9/15/19 1:25 PM, Helen Koike wrote:
> Hi Shuah,
> 
> Thanks for the patch.
> 
> On 9/6/19 11:42 PM, Shuah Khan wrote:
>> vimc uses Component API to split the driver into functional components.
>> The real hardware resembles a monolith structure than component and
>> component structure added a level of complexity making it hard to
>> maintain without adding any real benefit.
>>
>> The sensor is one vimc component that would makes sense to be a separate
>> module to closely align with the real hardware. It would be easier to
>> collapse vimc into single monolithic driver first and then split the
>> sensor off as a separate module.
>>
>> Collapse it into a single monolithic driver removing the Component API.
>> This patch removes the component API and makes minimal changes to the
>> code base preserving the functional division of the code structure.
>> Preserving the functional structure allows us to split the sensor off
>> as a separate module in the future.
>>
>> Major design elements in this change are:
>> - Use existing struct vimc_ent_config and struct vimc_pipeline_config
>>    to drive the initialization of the functional components.
>> - Make vimc_device and vimc_ent_config global by moving them to
>>    vimc-common.h
>> - Add two new hooks add and rm to initialize and register, unregister
>>    and free subdevs.
>> - All component API is now gone and bind and unbind hooks are modified
>>    to do "add" and "rm" with minimal changes to just add and rm subdevs.
>> - vimc-core's bind and unbind are now register and unregister.
>> - Add a new field to vimc_device structure for saving the pointers to
>>    struct vimc_ent_device(s) subdevs create in their "add" hooks. These
>>    get used to create links and removing the subdevs. vimc-core allocates
>>    this array which sized to number of entries in the topology defined in
>>    the vimc_pipeline_config structure.
>> - vimc-core invokes "add" hooks from its vimc_register_devices().
>>    The "add" hooks remain the same and register subdevs. They don't
>>    create platform devices of their own and use vimc's pdev.dev as
>>    their reference device. Each "add" hook returns pointer to its struct
>>    vimc_ent_device. This is saved in the vimc_device ent_devs array.
>> - vimc-core invokes "rm" hooks from its unregister to unregister subdevs
>>    and cleanup.
>> - vimc-core invokes "add" and "rm" hooks with pointer to struct vimc_device
>>    and the corresponding vimc_ent_device saved in the ent_devs.
>>
>> The following configure and stream test works on all devices.
>>
>> media-ctl -d platform:vimc -V '"Sensor A":0[fmt:SBGGR8_1X8/640x480]'
>> media-ctl -d platform:vimc -V '"Debayer A":0[fmt:SBGGR8_1X8/640x480]'
>> media-ctl -d platform:vimc -V '"Sensor B":0[fmt:SBGGR8_1X8/640x480]'
>> media-ctl -d platform:vimc -V '"Debayer B":0[fmt:SBGGR8_1X8/640x480]'
>>
>> v4l2-ctl -z platform:vimc -d "RGB/YUV Capture" -v width=1920,height=1440
>> v4l2-ctl -z platform:vimc -d "Raw Capture 0" -v pixelformat=BA81
>> v4l2-ctl -z platform:vimc -d "Raw Capture 1" -v pixelformat=BA81
>>
>> v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video1
>> v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video2
>> v4l2-ctl --stream-mmap --stream-count=100 -d /dev/video3
>>
>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>> ---
>>   drivers/media/platform/vimc/Makefile       |   7 +-
>>   drivers/media/platform/vimc/vimc-capture.c |  81 ++------
>>   drivers/media/platform/vimc/vimc-common.h  |  54 ++++++
>>   drivers/media/platform/vimc/vimc-core.c    | 216 +++++++++------------
>>   drivers/media/platform/vimc/vimc-debayer.c |  69 ++-----
>>   drivers/media/platform/vimc/vimc-scaler.c  |  73 ++-----
>>   drivers/media/platform/vimc/vimc-sensor.c  |  73 ++-----
>>   7 files changed, 193 insertions(+), 380 deletions(-)
>>
>> diff --git a/drivers/media/platform/vimc/Makefile b/drivers/media/platform/vimc/Makefile
>> index 96d06f030c31..a53b2b532e9f 100644
>> --- a/drivers/media/platform/vimc/Makefile
>> +++ b/drivers/media/platform/vimc/Makefile
>> @@ -1,5 +1,6 @@
>>   # SPDX-License-Identifier: GPL-2.0
>> -vimc-y := vimc-core.o vimc-common.o vimc-streamer.o
>> +vimc-y := vimc-core.o vimc-common.o vimc-streamer.o vimc-capture.o \
>> +		vimc-debayer.o vimc-scaler.o vimc-sensor.o
>> +
>> +obj-$(CONFIG_VIDEO_VIMC) += vimc.o
>>   
>> -obj-$(CONFIG_VIDEO_VIMC) += vimc.o vimc-capture.o vimc-debayer.o \
>> -                vimc-scaler.o vimc-sensor.o
>> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
>> index 1d56b91830ba..602f80323031 100644
>> --- a/drivers/media/platform/vimc/vimc-capture.c
>> +++ b/drivers/media/platform/vimc/vimc-capture.c
>> @@ -5,10 +5,6 @@
>>    * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com>
>>    */
>>   
>> -#include <linux/component.h>
>> -#include <linux/module.h>
>> -#include <linux/mod_devicetable.h>
>> -#include <linux/platform_device.h>
>>   #include <media/v4l2-ioctl.h>
>>   #include <media/videobuf2-core.h>
>>   #include <media/videobuf2-vmalloc.h>
>> @@ -16,8 +12,6 @@
>>   #include "vimc-common.h"
>>   #include "vimc-streamer.h"
>>   
>> -#define VIMC_CAP_DRV_NAME "vimc-capture"
>> -
>>   struct vimc_cap_device {
>>   	struct vimc_ent_device ved;
>>   	struct video_device vdev;
>> @@ -340,13 +334,11 @@ static void vimc_cap_release(struct video_device *vdev)
>>   	kfree(vcap);
>>   }
>>   
>> -static void vimc_cap_comp_unbind(struct device *comp, struct device *master,
>> -				 void *master_data)
>> +void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
>>   {
>> -	struct vimc_ent_device *ved = dev_get_drvdata(comp);
>> -	struct vimc_cap_device *vcap = container_of(ved, struct vimc_cap_device,
>> -						    ved);
>> +	struct vimc_cap_device *vcap;
>>   
>> +	vcap = container_of(ved, struct vimc_cap_device, ved);
>>   	vb2_queue_release(&vcap->queue);
>>   	media_entity_cleanup(ved->ent);
>>   	video_unregister_device(&vcap->vdev);
>> @@ -391,11 +383,10 @@ static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
>>   	return NULL;
>>   }
>>   
>> -static int vimc_cap_comp_bind(struct device *comp, struct device *master,
>> -			      void *master_data)
>> +struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
>> +				     const char *vcfg_name)
>>   {
>> -	struct v4l2_device *v4l2_dev = master_data;
>> -	struct vimc_platform_data *pdata = comp->platform_data;
>> +	struct v4l2_device *v4l2_dev = &vimc->v4l2_dev;
>>   	const struct vimc_pix_map *vpix;
>>   	struct vimc_cap_device *vcap;
>>   	struct video_device *vdev;
>> @@ -405,7 +396,7 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
>>   	/* Allocate the vimc_cap_device struct */
>>   	vcap = kzalloc(sizeof(*vcap), GFP_KERNEL);
>>   	if (!vcap)
>> -		return -ENOMEM;
>> +		return NULL;
>>   
>>   	/* Allocate the pads */
>>   	vcap->ved.pads =
>> @@ -416,7 +407,7 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
>>   	}
>>   
>>   	/* Initialize the media entity */
>> -	vcap->vdev.entity.name = pdata->entity_name;
>> +	vcap->vdev.entity.name = vcfg_name;
>>   	vcap->vdev.entity.function = MEDIA_ENT_F_IO_V4L;
>>   	ret = media_entity_pads_init(&vcap->vdev.entity,
>>   				     1, vcap->ved.pads);
>> @@ -440,8 +431,8 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
>>   
>>   	ret = vb2_queue_init(q);
>>   	if (ret) {
>> -		dev_err(comp, "%s: vb2 queue init failed (err=%d)\n",
>> -			pdata->entity_name, ret);
>> +		dev_err(&vimc->pdev.dev, "%s: vb2 queue init failed (err=%d)\n",
>> +			vcfg_name, ret);
>>   		goto err_clean_m_ent;
>>   	}
>>   
>> @@ -460,8 +451,7 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
>>   	vcap->ved.ent = &vcap->vdev.entity;
>>   	vcap->ved.process_frame = vimc_cap_process_frame;
>>   	vcap->ved.vdev_get_format = vimc_cap_get_format;
>> -	dev_set_drvdata(comp, &vcap->ved);
>> -	vcap->dev = comp;
>> +	vcap->dev = &vimc->pdev.dev;
>>   
>>   	/* Initialize the video_device struct */
>>   	vdev = &vcap->vdev;
>> @@ -474,18 +464,18 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
>>   	vdev->queue = q;
>>   	vdev->v4l2_dev = v4l2_dev;
>>   	vdev->vfl_dir = VFL_DIR_RX;
>> -	strscpy(vdev->name, pdata->entity_name, sizeof(vdev->name));
>> +	strscpy(vdev->name, vcfg_name, sizeof(vdev->name));
>>   	video_set_drvdata(vdev, &vcap->ved);
>>   
>>   	/* Register the video_device with the v4l2 and the media framework */
>>   	ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
>>   	if (ret) {
>> -		dev_err(comp, "%s: video register failed (err=%d)\n",
>> +		dev_err(&vimc->pdev.dev, "%s: video register failed (err=%d)\n",
>>   			vcap->vdev.name, ret);
>>   		goto err_release_queue;
>>   	}
>>   
>> -	return 0;
>> +	return &vcap->ved;
>>   
>>   err_release_queue:
>>   	vb2_queue_release(q);
>> @@ -496,46 +486,5 @@ static int vimc_cap_comp_bind(struct device *comp, struct device *master,
>>   err_free_vcap:
>>   	kfree(vcap);
>>   
>> -	return ret;
>> -}
>> -
>> -static const struct component_ops vimc_cap_comp_ops = {
>> -	.bind = vimc_cap_comp_bind,
>> -	.unbind = vimc_cap_comp_unbind,
>> -};
>> -
>> -static int vimc_cap_probe(struct platform_device *pdev)
>> -{
>> -	return component_add(&pdev->dev, &vimc_cap_comp_ops);
>> -}
>> -
>> -static int vimc_cap_remove(struct platform_device *pdev)
>> -{
>> -	component_del(&pdev->dev, &vimc_cap_comp_ops);
>> -
>> -	return 0;
>> +	return NULL;
>>   }
>> -
>> -static const struct platform_device_id vimc_cap_driver_ids[] = {
>> -	{
>> -		.name           = VIMC_CAP_DRV_NAME,
>> -	},
>> -	{ }
>> -};
>> -
>> -static struct platform_driver vimc_cap_pdrv = {
>> -	.probe		= vimc_cap_probe,
>> -	.remove		= vimc_cap_remove,
>> -	.id_table	= vimc_cap_driver_ids,
>> -	.driver		= {
>> -		.name	= VIMC_CAP_DRV_NAME,
>> -	},
>> -};
>> -
>> -module_platform_driver(vimc_cap_pdrv);
>> -
>> -MODULE_DEVICE_TABLE(platform, vimc_cap_driver_ids);
>> -
>> -MODULE_DESCRIPTION("Virtual Media Controller Driver (VIMC) Capture");
>> -MODULE_AUTHOR("Helen Mae Koike Fornazier <helen.fornazier@gmail.com>");
>> -MODULE_LICENSE("GPL");
>> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
>> index 9c2e0e216c6b..87ee84f78322 100644
>> --- a/drivers/media/platform/vimc/vimc-common.h
>> +++ b/drivers/media/platform/vimc/vimc-common.h
>> @@ -9,6 +9,7 @@
>>   #define _VIMC_COMMON_H_
>>   
>>   #include <linux/slab.h>
>> +#include <linux/platform_device.h>
> 
> This should go before linux/slab.h to keep in alphabetical order.

Will do.

> 
>>   #include <media/media-device.h>
>>   #include <media/v4l2-device.h>
>>   
>> @@ -111,6 +112,59 @@ struct vimc_ent_device {
>>   			      struct v4l2_pix_format *fmt);
>>   };
>>   
>> +/**
>> + * struct vimc_device - main device for vimc driver
>> + *
>> + * @pdev	pointer to the platform device
>> + * @pipe_cfg	pointer to the vimc pipeline configuration structure
>> + * @ent_devs	arry of vimc_ent_device pointers
> 
> s/arry/array
> 
>> + * @mdev	the associated media_device parent
>> + * @v4l2_dev	Internal v4l2 parent device
>> + */
>> +struct vimc_device {
>> +	struct platform_device pdev;
>> +	const struct vimc_pipeline_config *pipe_cfg;
>> +	struct vimc_ent_device **ent_devs;
>> +	struct media_device mdev;
>> +	struct v4l2_device v4l2_dev;
>> +};
>> +
>> +/**
>> + * struct vimc_ent_config	Structure which describes individual
>> + *				configuration for each entity
>> + *
>> + * @name			entity name
>> + * @ved				pointer to vimc_ent_device (a node in the
>> + *					topology)
>> + * @add				subdev add hook - initializes and registers
>> + *					subdev called from vimc-core
>> + * @rm				subdev rm hook - unregisters and frees
>> + *					subdev called from vimc-core
>> + */
>> +struct vimc_ent_config {
>> +	const char *name;
>> +	struct vimc_ent_device *(*add)(struct vimc_device *vimc,
>> +				       const char *vcfg_name);
>> +	void (*rm)(struct vimc_device *vimc, struct vimc_ent_device *ved);
>> +};
>> +
>> +/* prototypes for vimc_ent_config add and rm hooks */
>> +struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
>> +				     const char *vcfg_name);
>> +void vimc_cap_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
>> +
>> +struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
>> +				     const char *vcfg_name);
>> +void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
>> +
>> +struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
>> +				     const char *vcfg_name);
>> +void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
>> +
>> +struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
>> +				     const char *vcfg_name);
>> +void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved);
>> +
>>   /**
>>    * vimc_pads_init - initialize pads
>>    *
>> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
>> index 571c55aa0e16..a1218578cb9a 100644
>> --- a/drivers/media/platform/vimc/vimc-core.c
>> +++ b/drivers/media/platform/vimc/vimc-core.c
>> @@ -5,7 +5,6 @@
>>    * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com>
>>    */
>>   
>> -#include <linux/component.h>
>>   #include <linux/init.h>
>>   #include <linux/module.h>
>>   #include <linux/platform_device.h>
>> @@ -24,29 +23,6 @@
>>   	.flags = link_flags,					\
>>   }
>>   
>> -struct vimc_device {
>> -	/* The platform device */
>> -	struct platform_device pdev;
>> -
>> -	/* The pipeline configuration */
>> -	const struct vimc_pipeline_config *pipe_cfg;
>> -
>> -	/* The Associated media_device parent */
>> -	struct media_device mdev;
>> -
>> -	/* Internal v4l2 parent device*/
>> -	struct v4l2_device v4l2_dev;
>> -
>> -	/* Subdevices */
>> -	struct platform_device **subdevs;
>> -};
>> -
>> -/* Structure which describes individual configuration for each entity */
>> -struct vimc_ent_config {
>> -	const char *name;
>> -	const char *drv;
>> -};
>> -
>>   /* Structure which describes links between entities */
>>   struct vimc_ent_link {
>>   	unsigned int src_ent;
>> @@ -68,43 +44,52 @@ struct vimc_pipeline_config {
>>    * Topology Configuration
>>    */
>>   
>> -static const struct vimc_ent_config ent_config[] = {
>> +static struct vimc_ent_config ent_config[] = {
>>   	{
>>   		.name = "Sensor A",
>> -		.drv = "vimc-sensor",
>> +		.add = vimc_sen_add,
>> +		.rm = vimc_sen_rm,
>>   	},
>>   	{
>>   		.name = "Sensor B",
>> -		.drv = "vimc-sensor",
>> +		.add = vimc_sen_add,
>> +		.rm = vimc_sen_rm,
>>   	},
>>   	{
>>   		.name = "Debayer A",
>> -		.drv = "vimc-debayer",
>> +		.add = vimc_deb_add,
>> +		.rm = vimc_deb_rm,
>>   	},
>>   	{
>>   		.name = "Debayer B",
>> -		.drv = "vimc-debayer",
>> +		.add = vimc_deb_add,
>> +		.rm = vimc_deb_rm,
>>   	},
>>   	{
>>   		.name = "Raw Capture 0",
>> -		.drv = "vimc-capture",
>> +		.add = vimc_cap_add,
>> +		.rm = vimc_cap_rm,
>>   	},
>>   	{
>>   		.name = "Raw Capture 1",
>> -		.drv = "vimc-capture",
>> +		.add = vimc_cap_add,
>> +		.rm = vimc_cap_rm,
>>   	},
>>   	{
>> -		.name = "RGB/YUV Input",
>>   		/* TODO: change this to vimc-input when it is implemented */
>> -		.drv = "vimc-sensor",
>> +		.name = "RGB/YUV Input",
>> +		.add = vimc_sen_add,
>> +		.rm = vimc_sen_rm,
>>   	},
>>   	{
>>   		.name = "Scaler",
>> -		.drv = "vimc-scaler",
>> +		.add = vimc_sca_add,
>> +		.rm = vimc_sca_rm,
>>   	},
>>   	{
>>   		.name = "RGB/YUV Capture",
>> -		.drv = "vimc-capture",
>> +		.add = vimc_cap_add,
>> +		.rm = vimc_cap_rm,
>>   	},
>>   };
>>   
>> @@ -127,7 +112,7 @@ static const struct vimc_ent_link ent_links[] = {
>>   	VIMC_ENT_LINK(7, 1, 8, 0, MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE),
>>   };
>>   
>> -static const struct vimc_pipeline_config pipe_cfg = {
>> +static struct vimc_pipeline_config pipe_cfg = {
>>   	.ents		= ent_config,
>>   	.num_ents	= ARRAY_SIZE(ent_config),
>>   	.links		= ent_links,
>> @@ -136,6 +121,14 @@ static const struct vimc_pipeline_config pipe_cfg = {
>>   
>>   /* -------------------------------------------------------------------------- */
>>   
>> +static void vimc_rm_links(struct vimc_device *vimc)
>> +{
>> +	unsigned int i;
>> +
>> +	for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
>> +		media_entity_remove_links(vimc->ent_devs[i]->ent);
>> +}
>> +
>>   static int vimc_create_links(struct vimc_device *vimc)
>>   {
>>   	unsigned int i;
>> @@ -144,32 +137,58 @@ static int vimc_create_links(struct vimc_device *vimc)
>>   	/* Initialize the links between entities */
>>   	for (i = 0; i < vimc->pipe_cfg->num_links; i++) {
>>   		const struct vimc_ent_link *link = &vimc->pipe_cfg->links[i];
>> -		/*
>> -		 * TODO: Check another way of retrieving ved struct without
>> -		 * relying on platform_get_drvdata
>> -		 */
>> +
>>   		struct vimc_ent_device *ved_src =
>> -			platform_get_drvdata(vimc->subdevs[link->src_ent]);
>> +			vimc->ent_devs[link->src_ent];
>>   		struct vimc_ent_device *ved_sink =
>> -			platform_get_drvdata(vimc->subdevs[link->sink_ent]);
>> +			vimc->ent_devs[link->sink_ent];
>>   
>>   		ret = media_create_pad_link(ved_src->ent, link->src_pad,
>>   					    ved_sink->ent, link->sink_pad,
>>   					    link->flags);
>>   		if (ret)
>> -			return ret;
>> +			goto err_rm_links;
>>   	}
>>   
>>   	return 0;
>> +
>> +err_rm_links:
>> +	vimc_rm_links(vimc);
>> +	return ret;
>>   }
>>   
>> -static int vimc_comp_bind(struct device *master)
>> +static int vimc_add_subdevs(struct vimc_device *vimc)
>>   {
>> -	struct vimc_device *vimc = container_of(to_platform_device(master),
>> -						struct vimc_device, pdev);
>> -	int ret;
>> +	unsigned int i;
>> +	struct vimc_ent_device *ved;
>> +
>> +	for (i = 0; i < vimc->pipe_cfg->num_ents; i++) {
>> +		dev_dbg(&vimc->pdev.dev, "new entity for %s\n",
>> +			vimc->pipe_cfg->ents[i].name);
>> +		ved = vimc->pipe_cfg->ents[i].add(vimc,
>> +					vimc->pipe_cfg->ents[i].name);
>> +		if (!ved) {
>> +			dev_err(&vimc->pdev.dev, "add new entity for %s\n",
>> +				vimc->pipe_cfg->ents[i].name);
>> +			return -1;
> 
> return -EINVAL

Will do.

> 
>> +		}
>> +		vimc->ent_devs[i] = ved;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static void vimc_rm_subdevs(struct vimc_device *vimc)
>> +{
>> +	unsigned int i;
>>   
>> -	dev_dbg(master, "bind");
>> +	for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
>> +		if (vimc->ent_devs[i])
>> +			vimc->pipe_cfg->ents[i].rm(vimc, vimc->ent_devs[i]);
>> +}
>> +
>> +static int vimc_register_devices(struct vimc_device *vimc)
>> +{
>> +	int ret;
>>   
>>   	/* Register the v4l2 struct */
>>   	ret = v4l2_device_register(vimc->mdev.dev, &vimc->v4l2_dev);
>> @@ -179,22 +198,30 @@ static int vimc_comp_bind(struct device *master)
>>   		return ret;
>>   	}
>>   
>> -	/* Bind subdevices */
>> -	ret = component_bind_all(master, &vimc->v4l2_dev);
>> -	if (ret)
>> +	/* allocate ent_devs */
>> +	vimc->ent_devs = kmalloc_array(vimc->pipe_cfg->num_ents,
>> +				       sizeof(*vimc->ent_devs),
>> +				       GFP_KERNEL);
>> +	if (!vimc->ent_devs)
>>   		goto err_v4l2_unregister;
>>   
>> +	/* 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;
>> +
>>   	/* Initialize links */
>>   	ret = vimc_create_links(vimc);
>>   	if (ret)
>> -		goto err_comp_unbind_all;
>> +		goto err_rm_subdevs;
>>   
>>   	/* Register the media device */
>>   	ret = media_device_register(&vimc->mdev);
>>   	if (ret) {
>>   		dev_err(vimc->mdev.dev,
>>   			"media device register failed (err=%d)\n", ret);
>> -		goto err_comp_unbind_all;
>> +		goto err_rm_subdevs;
>>   	}
>>   
>>   	/* Expose all subdev's nodes*/
>> @@ -211,98 +238,32 @@ static int vimc_comp_bind(struct device *master)
>>   err_mdev_unregister:
>>   	media_device_unregister(&vimc->mdev);
>>   	media_device_cleanup(&vimc->mdev);
>> -err_comp_unbind_all:
>> -	component_unbind_all(master, NULL);
>> +err_rm_subdevs:
>> +	vimc_rm_subdevs(vimc);
>> +	kfree(vimc->ent_devs);
>>   err_v4l2_unregister:
>>   	v4l2_device_unregister(&vimc->v4l2_dev);
>>   
>>   	return ret;
>>   }
>>   
>> -static void vimc_comp_unbind(struct device *master)
>> +static void vimc_unregister(struct vimc_device *vimc)
>>   {
>> -	struct vimc_device *vimc = container_of(to_platform_device(master),
>> -						struct vimc_device, pdev);
>> -
>> -	dev_dbg(master, "unbind");
>> -
>>   	media_device_unregister(&vimc->mdev);
>>   	media_device_cleanup(&vimc->mdev);
>> -	component_unbind_all(master, NULL);
>>   	v4l2_device_unregister(&vimc->v4l2_dev);
>> +	kfree(vimc->ent_devs);
>>   }
>>   
>> -static int vimc_comp_compare(struct device *comp, void *data)
>> -{
>> -	return comp == data;
>> -}
>> -
>> -static struct component_match *vimc_add_subdevs(struct vimc_device *vimc)
>> -{
>> -	struct component_match *match = NULL;
>> -	struct vimc_platform_data pdata;
>> -	int i;
>> -
>> -	for (i = 0; i < vimc->pipe_cfg->num_ents; i++) {
>> -		dev_dbg(&vimc->pdev.dev, "new pdev for %s\n",
>> -			vimc->pipe_cfg->ents[i].drv);
>> -
>> -		strscpy(pdata.entity_name, vimc->pipe_cfg->ents[i].name,
>> -			sizeof(pdata.entity_name));
>> -
>> -		vimc->subdevs[i] = platform_device_register_data(&vimc->pdev.dev,
>> -						vimc->pipe_cfg->ents[i].drv,
>> -						PLATFORM_DEVID_AUTO,
>> -						&pdata,
>> -						sizeof(pdata));
>> -		if (IS_ERR(vimc->subdevs[i])) {
>> -			match = ERR_CAST(vimc->subdevs[i]);
>> -			while (--i >= 0)
>> -				platform_device_unregister(vimc->subdevs[i]);
>> -
>> -			return match;
>> -		}
>> -
>> -		component_match_add(&vimc->pdev.dev, &match, vimc_comp_compare,
>> -				    &vimc->subdevs[i]->dev);
>> -	}
>> -
>> -	return match;
>> -}
>> -
>> -static void vimc_rm_subdevs(struct vimc_device *vimc)
>> -{
>> -	unsigned int i;
>> -
>> -	for (i = 0; i < vimc->pipe_cfg->num_ents; i++)
>> -		platform_device_unregister(vimc->subdevs[i]);
>> -}
>> -
>> -static const struct component_master_ops vimc_comp_ops = {
>> -	.bind = vimc_comp_bind,
>> -	.unbind = vimc_comp_unbind,
>> -};
>> -
>>   static int vimc_probe(struct platform_device *pdev)
>>   {
>>   	struct vimc_device *vimc = container_of(pdev, struct vimc_device, pdev);
>> -	struct component_match *match = NULL;
>>   	int ret;
>>   
>>   	dev_dbg(&pdev->dev, "probe");
>>   
>>   	memset(&vimc->mdev, 0, sizeof(vimc->mdev));
>>   
>> -	/* Create platform_device for each entity in the topology*/
>> -	vimc->subdevs = devm_kcalloc(&vimc->pdev.dev, vimc->pipe_cfg->num_ents,
>> -				     sizeof(*vimc->subdevs), GFP_KERNEL);
>> -	if (!vimc->subdevs)
>> -		return -ENOMEM;
>> -
>> -	match = vimc_add_subdevs(vimc);
>> -	if (IS_ERR(match))
>> -		return PTR_ERR(match);
>> -
>>   	/* Link the media device within the v4l2_device */
>>   	vimc->v4l2_dev.mdev = &vimc->mdev;
>>   
>> @@ -314,12 +275,9 @@ static int vimc_probe(struct platform_device *pdev)
>>   	vimc->mdev.dev = &pdev->dev;
>>   	media_device_init(&vimc->mdev);
>>   
>> -	/* Add self to the component system */
>> -	ret = component_master_add_with_match(&pdev->dev, &vimc_comp_ops,
>> -					      match);
>> +	ret = vimc_register_devices(vimc);
>>   	if (ret) {
>>   		media_device_cleanup(&vimc->mdev);
>> -		vimc_rm_subdevs(vimc);
>>   		return ret;
>>   	}
>>   
>> @@ -332,8 +290,8 @@ static int vimc_remove(struct platform_device *pdev)
>>   
>>   	dev_dbg(&pdev->dev, "remove");
>>   
>> -	component_master_del(&pdev->dev, &vimc_comp_ops);
>>   	vimc_rm_subdevs(vimc);
>> +	vimc_unregister(vimc);
>>   
>>   	return 0;
>>   }
>> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
>> index b72b8385067b..00850f2501ad 100644
>> --- a/drivers/media/platform/vimc/vimc-debayer.c
>> +++ b/drivers/media/platform/vimc/vimc-debayer.c
>> @@ -15,8 +15,6 @@
>>   
> 
> Could you also remove the following headers?
> 
> #include·<linux/component.h>
> #include·<linux/module.h>
> #include·<linux/mod_devicetable.h>
> #include·<linux/platform_device.h>

I will remove all the ones I can. I think I need module.h for
the module_param()

> 
>>   #include "vimc-common.h"
>>   
>> -#define VIMC_DEB_DRV_NAME "vimc-debayer"
>> -
>>   static unsigned int deb_mean_win_size = 3;
>>   module_param(deb_mean_win_size, uint, 0000);
>>   MODULE_PARM_DESC(deb_mean_win_size, " the window size to calculate the mean.\n"
>> @@ -491,44 +489,40 @@ static const struct v4l2_subdev_internal_ops vimc_deb_int_ops = {
>>   	.release = vimc_deb_release,
>>   };
>>   
>> -static void vimc_deb_comp_unbind(struct device *comp, struct device *master,
>> -				 void *master_data)
>> +void vimc_deb_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
>>   {
>> -	struct vimc_ent_device *ved = dev_get_drvdata(comp);
>> -	struct vimc_deb_device *vdeb = container_of(ved, struct vimc_deb_device,
>> -						    ved);
>> +	struct vimc_deb_device *vdeb;
>>   
>> +	vdeb = container_of(ved, struct vimc_deb_device, ved);
>>   	vimc_ent_sd_unregister(ved, &vdeb->sd);
>>   }
>>   
>> -static int vimc_deb_comp_bind(struct device *comp, struct device *master,
>> -			      void *master_data)
>> +struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
>> +				     const char *vcfg_name)
>>   {
>> -	struct v4l2_device *v4l2_dev = master_data;
>> -	struct vimc_platform_data *pdata = comp->platform_data;
>> +	struct v4l2_device *v4l2_dev = &vimc->v4l2_dev;
>>   	struct vimc_deb_device *vdeb;
>>   	int ret;
>>   
>>   	/* Allocate the vdeb struct */
>>   	vdeb = kzalloc(sizeof(*vdeb), GFP_KERNEL);
>>   	if (!vdeb)
>> -		return -ENOMEM;
>> +		return NULL;
>>   
>>   	/* Initialize ved and sd */
>>   	ret = vimc_ent_sd_register(&vdeb->ved, &vdeb->sd, v4l2_dev,
>> -				   pdata->entity_name,
>> +				   vcfg_name,
>>   				   MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV, 2,
>>   				   (const unsigned long[2]) {MEDIA_PAD_FL_SINK,
>>   				   MEDIA_PAD_FL_SOURCE},
>>   				   &vimc_deb_int_ops, &vimc_deb_ops);
>>   	if (ret) {
>>   		kfree(vdeb);
>> -		return ret;
>> +		return NULL;
>>   	}
>>   
>>   	vdeb->ved.process_frame = vimc_deb_process_frame;
>> -	dev_set_drvdata(comp, &vdeb->ved);
>> -	vdeb->dev = comp;
>> +	vdeb->dev = &vimc->pdev.dev;
>>   
>>   	/* Initialize the frame format */
>>   	vdeb->sink_fmt = sink_fmt_default;
>> @@ -541,46 +535,5 @@ static int vimc_deb_comp_bind(struct device *comp, struct device *master,
>>   	vdeb->src_code = MEDIA_BUS_FMT_RGB888_1X24;
>>   	vdeb->set_rgb_src = vimc_deb_set_rgb_mbus_fmt_rgb888_1x24;
>>   
>> -	return 0;
>> +	return &vdeb->ved;
>>   }
>> -
>> -static const struct component_ops vimc_deb_comp_ops = {
>> -	.bind = vimc_deb_comp_bind,
>> -	.unbind = vimc_deb_comp_unbind,
>> -};
>> -
>> -static int vimc_deb_probe(struct platform_device *pdev)
>> -{
>> -	return component_add(&pdev->dev, &vimc_deb_comp_ops);
>> -}
>> -
>> -static int vimc_deb_remove(struct platform_device *pdev)
>> -{
>> -	component_del(&pdev->dev, &vimc_deb_comp_ops);
>> -
>> -	return 0;
>> -}
>> -
>> -static const struct platform_device_id vimc_deb_driver_ids[] = {
>> -	{
>> -		.name           = VIMC_DEB_DRV_NAME,
>> -	},
>> -	{ }
>> -};
>> -
>> -static struct platform_driver vimc_deb_pdrv = {
>> -	.probe		= vimc_deb_probe,
>> -	.remove		= vimc_deb_remove,
>> -	.id_table	= vimc_deb_driver_ids,
>> -	.driver		= {
>> -		.name	= VIMC_DEB_DRV_NAME,
>> -	},
>> -};
>> -
>> -module_platform_driver(vimc_deb_pdrv);
>> -
>> -MODULE_DEVICE_TABLE(platform, vimc_deb_driver_ids);
>> -
>> -MODULE_DESCRIPTION("Virtual Media Controller Driver (VIMC) Debayer");
>> -MODULE_AUTHOR("Helen Mae Koike Fornazier <helen.fornazier@gmail.com>");
>> -MODULE_LICENSE("GPL");
>> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
>> index 49ab8d9dd9c9..28a947950a0b 100644
>> --- a/drivers/media/platform/vimc/vimc-scaler.c
>> +++ b/drivers/media/platform/vimc/vimc-scaler.c
>> @@ -5,18 +5,13 @@
>>    * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com>
>>    */
>>   
>> -#include <linux/component.h>
>>   #include <linux/module.h>
> 
> You can remove this include as well.

We need this for module_param stuff.

> 
>> -#include <linux/mod_devicetable.h>
>> -#include <linux/platform_device.h>
>>   #include <linux/vmalloc.h>
>>   #include <linux/v4l2-mediabus.h>
>>   #include <media/v4l2-subdev.h>
>>   
>>   #include "vimc-common.h"
>>   
>> -#define VIMC_SCA_DRV_NAME "vimc-scaler"
>> -
>>   static unsigned int sca_mult = 3;
>>   module_param(sca_mult, uint, 0000);
>>   MODULE_PARM_DESC(sca_mult, " the image size multiplier");
>> @@ -350,89 +345,43 @@ static const struct v4l2_subdev_internal_ops vimc_sca_int_ops = {
>>   	.release = vimc_sca_release,
>>   };
>>   
>> -static void vimc_sca_comp_unbind(struct device *comp, struct device *master,
>> -				 void *master_data)
>> +void vimc_sca_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
>>   {
>> -	struct vimc_ent_device *ved = dev_get_drvdata(comp);
>> -	struct vimc_sca_device *vsca = container_of(ved, struct vimc_sca_device,
>> -						    ved);
>> +	struct vimc_sca_device *vsca;
>>   
>> +	vsca = container_of(ved, struct vimc_sca_device, ved);
>>   	vimc_ent_sd_unregister(ved, &vsca->sd);
>>   }
>>   
>> -
>> -static int vimc_sca_comp_bind(struct device *comp, struct device *master,
>> -			      void *master_data)
>> +struct vimc_ent_device *vimc_sca_add(struct vimc_device *vimc,
>> +				     const char *vcfg_name)
>>   {
>> -	struct v4l2_device *v4l2_dev = master_data;
>> -	struct vimc_platform_data *pdata = comp->platform_data;
>> +	struct v4l2_device *v4l2_dev = &vimc->v4l2_dev;
>>   	struct vimc_sca_device *vsca;
>>   	int ret;
>>   
>>   	/* Allocate the vsca struct */
>>   	vsca = kzalloc(sizeof(*vsca), GFP_KERNEL);
>>   	if (!vsca)
>> -		return -ENOMEM;
>> +		return NULL;
>>   
>>   	/* Initialize ved and sd */
>>   	ret = vimc_ent_sd_register(&vsca->ved, &vsca->sd, v4l2_dev,
>> -				   pdata->entity_name,
>> +				   vcfg_name,
>>   				   MEDIA_ENT_F_PROC_VIDEO_SCALER, 2,
>>   				   (const unsigned long[2]) {MEDIA_PAD_FL_SINK,
>>   				   MEDIA_PAD_FL_SOURCE},
>>   				   &vimc_sca_int_ops, &vimc_sca_ops);
>>   	if (ret) {
>>   		kfree(vsca);
>> -		return ret;
>> +		return NULL;
>>   	}
>>   
>>   	vsca->ved.process_frame = vimc_sca_process_frame;
>> -	dev_set_drvdata(comp, &vsca->ved);
>> -	vsca->dev = comp;
>> +	vsca->dev = &vimc->pdev.dev;
>>   
>>   	/* Initialize the frame format */
>>   	vsca->sink_fmt = sink_fmt_default;
>>   
>> -	return 0;
>> -}
>> -
>> -static const struct component_ops vimc_sca_comp_ops = {
>> -	.bind = vimc_sca_comp_bind,
>> -	.unbind = vimc_sca_comp_unbind,
>> -};
>> -
>> -static int vimc_sca_probe(struct platform_device *pdev)
>> -{
>> -	return component_add(&pdev->dev, &vimc_sca_comp_ops);
>> +	return &vsca->ved;
>>   }
>> -
>> -static int vimc_sca_remove(struct platform_device *pdev)
>> -{
>> -	component_del(&pdev->dev, &vimc_sca_comp_ops);
>> -
>> -	return 0;
>> -}
>> -
>> -static const struct platform_device_id vimc_sca_driver_ids[] = {
>> -	{
>> -		.name           = VIMC_SCA_DRV_NAME,
>> -	},
>> -	{ }
>> -};
>> -
>> -static struct platform_driver vimc_sca_pdrv = {
>> -	.probe		= vimc_sca_probe,
>> -	.remove		= vimc_sca_remove,
>> -	.id_table	= vimc_sca_driver_ids,
>> -	.driver		= {
>> -		.name	= VIMC_SCA_DRV_NAME,
>> -	},
>> -};
>> -
>> -module_platform_driver(vimc_sca_pdrv);
>> -
>> -MODULE_DEVICE_TABLE(platform, vimc_sca_driver_ids);
>> -
>> -MODULE_DESCRIPTION("Virtual Media Controller Driver (VIMC) Scaler");
>> -MODULE_AUTHOR("Helen Mae Koike Fornazier <helen.fornazier@gmail.com>");
>> -MODULE_LICENSE("GPL");
>> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
>> index 6c53b9fc1617..1f15637ca8bb 100644
>> --- a/drivers/media/platform/vimc/vimc-sensor.c
>> +++ b/drivers/media/platform/vimc/vimc-sensor.c
>> @@ -5,10 +5,6 @@
>>    * Copyright (C) 2015-2017 Helen Koike <helen.fornazier@gmail.com>
>>    */
>>   
>> -#include <linux/component.h>
>> -#include <linux/module.h>
>> -#include <linux/mod_devicetable.h>
>> -#include <linux/platform_device.h>
>>   #include <linux/v4l2-mediabus.h>
>>   #include <linux/vmalloc.h>
>>   #include <media/v4l2-ctrls.h>
>> @@ -18,8 +14,6 @@
>>   
>>   #include "vimc-common.h"
>>   
>> -#define VIMC_SEN_DRV_NAME "vimc-sensor"
>> -
>>   struct vimc_sen_device {
>>   	struct vimc_ent_device ved;
>>   	struct v4l2_subdev sd;
>> @@ -304,13 +298,11 @@ static const struct v4l2_subdev_internal_ops vimc_sen_int_ops = {
>>   	.release = vimc_sen_release,
>>   };
>>   
>> -static void vimc_sen_comp_unbind(struct device *comp, struct device *master,
>> -				 void *master_data)
>> +void vimc_sen_rm(struct vimc_device *vimc, struct vimc_ent_device *ved)
>>   {
>> -	struct vimc_ent_device *ved = dev_get_drvdata(comp);
>> -	struct vimc_sen_device *vsen =
>> -				container_of(ved, struct vimc_sen_device, ved);
>> +	struct vimc_sen_device *vsen;
>>   
>> +	vsen = container_of(ved, struct vimc_sen_device, ved);
>>   	vimc_ent_sd_unregister(ved, &vsen->sd);
>>   }
>>   
>> @@ -331,18 +323,17 @@ static const struct v4l2_ctrl_config vimc_sen_ctrl_test_pattern = {
>>   	.qmenu = tpg_pattern_strings,
>>   };
>>   
>> -static int vimc_sen_comp_bind(struct device *comp, struct device *master,
>> -			      void *master_data)
>> +struct vimc_ent_device *vimc_sen_add(struct vimc_device *vimc,
>> +				     const char *vcfg_name)
>>   {
>> -	struct v4l2_device *v4l2_dev = master_data;
>> -	struct vimc_platform_data *pdata = comp->platform_data;
>> +	struct v4l2_device *v4l2_dev = &vimc->v4l2_dev;
>>   	struct vimc_sen_device *vsen;
>>   	int ret;
>>   
>>   	/* Allocate the vsen struct */
>>   	vsen = kzalloc(sizeof(*vsen), GFP_KERNEL);
>>   	if (!vsen)
>> -		return -ENOMEM;
>> +		return NULL;
>>   
>>   	v4l2_ctrl_handler_init(&vsen->hdl, 4);
>>   
>> @@ -368,7 +359,7 @@ static int vimc_sen_comp_bind(struct device *comp, struct device *master,
>>   
>>   	/* Initialize ved and sd */
>>   	ret = vimc_ent_sd_register(&vsen->ved, &vsen->sd, v4l2_dev,
>> -				   pdata->entity_name,
>> +				   vcfg_name,
>>   				   MEDIA_ENT_F_CAM_SENSOR, 1,
>>   				   (const unsigned long[1]) {MEDIA_PAD_FL_SOURCE},
>>   				   &vimc_sen_int_ops, &vimc_sen_ops);
>> @@ -376,8 +367,7 @@ static int vimc_sen_comp_bind(struct device *comp, struct device *master,
>>   		goto err_free_hdl;
>>   
>>   	vsen->ved.process_frame = vimc_sen_process_frame;
>> -	dev_set_drvdata(comp, &vsen->ved);
>> -	vsen->dev = comp;
>> +	vsen->dev = &vimc->pdev.dev;
>>   
>>   	/* Initialize the frame format */
>>   	vsen->mbus_format = fmt_default;
>> @@ -389,7 +379,7 @@ static int vimc_sen_comp_bind(struct device *comp, struct device *master,
>>   	if (ret)
>>   		goto err_unregister_ent_sd;
>>   
>> -	return 0;
>> +	return &vsen->ved;
>>   
>>   err_unregister_ent_sd:
>>   	vimc_ent_sd_unregister(&vsen->ved,  &vsen->sd);
>> @@ -398,46 +388,5 @@ static int vimc_sen_comp_bind(struct device *comp, struct device *master,
>>   err_free_vsen:
>>   	kfree(vsen);
>>   
>> -	return ret;
>> +	return NULL;
>>   }
>> -
>> -static const struct component_ops vimc_sen_comp_ops = {
>> -	.bind = vimc_sen_comp_bind,
>> -	.unbind = vimc_sen_comp_unbind,
>> -};
>> -
>> -static int vimc_sen_probe(struct platform_device *pdev)
>> -{
>> -	return component_add(&pdev->dev, &vimc_sen_comp_ops);
>> -}
>> -
>> -static int vimc_sen_remove(struct platform_device *pdev)
>> -{
>> -	component_del(&pdev->dev, &vimc_sen_comp_ops);
>> -
>> -	return 0;
>> -}
>> -
>> -static const struct platform_device_id vimc_sen_driver_ids[] = {
>> -	{
>> -		.name           = VIMC_SEN_DRV_NAME,
>> -	},
>> -	{ }
>> -};
>> -
>> -static struct platform_driver vimc_sen_pdrv = {
>> -	.probe		= vimc_sen_probe,
>> -	.remove		= vimc_sen_remove,
>> -	.id_table	= vimc_sen_driver_ids,
>> -	.driver		= {
>> -		.name	= VIMC_SEN_DRV_NAME,
>> -	},
>> -};
>> -
>> -module_platform_driver(vimc_sen_pdrv);
>> -
>> -MODULE_DEVICE_TABLE(platform, vimc_sen_driver_ids);
>> -
>> -MODULE_DESCRIPTION("Virtual Media Controller Driver (VIMC) Sensor");
>> -MODULE_AUTHOR("Helen Mae Koike Fornazier <helen.fornazier@gmail.com>");
>> -MODULE_LICENSE("GPL");
>>
> 
> With these changes:

I will send v5 with the suggested changes.

> 
> Acked-by: Helen Koike <helen.koike@collabora.com>
> 

thanks,
-- Shuah

[PATCH v4 2/5] media: vimc: Fix gpf in rmmod path when stream is active

[Shuah Khan]

If vimc module is removed while streaming is in progress, sensor subdev
unregister runs into general protection fault when it tries to unregister
media entities. This is a common subdev problem related to releasing
pads from v4l2_device_unregister_subdev() before calling unregister.
Unregister references pads during unregistering subdev.

The sd release handler is the right place for releasing all sd resources
including pads. The release handlers currently release all resources
except the pads.

Fix v4l2_device_unregister_subdev() not release pads and release pads
from the sd_int_op release handlers.

kernel: [ 4136.715839] general protection fault: 0000 [#1] SMP PTI
kernel: [ 4136.715847] CPU: 2 PID: 1972 Comm: bash Not tainted 5.3.0-rc2+ #4
kernel: [ 4136.715850] Hardware name: Dell Inc. OptiPlex 790/0HY9JP, BIOS A18 09/24/2013
kernel: [ 4136.715858] RIP: 0010:media_gobj_destroy.part.16+0x1f/0x60
kernel: [ 4136.715863] Code: ff 66 2e 0f 1f 84 00 00 00 00 00 66 66 66 66 90 55 48 89 fe 48 89 e5 53 48 89 fb 48 c7 c7 00 7f cf b0 e8 24 fa ff ff 48 8b 03 <48> 83 80 a0 00 00 00 01 48 8b 43 18 48 8b 53 10 48 89 42 08 48 89
kernel: [ 4136.715866] RSP: 0018:ffff9b2248fe3cb0 EFLAGS: 00010246
kernel: [ 4136.715870] RAX: bcf2bfbfa0d63c2f RBX: ffff88c3eb37e9c0 RCX: 00000000802a0018
kernel: [ 4136.715873] RDX: ffff88c3e4f6a078 RSI: ffff88c3eb37e9c0 RDI: ffffffffb0cf7f00
kernel: [ 4136.715876] RBP: ffff9b2248fe3cb8 R08: 0000000001000002 R09: ffffffffb0492b00
kernel: [ 4136.715879] R10: ffff9b2248fe3c28 R11: 0000000000000001 R12: 0000000000000038
kernel: [ 4136.715881] R13: ffffffffc09a1628 R14: ffff88c3e4f6a028 R15: fffffffffffffff2
kernel: [ 4136.715885] FS:  00007f8389647740(0000) GS:ffff88c465500000(0000) knlGS:0000000000000000
kernel: [ 4136.715888] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
kernel: [ 4136.715891] CR2: 000055d008f80fd8 CR3: 00000001996ec005 CR4: 00000000000606e0
kernel: [ 4136.715894] Call Trace:
kernel: [ 4136.715903]  media_gobj_destroy+0x14/0x20
kernel: [ 4136.715908]  __media_device_unregister_entity+0xb3/0xe0
kernel: [ 4136.715915]  media_device_unregister_entity+0x30/0x40
kernel: [ 4136.715920]  v4l2_device_unregister_subdev+0xa8/0xe0
kernel: [ 4136.715928]  vimc_ent_sd_unregister+0x1e/0x30 [vimc]
kernel: [ 4136.715933]  vimc_sen_rm+0x16/0x20 [vimc]
kernel: [ 4136.715938]  vimc_remove+0x3e/0xa0 [vimc]
kernel: [ 4136.715945]  platform_drv_remove+0x25/0x50
kernel: [ 4136.715951]  device_release_driver_internal+0xe0/0x1b0
kernel: [ 4136.715956]  device_driver_detach+0x14/0x20
kernel: [ 4136.715960]  unbind_store+0xd1/0x130
kernel: [ 4136.715965]  drv_attr_store+0x27/0x40
kernel: [ 4136.715971]  sysfs_kf_write+0x48/0x60
kernel: [ 4136.715976]  kernfs_fop_write+0x128/0x1b0
kernel: [ 4136.715982]  __vfs_write+0x1b/0x40
kernel: [ 4136.715987]  vfs_write+0xc3/0x1d0
kernel: [ 4136.715993]  ksys_write+0xaa/0xe0
kernel: [ 4136.715999]  __x64_sys_write+0x1a/0x20
kernel: [ 4136.716005]  do_syscall_64+0x5a/0x130
kernel: [ 4136.716010]  entry_SYSCALL_64_after_hwframe+0x4
Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/media/platform/vimc/vimc-common.c  | 3 +--
 drivers/media/platform/vimc/vimc-debayer.c | 1 +
 drivers/media/platform/vimc/vimc-scaler.c  | 1 +
 drivers/media/platform/vimc/vimc-sensor.c  | 1 +
 4 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
index 7e1ae0b12f1e..a3120f4f7a90 100644
--- a/drivers/media/platform/vimc/vimc-common.c
+++ b/drivers/media/platform/vimc/vimc-common.c
@@ -375,7 +375,7 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
 {
 	int ret;
 
-	/* Allocate the pads */
+	/* Allocate the pads. Should be released from the sd_int_op release */
 	ved->pads = vimc_pads_init(num_pads, pads_flag);
 	if (IS_ERR(ved->pads))
 		return PTR_ERR(ved->pads);
@@ -424,7 +424,6 @@ EXPORT_SYMBOL_GPL(vimc_ent_sd_register);
 void vimc_ent_sd_unregister(struct vimc_ent_device *ved, struct v4l2_subdev *sd)
 {
 	media_entity_cleanup(ved->ent);
-	vimc_pads_cleanup(ved->pads);
 	v4l2_device_unregister_subdev(sd);
 }
 EXPORT_SYMBOL_GPL(vimc_ent_sd_unregister);
diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
index 00850f2501ad..b38b55f51a24 100644
--- a/drivers/media/platform/vimc/vimc-debayer.c
+++ b/drivers/media/platform/vimc/vimc-debayer.c
@@ -482,6 +482,7 @@ static void vimc_deb_release(struct v4l2_subdev *sd)
 	struct vimc_deb_device *vdeb =
 				container_of(sd, struct vimc_deb_device, sd);
 
+	vimc_pads_cleanup(vdeb->ved.pads);
 	kfree(vdeb);
 }
 
diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
index 28a947950a0b..05db5070e268 100644
--- a/drivers/media/platform/vimc/vimc-scaler.c
+++ b/drivers/media/platform/vimc/vimc-scaler.c
@@ -338,6 +338,7 @@ static void vimc_sca_release(struct v4l2_subdev *sd)
 	struct vimc_sca_device *vsca =
 				container_of(sd, struct vimc_sca_device, sd);
 
+	vimc_pads_cleanup(vsca->ved.pads);
 	kfree(vsca);
 }
 
diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
index 1f15637ca8bb..46dc6a535abe 100644
--- a/drivers/media/platform/vimc/vimc-sensor.c
+++ b/drivers/media/platform/vimc/vimc-sensor.c
@@ -291,6 +291,7 @@ static void vimc_sen_release(struct v4l2_subdev *sd)
 
 	v4l2_ctrl_handler_free(&vsen->hdl);
 	tpg_free(&vsen->tpg);
+	vimc_pads_cleanup(vsen->ved.pads);
 	kfree(vsen);
 }
 
-- 
2.20.1

Re: [PATCH v4 2/5] media: vimc: Fix gpf in rmmod path when stream is active

[Helen Koike]

On 9/6/19 11:42 PM, Shuah Khan wrote:
> If vimc module is removed while streaming is in progress, sensor subdev
> unregister runs into general protection fault when it tries to unregister
> media entities. This is a common subdev problem related to releasing
> pads from v4l2_device_unregister_subdev() before calling unregister.
> Unregister references pads during unregistering subdev.
> 
> The sd release handler is the right place for releasing all sd resources
> including pads. The release handlers currently release all resources
> except the pads.
> 
> Fix v4l2_device_unregister_subdev() not release pads and release pads
> from the sd_int_op release handlers.
> 
> kernel: [ 4136.715839] general protection fault: 0000 [#1] SMP PTI
> kernel: [ 4136.715847] CPU: 2 PID: 1972 Comm: bash Not tainted 5.3.0-rc2+ #4
> kernel: [ 4136.715850] Hardware name: Dell Inc. OptiPlex 790/0HY9JP, BIOS A18 09/24/2013
> kernel: [ 4136.715858] RIP: 0010:media_gobj_destroy.part.16+0x1f/0x60
> kernel: [ 4136.715863] Code: ff 66 2e 0f 1f 84 00 00 00 00 00 66 66 66 66 90 55 48 89 fe 48 89 e5 53 48 89 fb 48 c7 c7 00 7f cf b0 e8 24 fa ff ff 48 8b 03 <48> 83 80 a0 00 00 00 01 48 8b 43 18 48 8b 53 10 48 89 42 08 48 89
> kernel: [ 4136.715866] RSP: 0018:ffff9b2248fe3cb0 EFLAGS: 00010246
> kernel: [ 4136.715870] RAX: bcf2bfbfa0d63c2f RBX: ffff88c3eb37e9c0 RCX: 00000000802a0018
> kernel: [ 4136.715873] RDX: ffff88c3e4f6a078 RSI: ffff88c3eb37e9c0 RDI: ffffffffb0cf7f00
> kernel: [ 4136.715876] RBP: ffff9b2248fe3cb8 R08: 0000000001000002 R09: ffffffffb0492b00
> kernel: [ 4136.715879] R10: ffff9b2248fe3c28 R11: 0000000000000001 R12: 0000000000000038
> kernel: [ 4136.715881] R13: ffffffffc09a1628 R14: ffff88c3e4f6a028 R15: fffffffffffffff2
> kernel: [ 4136.715885] FS:  00007f8389647740(0000) GS:ffff88c465500000(0000) knlGS:0000000000000000
> kernel: [ 4136.715888] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> kernel: [ 4136.715891] CR2: 000055d008f80fd8 CR3: 00000001996ec005 CR4: 00000000000606e0
> kernel: [ 4136.715894] Call Trace:
> kernel: [ 4136.715903]  media_gobj_destroy+0x14/0x20
> kernel: [ 4136.715908]  __media_device_unregister_entity+0xb3/0xe0
> kernel: [ 4136.715915]  media_device_unregister_entity+0x30/0x40
> kernel: [ 4136.715920]  v4l2_device_unregister_subdev+0xa8/0xe0
> kernel: [ 4136.715928]  vimc_ent_sd_unregister+0x1e/0x30 [vimc]
> kernel: [ 4136.715933]  vimc_sen_rm+0x16/0x20 [vimc]
> kernel: [ 4136.715938]  vimc_remove+0x3e/0xa0 [vimc]
> kernel: [ 4136.715945]  platform_drv_remove+0x25/0x50
> kernel: [ 4136.715951]  device_release_driver_internal+0xe0/0x1b0
> kernel: [ 4136.715956]  device_driver_detach+0x14/0x20
> kernel: [ 4136.715960]  unbind_store+0xd1/0x130
> kernel: [ 4136.715965]  drv_attr_store+0x27/0x40
> kernel: [ 4136.715971]  sysfs_kf_write+0x48/0x60
> kernel: [ 4136.715976]  kernfs_fop_write+0x128/0x1b0
> kernel: [ 4136.715982]  __vfs_write+0x1b/0x40
> kernel: [ 4136.715987]  vfs_write+0xc3/0x1d0
> kernel: [ 4136.715993]  ksys_write+0xaa/0xe0
> kernel: [ 4136.715999]  __x64_sys_write+0x1a/0x20
> kernel: [ 4136.716005]  do_syscall_64+0x5a/0x130
> kernel: [ 4136.716010]  entry_SYSCALL_64_after_hwframe+0x4
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

Acked-by: Helen Koike <helen.koike@collabora.com>

> ---
>  drivers/media/platform/vimc/vimc-common.c  | 3 +--
>  drivers/media/platform/vimc/vimc-debayer.c | 1 +
>  drivers/media/platform/vimc/vimc-scaler.c  | 1 +
>  drivers/media/platform/vimc/vimc-sensor.c  | 1 +
>  4 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-common.c b/drivers/media/platform/vimc/vimc-common.c
> index 7e1ae0b12f1e..a3120f4f7a90 100644
> --- a/drivers/media/platform/vimc/vimc-common.c
> +++ b/drivers/media/platform/vimc/vimc-common.c
> @@ -375,7 +375,7 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
>  {
>  	int ret;
>  
> -	/* Allocate the pads */
> +	/* Allocate the pads. Should be released from the sd_int_op release */
>  	ved->pads = vimc_pads_init(num_pads, pads_flag);
>  	if (IS_ERR(ved->pads))
>  		return PTR_ERR(ved->pads);
> @@ -424,7 +424,6 @@ EXPORT_SYMBOL_GPL(vimc_ent_sd_register);
>  void vimc_ent_sd_unregister(struct vimc_ent_device *ved, struct v4l2_subdev *sd)
>  {
>  	media_entity_cleanup(ved->ent);
> -	vimc_pads_cleanup(ved->pads);
>  	v4l2_device_unregister_subdev(sd);
>  }
>  EXPORT_SYMBOL_GPL(vimc_ent_sd_unregister);
> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
> index 00850f2501ad..b38b55f51a24 100644
> --- a/drivers/media/platform/vimc/vimc-debayer.c
> +++ b/drivers/media/platform/vimc/vimc-debayer.c
> @@ -482,6 +482,7 @@ static void vimc_deb_release(struct v4l2_subdev *sd)
>  	struct vimc_deb_device *vdeb =
>  				container_of(sd, struct vimc_deb_device, sd);
>  
> +	vimc_pads_cleanup(vdeb->ved.pads);
>  	kfree(vdeb);
>  }
>  
> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> index 28a947950a0b..05db5070e268 100644
> --- a/drivers/media/platform/vimc/vimc-scaler.c
> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> @@ -338,6 +338,7 @@ static void vimc_sca_release(struct v4l2_subdev *sd)
>  	struct vimc_sca_device *vsca =
>  				container_of(sd, struct vimc_sca_device, sd);
>  
> +	vimc_pads_cleanup(vsca->ved.pads);
>  	kfree(vsca);
>  }
>  
> diff --git a/drivers/media/platform/vimc/vimc-sensor.c b/drivers/media/platform/vimc/vimc-sensor.c
> index 1f15637ca8bb..46dc6a535abe 100644
> --- a/drivers/media/platform/vimc/vimc-sensor.c
> +++ b/drivers/media/platform/vimc/vimc-sensor.c
> @@ -291,6 +291,7 @@ static void vimc_sen_release(struct v4l2_subdev *sd)
>  
>  	v4l2_ctrl_handler_free(&vsen->hdl);
>  	tpg_free(&vsen->tpg);
> +	vimc_pads_cleanup(vsen->ved.pads);
>  	kfree(vsen);
>  }
>  
> 

[PATCH v4 3/5] vimc: move duplicated IS_SRC and IS_SINK to common header

[Shuah Khan]

Move duplicated IS_SRC and IS_SINK dfines to common header. Rename
them to VIMC_IS_SRC and VIM_IS_SINK.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 drivers/media/platform/vimc/vimc-common.h  |  4 ++++
 drivers/media/platform/vimc/vimc-debayer.c | 11 ++++-------
 drivers/media/platform/vimc/vimc-scaler.c  |  8 +++-----
 3 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
index 87ee84f78322..236412ad7548 100644
--- a/drivers/media/platform/vimc/vimc-common.h
+++ b/drivers/media/platform/vimc/vimc-common.h
@@ -27,6 +27,10 @@
 
 #define VIMC_FRAME_INDEX(lin, col, width, bpp) ((lin * width + col) * bpp)
 
+/* Source and sink pad checks */
+#define VIMC_IS_SRC(pad)	(pad)
+#define VIMC_IS_SINK(pad)	(!(pad))
+
 /**
  * struct vimc_colorimetry_clamp - Adjust colorimetry parameters
  *
diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
index b38b55f51a24..37f3767db469 100644
--- a/drivers/media/platform/vimc/vimc-debayer.c
+++ b/drivers/media/platform/vimc/vimc-debayer.c
@@ -22,9 +22,6 @@ MODULE_PARM_DESC(deb_mean_win_size, " the window size to calculate the mean.\n"
 	"stays in the center of the window, otherwise the next odd number "
 	"is considered");
 
-#define IS_SINK(pad) (!pad)
-#define IS_SRC(pad)  (pad)
-
 enum vimc_deb_rgb_colors {
 	VIMC_DEB_RED = 0,
 	VIMC_DEB_GREEN = 1,
@@ -157,7 +154,7 @@ static int vimc_deb_enum_mbus_code(struct v4l2_subdev *sd,
 				   struct v4l2_subdev_mbus_code_enum *code)
 {
 	/* We only support one format for source pads */
-	if (IS_SRC(code->pad)) {
+	if (VIMC_IS_SRC(code->pad)) {
 		struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd);
 
 		if (code->index)
@@ -183,7 +180,7 @@ static int vimc_deb_enum_frame_size(struct v4l2_subdev *sd,
 	if (fse->index)
 		return -EINVAL;
 
-	if (IS_SINK(fse->pad)) {
+	if (VIMC_IS_SINK(fse->pad)) {
 		const struct vimc_deb_pix_map *vpix =
 			vimc_deb_pix_map_by_code(fse->code);
 
@@ -213,7 +210,7 @@ static int vimc_deb_get_fmt(struct v4l2_subdev *sd,
 		      vdeb->sink_fmt;
 
 	/* Set the right code for the source pad */
-	if (IS_SRC(fmt->pad))
+	if (VIMC_IS_SRC(fmt->pad))
 		fmt->format.code = vdeb->src_code;
 
 	return 0;
@@ -260,7 +257,7 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd,
 	 * Do not change the format of the source pad,
 	 * it is propagated from the sink
 	 */
-	if (IS_SRC(fmt->pad)) {
+	if (VIMC_IS_SRC(fmt->pad)) {
 		fmt->format = *sink_fmt;
 		/* TODO: Add support for other formats */
 		fmt->format.code = vdeb->src_code;
diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
index 05db5070e268..a5a0855ad9cd 100644
--- a/drivers/media/platform/vimc/vimc-scaler.c
+++ b/drivers/media/platform/vimc/vimc-scaler.c
@@ -16,8 +16,6 @@ static unsigned int sca_mult = 3;
 module_param(sca_mult, uint, 0000);
 MODULE_PARM_DESC(sca_mult, " the image size multiplier");
 
-#define IS_SINK(pad)	(!pad)
-#define IS_SRC(pad)	(pad)
 #define MAX_ZOOM	8
 
 struct vimc_sca_device {
@@ -93,7 +91,7 @@ static int vimc_sca_enum_frame_size(struct v4l2_subdev *sd,
 	fse->min_width = VIMC_FRAME_MIN_WIDTH;
 	fse->min_height = VIMC_FRAME_MIN_HEIGHT;
 
-	if (IS_SINK(fse->pad)) {
+	if (VIMC_IS_SINK(fse->pad)) {
 		fse->max_width = VIMC_FRAME_MAX_WIDTH;
 		fse->max_height = VIMC_FRAME_MAX_HEIGHT;
 	} else {
@@ -116,7 +114,7 @@ static int vimc_sca_get_fmt(struct v4l2_subdev *sd,
 			 vsca->sink_fmt;
 
 	/* Scale the frame size for the source pad */
-	if (IS_SRC(format->pad)) {
+	if (VIMC_IS_SRC(format->pad)) {
 		format->format.width = vsca->sink_fmt.width * sca_mult;
 		format->format.height = vsca->sink_fmt.height * sca_mult;
 	}
@@ -165,7 +163,7 @@ static int vimc_sca_set_fmt(struct v4l2_subdev *sd,
 	 * Do not change the format of the source pad,
 	 * it is propagated from the sink
 	 */
-	if (IS_SRC(fmt->pad)) {
+	if (VIMC_IS_SRC(fmt->pad)) {
 		fmt->format = *sink_fmt;
 		fmt->format.width = sink_fmt->width * sca_mult;
 		fmt->format.height = sink_fmt->height * sca_mult;
-- 
2.20.1

Re: [PATCH v4 3/5] vimc: move duplicated IS_SRC and IS_SINK to common header

[Helen Koike]

Hi Shuah,

On 9/6/19 11:42 PM, Shuah Khan wrote:
> Move duplicated IS_SRC and IS_SINK dfines to common header. Rename
> them to VIMC_IS_SRC and VIM_IS_SINK.
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
> ---
>  drivers/media/platform/vimc/vimc-common.h  |  4 ++++
>  drivers/media/platform/vimc/vimc-debayer.c | 11 ++++-------
>  drivers/media/platform/vimc/vimc-scaler.c  |  8 +++-----
>  3 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
> index 87ee84f78322..236412ad7548 100644
> --- a/drivers/media/platform/vimc/vimc-common.h
> +++ b/drivers/media/platform/vimc/vimc-common.h
> @@ -27,6 +27,10 @@
>  
>  #define VIMC_FRAME_INDEX(lin, col, width, bpp) ((lin * width + col) * bpp)
>  
> +/* Source and sink pad checks */
> +#define VIMC_IS_SRC(pad)	(pad)
> +#define VIMC_IS_SINK(pad)	(!(pad))

This is true now, but it might not be true in the future.
In the output video device (that was sent by André but not yet upstream) for instance, only have a single
source pad (which I suppose the index will be 0), and this macro won't be true.

Maybe we could check pad flags in sd->entity->pads[index].flags ?

Thanks
Helen

> +
>  /**
>   * struct vimc_colorimetry_clamp - Adjust colorimetry parameters
>   *
> diff --git a/drivers/media/platform/vimc/vimc-debayer.c b/drivers/media/platform/vimc/vimc-debayer.c
> index b38b55f51a24..37f3767db469 100644
> --- a/drivers/media/platform/vimc/vimc-debayer.c
> +++ b/drivers/media/platform/vimc/vimc-debayer.c
> @@ -22,9 +22,6 @@ MODULE_PARM_DESC(deb_mean_win_size, " the window size to calculate the mean.\n"
>  	"stays in the center of the window, otherwise the next odd number "
>  	"is considered");
>  
> -#define IS_SINK(pad) (!pad)
> -#define IS_SRC(pad)  (pad)
> -
>  enum vimc_deb_rgb_colors {
>  	VIMC_DEB_RED = 0,
>  	VIMC_DEB_GREEN = 1,
> @@ -157,7 +154,7 @@ static int vimc_deb_enum_mbus_code(struct v4l2_subdev *sd,
>  				   struct v4l2_subdev_mbus_code_enum *code)
>  {
>  	/* We only support one format for source pads */
> -	if (IS_SRC(code->pad)) {
> +	if (VIMC_IS_SRC(code->pad)) {
>  		struct vimc_deb_device *vdeb = v4l2_get_subdevdata(sd);
>  
>  		if (code->index)
> @@ -183,7 +180,7 @@ static int vimc_deb_enum_frame_size(struct v4l2_subdev *sd,
>  	if (fse->index)
>  		return -EINVAL;
>  
> -	if (IS_SINK(fse->pad)) {
> +	if (VIMC_IS_SINK(fse->pad)) {
>  		const struct vimc_deb_pix_map *vpix =
>  			vimc_deb_pix_map_by_code(fse->code);
>  
> @@ -213,7 +210,7 @@ static int vimc_deb_get_fmt(struct v4l2_subdev *sd,
>  		      vdeb->sink_fmt;
>  
>  	/* Set the right code for the source pad */
> -	if (IS_SRC(fmt->pad))
> +	if (VIMC_IS_SRC(fmt->pad))
>  		fmt->format.code = vdeb->src_code;
>  
>  	return 0;
> @@ -260,7 +257,7 @@ static int vimc_deb_set_fmt(struct v4l2_subdev *sd,
>  	 * Do not change the format of the source pad,
>  	 * it is propagated from the sink
>  	 */
> -	if (IS_SRC(fmt->pad)) {
> +	if (VIMC_IS_SRC(fmt->pad)) {
>  		fmt->format = *sink_fmt;
>  		/* TODO: Add support for other formats */
>  		fmt->format.code = vdeb->src_code;
> diff --git a/drivers/media/platform/vimc/vimc-scaler.c b/drivers/media/platform/vimc/vimc-scaler.c
> index 05db5070e268..a5a0855ad9cd 100644
> --- a/drivers/media/platform/vimc/vimc-scaler.c
> +++ b/drivers/media/platform/vimc/vimc-scaler.c
> @@ -16,8 +16,6 @@ static unsigned int sca_mult = 3;
>  module_param(sca_mult, uint, 0000);
>  MODULE_PARM_DESC(sca_mult, " the image size multiplier");
>  
> -#define IS_SINK(pad)	(!pad)
> -#define IS_SRC(pad)	(pad)
>  #define MAX_ZOOM	8
>  
>  struct vimc_sca_device {
> @@ -93,7 +91,7 @@ static int vimc_sca_enum_frame_size(struct v4l2_subdev *sd,
>  	fse->min_width = VIMC_FRAME_MIN_WIDTH;
>  	fse->min_height = VIMC_FRAME_MIN_HEIGHT;
>  
> -	if (IS_SINK(fse->pad)) {
> +	if (VIMC_IS_SINK(fse->pad)) {
>  		fse->max_width = VIMC_FRAME_MAX_WIDTH;
>  		fse->max_height = VIMC_FRAME_MAX_HEIGHT;
>  	} else {
> @@ -116,7 +114,7 @@ static int vimc_sca_get_fmt(struct v4l2_subdev *sd,
>  			 vsca->sink_fmt;
>  
>  	/* Scale the frame size for the source pad */
> -	if (IS_SRC(format->pad)) {
> +	if (VIMC_IS_SRC(format->pad)) {
>  		format->format.width = vsca->sink_fmt.width * sca_mult;
>  		format->format.height = vsca->sink_fmt.height * sca_mult;
>  	}
> @@ -165,7 +163,7 @@ static int vimc_sca_set_fmt(struct v4l2_subdev *sd,
>  	 * Do not change the format of the source pad,
>  	 * it is propagated from the sink
>  	 */
> -	if (IS_SRC(fmt->pad)) {
> +	if (VIMC_IS_SRC(fmt->pad)) {
>  		fmt->format = *sink_fmt;
>  		fmt->format.width = sink_fmt->width * sca_mult;
>  		fmt->format.height = sink_fmt->height * sca_mult;
> 

Re: [PATCH v4 3/5] vimc: move duplicated IS_SRC and IS_SINK to common header

[Shuah Khan]

On 9/15/19 1:25 PM, Helen Koike wrote:
> Hi Shuah,
> 
> On 9/6/19 11:42 PM, Shuah Khan wrote:
>> Move duplicated IS_SRC and IS_SINK dfines to common header. Rename
>> them to VIMC_IS_SRC and VIM_IS_SINK.
>>
>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>> ---
>>   drivers/media/platform/vimc/vimc-common.h  |  4 ++++
>>   drivers/media/platform/vimc/vimc-debayer.c | 11 ++++-------
>>   drivers/media/platform/vimc/vimc-scaler.c  |  8 +++-----
>>   3 files changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
>> index 87ee84f78322..236412ad7548 100644
>> --- a/drivers/media/platform/vimc/vimc-common.h
>> +++ b/drivers/media/platform/vimc/vimc-common.h
>> @@ -27,6 +27,10 @@
>>   
>>   #define VIMC_FRAME_INDEX(lin, col, width, bpp) ((lin * width + col) * bpp)
>>   
>> +/* Source and sink pad checks */
>> +#define VIMC_IS_SRC(pad)	(pad)
>> +#define VIMC_IS_SINK(pad)	(!(pad))
> 
> This is true now, but it might not be true in the future.
> In the output video device (that was sent by André but not yet upstream) for instance, only have a single
> source pad (which I suppose the index will be 0), and this macro won't be true.
> 

> Maybe we could check pad flags in sd->entity->pads[index].flags ?
> 

I think this change should be done in André's patch?

thanks,
-- Shuah

Re: [PATCH v4 3/5] vimc: move duplicated IS_SRC and IS_SINK to common header

[Helen Koike]

On 9/15/19 8:52 PM, Shuah Khan wrote:
> On 9/15/19 1:25 PM, Helen Koike wrote:
>> Hi Shuah,
>>
>> On 9/6/19 11:42 PM, Shuah Khan wrote:
>>> Move duplicated IS_SRC and IS_SINK dfines to common header. Rename
>>> them to VIMC_IS_SRC and VIM_IS_SINK.
>>>
>>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>>> ---
>>>   drivers/media/platform/vimc/vimc-common.h  |  4 ++++
>>>   drivers/media/platform/vimc/vimc-debayer.c | 11 ++++-------
>>>   drivers/media/platform/vimc/vimc-scaler.c  |  8 +++-----
>>>   3 files changed, 11 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
>>> index 87ee84f78322..236412ad7548 100644
>>> --- a/drivers/media/platform/vimc/vimc-common.h
>>> +++ b/drivers/media/platform/vimc/vimc-common.h
>>> @@ -27,6 +27,10 @@
>>>     #define VIMC_FRAME_INDEX(lin, col, width, bpp) ((lin * width + col) * bpp)
>>>   +/* Source and sink pad checks */
>>> +#define VIMC_IS_SRC(pad)    (pad)
>>> +#define VIMC_IS_SINK(pad)    (!(pad))
>>
>> This is true now, but it might not be true in the future.
>> In the output video device (that was sent by André but not yet upstream) for instance, only have a single
>> source pad (which I suppose the index will be 0), and this macro won't be true.
>>
> 
>> Maybe we could check pad flags in sd->entity->pads[index].flags ?
>>
> 
> I think this change should be done in André's patch?

Could be yes, making it generic since the start would be nice,
but I don't mind updating this latter when needed.

Then:

Acked-by: Helen Koike <helen.koike@collabora.com>

> 
> thanks,
> -- Shuah
Thanks
Helen

Re: [PATCH v4 3/5] vimc: move duplicated IS_SRC and IS_SINK to common header

[Shuah Khan]

On 9/16/19 4:42 AM, Helen Koike wrote:
> 
> 
> On 9/15/19 8:52 PM, Shuah Khan wrote:
>> On 9/15/19 1:25 PM, Helen Koike wrote:
>>> Hi Shuah,
>>>
>>> On 9/6/19 11:42 PM, Shuah Khan wrote:
>>>> Move duplicated IS_SRC and IS_SINK dfines to common header. Rename
>>>> them to VIMC_IS_SRC and VIM_IS_SINK.
>>>>
>>>> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
>>>> ---
>>>>    drivers/media/platform/vimc/vimc-common.h  |  4 ++++
>>>>    drivers/media/platform/vimc/vimc-debayer.c | 11 ++++-------
>>>>    drivers/media/platform/vimc/vimc-scaler.c  |  8 +++-----
>>>>    3 files changed, 11 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
>>>> index 87ee84f78322..236412ad7548 100644
>>>> --- a/drivers/media/platform/vimc/vimc-common.h
>>>> +++ b/drivers/media/platform/vimc/vimc-common.h
>>>> @@ -27,6 +27,10 @@
>>>>      #define VIMC_FRAME_INDEX(lin, col, width, bpp) ((lin * width + col) * bpp)
>>>>    +/* Source and sink pad checks */
>>>> +#define VIMC_IS_SRC(pad)    (pad)
>>>> +#define VIMC_IS_SINK(pad)    (!(pad))
>>>
>>> This is true now, but it might not be true in the future.
>>> In the output video device (that was sent by André but not yet upstream) for instance, only have a single
>>> source pad (which I suppose the index will be 0), and this macro won't be true.
>>>
>>
>>> Maybe we could check pad flags in sd->entity->pads[index].flags ?
>>>
>>
>> I think this change should be done in André's patch?
> 
> Could be yes, making it generic since the start would be nice,
> but I don't mind updating this latter when needed.

Let's go with that then. This way we can get this series in and then
we can clean this up in André's patch.

> 
> Then:
> 
> Acked-by: Helen Koike <helen.koike@collabora.com>
> 
>>

thanks,
-- Shuah

[PATCH v4 4/5] doc: media: vimc: Update module parameter usage information

[Shuah Khan]

vimc driver is now a monolithic driver. Update the module parameter
usage information to reflect that.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 Documentation/media/v4l-drivers/vimc.rst | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/media/v4l-drivers/vimc.rst b/Documentation/media/v4l-drivers/vimc.rst
index 406417680db5..a582af0509ee 100644
--- a/Documentation/media/v4l-drivers/vimc.rst
+++ b/Documentation/media/v4l-drivers/vimc.rst
@@ -76,22 +76,22 @@ vimc-capture:
 	* 1 Pad sink
 	* 1 Pad source
 
-Module options
+
+        Module options
 ---------------
 
-Vimc has a few module parameters to configure the driver. You should pass
-those arguments to each subdevice, not to the vimc module. For example::
+Vimc has a few module parameters to configure the driver.
 
-        vimc_subdevice.param=value
+        param=value
 
-* ``vimc_scaler.sca_mult=<unsigned int>``
+* ``sca_mult=<unsigned int>``
 
         Image size multiplier factor to be used to multiply both width and
         height, so the image size will be ``sca_mult^2`` bigger than the
         original one. Currently, only supports scaling up (the default value
         is 3).
 
-* ``vimc_debayer.deb_mean_win_size=<unsigned int>``
+* ``deb_mean_win_size=<unsigned int>``
 
         Window size to calculate the mean. Note: the window size needs to be an
         odd number, as the main pixel stays in the center of the window,
-- 
2.20.1

Re: [PATCH v4 4/5] doc: media: vimc: Update module parameter usage information

[Helen Koike]

On 9/6/19 11:42 PM, Shuah Khan wrote:
> vimc driver is now a monolithic driver. Update the module parameter
> usage information to reflect that.
> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

Acked-by: Helen Koike <helen.koike@collabora.com>

> ---
>  Documentation/media/v4l-drivers/vimc.rst | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/media/v4l-drivers/vimc.rst b/Documentation/media/v4l-drivers/vimc.rst
> index 406417680db5..a582af0509ee 100644
> --- a/Documentation/media/v4l-drivers/vimc.rst
> +++ b/Documentation/media/v4l-drivers/vimc.rst
> @@ -76,22 +76,22 @@ vimc-capture:
>  	* 1 Pad sink
>  	* 1 Pad source
>  
> -Module options
> +
> +        Module options
>  ---------------
>  
> -Vimc has a few module parameters to configure the driver. You should pass
> -those arguments to each subdevice, not to the vimc module. For example::
> +Vimc has a few module parameters to configure the driver.
>  
> -        vimc_subdevice.param=value
> +        param=value
>  
> -* ``vimc_scaler.sca_mult=<unsigned int>``
> +* ``sca_mult=<unsigned int>``
>  
>          Image size multiplier factor to be used to multiply both width and
>          height, so the image size will be ``sca_mult^2`` bigger than the
>          original one. Currently, only supports scaling up (the default value
>          is 3).
>  
> -* ``vimc_debayer.deb_mean_win_size=<unsigned int>``
> +* ``deb_mean_win_size=<unsigned int>``
>  
>          Window size to calculate the mean. Note: the window size needs to be an
>          odd number, as the main pixel stays in the center of the window,
> 

[PATCH v4 5/5] MAINTAINERS: Add reviewer to vimc driver

[Shuah Khan]

After practically re-writing the driver to collpase it into a monolith,
I am adding myself as a reviewer for vimc driver.

Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 7c62b45201d7..4529d257f8db 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17041,6 +17041,7 @@ F:	include/media/videobuf2-*
 
 VIMC VIRTUAL MEDIA CONTROLLER DRIVER
 M:	Helen Koike <helen.koike@collabora.com>
+R:	Shuah Khan <skhan@linuxfoundation.org>
 L:	linux-media@vger.kernel.org
 T:	git git://linuxtv.org/media_tree.git
 W:	https://linuxtv.org
-- 
2.20.1

Re: [PATCH v4 5/5] MAINTAINERS: Add reviewer to vimc driver

[Helen Koike]

On 9/6/19 11:42 PM, Shuah Khan wrote:
> After practically re-writing the driver to collpase it into a monolith,
> I am adding myself as a reviewer for vimc driver.

Thank you!

> 
> Signed-off-by: Shuah Khan <skhan@linuxfoundation.org>

Acked-by: Helen Koike <helen.koike@collabora.com>

> ---
>  MAINTAINERS | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7c62b45201d7..4529d257f8db 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17041,6 +17041,7 @@ F:	include/media/videobuf2-*
>  
>  VIMC VIRTUAL MEDIA CONTROLLER DRIVER
>  M:	Helen Koike <helen.koike@collabora.com>
> +R:	Shuah Khan <skhan@linuxfoundation.org>
>  L:	linux-media@vger.kernel.org
>  T:	git git://linuxtv.org/media_tree.git
>  W:	https://linuxtv.org
>