[PATCH v2 0/9] media: staging: rkisp1: document rkisp1-common.h

[PATCH v2 0/9] media: staging: rkisp1: document rkisp1-common.h

[Dafna Hirschfeld]

This patchset extends the documentation of rkisp1-common.h. It adds
a line description of every struct and every field, a line description
of every lock and short description of the functions, enums and macros.

Patches 1-8 are cleanups and minor fixes found during documentation.
This patchset solves the TODO item: 'Document rkisp1-common.h'

NOTE: This patchset is rebased on top of v2 of the patchset
"media: staging: rkisp1: move stats reading to irq handler"

https://patchwork.kernel.org/project/linux-media/list/?series=308787

This is because that patchset changes the lock fields in the rkisp1-stats
struct.

Changes since v2:
- 4 added cleanup patches - patches 4-8
- fixes due to comments
- added documentation to the debugfs parameters, the functions, the enum and the macros.


Dafna Hirschfeld (9):
  media: staging: rkisp1: remove unused field ctrl_handler from struct
    rkisp1_device
  media: staging: rkisp1: remove unused field alloc_ctx from struct
    rkisp1_device
  media: staging: rkisp1: set pads array of the resizer to size 2
  media: staging: rkisp1: don't define vaddr field in rkisp1_buffer as
    an array
  media: staging: rkisp1: add a pointer to rkisp1_device from struct
    rkisp1_isp
  media: staging: rkisp1: unify (un)register functions to have the same
    parameters
  media: staging: rkisp1: remove declaration of unimplemented function
    'rkisp1_params_isr_handler'
  media: staging: rkisp1: group declaration of similar functions
    together
  media: staging: rkisp1: improve documentation of rkisp1-common.h

 drivers/staging/media/rkisp1/TODO             |   1 -
 drivers/staging/media/rkisp1/rkisp1-common.h  | 272 +++++++++++++-----
 drivers/staging/media/rkisp1/rkisp1-dev.c     |  15 +-
 drivers/staging/media/rkisp1/rkisp1-isp.c     |  17 +-
 drivers/staging/media/rkisp1/rkisp1-params.c  |  14 +-
 drivers/staging/media/rkisp1/rkisp1-resizer.c |   2 +-
 drivers/staging/media/rkisp1/rkisp1-stats.c   |  14 +-
 7 files changed, 235 insertions(+), 100 deletions(-)

-- 
2.17.1

[PATCH v2 1/9] media: staging: rkisp1: remove unused field ctrl_handler from struct rkisp1_device

[Dafna Hirschfeld]

The field ctrl_handler in struct rkisp1_device is not used.
This patch removes it.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Acked-by: Helen Koike <helen.koike@collabora.com>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/staging/media/rkisp1/rkisp1-common.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 2603a5aa5210..3f0621814696 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -255,7 +255,6 @@ struct rkisp1_device {
 	unsigned int clk_size;
 	struct clk_bulk_data clks[RKISP1_MAX_BUS_CLK];
 	struct v4l2_device v4l2_dev;
-	struct v4l2_ctrl_handler ctrl_handler;
 	struct media_device media_dev;
 	struct v4l2_async_notifier notifier;
 	struct rkisp1_sensor_async *active_sensor;
-- 
2.17.1

[PATCH v2 2/9] media: staging: rkisp1: remove unused field alloc_ctx from struct rkisp1_device

[Dafna Hirschfeld]

The field alloc_ctx in struct rkisp1_device is not used.
This patch removes it.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Acked-by: Helen Koike <helen.koike@collabora.com>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/staging/media/rkisp1/rkisp1-common.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 3f0621814696..2af8ffa0de52 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -264,7 +264,6 @@ struct rkisp1_device {
 	struct rkisp1_stats stats;
 	struct rkisp1_params params;
 	struct media_pipeline pipe;
-	struct vb2_alloc_ctx *alloc_ctx;
 	struct mutex stream_lock;
 	struct rkisp1_debug debug;
 };
-- 
2.17.1

[PATCH v2 3/9] media: staging: rkisp1: set pads array of the resizer to size 2

[Dafna Hirschfeld]

Currently the 'pads' and 'pad_cfg' arrays of the rkisp1_resizer
are of size 'RKISP1_ISP_PAD_MAX' which is 4. But the resizer
has only two pads. Change the size of the arrays to 2 by adding
and using 'RKISP1_RSZ_PAD_MAX' similar to the way it is done in
the isp entity.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Acked-by: Helen Koike <helen.koike@collabora.com>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
[hverkuil: fix typos in commit log]
---
 drivers/staging/media/rkisp1/rkisp1-common.h  | 5 +++--
 drivers/staging/media/rkisp1/rkisp1-resizer.c | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 2af8ffa0de52..3dc51d703f73 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -52,6 +52,7 @@
 enum rkisp1_rsz_pad {
 	RKISP1_RSZ_PAD_SINK,
 	RKISP1_RSZ_PAD_SRC,
+	RKISP1_RSZ_PAD_MAX
 };
 
 enum rkisp1_stream_id {
@@ -219,8 +220,8 @@ struct rkisp1_resizer {
 	struct v4l2_subdev sd;
 	enum rkisp1_stream_id id;
 	struct rkisp1_device *rkisp1;
-	struct media_pad pads[RKISP1_ISP_PAD_MAX];
-	struct v4l2_subdev_pad_config pad_cfg[RKISP1_ISP_PAD_MAX];
+	struct media_pad pads[RKISP1_RSZ_PAD_MAX];
+	struct v4l2_subdev_pad_config pad_cfg[RKISP1_RSZ_PAD_MAX];
 	const struct rkisp1_rsz_config *config;
 	enum v4l2_pixel_encoding pixel_enc;
 	struct mutex ops_lock;
diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
index 31aea7dfc976..c66d2a52fd71 100644
--- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
+++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
@@ -735,7 +735,7 @@ static int rkisp1_rsz_register(struct rkisp1_resizer *rsz)
 	rsz->pixel_enc = RKISP1_DEF_PIXEL_ENC;
 
 	mutex_init(&rsz->ops_lock);
-	ret = media_entity_pads_init(&sd->entity, 2, pads);
+	ret = media_entity_pads_init(&sd->entity, RKISP1_RSZ_PAD_MAX, pads);
 	if (ret)
 		return ret;
 
-- 
2.17.1

[PATCH v2 4/9] media: staging: rkisp1: don't define vaddr field in rkisp1_buffer as an array

[Dafna Hirschfeld]

The field vaddr in rkisp1_buffer struct is used only by the
rkisp1-stats and rkisp1-params entities and they both use only
vaddr[0] so there is no need to define this field as an array.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-common.h | 2 +-
 drivers/staging/media/rkisp1/rkisp1-params.c | 4 ++--
 drivers/staging/media/rkisp1/rkisp1-stats.c  | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 3dc51d703f73..e54793d13c3d 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -127,7 +127,7 @@ struct rkisp1_buffer {
 	struct list_head queue;
 	union {
 		u32 buff_addr[VIDEO_MAX_PLANES];
-		void *vaddr[VIDEO_MAX_PLANES];
+		void *vaddr;
 	};
 };
 
diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 797e79de659c..2ab25062cde6 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1215,7 +1215,7 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis)
 	if (!cur_buf)
 		return;
 
-	new_params = (struct rkisp1_params_cfg *)(cur_buf->vaddr[0]);
+	new_params = (struct rkisp1_params_cfg *)(cur_buf->vaddr);
 
 	if (isp_mis & RKISP1_CIF_ISP_FRAME) {
 		u32 isp_ctrl;
@@ -1463,7 +1463,7 @@ static void rkisp1_params_vb2_buf_queue(struct vb2_buffer *vb)
 		return;
 	}
 
-	params_buf->vaddr[0] = vb2_plane_vaddr(vb, 0);
+	params_buf->vaddr = vb2_plane_vaddr(vb, 0);
 	spin_lock_irqsave(&params->config_lock, flags);
 	list_add_tail(&params_buf->queue, &params->params);
 	spin_unlock_irqrestore(&params->config_lock, flags);
diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
index 87e4104d20dd..a67c233b8641 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -116,7 +116,7 @@ static void rkisp1_stats_vb2_buf_queue(struct vb2_buffer *vb)
 	struct vb2_queue *vq = vb->vb2_queue;
 	struct rkisp1_stats *stats_dev = vq->drv_priv;
 
-	stats_buf->vaddr[0] = vb2_plane_vaddr(vb, 0);
+	stats_buf->vaddr = vb2_plane_vaddr(vb, 0);
 
 	spin_lock_irq(&stats_dev->lock);
 	list_add_tail(&stats_buf->queue, &stats_dev->stat);
@@ -322,7 +322,7 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats, u32 isp_ris)
 		return;
 
 	cur_stat_buf =
-		(struct rkisp1_stat_buffer *)(cur_buf->vaddr[0]);
+		(struct rkisp1_stat_buffer *)(cur_buf->vaddr);
 
 	if (isp_ris & RKISP1_CIF_ISP_AWB_DONE)
 		rkisp1_stats_get_awb_meas(stats, cur_stat_buf);
-- 
2.17.1

[PATCH v2 5/9] media: staging: rkisp1: add a pointer to rkisp1_device from struct rkisp1_isp

[Dafna Hirschfeld]

The code in rkisp1-isp.c requires access to struct 'rkisp1_device'
in several places. It access it using the 'container_of' macro.
This patch adds a pointer to 'rkisp1_device' in struct 'rkisp1_isp'
to simplify the access.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-common.h |  1 +
 drivers/staging/media/rkisp1/rkisp1-isp.c    | 12 +++++-------
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index e54793d13c3d..893caa9df891 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -106,6 +106,7 @@ struct rkisp1_sensor_async {
  */
 struct rkisp1_isp {
 	struct v4l2_subdev sd;
+	struct rkisp1_device *rkisp1;
 	struct media_pad pads[RKISP1_ISP_PAD_MAX];
 	struct v4l2_subdev_pad_config pad_cfg[RKISP1_ISP_PAD_MAX];
 	const struct rkisp1_isp_mbus_info *sink_fmt;
diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index 6ec1e9816e9f..b2131aea5488 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -836,9 +836,8 @@ static int rkisp1_isp_set_selection(struct v4l2_subdev *sd,
 				    struct v4l2_subdev_pad_config *cfg,
 				    struct v4l2_subdev_selection *sel)
 {
-	struct rkisp1_device *rkisp1 =
-		container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev);
 	struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd);
+	struct rkisp1_device *rkisp1 = isp->rkisp1;
 	int ret = 0;
 
 	if (sel->target != V4L2_SEL_TGT_CROP)
@@ -883,8 +882,7 @@ static const struct v4l2_subdev_pad_ops rkisp1_isp_pad_ops = {
 static int rkisp1_mipi_csi2_start(struct rkisp1_isp *isp,
 				  struct rkisp1_sensor_async *sensor)
 {
-	struct rkisp1_device *rkisp1 =
-		container_of(isp->sd.v4l2_dev, struct rkisp1_device, v4l2_dev);
+	struct rkisp1_device *rkisp1 = isp->rkisp1;
 	union phy_configure_opts opts;
 	struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy;
 	s64 pixel_clock;
@@ -916,9 +914,8 @@ static void rkisp1_mipi_csi2_stop(struct rkisp1_sensor_async *sensor)
 
 static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
 {
-	struct rkisp1_device *rkisp1 =
-		container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev);
-	struct rkisp1_isp *isp = &rkisp1->isp;
+	struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd);
+	struct rkisp1_device *rkisp1 = isp->rkisp1;
 	struct v4l2_subdev *sensor_sd;
 	int ret = 0;
 
@@ -997,6 +994,7 @@ int rkisp1_isp_register(struct rkisp1_device *rkisp1,
 	struct v4l2_subdev *sd = &isp->sd;
 	int ret;
 
+	isp->rkisp1 = rkisp1;
 	v4l2_subdev_init(sd, &rkisp1_isp_ops);
 	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
 	sd->entity.ops = &rkisp1_isp_media_ops;
-- 
2.17.1

[PATCH v2 6/9] media: staging: rkisp1: unify (un)register functions to have the same parameters

[Dafna Hirschfeld]

The different register/unregister functions receive
different parameters. This patch unify them so they all receive just
'struct *rkisp1_device' as parameter.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-common.h | 15 +++++----------
 drivers/staging/media/rkisp1/rkisp1-dev.c    | 15 +++++++--------
 drivers/staging/media/rkisp1/rkisp1-isp.c    |  5 ++---
 drivers/staging/media/rkisp1/rkisp1-params.c | 10 +++++-----
 drivers/staging/media/rkisp1/rkisp1-stats.c  | 10 +++++-----
 5 files changed, 24 insertions(+), 31 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 893caa9df891..fe1a775de768 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -304,8 +304,7 @@ void rkisp1_sd_adjust_crop_rect(struct v4l2_rect *crop,
 void rkisp1_sd_adjust_crop(struct v4l2_rect *crop,
 			   const struct v4l2_mbus_framefmt *bounds);
 
-int rkisp1_isp_register(struct rkisp1_device *rkisp1,
-			struct v4l2_device *v4l2_dev);
+int rkisp1_isp_register(struct rkisp1_device *rkisp1);
 void rkisp1_isp_unregister(struct rkisp1_device *rkisp1);
 
 const struct rkisp1_isp_mbus_info *rkisp1_isp_mbus_info_get(u32 mbus_code);
@@ -322,19 +321,15 @@ void rkisp1_capture_devs_unregister(struct rkisp1_device *rkisp1);
 int rkisp1_resizer_devs_register(struct rkisp1_device *rkisp1);
 void rkisp1_resizer_devs_unregister(struct rkisp1_device *rkisp1);
 
-int rkisp1_stats_register(struct rkisp1_stats *stats,
-			  struct v4l2_device *v4l2_dev,
-			  struct rkisp1_device *rkisp1);
-void rkisp1_stats_unregister(struct rkisp1_stats *stats);
+int rkisp1_stats_register(struct rkisp1_device *rkisp1);
+void rkisp1_stats_unregister(struct rkisp1_device *rkisp1);
 
 void rkisp1_params_configure(struct rkisp1_params *params,
 			     enum rkisp1_fmt_raw_pat_type bayer_pat,
 			     enum v4l2_quantization quantization);
 void rkisp1_params_disable(struct rkisp1_params *params);
-int rkisp1_params_register(struct rkisp1_params *params,
-			   struct v4l2_device *v4l2_dev,
-			   struct rkisp1_device *rkisp1);
-void rkisp1_params_unregister(struct rkisp1_params *params);
+int rkisp1_params_register(struct rkisp1_device *rkisp1);
+void rkisp1_params_unregister(struct rkisp1_device *rkisp1);
 
 void rkisp1_params_isr_handler(struct rkisp1_device *rkisp1, u32 isp_mis);
 
diff --git a/drivers/staging/media/rkisp1/rkisp1-dev.c b/drivers/staging/media/rkisp1/rkisp1-dev.c
index a0eb8f08708b..d85ac10e5494 100644
--- a/drivers/staging/media/rkisp1/rkisp1-dev.c
+++ b/drivers/staging/media/rkisp1/rkisp1-dev.c
@@ -345,7 +345,7 @@ static int rkisp1_entities_register(struct rkisp1_device *rkisp1)
 {
 	int ret;
 
-	ret = rkisp1_isp_register(rkisp1, &rkisp1->v4l2_dev);
+	ret = rkisp1_isp_register(rkisp1);
 	if (ret)
 		return ret;
 
@@ -357,12 +357,11 @@ static int rkisp1_entities_register(struct rkisp1_device *rkisp1)
 	if (ret)
 		goto err_unreg_resizer_devs;
 
-	ret = rkisp1_stats_register(&rkisp1->stats, &rkisp1->v4l2_dev, rkisp1);
+	ret = rkisp1_stats_register(rkisp1);
 	if (ret)
 		goto err_unreg_capture_devs;
 
-	ret = rkisp1_params_register(&rkisp1->params,
-				     &rkisp1->v4l2_dev, rkisp1);
+	ret = rkisp1_params_register(rkisp1);
 	if (ret)
 		goto err_unreg_stats;
 
@@ -375,9 +374,9 @@ static int rkisp1_entities_register(struct rkisp1_device *rkisp1)
 
 	return 0;
 err_unreg_params:
-	rkisp1_params_unregister(&rkisp1->params);
+	rkisp1_params_unregister(rkisp1);
 err_unreg_stats:
-	rkisp1_stats_unregister(&rkisp1->stats);
+	rkisp1_stats_unregister(rkisp1);
 err_unreg_capture_devs:
 	rkisp1_capture_devs_unregister(rkisp1);
 err_unreg_resizer_devs:
@@ -551,8 +550,8 @@ static int rkisp1_remove(struct platform_device *pdev)
 	v4l2_async_notifier_unregister(&rkisp1->notifier);
 	v4l2_async_notifier_cleanup(&rkisp1->notifier);
 
-	rkisp1_params_unregister(&rkisp1->params);
-	rkisp1_stats_unregister(&rkisp1->stats);
+	rkisp1_params_unregister(rkisp1);
+	rkisp1_stats_unregister(rkisp1);
 	rkisp1_capture_devs_unregister(rkisp1);
 	rkisp1_resizer_devs_unregister(rkisp1);
 	rkisp1_isp_unregister(rkisp1);
diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
index b2131aea5488..d7e533bb6f1d 100644
--- a/drivers/staging/media/rkisp1/rkisp1-isp.c
+++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
@@ -986,8 +986,7 @@ static const struct v4l2_subdev_ops rkisp1_isp_ops = {
 	.pad = &rkisp1_isp_pad_ops,
 };
 
-int rkisp1_isp_register(struct rkisp1_device *rkisp1,
-			struct v4l2_device *v4l2_dev)
+int rkisp1_isp_register(struct rkisp1_device *rkisp1)
 {
 	struct rkisp1_isp *isp = &rkisp1->isp;
 	struct media_pad *pads = isp->pads;
@@ -1016,7 +1015,7 @@ int rkisp1_isp_register(struct rkisp1_device *rkisp1,
 	if (ret)
 		return ret;
 
-	ret = v4l2_device_register_subdev(v4l2_dev, sd);
+	ret = v4l2_device_register_subdev(&rkisp1->v4l2_dev, sd);
 	if (ret) {
 		dev_err(rkisp1->dev, "Failed to register isp subdev\n");
 		goto err_cleanup_media_entity;
diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
index 2ab25062cde6..c3c0ab5a5713 100644
--- a/drivers/staging/media/rkisp1/rkisp1-params.c
+++ b/drivers/staging/media/rkisp1/rkisp1-params.c
@@ -1570,10 +1570,9 @@ static void rkisp1_init_params(struct rkisp1_params *params)
 		sizeof(struct rkisp1_params_cfg);
 }
 
-int rkisp1_params_register(struct rkisp1_params *params,
-			   struct v4l2_device *v4l2_dev,
-			   struct rkisp1_device *rkisp1)
+int rkisp1_params_register(struct rkisp1_device *rkisp1)
 {
+	struct rkisp1_params *params = &rkisp1->params;
 	struct rkisp1_vdev_node *node = &params->vnode;
 	struct video_device *vdev = &node->vdev;
 	int ret;
@@ -1593,7 +1592,7 @@ int rkisp1_params_register(struct rkisp1_params *params,
 	 * to protect all fops and v4l2 ioctls.
 	 */
 	vdev->lock = &node->vlock;
-	vdev->v4l2_dev = v4l2_dev;
+	vdev->v4l2_dev = &rkisp1->v4l2_dev;
 	vdev->queue = &node->buf_queue;
 	vdev->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_META_OUTPUT;
 	vdev->vfl_dir = VFL_DIR_TX;
@@ -1619,8 +1618,9 @@ int rkisp1_params_register(struct rkisp1_params *params,
 	return ret;
 }
 
-void rkisp1_params_unregister(struct rkisp1_params *params)
+void rkisp1_params_unregister(struct rkisp1_device *rkisp1)
 {
+	struct rkisp1_params *params = &rkisp1->params;
 	struct rkisp1_vdev_node *node = &params->vnode;
 	struct video_device *vdev = &node->vdev;
 
diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
index a67c233b8641..f5dbd134ee24 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -375,10 +375,9 @@ static void rkisp1_init_stats(struct rkisp1_stats *stats)
 		sizeof(struct rkisp1_stat_buffer);
 }
 
-int rkisp1_stats_register(struct rkisp1_stats *stats,
-			  struct v4l2_device *v4l2_dev,
-			  struct rkisp1_device *rkisp1)
+int rkisp1_stats_register(struct rkisp1_device *rkisp1)
 {
+	struct rkisp1_stats *stats = &rkisp1->stats;
 	struct rkisp1_vdev_node *node = &stats->vnode;
 	struct video_device *vdev = &node->vdev;
 	int ret;
@@ -395,7 +394,7 @@ int rkisp1_stats_register(struct rkisp1_stats *stats,
 	vdev->fops = &rkisp1_stats_fops;
 	vdev->release = video_device_release_empty;
 	vdev->lock = &node->vlock;
-	vdev->v4l2_dev = v4l2_dev;
+	vdev->v4l2_dev = &rkisp1->v4l2_dev;
 	vdev->queue = &node->buf_queue;
 	vdev->device_caps = V4L2_CAP_META_CAPTURE | V4L2_CAP_STREAMING;
 	vdev->vfl_dir =  VFL_DIR_RX;
@@ -425,8 +424,9 @@ int rkisp1_stats_register(struct rkisp1_stats *stats,
 	return ret;
 }
 
-void rkisp1_stats_unregister(struct rkisp1_stats *stats)
+void rkisp1_stats_unregister(struct rkisp1_device *rkisp1)
 {
+	struct rkisp1_stats *stats = &rkisp1->stats;
 	struct rkisp1_vdev_node *node = &stats->vnode;
 	struct video_device *vdev = &node->vdev;
 
-- 
2.17.1

[PATCH v2 7/9] media: staging: rkisp1: remove declaration of unimplemented function 'rkisp1_params_isr_handler'

[Dafna Hirschfeld]

The file rkisp1-common.h has a function declarion
'rkisp1_params_isr_handler' that is not implemented.
Remove it.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-common.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index fe1a775de768..d18f901de70c 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -331,6 +331,4 @@ void rkisp1_params_disable(struct rkisp1_params *params);
 int rkisp1_params_register(struct rkisp1_device *rkisp1);
 void rkisp1_params_unregister(struct rkisp1_device *rkisp1);
 
-void rkisp1_params_isr_handler(struct rkisp1_device *rkisp1, u32 isp_mis);
-
 #endif /* _RKISP1_COMMON_H */
-- 
2.17.1

[PATCH v2 8/9] media: staging: rkisp1: group declaration of similar functions together

[Dafna Hirschfeld]

In file rkisp1-common.h, group declaration of register/unregister
functions together and group other functions together to make
the code easier to read.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-common.h | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index d18f901de70c..67419cf1a739 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -304,11 +304,13 @@ void rkisp1_sd_adjust_crop_rect(struct v4l2_rect *crop,
 void rkisp1_sd_adjust_crop(struct v4l2_rect *crop,
 			   const struct v4l2_mbus_framefmt *bounds);
 
-int rkisp1_isp_register(struct rkisp1_device *rkisp1);
-void rkisp1_isp_unregister(struct rkisp1_device *rkisp1);
-
 const struct rkisp1_isp_mbus_info *rkisp1_isp_mbus_info_get(u32 mbus_code);
 
+void rkisp1_params_configure(struct rkisp1_params *params,
+			     enum rkisp1_fmt_raw_pat_type bayer_pat,
+			     enum v4l2_quantization quantization);
+void rkisp1_params_disable(struct rkisp1_params *params);
+
 void rkisp1_isp_isr(struct rkisp1_device *rkisp1);
 void rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
 void rkisp1_capture_isr(struct rkisp1_device *rkisp1);
@@ -318,16 +320,15 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis);
 int rkisp1_capture_devs_register(struct rkisp1_device *rkisp1);
 void rkisp1_capture_devs_unregister(struct rkisp1_device *rkisp1);
 
+int rkisp1_isp_register(struct rkisp1_device *rkisp1);
+void rkisp1_isp_unregister(struct rkisp1_device *rkisp1);
+
 int rkisp1_resizer_devs_register(struct rkisp1_device *rkisp1);
 void rkisp1_resizer_devs_unregister(struct rkisp1_device *rkisp1);
 
 int rkisp1_stats_register(struct rkisp1_device *rkisp1);
 void rkisp1_stats_unregister(struct rkisp1_device *rkisp1);
 
-void rkisp1_params_configure(struct rkisp1_params *params,
-			     enum rkisp1_fmt_raw_pat_type bayer_pat,
-			     enum v4l2_quantization quantization);
-void rkisp1_params_disable(struct rkisp1_params *params);
 int rkisp1_params_register(struct rkisp1_device *rkisp1);
 void rkisp1_params_unregister(struct rkisp1_device *rkisp1);
 
-- 
2.17.1

[PATCH v2 9/9] media: staging: rkisp1: improve documentation of rkisp1-common.h

[Dafna Hirschfeld]

Add more detailed documentation of the structs and functions
in rkisp1-common.h

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/TODO            |   1 -
 drivers/staging/media/rkisp1/rkisp1-common.h | 232 +++++++++++++++----
 2 files changed, 189 insertions(+), 44 deletions(-)

diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO
index bdb1b8f73556..f0c90d1c86a8 100644
--- a/drivers/staging/media/rkisp1/TODO
+++ b/drivers/staging/media/rkisp1/TODO
@@ -2,7 +2,6 @@
 * Fix checkpatch errors.
 * Review and comment every lock
 * Handle quantization
-* Document rkisp1-common.h
 * streaming paths (mainpath and selfpath) check if the other path is streaming
 in several places of the code, review this, specially that it doesn't seem it
 supports streaming from both paths at the same time.
diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 67419cf1a739..2f91630e109c 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -22,9 +22,14 @@
 #include "rkisp1-regs.h"
 #include "uapi/rkisp1-config.h"
 
+/*
+ * flags on the 'direction' field in struct 'rkisp1_isp_mbus_info' that indicate
+ * on which pad the media bus format is supported
+ */
 #define RKISP1_ISP_SD_SRC BIT(0)
 #define RKISP1_ISP_SD_SINK BIT(1)
 
+/* min and max values for the widths and heights of the entities */
 #define RKISP1_ISP_MAX_WIDTH		4032
 #define RKISP1_ISP_MAX_HEIGHT		3024
 #define RKISP1_ISP_MIN_WIDTH		32
@@ -37,29 +42,36 @@
 #define RKISP1_RSZ_SRC_MIN_WIDTH		32
 #define RKISP1_RSZ_SRC_MIN_HEIGHT		16
 
+/* the default width and height of all the entities */
 #define RKISP1_DEFAULT_WIDTH		800
 #define RKISP1_DEFAULT_HEIGHT		600
 
 #define RKISP1_DRIVER_NAME	"rkisp1"
 #define RKISP1_BUS_INFO		"platform:" RKISP1_DRIVER_NAME
 
+/* maximum number of clocks */
 #define RKISP1_MAX_BUS_CLK	8
 
+/* a bitmask of the ready stats */
 #define RKISP1_STATS_MEAS_MASK		(RKISP1_CIF_ISP_AWB_DONE |	\
 					 RKISP1_CIF_ISP_AFM_FIN |	\
 					 RKISP1_CIF_ISP_EXP_END |	\
 					 RKISP1_CIF_ISP_HIST_MEASURE_RDY)
+
+/* enum for the resizer pads */
 enum rkisp1_rsz_pad {
 	RKISP1_RSZ_PAD_SINK,
 	RKISP1_RSZ_PAD_SRC,
 	RKISP1_RSZ_PAD_MAX
 };
 
+/* enum for the capture id */
 enum rkisp1_stream_id {
 	RKISP1_MAINPATH,
 	RKISP1_SELFPATH,
 };
 
+/* bayer patterns */
 enum rkisp1_fmt_raw_pat_type {
 	RKISP1_RAW_RGGB = 0,
 	RKISP1_RAW_GRBG,
@@ -67,6 +79,7 @@ enum rkisp1_fmt_raw_pat_type {
 	RKISP1_RAW_BGGR,
 };
 
+/* enum for the isp pads */
 enum rkisp1_isp_pad {
 	RKISP1_ISP_PAD_SINK_VIDEO,
 	RKISP1_ISP_PAD_SINK_PARAMS,
@@ -76,8 +89,16 @@ enum rkisp1_isp_pad {
 };
 
 /*
- * struct rkisp1_sensor_async - Sensor information
- * @mbus: media bus configuration
+ * struct rkisp1_sensor_async - A container for the v4l2_async_subdev to add to the notifier
+ *				of the v4l2-async API
+ *
+ * @asd:		async_subdev variable for the sensor
+ * @lanes:		number of lanes
+ * @mbus_type:		type of bus (currently only CSI2 is supported)
+ * @mbus_flags:		media bus (V4L2_MBUS_*) flags
+ * @sd:			a pointer to v4l2_subdev struct of the sensor
+ * @pixel_rate_ctrl:	pixel rate of the sensor, used to initialize the phy
+ * @dphy:		a pointer to the phy
  */
 struct rkisp1_sensor_async {
 	struct v4l2_async_subdev asd;
@@ -90,19 +111,17 @@ struct rkisp1_sensor_async {
 };
 
 /*
- * struct rkisp1_isp - ISP sub-device
- *
- * See Cropping regions of ISP in rkisp1.c for details
- * @sink_frm: input size, don't have to be equal to sensor size
- * @sink_fmt: input format
- * @sink_crop: crop for sink pad
- * @src_fmt: output format
- * @src_crop: output size
- * @ops_lock: ops serialization
+ * struct rkisp1_isp - ISP subdev entity
  *
- * @is_dphy_errctrl_disabled : if dphy errctrl is disabled (avoid endless interrupt)
- * @frame_sequence: used to synchronize frame_id between video devices.
- * @quantization: output quantization
+ * @sd:				v4l2_subdev variable
+ * @rkisp1:			pointer to rkisp1_device
+ * @pads:			media pads
+ * @pad_cfg:			pads configurations
+ * @sink_fmt:			input format
+ * @src_fmt:			output format
+ * @ops_lock:			ops serialization
+ * @is_dphy_errctrl_disabled:	if dphy errctrl is disabled (avoid endless interrupt)
+ * @frame_sequence:		used to synchronize frame_id between video devices.
  */
 struct rkisp1_isp {
 	struct v4l2_subdev sd;
@@ -111,11 +130,19 @@ struct rkisp1_isp {
 	struct v4l2_subdev_pad_config pad_cfg[RKISP1_ISP_PAD_MAX];
 	const struct rkisp1_isp_mbus_info *sink_fmt;
 	const struct rkisp1_isp_mbus_info *src_fmt;
-	struct mutex ops_lock;
+	struct mutex ops_lock; /* serialize the subdevice ops */
 	bool is_dphy_errctrl_disabled;
 	atomic_t frame_sequence;
 };
 
+/*
+ * struct rkisp1_vdev_node - Container for the video nodes: params, stats, mainpath, selfpath
+ *
+ * @buf_queue:	queue of buffers
+ * @vlock:	lock of the video node
+ * @vdev:	video node
+ * @pad:	media pad
+ */
 struct rkisp1_vdev_node {
 	struct vb2_queue buf_queue;
 	struct mutex vlock; /* ioctl serialization mutex */
@@ -123,6 +150,15 @@ struct rkisp1_vdev_node {
 	struct media_pad pad;
 };
 
+/*
+ * struct rkisp1_buffer - A container for the vb2 buffers used by the video devices:
+ *			  params, stats, mainpath, selfpath
+ *
+ * @vb:		vb2 buffer
+ * @queue:	entry of the buffer in the queue
+ * @buff_addr:	dma addresses of each plane, used only by the capture devices: selfpath, mainpath
+ * @vaddr:	virtual address for buffers used by params and stats devices
+ */
 struct rkisp1_buffer {
 	struct vb2_v4l2_buffer vb;
 	struct list_head queue;
@@ -132,6 +168,14 @@ struct rkisp1_buffer {
 	};
 };
 
+/*
+ * struct rkisp1_dummy_buffer - A buffer to write the next frame to in case
+ *				there are no vb2 buffers available.
+ *
+ * @vaddr:	return value of call to dma_alloc_attrs.
+ * @dma_addr:	dma address of the buffer.
+ * @size:	size of the buffer.
+ */
 struct rkisp1_dummy_buffer {
 	void *vaddr;
 	dma_addr_t dma_addr;
@@ -143,17 +187,29 @@ struct rkisp1_device;
 /*
  * struct rkisp1_capture - ISP capture video device
  *
- * @pix.fmt: buffer format
- * @pix.info: pixel information
- * @pix.cfg: pixel configuration
- *
- * @buf.lock: lock to protect buf_queue
- * @buf.queue: queued buffer list
- * @buf.dummy: dummy space to store dropped data
+ * @vnode:	  video node
+ * @rkisp1:	  pointer to rkisp1_device
+ * @id:		  id of the capture, one of RKISP1_SELFPATH, RKISP1_MAINPATH
+ * @ops:	  list of callbacks to configure the capture device.
+ * @config:	  a pointer to the list of registers to configure the capture format.
+ * @is_streaming: device is streaming
+ * @is_stopping:  stop_streaming callback was called and the device is in the process of
+ *		  stopping the streaming.
+ * @done:	  when stop_streaming callback is called, the device waits for the next irq
+ *		  handler to stop the streaming by waiting on the 'done' wait queue.
+ *		  If the irq handler is not called, the stream is stopped by the callback
+ *		  after timeout.
+ * @sp_y_stride:  the selfpath allows to configure a y stride that is longer than the image width.
+ * @buf.lock:	  lock to protect buf.queue
+ * @buf.queue:	  queued buffer list
+ * @buf.dummy:	  dummy space to store dropped data
  *
- * rkisp1 use shadowsock registers, so it need two buffer at a time
- * @buf.curr: the buffer used for current frame
- * @buf.next: the buffer used for next frame
+ * rkisp1 uses shadow registers, so it needs two buffers at a time
+ * @buf.curr:	  the buffer used for current frame
+ * @buf.next:	  the buffer used for next frame
+ * @pix.cfg:	  pixel configuration
+ * @pix.info:	  a pointer to the v4l2_format_info of the pixel format
+ * @pix.fmt:	  buffer format
  */
 struct rkisp1_capture {
 	struct rkisp1_vdev_node vnode;
@@ -183,14 +239,18 @@ struct rkisp1_capture {
 /*
  * struct rkisp1_stats - ISP Statistics device
  *
- * @lock: locks the buffer list 'stat' and 'is_streaming'
- * @stat: stats buffer list
+ * @vnode:	  video node
+ * @rkisp1:	  pointer to the rkisp1 device
+ * @lock:	  locks the buffer list 'stat' and 'is_streaming'
+ * @stat:	  queue of rkisp1_buffer
+ * @vdev_fmt:	  v4l2_format of the metadata format
+ * @is_streaming: device is streaming
  */
 struct rkisp1_stats {
 	struct rkisp1_vdev_node vnode;
 	struct rkisp1_device *rkisp1;
 
-	spinlock_t lock; /* locks 'is_streaming', and 'stats' */
+	spinlock_t lock; /* locks the buffers list 'stats' and 'is_streaming' */
 	struct list_head stat;
 	struct v4l2_format vdev_fmt;
 	bool is_streaming;
@@ -199,14 +259,22 @@ struct rkisp1_stats {
 /*
  * struct rkisp1_params - ISP input parameters device
  *
- * @cur_params: Current ISP parameters
- * @is_first_params: the first params should take effect immediately
+ * @vnode:		video node
+ * @rkisp1:		pointer to the rkisp1 device
+ * @config_lock:	locks the buffer list 'params' and 'is_streaming'
+ * @params:		queue of rkisp1_buffer
+ * @cur_params:		the first params values from userspace
+ * @vdev_fmt:		v4l2_format of the metadata format
+ * @is_streaming:	device is streaming
+ * @is_first_params:	the first params should take effect immediately
+ * @quantization:	the quantization configured on the isp's src pad
+ * @raw_type:		the bayer pattern on the isp video sink pad
  */
 struct rkisp1_params {
 	struct rkisp1_vdev_node vnode;
 	struct rkisp1_device *rkisp1;
 
-	spinlock_t config_lock;
+	spinlock_t config_lock; /* locks the buffers list 'params' and 'is_streaming' */
 	struct list_head params;
 	struct rkisp1_params_cfg cur_params;
 	struct v4l2_format vdev_fmt;
@@ -217,6 +285,18 @@ struct rkisp1_params {
 	enum rkisp1_fmt_raw_pat_type raw_type;
 };
 
+/*
+ * struct rkisp1_resizer - Resizer subdev
+ *
+ * @sd:	       v4l2_subdev variable
+ * @id:	       id of the resizer, one of RKISP1_SELFPATH, RKISP1_MAINPATH
+ * @rkisp1:    pointer to the rkisp1 device
+ * @pads:      media pads
+ * @pad_cfg:   configurations for the pads
+ * @config:    the set of registers to configure the resizer
+ * @pixel_enc: pixel encoding of the resizer
+ * @ops_lock:  a lock for the subdev ops
+ */
 struct rkisp1_resizer {
 	struct v4l2_subdev sd;
 	enum rkisp1_stream_id id;
@@ -225,9 +305,26 @@ struct rkisp1_resizer {
 	struct v4l2_subdev_pad_config pad_cfg[RKISP1_RSZ_PAD_MAX];
 	const struct rkisp1_rsz_config *config;
 	enum v4l2_pixel_encoding pixel_enc;
-	struct mutex ops_lock;
+	struct mutex ops_lock; /* serialize the subdevice ops */
 };
 
+/*
+ * struct rkisp1_debug - Values to be exposed on debugfs.
+ *			 The parameters are counters of the number of times the
+ *			 event occurred since the driver was loaded.
+ *
+ * @data_loss:			  loss of data occurred within a line, processing failure
+ * @outform_size_error:		  size error is generated in outmux submodule
+ * @img_stabilization_size_error: size error is generated in image stabilization submodule
+ * @inform_size_err:		  size error is generated in inform submodule
+ * @mipi_error:			  mipi error occurred
+ * @stats_error:		  writing to the 'Interrupt clear register' did not clear
+ *				  it in the register 'Masked interrupt status'
+ * @stop_timeout:		  upon stream stop, the capture waits 1 second for the isr to stop
+ *				  the stream. This param is incremented in case of timeout.
+ * @frame_drop:			  a frame was ready but the buffer queue was empty so the frame
+ *				  was not sent to userspace
+ */
 struct rkisp1_debug {
 	struct dentry *debugfs_dir;
 	unsigned long data_loss;
@@ -242,13 +339,24 @@ struct rkisp1_debug {
 
 /*
  * struct rkisp1_device - ISP platform device
- * @base_addr: base register address
+ *
+ * @base_addr:	   base register address
+ * @irq:	   the irq number
+ * @dev:	   a pointer to the struct device
+ * @clk_size:	   number of clocks
+ * @clks:	   array of clocks
+ * @v4l2_dev:	   v4l2_device variable
+ * @media_dev:	   media_device variable
+ * @notifier:	   a notifier to register on the v4l2-async API to be notified on the sensor
  * @active_sensor: sensor in-use, set when streaming on
- * @isp: ISP sub-device
- * @rkisp1_capture: capture video device
- * @stats: ISP statistics output device
- * @params: ISP input parameters device
- * @stream_lock: lock to serialize start/stop streaming in capture devices.
+ * @isp:	   ISP sub-device
+ * @resizer_devs:  resizer sub-devices
+ * @capture_devs:  capture devices
+ * @stats:	   ISP statistics metadata capture device
+ * @params:	   ISP parameters metadata output device
+ * @pipe:	   media pipeline
+ * @stream_lock:   serializes {start/stop}_streaming callbacks between the capture devices.
+ * @debug:	   debug params to be exposed on debugfs
  */
 struct rkisp1_device {
 	void __iomem *base_addr;
@@ -266,16 +374,20 @@ struct rkisp1_device {
 	struct rkisp1_stats stats;
 	struct rkisp1_params params;
 	struct media_pipeline pipe;
-	struct mutex stream_lock;
+	struct mutex stream_lock; /* serialize {start/stop}_streaming cb between capture devices */
 	struct rkisp1_debug debug;
 };
 
 /*
- * struct rkisp1_isp_mbus_info - ISP pad format info
+ * struct rkisp1_isp_mbus_info - ISP media bus info, Translates media bus code to hardware
+ *				 format values
  *
- * Translate mbus_code to hardware format values
- *
- * @bus_width: used for parallel
+ * @mbus_code: media bus code
+ * @pixel_enc: pixel encoding
+ * @mipi_dt:   mipi data type
+ * @bus_width: bus width
+ * @bayer_pat: bayer pattern
+ * @direction: a bitmask of the flags indicating on which pad the format is supported on
  */
 struct rkisp1_isp_mbus_info {
 	u32 mbus_code;
@@ -298,25 +410,59 @@ static inline u32 rkisp1_read(struct rkisp1_device *rkisp1, unsigned int addr)
 	return readl(rkisp1->base_addr + addr);
 }
 
+/*
+ * rkisp1_sd_adjust_crop_rect - adjust a rectangle to fit into another rectangle.
+ *
+ * @crop:   rectangle to adjust.
+ * @bounds: rectangle used as bounds.
+ */
 void rkisp1_sd_adjust_crop_rect(struct v4l2_rect *crop,
 				const struct v4l2_rect *bounds);
 
+/*
+ * rkisp1_sd_adjust_crop - adjust a rectangle to fit into media bus format
+ *
+ * @crop:   rectangle to adjust.
+ * @bounds: media bus format used as bounds.
+ */
 void rkisp1_sd_adjust_crop(struct v4l2_rect *crop,
 			   const struct v4l2_mbus_framefmt *bounds);
 
+/*
+ * rkisp1_isp_mbus_info - get the isp info of the media bus code
+ *
+ * @mbus_code: the media bus code
+ */
 const struct rkisp1_isp_mbus_info *rkisp1_isp_mbus_info_get(u32 mbus_code);
 
+/* rkisp1_params_configure - configure the params when stream starts.
+ *			     This function is called by the isp entity upon stream starts.
+ *			     The function applies the initial configuration of the parameters.
+ *
+ * @params:	  pointer to rkisp1_params.
+ * @bayer_pat:	  the bayer pattern on the isp video sink pad
+ * @quantization: the quantization configured on the isp's src pad
+ */
 void rkisp1_params_configure(struct rkisp1_params *params,
 			     enum rkisp1_fmt_raw_pat_type bayer_pat,
 			     enum v4l2_quantization quantization);
+
+/* rkisp1_params_disable - disable all parameters.
+ *			   This function is called by the isp entity upon stream start
+ *			   when capturing bayer format.
+ *
+ * @params: pointer to rkisp1_params.
+ */
 void rkisp1_params_disable(struct rkisp1_params *params);
 
+/* irq handlers */
 void rkisp1_isp_isr(struct rkisp1_device *rkisp1);
 void rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
 void rkisp1_capture_isr(struct rkisp1_device *rkisp1);
 void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris);
 void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis);
 
+/* register/unregisters functions of the entities */
 int rkisp1_capture_devs_register(struct rkisp1_device *rkisp1);
 void rkisp1_capture_devs_unregister(struct rkisp1_device *rkisp1);
 
-- 
2.17.1

Re: [PATCH v2 4/9] media: staging: rkisp1: don't define vaddr field in rkisp1_buffer as an array

[Helen Koike]

On 7/18/20 11:59 AM, Dafna Hirschfeld wrote:
> The field vaddr in rkisp1_buffer struct is used only by the
> rkisp1-stats and rkisp1-params entities and they both use only
> vaddr[0] so there is no need to define this field as an array.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

> ---
>  drivers/staging/media/rkisp1/rkisp1-common.h | 2 +-
>  drivers/staging/media/rkisp1/rkisp1-params.c | 4 ++--
>  drivers/staging/media/rkisp1/rkisp1-stats.c  | 4 ++--
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 3dc51d703f73..e54793d13c3d 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -127,7 +127,7 @@ struct rkisp1_buffer {
>  	struct list_head queue;
>  	union {
>  		u32 buff_addr[VIDEO_MAX_PLANES];
> -		void *vaddr[VIDEO_MAX_PLANES];
> +		void *vaddr;
>  	};
>  };
>  
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 797e79de659c..2ab25062cde6 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1215,7 +1215,7 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis)
>  	if (!cur_buf)
>  		return;
>  
> -	new_params = (struct rkisp1_params_cfg *)(cur_buf->vaddr[0]);
> +	new_params = (struct rkisp1_params_cfg *)(cur_buf->vaddr);
>  
>  	if (isp_mis & RKISP1_CIF_ISP_FRAME) {
>  		u32 isp_ctrl;
> @@ -1463,7 +1463,7 @@ static void rkisp1_params_vb2_buf_queue(struct vb2_buffer *vb)
>  		return;
>  	}
>  
> -	params_buf->vaddr[0] = vb2_plane_vaddr(vb, 0);
> +	params_buf->vaddr = vb2_plane_vaddr(vb, 0);
>  	spin_lock_irqsave(&params->config_lock, flags);
>  	list_add_tail(&params_buf->queue, &params->params);
>  	spin_unlock_irqrestore(&params->config_lock, flags);
> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> index 87e4104d20dd..a67c233b8641 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -116,7 +116,7 @@ static void rkisp1_stats_vb2_buf_queue(struct vb2_buffer *vb)
>  	struct vb2_queue *vq = vb->vb2_queue;
>  	struct rkisp1_stats *stats_dev = vq->drv_priv;
>  
> -	stats_buf->vaddr[0] = vb2_plane_vaddr(vb, 0);
> +	stats_buf->vaddr = vb2_plane_vaddr(vb, 0);
>  
>  	spin_lock_irq(&stats_dev->lock);
>  	list_add_tail(&stats_buf->queue, &stats_dev->stat);
> @@ -322,7 +322,7 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats, u32 isp_ris)
>  		return;
>  
>  	cur_stat_buf =
> -		(struct rkisp1_stat_buffer *)(cur_buf->vaddr[0]);
> +		(struct rkisp1_stat_buffer *)(cur_buf->vaddr);
>  
>  	if (isp_ris & RKISP1_CIF_ISP_AWB_DONE)
>  		rkisp1_stats_get_awb_meas(stats, cur_stat_buf);
> 

Re: [PATCH v2 5/9] media: staging: rkisp1: add a pointer to rkisp1_device from struct rkisp1_isp

[Helen Koike]

On 7/18/20 11:59 AM, Dafna Hirschfeld wrote:
> The code in rkisp1-isp.c requires access to struct 'rkisp1_device'
> in several places. It access it using the 'container_of' macro.
> This patch adds a pointer to 'rkisp1_device' in struct 'rkisp1_isp'
> to simplify the access.

What is wrong with container_of?

I usually prefer moving to container_of then the other way around, since this avoid a new field
in the struct, and also avoid the requirements of keeping the pointer in sync.

Thanks
Helen

> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-common.h |  1 +
>  drivers/staging/media/rkisp1/rkisp1-isp.c    | 12 +++++-------
>  2 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index e54793d13c3d..893caa9df891 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -106,6 +106,7 @@ struct rkisp1_sensor_async {
>   */
>  struct rkisp1_isp {
>  	struct v4l2_subdev sd;
> +	struct rkisp1_device *rkisp1;
>  	struct media_pad pads[RKISP1_ISP_PAD_MAX];
>  	struct v4l2_subdev_pad_config pad_cfg[RKISP1_ISP_PAD_MAX];
>  	const struct rkisp1_isp_mbus_info *sink_fmt;
> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index 6ec1e9816e9f..b2131aea5488 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -836,9 +836,8 @@ static int rkisp1_isp_set_selection(struct v4l2_subdev *sd,
>  				    struct v4l2_subdev_pad_config *cfg,
>  				    struct v4l2_subdev_selection *sel)
>  {
> -	struct rkisp1_device *rkisp1 =
> -		container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev);
>  	struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd);
> +	struct rkisp1_device *rkisp1 = isp->rkisp1;
>  	int ret = 0;
>  
>  	if (sel->target != V4L2_SEL_TGT_CROP)
> @@ -883,8 +882,7 @@ static const struct v4l2_subdev_pad_ops rkisp1_isp_pad_ops = {
>  static int rkisp1_mipi_csi2_start(struct rkisp1_isp *isp,
>  				  struct rkisp1_sensor_async *sensor)
>  {
> -	struct rkisp1_device *rkisp1 =
> -		container_of(isp->sd.v4l2_dev, struct rkisp1_device, v4l2_dev);
> +	struct rkisp1_device *rkisp1 = isp->rkisp1;
>  	union phy_configure_opts opts;
>  	struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy;
>  	s64 pixel_clock;
> @@ -916,9 +914,8 @@ static void rkisp1_mipi_csi2_stop(struct rkisp1_sensor_async *sensor)
>  
>  static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
>  {
> -	struct rkisp1_device *rkisp1 =
> -		container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev);
> -	struct rkisp1_isp *isp = &rkisp1->isp;
> +	struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd);
> +	struct rkisp1_device *rkisp1 = isp->rkisp1;
>  	struct v4l2_subdev *sensor_sd;
>  	int ret = 0;
>  
> @@ -997,6 +994,7 @@ int rkisp1_isp_register(struct rkisp1_device *rkisp1,
>  	struct v4l2_subdev *sd = &isp->sd;
>  	int ret;
>  
> +	isp->rkisp1 = rkisp1;
>  	v4l2_subdev_init(sd, &rkisp1_isp_ops);
>  	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
>  	sd->entity.ops = &rkisp1_isp_media_ops;
> 

Re: [PATCH v2 6/9] media: staging: rkisp1: unify (un)register functions to have the same parameters

[Helen Koike]

On 7/18/20 11:59 AM, Dafna Hirschfeld wrote:
> The different register/unregister functions receive
> different parameters. This patch unify them so they all receive just
> 'struct *rkisp1_device' as parameter.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

> ---
>  drivers/staging/media/rkisp1/rkisp1-common.h | 15 +++++----------
>  drivers/staging/media/rkisp1/rkisp1-dev.c    | 15 +++++++--------
>  drivers/staging/media/rkisp1/rkisp1-isp.c    |  5 ++---
>  drivers/staging/media/rkisp1/rkisp1-params.c | 10 +++++-----
>  drivers/staging/media/rkisp1/rkisp1-stats.c  | 10 +++++-----
>  5 files changed, 24 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 893caa9df891..fe1a775de768 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -304,8 +304,7 @@ void rkisp1_sd_adjust_crop_rect(struct v4l2_rect *crop,
>  void rkisp1_sd_adjust_crop(struct v4l2_rect *crop,
>  			   const struct v4l2_mbus_framefmt *bounds);
>  
> -int rkisp1_isp_register(struct rkisp1_device *rkisp1,
> -			struct v4l2_device *v4l2_dev);
> +int rkisp1_isp_register(struct rkisp1_device *rkisp1);
>  void rkisp1_isp_unregister(struct rkisp1_device *rkisp1);
>  
>  const struct rkisp1_isp_mbus_info *rkisp1_isp_mbus_info_get(u32 mbus_code);
> @@ -322,19 +321,15 @@ void rkisp1_capture_devs_unregister(struct rkisp1_device *rkisp1);
>  int rkisp1_resizer_devs_register(struct rkisp1_device *rkisp1);
>  void rkisp1_resizer_devs_unregister(struct rkisp1_device *rkisp1);
>  
> -int rkisp1_stats_register(struct rkisp1_stats *stats,
> -			  struct v4l2_device *v4l2_dev,
> -			  struct rkisp1_device *rkisp1);
> -void rkisp1_stats_unregister(struct rkisp1_stats *stats);
> +int rkisp1_stats_register(struct rkisp1_device *rkisp1);
> +void rkisp1_stats_unregister(struct rkisp1_device *rkisp1);
>  
>  void rkisp1_params_configure(struct rkisp1_params *params,
>  			     enum rkisp1_fmt_raw_pat_type bayer_pat,
>  			     enum v4l2_quantization quantization);
>  void rkisp1_params_disable(struct rkisp1_params *params);
> -int rkisp1_params_register(struct rkisp1_params *params,
> -			   struct v4l2_device *v4l2_dev,
> -			   struct rkisp1_device *rkisp1);
> -void rkisp1_params_unregister(struct rkisp1_params *params);
> +int rkisp1_params_register(struct rkisp1_device *rkisp1);
> +void rkisp1_params_unregister(struct rkisp1_device *rkisp1);
>  
>  void rkisp1_params_isr_handler(struct rkisp1_device *rkisp1, u32 isp_mis);
>  
> diff --git a/drivers/staging/media/rkisp1/rkisp1-dev.c b/drivers/staging/media/rkisp1/rkisp1-dev.c
> index a0eb8f08708b..d85ac10e5494 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-dev.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-dev.c
> @@ -345,7 +345,7 @@ static int rkisp1_entities_register(struct rkisp1_device *rkisp1)
>  {
>  	int ret;
>  
> -	ret = rkisp1_isp_register(rkisp1, &rkisp1->v4l2_dev);
> +	ret = rkisp1_isp_register(rkisp1);
>  	if (ret)
>  		return ret;
>  
> @@ -357,12 +357,11 @@ static int rkisp1_entities_register(struct rkisp1_device *rkisp1)
>  	if (ret)
>  		goto err_unreg_resizer_devs;
>  
> -	ret = rkisp1_stats_register(&rkisp1->stats, &rkisp1->v4l2_dev, rkisp1);
> +	ret = rkisp1_stats_register(rkisp1);
>  	if (ret)
>  		goto err_unreg_capture_devs;
>  
> -	ret = rkisp1_params_register(&rkisp1->params,
> -				     &rkisp1->v4l2_dev, rkisp1);
> +	ret = rkisp1_params_register(rkisp1);
>  	if (ret)
>  		goto err_unreg_stats;
>  
> @@ -375,9 +374,9 @@ static int rkisp1_entities_register(struct rkisp1_device *rkisp1)
>  
>  	return 0;
>  err_unreg_params:
> -	rkisp1_params_unregister(&rkisp1->params);
> +	rkisp1_params_unregister(rkisp1);
>  err_unreg_stats:
> -	rkisp1_stats_unregister(&rkisp1->stats);
> +	rkisp1_stats_unregister(rkisp1);
>  err_unreg_capture_devs:
>  	rkisp1_capture_devs_unregister(rkisp1);
>  err_unreg_resizer_devs:
> @@ -551,8 +550,8 @@ static int rkisp1_remove(struct platform_device *pdev)
>  	v4l2_async_notifier_unregister(&rkisp1->notifier);
>  	v4l2_async_notifier_cleanup(&rkisp1->notifier);
>  
> -	rkisp1_params_unregister(&rkisp1->params);
> -	rkisp1_stats_unregister(&rkisp1->stats);
> +	rkisp1_params_unregister(rkisp1);
> +	rkisp1_stats_unregister(rkisp1);
>  	rkisp1_capture_devs_unregister(rkisp1);
>  	rkisp1_resizer_devs_unregister(rkisp1);
>  	rkisp1_isp_unregister(rkisp1);
> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
> index b2131aea5488..d7e533bb6f1d 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
> @@ -986,8 +986,7 @@ static const struct v4l2_subdev_ops rkisp1_isp_ops = {
>  	.pad = &rkisp1_isp_pad_ops,
>  };
>  
> -int rkisp1_isp_register(struct rkisp1_device *rkisp1,
> -			struct v4l2_device *v4l2_dev)
> +int rkisp1_isp_register(struct rkisp1_device *rkisp1)
>  {
>  	struct rkisp1_isp *isp = &rkisp1->isp;
>  	struct media_pad *pads = isp->pads;
> @@ -1016,7 +1015,7 @@ int rkisp1_isp_register(struct rkisp1_device *rkisp1,
>  	if (ret)
>  		return ret;
>  
> -	ret = v4l2_device_register_subdev(v4l2_dev, sd);
> +	ret = v4l2_device_register_subdev(&rkisp1->v4l2_dev, sd);
>  	if (ret) {
>  		dev_err(rkisp1->dev, "Failed to register isp subdev\n");
>  		goto err_cleanup_media_entity;
> diff --git a/drivers/staging/media/rkisp1/rkisp1-params.c b/drivers/staging/media/rkisp1/rkisp1-params.c
> index 2ab25062cde6..c3c0ab5a5713 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-params.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-params.c
> @@ -1570,10 +1570,9 @@ static void rkisp1_init_params(struct rkisp1_params *params)
>  		sizeof(struct rkisp1_params_cfg);
>  }
>  
> -int rkisp1_params_register(struct rkisp1_params *params,
> -			   struct v4l2_device *v4l2_dev,
> -			   struct rkisp1_device *rkisp1)
> +int rkisp1_params_register(struct rkisp1_device *rkisp1)
>  {
> +	struct rkisp1_params *params = &rkisp1->params;
>  	struct rkisp1_vdev_node *node = &params->vnode;
>  	struct video_device *vdev = &node->vdev;
>  	int ret;
> @@ -1593,7 +1592,7 @@ int rkisp1_params_register(struct rkisp1_params *params,
>  	 * to protect all fops and v4l2 ioctls.
>  	 */
>  	vdev->lock = &node->vlock;
> -	vdev->v4l2_dev = v4l2_dev;
> +	vdev->v4l2_dev = &rkisp1->v4l2_dev;
>  	vdev->queue = &node->buf_queue;
>  	vdev->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_META_OUTPUT;
>  	vdev->vfl_dir = VFL_DIR_TX;
> @@ -1619,8 +1618,9 @@ int rkisp1_params_register(struct rkisp1_params *params,
>  	return ret;
>  }
>  
> -void rkisp1_params_unregister(struct rkisp1_params *params)
> +void rkisp1_params_unregister(struct rkisp1_device *rkisp1)
>  {
> +	struct rkisp1_params *params = &rkisp1->params;
>  	struct rkisp1_vdev_node *node = &params->vnode;
>  	struct video_device *vdev = &node->vdev;
>  
> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> index a67c233b8641..f5dbd134ee24 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -375,10 +375,9 @@ static void rkisp1_init_stats(struct rkisp1_stats *stats)
>  		sizeof(struct rkisp1_stat_buffer);
>  }
>  
> -int rkisp1_stats_register(struct rkisp1_stats *stats,
> -			  struct v4l2_device *v4l2_dev,
> -			  struct rkisp1_device *rkisp1)
> +int rkisp1_stats_register(struct rkisp1_device *rkisp1)
>  {
> +	struct rkisp1_stats *stats = &rkisp1->stats;
>  	struct rkisp1_vdev_node *node = &stats->vnode;
>  	struct video_device *vdev = &node->vdev;
>  	int ret;
> @@ -395,7 +394,7 @@ int rkisp1_stats_register(struct rkisp1_stats *stats,
>  	vdev->fops = &rkisp1_stats_fops;
>  	vdev->release = video_device_release_empty;
>  	vdev->lock = &node->vlock;
> -	vdev->v4l2_dev = v4l2_dev;
> +	vdev->v4l2_dev = &rkisp1->v4l2_dev;
>  	vdev->queue = &node->buf_queue;
>  	vdev->device_caps = V4L2_CAP_META_CAPTURE | V4L2_CAP_STREAMING;
>  	vdev->vfl_dir =  VFL_DIR_RX;
> @@ -425,8 +424,9 @@ int rkisp1_stats_register(struct rkisp1_stats *stats,
>  	return ret;
>  }
>  
> -void rkisp1_stats_unregister(struct rkisp1_stats *stats)
> +void rkisp1_stats_unregister(struct rkisp1_device *rkisp1)
>  {
> +	struct rkisp1_stats *stats = &rkisp1->stats;
>  	struct rkisp1_vdev_node *node = &stats->vnode;
>  	struct video_device *vdev = &node->vdev;
>  
> 

Re: [PATCH v2 7/9] media: staging: rkisp1: remove declaration of unimplemented function 'rkisp1_params_isr_handler'

[Helen Koike]

On 7/18/20 11:59 AM, Dafna Hirschfeld wrote:
> The file rkisp1-common.h has a function declarion
> 'rkisp1_params_isr_handler' that is not implemented.
> Remove it.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

> ---
>  drivers/staging/media/rkisp1/rkisp1-common.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index fe1a775de768..d18f901de70c 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -331,6 +331,4 @@ void rkisp1_params_disable(struct rkisp1_params *params);
>  int rkisp1_params_register(struct rkisp1_device *rkisp1);
>  void rkisp1_params_unregister(struct rkisp1_device *rkisp1);
>  
> -void rkisp1_params_isr_handler(struct rkisp1_device *rkisp1, u32 isp_mis);
> -
>  #endif /* _RKISP1_COMMON_H */
> 

Re: [PATCH v2 8/9] media: staging: rkisp1: group declaration of similar functions together

[Helen Koike]

On 7/18/20 11:59 AM, Dafna Hirschfeld wrote:
> In file rkisp1-common.h, group declaration of register/unregister
> functions together and group other functions together to make
> the code easier to read.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

> ---
>  drivers/staging/media/rkisp1/rkisp1-common.h | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index d18f901de70c..67419cf1a739 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -304,11 +304,13 @@ void rkisp1_sd_adjust_crop_rect(struct v4l2_rect *crop,
>  void rkisp1_sd_adjust_crop(struct v4l2_rect *crop,
>  			   const struct v4l2_mbus_framefmt *bounds);
>  
> -int rkisp1_isp_register(struct rkisp1_device *rkisp1);
> -void rkisp1_isp_unregister(struct rkisp1_device *rkisp1);
> -
>  const struct rkisp1_isp_mbus_info *rkisp1_isp_mbus_info_get(u32 mbus_code);
>  
> +void rkisp1_params_configure(struct rkisp1_params *params,
> +			     enum rkisp1_fmt_raw_pat_type bayer_pat,
> +			     enum v4l2_quantization quantization);
> +void rkisp1_params_disable(struct rkisp1_params *params);
> +
>  void rkisp1_isp_isr(struct rkisp1_device *rkisp1);
>  void rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
>  void rkisp1_capture_isr(struct rkisp1_device *rkisp1);
> @@ -318,16 +320,15 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis);
>  int rkisp1_capture_devs_register(struct rkisp1_device *rkisp1);
>  void rkisp1_capture_devs_unregister(struct rkisp1_device *rkisp1);
>  
> +int rkisp1_isp_register(struct rkisp1_device *rkisp1);
> +void rkisp1_isp_unregister(struct rkisp1_device *rkisp1);
> +
>  int rkisp1_resizer_devs_register(struct rkisp1_device *rkisp1);
>  void rkisp1_resizer_devs_unregister(struct rkisp1_device *rkisp1);
>  
>  int rkisp1_stats_register(struct rkisp1_device *rkisp1);
>  void rkisp1_stats_unregister(struct rkisp1_device *rkisp1);
>  
> -void rkisp1_params_configure(struct rkisp1_params *params,
> -			     enum rkisp1_fmt_raw_pat_type bayer_pat,
> -			     enum v4l2_quantization quantization);
> -void rkisp1_params_disable(struct rkisp1_params *params);
>  int rkisp1_params_register(struct rkisp1_device *rkisp1);
>  void rkisp1_params_unregister(struct rkisp1_device *rkisp1);
>  
> 

Re: [PATCH v2 9/9] media: staging: rkisp1: improve documentation of rkisp1-common.h

[Helen Koike]

Hi Dafna,

Thanks for improving this doc!

On 7/18/20 11:59 AM, Dafna Hirschfeld wrote:
> Add more detailed documentation of the structs and functions
> in rkisp1-common.h
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/TODO            |   1 -
>  drivers/staging/media/rkisp1/rkisp1-common.h | 232 +++++++++++++++----
>  2 files changed, 189 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/TODO b/drivers/staging/media/rkisp1/TODO
> index bdb1b8f73556..f0c90d1c86a8 100644
> --- a/drivers/staging/media/rkisp1/TODO
> +++ b/drivers/staging/media/rkisp1/TODO
> @@ -2,7 +2,6 @@
>  * Fix checkpatch errors.
>  * Review and comment every lock
>  * Handle quantization
> -* Document rkisp1-common.h
>  * streaming paths (mainpath and selfpath) check if the other path is streaming
>  in several places of the code, review this, specially that it doesn't seem it
>  supports streaming from both paths at the same time.
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 67419cf1a739..2f91630e109c 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -22,9 +22,14 @@
>  #include "rkisp1-regs.h"
>  #include "uapi/rkisp1-config.h"
>  
> +/*
> + * flags on the 'direction' field in struct 'rkisp1_isp_mbus_info' that indicate
> + * on which pad the media bus format is supported
> + */
>  #define RKISP1_ISP_SD_SRC BIT(0)
>  #define RKISP1_ISP_SD_SINK BIT(1)
>  
> +/* min and max values for the widths and heights of the entities */
>  #define RKISP1_ISP_MAX_WIDTH		4032
>  #define RKISP1_ISP_MAX_HEIGHT		3024
>  #define RKISP1_ISP_MIN_WIDTH		32
> @@ -37,29 +42,36 @@
>  #define RKISP1_RSZ_SRC_MIN_WIDTH		32
>  #define RKISP1_RSZ_SRC_MIN_HEIGHT		16
>  
> +/* the default width and height of all the entities */
>  #define RKISP1_DEFAULT_WIDTH		800
>  #define RKISP1_DEFAULT_HEIGHT		600
>  
>  #define RKISP1_DRIVER_NAME	"rkisp1"
>  #define RKISP1_BUS_INFO		"platform:" RKISP1_DRIVER_NAME
>  
> +/* maximum number of clocks */
>  #define RKISP1_MAX_BUS_CLK	8
>  
> +/* a bitmask of the ready stats */
>  #define RKISP1_STATS_MEAS_MASK		(RKISP1_CIF_ISP_AWB_DONE |	\
>  					 RKISP1_CIF_ISP_AFM_FIN |	\
>  					 RKISP1_CIF_ISP_EXP_END |	\
>  					 RKISP1_CIF_ISP_HIST_MEASURE_RDY)
> +
> +/* enum for the resizer pads */
>  enum rkisp1_rsz_pad {
>  	RKISP1_RSZ_PAD_SINK,
>  	RKISP1_RSZ_PAD_SRC,
>  	RKISP1_RSZ_PAD_MAX
>  };
>  
> +/* enum for the capture id */
>  enum rkisp1_stream_id {
>  	RKISP1_MAINPATH,
>  	RKISP1_SELFPATH,
>  };
>  
> +/* bayer patterns */
>  enum rkisp1_fmt_raw_pat_type {
>  	RKISP1_RAW_RGGB = 0,
>  	RKISP1_RAW_GRBG,
> @@ -67,6 +79,7 @@ enum rkisp1_fmt_raw_pat_type {
>  	RKISP1_RAW_BGGR,
>  };
>  
> +/* enum for the isp pads */
>  enum rkisp1_isp_pad {
>  	RKISP1_ISP_PAD_SINK_VIDEO,
>  	RKISP1_ISP_PAD_SINK_PARAMS,
> @@ -76,8 +89,16 @@ enum rkisp1_isp_pad {
>  };
>  
>  /*
> - * struct rkisp1_sensor_async - Sensor information
> - * @mbus: media bus configuration
> + * struct rkisp1_sensor_async - A container for the v4l2_async_subdev to add to the notifier
> + *				of the v4l2-async API
> + *
> + * @asd:		async_subdev variable for the sensor
> + * @lanes:		number of lanes
> + * @mbus_type:		type of bus (currently only CSI2 is supported)
> + * @mbus_flags:		media bus (V4L2_MBUS_*) flags
> + * @sd:			a pointer to v4l2_subdev struct of the sensor
> + * @pixel_rate_ctrl:	pixel rate of the sensor, used to initialize the phy
> + * @dphy:		a pointer to the phy
>   */
>  struct rkisp1_sensor_async {
>  	struct v4l2_async_subdev asd;
> @@ -90,19 +111,17 @@ struct rkisp1_sensor_async {
>  };
>  
>  /*
> - * struct rkisp1_isp - ISP sub-device
> - *
> - * See Cropping regions of ISP in rkisp1.c for details
> - * @sink_frm: input size, don't have to be equal to sensor size
> - * @sink_fmt: input format
> - * @sink_crop: crop for sink pad
> - * @src_fmt: output format
> - * @src_crop: output size
> - * @ops_lock: ops serialization
> + * struct rkisp1_isp - ISP subdev entity
>   *
> - * @is_dphy_errctrl_disabled : if dphy errctrl is disabled (avoid endless interrupt)
> - * @frame_sequence: used to synchronize frame_id between video devices.
> - * @quantization: output quantization
> + * @sd:				v4l2_subdev variable
> + * @rkisp1:			pointer to rkisp1_device
> + * @pads:			media pads
> + * @pad_cfg:			pads configurations
> + * @sink_fmt:			input format
> + * @src_fmt:			output format
> + * @ops_lock:			ops serialization
> + * @is_dphy_errctrl_disabled:	if dphy errctrl is disabled (avoid endless interrupt)
> + * @frame_sequence:		used to synchronize frame_id between video devices.
>   */
>  struct rkisp1_isp {
>  	struct v4l2_subdev sd;
> @@ -111,11 +130,19 @@ struct rkisp1_isp {
>  	struct v4l2_subdev_pad_config pad_cfg[RKISP1_ISP_PAD_MAX];
>  	const struct rkisp1_isp_mbus_info *sink_fmt;
>  	const struct rkisp1_isp_mbus_info *src_fmt;
> -	struct mutex ops_lock;
> +	struct mutex ops_lock; /* serialize the subdevice ops */
>  	bool is_dphy_errctrl_disabled;
>  	atomic_t frame_sequence;
>  };
>  
> +/*
> + * struct rkisp1_vdev_node - Container for the video nodes: params, stats, mainpath, selfpath
> + *
> + * @buf_queue:	queue of buffers
> + * @vlock:	lock of the video node
> + * @vdev:	video node
> + * @pad:	media pad
> + */
>  struct rkisp1_vdev_node {
>  	struct vb2_queue buf_queue;
>  	struct mutex vlock; /* ioctl serialization mutex */
> @@ -123,6 +150,15 @@ struct rkisp1_vdev_node {
>  	struct media_pad pad;
>  };
>  
> +/*
> + * struct rkisp1_buffer - A container for the vb2 buffers used by the video devices:
> + *			  params, stats, mainpath, selfpath
> + *
> + * @vb:		vb2 buffer
> + * @queue:	entry of the buffer in the queue
> + * @buff_addr:	dma addresses of each plane, used only by the capture devices: selfpath, mainpath
> + * @vaddr:	virtual address for buffers used by params and stats devices
> + */
>  struct rkisp1_buffer {
>  	struct vb2_v4l2_buffer vb;
>  	struct list_head queue;
> @@ -132,6 +168,14 @@ struct rkisp1_buffer {
>  	};
>  };
>  
> +/*
> + * struct rkisp1_dummy_buffer - A buffer to write the next frame to in case
> + *				there are no vb2 buffers available.
> + *
> + * @vaddr:	return value of call to dma_alloc_attrs.
> + * @dma_addr:	dma address of the buffer.
> + * @size:	size of the buffer.
> + */
>  struct rkisp1_dummy_buffer {
>  	void *vaddr;
>  	dma_addr_t dma_addr;
> @@ -143,17 +187,29 @@ struct rkisp1_device;
>  /*
>   * struct rkisp1_capture - ISP capture video device
>   *
> - * @pix.fmt: buffer format
> - * @pix.info: pixel information
> - * @pix.cfg: pixel configuration
> - *
> - * @buf.lock: lock to protect buf_queue
> - * @buf.queue: queued buffer list
> - * @buf.dummy: dummy space to store dropped data
> + * @vnode:	  video node
> + * @rkisp1:	  pointer to rkisp1_device
> + * @id:		  id of the capture, one of RKISP1_SELFPATH, RKISP1_MAINPATH
> + * @ops:	  list of callbacks to configure the capture device.
> + * @config:	  a pointer to the list of registers to configure the capture format.
> + * @is_streaming: device is streaming
> + * @is_stopping:  stop_streaming callback was called and the device is in the process of
> + *		  stopping the streaming.
> + * @done:	  when stop_streaming callback is called, the device waits for the next irq
> + *		  handler to stop the streaming by waiting on the 'done' wait queue.
> + *		  If the irq handler is not called, the stream is stopped by the callback
> + *		  after timeout.
> + * @sp_y_stride:  the selfpath allows to configure a y stride that is longer than the image width.
> + * @buf.lock:	  lock to protect buf.queue
> + * @buf.queue:	  queued buffer list
> + * @buf.dummy:	  dummy space to store dropped data
>   *
> - * rkisp1 use shadowsock registers, so it need two buffer at a time
> - * @buf.curr: the buffer used for current frame
> - * @buf.next: the buffer used for next frame
> + * rkisp1 uses shadow registers, so it needs two buffers at a time
> + * @buf.curr:	  the buffer used for current frame
> + * @buf.next:	  the buffer used for next frame
> + * @pix.cfg:	  pixel configuration
> + * @pix.info:	  a pointer to the v4l2_format_info of the pixel format
> + * @pix.fmt:	  buffer format
>   */
>  struct rkisp1_capture {
>  	struct rkisp1_vdev_node vnode;
> @@ -183,14 +239,18 @@ struct rkisp1_capture {
>  /*
>   * struct rkisp1_stats - ISP Statistics device
>   *
> - * @lock: locks the buffer list 'stat' and 'is_streaming'
> - * @stat: stats buffer list
> + * @vnode:	  video node
> + * @rkisp1:	  pointer to the rkisp1 device
> + * @lock:	  locks the buffer list 'stat' and 'is_streaming'
> + * @stat:	  queue of rkisp1_buffer
> + * @vdev_fmt:	  v4l2_format of the metadata format
> + * @is_streaming: device is streaming
>   */
>  struct rkisp1_stats {
>  	struct rkisp1_vdev_node vnode;
>  	struct rkisp1_device *rkisp1;
>  
> -	spinlock_t lock; /* locks 'is_streaming', and 'stats' */
> +	spinlock_t lock; /* locks the buffers list 'stats' and 'is_streaming' */
>  	struct list_head stat;
>  	struct v4l2_format vdev_fmt;
>  	bool is_streaming;
> @@ -199,14 +259,22 @@ struct rkisp1_stats {
>  /*
>   * struct rkisp1_params - ISP input parameters device
>   *
> - * @cur_params: Current ISP parameters
> - * @is_first_params: the first params should take effect immediately
> + * @vnode:		video node
> + * @rkisp1:		pointer to the rkisp1 device
> + * @config_lock:	locks the buffer list 'params' and 'is_streaming'
> + * @params:		queue of rkisp1_buffer
> + * @cur_params:		the first params values from userspace
> + * @vdev_fmt:		v4l2_format of the metadata format
> + * @is_streaming:	device is streaming
> + * @is_first_params:	the first params should take effect immediately
> + * @quantization:	the quantization configured on the isp's src pad
> + * @raw_type:		the bayer pattern on the isp video sink pad
>   */
>  struct rkisp1_params {
>  	struct rkisp1_vdev_node vnode;
>  	struct rkisp1_device *rkisp1;
>  
> -	spinlock_t config_lock;
> +	spinlock_t config_lock; /* locks the buffers list 'params' and 'is_streaming' */
>  	struct list_head params;
>  	struct rkisp1_params_cfg cur_params;
>  	struct v4l2_format vdev_fmt;
> @@ -217,6 +285,18 @@ struct rkisp1_params {
>  	enum rkisp1_fmt_raw_pat_type raw_type;
>  };
>  
> +/*
> + * struct rkisp1_resizer - Resizer subdev
> + *
> + * @sd:	       v4l2_subdev variable
> + * @id:	       id of the resizer, one of RKISP1_SELFPATH, RKISP1_MAINPATH
> + * @rkisp1:    pointer to the rkisp1 device
> + * @pads:      media pads
> + * @pad_cfg:   configurations for the pads
> + * @config:    the set of registers to configure the resizer
> + * @pixel_enc: pixel encoding of the resizer
> + * @ops_lock:  a lock for the subdev ops
> + */
>  struct rkisp1_resizer {
>  	struct v4l2_subdev sd;
>  	enum rkisp1_stream_id id;
> @@ -225,9 +305,26 @@ struct rkisp1_resizer {
>  	struct v4l2_subdev_pad_config pad_cfg[RKISP1_RSZ_PAD_MAX];
>  	const struct rkisp1_rsz_config *config;
>  	enum v4l2_pixel_encoding pixel_enc;
> -	struct mutex ops_lock;
> +	struct mutex ops_lock; /* serialize the subdevice ops */
>  };
>  
> +/*
> + * struct rkisp1_debug - Values to be exposed on debugfs.
> + *			 The parameters are counters of the number of times the
> + *			 event occurred since the driver was loaded.
> + *
> + * @data_loss:			  loss of data occurred within a line, processing failure
> + * @outform_size_error:		  size error is generated in outmux submodule
> + * @img_stabilization_size_error: size error is generated in image stabilization submodule
> + * @inform_size_err:		  size error is generated in inform submodule
> + * @mipi_error:			  mipi error occurred
> + * @stats_error:		  writing to the 'Interrupt clear register' did not clear
> + *				  it in the register 'Masked interrupt status'
> + * @stop_timeout:		  upon stream stop, the capture waits 1 second for the isr to stop
> + *				  the stream. This param is incremented in case of timeout.
> + * @frame_drop:			  a frame was ready but the buffer queue was empty so the frame
> + *				  was not sent to userspace
> + */
>  struct rkisp1_debug {
>  	struct dentry *debugfs_dir;
>  	unsigned long data_loss;
> @@ -242,13 +339,24 @@ struct rkisp1_debug {
>  
>  /*
>   * struct rkisp1_device - ISP platform device
> - * @base_addr: base register address
> + *
> + * @base_addr:	   base register address
> + * @irq:	   the irq number
> + * @dev:	   a pointer to the struct device
> + * @clk_size:	   number of clocks
> + * @clks:	   array of clocks
> + * @v4l2_dev:	   v4l2_device variable
> + * @media_dev:	   media_device variable
> + * @notifier:	   a notifier to register on the v4l2-async API to be notified on the sensor
>   * @active_sensor: sensor in-use, set when streaming on
> - * @isp: ISP sub-device
> - * @rkisp1_capture: capture video device
> - * @stats: ISP statistics output device
> - * @params: ISP input parameters device
> - * @stream_lock: lock to serialize start/stop streaming in capture devices.
> + * @isp:	   ISP sub-device
> + * @resizer_devs:  resizer sub-devices
> + * @capture_devs:  capture devices
> + * @stats:	   ISP statistics metadata capture device
> + * @params:	   ISP parameters metadata output device
> + * @pipe:	   media pipeline
> + * @stream_lock:   serializes {start/stop}_streaming callbacks between the capture devices.
> + * @debug:	   debug params to be exposed on debugfs
>   */
>  struct rkisp1_device {
>  	void __iomem *base_addr;
> @@ -266,16 +374,20 @@ struct rkisp1_device {
>  	struct rkisp1_stats stats;
>  	struct rkisp1_params params;
>  	struct media_pipeline pipe;
> -	struct mutex stream_lock;
> +	struct mutex stream_lock; /* serialize {start/stop}_streaming cb between capture devices */
>  	struct rkisp1_debug debug;
>  };
>  
>  /*
> - * struct rkisp1_isp_mbus_info - ISP pad format info
> + * struct rkisp1_isp_mbus_info - ISP media bus info, Translates media bus code to hardware
> + *				 format values
>   *
> - * Translate mbus_code to hardware format values
> - *
> - * @bus_width: used for parallel
> + * @mbus_code: media bus code
> + * @pixel_enc: pixel encoding
> + * @mipi_dt:   mipi data type

yuv_seq missing here.

With this:

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

Thanks!
Helen

> + * @bus_width: bus width
> + * @bayer_pat: bayer pattern
> + * @direction: a bitmask of the flags indicating on which pad the format is supported on
>   */
>  struct rkisp1_isp_mbus_info {
>  	u32 mbus_code;
> @@ -298,25 +410,59 @@ static inline u32 rkisp1_read(struct rkisp1_device *rkisp1, unsigned int addr)
>  	return readl(rkisp1->base_addr + addr);
>  }
>  
> +/*
> + * rkisp1_sd_adjust_crop_rect - adjust a rectangle to fit into another rectangle.
> + *
> + * @crop:   rectangle to adjust.
> + * @bounds: rectangle used as bounds.
> + */
>  void rkisp1_sd_adjust_crop_rect(struct v4l2_rect *crop,
>  				const struct v4l2_rect *bounds);
>  
> +/*
> + * rkisp1_sd_adjust_crop - adjust a rectangle to fit into media bus format
> + *
> + * @crop:   rectangle to adjust.
> + * @bounds: media bus format used as bounds.
> + */
>  void rkisp1_sd_adjust_crop(struct v4l2_rect *crop,
>  			   const struct v4l2_mbus_framefmt *bounds);
>  
> +/*
> + * rkisp1_isp_mbus_info - get the isp info of the media bus code
> + *
> + * @mbus_code: the media bus code
> + */
>  const struct rkisp1_isp_mbus_info *rkisp1_isp_mbus_info_get(u32 mbus_code);
>  
> +/* rkisp1_params_configure - configure the params when stream starts.
> + *			     This function is called by the isp entity upon stream starts.
> + *			     The function applies the initial configuration of the parameters.
> + *
> + * @params:	  pointer to rkisp1_params.
> + * @bayer_pat:	  the bayer pattern on the isp video sink pad
> + * @quantization: the quantization configured on the isp's src pad
> + */
>  void rkisp1_params_configure(struct rkisp1_params *params,
>  			     enum rkisp1_fmt_raw_pat_type bayer_pat,
>  			     enum v4l2_quantization quantization);
> +
> +/* rkisp1_params_disable - disable all parameters.
> + *			   This function is called by the isp entity upon stream start
> + *			   when capturing bayer format.
> + *
> + * @params: pointer to rkisp1_params.
> + */
>  void rkisp1_params_disable(struct rkisp1_params *params);
>  
> +/* irq handlers */
>  void rkisp1_isp_isr(struct rkisp1_device *rkisp1);
>  void rkisp1_mipi_isr(struct rkisp1_device *rkisp1);
>  void rkisp1_capture_isr(struct rkisp1_device *rkisp1);
>  void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris);
>  void rkisp1_params_isr(struct rkisp1_device *rkisp1, u32 isp_mis);
>  
> +/* register/unregisters functions of the entities */
>  int rkisp1_capture_devs_register(struct rkisp1_device *rkisp1);
>  void rkisp1_capture_devs_unregister(struct rkisp1_device *rkisp1);
>  
> 

Re: [PATCH v2 5/9] media: staging: rkisp1: add a pointer to rkisp1_device from struct rkisp1_isp

[Dafna Hirschfeld]

Hi,

On 20.07.20 21:25, Helen Koike wrote:
> 
> 
> On 7/18/20 11:59 AM, Dafna Hirschfeld wrote:
>> The code in rkisp1-isp.c requires access to struct 'rkisp1_device'
>> in several places. It access it using the 'container_of' macro.
>> This patch adds a pointer to 'rkisp1_device' in struct 'rkisp1_isp'
>> to simplify the access.
> 
> What is wrong with container_of?

I remember Laurent suggested it a while ago.
I also feel container_of is a bit cumbersome and other entities already have a pointer to rkisp1_device.

Thanks,
Dafna

> 
> I usually prefer moving to container_of then the other way around, since this avoid a new field
> in the struct, and also avoid the requirements of keeping the pointer in sync.
> 
> Thanks
> Helen
> 
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-common.h |  1 +
>>   drivers/staging/media/rkisp1/rkisp1-isp.c    | 12 +++++-------
>>   2 files changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
>> index e54793d13c3d..893caa9df891 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
>> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
>> @@ -106,6 +106,7 @@ struct rkisp1_sensor_async {
>>    */
>>   struct rkisp1_isp {
>>   	struct v4l2_subdev sd;
>> +	struct rkisp1_device *rkisp1;
>>   	struct media_pad pads[RKISP1_ISP_PAD_MAX];
>>   	struct v4l2_subdev_pad_config pad_cfg[RKISP1_ISP_PAD_MAX];
>>   	const struct rkisp1_isp_mbus_info *sink_fmt;
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-isp.c b/drivers/staging/media/rkisp1/rkisp1-isp.c
>> index 6ec1e9816e9f..b2131aea5488 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-isp.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-isp.c
>> @@ -836,9 +836,8 @@ static int rkisp1_isp_set_selection(struct v4l2_subdev *sd,
>>   				    struct v4l2_subdev_pad_config *cfg,
>>   				    struct v4l2_subdev_selection *sel)
>>   {
>> -	struct rkisp1_device *rkisp1 =
>> -		container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev);
>>   	struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd);
>> +	struct rkisp1_device *rkisp1 = isp->rkisp1;
>>   	int ret = 0;
>>   
>>   	if (sel->target != V4L2_SEL_TGT_CROP)
>> @@ -883,8 +882,7 @@ static const struct v4l2_subdev_pad_ops rkisp1_isp_pad_ops = {
>>   static int rkisp1_mipi_csi2_start(struct rkisp1_isp *isp,
>>   				  struct rkisp1_sensor_async *sensor)
>>   {
>> -	struct rkisp1_device *rkisp1 =
>> -		container_of(isp->sd.v4l2_dev, struct rkisp1_device, v4l2_dev);
>> +	struct rkisp1_device *rkisp1 = isp->rkisp1;
>>   	union phy_configure_opts opts;
>>   	struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy;
>>   	s64 pixel_clock;
>> @@ -916,9 +914,8 @@ static void rkisp1_mipi_csi2_stop(struct rkisp1_sensor_async *sensor)
>>   
>>   static int rkisp1_isp_s_stream(struct v4l2_subdev *sd, int enable)
>>   {
>> -	struct rkisp1_device *rkisp1 =
>> -		container_of(sd->v4l2_dev, struct rkisp1_device, v4l2_dev);
>> -	struct rkisp1_isp *isp = &rkisp1->isp;
>> +	struct rkisp1_isp *isp = container_of(sd, struct rkisp1_isp, sd);
>> +	struct rkisp1_device *rkisp1 = isp->rkisp1;
>>   	struct v4l2_subdev *sensor_sd;
>>   	int ret = 0;
>>   
>> @@ -997,6 +994,7 @@ int rkisp1_isp_register(struct rkisp1_device *rkisp1,
>>   	struct v4l2_subdev *sd = &isp->sd;
>>   	int ret;
>>   
>> +	isp->rkisp1 = rkisp1;
>>   	v4l2_subdev_init(sd, &rkisp1_isp_ops);
>>   	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
>>   	sd->entity.ops = &rkisp1_isp_media_ops;
>>

Re: [PATCH v2 5/9] media: staging: rkisp1: add a pointer to rkisp1_device from struct rkisp1_isp

[Tomasz Figa]

On Tue, Jul 21, 2020 at 2:26 PM Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
>
> Hi,
>
> On 20.07.20 21:25, Helen Koike wrote:
> >
> >
> > On 7/18/20 11:59 AM, Dafna Hirschfeld wrote:
> >> The code in rkisp1-isp.c requires access to struct 'rkisp1_device'
> >> in several places. It access it using the 'container_of' macro.
> >> This patch adds a pointer to 'rkisp1_device' in struct 'rkisp1_isp'
> >> to simplify the access.
> >
> > What is wrong with container_of?
>
> I remember Laurent suggested it a while ago.
> I also feel container_of is a bit cumbersome and other entities already have a pointer to rkisp1_device.
>

Do we even need the rkisp1_isp struct? Could we just pass rkisp1_device instead?

Best regards,
Tomasz

Re: [PATCH v2 5/9] media: staging: rkisp1: add a pointer to rkisp1_device from struct rkisp1_isp

[Dafna Hirschfeld]

Hi,

On 21.07.20 14:36, Tomasz Figa wrote:
> On Tue, Jul 21, 2020 at 2:26 PM Dafna Hirschfeld
> <dafna.hirschfeld@collabora.com> wrote:
>>
>> Hi,
>>
>> On 20.07.20 21:25, Helen Koike wrote:
>>>
>>>
>>> On 7/18/20 11:59 AM, Dafna Hirschfeld wrote:
>>>> The code in rkisp1-isp.c requires access to struct 'rkisp1_device'
>>>> in several places. It access it using the 'container_of' macro.
>>>> This patch adds a pointer to 'rkisp1_device' in struct 'rkisp1_isp'
>>>> to simplify the access.
>>>
>>> What is wrong with container_of?
>>
>> I remember Laurent suggested it a while ago.
>> I also feel container_of is a bit cumbersome and other entities already have a pointer to rkisp1_device.
>>
> 
> Do we even need the rkisp1_isp struct? Could we just pass rkisp1_device instead?

pass to where ?  You mean to the function rkisp1_mipi_csi2_start ?

Thanks,
Dafna


> 
> Best regards,
> Tomasz
> 

Re: [PATCH v2 5/9] media: staging: rkisp1: add a pointer to rkisp1_device from struct rkisp1_isp

[Tomasz Figa]

On Tue, Jul 21, 2020 at 5:30 PM Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
>
> Hi,
>
> On 21.07.20 14:36, Tomasz Figa wrote:
> > On Tue, Jul 21, 2020 at 2:26 PM Dafna Hirschfeld
> > <dafna.hirschfeld@collabora.com> wrote:
> >>
> >> Hi,
> >>
> >> On 20.07.20 21:25, Helen Koike wrote:
> >>>
> >>>
> >>> On 7/18/20 11:59 AM, Dafna Hirschfeld wrote:
> >>>> The code in rkisp1-isp.c requires access to struct 'rkisp1_device'
> >>>> in several places. It access it using the 'container_of' macro.
> >>>> This patch adds a pointer to 'rkisp1_device' in struct 'rkisp1_isp'
> >>>> to simplify the access.
> >>>
> >>> What is wrong with container_of?
> >>
> >> I remember Laurent suggested it a while ago.
> >> I also feel container_of is a bit cumbersome and other entities already have a pointer to rkisp1_device.
> >>
> >
> > Do we even need the rkisp1_isp struct? Could we just pass rkisp1_device instead?
>
> pass to where ?  You mean to the function rkisp1_mipi_csi2_start ?

Yes, all around the driver, where rkisp1_isp is passed.

Re: [PATCH v2 5/9] media: staging: rkisp1: add a pointer to rkisp1_device from struct rkisp1_isp

[Dafna Hirschfeld]

On 21.07.20 17:32, Tomasz Figa wrote:
> On Tue, Jul 21, 2020 at 5:30 PM Dafna Hirschfeld
> <dafna.hirschfeld@collabora.com> wrote:
>>
>> Hi,
>>
>> On 21.07.20 14:36, Tomasz Figa wrote:
>>> On Tue, Jul 21, 2020 at 2:26 PM Dafna Hirschfeld
>>> <dafna.hirschfeld@collabora.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 20.07.20 21:25, Helen Koike wrote:
>>>>>
>>>>>
>>>>> On 7/18/20 11:59 AM, Dafna Hirschfeld wrote:
>>>>>> The code in rkisp1-isp.c requires access to struct 'rkisp1_device'
>>>>>> in several places. It access it using the 'container_of' macro.
>>>>>> This patch adds a pointer to 'rkisp1_device' in struct 'rkisp1_isp'
>>>>>> to simplify the access.
>>>>>
>>>>> What is wrong with container_of?
>>>>
>>>> I remember Laurent suggested it a while ago.
>>>> I also feel container_of is a bit cumbersome and other entities already have a pointer to rkisp1_device.
>>>>
>>>
>>> Do we even need the rkisp1_isp struct? Could we just pass rkisp1_device instead?
>>
>> pass to where ?  You mean to the function rkisp1_mipi_csi2_start ?
> 
> Yes, all around the driver, where rkisp1_isp is passed.

Most of the functions are part of subdevice callback implementation
where the rkisp1_device is not needed, so I don't the see the point.

Thanks,
Dafna

> 

Re: [PATCH v2 5/9] media: staging: rkisp1: add a pointer to rkisp1_device from struct rkisp1_isp

[Tomasz Figa]

On Sat, Aug 01, 2020 at 06:08:06PM +0200, Dafna Hirschfeld wrote:
> 
> 
> On 21.07.20 17:32, Tomasz Figa wrote:
> > On Tue, Jul 21, 2020 at 5:30 PM Dafna Hirschfeld
> > <dafna.hirschfeld@collabora.com> wrote:
> > > 
> > > Hi,
> > > 
> > > On 21.07.20 14:36, Tomasz Figa wrote:
> > > > On Tue, Jul 21, 2020 at 2:26 PM Dafna Hirschfeld
> > > > <dafna.hirschfeld@collabora.com> wrote:
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > On 20.07.20 21:25, Helen Koike wrote:
> > > > > > 
> > > > > > 
> > > > > > On 7/18/20 11:59 AM, Dafna Hirschfeld wrote:
> > > > > > > The code in rkisp1-isp.c requires access to struct 'rkisp1_device'
> > > > > > > in several places. It access it using the 'container_of' macro.
> > > > > > > This patch adds a pointer to 'rkisp1_device' in struct 'rkisp1_isp'
> > > > > > > to simplify the access.
> > > > > > 
> > > > > > What is wrong with container_of?
> > > > > 
> > > > > I remember Laurent suggested it a while ago.
> > > > > I also feel container_of is a bit cumbersome and other entities already have a pointer to rkisp1_device.
> > > > > 
> > > > 
> > > > Do we even need the rkisp1_isp struct? Could we just pass rkisp1_device instead?
> > > 
> > > pass to where ?  You mean to the function rkisp1_mipi_csi2_start ?
> > 
> > Yes, all around the driver, where rkisp1_isp is passed.
> 
> Most of the functions are part of subdevice callback implementation
> where the rkisp1_device is not needed, so I don't the see the point.

Okay, so then I'd just lean towards keeping it as is. container_of is
generally considered more efficient than a pointer, because it doesn't
require a load operation to obtain a reference to the parent struct.

Best regards,
Tomasz