[PATCH 0/5] staging: media: atomisp: Remove #ifdef 2401

[PATCH 0/5] staging: media: atomisp: Remove #ifdef 2401

[Kate Hsuan]

Since #ifdef ISP2401 is used to determine the action of both models in compiler
time, we have to provide two binaries for both models. It is very unfriendly for
the users and for the package management aspect.

The proposed patch removed the #ifdef ISP2041 from the codes and made the 
path for both models can be determined at the runtime. Some of the #ifdef is 
highly integrated with functions and data structures. If we try to remove 
them, it will cause many issues, such as duplicated variable/function name
and data length. Therefore, these patches focus on removing the #ifdef
without affecting the change of structure.

Kate Hsuan (5):
  staging: media: atomisp: sh_css: Remove #ifdef ISP2401
  staging: media: atomisp: runtime: frame: remove #ifdef ISP2401
  staging: media: atomisp: sh_css_sp: Remove #ifdef ISP2401
  staging: media: atomisp: sh_css_firmware: determine firmware version
    at runtime
  staging: media: atomisp: sh_css_mipi: Remove #ifdef 2041

 .../atomisp/pci/runtime/frame/src/frame.c     |  15 +-
 drivers/staging/media/atomisp/pci/sh_css.c    | 584 +++++++++---------
 .../media/atomisp/pci/sh_css_firmware.c       |  18 +-
 .../staging/media/atomisp/pci/sh_css_mipi.c   | 101 ++-
 drivers/staging/media/atomisp/pci/sh_css_sp.c |  10 +-
 5 files changed, 359 insertions(+), 369 deletions(-)

-- 
2.39.2

[PATCH 1/5] staging: media: atomisp: sh_css: Remove #ifdef ISP2401

[Kate Hsuan]

The actions of ISP2401 and 2400 are determined at the runtime.

Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 drivers/staging/media/atomisp/pci/sh_css.c | 584 ++++++++++-----------
 1 file changed, 286 insertions(+), 298 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/sh_css.c b/drivers/staging/media/atomisp/pci/sh_css.c
index 93789500416f..0cc008b3c921 100644
--- a/drivers/staging/media/atomisp/pci/sh_css.c
+++ b/drivers/staging/media/atomisp/pci/sh_css.c
@@ -1529,15 +1529,14 @@ ia_css_init(struct device *dev, const struct ia_css_env *env,
 
 	mipi_init();
 
-#ifndef ISP2401
 	/*
 	 * In case this has been programmed already, update internal
 	 * data structure ...
 	 * DEPRECATED
 	 */
-	my_css.page_table_base_index = mmu_get_page_table_base_index(MMU0_ID);
+	if (!IS_ISP2401)
+		my_css.page_table_base_index = mmu_get_page_table_base_index(MMU0_ID);
 
-#endif
 	my_css.irq_type = irq_type;
 
 	my_css_save.irq_type = irq_type;
@@ -1596,10 +1595,8 @@ ia_css_init(struct device *dev, const struct ia_css_env *env,
 	 * sh_css_init_buffer_queues();
 	 */
 
-#if defined(ISP2401)
-	gp_device_reg_store(GP_DEVICE0_ID, _REG_GP_SWITCH_ISYS2401_ADDR, 1);
-#endif
-
+	if (IS_ISP2401)
+		gp_device_reg_store(GP_DEVICE0_ID, _REG_GP_SWITCH_ISYS2401_ADDR, 1);
 
 	if (!IS_ISP2401)
 		dma_set_max_burst_size(DMA0_ID, HIVE_DMA_BUS_DDR_CONN,
@@ -2128,13 +2125,13 @@ ia_css_pipe_destroy(struct ia_css_pipe *pipe)
 						    err);
 			}
 		}
-#ifndef ISP2401
-		ia_css_frame_free_multiple(NUM_VIDEO_TNR_FRAMES,
-					   pipe->pipe_settings.video.tnr_frames);
-#else
-		ia_css_frame_free_multiple(NUM_VIDEO_TNR_FRAMES,
-					   pipe->pipe_settings.video.tnr_frames);
-#endif
+		if (IS_ISP2401)
+			ia_css_frame_free_multiple(NUM_VIDEO_TNR_FRAMES,
+						   pipe->pipe_settings.video.tnr_frames);
+		else
+			ia_css_frame_free_multiple(NUM_VIDEO_TNR_FRAMES,
+						   pipe->pipe_settings.video.tnr_frames);
+
 		ia_css_frame_free_multiple(MAX_NUM_VIDEO_DELAY_FRAMES,
 					   pipe->pipe_settings.video.delay_frames);
 		break;
@@ -2238,11 +2235,10 @@ int ia_css_irq_translate(
 		case virq_isys_csi:
 			infos |= IA_CSS_IRQ_INFO_INPUT_SYSTEM_ERROR;
 			break;
-#if !defined(ISP2401)
 		case virq_ifmt0_id:
-			infos |= IA_CSS_IRQ_INFO_IF_ERROR;
+			if (!IS_ISP2401)
+				infos |= IA_CSS_IRQ_INFO_IF_ERROR;
 			break;
-#endif
 		case virq_dma:
 			infos |= IA_CSS_IRQ_INFO_DMA_ERROR;
 			break;
@@ -2277,27 +2273,34 @@ int ia_css_irq_enable(
 	IA_CSS_ENTER("info=%d, enable=%d", info, enable);
 
 	switch (info) {
-#if !defined(ISP2401)
 	case IA_CSS_IRQ_INFO_CSS_RECEIVER_SOF:
+		if (IS_ISP2401)
+			/* Just ignore those unused IRQs without printing errors */
+			return 0;
+
 		irq = virq_isys_sof;
 		break;
 	case IA_CSS_IRQ_INFO_CSS_RECEIVER_EOF:
+		if (IS_ISP2401)
+			/* Just ignore those unused IRQs without printing errors */
+			return 0;
+
 		irq = virq_isys_eof;
 		break;
 	case IA_CSS_IRQ_INFO_INPUT_SYSTEM_ERROR:
+		if (IS_ISP2401)
+			/* Just ignore those unused IRQs without printing errors */
+			return 0;
+
 		irq = virq_isys_csi;
 		break;
 	case IA_CSS_IRQ_INFO_IF_ERROR:
+		if (IS_ISP2401)
+			/* Just ignore those unused IRQs without printing errors */
+			return 0;
+
 		irq = virq_ifmt0_id;
 		break;
-#else
-	case IA_CSS_IRQ_INFO_CSS_RECEIVER_SOF:
-	case IA_CSS_IRQ_INFO_CSS_RECEIVER_EOF:
-	case IA_CSS_IRQ_INFO_INPUT_SYSTEM_ERROR:
-	case IA_CSS_IRQ_INFO_IF_ERROR:
-		/* Just ignore those unused IRQs without printing errors */
-		return 0;
-#endif
 	case IA_CSS_IRQ_INFO_DMA_ERROR:
 		irq = virq_dma;
 		break;
@@ -2413,14 +2416,14 @@ alloc_continuous_frames(struct ia_css_pipe *pipe, bool init_time)
 		return -EINVAL;
 	}
 
-#if defined(ISP2401)
-	/* For CSI2+, the continuous frame will hold the full input frame */
-	ref_info.res.width = pipe->stream->config.input_config.input_res.width;
-	ref_info.res.height = pipe->stream->config.input_config.input_res.height;
+	if (IS_ISP2401) {
+		/* For CSI2+, the continuous frame will hold the full input frame */
+		ref_info.res.width = pipe->stream->config.input_config.input_res.width;
+		ref_info.res.height = pipe->stream->config.input_config.input_res.height;
 
-	/* Ensure padded width is aligned for 2401 */
-	ref_info.padded_width = CEIL_MUL(ref_info.res.width, 2 * ISP_VEC_NELEMS);
-#endif
+		/* Ensure padded width is aligned for 2401 */
+		ref_info.padded_width = CEIL_MUL(ref_info.res.width, 2 * ISP_VEC_NELEMS);
+	}
 
 	if (pipe->stream->config.pack_raw_pixels) {
 		ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
@@ -2499,11 +2502,9 @@ load_preview_binaries(struct ia_css_pipe *pipe)
 	int err = 0;
 	bool need_vf_pp = false;
 	bool need_isp_copy_binary = false;
-#ifdef ISP2401
 	bool sensor = false;
-#else
 	bool continuous;
-#endif
+
 	/* preview only have 1 output pin now */
 	struct ia_css_frame_info *pipe_out_info = &pipe->output_info[0];
 	struct ia_css_preview_settings *mycs  = &pipe->pipe_settings.preview;
@@ -2514,11 +2515,11 @@ load_preview_binaries(struct ia_css_pipe *pipe)
 	assert(pipe->mode == IA_CSS_PIPE_ID_PREVIEW);
 
 	online = pipe->stream->config.online;
-#ifdef ISP2401
-	sensor = pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR;
-#else
-	continuous = pipe->stream->config.continuous;
-#endif
+
+	if (IS_ISP2401)
+		sensor = pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR;
+	else
+		continuous = pipe->stream->config.continuous;
 
 	if (mycs->preview_binary.info)
 		return 0;
@@ -2627,24 +2628,26 @@ load_preview_binaries(struct ia_css_pipe *pipe)
 			return err;
 	}
 
-#ifdef ISP2401
-	/*
-	 * When the input system is 2401, only the Direct Sensor Mode
-	 * Offline Preview uses the ISP copy binary.
-	 */
-	need_isp_copy_binary = !online && sensor;
-#else
-	/*
-	 * About pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY:
-	 * This is typical the case with SkyCam (which has no input system) but it also applies to all cases
-	 * where the driver chooses for memory based input frames. In these cases, a copy binary (which typical
-	 * copies sensor data to DDR) does not have much use.
-	 */
-	if (!IS_ISP2401)
-		need_isp_copy_binary = !online && !continuous;
+	if (IS_ISP2401)
+		/*
+		 * When the input system is 2401, only the Direct Sensor Mode
+		 * Offline Preview uses the ISP copy binary.
+		 */
+		need_isp_copy_binary = !online && sensor;
 	else
-		need_isp_copy_binary = !online && !continuous && !(pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY);
-#endif
+		/*
+		 * About pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY:
+		 * This is typical the case with SkyCam (which has no input system) but it also
+		 * applies to all cases where the driver chooses for memory based input frames.
+		 * In these cases, a copy binary (which typical copies sensor data to DDR) does
+		 * not have much use.
+		 */
+		if (!IS_ISP2401)
+			need_isp_copy_binary = !online && !continuous;
+		else
+			need_isp_copy_binary = !online && !continuous &&
+					       !(pipe->stream->config.mode ==
+						 IA_CSS_INPUT_MODE_MEMORY);
 
 	/* Copy */
 	if (need_isp_copy_binary) {
@@ -3125,11 +3128,11 @@ init_in_frameinfo_memory_defaults(struct ia_css_pipe *pipe,
 
 	in_frame->frame_info.format = format;
 
-#ifdef ISP2401
-	if (format == IA_CSS_FRAME_FORMAT_RAW)
-		in_frame->frame_info.format = (pipe->stream->config.pack_raw_pixels) ?
-		IA_CSS_FRAME_FORMAT_RAW_PACKED : IA_CSS_FRAME_FORMAT_RAW;
-#endif
+	if (IS_ISP2401) {
+		if (format == IA_CSS_FRAME_FORMAT_RAW)
+			in_frame->frame_info.format = (pipe->stream->config.pack_raw_pixels) ?
+			IA_CSS_FRAME_FORMAT_RAW_PACKED : IA_CSS_FRAME_FORMAT_RAW;
+	}
 
 	in_frame->frame_info.res.width = pipe->stream->config.input_config.input_res.width;
 	in_frame->frame_info.res.height = pipe->stream->config.input_config.input_res.height;
@@ -3141,9 +3144,12 @@ init_in_frameinfo_memory_defaults(struct ia_css_pipe *pipe,
 	ia_css_query_internal_queue_id(IA_CSS_BUFFER_TYPE_INPUT_FRAME, thread_id, &queue_id);
 	in_frame->dynamic_queue_id = queue_id;
 	in_frame->buf_type = IA_CSS_BUFFER_TYPE_INPUT_FRAME;
+
 #ifdef ISP2401
+
 	ia_css_get_crop_offsets(pipe, &in_frame->frame_info);
 #endif
+
 	err = ia_css_frame_init_planes(in_frame);
 
 	ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, "%s() bayer_order = %d\n",
@@ -3211,18 +3217,18 @@ static int create_host_video_pipeline(struct ia_css_pipe *pipe)
 
 	me->dvs_frame_delay = pipe->dvs_frame_delay;
 
-#ifdef ISP2401
-	/*
-	 * When the input system is 2401, always enable 'in_frameinfo_memory'
-	 * except for the following: online or continuous
-	 */
-	need_in_frameinfo_memory = !(pipe->stream->config.online ||
-				     pipe->stream->config.continuous);
-#else
-	/* Construct in_frame info (only in case we have dynamic input */
-	need_in_frameinfo_memory = pipe->stream->config.mode ==
-				   IA_CSS_INPUT_MODE_MEMORY;
-#endif
+	if (IS_ISP2401) {
+		/*
+		 * When the input system is 2401, always enable 'in_frameinfo_memory'
+		 * except for the following: online or continuous
+		 */
+		need_in_frameinfo_memory = !(pipe->stream->config.online ||
+					     pipe->stream->config.continuous);
+	} else {
+		/* Construct in_frame info (only in case we have dynamic input */
+		need_in_frameinfo_memory = pipe->stream->config.mode ==
+					   IA_CSS_INPUT_MODE_MEMORY;
+	}
 
 	/* Construct in_frame info (only in case we have dynamic input */
 	if (need_in_frameinfo_memory) {
@@ -3268,15 +3274,14 @@ static int create_host_video_pipeline(struct ia_css_pipe *pipe)
 			goto ERR;
 		in_frame = me->stages->args.out_frame[0];
 	} else if (pipe->stream->config.continuous) {
-#ifdef ISP2401
-		/*
-		 * When continuous is enabled, configure in_frame with the
-		 * last pipe, which is the copy pipe.
-		 */
-		in_frame = pipe->stream->last_pipe->continuous_frames[0];
-#else
-		in_frame = pipe->continuous_frames[0];
-#endif
+		if (IS_ISP2401)
+			/*
+			 * When continuous is enabled, configure in_frame with the
+			 * last pipe, which is the copy pipe.
+			 */
+			in_frame = pipe->stream->last_pipe->continuous_frames[0];
+		else
+			in_frame = pipe->continuous_frames[0];
 	}
 
 	ia_css_pipe_util_set_output_frames(out_frames, 0,
@@ -3373,12 +3378,10 @@ create_host_preview_pipeline(struct ia_css_pipe *pipe)
 	struct ia_css_frame *out_frame;
 	struct ia_css_frame *out_frames[IA_CSS_BINARY_MAX_OUTPUT_PORTS];
 	bool need_in_frameinfo_memory = false;
-#ifdef ISP2401
 	bool sensor = false;
 	bool buffered_sensor = false;
 	bool online = false;
 	bool continuous = false;
-#endif
 
 	IA_CSS_ENTER_PRIVATE("pipe = %p", pipe);
 	if ((!pipe) || (!pipe->stream) || (pipe->mode != IA_CSS_PIPE_ID_PREVIEW)) {
@@ -3391,25 +3394,25 @@ create_host_preview_pipeline(struct ia_css_pipe *pipe)
 	me = &pipe->pipeline;
 	ia_css_pipeline_clean(me);
 
-#ifdef ISP2401
-	/*
-	 * When the input system is 2401, always enable 'in_frameinfo_memory'
-	 * except for the following:
-	 * - Direct Sensor Mode Online Preview
-	 * - Buffered Sensor Mode Online Preview
-	 * - Direct Sensor Mode Continuous Preview
-	 * - Buffered Sensor Mode Continuous Preview
-	 */
-	sensor = (pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR);
-	buffered_sensor = (pipe->stream->config.mode == IA_CSS_INPUT_MODE_BUFFERED_SENSOR);
-	online = pipe->stream->config.online;
-	continuous = pipe->stream->config.continuous;
-	need_in_frameinfo_memory =
-	!((sensor && (online || continuous)) || (buffered_sensor && (online || continuous)));
-#else
-	/* Construct in_frame info (only in case we have dynamic input */
-	need_in_frameinfo_memory = pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY;
-#endif
+	if (IS_ISP2401) {
+		/*
+		 * When the input system is 2401, always enable 'in_frameinfo_memory'
+		 * except for the following:
+		 * - Direct Sensor Mode Online Preview
+		 * - Buffered Sensor Mode Online Preview
+		 * - Direct Sensor Mode Continuous Preview
+		 * - Buffered Sensor Mode Continuous Preview
+		 */
+		sensor = (pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR);
+		buffered_sensor = (pipe->stream->config.mode == IA_CSS_INPUT_MODE_BUFFERED_SENSOR);
+		online = pipe->stream->config.online;
+		continuous = pipe->stream->config.continuous;
+		need_in_frameinfo_memory =
+		!((sensor && (online || continuous)) || (buffered_sensor &&
+							(online || continuous)));
+	} else
+		/* Construct in_frame info (only in case we have dynamic input */
+		need_in_frameinfo_memory = pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY;
 	if (need_in_frameinfo_memory) {
 		err = init_in_frameinfo_memory_defaults(pipe, &me->in_frame,
 							IA_CSS_FRAME_FORMAT_RAW);
@@ -3417,9 +3420,9 @@ create_host_preview_pipeline(struct ia_css_pipe *pipe)
 			goto ERR;
 
 		in_frame = &me->in_frame;
-	} else {
+	} else
 		in_frame = NULL;
-	}
+
 
 	err = init_out_frameinfo_defaults(pipe, &me->out_frame[0], 0);
 	if (err)
@@ -3441,17 +3444,16 @@ create_host_preview_pipeline(struct ia_css_pipe *pipe)
 			goto ERR;
 		in_frame = me->stages->args.out_frame[0];
 	} else if (pipe->stream->config.continuous) {
-#ifdef ISP2401
-		/*
-		 * When continuous is enabled, configure in_frame with the
-		 * last pipe, which is the copy pipe.
-		 */
-		if (continuous || !online)
-			in_frame = pipe->stream->last_pipe->continuous_frames[0];
-
-#else
-		in_frame = pipe->continuous_frames[0];
-#endif
+		if (IS_ISP2401) {
+			/*
+			 * When continuous is enabled, configure in_frame with the
+			 * last pipe, which is the copy pipe.
+			 */
+			if (continuous || !online)
+				in_frame = pipe->stream->last_pipe->continuous_frames[0];
+		} else {
+			in_frame = pipe->continuous_frames[0];
+		}
 	}
 
 	if (vf_pp_binary) {
@@ -3925,19 +3927,19 @@ ia_css_pipe_dequeue_buffer(struct ia_css_pipe *pipe,
 			case IA_CSS_BUFFER_TYPE_OUTPUT_FRAME:
 			case IA_CSS_BUFFER_TYPE_SEC_OUTPUT_FRAME:
 				if (pipe && pipe->stop_requested) {
-#if !defined(ISP2401)
-					/*
-					 * free mipi frames only for old input
-					 * system for 2401 it is done in
-					 * ia_css_stream_destroy call
-					 */
-					return_err = free_mipi_frames(pipe);
-					if (return_err) {
-						IA_CSS_LOG("free_mipi_frames() failed");
-						IA_CSS_LEAVE_ERR(return_err);
-						return return_err;
+					if (!IS_ISP2401) {
+						/*
+						 * free mipi frames only for old input
+						 * system for 2401 it is done in
+						 * ia_css_stream_destroy call
+						 */
+						return_err = free_mipi_frames(pipe);
+						if (return_err) {
+							IA_CSS_LOG("free_mipi_frames() failed");
+							IA_CSS_LEAVE_ERR(return_err);
+							return return_err;
+						}
 					}
-#endif
 					pipe->stop_requested = false;
 				}
 				fallthrough;
@@ -3959,12 +3961,11 @@ ia_css_pipe_dequeue_buffer(struct ia_css_pipe *pipe,
 					pipe->num_invalid_frames--;
 
 				if (frame->frame_info.format == IA_CSS_FRAME_FORMAT_BINARY_8) {
-#ifdef ISP2401
-					frame->planes.binary.size = frame->data_bytes;
-#else
-					frame->planes.binary.size =
-					    sh_css_sp_get_binary_copy_size();
-#endif
+					if (IS_ISP2401)
+						frame->planes.binary.size = frame->data_bytes;
+					else
+						frame->planes.binary.size =
+						    sh_css_sp_get_binary_copy_size();
 				}
 				if (buf_type == IA_CSS_BUFFER_TYPE_OUTPUT_FRAME) {
 					IA_CSS_LOG("pfp: dequeued OF %d with config id %d thread %d",
@@ -4880,22 +4881,20 @@ static int load_video_binaries(struct ia_css_pipe *pipe)
 			    pipe->num_invalid_frames, pipe->dvs_frame_delay);
 
 	/* pqiao TODO: temp hack for PO, should be removed after offline YUVPP is enabled */
-#if !defined(ISP2401)
-	/* Copy */
-	if (!online && !continuous) {
-		/*
-		 * TODO: what exactly needs doing, prepend the copy binary to
-		 *	 video base this only on !online?
-		 */
-		err = load_copy_binary(pipe,
-				       &mycs->copy_binary,
-				       &mycs->video_binary);
-		if (err)
-			return err;
+	if (!IS_ISP2401) {
+		/* Copy */
+		if (!online && !continuous) {
+			/*
+			 * TODO: what exactly needs doing, prepend the copy binary to
+			 *	 video base this only on !online?
+			 */
+			err = load_copy_binary(pipe,
+					       &mycs->copy_binary,
+					       &mycs->video_binary);
+			if (err)
+				return err;
+		}
 	}
-#else
-	(void)continuous;
-#endif
 
 	if (pipe->enable_viewfinder[IA_CSS_PIPE_OUTPUT_STAGE_0] && need_vf_pp) {
 		struct ia_css_binary_descr vf_pp_descr;
@@ -5227,11 +5226,8 @@ static int load_primary_binaries(
 	bool need_pp = false;
 	bool need_isp_copy_binary = false;
 	bool need_ldc = false;
-#ifdef ISP2401
 	bool sensor = false;
-#else
 	bool memory, continuous;
-#endif
 	struct ia_css_frame_info prim_in_info,
 		       prim_out_info,
 		       capt_pp_out_info, vf_info,
@@ -5251,12 +5247,12 @@ static int load_primary_binaries(
 	       pipe->mode == IA_CSS_PIPE_ID_COPY);
 
 	online = pipe->stream->config.online;
-#ifdef ISP2401
-	sensor = (pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR);
-#else
-	memory = pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY;
-	continuous = pipe->stream->config.continuous;
-#endif
+	if (IS_ISP2401) {
+		sensor = (pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR);
+	} else {
+		memory = pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY;
+		continuous = pipe->stream->config.continuous;
+	}
 
 	mycs = &pipe->pipe_settings.capture;
 	pipe_out_info = &pipe->output_info[0];
@@ -5462,15 +5458,14 @@ static int load_primary_binaries(
 	if (err)
 		return err;
 
-#ifdef ISP2401
-	/*
-	 * When the input system is 2401, only the Direct Sensor Mode
-	 * Offline Capture uses the ISP copy binary.
-	 */
-	need_isp_copy_binary = !online && sensor;
-#else
-	need_isp_copy_binary = !online && !continuous && !memory;
-#endif
+	if (IS_ISP2401)
+		/*
+		 * When the input system is 2401, only the Direct Sensor Mode
+		 * Offline Capture uses the ISP copy binary.
+		 */
+		need_isp_copy_binary = !online && sensor;
+	else
+		need_isp_copy_binary = !online && !continuous && !memory;
 
 	/* ISP Copy */
 	if (need_isp_copy_binary) {
@@ -5681,10 +5676,10 @@ static int load_advanced_binaries(struct ia_css_pipe *pipe)
 	}
 
 	/* Copy */
-#ifdef ISP2401
-	/* For CSI2+, only the direct sensor mode/online requires ISP copy */
-	need_isp_copy = pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR;
-#endif
+	if (IS_ISP2401)
+		/* For CSI2+, only the direct sensor mode/online requires ISP copy */
+		need_isp_copy = pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR;
+
 	if (need_isp_copy)
 		load_copy_binary(pipe,
 				 &pipe->pipe_settings.capture.copy_binary,
@@ -5829,10 +5824,10 @@ static int load_low_light_binaries(struct ia_css_pipe *pipe)
 	}
 
 	/* Copy */
-#ifdef ISP2401
-	/* For CSI2+, only the direct sensor mode/online requires ISP copy */
-	need_isp_copy = pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR;
-#endif
+	if (IS_ISP2401)
+		/* For CSI2+, only the direct sensor mode/online requires ISP copy */
+		need_isp_copy = pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR;
+
 	if (need_isp_copy)
 		err = load_copy_binary(pipe,
 				       &pipe->pipe_settings.capture.copy_binary,
@@ -5902,10 +5897,9 @@ static int load_capture_binaries(struct ia_css_pipe *pipe)
 	switch (pipe->config.default_capture_config.mode) {
 	case IA_CSS_CAPTURE_MODE_RAW:
 		err = load_copy_binaries(pipe);
-#if defined(ISP2401)
-		if (!err)
+		if (!err && IS_ISP2401)
 			pipe->pipe_settings.capture.copy_binary.online = pipe->stream->config.online;
-#endif
+
 		break;
 	case IA_CSS_CAPTURE_MODE_BAYER:
 		err = load_bayer_isp_binaries(pipe);
@@ -6409,7 +6403,6 @@ load_yuvpp_binaries(struct ia_css_pipe *pipe)
 	else
 		next_binary = NULL;
 
-#if defined(ISP2401)
 	/*
 	 * NOTES
 	 * - Why does the "yuvpp" pipe needs "isp_copy_binary" (i.e. ISP Copy) when
@@ -6427,11 +6420,11 @@ load_yuvpp_binaries(struct ia_css_pipe *pipe)
 	 *   pp_defs.h" for the list of input-frame formats that are supported by the
 	 *   "yuv_scale_binary".
 	 */
-	need_isp_copy_binary =
-	    (pipe->stream->config.input_config.format == ATOMISP_INPUT_FORMAT_YUV422_8);
-#else  /* !ISP2401 */
-	need_isp_copy_binary = true;
-#endif /*  ISP2401 */
+	if (IS_ISP2401)
+		need_isp_copy_binary =
+		    (pipe->stream->config.input_config.format == ATOMISP_INPUT_FORMAT_YUV422_8);
+	else
+		need_isp_copy_binary = true;
 
 	if (need_isp_copy_binary) {
 		err = load_copy_binary(pipe,
@@ -6678,12 +6671,10 @@ create_host_yuvpp_pipeline(struct ia_css_pipe *pipe)
 	struct ia_css_frame *vf_frame[IA_CSS_PIPE_MAX_OUTPUT_STAGE];
 	struct ia_css_pipeline_stage_desc stage_desc;
 	bool need_in_frameinfo_memory = false;
-#ifdef ISP2401
 	bool sensor = false;
 	bool buffered_sensor = false;
 	bool online = false;
 	bool continuous = false;
-#endif
 
 	IA_CSS_ENTER_PRIVATE("pipe = %p", pipe);
 	if ((!pipe) || (!pipe->stream) || (pipe->mode != IA_CSS_PIPE_ID_YUVPP)) {
@@ -6700,24 +6691,23 @@ create_host_yuvpp_pipeline(struct ia_css_pipe *pipe)
 	num_stage  = pipe->pipe_settings.yuvpp.num_yuv_scaler;
 	num_output_stage   = pipe->pipe_settings.yuvpp.num_output;
 
-#ifdef ISP2401
-	/*
-	 * When the input system is 2401, always enable 'in_frameinfo_memory'
-	 * except for the following:
-	 * - Direct Sensor Mode Online Capture
-	 * - Direct Sensor Mode Continuous Capture
-	 * - Buffered Sensor Mode Continuous Capture
-	 */
-	sensor = pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR;
-	buffered_sensor = pipe->stream->config.mode == IA_CSS_INPUT_MODE_BUFFERED_SENSOR;
-	online = pipe->stream->config.online;
-	continuous = pipe->stream->config.continuous;
-	need_in_frameinfo_memory =
-	!((sensor && (online || continuous)) || (buffered_sensor && continuous));
-#else
-	/* Construct in_frame info (only in case we have dynamic input */
-	need_in_frameinfo_memory = pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY;
-#endif
+	if (IS_ISP2401) {
+		/*
+		 * When the input system is 2401, always enable 'in_frameinfo_memory'
+		 * except for the following:
+		 * - Direct Sensor Mode Online Capture
+		 * - Direct Sensor Mode Continuous Capture
+		 * - Buffered Sensor Mode Continuous Capture
+		 */
+		sensor = pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR;
+		buffered_sensor = pipe->stream->config.mode == IA_CSS_INPUT_MODE_BUFFERED_SENSOR;
+		online = pipe->stream->config.online;
+		continuous = pipe->stream->config.continuous;
+		need_in_frameinfo_memory =
+		!((sensor && (online || continuous)) || (buffered_sensor && continuous));
+	} else
+		/* Construct in_frame info (only in case we have dynamic input */
+		need_in_frameinfo_memory = pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY;
 	/*
 	 * the input frame can come from:
 	 *
@@ -6808,11 +6798,11 @@ create_host_yuvpp_pipeline(struct ia_css_pipe *pipe)
 	if (pipe->pipe_settings.yuvpp.copy_binary.info) {
 		struct ia_css_frame *in_frame_local = NULL;
 
-#ifdef ISP2401
-		/* After isp copy is enabled in_frame needs to be passed. */
-		if (!online)
-			in_frame_local = in_frame;
-#endif
+		if (IS_ISP2401) {
+			/* After isp copy is enabled in_frame needs to be passed. */
+			if (!online)
+				in_frame_local = in_frame;
+		}
 
 		if (need_scaler) {
 			ia_css_pipe_util_set_output_frames(bin_out_frame,
@@ -7031,12 +7021,10 @@ create_host_regular_capture_pipeline(struct ia_css_pipe *pipe)
 	struct ia_css_frame *vf_frame;
 	struct ia_css_pipeline_stage_desc stage_desc;
 	bool need_in_frameinfo_memory = false;
-#ifdef ISP2401
 	bool sensor = false;
 	bool buffered_sensor = false;
 	bool online = false;
 	bool continuous = false;
-#endif
 	unsigned int i, num_yuv_scaler, num_primary_stage;
 	bool need_yuv_pp = false;
 	bool *is_output_stage = NULL;
@@ -7054,25 +7042,26 @@ create_host_regular_capture_pipeline(struct ia_css_pipe *pipe)
 	ia_css_pipeline_clean(me);
 	ia_css_pipe_util_create_output_frames(out_frames);
 
-#ifdef ISP2401
-	/*
-	 * When the input system is 2401, always enable 'in_frameinfo_memory'
-	 * except for the following:
-	 * - Direct Sensor Mode Online Capture
-	 * - Direct Sensor Mode Online Capture
-	 * - Direct Sensor Mode Continuous Capture
-	 * - Buffered Sensor Mode Continuous Capture
-	 */
-	sensor = (pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR);
-	buffered_sensor = (pipe->stream->config.mode == IA_CSS_INPUT_MODE_BUFFERED_SENSOR);
-	online = pipe->stream->config.online;
-	continuous = pipe->stream->config.continuous;
-	need_in_frameinfo_memory =
-	!((sensor && (online || continuous)) || (buffered_sensor && (online || continuous)));
-#else
-	/* Construct in_frame info (only in case we have dynamic input */
-	need_in_frameinfo_memory = pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY;
-#endif
+	if (IS_ISP2401) {
+		/*
+		 * When the input system is 2401, always enable 'in_frameinfo_memory'
+		 * except for the following:
+		 * - Direct Sensor Mode Online Capture
+		 * - Direct Sensor Mode Online Capture
+		 * - Direct Sensor Mode Continuous Capture
+		 * - Buffered Sensor Mode Continuous Capture
+		 */
+		sensor = (pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR);
+		buffered_sensor = (pipe->stream->config.mode == IA_CSS_INPUT_MODE_BUFFERED_SENSOR);
+		online = pipe->stream->config.online;
+		continuous = pipe->stream->config.continuous;
+		need_in_frameinfo_memory =
+		!((sensor && (online || continuous)) || (buffered_sensor &&
+							(online || continuous)));
+	} else
+		/* Construct in_frame info (only in case we have dynamic input */
+		need_in_frameinfo_memory = pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY;
+
 	if (need_in_frameinfo_memory) {
 		err = init_in_frameinfo_memory_defaults(pipe, &me->in_frame,
 							IA_CSS_FRAME_FORMAT_RAW);
@@ -7135,27 +7124,27 @@ create_host_regular_capture_pipeline(struct ia_css_pipe *pipe)
 	if (pipe->pipe_settings.capture.copy_binary.info) {
 		if (raw) {
 			ia_css_pipe_util_set_output_frames(out_frames, 0, out_frame);
-#if defined(ISP2401)
-			if (!continuous) {
-				ia_css_pipe_get_generic_stage_desc(&stage_desc,
-								   copy_binary,
-								   out_frames,
-								   in_frame,
-								   NULL);
+			if (IS_ISP2401) {
+				if (!continuous) {
+					ia_css_pipe_get_generic_stage_desc(&stage_desc,
+									   copy_binary,
+									   out_frames,
+									   in_frame,
+									   NULL);
+				} else {
+					in_frame = pipe->stream->last_pipe->continuous_frames[0];
+					ia_css_pipe_get_generic_stage_desc(&stage_desc,
+									   copy_binary,
+									   out_frames,
+									   in_frame,
+									   NULL);
+				}
 			} else {
-				in_frame = pipe->stream->last_pipe->continuous_frames[0];
 				ia_css_pipe_get_generic_stage_desc(&stage_desc,
 								   copy_binary,
 								   out_frames,
-								   in_frame,
-								   NULL);
+								   NULL, NULL);
 			}
-#else
-			ia_css_pipe_get_generic_stage_desc(&stage_desc,
-							   copy_binary,
-							   out_frames,
-							   NULL, NULL);
-#endif
 		} else {
 			ia_css_pipe_util_set_output_frames(out_frames, 0,
 							   in_frame);
@@ -7185,14 +7174,17 @@ create_host_regular_capture_pipeline(struct ia_css_pipe *pipe)
 				local_in_frame = in_frame;
 			else
 				local_in_frame = NULL;
-#ifndef ISP2401
-			if (!need_pp && (i == num_primary_stage - 1))
-#else
-			if (!need_pp && (i == num_primary_stage - 1) && !need_ldc)
-#endif
-				local_out_frame = out_frame;
-			else
+
+			if (!need_pp && (i == num_primary_stage - 1)) {
+				if (IS_ISP2401) {
+					if (!need_ldc)
+						local_out_frame = out_frame;
+				} else {
+					local_out_frame = out_frame;
+				}
+			} else {
 				local_out_frame = NULL;
+			}
 			ia_css_pipe_util_set_output_frames(out_frames, 0, local_out_frame);
 			/*
 			 * WARNING: The #if def flag has been added below as a
@@ -7401,22 +7393,22 @@ static int capture_start(struct ia_css_pipe *pipe)
 		}
 	}
 
-#if !defined(ISP2401)
-	/* old isys: need to send_mipi_frames() in all pipe modes */
-	err = send_mipi_frames(pipe);
-	if (err) {
-		IA_CSS_LEAVE_ERR_PRIVATE(err);
-		return err;
-	}
-#else
-	if (pipe->config.mode != IA_CSS_PIPE_MODE_COPY) {
+	if (IS_ISP2401) {
+		if (pipe->config.mode != IA_CSS_PIPE_MODE_COPY) {
+			err = send_mipi_frames(pipe);
+			if (err) {
+				IA_CSS_LEAVE_ERR_PRIVATE(err);
+				return err;
+			}
+		}
+	} else {
+		/* old isys: need to send_mipi_frames() in all pipe modes */
 		err = send_mipi_frames(pipe);
 		if (err) {
 			IA_CSS_LEAVE_ERR_PRIVATE(err);
 			return err;
 		}
 	}
-#endif
 
 	ia_css_pipeline_get_sp_thread_id(ia_css_pipe_get_pipe_num(pipe), &thread_id);
 	copy_ovrd = 1 << thread_id;
@@ -8123,24 +8115,22 @@ ia_css_stream_create(const struct ia_css_stream_config *stream_config,
 		return err;
 	}
 
-#if !defined(ISP2401)
-	/* We don't support metadata for JPEG stream, since they both use str2mem */
-	if (stream_config->input_config.format == ATOMISP_INPUT_FORMAT_BINARY_8 &&
-	    stream_config->metadata_config.resolution.height > 0) {
-		err = -EINVAL;
-		IA_CSS_LEAVE_ERR(err);
-		return err;
-	}
-#endif
-
-#ifdef ISP2401
-	if (stream_config->online && stream_config->pack_raw_pixels) {
-		IA_CSS_LOG("online and pack raw is invalid on input system 2401");
-		err = -EINVAL;
-		IA_CSS_LEAVE_ERR(err);
-		return err;
+	if (!IS_ISP2401) {
+		/* We don't support metadata for JPEG stream, since they both use str2mem */
+		if (stream_config->input_config.format == ATOMISP_INPUT_FORMAT_BINARY_8 &&
+		    stream_config->metadata_config.resolution.height > 0) {
+			err = -EINVAL;
+			IA_CSS_LEAVE_ERR(err);
+			return err;
+		}
+	} else {
+		if (stream_config->online && stream_config->pack_raw_pixels) {
+			IA_CSS_LOG("online and pack raw is invalid on input system 2401");
+			err = -EINVAL;
+			IA_CSS_LEAVE_ERR(err);
+			return err;
+		}
 	}
-#endif
 
 	ia_css_debug_pipe_graph_dump_stream_config(stream_config);
 
@@ -8223,19 +8213,17 @@ ia_css_stream_create(const struct ia_css_stream_config *stream_config,
 	/* take over stream config */
 	curr_stream->config = *stream_config;
 
-#if defined(ISP2401)
-	if (stream_config->mode == IA_CSS_INPUT_MODE_BUFFERED_SENSOR &&
-	    stream_config->online)
-		curr_stream->config.online = false;
-#endif
+	if (IS_ISP2401) {
+		if (stream_config->mode == IA_CSS_INPUT_MODE_BUFFERED_SENSOR &&
+		    stream_config->online)
+			curr_stream->config.online = false;
 
-#ifdef ISP2401
-	if (curr_stream->config.online) {
-		curr_stream->config.source.port.num_lanes =
-		    stream_config->source.port.num_lanes;
-		curr_stream->config.mode =  IA_CSS_INPUT_MODE_BUFFERED_SENSOR;
+		if (curr_stream->config.online) {
+			curr_stream->config.source.port.num_lanes =
+			    stream_config->source.port.num_lanes;
+			curr_stream->config.mode =  IA_CSS_INPUT_MODE_BUFFERED_SENSOR;
+		}
 	}
-#endif
 	/* in case driver doesn't configure init number of raw buffers, configure it here */
 	if (curr_stream->config.target_num_cont_raw_buf == 0)
 		curr_stream->config.target_num_cont_raw_buf = NUM_CONTINUOUS_FRAMES;
@@ -8665,6 +8653,7 @@ ia_css_stream_start(struct ia_css_stream *stream)
 		for (idx = 0; idx < IA_CSS_MIPI_SIZE_CHECK_MAX_NOF_ENTRIES_PER_PORT; idx++) {
 			sh_css_sp_group.config.mipi_sizes_for_check[port][idx] =
 			sh_css_get_mipi_sizes_for_check(port, idx);
+
 		}
 	}
 #endif
@@ -9162,11 +9151,10 @@ void ia_css_pipe_map_queue(struct ia_css_pipe *pipe, bool map)
 
 	ia_css_pipeline_get_sp_thread_id(pipe_num, &thread_id);
 
-#if defined(ISP2401)
-	need_input_queue = true;
-#else
-	need_input_queue = pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY;
-#endif
+	if (IS_ISP2401)
+		need_input_queue = true;
+	else
+		need_input_queue = pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY;
 
 	/* map required buffer queues to resources */
 	/* TODO: to be improved */
-- 
2.39.2

[PATCH 2/5] staging: media: atomisp: runtime: frame: remove #ifdef ISP2401

[Kate Hsuan]

The actions of ISP2401 and 2400 are determined at the runtime.

Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 .../media/atomisp/pci/runtime/frame/src/frame.c   | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/runtime/frame/src/frame.c b/drivers/staging/media/atomisp/pci/runtime/frame/src/frame.c
index 83bb42e05421..425e75f7dda7 100644
--- a/drivers/staging/media/atomisp/pci/runtime/frame/src/frame.c
+++ b/drivers/staging/media/atomisp/pci/runtime/frame/src/frame.c
@@ -601,9 +601,9 @@ static void frame_init_qplane6_planes(struct ia_css_frame *frame)
 
 static int frame_allocate_buffer_data(struct ia_css_frame *frame)
 {
-#ifdef ISP2401
-	IA_CSS_ENTER_LEAVE_PRIVATE("frame->data_bytes=%d\n", frame->data_bytes);
-#endif
+	if (IS_ISP2401)
+		IA_CSS_ENTER_LEAVE_PRIVATE("frame->data_bytes=%d\n", frame->data_bytes);
+
 	frame->data = hmm_alloc(frame->data_bytes);
 	if (frame->data == mmgr_NULL)
 		return -ENOMEM;
@@ -635,11 +635,10 @@ static int frame_allocate_with_data(struct ia_css_frame **frame,
 
 	if (err) {
 		kvfree(me);
-#ifndef ISP2401
-		return err;
-#else
-		me = NULL;
-#endif
+		if (IS_ISP2401)
+			me = NULL;
+		else
+			return err;
 	}
 
 	*frame = me;
-- 
2.39.2

[PATCH 3/5] staging: media: atomisp: sh_css_sp: Remove #ifdef ISP2401

[Kate Hsuan]

The actions of ISP2401 and 2400 will be determined at the runtime.

Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 drivers/staging/media/atomisp/pci/sh_css_sp.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/sh_css_sp.c b/drivers/staging/media/atomisp/pci/sh_css_sp.c
index 0dd58a7fe2cc..297e1b981720 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_sp.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_sp.c
@@ -952,12 +952,10 @@ sh_css_sp_init_stage(struct ia_css_binary *binary,
 		return 0;
 	}
 
-#if defined(ISP2401)
-	(void)continuous;
-	sh_css_sp_stage.deinterleaved = 0;
-#else
-	sh_css_sp_stage.deinterleaved = ((stage == 0) && continuous);
-#endif
+	if (IS_ISP2401)
+		sh_css_sp_stage.deinterleaved = 0;
+	else
+		sh_css_sp_stage.deinterleaved = ((stage == 0) && continuous);
 
 	initialize_stage_frames(&sh_css_sp_stage.frames);
 	/*
-- 
2.39.2

[PATCH 4/5] staging: media: atomisp: sh_css_firmware: determine firmware version at runtime

[Kate Hsuan]

The firmware version of ISP2401 and 2400 is determined at runtime.

Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 .../media/atomisp/pci/sh_css_firmware.c        | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/sh_css_firmware.c b/drivers/staging/media/atomisp/pci/sh_css_firmware.c
index e7ef578db8ab..49ee88fe151d 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_firmware.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_firmware.c
@@ -56,11 +56,8 @@ static struct firmware_header *firmware_header;
  * which will be replaced with the actual RELEASE_VERSION
  * during package generation. Please do not modify
  */
-#ifdef ISP2401
-static const char *release_version = STR(irci_stable_candrpv_0415_20150521_0458);
-#else
-static const char *release_version = STR(irci_stable_candrpv_0415_20150423_1753);
-#endif
+static const char *release_version_2401 = STR(irci_stable_candrpv_0415_20150521_0458);
+static const char *release_version_2400 = STR(irci_stable_candrpv_0415_20150423_1753);
 
 #define MAX_FW_REL_VER_NAME	300
 static char FW_rel_ver_name[MAX_FW_REL_VER_NAME] = "---";
@@ -191,8 +188,14 @@ sh_css_load_blob_info(const char *fw, const struct ia_css_fw_info *bi,
 bool
 sh_css_check_firmware_version(struct device *dev, const char *fw_data)
 {
+	const char *release_version;
 	struct sh_css_fw_bi_file_h *file_header;
 
+	if (IS_ISP2401)
+		release_version = release_version_2401;
+	else
+		release_version = release_version_2400;
+
 	firmware_header = (struct firmware_header *)fw_data;
 	file_header = &firmware_header->file_header;
 
@@ -225,6 +228,7 @@ sh_css_load_firmware(struct device *dev, const char *fw_data,
 		     unsigned int fw_size)
 {
 	unsigned int i;
+	const char *release_version;
 	struct ia_css_fw_info *binaries;
 	struct sh_css_fw_bi_file_h *file_header;
 	int ret;
@@ -234,6 +238,10 @@ sh_css_load_firmware(struct device *dev, const char *fw_data,
 	binaries = &firmware_header->binary_header;
 	strscpy(FW_rel_ver_name, file_header->version,
 		min(sizeof(FW_rel_ver_name), sizeof(file_header->version)));
+	if (IS_ISP2401)
+		release_version = release_version_2401;
+	else
+		release_version = release_version_2400;
 	ret = sh_css_check_firmware_version(dev, fw_data);
 	if (ret) {
 		IA_CSS_ERROR("CSS code version (%s) and firmware version (%s) mismatch!",
-- 
2.39.2

[PATCH 5/5] staging: media: atomisp: sh_css_mipi: Remove #ifdef 2041

[Kate Hsuan]

The actions of ISP2401 and 2400 are determined at the runtime.

Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 .../staging/media/atomisp/pci/sh_css_mipi.c   | 101 +++++++++---------
 1 file changed, 49 insertions(+), 52 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
index bc6e8598a776..9c9d3b27ded4 100644
--- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
+++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
@@ -67,13 +67,12 @@ ia_css_mipi_frame_calculate_size(const unsigned int width,
 	unsigned int mem_words = 0;
 	unsigned int width_padded = width;
 
-#if defined(ISP2401)
 	/* The changes will be reverted as soon as RAW
 	 * Buffers are deployed by the 2401 Input System
 	 * in the non-continuous use scenario.
 	 */
-	width_padded += (2 * ISP_VEC_NELEMS);
-#endif
+	if (IS_ISP2401)
+		width_padded += (2 * ISP_VEC_NELEMS);
 
 	IA_CSS_ENTER("padded_width=%d, height=%d, format=%d, hasSOLandEOL=%d, embedded_data_size_words=%d\n",
 		     width_padded, height, format, hasSOLandEOL, embedded_data_size_words);
@@ -235,7 +234,6 @@ bool mipi_is_free(void)
 	return true;
 }
 
-#if defined(ISP2401)
 /*
  * @brief Calculate the required MIPI buffer sizes.
  * Based on the stream configuration, calculate the
@@ -342,7 +340,6 @@ static int calculate_mipi_buff_size(struct ia_css_stream_config *stream_cfg,
 	IA_CSS_LEAVE_ERR(err);
 	return err;
 }
-#endif
 
 int
 allocate_mipi_frames(struct ia_css_pipe *pipe,
@@ -363,15 +360,15 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
 		return -EINVAL;
 	}
 
-#ifdef ISP2401
-	if (pipe->stream->config.online) {
-		ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
-				    "allocate_mipi_frames(%p) exit: no buffers needed for 2401 pipe mode.\n",
-				    pipe);
-		return 0;
+	if (IS_ISP2401) {
+		if (pipe->stream->config.online) {
+			ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
+					    "allocate_mipi_frames(%p) exit: no buffers needed for 2401 pipe mode.\n",
+					    pipe);
+			return 0;
+		}
 	}
 
-#endif
 	if (pipe->stream->config.mode != IA_CSS_INPUT_MODE_BUFFERED_SENSOR) {
 		ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
 				    "allocate_mipi_frames(%p) exit: no buffers needed for pipe mode.\n",
@@ -386,30 +383,30 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
 		return -EINVAL;
 	}
 
-#ifdef ISP2401
-	err = calculate_mipi_buff_size(&pipe->stream->config,
-				       &my_css.mipi_frame_size[port]);
-	/*
-	 * 2401 system allows multiple streams to use same physical port. This is not
-	 * true for 2400 system. Currently 2401 uses MIPI buffers as a temporary solution.
-	 * TODO AM: Once that is changed (removed) this code should be removed as well.
-	 * In that case only 2400 related code should remain.
-	 */
-	if (ref_count_mipi_allocation[port] != 0) {
-		ref_count_mipi_allocation[port]++;
-		ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
-				    "allocate_mipi_frames(%p) leave: nothing to do, already allocated for this port (port=%d).\n",
-				    pipe, port);
-		return 0;
-	}
-#else
-	if (ref_count_mipi_allocation[port] != 0) {
-		ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
-				    "allocate_mipi_frames(%p) exit: already allocated for this port (port=%d).\n",
-				    pipe, port);
-		return 0;
+	if (IS_ISP2401) {
+		err = calculate_mipi_buff_size(&pipe->stream->config,
+					       &my_css.mipi_frame_size[port]);
+		/*
+		 * 2401 system allows multiple streams to use same physical port. This is not
+		 * true for 2400 system. Currently 2401 uses MIPI buffers as a temporary solution.
+		 * TODO AM: Once that is changed (removed) this code should be removed as well.
+		 * In that case only 2400 related code should remain.
+		 */
+		if (ref_count_mipi_allocation[port] != 0) {
+			ref_count_mipi_allocation[port]++;
+			ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
+					    "allocate_mipi_frames(%p) leave: nothing to do, already allocated for this port (port=%d).\n",
+					    pipe, port);
+			return 0;
+		}
+	} else {
+		if (ref_count_mipi_allocation[port] != 0) {
+			ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
+					    "allocate_mipi_frames(%p) exit: already allocated for this port (port=%d).\n",
+					    pipe, port);
+			return 0;
+		}
 	}
-#endif
 
 	ref_count_mipi_allocation[port]++;
 
@@ -503,14 +500,14 @@ free_mipi_frames(struct ia_css_pipe *pipe)
 		}
 
 		if (ref_count_mipi_allocation[port] > 0) {
-#if !defined(ISP2401)
-			assert(ref_count_mipi_allocation[port] == 1);
-			if (ref_count_mipi_allocation[port] != 1) {
-				IA_CSS_ERROR("free_mipi_frames(%p) exit: wrong ref_count (ref_count=%d).",
-					     pipe, ref_count_mipi_allocation[port]);
-				return err;
+			if (!IS_ISP2401) {
+				assert(ref_count_mipi_allocation[port] == 1);
+				if (ref_count_mipi_allocation[port] != 1) {
+					IA_CSS_ERROR("free_mipi_frames(%p) exit: wrong ref_count (ref_count=%d).",
+						     pipe, ref_count_mipi_allocation[port]);
+					return err;
+				}
 			}
-#endif
 
 			ref_count_mipi_allocation[port]--;
 
@@ -534,18 +531,18 @@ free_mipi_frames(struct ia_css_pipe *pipe)
 				ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
 						    "free_mipi_frames(%p) exit (deallocated).\n", pipe);
 			}
-#if defined(ISP2401)
 			else {
-				/* 2401 system allows multiple streams to use same physical port. This is not
-				 * true for 2400 system. Currently 2401 uses MIPI buffers as a temporary solution.
-				 * TODO AM: Once that is changed (removed) this code should be removed as well.
-				 * In that case only 2400 related code should remain.
-				 */
-				ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
-						    "free_mipi_frames(%p) leave: nothing to do, other streams still use this port (port=%d).\n",
-						    pipe, port);
+				if (IS_ISP2401) {
+					/* 2401 system allows multiple streams to use same physical port. This is not
+					 * true for 2400 system. Currently 2401 uses MIPI buffers as a temporary solution.
+					 * TODO AM: Once that is changed (removed) this code should be removed as well.
+					 * In that case only 2400 related code should remain.
+					 */
+					ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
+							    "free_mipi_frames(%p) leave: nothing to do, other streams still use this port (port=%d).\n",
+							    pipe, port);
+				}
 			}
-#endif
 		}
 	} else { /* pipe ==NULL */
 		/* AM TEMP: free-ing all mipi buffers just like a legacy code. */
-- 
2.39.2

Re: [PATCH 1/5] staging: media: atomisp: sh_css: Remove #ifdef ISP2401

[Hans de Goede]

Hi Kate,

Thank you for your work on this!

A few small remarks below / inline:

On 4/25/23 09:48, Kate Hsuan wrote:
> The actions of ISP2401 and 2400 are determined at the runtime.
> 
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---
>  drivers/staging/media/atomisp/pci/sh_css.c | 584 ++++++++++-----------
>  1 file changed, 286 insertions(+), 298 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/sh_css.c b/drivers/staging/media/atomisp/pci/sh_css.c
> index 93789500416f..0cc008b3c921 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css.c
> +++ b/drivers/staging/media/atomisp/pci/sh_css.c
> @@ -1529,15 +1529,14 @@ ia_css_init(struct device *dev, const struct ia_css_env *env,
>  
>  	mipi_init();
>  
> -#ifndef ISP2401
>  	/*
>  	 * In case this has been programmed already, update internal
>  	 * data structure ...
>  	 * DEPRECATED
>  	 */
> -	my_css.page_table_base_index = mmu_get_page_table_base_index(MMU0_ID);
> +	if (!IS_ISP2401)
> +		my_css.page_table_base_index = mmu_get_page_table_base_index(MMU0_ID);
>  
> -#endif
>  	my_css.irq_type = irq_type;
>  
>  	my_css_save.irq_type = irq_type;
> @@ -1596,10 +1595,8 @@ ia_css_init(struct device *dev, const struct ia_css_env *env,
>  	 * sh_css_init_buffer_queues();
>  	 */
>  
> -#if defined(ISP2401)
> -	gp_device_reg_store(GP_DEVICE0_ID, _REG_GP_SWITCH_ISYS2401_ADDR, 1);
> -#endif
> -
> +	if (IS_ISP2401)
> +		gp_device_reg_store(GP_DEVICE0_ID, _REG_GP_SWITCH_ISYS2401_ADDR, 1);
>  
>  	if (!IS_ISP2401)
>  		dma_set_max_burst_size(DMA0_ID, HIVE_DMA_BUS_DDR_CONN,
> @@ -2128,13 +2125,13 @@ ia_css_pipe_destroy(struct ia_css_pipe *pipe)
>  						    err);
>  			}
>  		}
> -#ifndef ISP2401
> -		ia_css_frame_free_multiple(NUM_VIDEO_TNR_FRAMES,
> -					   pipe->pipe_settings.video.tnr_frames);
> -#else
> -		ia_css_frame_free_multiple(NUM_VIDEO_TNR_FRAMES,
> -					   pipe->pipe_settings.video.tnr_frames);
> -#endif
> +		if (IS_ISP2401)
> +			ia_css_frame_free_multiple(NUM_VIDEO_TNR_FRAMES,
> +						   pipe->pipe_settings.video.tnr_frames);
> +		else
> +			ia_css_frame_free_multiple(NUM_VIDEO_TNR_FRAMES,
> +						   pipe->pipe_settings.video.tnr_frames);
> +

Unless I am missing something here the if and else branches
contain the same statement here (and so did the original code)
so I think this can if ...  else ... can be simplified to just:

		ia_css_frame_free_multiple(NUM_VIDEO_TNR_FRAMES,
					   pipe->pipe_settings.video.tnr_frames);

?

<snip (+ more snips below)>

> @@ -2514,11 +2515,11 @@ load_preview_binaries(struct ia_css_pipe *pipe)
>  	assert(pipe->mode == IA_CSS_PIPE_ID_PREVIEW);
>  
>  	online = pipe->stream->config.online;
> -#ifdef ISP2401
> -	sensor = pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR;
> -#else
> -	continuous = pipe->stream->config.continuous;
> -#endif
> +
> +	if (IS_ISP2401)
> +		sensor = pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR;
> +	else
> +		continuous = pipe->stream->config.continuous;

You can drop the if ... else here and just always initialize both
helper variables:

	sensor = pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR;
	continuous = pipe->stream->config.continuous;


>  
>  	if (mycs->preview_binary.info)
>  		return 0;
> @@ -2627,24 +2628,26 @@ load_preview_binaries(struct ia_css_pipe *pipe)
>  			return err;
>  	}
>  
> -#ifdef ISP2401
> -	/*
> -	 * When the input system is 2401, only the Direct Sensor Mode
> -	 * Offline Preview uses the ISP copy binary.
> -	 */
> -	need_isp_copy_binary = !online && sensor;
> -#else
> -	/*
> -	 * About pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY:
> -	 * This is typical the case with SkyCam (which has no input system) but it also applies to all cases
> -	 * where the driver chooses for memory based input frames. In these cases, a copy binary (which typical
> -	 * copies sensor data to DDR) does not have much use.
> -	 */
> -	if (!IS_ISP2401)
> -		need_isp_copy_binary = !online && !continuous;
> +	if (IS_ISP2401)
> +		/*
> +		 * When the input system is 2401, only the Direct Sensor Mode
> +		 * Offline Preview uses the ISP copy binary.
> +		 */
> +		need_isp_copy_binary = !online && sensor;
>  	else
> -		need_isp_copy_binary = !online && !continuous && !(pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY);
> -#endif
> +		/*
> +		 * About pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY:
> +		 * This is typical the case with SkyCam (which has no input system) but it also
> +		 * applies to all cases where the driver chooses for memory based input frames.
> +		 * In these cases, a copy binary (which typical copies sensor data to DDR) does
> +		 * not have much use.
> +		 */
> +		if (!IS_ISP2401)
> +			need_isp_copy_binary = !online && !continuous;
> +		else
> +			need_isp_copy_binary = !online && !continuous &&
> +					       !(pipe->stream->config.mode ==
> +						 IA_CSS_INPUT_MODE_MEMORY);
>  
>  	/* Copy */
>  	if (need_isp_copy_binary) {

The if (!IS_ISP2401) is always true here since we are in an else
branch of the "if (IS_ISP2401)" check above (this was already the
case in the old code with the #ifdef, but no-one noticed sofar).

So the else branch of the nested if never gets run and can be
removed. And since the comment is only about the else branch
it can be removed too, so this entire block / chunk can be
similified to:

	if (IS_ISP2401) {
		/*
		 * When the input system is 2401, only the Direct Sensor Mode
		 * Offline Preview uses the ISP copy binary.
		 */
		need_isp_copy_binary = !online && sensor;
  	} else {
		need_isp_copy_binary = !online && !continuous;
	}



> @@ -3125,11 +3128,11 @@ init_in_frameinfo_memory_defaults(struct ia_css_pipe *pipe,
>  
>  	in_frame->frame_info.format = format;
>  
> -#ifdef ISP2401
> -	if (format == IA_CSS_FRAME_FORMAT_RAW)
> -		in_frame->frame_info.format = (pipe->stream->config.pack_raw_pixels) ?
> -		IA_CSS_FRAME_FORMAT_RAW_PACKED : IA_CSS_FRAME_FORMAT_RAW;
> -#endif
> +	if (IS_ISP2401) {
> +		if (format == IA_CSS_FRAME_FORMAT_RAW)
> +			in_frame->frame_info.format = (pipe->stream->config.pack_raw_pixels) ?
> +			IA_CSS_FRAME_FORMAT_RAW_PACKED : IA_CSS_FRAME_FORMAT_RAW;
> +	}
>  
>  	in_frame->frame_info.res.width = pipe->stream->config.input_config.input_res.width;
>  	in_frame->frame_info.res.height = pipe->stream->config.input_config.input_res.height;

Please change this to:

	if (IS_ISP2401 && format == IA_CSS_FRAME_FORMAT_RAW) {
		in_frame->frame_info.format = (pipe->stream->config.pack_raw_pixels) ?
		IA_CSS_FRAME_FORMAT_RAW_PACKED : IA_CSS_FRAME_FORMAT_RAW;
	}

to avoid unnecessary if nesting.

> @@ -3141,9 +3144,12 @@ init_in_frameinfo_memory_defaults(struct ia_css_pipe *pipe,
>  	ia_css_query_internal_queue_id(IA_CSS_BUFFER_TYPE_INPUT_FRAME, thread_id, &queue_id);
>  	in_frame->dynamic_queue_id = queue_id;
>  	in_frame->buf_type = IA_CSS_BUFFER_TYPE_INPUT_FRAME;
> +
>  #ifdef ISP2401
> +
>  	ia_css_get_crop_offsets(pipe, &in_frame->frame_info);
>  #endif
> +
>  	err = ia_css_frame_init_planes(in_frame);
>  
>  	ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE, "%s() bayer_order = %d\n",

You are only adding whitespace here? I guess the intent was to
actually replace the #ifdef with a if (IS_ISP2401) ?

> @@ -3417,9 +3420,9 @@ create_host_preview_pipeline(struct ia_css_pipe *pipe)
>  			goto ERR;
>  
>  		in_frame = &me->in_frame;
> -	} else {
> +	} else
>  		in_frame = NULL;
> -	}
> +
>  
>  	err = init_out_frameinfo_defaults(pipe, &me->out_frame[0], 0);
>  	if (err)

Unrelated coding style change, please drop.

> @@ -5251,12 +5247,12 @@ static int load_primary_binaries(
>  	       pipe->mode == IA_CSS_PIPE_ID_COPY);
>  
>  	online = pipe->stream->config.online;
> -#ifdef ISP2401
> -	sensor = (pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR);
> -#else
> -	memory = pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY;
> -	continuous = pipe->stream->config.continuous;
> -#endif
> +	if (IS_ISP2401) {
> +		sensor = (pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR);
> +	} else {
> +		memory = pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY;
> +		continuous = pipe->stream->config.continuous;
> +	}

As above, just always initialize all helper variables like these:

	sensor = (pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR);
	memory = pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY;
	continuous = pipe->stream->config.continuous;

> @@ -6808,11 +6798,11 @@ create_host_yuvpp_pipeline(struct ia_css_pipe *pipe)
>  	if (pipe->pipe_settings.yuvpp.copy_binary.info) {
>  		struct ia_css_frame *in_frame_local = NULL;
>  
> -#ifdef ISP2401
> -		/* After isp copy is enabled in_frame needs to be passed. */
> -		if (!online)
> -			in_frame_local = in_frame;
> -#endif
> +		if (IS_ISP2401) {
> +			/* After isp copy is enabled in_frame needs to be passed. */
> +			if (!online)
> +				in_frame_local = in_frame;
> +		}

Please && the 2 conditions together to avoid unnecessary if nesting:

		if (IS_ISP2401 && !online) {
			/* After isp copy is enabled in_frame needs to be passed. */
			in_frame_local = in_frame;
		}

> @@ -7054,25 +7042,26 @@ create_host_regular_capture_pipeline(struct ia_css_pipe *pipe)
>  	ia_css_pipeline_clean(me);
>  	ia_css_pipe_util_create_output_frames(out_frames);
>  
> -#ifdef ISP2401
> -	/*
> -	 * When the input system is 2401, always enable 'in_frameinfo_memory'
> -	 * except for the following:
> -	 * - Direct Sensor Mode Online Capture
> -	 * - Direct Sensor Mode Online Capture
> -	 * - Direct Sensor Mode Continuous Capture
> -	 * - Buffered Sensor Mode Continuous Capture
> -	 */
> -	sensor = (pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR);
> -	buffered_sensor = (pipe->stream->config.mode == IA_CSS_INPUT_MODE_BUFFERED_SENSOR);
> -	online = pipe->stream->config.online;
> -	continuous = pipe->stream->config.continuous;
> -	need_in_frameinfo_memory =
> -	!((sensor && (online || continuous)) || (buffered_sensor && (online || continuous)));
> -#else
> -	/* Construct in_frame info (only in case we have dynamic input */
> -	need_in_frameinfo_memory = pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY;
> -#endif
> +	if (IS_ISP2401) {
> +		/*
> +		 * When the input system is 2401, always enable 'in_frameinfo_memory'
> +		 * except for the following:
> +		 * - Direct Sensor Mode Online Capture
> +		 * - Direct Sensor Mode Online Capture
> +		 * - Direct Sensor Mode Continuous Capture
> +		 * - Buffered Sensor Mode Continuous Capture
> +		 */
> +		sensor = (pipe->stream->config.mode == IA_CSS_INPUT_MODE_SENSOR);
> +		buffered_sensor = (pipe->stream->config.mode == IA_CSS_INPUT_MODE_BUFFERED_SENSOR);
> +		online = pipe->stream->config.online;
> +		continuous = pipe->stream->config.continuous;
> +		need_in_frameinfo_memory =
> +		!((sensor && (online || continuous)) || (buffered_sensor &&
> +							(online || continuous)));
> +	} else
> +		/* Construct in_frame info (only in case we have dynamic input */
> +		need_in_frameinfo_memory = pipe->stream->config.mode == IA_CSS_INPUT_MODE_MEMORY;
> +

When one branch of an if...else... block needs {} please add {}
to both branches (so also add it to the else here).


> @@ -7185,14 +7174,17 @@ create_host_regular_capture_pipeline(struct ia_css_pipe *pipe)
>  				local_in_frame = in_frame;
>  			else
>  				local_in_frame = NULL;
> -#ifndef ISP2401
> -			if (!need_pp && (i == num_primary_stage - 1))
> -#else
> -			if (!need_pp && (i == num_primary_stage - 1) && !need_ldc)
> -#endif
> -				local_out_frame = out_frame;
> -			else
> +
> +			if (!need_pp && (i == num_primary_stage - 1)) {
> +				if (IS_ISP2401) {
> +					if (!need_ldc)
> +						local_out_frame = out_frame;

this is missing a:

					else
						local_out_frame = NULL;

> +				} else {
> +					local_out_frame = out_frame;
> +				}
> +			} else {
>  				local_out_frame = NULL;
> +			}

Rather then adding the missing else, I think it would be
better to just fold this all into:

			if (!need_pp && (i == num_primary_stage - 1) && (!IS_ISP2401 || !need_ldc))
				local_out_frame = out_frame;
			else
				local_out_frame = NULL;

?

>  			ia_css_pipe_util_set_output_frames(out_frames, 0, local_out_frame);
>  			/*
>  			 * WARNING: The #if def flag has been added below as a
> @@ -7401,22 +7393,22 @@ static int capture_start(struct ia_css_pipe *pipe)
>  		}
>  	}
>  
> -#if !defined(ISP2401)
> -	/* old isys: need to send_mipi_frames() in all pipe modes */
> -	err = send_mipi_frames(pipe);
> -	if (err) {
> -		IA_CSS_LEAVE_ERR_PRIVATE(err);
> -		return err;
> -	}
> -#else
> -	if (pipe->config.mode != IA_CSS_PIPE_MODE_COPY) {
> +	if (IS_ISP2401) {
> +		if (pipe->config.mode != IA_CSS_PIPE_MODE_COPY) {
> +			err = send_mipi_frames(pipe);
> +			if (err) {
> +				IA_CSS_LEAVE_ERR_PRIVATE(err);
> +				return err;
> +			}
> +		}
> +	} else {
> +		/* old isys: need to send_mipi_frames() in all pipe modes */
>  		err = send_mipi_frames(pipe);
>  		if (err) {
>  			IA_CSS_LEAVE_ERR_PRIVATE(err);
>  			return err;
>  		}
>  	}
> -#endif
>  
>  	ia_css_pipeline_get_sp_thread_id(ia_css_pipe_get_pipe_num(pipe), &thread_id);
>  	copy_ovrd = 1 << thread_id;

Maybe simplify this entire block to:

	/* old isys: need to send_mipi_frames() in all pipe modes */
	if (!IS_ISP2401 || !pipe->config.mode != IA_CSS_PIPE_MODE_COPY) {
		if (pipe->config.mode != IA_CSS_PIPE_MODE_COPY) {
			err = send_mipi_frames(pipe);
			if (err) {
				IA_CSS_LEAVE_ERR_PRIVATE(err);
				return err;
			}
		}
	}

?

> @@ -8665,6 +8653,7 @@ ia_css_stream_start(struct ia_css_stream *stream)
>  		for (idx = 0; idx < IA_CSS_MIPI_SIZE_CHECK_MAX_NOF_ENTRIES_PER_PORT; idx++) {
>  			sh_css_sp_group.config.mipi_sizes_for_check[port][idx] =
>  			sh_css_get_mipi_sizes_for_check(port, idx);
> +
>  		}
>  	}
>  #endif

Unrelated whitespace chage, please drop.

Regards,

Hans

Re: [PATCH 2/5] staging: media: atomisp: runtime: frame: remove #ifdef ISP2401

[Hans de Goede]

Hi Kate,

On 4/25/23 09:48, Kate Hsuan wrote:
> The actions of ISP2401 and 2400 are determined at the runtime.
> 
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---
>  .../media/atomisp/pci/runtime/frame/src/frame.c   | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/runtime/frame/src/frame.c b/drivers/staging/media/atomisp/pci/runtime/frame/src/frame.c
> index 83bb42e05421..425e75f7dda7 100644
> --- a/drivers/staging/media/atomisp/pci/runtime/frame/src/frame.c
> +++ b/drivers/staging/media/atomisp/pci/runtime/frame/src/frame.c
> @@ -601,9 +601,9 @@ static void frame_init_qplane6_planes(struct ia_css_frame *frame)
>  
>  static int frame_allocate_buffer_data(struct ia_css_frame *frame)
>  {
> -#ifdef ISP2401
> -	IA_CSS_ENTER_LEAVE_PRIVATE("frame->data_bytes=%d\n", frame->data_bytes);
> -#endif
> +	if (IS_ISP2401)
> +		IA_CSS_ENTER_LEAVE_PRIVATE("frame->data_bytes=%d\n", frame->data_bytes);
> +
>  	frame->data = hmm_alloc(frame->data_bytes);
>  	if (frame->data == mmgr_NULL)
>  		return -ENOMEM;

This is just a debug log message, IMHO it is best to just completely remove
the message for both ISP models.


> @@ -635,11 +635,10 @@ static int frame_allocate_with_data(struct ia_css_frame **frame,
>  
>  	if (err) {
>  		kvfree(me);
> -#ifndef ISP2401
> -		return err;
> -#else
> -		me = NULL;
> -#endif
> +		if (IS_ISP2401)
> +			me = NULL;
> +		else
> +			return err;
>  	}
>  
>  	*frame = me;

This one is also weird. I have checked the only caller of this and it
does not matter what *frame is set to since as long as this returns
non 0 the *frame is ignored and the functions always returns err
(just outside of the context of the patch).

So this can be simplified to just:

	if (err) {
		kvfree(me);
		me = NULL;
	}

And then fall through to the original code of:

  	*frame = me;

	return err;
}


The me = NULL is not strictly necessary but setting *frame = NULL
on errors is a bit cleaner and may help catch future bugs.

Regards,

Hans

Re: [PATCH 3/5] staging: media: atomisp: sh_css_sp: Remove #ifdef ISP2401

[Hans de Goede]

Hi,

On 4/25/23 09:48, Kate Hsuan wrote:
> The actions of ISP2401 and 2400 will be determined at the runtime.
> 
> Signed-off-by: Kate Hsuan <hpa@redhat.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  drivers/staging/media/atomisp/pci/sh_css_sp.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/sh_css_sp.c b/drivers/staging/media/atomisp/pci/sh_css_sp.c
> index 0dd58a7fe2cc..297e1b981720 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css_sp.c
> +++ b/drivers/staging/media/atomisp/pci/sh_css_sp.c
> @@ -952,12 +952,10 @@ sh_css_sp_init_stage(struct ia_css_binary *binary,
>  		return 0;
>  	}
>  
> -#if defined(ISP2401)
> -	(void)continuous;
> -	sh_css_sp_stage.deinterleaved = 0;
> -#else
> -	sh_css_sp_stage.deinterleaved = ((stage == 0) && continuous);
> -#endif
> +	if (IS_ISP2401)
> +		sh_css_sp_stage.deinterleaved = 0;
> +	else
> +		sh_css_sp_stage.deinterleaved = ((stage == 0) && continuous);
>  
>  	initialize_stage_frames(&sh_css_sp_stage.frames);
>  	/*

Re: [PATCH 4/5] staging: media: atomisp: sh_css_firmware: determine firmware version at runtime

[Hans de Goede]

Hi,

On 4/25/23 09:48, Kate Hsuan wrote:
> The firmware version of ISP2401 and 2400 is determined at runtime.
> 
> Signed-off-by: Kate Hsuan <hpa@redhat.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  .../media/atomisp/pci/sh_css_firmware.c        | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/sh_css_firmware.c b/drivers/staging/media/atomisp/pci/sh_css_firmware.c
> index e7ef578db8ab..49ee88fe151d 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css_firmware.c
> +++ b/drivers/staging/media/atomisp/pci/sh_css_firmware.c
> @@ -56,11 +56,8 @@ static struct firmware_header *firmware_header;
>   * which will be replaced with the actual RELEASE_VERSION
>   * during package generation. Please do not modify
>   */
> -#ifdef ISP2401
> -static const char *release_version = STR(irci_stable_candrpv_0415_20150521_0458);
> -#else
> -static const char *release_version = STR(irci_stable_candrpv_0415_20150423_1753);
> -#endif
> +static const char *release_version_2401 = STR(irci_stable_candrpv_0415_20150521_0458);
> +static const char *release_version_2400 = STR(irci_stable_candrpv_0415_20150423_1753);
>  
>  #define MAX_FW_REL_VER_NAME	300
>  static char FW_rel_ver_name[MAX_FW_REL_VER_NAME] = "---";
> @@ -191,8 +188,14 @@ sh_css_load_blob_info(const char *fw, const struct ia_css_fw_info *bi,
>  bool
>  sh_css_check_firmware_version(struct device *dev, const char *fw_data)
>  {
> +	const char *release_version;
>  	struct sh_css_fw_bi_file_h *file_header;
>  
> +	if (IS_ISP2401)
> +		release_version = release_version_2401;
> +	else
> +		release_version = release_version_2400;
> +
>  	firmware_header = (struct firmware_header *)fw_data;
>  	file_header = &firmware_header->file_header;
>  
> @@ -225,6 +228,7 @@ sh_css_load_firmware(struct device *dev, const char *fw_data,
>  		     unsigned int fw_size)
>  {
>  	unsigned int i;
> +	const char *release_version;
>  	struct ia_css_fw_info *binaries;
>  	struct sh_css_fw_bi_file_h *file_header;
>  	int ret;
> @@ -234,6 +238,10 @@ sh_css_load_firmware(struct device *dev, const char *fw_data,
>  	binaries = &firmware_header->binary_header;
>  	strscpy(FW_rel_ver_name, file_header->version,
>  		min(sizeof(FW_rel_ver_name), sizeof(file_header->version)));
> +	if (IS_ISP2401)
> +		release_version = release_version_2401;
> +	else
> +		release_version = release_version_2400;
>  	ret = sh_css_check_firmware_version(dev, fw_data);
>  	if (ret) {
>  		IA_CSS_ERROR("CSS code version (%s) and firmware version (%s) mismatch!",

Re: [PATCH 5/5] staging: media: atomisp: sh_css_mipi: Remove #ifdef 2041

[Hans de Goede]

Hi Kate,

On 4/25/23 09:48, Kate Hsuan wrote:
> The actions of ISP2401 and 2400 are determined at the runtime.
> 
> Signed-off-by: Kate Hsuan <hpa@redhat.com>
> ---
>  .../staging/media/atomisp/pci/sh_css_mipi.c   | 101 +++++++++---------
>  1 file changed, 49 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> index bc6e8598a776..9c9d3b27ded4 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c

<snip>

> @@ -363,15 +360,15 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
>  		return -EINVAL;
>  	}
>  
> -#ifdef ISP2401
> -	if (pipe->stream->config.online) {
> -		ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> -				    "allocate_mipi_frames(%p) exit: no buffers needed for 2401 pipe mode.\n",
> -				    pipe);
> -		return 0;
> +	if (IS_ISP2401) {
> +		if (pipe->stream->config.online) {
> +			ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> +					    "allocate_mipi_frames(%p) exit: no buffers needed for 2401 pipe mode.\n",
> +					    pipe);
> +			return 0;
> +		}
>  	}
>  
> -#endif
>  	if (pipe->stream->config.mode != IA_CSS_INPUT_MODE_BUFFERED_SENSOR) {
>  		ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
>  				    "allocate_mipi_frames(%p) exit: no buffers needed for pipe mode.\n",

Please combine the 2 conditions with && instead of using nested if-s:

	if (IS_ISP2401 && pipe->stream->config.online) {
		ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
				    "allocate_mipi_frames(%p) exit: no buffers needed for 2401 pipe mode.\n",
				    pipe);
		return 0;
	}


> @@ -386,30 +383,30 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
>  		return -EINVAL;
>  	}
>  
> -#ifdef ISP2401
> -	err = calculate_mipi_buff_size(&pipe->stream->config,
> -				       &my_css.mipi_frame_size[port]);
> -	/*
> -	 * 2401 system allows multiple streams to use same physical port. This is not
> -	 * true for 2400 system. Currently 2401 uses MIPI buffers as a temporary solution.
> -	 * TODO AM: Once that is changed (removed) this code should be removed as well.
> -	 * In that case only 2400 related code should remain.
> -	 */
> -	if (ref_count_mipi_allocation[port] != 0) {
> -		ref_count_mipi_allocation[port]++;
> -		ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> -				    "allocate_mipi_frames(%p) leave: nothing to do, already allocated for this port (port=%d).\n",
> -				    pipe, port);
> -		return 0;
> -	}
> -#else
> -	if (ref_count_mipi_allocation[port] != 0) {
> -		ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> -				    "allocate_mipi_frames(%p) exit: already allocated for this port (port=%d).\n",
> -				    pipe, port);
> -		return 0;
> +	if (IS_ISP2401) {
> +		err = calculate_mipi_buff_size(&pipe->stream->config,
> +					       &my_css.mipi_frame_size[port]);
> +		/*
> +		 * 2401 system allows multiple streams to use same physical port. This is not
> +		 * true for 2400 system. Currently 2401 uses MIPI buffers as a temporary solution.
> +		 * TODO AM: Once that is changed (removed) this code should be removed as well.
> +		 * In that case only 2400 related code should remain.
> +		 */
> +		if (ref_count_mipi_allocation[port] != 0) {
> +			ref_count_mipi_allocation[port]++;
> +			ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> +					    "allocate_mipi_frames(%p) leave: nothing to do, already allocated for this port (port=%d).\n",
> +					    pipe, port);
> +			return 0;
> +		}
> +	} else {
> +		if (ref_count_mipi_allocation[port] != 0) {
> +			ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> +					    "allocate_mipi_frames(%p) exit: already allocated for this port (port=%d).\n",
> +					    pipe, port);
> +			return 0;
> +		}
>  	}
> -#endif

The "if (ref_count_mipi_allocation[port] != 0) { log-message; return 0; }"
block is shared between the ISP2400 and ISP2401. Except for one debug log
message using "leave" and the other using "exit" these 2 blocks are 100%
the same, so please pick one of the 2 log messages and move this block
out of the if (IS_ISP2401) {} block (and drop the else branch).

> @@ -534,18 +531,18 @@ free_mipi_frames(struct ia_css_pipe *pipe)
>  				ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
>  						    "free_mipi_frames(%p) exit (deallocated).\n", pipe);
>  			}
> -#if defined(ISP2401)
>  			else {
> -				/* 2401 system allows multiple streams to use same physical port. This is not
> -				 * true for 2400 system. Currently 2401 uses MIPI buffers as a temporary solution.
> -				 * TODO AM: Once that is changed (removed) this code should be removed as well.
> -				 * In that case only 2400 related code should remain.
> -				 */
> -				ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> -						    "free_mipi_frames(%p) leave: nothing to do, other streams still use this port (port=%d).\n",
> -						    pipe, port);
> +				if (IS_ISP2401) {
> +					/* 2401 system allows multiple streams to use same physical port. This is not
> +					 * true for 2400 system. Currently 2401 uses MIPI buffers as a temporary solution.
> +					 * TODO AM: Once that is changed (removed) this code should be removed as well.
> +					 * In that case only 2400 related code should remain.
> +					 */
> +					ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> +							    "free_mipi_frames(%p) leave: nothing to do, other streams still use this port (port=%d).\n",
> +							    pipe, port);
> +				}
>  			}
> -#endif
>  		}
>  	} else { /* pipe ==NULL */
>  		/* AM TEMP: free-ing all mipi buffers just like a legacy code. */

Hmm maybe just drop the entire ia_css_debug_dtrace() call here
instead of replace the #ifdef with if (IS_ISP2401) { } ?

I don't see much value in the ia_css_debug_dtrace() call, so to
me just dropping it seems best.

Regards,

Hans