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

[PATCH 0/4] 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, and also
a line description of every lock.
Three minor fixes found during documentation are also in this set.
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.


Dafna Hirschfeld (4):
  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: improve documentation of rkisp1-common.h

 drivers/staging/media/rkisp1/TODO             |   1 -
 drivers/staging/media/rkisp1/rkisp1-common.h  | 140 ++++++++++++++----
 drivers/staging/media/rkisp1/rkisp1-resizer.c |   2 +-
 3 files changed, 110 insertions(+), 33 deletions(-)

-- 
2.17.1

[PATCH 1/4] 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>
---
 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 45e554169224..eb0dbc42d09c 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -250,7 +250,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

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

[Helen Koike]

On 6/29/20 3:57 AM, Dafna Hirschfeld wrote:
> 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>

Thanks

> ---
>  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 45e554169224..eb0dbc42d09c 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -250,7 +250,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;
> 

[PATCH 2/4] 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>
---
 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 eb0dbc42d09c..b7dc523dd8f0 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -259,7 +259,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

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

[Helen Koike]

On 6/29/20 3:57 AM, Dafna Hirschfeld wrote:
> 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>

Thanks

> ---
>  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 eb0dbc42d09c..b7dc523dd8f0 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -259,7 +259,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;
>  };
> 

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

[Dafna Hirschfeld]

Currently the 'pads' and 'pad_cfg' arries of the rkisp1_resizer
are of size 'RKISP1_ISP_PAD_MAX' which is 4. But the resizer
has only two pads. This patch change the size of the arries 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>
---
 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 b7dc523dd8f0..4185487c520c 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -49,6 +49,7 @@
 enum rkisp1_rsz_pad {
 	RKISP1_RSZ_PAD_SINK,
 	RKISP1_RSZ_PAD_SRC,
+	RKISP1_RSZ_PAD_MAX
 };
 
 enum rkisp1_stream_id {
@@ -216,8 +217,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 26fb41053f56..d8ebe4422e77 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

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

[Helen Koike]

On 6/29/20 3:57 AM, Dafna Hirschfeld wrote:
> Currently the 'pads' and 'pad_cfg' arries of the rkisp1_resizer
> are of size 'RKISP1_ISP_PAD_MAX' which is 4. But the resizer
> has only two pads. This patch change the size of the arries to 2
> by adding and using 'RKISP1_RSZ_PAD_MAX' similar to the way it is
> done in the isp entity.

s/arries/arrays

s/This patch change/Change

Hans, could you correct these when picking it up?

> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

Thanks
Helen

> ---
>  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 b7dc523dd8f0..4185487c520c 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -49,6 +49,7 @@
>  enum rkisp1_rsz_pad {
>  	RKISP1_RSZ_PAD_SINK,
>  	RKISP1_RSZ_PAD_SRC,
> +	RKISP1_RSZ_PAD_MAX
>  };
>  
>  enum rkisp1_stream_id {
> @@ -216,8 +217,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 26fb41053f56..d8ebe4422e77 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;
>  
> 

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

[Hans Verkuil]

On 13/07/2020 13:12, Helen Koike wrote:
> 
> 
> On 6/29/20 3:57 AM, Dafna Hirschfeld wrote:
>> Currently the 'pads' and 'pad_cfg' arries of the rkisp1_resizer
>> are of size 'RKISP1_ISP_PAD_MAX' which is 4. But the resizer
>> has only two pads. This patch change the size of the arries to 2
>> by adding and using 'RKISP1_RSZ_PAD_MAX' similar to the way it is
>> done in the isp entity.
> 
> s/arries/arrays
> 
> s/This patch change/Change
> 
> Hans, could you correct these when picking it up?

I can.

I'll take patches 1-3 and will take a v2 of patch 4/4 once it is posted.

Regards,

	Hans

> 
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> 
> Acked-by: Helen Koike <helen.koike@collabora.com>
> 
> Thanks
> Helen
> 
>> ---
>>  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 b7dc523dd8f0..4185487c520c 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
>> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
>> @@ -49,6 +49,7 @@
>>  enum rkisp1_rsz_pad {
>>  	RKISP1_RSZ_PAD_SINK,
>>  	RKISP1_RSZ_PAD_SRC,
>> +	RKISP1_RSZ_PAD_MAX
>>  };
>>  
>>  enum rkisp1_stream_id {
>> @@ -216,8 +217,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 26fb41053f56..d8ebe4422e77 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;
>>  
>>

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

[Dafna Hirschfeld]

Add more detailed documentation of the structs 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 | 133 +++++++++++++++----
 2 files changed, 106 insertions(+), 28 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 4185487c520c..47b03a1dfe1b 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -73,8 +73,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: the async_subdev var 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 the v4l2_subdev struct of the sensor
+ * @pixel_rate_ctrl: the 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;
@@ -87,19 +95,16 @@ struct rkisp1_sensor_async {
 };
 
 /*
- * struct rkisp1_isp - ISP sub-device
+ * struct rkisp1_isp - ISP subdev entity
  *
- * See Cropping regions of ISP in rkisp1.c for details
- * @sink_frm: input size, don't have to be equal to sensor size
+ * @sd: the v4l2_subdev var
+ * @pads: the media pads
+ * @pad_cfg: the pads configurations
  * @sink_fmt: input format
- * @sink_crop: crop for sink pad
  * @src_fmt: output format
- * @src_crop: output size
  * @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.
- * @quantization: output quantization
  */
 struct rkisp1_isp {
 	struct v4l2_subdev sd;
@@ -107,11 +112,20 @@ 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 - A Container for the video nodes of the video devices:
+ *			     params, stats, mainpath, selfpath
+ *
+ * @buf_queue: the queue of buffers.
+ * @vlock: the lock of the video node
+ * @vdev: the video node
+ * @pad: the media pad
+ */
 struct rkisp1_vdev_node {
 	struct vb2_queue buf_queue;
 	struct mutex vlock; /* ioctl serialization mutex */
@@ -119,6 +133,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: the vb2 buffer
+ * @queue: the entry of the buffer in the queue
+ * @buff_addr: the dma addresses of each plain, used only by the capture devices: selfpath, mainpath
+ * @vaddr: the virtual addresses, used only by the params and stats devices
+ */
 struct rkisp1_buffer {
 	struct vb2_v4l2_buffer vb;
 	struct list_head queue;
@@ -128,6 +151,9 @@ struct rkisp1_buffer {
 	};
 };
 
+/*
+ * A dummy buffer to write the next frame to in case there are no vb2 buffers available.
+ */
 struct rkisp1_dummy_buffer {
 	void *vaddr;
 	dma_addr_t dma_addr;
@@ -139,17 +165,28 @@ 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
+ * @vnode: the video node
+ * @rkisp1: pointer to the rkisp1 device
+ * @id: the 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 from there by waiting on the 'done' wait queue.
+ *	  If the irq handler is not called, the stream is stopped from 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
+ * 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;
@@ -179,14 +216,18 @@ struct rkisp1_capture {
 /*
  * struct rkisp1_stats - ISP Statistics device
  *
+ * @vnode: the video node
+ * @rkisp1: pointer to the rkisp1 device
  * @lock: locks the buffer list 'stat' and 'is_streaming'
- * @stat: stats buffer list
+ * @stat: the queue of rkisp1_buffer
+ * @vdev_fmt: the v4l2_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;
@@ -195,14 +236,22 @@ struct rkisp1_stats {
 /*
  * struct rkisp1_params - ISP input parameters device
  *
- * @cur_params: Current ISP parameters
+ * @vnode: the video node
+ * @rkisp1: pointer to the rkisp1 device
+ * @config_lock: locks the buffer list 'params' and 'is_streaming'
+ * @params: the queue of rkisp1_buffer
+ * @cur_params: the first params values from userspace
+ * @vdev_fmt: the v4l2_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;
@@ -213,6 +262,18 @@ struct rkisp1_params {
 	enum rkisp1_fmt_raw_pat_type raw_type;
 };
 
+/*
+ * struct rkisp1_resizer - Resizer subdev
+ *
+ * @sd: the v4l2_subdev var
+ * @id: the id of the resizer, one of RKISP1_SELFPATH, RKISP1_MAINPATH
+ * @rkisp1: pointer to the rkisp1 device
+ * @pads: the media pads
+ * @pad_cfg: the configurations for the pads
+ * @config: the set of registers to configure the resizer
+ * @pixel_enc: the 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;
@@ -221,9 +282,12 @@ 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
+ */
 struct rkisp1_debug {
 	struct dentry *debugfs_dir;
 	unsigned long data_loss;
@@ -237,12 +301,22 @@ struct rkisp1_debug {
 /*
  * struct rkisp1_device - ISP platform device
  * @base_addr: base register address
+ * @irq: the irq number
+ * @dev: a pointer to the struct device
+ * @clk_size: the number of clocks
+ * @clks: the array of clocks
+ * @v4l2_dev: the v4l2_device
+ * @media_dev: the media_device
+ * @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
+ * @resizer_devs: the two resizer sub-devices
+ * @capture_devs: the two capture devices
  * @stats: ISP statistics output device
  * @params: ISP input parameters device
- * @stream_lock: lock to serialize start/stop streaming in capture devices.
+ * @pipe: the media pipeline
+ * @stream_lock: serializes {start/stop}_streaming callbacks between the capture devices.
+ * @debug: the debug params to be exposed on debugfs
  */
 struct rkisp1_device {
 	void __iomem *base_addr;
@@ -260,16 +334,21 @@ 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
  *
- * Translate mbus_code to hardware format values
+ * Translates mbus_code to hardware format values
  *
- * @bus_width: used for parallel
+ * @mbus_code: the media bus code
+ * @pixel_enc: the pixel encoding
+ * @mipi_dt: the mipi data type
+ * @bus_width: the bus width
+ * @bayer_pat: the 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;
-- 
2.17.1

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

[Helen Koike]

Hi Dafna,

Thanks for the patch.

On 6/29/20 3:57 AM, Dafna Hirschfeld wrote:
> Add more detailed documentation of the structs 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 | 133 +++++++++++++++----
>  2 files changed, 106 insertions(+), 28 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 4185487c520c..47b03a1dfe1b 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -73,8 +73,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: the async_subdev var for the sensor

I think you can remove "the" here, same for other fields in the rest of this patch.

> + * @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 the v4l2_subdev struct of the sensor
> + * @pixel_rate_ctrl: the pixel rate of the sensor, used to initialize the phy
> + * @dphy: a pointer to the phy
>   */

Just a nit, could you align the comments? I think it is easier to read.

>  struct rkisp1_sensor_async {
>  	struct v4l2_async_subdev asd;
> @@ -87,19 +95,16 @@ struct rkisp1_sensor_async {
>  };
>  
>  /*
> - * struct rkisp1_isp - ISP sub-device
> + * struct rkisp1_isp - ISP subdev entity
>   *
> - * See Cropping regions of ISP in rkisp1.c for details
> - * @sink_frm: input size, don't have to be equal to sensor size
> + * @sd: the v4l2_subdev var
> + * @pads: the media pads
> + * @pad_cfg: the pads configurations
>   * @sink_fmt: input format
> - * @sink_crop: crop for sink pad
>   * @src_fmt: output format
> - * @src_crop: output size
>   * @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.
> - * @quantization: output quantization
>   */
>  struct rkisp1_isp {
>  	struct v4l2_subdev sd;
> @@ -107,11 +112,20 @@ 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 - A Container for the video nodes of the video devices:
> + *			     params, stats, mainpath, selfpath

Or maybe just:
Container for the video devices nodes: params, stats, mainpath, selfpath

> + *
> + * @buf_queue: the queue of buffers.
> + * @vlock: the lock of the video node
> + * @vdev: the video node
> + * @pad: the media pad
> + */
>  struct rkisp1_vdev_node {
>  	struct vb2_queue buf_queue;
>  	struct mutex vlock; /* ioctl serialization mutex */
> @@ -119,6 +133,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: the vb2 buffer
> + * @queue: the entry of the buffer in the queue
> + * @buff_addr: the dma addresses of each plain, used only by the capture devices: selfpath, mainpath

s/plain/plane

> + * @vaddr: the virtual addresses, used only by the params and stats devices

maybe:

virtual addresses for buffers used by params and stats devices

I also noticed that params and stats only use vaddr[0], maybe we can remove the array and
replaced by a simple variable.

> + */
>  struct rkisp1_buffer {
>  	struct vb2_v4l2_buffer vb;
>  	struct list_head queue;
> @@ -128,6 +151,9 @@ struct rkisp1_buffer {
>  	};
>  };
>  
> +/*
> + * A dummy buffer to write the next frame to in case there are no vb2 buffers available.
> + */

Could you please follow the same standard used in the rest of this patch?

>  struct rkisp1_dummy_buffer {
>  	void *vaddr;
>  	dma_addr_t dma_addr;
> @@ -139,17 +165,28 @@ 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
> + * @vnode: the video node
> + * @rkisp1: pointer to the rkisp1 device
> + * @id: the 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 from there by waiting on the 'done' wait queue.

s/to stop the streaming from there by waiting/to stop the streaming by waiting/

> + *	  If the irq handler is not called, the stream is stopped from the callback after timeout.

s/from/by

> + * @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
> + * 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;
> @@ -179,14 +216,18 @@ struct rkisp1_capture {
>  /*
>   * struct rkisp1_stats - ISP Statistics device
>   *
> + * @vnode: the video node
> + * @rkisp1: pointer to the rkisp1 device
>   * @lock: locks the buffer list 'stat' and 'is_streaming'
> - * @stat: stats buffer list
> + * @stat: the queue of rkisp1_buffer
> + * @vdev_fmt: the v4l2_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;
> @@ -195,14 +236,22 @@ struct rkisp1_stats {
>  /*
>   * struct rkisp1_params - ISP input parameters device
>   *
> - * @cur_params: Current ISP parameters
> + * @vnode: the video node
> + * @rkisp1: pointer to the rkisp1 device
> + * @config_lock: locks the buffer list 'params' and 'is_streaming'
> + * @params: the queue of rkisp1_buffer
> + * @cur_params: the first params values from userspace
> + * @vdev_fmt: the v4l2_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

Just a note unrelated to this patch:
Maybe quantization could be moved to struct rkisp1_device, so it is accessible by all the entities
without the need to maintain copies around.

> + * @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;
> @@ -213,6 +262,18 @@ struct rkisp1_params {
>  	enum rkisp1_fmt_raw_pat_type raw_type;
>  };
>  
> +/*
> + * struct rkisp1_resizer - Resizer subdev
> + *
> + * @sd: the v4l2_subdev var
> + * @id: the id of the resizer, one of RKISP1_SELFPATH, RKISP1_MAINPATH
> + * @rkisp1: pointer to the rkisp1 device
> + * @pads: the media pads
> + * @pad_cfg: the configurations for the pads
> + * @config: the set of registers to configure the resizer
> + * @pixel_enc: the 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;
> @@ -221,9 +282,12 @@ 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
> + */
>  struct rkisp1_debug {
>  	struct dentry *debugfs_dir;
>  	unsigned long data_loss;
> @@ -237,12 +301,22 @@ struct rkisp1_debug {
>  /*
>   * struct rkisp1_device - ISP platform device

Missing new line here

>   * @base_addr: base register address
> + * @irq: the irq number
> + * @dev: a pointer to the struct device
> + * @clk_size: the number of clocks
> + * @clks: the array of clocks
> + * @v4l2_dev: the v4l2_device
> + * @media_dev: the media_device
> + * @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
> + * @resizer_devs: the two resizer sub-devices
> + * @capture_devs: the two capture devices
>   * @stats: ISP statistics output device
>   * @params: ISP input parameters device
> - * @stream_lock: lock to serialize start/stop streaming in capture devices.
> + * @pipe: the media pipeline
> + * @stream_lock: serializes {start/stop}_streaming callbacks between the capture devices.
> + * @debug: the debug params to be exposed on debugfs
>   */
>  struct rkisp1_device {
>  	void __iomem *base_addr;
> @@ -260,16 +334,21 @@ 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
>   *
> - * Translate mbus_code to hardware format values
> + * Translates mbus_code to hardware format values
>   *
> - * @bus_width: used for parallel
> + * @mbus_code: the media bus code
> + * @pixel_enc: the pixel encoding
> + * @mipi_dt: the mipi data type
> + * @bus_width: the bus width
> + * @bayer_pat: the 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;
> 

It would also be nice to add docs to the enums, macros and functions, but since this is an internal
header, it's fine, we can add them later.

With these changes:

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

Thanks
Helen

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

[Dafna Hirschfeld]

Hi

On 13.07.20 13:52, Helen Koike wrote:
> Hi Dafna,
> 
> Thanks for the patch.
> 
> On 6/29/20 3:57 AM, Dafna Hirschfeld wrote:
>> Add more detailed documentation of the structs 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 | 133 +++++++++++++++----
>>   2 files changed, 106 insertions(+), 28 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 4185487c520c..47b03a1dfe1b 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
>> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
>> @@ -73,8 +73,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: the async_subdev var for the sensor
> 
> I think you can remove "the" here, same for other fields in the rest of this patch.
> 
>> + * @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 the v4l2_subdev struct of the sensor
>> + * @pixel_rate_ctrl: the pixel rate of the sensor, used to initialize the phy
>> + * @dphy: a pointer to the phy
>>    */
> 
> Just a nit, could you align the comments? I think it is easier to read.
> 
>>   struct rkisp1_sensor_async {
>>   	struct v4l2_async_subdev asd;
>> @@ -87,19 +95,16 @@ struct rkisp1_sensor_async {
>>   };
>>   
>>   /*
>> - * struct rkisp1_isp - ISP sub-device
>> + * struct rkisp1_isp - ISP subdev entity
>>    *
>> - * See Cropping regions of ISP in rkisp1.c for details
>> - * @sink_frm: input size, don't have to be equal to sensor size
>> + * @sd: the v4l2_subdev var
>> + * @pads: the media pads
>> + * @pad_cfg: the pads configurations
>>    * @sink_fmt: input format
>> - * @sink_crop: crop for sink pad
>>    * @src_fmt: output format
>> - * @src_crop: output size
>>    * @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.
>> - * @quantization: output quantization
>>    */
>>   struct rkisp1_isp {
>>   	struct v4l2_subdev sd;
>> @@ -107,11 +112,20 @@ 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 - A Container for the video nodes of the video devices:
>> + *			     params, stats, mainpath, selfpath
> 
> Or maybe just:
> Container for the video devices nodes: params, stats, mainpath, selfpath
> 
>> + *
>> + * @buf_queue: the queue of buffers.
>> + * @vlock: the lock of the video node
>> + * @vdev: the video node
>> + * @pad: the media pad
>> + */
>>   struct rkisp1_vdev_node {
>>   	struct vb2_queue buf_queue;
>>   	struct mutex vlock; /* ioctl serialization mutex */
>> @@ -119,6 +133,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: the vb2 buffer
>> + * @queue: the entry of the buffer in the queue
>> + * @buff_addr: the dma addresses of each plain, used only by the capture devices: selfpath, mainpath
> 
> s/plain/plane
> 
>> + * @vaddr: the virtual addresses, used only by the params and stats devices
> 
> maybe:
> 
> virtual addresses for buffers used by params and stats devices
> 
> I also noticed that params and stats only use vaddr[0], maybe we can remove the array and
> replaced by a simple variable.
> 
>> + */
>>   struct rkisp1_buffer {
>>   	struct vb2_v4l2_buffer vb;
>>   	struct list_head queue;
>> @@ -128,6 +151,9 @@ struct rkisp1_buffer {
>>   	};
>>   };
>>   
>> +/*
>> + * A dummy buffer to write the next frame to in case there are no vb2 buffers available.
>> + */
> 
> Could you please follow the same standard used in the rest of this patch?
> 
>>   struct rkisp1_dummy_buffer {
>>   	void *vaddr;
>>   	dma_addr_t dma_addr;
>> @@ -139,17 +165,28 @@ 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
>> + * @vnode: the video node
>> + * @rkisp1: pointer to the rkisp1 device
>> + * @id: the 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 from there by waiting on the 'done' wait queue.
> 
> s/to stop the streaming from there by waiting/to stop the streaming by waiting/
> 
>> + *	  If the irq handler is not called, the stream is stopped from the callback after timeout.
> 
> s/from/by
> 
>> + * @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
>> + * 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;
>> @@ -179,14 +216,18 @@ struct rkisp1_capture {
>>   /*
>>    * struct rkisp1_stats - ISP Statistics device
>>    *
>> + * @vnode: the video node
>> + * @rkisp1: pointer to the rkisp1 device
>>    * @lock: locks the buffer list 'stat' and 'is_streaming'
>> - * @stat: stats buffer list
>> + * @stat: the queue of rkisp1_buffer
>> + * @vdev_fmt: the v4l2_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;
>> @@ -195,14 +236,22 @@ struct rkisp1_stats {
>>   /*
>>    * struct rkisp1_params - ISP input parameters device
>>    *
>> - * @cur_params: Current ISP parameters
>> + * @vnode: the video node
>> + * @rkisp1: pointer to the rkisp1 device
>> + * @config_lock: locks the buffer list 'params' and 'is_streaming'
>> + * @params: the queue of rkisp1_buffer
>> + * @cur_params: the first params values from userspace
>> + * @vdev_fmt: the v4l2_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
> 
> Just a note unrelated to this patch:
> Maybe quantization could be moved to struct rkisp1_device, so it is accessible by all the entities
> without the need to maintain copies around.

But quantization is only copied to the params entity so I don't think it should
be moved to rkisp1_device.

Thanks,
Dafna
  
> 
>> + * @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;
>> @@ -213,6 +262,18 @@ struct rkisp1_params {
>>   	enum rkisp1_fmt_raw_pat_type raw_type;
>>   };
>>   
>> +/*
>> + * struct rkisp1_resizer - Resizer subdev
>> + *
>> + * @sd: the v4l2_subdev var
>> + * @id: the id of the resizer, one of RKISP1_SELFPATH, RKISP1_MAINPATH
>> + * @rkisp1: pointer to the rkisp1 device
>> + * @pads: the media pads
>> + * @pad_cfg: the configurations for the pads
>> + * @config: the set of registers to configure the resizer
>> + * @pixel_enc: the 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;
>> @@ -221,9 +282,12 @@ 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
>> + */
>>   struct rkisp1_debug {
>>   	struct dentry *debugfs_dir;
>>   	unsigned long data_loss;
>> @@ -237,12 +301,22 @@ struct rkisp1_debug {
>>   /*
>>    * struct rkisp1_device - ISP platform device
> 
> Missing new line here
> 
>>    * @base_addr: base register address
>> + * @irq: the irq number
>> + * @dev: a pointer to the struct device
>> + * @clk_size: the number of clocks
>> + * @clks: the array of clocks
>> + * @v4l2_dev: the v4l2_device
>> + * @media_dev: the media_device
>> + * @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
>> + * @resizer_devs: the two resizer sub-devices
>> + * @capture_devs: the two capture devices
>>    * @stats: ISP statistics output device
>>    * @params: ISP input parameters device
>> - * @stream_lock: lock to serialize start/stop streaming in capture devices.
>> + * @pipe: the media pipeline
>> + * @stream_lock: serializes {start/stop}_streaming callbacks between the capture devices.
>> + * @debug: the debug params to be exposed on debugfs
>>    */
>>   struct rkisp1_device {
>>   	void __iomem *base_addr;
>> @@ -260,16 +334,21 @@ 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
>>    *
>> - * Translate mbus_code to hardware format values
>> + * Translates mbus_code to hardware format values
>>    *
>> - * @bus_width: used for parallel
>> + * @mbus_code: the media bus code
>> + * @pixel_enc: the pixel encoding
>> + * @mipi_dt: the mipi data type
>> + * @bus_width: the bus width
>> + * @bayer_pat: the 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;
>>
> 
> It would also be nice to add docs to the enums, macros and functions, but since this is an internal
> header, it's fine, we can add them later.
> 
> With these changes:
> 
> Acked-by: Helen Koike <helen.koike@collabora.com>
> 
> Thanks
> Helen
>