[PATCH 0/9] media: atomisp: Changes for libcamera support

[PATCH 0/9] media: atomisp: Changes for libcamera support

[Hans de Goede]

Hi All,

Here is a series of cleanups (prep work) + some small(ish)
changes to make the atomisp driver work with libcamera.

This has been tested with libcamera's simple pipelinehandler
together with an ov2680 sensor.

Regards,

Hans

p.s.

Andy, I known I still need to do some small cleanups from my last
pull-requests. I have this on my TODO list :)


Hans de Goede (9):
  media: atomisp: Remove isp_subdev_propagate()
  media: atomisp: Rename atomisp_set_crop_and_fmt()
  media: atomisp: Remove custom VCM handling
  media: atomisp: Remove ISP controls which get passed through to the
    camera
  media: atomisp: Stop setting sd->devnode for the ATOMISP_SUBDEV
    v4l2-subdev
  media: atomisp: Add DMABUF support
  media: atomisp: Change ISP subdev name to "ATOM ISP"
  media: atomisp: Make MC link from ISP to /dev/video# output node
    immutable
  media: atomisp: Implement link_setup op for ISP subdev MC entity

 .../media/atomisp/include/linux/atomisp.h     |  24 --
 .../atomisp/include/linux/atomisp_platform.h  |   5 +-
 .../staging/media/atomisp/pci/atomisp_cmd.c   |  12 +-
 .../media/atomisp/pci/atomisp_internal.h      |   4 -
 .../staging/media/atomisp/pci/atomisp_ioctl.c | 333 +-----------------
 .../media/atomisp/pci/atomisp_subdev.c        |  95 +++--
 .../staging/media/atomisp/pci/atomisp_v4l2.c  |  18 +-
 7 files changed, 72 insertions(+), 419 deletions(-)

-- 
2.43.0

[PATCH 1/9] media: atomisp: Remove isp_subdev_propagate()

[Hans de Goede]

isp_subdev_propagate() is a wrapper around atomisp_subdev_set_selection()
which gets only used in a single place.

Scall atomisp_subdev_set_selection() directly in that single place instead.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/pci/atomisp_subdev.c        | 38 ++++---------------
 1 file changed, 7 insertions(+), 31 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
index a87fc74159e2..8293bda0c681 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
@@ -287,35 +287,6 @@ static void isp_get_fmt_rect(struct v4l2_subdev *sd,
 	}
 }
 
-static void isp_subdev_propagate(struct v4l2_subdev *sd,
-				 struct v4l2_subdev_state *sd_state,
-				 u32 which, uint32_t pad, uint32_t target,
-				 uint32_t flags)
-{
-	struct v4l2_mbus_framefmt *ffmt[ATOMISP_SUBDEV_PADS_NUM];
-	struct v4l2_rect *crop[ATOMISP_SUBDEV_PADS_NUM],
-		       *comp[ATOMISP_SUBDEV_PADS_NUM];
-
-	if (flags & V4L2_SEL_FLAG_KEEP_CONFIG)
-		return;
-
-	isp_get_fmt_rect(sd, sd_state, which, ffmt, crop, comp);
-
-	switch (pad) {
-	case ATOMISP_SUBDEV_PAD_SINK: {
-		struct v4l2_rect r = {0};
-
-		/* Only crop target supported on sink pad. */
-		r.width = ffmt[pad]->width;
-		r.height = ffmt[pad]->height;
-
-		atomisp_subdev_set_selection(sd, sd_state, which, pad,
-					     target, flags, &r);
-		break;
-	}
-	}
-}
-
 static int isp_subdev_get_selection(struct v4l2_subdev *sd,
 				    struct v4l2_subdev_state *sd_state,
 				    struct v4l2_subdev_selection *sel)
@@ -541,6 +512,7 @@ void atomisp_subdev_set_ffmt(struct v4l2_subdev *sd,
 	case ATOMISP_SUBDEV_PAD_SINK: {
 		const struct atomisp_in_fmt_conv *fc =
 		    atomisp_find_in_fmt_conv(ffmt->code);
+		struct v4l2_rect r = {};
 
 		if (!fc) {
 			fc = atomisp_in_fmt_conv;
@@ -551,8 +523,12 @@ void atomisp_subdev_set_ffmt(struct v4l2_subdev *sd,
 
 		*__ffmt = *ffmt;
 
-		isp_subdev_propagate(sd, sd_state, which, pad,
-				     V4L2_SEL_TGT_CROP, 0);
+		/* Propagate new ffmt to selection */
+		r.width = ffmt->width;
+		r.height = ffmt->height;
+		/* Only crop target supported on sink pad. */
+		atomisp_subdev_set_selection(sd, sd_state, which, pad,
+					     V4L2_SEL_TGT_CROP, 0, &r);
 
 		if (which == V4L2_SUBDEV_FORMAT_ACTIVE) {
 			atomisp_css_input_set_resolution(isp_sd,
-- 
2.43.0

[PATCH 2/9] media: atomisp: Rename atomisp_set_crop_and_fmt()

[Hans de Goede]

Rename atomisp_set_crop_and_fmt() to atomisp_set_sensor_crop_and_fmt()
to make clear that it operates on the sensor subdev.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/pci/atomisp_cmd.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 8593ba90605f..eb37bb6e41f9 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -3721,9 +3721,9 @@ void atomisp_get_padding(struct atomisp_device *isp, u32 width, u32 height,
 	*padding_h = max_t(u32, *padding_h, min_pad_h);
 }
 
-static int atomisp_set_crop_and_fmt(struct atomisp_device *isp,
-				    struct v4l2_mbus_framefmt *ffmt,
-				    int which)
+static int atomisp_set_sensor_crop_and_fmt(struct atomisp_device *isp,
+					   struct v4l2_mbus_framefmt *ffmt,
+					   int which)
 {
 	struct atomisp_input_subdev *input = &isp->inputs[isp->asd.input_curr];
 	struct v4l2_subdev_selection sel = {
@@ -3817,7 +3817,7 @@ int atomisp_try_fmt(struct atomisp_device *isp, struct v4l2_pix_format *f,
 
 	dev_dbg(isp->dev, "try_mbus_fmt: try %ux%u\n", ffmt.width, ffmt.height);
 
-	ret = atomisp_set_crop_and_fmt(isp, &ffmt, V4L2_SUBDEV_FORMAT_TRY);
+	ret = atomisp_set_sensor_crop_and_fmt(isp, &ffmt, V4L2_SUBDEV_FORMAT_TRY);
 	if (ret)
 		return ret;
 
@@ -4263,7 +4263,7 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev, const struct v4l2_p
 
 	/* Disable dvs if resolution can't be supported by sensor */
 	if (asd->params.video_dis_en && asd->run_mode->val == ATOMISP_RUN_MODE_VIDEO) {
-		ret = atomisp_set_crop_and_fmt(isp, &ffmt, V4L2_SUBDEV_FORMAT_TRY);
+		ret = atomisp_set_sensor_crop_and_fmt(isp, &ffmt, V4L2_SUBDEV_FORMAT_TRY);
 		if (ret)
 			return ret;
 
@@ -4281,7 +4281,7 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev, const struct v4l2_p
 		}
 	}
 
-	ret = atomisp_set_crop_and_fmt(isp, &ffmt, V4L2_SUBDEV_FORMAT_ACTIVE);
+	ret = atomisp_set_sensor_crop_and_fmt(isp, &ffmt, V4L2_SUBDEV_FORMAT_ACTIVE);
 	if (ret)
 		return ret;
 
-- 
2.43.0

[PATCH 3/9] media: atomisp: Remove custom VCM handling

[Hans de Goede]

Remove the custom VCM handling, instead the VCM should be controlled
through its own v4l2-subdev and the new ipu-bridge.c code already
supports instantiating an i2c_client for this and setting up
the necessary endpoints in the fwnode graph.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/include/linux/atomisp.h     |  3 -
 .../atomisp/include/linux/atomisp_platform.h  |  5 +-
 .../media/atomisp/pci/atomisp_internal.h      |  4 -
 .../staging/media/atomisp/pci/atomisp_ioctl.c | 86 +------------------
 .../staging/media/atomisp/pci/atomisp_v4l2.c  | 15 ----
 5 files changed, 3 insertions(+), 110 deletions(-)

diff --git a/drivers/staging/media/atomisp/include/linux/atomisp.h b/drivers/staging/media/atomisp/include/linux/atomisp.h
index bbbd904b696a..d9a7a599038d 100644
--- a/drivers/staging/media/atomisp/include/linux/atomisp.h
+++ b/drivers/staging/media/atomisp/include/linux/atomisp.h
@@ -914,9 +914,6 @@ enum atomisp_burst_capture_options {
 /* VCM step time */
 #define V4L2_CID_VCM_TIMING                (V4L2_CID_CAMERA_LASTP1 + 12)
 
-/* Query Focus Status */
-#define V4L2_CID_FOCUS_STATUS              (V4L2_CID_CAMERA_LASTP1 + 14)
-
 /* number of frames to skip at stream start */
 #define V4L2_CID_G_SKIP_FRAMES		   (V4L2_CID_CAMERA_LASTP1 + 17)
 
diff --git a/drivers/staging/media/atomisp/include/linux/atomisp_platform.h b/drivers/staging/media/atomisp/include/linux/atomisp_platform.h
index 487ef5846c24..2535402afd73 100644
--- a/drivers/staging/media/atomisp/include/linux/atomisp_platform.h
+++ b/drivers/staging/media/atomisp/include/linux/atomisp_platform.h
@@ -111,9 +111,8 @@ enum atomisp_input_format {
 
 enum intel_v4l2_subdev_type {
 	RAW_CAMERA = 1,
-	CAMERA_MOTOR = 2,
-	LED_FLASH = 3,
-	TEST_PATTERN = 4,
+	LED_FLASH = 2,
+	TEST_PATTERN = 3,
 };
 
 struct intel_v4l2_subdev_id {
diff --git a/drivers/staging/media/atomisp/pci/atomisp_internal.h b/drivers/staging/media/atomisp/pci/atomisp_internal.h
index bba9bc64d447..ca8ed3a6b9b8 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_internal.h
+++ b/drivers/staging/media/atomisp/pci/atomisp_internal.h
@@ -134,9 +134,6 @@ struct atomisp_input_subdev {
 	struct v4l2_rect active_rect;
 	/* Sensor state for which == V4L2_SUBDEV_FORMAT_TRY calls */
 	struct v4l2_subdev_state *try_sd_state;
-
-	struct v4l2_subdev *motor;
-
 	/*
 	 * To show this resource is used by
 	 * which stream, in ISP multiple stream mode
@@ -210,7 +207,6 @@ struct atomisp_device {
 	unsigned int input_cnt;
 	struct atomisp_input_subdev inputs[ATOM_ISP_MAX_INPUTS];
 	struct v4l2_subdev *flash;
-	struct v4l2_subdev *motor;
 
 	struct atomisp_regs saved_regs;
 	struct atomisp_css_env css_env;
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index bb8e5e883b50..ef555054fdbf 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -557,7 +557,6 @@ static int atomisp_enum_input(struct file *file, void *fh,
 	struct video_device *vdev = video_devdata(file);
 	struct atomisp_device *isp = video_get_drvdata(vdev);
 	int index = input->index;
-	struct v4l2_subdev *motor;
 
 	if (index >= isp->input_cnt)
 		return -EINVAL;
@@ -569,28 +568,6 @@ static int atomisp_enum_input(struct file *file, void *fh,
 	strscpy(input->name, isp->inputs[index].camera->name,
 		sizeof(input->name));
 
-	/*
-	 * HACK: append actuator's name to sensor's
-	 * As currently userspace can't talk directly to subdev nodes, this
-	 * ioctl is the only way to enum inputs + possible external actuators
-	 * for 3A tuning purpose.
-	 */
-	if (!IS_ISP2401)
-		motor = isp->inputs[index].motor;
-	else
-		motor = isp->motor;
-
-	if (motor && strlen(motor->name) > 0) {
-		const int cur_len = strlen(input->name);
-		const int max_size = sizeof(input->name) - cur_len - 1;
-
-		if (max_size > 1) {
-			input->name[cur_len] = '+';
-			strscpy(&input->name[cur_len + 1],
-				motor->name, max_size);
-		}
-	}
-
 	input->type = V4L2_INPUT_TYPE_CAMERA;
 	input->index = index;
 	input->reserved[0] = isp->inputs[index].type;
@@ -629,7 +606,6 @@ static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
 	struct atomisp_video_pipe *pipe = atomisp_to_video_pipe(vdev);
 	struct atomisp_sub_device *asd = pipe->asd;
 	struct v4l2_subdev *camera = NULL;
-	struct v4l2_subdev *motor;
 	int ret;
 
 	ret = atomisp_pipe_check(pipe, true);
@@ -666,17 +642,6 @@ static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
 		return ret;
 	}
 
-	if (!IS_ISP2401) {
-		motor = isp->inputs[input].motor;
-	} else {
-		motor = isp->motor;
-		if (motor)
-			ret = v4l2_subdev_call(motor, core, s_power, 1);
-	}
-
-	if (motor)
-		ret = v4l2_subdev_call(motor, core, init, 1);
-
 	asd->input_curr = input;
 	/* mark this camera is used by the current stream */
 	isp->inputs[input].asd = asd;
@@ -1433,26 +1398,8 @@ static int atomisp_s_ctrl(struct file *file, void *fh,
 static int atomisp_queryctl(struct file *file, void *fh,
 			    struct v4l2_queryctrl *qc)
 {
-	int i, ret = -EINVAL;
 	struct video_device *vdev = video_devdata(file);
-	struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
-	struct atomisp_device *isp = video_get_drvdata(vdev);
-
-	switch (qc->id) {
-	case V4L2_CID_FOCUS_ABSOLUTE:
-	case V4L2_CID_FOCUS_RELATIVE:
-	case V4L2_CID_FOCUS_STATUS:
-		if (!IS_ISP2401) {
-			return v4l2_queryctrl(isp->inputs[asd->input_curr].camera->
-					    ctrl_handler, qc);
-		}
-		/* ISP2401 */
-		if (isp->motor)
-			return v4l2_queryctrl(isp->motor->ctrl_handler, qc);
-		else
-			return v4l2_queryctrl(isp->inputs[asd->input_curr].
-					      camera->ctrl_handler, qc);
-	}
+	int i, ret = -EINVAL;
 
 	if (qc->id & V4L2_CTRL_FLAG_NEXT_CTRL)
 		return ret;
@@ -1478,16 +1425,10 @@ static int atomisp_camera_g_ext_ctrls(struct file *file, void *fh,
 	struct video_device *vdev = video_devdata(file);
 	struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
 	struct atomisp_device *isp = video_get_drvdata(vdev);
-	struct v4l2_subdev *motor;
 	struct v4l2_control ctrl;
 	int i;
 	int ret = 0;
 
-	if (!IS_ISP2401)
-		motor = isp->inputs[asd->input_curr].motor;
-	else
-		motor = isp->motor;
-
 	for (i = 0; i < c->count; i++) {
 		ctrl.id = c->controls[i].id;
 		ctrl.value = c->controls[i].value;
@@ -1509,13 +1450,6 @@ static int atomisp_camera_g_ext_ctrls(struct file *file, void *fh,
 			    v4l2_g_ctrl(isp->inputs[asd->input_curr].camera->
 					ctrl_handler, &ctrl);
 			break;
-		case V4L2_CID_FOCUS_ABSOLUTE:
-		case V4L2_CID_FOCUS_RELATIVE:
-		case V4L2_CID_FOCUS_STATUS:
-		case V4L2_CID_FOCUS_AUTO:
-			if (motor)
-				ret = v4l2_g_ctrl(motor->ctrl_handler, &ctrl);
-			break;
 		case V4L2_CID_FLASH_STATUS:
 		case V4L2_CID_FLASH_INTENSITY:
 		case V4L2_CID_FLASH_TORCH_INTENSITY:
@@ -1584,16 +1518,10 @@ static int atomisp_camera_s_ext_ctrls(struct file *file, void *fh,
 	struct video_device *vdev = video_devdata(file);
 	struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
 	struct atomisp_device *isp = video_get_drvdata(vdev);
-	struct v4l2_subdev *motor;
 	struct v4l2_control ctrl;
 	int i;
 	int ret = 0;
 
-	if (!IS_ISP2401)
-		motor = isp->inputs[asd->input_curr].motor;
-	else
-		motor = isp->motor;
-
 	for (i = 0; i < c->count; i++) {
 		struct v4l2_ctrl *ctr;
 
@@ -1616,18 +1544,6 @@ static int atomisp_camera_s_ext_ctrls(struct file *file, void *fh,
 					  isp->inputs[asd->input_curr].camera->
 					  ctrl_handler, &ctrl);
 			break;
-		case V4L2_CID_FOCUS_ABSOLUTE:
-		case V4L2_CID_FOCUS_RELATIVE:
-		case V4L2_CID_FOCUS_STATUS:
-		case V4L2_CID_FOCUS_AUTO:
-			if (motor)
-				ret = v4l2_s_ctrl(NULL, motor->ctrl_handler,
-						  &ctrl);
-			else
-				ret = v4l2_s_ctrl(NULL,
-						  isp->inputs[asd->input_curr].
-						  camera->ctrl_handler, &ctrl);
-			break;
 		case V4L2_CID_FLASH_STATUS:
 		case V4L2_CID_FLASH_INTENSITY:
 		case V4L2_CID_FLASH_TORCH_INTENSITY:
diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index f736e54c7df3..1a936dbe8eb4 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -823,13 +823,6 @@ static int atomisp_subdev_probe(struct atomisp_device *isp)
 			isp->sensor_lanes[mipi_port] = subdevs->lanes;
 			isp->sensor_subdevs[subdevs->port] = subdevs->subdev;
 			break;
-		case CAMERA_MOTOR:
-			if (isp->motor) {
-				dev_warn(isp->dev, "too many atomisp motors\n");
-				continue;
-			}
-			isp->motor = subdevs->subdev;
-			break;
 		case LED_FLASH:
 			if (isp->flash) {
 				dev_warn(isp->dev, "too many atomisp flash devices\n");
@@ -1066,14 +1059,6 @@ int atomisp_register_device_nodes(struct atomisp_device *isp)
 
 		atomisp_init_sensor(input);
 
-		/*
-		 * HACK: Currently VCM belongs to primary sensor only, but correct
-		 * approach must be to acquire from platform code which sensor
-		 * owns it.
-		 */
-		if (i == ATOMISP_CAMERA_PORT_PRIMARY)
-			input->motor = isp->motor;
-
 		err = media_create_pad_link(&input->camera->entity, 0,
 					    &isp->csi2_port[i].subdev.entity,
 					    CSI2_PAD_SINK,
-- 
2.43.0

[PATCH 4/9] media: atomisp: Remove ISP controls which get passed through to the camera

[Hans de Goede]

Drop all ISP controls and ioctls which just get passed through
to the camera subdev. Instead these calls should be done directly
at the sensor subdev.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/include/linux/atomisp.h     |  21 --
 .../staging/media/atomisp/pci/atomisp_ioctl.c | 248 ------------------
 2 files changed, 269 deletions(-)

diff --git a/drivers/staging/media/atomisp/include/linux/atomisp.h b/drivers/staging/media/atomisp/include/linux/atomisp.h
index d9a7a599038d..b2735a008052 100644
--- a/drivers/staging/media/atomisp/include/linux/atomisp.h
+++ b/drivers/staging/media/atomisp/include/linux/atomisp.h
@@ -837,9 +837,6 @@ enum atomisp_burst_capture_options {
 #define ATOMISP_IOC_S_PARAMETERS \
 	_IOW('v', BASE_VIDIOC_PRIVATE + 32, struct atomisp_parameters)
 
-#define ATOMISP_IOC_EXT_ISP_CTRL \
-	_IOWR('v', BASE_VIDIOC_PRIVATE + 35, struct atomisp_ext_isp_ctrl)
-
 #define ATOMISP_IOC_EXP_ID_UNLOCK \
 	_IOW('v', BASE_VIDIOC_PRIVATE + 36, int)
 
@@ -909,19 +906,6 @@ enum atomisp_burst_capture_options {
 /* Set the flash mode (see enum atomisp_flash_mode) */
 #define V4L2_CID_FLASH_MODE                (V4L2_CID_CAMERA_LASTP1 + 10)
 
-/* VCM slew control */
-#define V4L2_CID_VCM_SLEW                  (V4L2_CID_CAMERA_LASTP1 + 11)
-/* VCM step time */
-#define V4L2_CID_VCM_TIMING                (V4L2_CID_CAMERA_LASTP1 + 12)
-
-/* number of frames to skip at stream start */
-#define V4L2_CID_G_SKIP_FRAMES		   (V4L2_CID_CAMERA_LASTP1 + 17)
-
-/* Query sensor's 2A status */
-#define V4L2_CID_2A_STATUS                 (V4L2_CID_CAMERA_LASTP1 + 18)
-#define V4L2_2A_STATUS_AE_READY            BIT(0)
-#define V4L2_2A_STATUS_AWB_READY           BIT(1)
-
 #define V4L2_CID_RUN_MODE			(V4L2_CID_CAMERA_LASTP1 + 20)
 #define ATOMISP_RUN_MODE_VIDEO			1
 #define ATOMISP_RUN_MODE_STILL_CAPTURE		2
@@ -952,11 +936,6 @@ enum atomisp_burst_capture_options {
 /* Disable digital zoom */
 #define V4L2_CID_DISABLE_DZ		(V4L2_CID_CAMERA_LASTP1 + 32)
 
-#define V4L2_CID_TEST_PATTERN_COLOR_R	(V4L2_CID_CAMERA_LASTP1 + 33)
-#define V4L2_CID_TEST_PATTERN_COLOR_GR	(V4L2_CID_CAMERA_LASTP1 + 34)
-#define V4L2_CID_TEST_PATTERN_COLOR_GB	(V4L2_CID_CAMERA_LASTP1 + 35)
-#define V4L2_CID_TEST_PATTERN_COLOR_B	(V4L2_CID_CAMERA_LASTP1 + 36)
-
 #define V4L2_CID_ATOMISP_SELECT_ISP_VERSION	(V4L2_CID_CAMERA_LASTP1 + 38)
 
 #define V4L2_BUF_FLAG_BUFFER_INVALID       0x0400
diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index ef555054fdbf..74cf055cb09b 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -82,15 +82,6 @@ static struct v4l2_queryctrl ci_v4l2_controls[] = {
 		.step = 1,
 		.default_value = 0x00,
 	},
-	{
-		.id = V4L2_CID_POWER_LINE_FREQUENCY,
-		.type = V4L2_CTRL_TYPE_MENU,
-		.name = "Light frequency filter",
-		.minimum = 1,
-		.maximum = 2,
-		.step = 1,
-		.default_value = 1,
-	},
 	{
 		.id = V4L2_CID_COLORFX,
 		.type = V4L2_CTRL_TYPE_INTEGER,
@@ -100,15 +91,6 @@ static struct v4l2_queryctrl ci_v4l2_controls[] = {
 		.step = 1,
 		.default_value = 0,
 	},
-	{
-		.id = V4L2_CID_COLORFX_CBCR,
-		.type = V4L2_CTRL_TYPE_INTEGER,
-		.name = "Image Color Effect CbCr",
-		.minimum = 0,
-		.maximum = 0xffff,
-		.step = 1,
-		.default_value = 0,
-	},
 	{
 		.id = V4L2_CID_ATOMISP_BAD_PIXEL_DETECTION,
 		.type = V4L2_CTRL_TYPE_INTEGER,
@@ -172,142 +154,6 @@ static struct v4l2_queryctrl ci_v4l2_controls[] = {
 		.step = 1,
 		.default_value = 1,
 	},
-	{
-		.id = V4L2_CID_2A_STATUS,
-		.type = V4L2_CTRL_TYPE_BITMASK,
-		.name = "AE and AWB status",
-		.minimum = 0,
-		.maximum = V4L2_2A_STATUS_AE_READY | V4L2_2A_STATUS_AWB_READY,
-		.step = 1,
-		.default_value = 0,
-	},
-	{
-		.id = V4L2_CID_EXPOSURE,
-		.type = V4L2_CTRL_TYPE_INTEGER,
-		.name = "exposure",
-		.minimum = -4,
-		.maximum = 4,
-		.step = 1,
-		.default_value = 0,
-	},
-	{
-		.id = V4L2_CID_EXPOSURE_ZONE_NUM,
-		.type = V4L2_CTRL_TYPE_INTEGER,
-		.name = "one-time exposure zone number",
-		.minimum = 0x0,
-		.maximum = 0xffff,
-		.step = 1,
-		.default_value = 0,
-	},
-	{
-		.id = V4L2_CID_EXPOSURE_AUTO_PRIORITY,
-		.type = V4L2_CTRL_TYPE_INTEGER,
-		.name = "Exposure auto priority",
-		.minimum = V4L2_EXPOSURE_AUTO,
-		.maximum = V4L2_EXPOSURE_APERTURE_PRIORITY,
-		.step = 1,
-		.default_value = V4L2_EXPOSURE_AUTO,
-	},
-	{
-		.id = V4L2_CID_SCENE_MODE,
-		.type = V4L2_CTRL_TYPE_INTEGER,
-		.name = "scene mode",
-		.minimum = 0,
-		.maximum = 13,
-		.step = 1,
-		.default_value = 0,
-	},
-	{
-		.id = V4L2_CID_ISO_SENSITIVITY,
-		.type = V4L2_CTRL_TYPE_INTEGER,
-		.name = "iso",
-		.minimum = -4,
-		.maximum = 4,
-		.step = 1,
-		.default_value = 0,
-	},
-	{
-		.id = V4L2_CID_ISO_SENSITIVITY_AUTO,
-		.type = V4L2_CTRL_TYPE_INTEGER,
-		.name = "iso mode",
-		.minimum = V4L2_ISO_SENSITIVITY_MANUAL,
-		.maximum = V4L2_ISO_SENSITIVITY_AUTO,
-		.step = 1,
-		.default_value = V4L2_ISO_SENSITIVITY_AUTO,
-	},
-	{
-		.id = V4L2_CID_AUTO_N_PRESET_WHITE_BALANCE,
-		.type = V4L2_CTRL_TYPE_INTEGER,
-		.name = "white balance",
-		.minimum = 0,
-		.maximum = 9,
-		.step = 1,
-		.default_value = 0,
-	},
-	{
-		.id = V4L2_CID_EXPOSURE_METERING,
-		.type = V4L2_CTRL_TYPE_MENU,
-		.name = "metering",
-		.minimum = 0,
-		.maximum = 3,
-		.step = 1,
-		.default_value = 1,
-	},
-	{
-		.id = V4L2_CID_3A_LOCK,
-		.type = V4L2_CTRL_TYPE_BITMASK,
-		.name = "3a lock",
-		.minimum = 0,
-		.maximum = V4L2_LOCK_EXPOSURE | V4L2_LOCK_WHITE_BALANCE
-		| V4L2_LOCK_FOCUS,
-		.step = 1,
-		.default_value = 0,
-	},
-	{
-		.id = V4L2_CID_TEST_PATTERN,
-		.type = V4L2_CTRL_TYPE_INTEGER,
-		.name = "Test Pattern",
-		.minimum = 0,
-		.maximum = 0xffff,
-		.step = 1,
-		.default_value = 0,
-	},
-	{
-		.id = V4L2_CID_TEST_PATTERN_COLOR_R,
-		.type = V4L2_CTRL_TYPE_INTEGER,
-		.name = "Test Pattern Solid Color R",
-		.minimum = INT_MIN,
-		.maximum = INT_MAX,
-		.step = 1,
-		.default_value = 0,
-	},
-	{
-		.id = V4L2_CID_TEST_PATTERN_COLOR_GR,
-		.type = V4L2_CTRL_TYPE_INTEGER,
-		.name = "Test Pattern Solid Color GR",
-		.minimum = INT_MIN,
-		.maximum = INT_MAX,
-		.step = 1,
-		.default_value = 0,
-	},
-	{
-		.id = V4L2_CID_TEST_PATTERN_COLOR_GB,
-		.type = V4L2_CTRL_TYPE_INTEGER,
-		.name = "Test Pattern Solid Color GB",
-		.minimum = INT_MIN,
-		.maximum = INT_MAX,
-		.step = 1,
-		.default_value = 0,
-	},
-	{
-		.id = V4L2_CID_TEST_PATTERN_COLOR_B,
-		.type = V4L2_CTRL_TYPE_INTEGER,
-		.name = "Test Pattern Solid Color B",
-		.minimum = INT_MIN,
-		.maximum = INT_MAX,
-		.step = 1,
-		.default_value = 0,
-	},
 };
 
 static const u32 ctrls_num = ARRAY_SIZE(ci_v4l2_controls);
@@ -1248,7 +1094,6 @@ static int atomisp_g_ctrl(struct file *file, void *fh,
 {
 	struct video_device *vdev = video_devdata(file);
 	struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
-	struct atomisp_device *isp = video_get_drvdata(vdev);
 	int i, ret = -EINVAL;
 
 	for (i = 0; i < ctrls_num; i++) {
@@ -1262,27 +1107,6 @@ static int atomisp_g_ctrl(struct file *file, void *fh,
 		return ret;
 
 	switch (control->id) {
-	case V4L2_CID_IRIS_ABSOLUTE:
-	case V4L2_CID_EXPOSURE_ABSOLUTE:
-	case V4L2_CID_2A_STATUS:
-	case V4L2_CID_AUTO_N_PRESET_WHITE_BALANCE:
-	case V4L2_CID_EXPOSURE:
-	case V4L2_CID_EXPOSURE_AUTO:
-	case V4L2_CID_SCENE_MODE:
-	case V4L2_CID_ISO_SENSITIVITY:
-	case V4L2_CID_ISO_SENSITIVITY_AUTO:
-	case V4L2_CID_CONTRAST:
-	case V4L2_CID_SATURATION:
-	case V4L2_CID_SHARPNESS:
-	case V4L2_CID_3A_LOCK:
-	case V4L2_CID_EXPOSURE_ZONE_NUM:
-	case V4L2_CID_TEST_PATTERN:
-	case V4L2_CID_TEST_PATTERN_COLOR_R:
-	case V4L2_CID_TEST_PATTERN_COLOR_GR:
-	case V4L2_CID_TEST_PATTERN_COLOR_GB:
-	case V4L2_CID_TEST_PATTERN_COLOR_B:
-		return v4l2_g_ctrl(isp->inputs[asd->input_curr].camera->
-				   ctrl_handler, control);
 	case V4L2_CID_COLORFX:
 		ret = atomisp_color_effect(asd, 0, &control->value);
 		break;
@@ -1322,7 +1146,6 @@ static int atomisp_s_ctrl(struct file *file, void *fh,
 {
 	struct video_device *vdev = video_devdata(file);
 	struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
-	struct atomisp_device *isp = video_get_drvdata(vdev);
 	int i, ret = -EINVAL;
 
 	for (i = 0; i < ctrls_num; i++) {
@@ -1336,28 +1159,6 @@ static int atomisp_s_ctrl(struct file *file, void *fh,
 		return ret;
 
 	switch (control->id) {
-	case V4L2_CID_AUTO_N_PRESET_WHITE_BALANCE:
-	case V4L2_CID_EXPOSURE:
-	case V4L2_CID_EXPOSURE_AUTO:
-	case V4L2_CID_EXPOSURE_AUTO_PRIORITY:
-	case V4L2_CID_SCENE_MODE:
-	case V4L2_CID_ISO_SENSITIVITY:
-	case V4L2_CID_ISO_SENSITIVITY_AUTO:
-	case V4L2_CID_POWER_LINE_FREQUENCY:
-	case V4L2_CID_EXPOSURE_METERING:
-	case V4L2_CID_CONTRAST:
-	case V4L2_CID_SATURATION:
-	case V4L2_CID_SHARPNESS:
-	case V4L2_CID_3A_LOCK:
-	case V4L2_CID_COLORFX_CBCR:
-	case V4L2_CID_TEST_PATTERN:
-	case V4L2_CID_TEST_PATTERN_COLOR_R:
-	case V4L2_CID_TEST_PATTERN_COLOR_GR:
-	case V4L2_CID_TEST_PATTERN_COLOR_GB:
-	case V4L2_CID_TEST_PATTERN_COLOR_B:
-		return v4l2_s_ctrl(NULL,
-				   isp->inputs[asd->input_curr].camera->
-				   ctrl_handler, control);
 	case V4L2_CID_COLORFX:
 		ret = atomisp_color_effect(asd, 1, &control->value);
 		break;
@@ -1398,7 +1199,6 @@ static int atomisp_s_ctrl(struct file *file, void *fh,
 static int atomisp_queryctl(struct file *file, void *fh,
 			    struct v4l2_queryctrl *qc)
 {
-	struct video_device *vdev = video_devdata(file);
 	int i, ret = -EINVAL;
 
 	if (qc->id & V4L2_CTRL_FLAG_NEXT_CTRL)
@@ -1433,23 +1233,6 @@ static int atomisp_camera_g_ext_ctrls(struct file *file, void *fh,
 		ctrl.id = c->controls[i].id;
 		ctrl.value = c->controls[i].value;
 		switch (ctrl.id) {
-		case V4L2_CID_EXPOSURE_ABSOLUTE:
-		case V4L2_CID_EXPOSURE_AUTO:
-		case V4L2_CID_IRIS_ABSOLUTE:
-		case V4L2_CID_3A_LOCK:
-		case V4L2_CID_TEST_PATTERN:
-		case V4L2_CID_TEST_PATTERN_COLOR_R:
-		case V4L2_CID_TEST_PATTERN_COLOR_GR:
-		case V4L2_CID_TEST_PATTERN_COLOR_GB:
-		case V4L2_CID_TEST_PATTERN_COLOR_B:
-			/*
-			 * Exposure related control will be handled by sensor
-			 * driver
-			 */
-			ret =
-			    v4l2_g_ctrl(isp->inputs[asd->input_curr].camera->
-					ctrl_handler, &ctrl);
-			break;
 		case V4L2_CID_FLASH_STATUS:
 		case V4L2_CID_FLASH_INTENSITY:
 		case V4L2_CID_FLASH_TORCH_INTENSITY:
@@ -1466,11 +1249,6 @@ static int atomisp_camera_g_ext_ctrls(struct file *file, void *fh,
 		case V4L2_CID_ZOOM_ABSOLUTE:
 			ret = atomisp_digital_zoom(asd, 0, &ctrl.value);
 			break;
-		case V4L2_CID_G_SKIP_FRAMES:
-			ret = v4l2_subdev_call(
-				  isp->inputs[asd->input_curr].camera,
-				  sensor, g_skip_frames, (u32 *)&ctrl.value);
-			break;
 		default:
 			ret = -EINVAL;
 		}
@@ -1528,22 +1306,6 @@ static int atomisp_camera_s_ext_ctrls(struct file *file, void *fh,
 		ctrl.id = c->controls[i].id;
 		ctrl.value = c->controls[i].value;
 		switch (ctrl.id) {
-		case V4L2_CID_EXPOSURE_ABSOLUTE:
-		case V4L2_CID_EXPOSURE_AUTO:
-		case V4L2_CID_EXPOSURE_METERING:
-		case V4L2_CID_IRIS_ABSOLUTE:
-		case V4L2_CID_VCM_TIMING:
-		case V4L2_CID_VCM_SLEW:
-		case V4L2_CID_3A_LOCK:
-		case V4L2_CID_TEST_PATTERN:
-		case V4L2_CID_TEST_PATTERN_COLOR_R:
-		case V4L2_CID_TEST_PATTERN_COLOR_GR:
-		case V4L2_CID_TEST_PATTERN_COLOR_GB:
-		case V4L2_CID_TEST_PATTERN_COLOR_B:
-			ret = v4l2_s_ctrl(NULL,
-					  isp->inputs[asd->input_curr].camera->
-					  ctrl_handler, &ctrl);
-			break;
 		case V4L2_CID_FLASH_STATUS:
 		case V4L2_CID_FLASH_INTENSITY:
 		case V4L2_CID_FLASH_TORCH_INTENSITY:
@@ -1692,7 +1454,6 @@ static long atomisp_vidioc_default(struct file *file, void *fh,
 				   bool valid_prio, unsigned int cmd, void *arg)
 {
 	struct video_device *vdev = video_devdata(file);
-	struct atomisp_device *isp = video_get_drvdata(vdev);
 	struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
 	int err;
 
@@ -1839,11 +1600,6 @@ static long atomisp_vidioc_default(struct file *file, void *fh,
 		err = atomisp_fixed_pattern_table(asd, arg);
 		break;
 
-	case ATOMISP_IOC_S_EXPOSURE:
-		err = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
-				       core, ioctl, cmd, arg);
-		break;
-
 	case ATOMISP_IOC_S_ISP_SHD_TAB:
 		err = atomisp_set_shading_table(asd, arg);
 		break;
@@ -1860,10 +1616,6 @@ static long atomisp_vidioc_default(struct file *file, void *fh,
 		err = atomisp_set_parameters(vdev, arg);
 		break;
 
-	case ATOMISP_IOC_EXT_ISP_CTRL:
-		err = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
-				       core, ioctl, cmd, arg);
-		break;
 	case ATOMISP_IOC_EXP_ID_UNLOCK:
 		err = atomisp_exp_id_unlock(asd, arg);
 		break;
-- 
2.43.0

[PATCH 5/9] media: atomisp: Stop setting sd->devnode for the ATOMISP_SUBDEV v4l2-subdev

[Hans de Goede]

Stop setting sd->devnode for the ATOMISP_SUBDEV v4l2-subdev, so that
a proper /dev/v4l-subdev# gets created for it.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/pci/atomisp_subdev.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
index 8293bda0c681..7f1ca05ce54a 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
@@ -802,7 +802,6 @@ static int isp_subdev_init_entities(struct atomisp_sub_device *asd)
 	sprintf(sd->name, "ATOMISP_SUBDEV");
 	v4l2_set_subdevdata(sd, asd);
 	sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS | V4L2_SUBDEV_FL_HAS_DEVNODE;
-	sd->devnode = &asd->video_out.vdev;
 
 	pads[ATOMISP_SUBDEV_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
 	pads[ATOMISP_SUBDEV_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
-- 
2.43.0

[PATCH 6/9] media: atomisp: Add DMABUF support

[Hans de Goede]

Add DMABUF support and while at it drop userptr support.

Now that atomisp has been ported to videobuf2 this is trivial.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/pci/atomisp_ioctl.c  | 1 +
 drivers/staging/media/atomisp/pci/atomisp_subdev.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
index 74cf055cb09b..fa6c9f0ea6eb 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
@@ -1666,6 +1666,7 @@ const struct v4l2_ioctl_ops atomisp_ioctl_ops = {
 	.vidioc_querybuf = vb2_ioctl_querybuf,
 	.vidioc_qbuf = atomisp_qbuf_wrapper,
 	.vidioc_dqbuf = atomisp_dqbuf_wrapper,
+	.vidioc_expbuf = vb2_ioctl_expbuf,
 	.vidioc_streamon = vb2_ioctl_streamon,
 	.vidioc_streamoff = vb2_ioctl_streamoff,
 	.vidioc_default = atomisp_vidioc_default,
diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
index 7f1ca05ce54a..8253b6faf8cd 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
@@ -765,7 +765,7 @@ static int atomisp_init_subdev_pipe(struct atomisp_sub_device *asd,
 
 	/* Init videobuf2 queue structure */
 	pipe->vb_queue.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
-	pipe->vb_queue.io_modes = VB2_MMAP | VB2_USERPTR;
+	pipe->vb_queue.io_modes = VB2_MMAP | VB2_DMABUF;
 	pipe->vb_queue.buf_struct_size = sizeof(struct ia_css_frame);
 	pipe->vb_queue.ops = &atomisp_vb2_ops;
 	pipe->vb_queue.mem_ops = &vb2_vmalloc_memops;
-- 
2.43.0

[PATCH 7/9] media: atomisp: Change ISP subdev name to "ATOM ISP"

[Hans de Goede]

Change the generic "ATOMISP_SUBDEV" name to "ATOM ISP" to make clear
that this is the subdev for the ISP itself.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/pci/atomisp_subdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
index 8253b6faf8cd..822fe7d129e2 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
@@ -799,7 +799,7 @@ static int isp_subdev_init_entities(struct atomisp_sub_device *asd)
 	int ret;
 
 	v4l2_subdev_init(sd, &isp_subdev_v4l2_ops);
-	sprintf(sd->name, "ATOMISP_SUBDEV");
+	sprintf(sd->name, "ATOM ISP");
 	v4l2_set_subdevdata(sd, asd);
 	sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS | V4L2_SUBDEV_FL_HAS_DEVNODE;
 
-- 
2.43.0

[PATCH 8/9] media: atomisp: Make MC link from ISP to /dev/video# output node immutable

[Hans de Goede]

The link from the ISP's source pad to the /dev/video# output v4l2-dev
is always enabled and immutable, mark it as such.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/pci/atomisp_v4l2.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
index 1a936dbe8eb4..86028721e7cb 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
@@ -1090,7 +1090,8 @@ int atomisp_register_device_nodes(struct atomisp_device *isp)
 		return err;
 
 	err = media_create_pad_link(&isp->asd.subdev.entity, ATOMISP_SUBDEV_PAD_SOURCE,
-				    &isp->asd.video_out.vdev.entity, 0, 0);
+				    &isp->asd.video_out.vdev.entity, 0,
+				    MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
 	if (err)
 		return err;
 
-- 
2.43.0

[PATCH 9/9] media: atomisp: Implement link_setup op for ISP subdev MC entity

[Hans de Goede]

The atomisp driver's Android heritage makes it weird in that
even though it uses MC + subdev-s it is designed to primarily
be controlled through its /dev/video# node.

It implements s_input on /dev/video# to select which sensor to use,
while ignoring setup_link calls to enable a link to another sensor.

Add support for selecting the active sensor the MC way by adding
setup_link support.

The implementation is a bit convoluted due to the atomisp driver's
heritage.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/pci/atomisp_subdev.c        | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
index 822fe7d129e2..17819d7586dd 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
@@ -30,6 +30,7 @@
 #include "atomisp_common.h"
 #include "atomisp_compat.h"
 #include "atomisp_fops.h"
+#include "atomisp_ioctl.h"
 #include "atomisp_internal.h"
 
 const struct atomisp_in_fmt_conv atomisp_in_fmt_conv[] = {
@@ -634,8 +635,59 @@ static void isp_subdev_init_params(struct atomisp_sub_device *asd)
 }
 
 /* media operations */
+static int atomisp_link_setup(struct media_entity *entity,
+			      const struct media_pad *local,
+			      const struct media_pad *remote, u32 flags)
+{
+	struct v4l2_subdev *sd = container_of(entity, struct v4l2_subdev,
+					      entity);
+	struct atomisp_sub_device *asd = v4l2_get_subdevdata(sd);
+	struct atomisp_device *isp = asd->isp;
+	int i, csi_idx, ret;
+
+	/* ISP's source is immutable */
+	if (local != &asd->pads[ATOMISP_SUBDEV_PAD_SINK]) {
+		v4l2_err(sd, "Error pad %d does not support changing flags\n",
+			 local->index);
+		return -EINVAL;
+	}
+
+	for (csi_idx = 0; csi_idx < ATOMISP_CAMERA_NR_PORTS; csi_idx++) {
+		if (&isp->csi2_port[csi_idx].pads[CSI2_PAD_SOURCE] == remote)
+			break;
+	}
+
+	if (csi_idx == ATOMISP_CAMERA_NR_PORTS) {
+		v4l2_err(sd, "Error cannot find CSI receiver for remote pad\n");
+		return -EINVAL;
+	}
+
+	/* Ignore disables, input_curr should only be updated on enables */
+	if (!(flags & MEDIA_LNK_FL_ENABLED))
+		return 0;
+
+	for (i = 0; i < isp->input_cnt; i++) {
+		if (isp->inputs[i].camera == isp->sensor_subdevs[csi_idx])
+			break;
+	}
+
+	if (i == isp->input_cnt) {
+		v4l2_err(sd, "Error no sensor for CSI receiver %d\n", csi_idx);
+		return -EINVAL;
+	}
+
+	mutex_lock(&isp->mutex);
+	ret = atomisp_pipe_check(&asd->video_out, true);
+	if (ret == 0)
+		asd->input_curr = i;
+	mutex_unlock(&isp->mutex);
+
+	return ret;
+}
+
 static const struct media_entity_operations isp_subdev_media_ops = {
 	.link_validate = v4l2_subdev_link_validate,
+	.link_setup = atomisp_link_setup,
 	/*	 .set_power = v4l2_subdev_set_power,	*/
 };
 
-- 
2.43.0

Re: [PATCH 1/9] media: atomisp: Remove isp_subdev_propagate()

[Kieran Bingham]

Quoting Hans de Goede (2024-02-17 11:24:30)
> isp_subdev_propagate() is a wrapper around atomisp_subdev_set_selection()
> which gets only used in a single place.
> 
> Scall atomisp_subdev_set_selection() directly in that single place instead.

s/Scall/Call/

Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  .../media/atomisp/pci/atomisp_subdev.c        | 38 ++++---------------
>  1 file changed, 7 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
> index a87fc74159e2..8293bda0c681 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
> @@ -287,35 +287,6 @@ static void isp_get_fmt_rect(struct v4l2_subdev *sd,
>         }
>  }
>  
> -static void isp_subdev_propagate(struct v4l2_subdev *sd,
> -                                struct v4l2_subdev_state *sd_state,
> -                                u32 which, uint32_t pad, uint32_t target,
> -                                uint32_t flags)
> -{
> -       struct v4l2_mbus_framefmt *ffmt[ATOMISP_SUBDEV_PADS_NUM];
> -       struct v4l2_rect *crop[ATOMISP_SUBDEV_PADS_NUM],
> -                      *comp[ATOMISP_SUBDEV_PADS_NUM];
> -
> -       if (flags & V4L2_SEL_FLAG_KEEP_CONFIG)
> -               return;
> -
> -       isp_get_fmt_rect(sd, sd_state, which, ffmt, crop, comp);
> -
> -       switch (pad) {
> -       case ATOMISP_SUBDEV_PAD_SINK: {
> -               struct v4l2_rect r = {0};
> -
> -               /* Only crop target supported on sink pad. */
> -               r.width = ffmt[pad]->width;
> -               r.height = ffmt[pad]->height;
> -
> -               atomisp_subdev_set_selection(sd, sd_state, which, pad,
> -                                            target, flags, &r);
> -               break;
> -       }
> -       }
> -}
> -
>  static int isp_subdev_get_selection(struct v4l2_subdev *sd,
>                                     struct v4l2_subdev_state *sd_state,
>                                     struct v4l2_subdev_selection *sel)
> @@ -541,6 +512,7 @@ void atomisp_subdev_set_ffmt(struct v4l2_subdev *sd,
>         case ATOMISP_SUBDEV_PAD_SINK: {
>                 const struct atomisp_in_fmt_conv *fc =
>                     atomisp_find_in_fmt_conv(ffmt->code);
> +               struct v4l2_rect r = {};
>  
>                 if (!fc) {
>                         fc = atomisp_in_fmt_conv;
> @@ -551,8 +523,12 @@ void atomisp_subdev_set_ffmt(struct v4l2_subdev *sd,
>  
>                 *__ffmt = *ffmt;
>  
> -               isp_subdev_propagate(sd, sd_state, which, pad,
> -                                    V4L2_SEL_TGT_CROP, 0);
> +               /* Propagate new ffmt to selection */
> +               r.width = ffmt->width;
> +               r.height = ffmt->height;
> +               /* Only crop target supported on sink pad. */
> +               atomisp_subdev_set_selection(sd, sd_state, which, pad,
> +                                            V4L2_SEL_TGT_CROP, 0, &r);
>  
>                 if (which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>                         atomisp_css_input_set_resolution(isp_sd,
> -- 
> 2.43.0
>

Re: [PATCH 2/9] media: atomisp: Rename atomisp_set_crop_and_fmt()

[Kieran Bingham]

Quoting Hans de Goede (2024-02-17 11:24:31)
> Rename atomisp_set_crop_and_fmt() to atomisp_set_sensor_crop_and_fmt()
> to make clear that it operates on the sensor subdev.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/staging/media/atomisp/pci/atomisp_cmd.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> index 8593ba90605f..eb37bb6e41f9 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
> @@ -3721,9 +3721,9 @@ void atomisp_get_padding(struct atomisp_device *isp, u32 width, u32 height,
>         *padding_h = max_t(u32, *padding_h, min_pad_h);
>  }
>  
> -static int atomisp_set_crop_and_fmt(struct atomisp_device *isp,
> -                                   struct v4l2_mbus_framefmt *ffmt,
> -                                   int which)
> +static int atomisp_set_sensor_crop_and_fmt(struct atomisp_device *isp,
> +                                          struct v4l2_mbus_framefmt *ffmt,
> +                                          int which)

I went to look this up and I only see atomisp_set_crop() so I guess I've
missed some patches.

Does the atomisp use the usual v4l2-subdev drivers for sensors? or are
they separate for now ?

I'm curious about how it's handling the binning in that function, but
that's not specific to this patch which looks reasonable so far so:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>  {
>         struct atomisp_input_subdev *input = &isp->inputs[isp->asd.input_curr];
>         struct v4l2_subdev_selection sel = {
> @@ -3817,7 +3817,7 @@ int atomisp_try_fmt(struct atomisp_device *isp, struct v4l2_pix_format *f,
>  
>         dev_dbg(isp->dev, "try_mbus_fmt: try %ux%u\n", ffmt.width, ffmt.height);
>  
> -       ret = atomisp_set_crop_and_fmt(isp, &ffmt, V4L2_SUBDEV_FORMAT_TRY);
> +       ret = atomisp_set_sensor_crop_and_fmt(isp, &ffmt, V4L2_SUBDEV_FORMAT_TRY);
>         if (ret)
>                 return ret;
>  
> @@ -4263,7 +4263,7 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev, const struct v4l2_p
>  
>         /* Disable dvs if resolution can't be supported by sensor */
>         if (asd->params.video_dis_en && asd->run_mode->val == ATOMISP_RUN_MODE_VIDEO) {
> -               ret = atomisp_set_crop_and_fmt(isp, &ffmt, V4L2_SUBDEV_FORMAT_TRY);
> +               ret = atomisp_set_sensor_crop_and_fmt(isp, &ffmt, V4L2_SUBDEV_FORMAT_TRY);
>                 if (ret)
>                         return ret;
>  
> @@ -4281,7 +4281,7 @@ static int atomisp_set_fmt_to_snr(struct video_device *vdev, const struct v4l2_p
>                 }
>         }
>  
> -       ret = atomisp_set_crop_and_fmt(isp, &ffmt, V4L2_SUBDEV_FORMAT_ACTIVE);
> +       ret = atomisp_set_sensor_crop_and_fmt(isp, &ffmt, V4L2_SUBDEV_FORMAT_ACTIVE);
>         if (ret)
>                 return ret;
>  
> -- 
> 2.43.0
>

Re: [PATCH 3/9] media: atomisp: Remove custom VCM handling

[Kieran Bingham]

Quoting Hans de Goede (2024-02-17 11:24:32)
> Remove the custom VCM handling, instead the VCM should be controlled
> through its own v4l2-subdev and the new ipu-bridge.c code already
> supports instantiating an i2c_client for this and setting up
> the necessary endpoints in the fwnode graph.

Agreed, this sounds like a good move forwards to align with other ISPs
and existing VCM driver frameworks. I must get back to my rework of the
VCM drivers I started though!

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  .../media/atomisp/include/linux/atomisp.h     |  3 -
>  .../atomisp/include/linux/atomisp_platform.h  |  5 +-
>  .../media/atomisp/pci/atomisp_internal.h      |  4 -
>  .../staging/media/atomisp/pci/atomisp_ioctl.c | 86 +------------------
>  .../staging/media/atomisp/pci/atomisp_v4l2.c  | 15 ----
>  5 files changed, 3 insertions(+), 110 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/include/linux/atomisp.h b/drivers/staging/media/atomisp/include/linux/atomisp.h
> index bbbd904b696a..d9a7a599038d 100644
> --- a/drivers/staging/media/atomisp/include/linux/atomisp.h
> +++ b/drivers/staging/media/atomisp/include/linux/atomisp.h
> @@ -914,9 +914,6 @@ enum atomisp_burst_capture_options {
>  /* VCM step time */
>  #define V4L2_CID_VCM_TIMING                (V4L2_CID_CAMERA_LASTP1 + 12)
>  
> -/* Query Focus Status */
> -#define V4L2_CID_FOCUS_STATUS              (V4L2_CID_CAMERA_LASTP1 + 14)
> -
>  /* number of frames to skip at stream start */
>  #define V4L2_CID_G_SKIP_FRAMES            (V4L2_CID_CAMERA_LASTP1 + 17)
>  
> diff --git a/drivers/staging/media/atomisp/include/linux/atomisp_platform.h b/drivers/staging/media/atomisp/include/linux/atomisp_platform.h
> index 487ef5846c24..2535402afd73 100644
> --- a/drivers/staging/media/atomisp/include/linux/atomisp_platform.h
> +++ b/drivers/staging/media/atomisp/include/linux/atomisp_platform.h
> @@ -111,9 +111,8 @@ enum atomisp_input_format {
>  
>  enum intel_v4l2_subdev_type {
>         RAW_CAMERA = 1,
> -       CAMERA_MOTOR = 2,
> -       LED_FLASH = 3,
> -       TEST_PATTERN = 4,
> +       LED_FLASH = 2,
> +       TEST_PATTERN = 3,
>  };
>  
>  struct intel_v4l2_subdev_id {
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_internal.h b/drivers/staging/media/atomisp/pci/atomisp_internal.h
> index bba9bc64d447..ca8ed3a6b9b8 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_internal.h
> +++ b/drivers/staging/media/atomisp/pci/atomisp_internal.h
> @@ -134,9 +134,6 @@ struct atomisp_input_subdev {
>         struct v4l2_rect active_rect;
>         /* Sensor state for which == V4L2_SUBDEV_FORMAT_TRY calls */
>         struct v4l2_subdev_state *try_sd_state;
> -
> -       struct v4l2_subdev *motor;
> -
>         /*
>          * To show this resource is used by
>          * which stream, in ISP multiple stream mode
> @@ -210,7 +207,6 @@ struct atomisp_device {
>         unsigned int input_cnt;
>         struct atomisp_input_subdev inputs[ATOM_ISP_MAX_INPUTS];
>         struct v4l2_subdev *flash;
> -       struct v4l2_subdev *motor;
>  
>         struct atomisp_regs saved_regs;
>         struct atomisp_css_env css_env;
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> index bb8e5e883b50..ef555054fdbf 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> @@ -557,7 +557,6 @@ static int atomisp_enum_input(struct file *file, void *fh,
>         struct video_device *vdev = video_devdata(file);
>         struct atomisp_device *isp = video_get_drvdata(vdev);
>         int index = input->index;
> -       struct v4l2_subdev *motor;
>  
>         if (index >= isp->input_cnt)
>                 return -EINVAL;
> @@ -569,28 +568,6 @@ static int atomisp_enum_input(struct file *file, void *fh,
>         strscpy(input->name, isp->inputs[index].camera->name,
>                 sizeof(input->name));
>  
> -       /*
> -        * HACK: append actuator's name to sensor's
> -        * As currently userspace can't talk directly to subdev nodes, this
> -        * ioctl is the only way to enum inputs + possible external actuators
> -        * for 3A tuning purpose.
> -        */
> -       if (!IS_ISP2401)
> -               motor = isp->inputs[index].motor;
> -       else
> -               motor = isp->motor;
> -
> -       if (motor && strlen(motor->name) > 0) {
> -               const int cur_len = strlen(input->name);
> -               const int max_size = sizeof(input->name) - cur_len - 1;
> -
> -               if (max_size > 1) {
> -                       input->name[cur_len] = '+';
> -                       strscpy(&input->name[cur_len + 1],
> -                               motor->name, max_size);
> -               }
> -       }
> -
>         input->type = V4L2_INPUT_TYPE_CAMERA;
>         input->index = index;
>         input->reserved[0] = isp->inputs[index].type;
> @@ -629,7 +606,6 @@ static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
>         struct atomisp_video_pipe *pipe = atomisp_to_video_pipe(vdev);
>         struct atomisp_sub_device *asd = pipe->asd;
>         struct v4l2_subdev *camera = NULL;
> -       struct v4l2_subdev *motor;
>         int ret;
>  
>         ret = atomisp_pipe_check(pipe, true);
> @@ -666,17 +642,6 @@ static int atomisp_s_input(struct file *file, void *fh, unsigned int input)
>                 return ret;
>         }
>  
> -       if (!IS_ISP2401) {
> -               motor = isp->inputs[input].motor;
> -       } else {
> -               motor = isp->motor;
> -               if (motor)
> -                       ret = v4l2_subdev_call(motor, core, s_power, 1);
> -       }
> -
> -       if (motor)
> -               ret = v4l2_subdev_call(motor, core, init, 1);
> -
>         asd->input_curr = input;
>         /* mark this camera is used by the current stream */
>         isp->inputs[input].asd = asd;
> @@ -1433,26 +1398,8 @@ static int atomisp_s_ctrl(struct file *file, void *fh,
>  static int atomisp_queryctl(struct file *file, void *fh,
>                             struct v4l2_queryctrl *qc)
>  {
> -       int i, ret = -EINVAL;
>         struct video_device *vdev = video_devdata(file);
> -       struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
> -       struct atomisp_device *isp = video_get_drvdata(vdev);
> -
> -       switch (qc->id) {
> -       case V4L2_CID_FOCUS_ABSOLUTE:
> -       case V4L2_CID_FOCUS_RELATIVE:
> -       case V4L2_CID_FOCUS_STATUS:
> -               if (!IS_ISP2401) {
> -                       return v4l2_queryctrl(isp->inputs[asd->input_curr].camera->
> -                                           ctrl_handler, qc);
> -               }
> -               /* ISP2401 */
> -               if (isp->motor)
> -                       return v4l2_queryctrl(isp->motor->ctrl_handler, qc);
> -               else
> -                       return v4l2_queryctrl(isp->inputs[asd->input_curr].
> -                                             camera->ctrl_handler, qc);
> -       }
> +       int i, ret = -EINVAL;
>  
>         if (qc->id & V4L2_CTRL_FLAG_NEXT_CTRL)
>                 return ret;
> @@ -1478,16 +1425,10 @@ static int atomisp_camera_g_ext_ctrls(struct file *file, void *fh,
>         struct video_device *vdev = video_devdata(file);
>         struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
>         struct atomisp_device *isp = video_get_drvdata(vdev);
> -       struct v4l2_subdev *motor;
>         struct v4l2_control ctrl;
>         int i;
>         int ret = 0;
>  
> -       if (!IS_ISP2401)
> -               motor = isp->inputs[asd->input_curr].motor;
> -       else
> -               motor = isp->motor;
> -
>         for (i = 0; i < c->count; i++) {
>                 ctrl.id = c->controls[i].id;
>                 ctrl.value = c->controls[i].value;
> @@ -1509,13 +1450,6 @@ static int atomisp_camera_g_ext_ctrls(struct file *file, void *fh,
>                             v4l2_g_ctrl(isp->inputs[asd->input_curr].camera->
>                                         ctrl_handler, &ctrl);
>                         break;
> -               case V4L2_CID_FOCUS_ABSOLUTE:
> -               case V4L2_CID_FOCUS_RELATIVE:
> -               case V4L2_CID_FOCUS_STATUS:
> -               case V4L2_CID_FOCUS_AUTO:
> -                       if (motor)
> -                               ret = v4l2_g_ctrl(motor->ctrl_handler, &ctrl);
> -                       break;
>                 case V4L2_CID_FLASH_STATUS:
>                 case V4L2_CID_FLASH_INTENSITY:
>                 case V4L2_CID_FLASH_TORCH_INTENSITY:
> @@ -1584,16 +1518,10 @@ static int atomisp_camera_s_ext_ctrls(struct file *file, void *fh,
>         struct video_device *vdev = video_devdata(file);
>         struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
>         struct atomisp_device *isp = video_get_drvdata(vdev);
> -       struct v4l2_subdev *motor;
>         struct v4l2_control ctrl;
>         int i;
>         int ret = 0;
>  
> -       if (!IS_ISP2401)
> -               motor = isp->inputs[asd->input_curr].motor;
> -       else
> -               motor = isp->motor;
> -
>         for (i = 0; i < c->count; i++) {
>                 struct v4l2_ctrl *ctr;
>  
> @@ -1616,18 +1544,6 @@ static int atomisp_camera_s_ext_ctrls(struct file *file, void *fh,
>                                           isp->inputs[asd->input_curr].camera->
>                                           ctrl_handler, &ctrl);
>                         break;
> -               case V4L2_CID_FOCUS_ABSOLUTE:
> -               case V4L2_CID_FOCUS_RELATIVE:
> -               case V4L2_CID_FOCUS_STATUS:
> -               case V4L2_CID_FOCUS_AUTO:
> -                       if (motor)
> -                               ret = v4l2_s_ctrl(NULL, motor->ctrl_handler,
> -                                                 &ctrl);
> -                       else
> -                               ret = v4l2_s_ctrl(NULL,
> -                                                 isp->inputs[asd->input_curr].
> -                                                 camera->ctrl_handler, &ctrl);
> -                       break;
>                 case V4L2_CID_FLASH_STATUS:
>                 case V4L2_CID_FLASH_INTENSITY:
>                 case V4L2_CID_FLASH_TORCH_INTENSITY:
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> index f736e54c7df3..1a936dbe8eb4 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> @@ -823,13 +823,6 @@ static int atomisp_subdev_probe(struct atomisp_device *isp)
>                         isp->sensor_lanes[mipi_port] = subdevs->lanes;
>                         isp->sensor_subdevs[subdevs->port] = subdevs->subdev;
>                         break;
> -               case CAMERA_MOTOR:
> -                       if (isp->motor) {
> -                               dev_warn(isp->dev, "too many atomisp motors\n");
> -                               continue;
> -                       }
> -                       isp->motor = subdevs->subdev;
> -                       break;
>                 case LED_FLASH:
>                         if (isp->flash) {
>                                 dev_warn(isp->dev, "too many atomisp flash devices\n");
> @@ -1066,14 +1059,6 @@ int atomisp_register_device_nodes(struct atomisp_device *isp)
>  
>                 atomisp_init_sensor(input);
>  
> -               /*
> -                * HACK: Currently VCM belongs to primary sensor only, but correct
> -                * approach must be to acquire from platform code which sensor
> -                * owns it.
> -                */
> -               if (i == ATOMISP_CAMERA_PORT_PRIMARY)
> -                       input->motor = isp->motor;
> -
>                 err = media_create_pad_link(&input->camera->entity, 0,
>                                             &isp->csi2_port[i].subdev.entity,
>                                             CSI2_PAD_SINK,
> -- 
> 2.43.0
>

Re: [PATCH 4/9] media: atomisp: Remove ISP controls which get passed through to the camera

[Kieran Bingham]

Quoting Hans de Goede (2024-02-17 11:24:33)
> Drop all ISP controls and ioctls which just get passed through
> to the camera subdev. Instead these calls should be done directly
> at the sensor subdev.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  .../media/atomisp/include/linux/atomisp.h     |  21 --
>  .../staging/media/atomisp/pci/atomisp_ioctl.c | 248 ------------------
>  2 files changed, 269 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/include/linux/atomisp.h b/drivers/staging/media/atomisp/include/linux/atomisp.h
> index d9a7a599038d..b2735a008052 100644
> --- a/drivers/staging/media/atomisp/include/linux/atomisp.h
> +++ b/drivers/staging/media/atomisp/include/linux/atomisp.h
> @@ -837,9 +837,6 @@ enum atomisp_burst_capture_options {
>  #define ATOMISP_IOC_S_PARAMETERS \
>         _IOW('v', BASE_VIDIOC_PRIVATE + 32, struct atomisp_parameters)
>  
> -#define ATOMISP_IOC_EXT_ISP_CTRL \
> -       _IOWR('v', BASE_VIDIOC_PRIVATE + 35, struct atomisp_ext_isp_ctrl)
> -
>  #define ATOMISP_IOC_EXP_ID_UNLOCK \
>         _IOW('v', BASE_VIDIOC_PRIVATE + 36, int)
>  
> @@ -909,19 +906,6 @@ enum atomisp_burst_capture_options {
>  /* Set the flash mode (see enum atomisp_flash_mode) */
>  #define V4L2_CID_FLASH_MODE                (V4L2_CID_CAMERA_LASTP1 + 10)
>  
> -/* VCM slew control */
> -#define V4L2_CID_VCM_SLEW                  (V4L2_CID_CAMERA_LASTP1 + 11)
> -/* VCM step time */
> -#define V4L2_CID_VCM_TIMING                (V4L2_CID_CAMERA_LASTP1 + 12)
> -
> -/* number of frames to skip at stream start */
> -#define V4L2_CID_G_SKIP_FRAMES            (V4L2_CID_CAMERA_LASTP1 + 17)
> -
> -/* Query sensor's 2A status */
> -#define V4L2_CID_2A_STATUS                 (V4L2_CID_CAMERA_LASTP1 + 18)
> -#define V4L2_2A_STATUS_AE_READY            BIT(0)
> -#define V4L2_2A_STATUS_AWB_READY           BIT(1)
> -
>  #define V4L2_CID_RUN_MODE                      (V4L2_CID_CAMERA_LASTP1 + 20)
>  #define ATOMISP_RUN_MODE_VIDEO                 1
>  #define ATOMISP_RUN_MODE_STILL_CAPTURE         2
> @@ -952,11 +936,6 @@ enum atomisp_burst_capture_options {
>  /* Disable digital zoom */
>  #define V4L2_CID_DISABLE_DZ            (V4L2_CID_CAMERA_LASTP1 + 32)
>  
> -#define V4L2_CID_TEST_PATTERN_COLOR_R  (V4L2_CID_CAMERA_LASTP1 + 33)
> -#define V4L2_CID_TEST_PATTERN_COLOR_GR (V4L2_CID_CAMERA_LASTP1 + 34)
> -#define V4L2_CID_TEST_PATTERN_COLOR_GB (V4L2_CID_CAMERA_LASTP1 + 35)
> -#define V4L2_CID_TEST_PATTERN_COLOR_B  (V4L2_CID_CAMERA_LASTP1 + 36)
> -
>  #define V4L2_CID_ATOMISP_SELECT_ISP_VERSION    (V4L2_CID_CAMERA_LASTP1 + 38)
>  
>  #define V4L2_BUF_FLAG_BUFFER_INVALID       0x0400
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> index ef555054fdbf..74cf055cb09b 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> @@ -82,15 +82,6 @@ static struct v4l2_queryctrl ci_v4l2_controls[] = {
>                 .step = 1,
>                 .default_value = 0x00,
>         },
> -       {
> -               .id = V4L2_CID_POWER_LINE_FREQUENCY,
> -               .type = V4L2_CTRL_TYPE_MENU,
> -               .name = "Light frequency filter",
> -               .minimum = 1,
> -               .maximum = 2,
> -               .step = 1,
> -               .default_value = 1,
> -       },
>         {
>                 .id = V4L2_CID_COLORFX,
>                 .type = V4L2_CTRL_TYPE_INTEGER,
> @@ -100,15 +91,6 @@ static struct v4l2_queryctrl ci_v4l2_controls[] = {
>                 .step = 1,
>                 .default_value = 0,
>         },
> -       {
> -               .id = V4L2_CID_COLORFX_CBCR,
> -               .type = V4L2_CTRL_TYPE_INTEGER,
> -               .name = "Image Color Effect CbCr",
> -               .minimum = 0,
> -               .maximum = 0xffff,
> -               .step = 1,
> -               .default_value = 0,
> -       },
>         {
>                 .id = V4L2_CID_ATOMISP_BAD_PIXEL_DETECTION,
>                 .type = V4L2_CTRL_TYPE_INTEGER,
> @@ -172,142 +154,6 @@ static struct v4l2_queryctrl ci_v4l2_controls[] = {
>                 .step = 1,
>                 .default_value = 1,
>         },
> -       {
> -               .id = V4L2_CID_2A_STATUS,
> -               .type = V4L2_CTRL_TYPE_BITMASK,
> -               .name = "AE and AWB status",
> -               .minimum = 0,
> -               .maximum = V4L2_2A_STATUS_AE_READY | V4L2_2A_STATUS_AWB_READY,
> -               .step = 1,
> -               .default_value = 0,
> -       },
> -       {
> -               .id = V4L2_CID_EXPOSURE,
> -               .type = V4L2_CTRL_TYPE_INTEGER,
> -               .name = "exposure",
> -               .minimum = -4,
> -               .maximum = 4,
> -               .step = 1,
> -               .default_value = 0,
> -       },
> -       {
> -               .id = V4L2_CID_EXPOSURE_ZONE_NUM,
> -               .type = V4L2_CTRL_TYPE_INTEGER,
> -               .name = "one-time exposure zone number",
> -               .minimum = 0x0,
> -               .maximum = 0xffff,
> -               .step = 1,
> -               .default_value = 0,
> -       },
> -       {
> -               .id = V4L2_CID_EXPOSURE_AUTO_PRIORITY,
> -               .type = V4L2_CTRL_TYPE_INTEGER,
> -               .name = "Exposure auto priority",
> -               .minimum = V4L2_EXPOSURE_AUTO,
> -               .maximum = V4L2_EXPOSURE_APERTURE_PRIORITY,
> -               .step = 1,
> -               .default_value = V4L2_EXPOSURE_AUTO,
> -       },
> -       {
> -               .id = V4L2_CID_SCENE_MODE,
> -               .type = V4L2_CTRL_TYPE_INTEGER,
> -               .name = "scene mode",
> -               .minimum = 0,
> -               .maximum = 13,
> -               .step = 1,
> -               .default_value = 0,
> -       },
> -       {
> -               .id = V4L2_CID_ISO_SENSITIVITY,
> -               .type = V4L2_CTRL_TYPE_INTEGER,
> -               .name = "iso",
> -               .minimum = -4,
> -               .maximum = 4,
> -               .step = 1,
> -               .default_value = 0,
> -       },
> -       {
> -               .id = V4L2_CID_ISO_SENSITIVITY_AUTO,
> -               .type = V4L2_CTRL_TYPE_INTEGER,
> -               .name = "iso mode",
> -               .minimum = V4L2_ISO_SENSITIVITY_MANUAL,
> -               .maximum = V4L2_ISO_SENSITIVITY_AUTO,
> -               .step = 1,
> -               .default_value = V4L2_ISO_SENSITIVITY_AUTO,
> -       },
> -       {
> -               .id = V4L2_CID_AUTO_N_PRESET_WHITE_BALANCE,
> -               .type = V4L2_CTRL_TYPE_INTEGER,
> -               .name = "white balance",
> -               .minimum = 0,
> -               .maximum = 9,
> -               .step = 1,
> -               .default_value = 0,
> -       },
> -       {
> -               .id = V4L2_CID_EXPOSURE_METERING,
> -               .type = V4L2_CTRL_TYPE_MENU,
> -               .name = "metering",
> -               .minimum = 0,
> -               .maximum = 3,
> -               .step = 1,
> -               .default_value = 1,
> -       },
> -       {
> -               .id = V4L2_CID_3A_LOCK,
> -               .type = V4L2_CTRL_TYPE_BITMASK,
> -               .name = "3a lock",
> -               .minimum = 0,
> -               .maximum = V4L2_LOCK_EXPOSURE | V4L2_LOCK_WHITE_BALANCE
> -               | V4L2_LOCK_FOCUS,
> -               .step = 1,
> -               .default_value = 0,
> -       },
> -       {
> -               .id = V4L2_CID_TEST_PATTERN,
> -               .type = V4L2_CTRL_TYPE_INTEGER,
> -               .name = "Test Pattern",
> -               .minimum = 0,
> -               .maximum = 0xffff,
> -               .step = 1,
> -               .default_value = 0,
> -       },
> -       {
> -               .id = V4L2_CID_TEST_PATTERN_COLOR_R,
> -               .type = V4L2_CTRL_TYPE_INTEGER,
> -               .name = "Test Pattern Solid Color R",
> -               .minimum = INT_MIN,
> -               .maximum = INT_MAX,
> -               .step = 1,
> -               .default_value = 0,
> -       },
> -       {
> -               .id = V4L2_CID_TEST_PATTERN_COLOR_GR,
> -               .type = V4L2_CTRL_TYPE_INTEGER,
> -               .name = "Test Pattern Solid Color GR",
> -               .minimum = INT_MIN,
> -               .maximum = INT_MAX,
> -               .step = 1,
> -               .default_value = 0,
> -       },
> -       {
> -               .id = V4L2_CID_TEST_PATTERN_COLOR_GB,
> -               .type = V4L2_CTRL_TYPE_INTEGER,
> -               .name = "Test Pattern Solid Color GB",
> -               .minimum = INT_MIN,
> -               .maximum = INT_MAX,
> -               .step = 1,
> -               .default_value = 0,
> -       },
> -       {
> -               .id = V4L2_CID_TEST_PATTERN_COLOR_B,
> -               .type = V4L2_CTRL_TYPE_INTEGER,
> -               .name = "Test Pattern Solid Color B",
> -               .minimum = INT_MIN,
> -               .maximum = INT_MAX,
> -               .step = 1,
> -               .default_value = 0,
> -       },
>  };
>  
>  static const u32 ctrls_num = ARRAY_SIZE(ci_v4l2_controls);
> @@ -1248,7 +1094,6 @@ static int atomisp_g_ctrl(struct file *file, void *fh,
>  {
>         struct video_device *vdev = video_devdata(file);
>         struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
> -       struct atomisp_device *isp = video_get_drvdata(vdev);
>         int i, ret = -EINVAL;
>  
>         for (i = 0; i < ctrls_num; i++) {
> @@ -1262,27 +1107,6 @@ static int atomisp_g_ctrl(struct file *file, void *fh,
>                 return ret;
>  
>         switch (control->id) {
> -       case V4L2_CID_IRIS_ABSOLUTE:
> -       case V4L2_CID_EXPOSURE_ABSOLUTE:
> -       case V4L2_CID_2A_STATUS:
> -       case V4L2_CID_AUTO_N_PRESET_WHITE_BALANCE:
> -       case V4L2_CID_EXPOSURE:
> -       case V4L2_CID_EXPOSURE_AUTO:
> -       case V4L2_CID_SCENE_MODE:
> -       case V4L2_CID_ISO_SENSITIVITY:
> -       case V4L2_CID_ISO_SENSITIVITY_AUTO:
> -       case V4L2_CID_CONTRAST:
> -       case V4L2_CID_SATURATION:
> -       case V4L2_CID_SHARPNESS:
> -       case V4L2_CID_3A_LOCK:
> -       case V4L2_CID_EXPOSURE_ZONE_NUM:
> -       case V4L2_CID_TEST_PATTERN:
> -       case V4L2_CID_TEST_PATTERN_COLOR_R:
> -       case V4L2_CID_TEST_PATTERN_COLOR_GR:
> -       case V4L2_CID_TEST_PATTERN_COLOR_GB:
> -       case V4L2_CID_TEST_PATTERN_COLOR_B:
> -               return v4l2_g_ctrl(isp->inputs[asd->input_curr].camera->
> -                                  ctrl_handler, control);
>         case V4L2_CID_COLORFX:
>                 ret = atomisp_color_effect(asd, 0, &control->value);
>                 break;
> @@ -1322,7 +1146,6 @@ static int atomisp_s_ctrl(struct file *file, void *fh,
>  {
>         struct video_device *vdev = video_devdata(file);
>         struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
> -       struct atomisp_device *isp = video_get_drvdata(vdev);
>         int i, ret = -EINVAL;
>  
>         for (i = 0; i < ctrls_num; i++) {
> @@ -1336,28 +1159,6 @@ static int atomisp_s_ctrl(struct file *file, void *fh,
>                 return ret;
>  
>         switch (control->id) {
> -       case V4L2_CID_AUTO_N_PRESET_WHITE_BALANCE:
> -       case V4L2_CID_EXPOSURE:
> -       case V4L2_CID_EXPOSURE_AUTO:
> -       case V4L2_CID_EXPOSURE_AUTO_PRIORITY:
> -       case V4L2_CID_SCENE_MODE:
> -       case V4L2_CID_ISO_SENSITIVITY:
> -       case V4L2_CID_ISO_SENSITIVITY_AUTO:
> -       case V4L2_CID_POWER_LINE_FREQUENCY:
> -       case V4L2_CID_EXPOSURE_METERING:
> -       case V4L2_CID_CONTRAST:
> -       case V4L2_CID_SATURATION:
> -       case V4L2_CID_SHARPNESS:
> -       case V4L2_CID_3A_LOCK:
> -       case V4L2_CID_COLORFX_CBCR:
> -       case V4L2_CID_TEST_PATTERN:
> -       case V4L2_CID_TEST_PATTERN_COLOR_R:
> -       case V4L2_CID_TEST_PATTERN_COLOR_GR:
> -       case V4L2_CID_TEST_PATTERN_COLOR_GB:
> -       case V4L2_CID_TEST_PATTERN_COLOR_B:
> -               return v4l2_s_ctrl(NULL,
> -                                  isp->inputs[asd->input_curr].camera->
> -                                  ctrl_handler, control);

Looks like a reasonable clean up from that.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>         case V4L2_CID_COLORFX:
>                 ret = atomisp_color_effect(asd, 1, &control->value);
>                 break;
> @@ -1398,7 +1199,6 @@ static int atomisp_s_ctrl(struct file *file, void *fh,
>  static int atomisp_queryctl(struct file *file, void *fh,
>                             struct v4l2_queryctrl *qc)
>  {
> -       struct video_device *vdev = video_devdata(file);
>         int i, ret = -EINVAL;
>  
>         if (qc->id & V4L2_CTRL_FLAG_NEXT_CTRL)
> @@ -1433,23 +1233,6 @@ static int atomisp_camera_g_ext_ctrls(struct file *file, void *fh,
>                 ctrl.id = c->controls[i].id;
>                 ctrl.value = c->controls[i].value;
>                 switch (ctrl.id) {
> -               case V4L2_CID_EXPOSURE_ABSOLUTE:
> -               case V4L2_CID_EXPOSURE_AUTO:
> -               case V4L2_CID_IRIS_ABSOLUTE:
> -               case V4L2_CID_3A_LOCK:
> -               case V4L2_CID_TEST_PATTERN:
> -               case V4L2_CID_TEST_PATTERN_COLOR_R:
> -               case V4L2_CID_TEST_PATTERN_COLOR_GR:
> -               case V4L2_CID_TEST_PATTERN_COLOR_GB:
> -               case V4L2_CID_TEST_PATTERN_COLOR_B:
> -                       /*
> -                        * Exposure related control will be handled by sensor
> -                        * driver
> -                        */
> -                       ret =
> -                           v4l2_g_ctrl(isp->inputs[asd->input_curr].camera->
> -                                       ctrl_handler, &ctrl);
> -                       break;
>                 case V4L2_CID_FLASH_STATUS:
>                 case V4L2_CID_FLASH_INTENSITY:
>                 case V4L2_CID_FLASH_TORCH_INTENSITY:
> @@ -1466,11 +1249,6 @@ static int atomisp_camera_g_ext_ctrls(struct file *file, void *fh,
>                 case V4L2_CID_ZOOM_ABSOLUTE:
>                         ret = atomisp_digital_zoom(asd, 0, &ctrl.value);
>                         break;
> -               case V4L2_CID_G_SKIP_FRAMES:
> -                       ret = v4l2_subdev_call(
> -                                 isp->inputs[asd->input_curr].camera,
> -                                 sensor, g_skip_frames, (u32 *)&ctrl.value);
> -                       break;
>                 default:
>                         ret = -EINVAL;
>                 }
> @@ -1528,22 +1306,6 @@ static int atomisp_camera_s_ext_ctrls(struct file *file, void *fh,
>                 ctrl.id = c->controls[i].id;
>                 ctrl.value = c->controls[i].value;
>                 switch (ctrl.id) {
> -               case V4L2_CID_EXPOSURE_ABSOLUTE:
> -               case V4L2_CID_EXPOSURE_AUTO:
> -               case V4L2_CID_EXPOSURE_METERING:
> -               case V4L2_CID_IRIS_ABSOLUTE:
> -               case V4L2_CID_VCM_TIMING:
> -               case V4L2_CID_VCM_SLEW:
> -               case V4L2_CID_3A_LOCK:
> -               case V4L2_CID_TEST_PATTERN:
> -               case V4L2_CID_TEST_PATTERN_COLOR_R:
> -               case V4L2_CID_TEST_PATTERN_COLOR_GR:
> -               case V4L2_CID_TEST_PATTERN_COLOR_GB:
> -               case V4L2_CID_TEST_PATTERN_COLOR_B:
> -                       ret = v4l2_s_ctrl(NULL,
> -                                         isp->inputs[asd->input_curr].camera->
> -                                         ctrl_handler, &ctrl);
> -                       break;
>                 case V4L2_CID_FLASH_STATUS:
>                 case V4L2_CID_FLASH_INTENSITY:
>                 case V4L2_CID_FLASH_TORCH_INTENSITY:
> @@ -1692,7 +1454,6 @@ static long atomisp_vidioc_default(struct file *file, void *fh,
>                                    bool valid_prio, unsigned int cmd, void *arg)
>  {
>         struct video_device *vdev = video_devdata(file);
> -       struct atomisp_device *isp = video_get_drvdata(vdev);
>         struct atomisp_sub_device *asd = atomisp_to_video_pipe(vdev)->asd;
>         int err;
>  
> @@ -1839,11 +1600,6 @@ static long atomisp_vidioc_default(struct file *file, void *fh,
>                 err = atomisp_fixed_pattern_table(asd, arg);
>                 break;
>  
> -       case ATOMISP_IOC_S_EXPOSURE:
> -               err = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> -                                      core, ioctl, cmd, arg);
> -               break;
> -
>         case ATOMISP_IOC_S_ISP_SHD_TAB:
>                 err = atomisp_set_shading_table(asd, arg);
>                 break;
> @@ -1860,10 +1616,6 @@ static long atomisp_vidioc_default(struct file *file, void *fh,
>                 err = atomisp_set_parameters(vdev, arg);
>                 break;
>  
> -       case ATOMISP_IOC_EXT_ISP_CTRL:
> -               err = v4l2_subdev_call(isp->inputs[asd->input_curr].camera,
> -                                      core, ioctl, cmd, arg);
> -               break;
>         case ATOMISP_IOC_EXP_ID_UNLOCK:
>                 err = atomisp_exp_id_unlock(asd, arg);
>                 break;
> -- 
> 2.43.0
>

Re: [PATCH 6/9] media: atomisp: Add DMABUF support

[Kieran Bingham]

Quoting Hans de Goede (2024-02-17 11:24:35)
> Add DMABUF support and while at it drop userptr support.
> 
> Now that atomisp has been ported to videobuf2 this is trivial.

Seems 'too' easy, Nice.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/staging/media/atomisp/pci/atomisp_ioctl.c  | 1 +
>  drivers/staging/media/atomisp/pci/atomisp_subdev.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> index 74cf055cb09b..fa6c9f0ea6eb 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_ioctl.c
> @@ -1666,6 +1666,7 @@ const struct v4l2_ioctl_ops atomisp_ioctl_ops = {
>         .vidioc_querybuf = vb2_ioctl_querybuf,
>         .vidioc_qbuf = atomisp_qbuf_wrapper,
>         .vidioc_dqbuf = atomisp_dqbuf_wrapper,
> +       .vidioc_expbuf = vb2_ioctl_expbuf,
>         .vidioc_streamon = vb2_ioctl_streamon,
>         .vidioc_streamoff = vb2_ioctl_streamoff,
>         .vidioc_default = atomisp_vidioc_default,
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
> index 7f1ca05ce54a..8253b6faf8cd 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
> @@ -765,7 +765,7 @@ static int atomisp_init_subdev_pipe(struct atomisp_sub_device *asd,
>  
>         /* Init videobuf2 queue structure */
>         pipe->vb_queue.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> -       pipe->vb_queue.io_modes = VB2_MMAP | VB2_USERPTR;
> +       pipe->vb_queue.io_modes = VB2_MMAP | VB2_DMABUF;
>         pipe->vb_queue.buf_struct_size = sizeof(struct ia_css_frame);
>         pipe->vb_queue.ops = &atomisp_vb2_ops;
>         pipe->vb_queue.mem_ops = &vb2_vmalloc_memops;
> -- 
> 2.43.0
>

Re: [PATCH 8/9] media: atomisp: Make MC link from ISP to /dev/video# output node immutable

[Kieran Bingham]

Quoting Hans de Goede (2024-02-17 11:24:37)
> The link from the ISP's source pad to the /dev/video# output v4l2-dev
> is always enabled and immutable, mark it as such.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/staging/media/atomisp/pci/atomisp_v4l2.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> index 1a936dbe8eb4..86028721e7cb 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c
> @@ -1090,7 +1090,8 @@ int atomisp_register_device_nodes(struct atomisp_device *isp)
>                 return err;
>  
>         err = media_create_pad_link(&isp->asd.subdev.entity, ATOMISP_SUBDEV_PAD_SOURCE,
> -                                   &isp->asd.video_out.vdev.entity, 0, 0);
> +                                   &isp->asd.video_out.vdev.entity, 0,
> +                                   MEDIA_LNK_FL_ENABLED | MEDIA_LNK_FL_IMMUTABLE);
>         if (err)
>                 return err;
>  
> -- 
> 2.43.0
>

Re: [PATCH 7/9] media: atomisp: Change ISP subdev name to "ATOM ISP"

[Kieran Bingham]

Quoting Hans de Goede (2024-02-17 11:24:36)
> Change the generic "ATOMISP_SUBDEV" name to "ATOM ISP" to make clear
> that this is the subdev for the ISP itself.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/staging/media/atomisp/pci/atomisp_subdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
> index 8253b6faf8cd..822fe7d129e2 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
> @@ -799,7 +799,7 @@ static int isp_subdev_init_entities(struct atomisp_sub_device *asd)
>         int ret;
>  
>         v4l2_subdev_init(sd, &isp_subdev_v4l2_ops);
> -       sprintf(sd->name, "ATOMISP_SUBDEV");
> +       sprintf(sd->name, "ATOM ISP");

Pure bikeshedding, but I'd probably lower the shouting to just 
	"Atom ISP"

Either way saying it's a subdev on a subdev isn't much of a value add so 

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>         v4l2_set_subdevdata(sd, asd);
>         sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS | V4L2_SUBDEV_FL_HAS_DEVNODE;
>  
> -- 
> 2.43.0
>

Re: [PATCH 7/9] media: atomisp: Change ISP subdev name to "ATOM ISP"

[Andy Shevchenko]

On Sat, Feb 17, 2024 at 6:00 PM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
> Quoting Hans de Goede (2024-02-17 11:24:36)

...

> Pure bikeshedding, but I'd probably lower the shouting to just
>         "Atom ISP"

You beat me to it, +1.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 9/9] media: atomisp: Implement link_setup op for ISP subdev MC entity

[Andy Shevchenko]

On Sat, Feb 17, 2024 at 1:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> The atomisp driver's Android heritage makes it weird in that
> even though it uses MC + subdev-s it is designed to primarily
> be controlled through its /dev/video# node.
>
> It implements s_input on /dev/video# to select which sensor to use,
> while ignoring setup_link calls to enable a link to another sensor.
>
> Add support for selecting the active sensor the MC way by adding
> setup_link support.

->link_setup()

> The implementation is a bit convoluted due to the atomisp driver's
> heritage.

...

>  #include "atomisp_common.h"
>  #include "atomisp_compat.h"
>  #include "atomisp_fops.h"
> +#include "atomisp_ioctl.h"
>  #include "atomisp_internal.h"

Hmm... Perhaps keep it ordered?

...

> +       mutex_lock(&isp->mutex);

Side note: Are you planning to use cleanup.h at some point?

> +       ret = atomisp_pipe_check(&asd->video_out, true);
> +       if (ret == 0)
> +               asd->input_curr = i;
> +       mutex_unlock(&isp->mutex);
> +
> +       return ret;


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/9] media: atomisp: Changes for libcamera support

[Andy Shevchenko]

On Sat, Feb 17, 2024 at 1:24 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi All,
>
> Here is a series of cleanups (prep work) + some small(ish)
> changes to make the atomisp driver work with libcamera.
>
> This has been tested with libcamera's simple pipelinehandler
> together with an ov2680 sensor.

With some minor comments,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>


> p.s.
>
> Andy, I known I still need to do some small cleanups from my last
> pull-requests. I have this on my TODO list :)

Hope you will find time to address. Thanks for maintaining this driver!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 9/9] media: atomisp: Implement link_setup op for ISP subdev MC entity

[Hans de Goede]

Hi,

On 2/17/24 19:52, Andy Shevchenko wrote:
> On Sat, Feb 17, 2024 at 1:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> The atomisp driver's Android heritage makes it weird in that
>> even though it uses MC + subdev-s it is designed to primarily
>> be controlled through its /dev/video# node.
>>
>> It implements s_input on /dev/video# to select which sensor to use,
>> while ignoring setup_link calls to enable a link to another sensor.
>>
>> Add support for selecting the active sensor the MC way by adding
>> setup_link support.
> 
> ->link_setup()
> 
>> The implementation is a bit convoluted due to the atomisp driver's
>> heritage.
> 
> ...
> 
>>  #include "atomisp_common.h"
>>  #include "atomisp_compat.h"
>>  #include "atomisp_fops.h"
>> +#include "atomisp_ioctl.h"
>>  #include "atomisp_internal.h"
> 
> Hmm... Perhaps keep it ordered?

Yeah my bad, I intended to keep it ordered but I somehow got it wrong.
this and the commit msg remark are both fixed in my personal tree now.

> 
> ...
> 
>> +       mutex_lock(&isp->mutex);
> 
> Side note: Are you planning to use cleanup.h at some point?

Maybe. I have no objections against. For functions needing
e.g. heap memory only locally it certainly makes sense.

Regards,

Hans



> 
>> +       ret = atomisp_pipe_check(&asd->video_out, true);
>> +       if (ret == 0)
>> +               asd->input_curr = i;
>> +       mutex_unlock(&isp->mutex);
>> +
>> +       return ret;
> 
> 

Re: [PATCH 7/9] media: atomisp: Change ISP subdev name to "ATOM ISP"

[Hans de Goede]

Hi,

On 2/17/24 17:00, Kieran Bingham wrote:
> Quoting Hans de Goede (2024-02-17 11:24:36)
>> Change the generic "ATOMISP_SUBDEV" name to "ATOM ISP" to make clear
>> that this is the subdev for the ISP itself.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/staging/media/atomisp/pci/atomisp_subdev.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/media/atomisp/pci/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
>> index 8253b6faf8cd..822fe7d129e2 100644
>> --- a/drivers/staging/media/atomisp/pci/atomisp_subdev.c
>> +++ b/drivers/staging/media/atomisp/pci/atomisp_subdev.c
>> @@ -799,7 +799,7 @@ static int isp_subdev_init_entities(struct atomisp_sub_device *asd)
>>         int ret;
>>  
>>         v4l2_subdev_init(sd, &isp_subdev_v4l2_ops);
>> -       sprintf(sd->name, "ATOMISP_SUBDEV");
>> +       sprintf(sd->name, "ATOM ISP");
> 
> Pure bikeshedding, but I'd probably lower the shouting to just 
> 	"Atom ISP"

Works for me, I've fixed this in my personal tree.

> Either way saying it's a subdev on a subdev isn't much of a value add so 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Thank you for this and all the other reviews.

Regards,

Hans

Re: [PATCH 0/9] media: atomisp: Changes for libcamera support

[Hans de Goede]

Hi Andy,

On 2/17/24 19:53, Andy Shevchenko wrote:
> On Sat, Feb 17, 2024 at 1:24 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi All,
>>
>> Here is a series of cleanups (prep work) + some small(ish)
>> changes to make the atomisp driver work with libcamera.
>>
>> This has been tested with libcamera's simple pipelinehandler
>> together with an ov2680 sensor.
> 
> With some minor comments,
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Thank you for all the reviews.

Regards,

Hans