[PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media

[PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media

[Hans de Goede]

Hi All,

Here is a patch series to bring the staging atomisp gc0310 driver inline
with the expectations for new v4l2 raw camera sensor drivers and then
after this move it to drivers/media/i2c since it is now just another
normal modern v4l2 raw camera sensor driver.

I plan to send a pull-request for patches 1-22 to get added to
media-commiters/next once they are reviewed. After that Sakari can review
the final resulting gc0310.c file and merge the last patch moving it.

Regards,

Hans


Hans de Goede (23):
  media: atomisp: gc0310: Rename "dev" function variable to "sensor"
  media: atomisp: gc0310: Drop unused GC0310_FOCAL_LENGTH_NUM define
  media: atomisp: gc0310: Modify vblank value to run at 30 fps
  media: atomisp: gc0310: Switch to CCI register access helpers
  media: atomisp: gc0310: Use V4L2_CID_ANALOGUE_GAIN for gain control
  media: atomisp: gc0310: Add selection API support
  media: atomisp: gc0310: Add link-frequency and pixelrate controls
  media: atomisp: gc0310: Add vblank and hblank controls
  media: atomisp: gc0310: Add camera orientation and sensor rotation
    controls
  media: atomisp: gc0310: Limit max exposure value to mode-height +
    vblank
  media: atomisp: gc0310: Add check_hwcfg() function
  media: atomisp: gc0310: Fix power on/off sleep times
  media: atomisp: gc0310: Remove unused is_streaming variable
  media: atomisp: gc0310: Switch to {enable,disable}_streams
  media: atomisp: gc0310: Switch to using the sub-device state lock
  media: atomisp: gc0310: Implement internal_ops.init_state
  media: atomisp: gc0310: Use v4l2_subdev_get_fmt() as
    v4l2_subdev_pad_ops.get_fmt()
  media: atomisp: gc0310: Switch to using sd.active_state fmt
  media: atomisp: gc0310: Move and rename suspend/resume functions
  media: atomisp: gc0310: runtime-PM fixes
  media: atomisp: gc0310: Drop gc0310_get_frame_interval()
  media: atomisp: gc0310: Drop gc0310_g_skip_frames()
  media: Move gc0310 sensor drivers to drivers/media/i2c/

 MAINTAINERS                                   |   6 +
 drivers/media/i2c/Kconfig                     |  10 +
 drivers/media/i2c/Makefile                    |   1 +
 drivers/media/i2c/gc0310.c                    | 800 ++++++++++++++++++
 drivers/staging/media/atomisp/i2c/Kconfig     |   8 -
 drivers/staging/media/atomisp/i2c/Makefile    |   1 -
 .../media/atomisp/i2c/atomisp-gc0310.c        | 712 ----------------
 7 files changed, 817 insertions(+), 721 deletions(-)
 create mode 100644 drivers/media/i2c/gc0310.c
 delete mode 100644 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c

-- 
2.49.0

[PATCH 01/23] media: atomisp: gc0310: Rename "dev" function variable to "sensor"

[Hans de Goede]

Many functions on the gc0310 driver use a function local variable called
"dev" but these variable's type is not "struct device *" type as one would
expect based on the name. Instead they point to the gc0310 driver data
struct.

Rename these variables to sensor to make their purpose more clear.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/i2c/atomisp-gc0310.c        | 112 +++++++++---------
 1 file changed, 56 insertions(+), 56 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index d35394f1ddbb..cc74e90a1457 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -282,16 +282,16 @@ static int gc0310_write_reg_array(struct i2c_client *client,
 	return 0;
 }
 
-static int gc0310_exposure_set(struct gc0310_device *dev, u32 exp)
+static int gc0310_exposure_set(struct gc0310_device *sensor, u32 exp)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&dev->sd);
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
 
 	return i2c_smbus_write_word_swapped(client, GC0310_AEC_PK_EXPO_H, exp);
 }
 
-static int gc0310_gain_set(struct gc0310_device *dev, u32 gain)
+static int gc0310_gain_set(struct gc0310_device *sensor, u32 gain)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&dev->sd);
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
 	u8 again, dgain;
 	int ret;
 
@@ -317,27 +317,27 @@ static int gc0310_gain_set(struct gc0310_device *dev, u32 gain)
 
 static int gc0310_s_ctrl(struct v4l2_ctrl *ctrl)
 {
-	struct gc0310_device *dev =
+	struct gc0310_device *sensor =
 		container_of(ctrl->handler, struct gc0310_device, ctrls.handler);
 	int ret;
 
 	/* Only apply changes to the controls if the device is powered up */
-	if (!pm_runtime_get_if_in_use(dev->sd.dev))
+	if (!pm_runtime_get_if_in_use(sensor->sd.dev))
 		return 0;
 
 	switch (ctrl->id) {
 	case V4L2_CID_EXPOSURE:
-		ret = gc0310_exposure_set(dev, ctrl->val);
+		ret = gc0310_exposure_set(sensor, ctrl->val);
 		break;
 	case V4L2_CID_GAIN:
-		ret = gc0310_gain_set(dev, ctrl->val);
+		ret = gc0310_gain_set(sensor, ctrl->val);
 		break;
 	default:
 		ret = -EINVAL;
 		break;
 	}
 
-	pm_runtime_put(dev->sd.dev);
+	pm_runtime_put(sensor->sd.dev);
 	return ret;
 }
 
@@ -346,14 +346,14 @@ static const struct v4l2_ctrl_ops ctrl_ops = {
 };
 
 static struct v4l2_mbus_framefmt *
-gc0310_get_pad_format(struct gc0310_device *dev,
+gc0310_get_pad_format(struct gc0310_device *sensor,
 		      struct v4l2_subdev_state *state,
 		      unsigned int pad, enum v4l2_subdev_format_whence which)
 {
 	if (which == V4L2_SUBDEV_FORMAT_TRY)
 		return v4l2_subdev_state_get_format(state, pad);
 
-	return &dev->mode.fmt;
+	return &sensor->mode.fmt;
 }
 
 /* The GC0310 currently only supports 1 fixed fmt */
@@ -370,10 +370,10 @@ static int gc0310_set_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_state *sd_state,
 			  struct v4l2_subdev_format *format)
 {
-	struct gc0310_device *dev = to_gc0310_sensor(sd);
+	struct gc0310_device *sensor = to_gc0310_sensor(sd);
 	struct v4l2_mbus_framefmt *fmt;
 
-	fmt = gc0310_get_pad_format(dev, sd_state, format->pad, format->which);
+	fmt = gc0310_get_pad_format(sensor, sd_state, format->pad, format->which);
 	gc0310_fill_format(fmt);
 
 	format->format = *fmt;
@@ -384,10 +384,10 @@ static int gc0310_get_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_state *sd_state,
 			  struct v4l2_subdev_format *format)
 {
-	struct gc0310_device *dev = to_gc0310_sensor(sd);
+	struct gc0310_device *sensor = to_gc0310_sensor(sd);
 	struct v4l2_mbus_framefmt *fmt;
 
-	fmt = gc0310_get_pad_format(dev, sd_state, format->pad, format->which);
+	fmt = gc0310_get_pad_format(sensor, sd_state, format->pad, format->which);
 	format->format = *fmt;
 	return 0;
 }
@@ -424,12 +424,12 @@ static int gc0310_detect(struct i2c_client *client)
 
 static int gc0310_s_stream(struct v4l2_subdev *sd, int enable)
 {
-	struct gc0310_device *dev = to_gc0310_sensor(sd);
+	struct gc0310_device *sensor = to_gc0310_sensor(sd);
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	int ret = 0;
 
 	dev_dbg(&client->dev, "%s S enable=%d\n", __func__, enable);
-	mutex_lock(&dev->input_lock);
+	mutex_lock(&sensor->input_lock);
 
 	if (enable) {
 		ret = pm_runtime_get_sync(&client->dev);
@@ -449,7 +449,7 @@ static int gc0310_s_stream(struct v4l2_subdev *sd, int enable)
 			goto error_power_down;
 
 		/* restore value of all ctrls */
-		ret = __v4l2_ctrl_handler_setup(&dev->ctrls.handler);
+		ret = __v4l2_ctrl_handler_setup(&sensor->ctrls.handler);
 		if (ret)
 			goto error_power_down;
 
@@ -475,14 +475,14 @@ static int gc0310_s_stream(struct v4l2_subdev *sd, int enable)
 	if (!enable)
 		pm_runtime_put(&client->dev);
 
-	dev->is_streaming = enable;
-	mutex_unlock(&dev->input_lock);
+	sensor->is_streaming = enable;
+	mutex_unlock(&sensor->input_lock);
 	return 0;
 
 error_power_down:
 	pm_runtime_put(&client->dev);
-	dev->is_streaming = false;
-	mutex_unlock(&dev->input_lock);
+	sensor->is_streaming = false;
+	mutex_unlock(&sensor->input_lock);
 	return ret;
 }
 
@@ -559,21 +559,21 @@ static const struct v4l2_subdev_ops gc0310_ops = {
 	.sensor = &gc0310_sensor_ops,
 };
 
-static int gc0310_init_controls(struct gc0310_device *dev)
+static int gc0310_init_controls(struct gc0310_device *sensor)
 {
-	struct v4l2_ctrl_handler *hdl = &dev->ctrls.handler;
+	struct v4l2_ctrl_handler *hdl = &sensor->ctrls.handler;
 
 	v4l2_ctrl_handler_init(hdl, 2);
 
 	/* Use the same lock for controls as for everything else */
-	hdl->lock = &dev->input_lock;
-	dev->sd.ctrl_handler = hdl;
+	hdl->lock = &sensor->input_lock;
+	sensor->sd.ctrl_handler = hdl;
 
-	dev->ctrls.exposure =
+	sensor->ctrls.exposure =
 		v4l2_ctrl_new_std(hdl, &ctrl_ops, V4L2_CID_EXPOSURE, 0, 4095, 1, 1023);
 
 	/* 32 steps at base gain 1 + 64 half steps at base gain 2 */
-	dev->ctrls.gain =
+	sensor->ctrls.gain =
 		v4l2_ctrl_new_std(hdl, &ctrl_ops, V4L2_CID_GAIN, 0, 95, 1, 31);
 
 	return hdl->error;
@@ -582,21 +582,21 @@ static int gc0310_init_controls(struct gc0310_device *dev)
 static void gc0310_remove(struct i2c_client *client)
 {
 	struct v4l2_subdev *sd = i2c_get_clientdata(client);
-	struct gc0310_device *dev = to_gc0310_sensor(sd);
+	struct gc0310_device *sensor = to_gc0310_sensor(sd);
 
 	dev_dbg(&client->dev, "gc0310_remove...\n");
 
 	v4l2_async_unregister_subdev(sd);
-	media_entity_cleanup(&dev->sd.entity);
-	v4l2_ctrl_handler_free(&dev->ctrls.handler);
-	mutex_destroy(&dev->input_lock);
+	media_entity_cleanup(&sensor->sd.entity);
+	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
+	mutex_destroy(&sensor->input_lock);
 	pm_runtime_disable(&client->dev);
 }
 
 static int gc0310_probe(struct i2c_client *client)
 {
 	struct fwnode_handle *ep_fwnode;
-	struct gc0310_device *dev;
+	struct gc0310_device *sensor;
 	int ret;
 
 	/*
@@ -609,25 +609,25 @@ static int gc0310_probe(struct i2c_client *client)
 
 	fwnode_handle_put(ep_fwnode);
 
-	dev = devm_kzalloc(&client->dev, sizeof(*dev), GFP_KERNEL);
-	if (!dev)
+	sensor = devm_kzalloc(&client->dev, sizeof(*sensor), GFP_KERNEL);
+	if (!sensor)
 		return -ENOMEM;
 
-	dev->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
-	if (IS_ERR(dev->reset)) {
-		return dev_err_probe(&client->dev, PTR_ERR(dev->reset),
+	sensor->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(sensor->reset)) {
+		return dev_err_probe(&client->dev, PTR_ERR(sensor->reset),
 				     "getting reset GPIO\n");
 	}
 
-	dev->powerdown = devm_gpiod_get(&client->dev, "powerdown", GPIOD_OUT_HIGH);
-	if (IS_ERR(dev->powerdown)) {
-		return dev_err_probe(&client->dev, PTR_ERR(dev->powerdown),
+	sensor->powerdown = devm_gpiod_get(&client->dev, "powerdown", GPIOD_OUT_HIGH);
+	if (IS_ERR(sensor->powerdown)) {
+		return dev_err_probe(&client->dev, PTR_ERR(sensor->powerdown),
 				     "getting powerdown GPIO\n");
 	}
 
-	mutex_init(&dev->input_lock);
-	v4l2_i2c_subdev_init(&dev->sd, client, &gc0310_ops);
-	gc0310_fill_format(&dev->mode.fmt);
+	mutex_init(&sensor->input_lock);
+	v4l2_i2c_subdev_init(&sensor->sd, client, &gc0310_ops);
+	gc0310_fill_format(&sensor->mode.fmt);
 
 	pm_runtime_set_suspended(&client->dev);
 	pm_runtime_enable(&client->dev);
@@ -640,23 +640,23 @@ static int gc0310_probe(struct i2c_client *client)
 		return ret;
 	}
 
-	dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
-	dev->pad.flags = MEDIA_PAD_FL_SOURCE;
-	dev->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+	sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
+	sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
 
-	ret = gc0310_init_controls(dev);
+	ret = gc0310_init_controls(sensor);
 	if (ret) {
 		gc0310_remove(client);
 		return ret;
 	}
 
-	ret = media_entity_pads_init(&dev->sd.entity, 1, &dev->pad);
+	ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
 	if (ret) {
 		gc0310_remove(client);
 		return ret;
 	}
 
-	ret = v4l2_async_register_subdev_sensor(&dev->sd);
+	ret = v4l2_async_register_subdev_sensor(&sensor->sd);
 	if (ret) {
 		gc0310_remove(client);
 		return ret;
@@ -668,22 +668,22 @@ static int gc0310_probe(struct i2c_client *client)
 static int gc0310_suspend(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
-	struct gc0310_device *gc0310_dev = to_gc0310_sensor(sd);
+	struct gc0310_device *sensor = to_gc0310_sensor(sd);
 
-	gpiod_set_value_cansleep(gc0310_dev->powerdown, 1);
-	gpiod_set_value_cansleep(gc0310_dev->reset, 1);
+	gpiod_set_value_cansleep(sensor->powerdown, 1);
+	gpiod_set_value_cansleep(sensor->reset, 1);
 	return 0;
 }
 
 static int gc0310_resume(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
-	struct gc0310_device *gc0310_dev = to_gc0310_sensor(sd);
+	struct gc0310_device *sensor = to_gc0310_sensor(sd);
 
 	usleep_range(10000, 15000);
-	gpiod_set_value_cansleep(gc0310_dev->reset, 0);
+	gpiod_set_value_cansleep(sensor->reset, 0);
 	usleep_range(10000, 15000);
-	gpiod_set_value_cansleep(gc0310_dev->powerdown, 0);
+	gpiod_set_value_cansleep(sensor->powerdown, 0);
 
 	return 0;
 }
-- 
2.49.0

Re: [PATCH 01/23] media: atomisp: gc0310: Rename "dev" function variable to "sensor"

[Kieran Bingham]

Quoting Hans de Goede (2025-05-17 12:40:44)
> Many functions on the gc0310 driver use a function local variable called
> "dev" but these variable's type is not "struct device *" type as one would
> expect based on the name. Instead they point to the gc0310 driver data
> struct.
> 
> Rename these variables to sensor to make their purpose more clear.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

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

> ---
>  .../media/atomisp/i2c/atomisp-gc0310.c        | 112 +++++++++---------
>  1 file changed, 56 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> index d35394f1ddbb..cc74e90a1457 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> @@ -282,16 +282,16 @@ static int gc0310_write_reg_array(struct i2c_client *client,
>         return 0;
>  }
>  
> -static int gc0310_exposure_set(struct gc0310_device *dev, u32 exp)
> +static int gc0310_exposure_set(struct gc0310_device *sensor, u32 exp)
>  {
> -       struct i2c_client *client = v4l2_get_subdevdata(&dev->sd);
> +       struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
>  
>         return i2c_smbus_write_word_swapped(client, GC0310_AEC_PK_EXPO_H, exp);
>  }
>  
> -static int gc0310_gain_set(struct gc0310_device *dev, u32 gain)
> +static int gc0310_gain_set(struct gc0310_device *sensor, u32 gain)
>  {
> -       struct i2c_client *client = v4l2_get_subdevdata(&dev->sd);
> +       struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
>         u8 again, dgain;
>         int ret;
>  
> @@ -317,27 +317,27 @@ static int gc0310_gain_set(struct gc0310_device *dev, u32 gain)
>  
>  static int gc0310_s_ctrl(struct v4l2_ctrl *ctrl)
>  {
> -       struct gc0310_device *dev =
> +       struct gc0310_device *sensor =
>                 container_of(ctrl->handler, struct gc0310_device, ctrls.handler);
>         int ret;
>  
>         /* Only apply changes to the controls if the device is powered up */
> -       if (!pm_runtime_get_if_in_use(dev->sd.dev))
> +       if (!pm_runtime_get_if_in_use(sensor->sd.dev))
>                 return 0;
>  
>         switch (ctrl->id) {
>         case V4L2_CID_EXPOSURE:
> -               ret = gc0310_exposure_set(dev, ctrl->val);
> +               ret = gc0310_exposure_set(sensor, ctrl->val);
>                 break;
>         case V4L2_CID_GAIN:
> -               ret = gc0310_gain_set(dev, ctrl->val);
> +               ret = gc0310_gain_set(sensor, ctrl->val);
>                 break;
>         default:
>                 ret = -EINVAL;
>                 break;
>         }
>  
> -       pm_runtime_put(dev->sd.dev);
> +       pm_runtime_put(sensor->sd.dev);
>         return ret;
>  }
>  
> @@ -346,14 +346,14 @@ static const struct v4l2_ctrl_ops ctrl_ops = {
>  };
>  
>  static struct v4l2_mbus_framefmt *
> -gc0310_get_pad_format(struct gc0310_device *dev,
> +gc0310_get_pad_format(struct gc0310_device *sensor,
>                       struct v4l2_subdev_state *state,
>                       unsigned int pad, enum v4l2_subdev_format_whence which)
>  {
>         if (which == V4L2_SUBDEV_FORMAT_TRY)
>                 return v4l2_subdev_state_get_format(state, pad);
>  
> -       return &dev->mode.fmt;
> +       return &sensor->mode.fmt;
>  }
>  
>  /* The GC0310 currently only supports 1 fixed fmt */
> @@ -370,10 +370,10 @@ static int gc0310_set_fmt(struct v4l2_subdev *sd,
>                           struct v4l2_subdev_state *sd_state,
>                           struct v4l2_subdev_format *format)
>  {
> -       struct gc0310_device *dev = to_gc0310_sensor(sd);
> +       struct gc0310_device *sensor = to_gc0310_sensor(sd);
>         struct v4l2_mbus_framefmt *fmt;
>  
> -       fmt = gc0310_get_pad_format(dev, sd_state, format->pad, format->which);
> +       fmt = gc0310_get_pad_format(sensor, sd_state, format->pad, format->which);
>         gc0310_fill_format(fmt);
>  
>         format->format = *fmt;
> @@ -384,10 +384,10 @@ static int gc0310_get_fmt(struct v4l2_subdev *sd,
>                           struct v4l2_subdev_state *sd_state,
>                           struct v4l2_subdev_format *format)
>  {
> -       struct gc0310_device *dev = to_gc0310_sensor(sd);
> +       struct gc0310_device *sensor = to_gc0310_sensor(sd);
>         struct v4l2_mbus_framefmt *fmt;
>  
> -       fmt = gc0310_get_pad_format(dev, sd_state, format->pad, format->which);
> +       fmt = gc0310_get_pad_format(sensor, sd_state, format->pad, format->which);
>         format->format = *fmt;
>         return 0;
>  }
> @@ -424,12 +424,12 @@ static int gc0310_detect(struct i2c_client *client)
>  
>  static int gc0310_s_stream(struct v4l2_subdev *sd, int enable)
>  {
> -       struct gc0310_device *dev = to_gc0310_sensor(sd);
> +       struct gc0310_device *sensor = to_gc0310_sensor(sd);
>         struct i2c_client *client = v4l2_get_subdevdata(sd);
>         int ret = 0;
>  
>         dev_dbg(&client->dev, "%s S enable=%d\n", __func__, enable);
> -       mutex_lock(&dev->input_lock);
> +       mutex_lock(&sensor->input_lock);
>  
>         if (enable) {
>                 ret = pm_runtime_get_sync(&client->dev);
> @@ -449,7 +449,7 @@ static int gc0310_s_stream(struct v4l2_subdev *sd, int enable)
>                         goto error_power_down;
>  
>                 /* restore value of all ctrls */
> -               ret = __v4l2_ctrl_handler_setup(&dev->ctrls.handler);
> +               ret = __v4l2_ctrl_handler_setup(&sensor->ctrls.handler);
>                 if (ret)
>                         goto error_power_down;
>  
> @@ -475,14 +475,14 @@ static int gc0310_s_stream(struct v4l2_subdev *sd, int enable)
>         if (!enable)
>                 pm_runtime_put(&client->dev);
>  
> -       dev->is_streaming = enable;
> -       mutex_unlock(&dev->input_lock);
> +       sensor->is_streaming = enable;
> +       mutex_unlock(&sensor->input_lock);
>         return 0;
>  
>  error_power_down:
>         pm_runtime_put(&client->dev);
> -       dev->is_streaming = false;
> -       mutex_unlock(&dev->input_lock);
> +       sensor->is_streaming = false;
> +       mutex_unlock(&sensor->input_lock);
>         return ret;
>  }
>  
> @@ -559,21 +559,21 @@ static const struct v4l2_subdev_ops gc0310_ops = {
>         .sensor = &gc0310_sensor_ops,
>  };
>  
> -static int gc0310_init_controls(struct gc0310_device *dev)
> +static int gc0310_init_controls(struct gc0310_device *sensor)
>  {
> -       struct v4l2_ctrl_handler *hdl = &dev->ctrls.handler;
> +       struct v4l2_ctrl_handler *hdl = &sensor->ctrls.handler;
>  
>         v4l2_ctrl_handler_init(hdl, 2);
>  
>         /* Use the same lock for controls as for everything else */
> -       hdl->lock = &dev->input_lock;
> -       dev->sd.ctrl_handler = hdl;
> +       hdl->lock = &sensor->input_lock;
> +       sensor->sd.ctrl_handler = hdl;
>  
> -       dev->ctrls.exposure =
> +       sensor->ctrls.exposure =
>                 v4l2_ctrl_new_std(hdl, &ctrl_ops, V4L2_CID_EXPOSURE, 0, 4095, 1, 1023);
>  
>         /* 32 steps at base gain 1 + 64 half steps at base gain 2 */
> -       dev->ctrls.gain =
> +       sensor->ctrls.gain =
>                 v4l2_ctrl_new_std(hdl, &ctrl_ops, V4L2_CID_GAIN, 0, 95, 1, 31);
>  
>         return hdl->error;
> @@ -582,21 +582,21 @@ static int gc0310_init_controls(struct gc0310_device *dev)
>  static void gc0310_remove(struct i2c_client *client)
>  {
>         struct v4l2_subdev *sd = i2c_get_clientdata(client);
> -       struct gc0310_device *dev = to_gc0310_sensor(sd);
> +       struct gc0310_device *sensor = to_gc0310_sensor(sd);
>  
>         dev_dbg(&client->dev, "gc0310_remove...\n");
>  
>         v4l2_async_unregister_subdev(sd);
> -       media_entity_cleanup(&dev->sd.entity);
> -       v4l2_ctrl_handler_free(&dev->ctrls.handler);
> -       mutex_destroy(&dev->input_lock);
> +       media_entity_cleanup(&sensor->sd.entity);
> +       v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> +       mutex_destroy(&sensor->input_lock);
>         pm_runtime_disable(&client->dev);
>  }
>  
>  static int gc0310_probe(struct i2c_client *client)
>  {
>         struct fwnode_handle *ep_fwnode;
> -       struct gc0310_device *dev;
> +       struct gc0310_device *sensor;
>         int ret;
>  
>         /*
> @@ -609,25 +609,25 @@ static int gc0310_probe(struct i2c_client *client)
>  
>         fwnode_handle_put(ep_fwnode);
>  
> -       dev = devm_kzalloc(&client->dev, sizeof(*dev), GFP_KERNEL);
> -       if (!dev)
> +       sensor = devm_kzalloc(&client->dev, sizeof(*sensor), GFP_KERNEL);
> +       if (!sensor)
>                 return -ENOMEM;
>  
> -       dev->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
> -       if (IS_ERR(dev->reset)) {
> -               return dev_err_probe(&client->dev, PTR_ERR(dev->reset),
> +       sensor->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
> +       if (IS_ERR(sensor->reset)) {
> +               return dev_err_probe(&client->dev, PTR_ERR(sensor->reset),
>                                      "getting reset GPIO\n");
>         }
>  
> -       dev->powerdown = devm_gpiod_get(&client->dev, "powerdown", GPIOD_OUT_HIGH);
> -       if (IS_ERR(dev->powerdown)) {
> -               return dev_err_probe(&client->dev, PTR_ERR(dev->powerdown),
> +       sensor->powerdown = devm_gpiod_get(&client->dev, "powerdown", GPIOD_OUT_HIGH);
> +       if (IS_ERR(sensor->powerdown)) {
> +               return dev_err_probe(&client->dev, PTR_ERR(sensor->powerdown),
>                                      "getting powerdown GPIO\n");
>         }
>  
> -       mutex_init(&dev->input_lock);
> -       v4l2_i2c_subdev_init(&dev->sd, client, &gc0310_ops);
> -       gc0310_fill_format(&dev->mode.fmt);
> +       mutex_init(&sensor->input_lock);
> +       v4l2_i2c_subdev_init(&sensor->sd, client, &gc0310_ops);
> +       gc0310_fill_format(&sensor->mode.fmt);
>  
>         pm_runtime_set_suspended(&client->dev);
>         pm_runtime_enable(&client->dev);
> @@ -640,23 +640,23 @@ static int gc0310_probe(struct i2c_client *client)
>                 return ret;
>         }
>  
> -       dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> -       dev->pad.flags = MEDIA_PAD_FL_SOURCE;
> -       dev->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> +       sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +       sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
> +       sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>  
> -       ret = gc0310_init_controls(dev);
> +       ret = gc0310_init_controls(sensor);
>         if (ret) {
>                 gc0310_remove(client);
>                 return ret;
>         }
>  
> -       ret = media_entity_pads_init(&dev->sd.entity, 1, &dev->pad);
> +       ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
>         if (ret) {
>                 gc0310_remove(client);
>                 return ret;
>         }
>  
> -       ret = v4l2_async_register_subdev_sensor(&dev->sd);
> +       ret = v4l2_async_register_subdev_sensor(&sensor->sd);
>         if (ret) {
>                 gc0310_remove(client);
>                 return ret;
> @@ -668,22 +668,22 @@ static int gc0310_probe(struct i2c_client *client)
>  static int gc0310_suspend(struct device *dev)
>  {
>         struct v4l2_subdev *sd = dev_get_drvdata(dev);
> -       struct gc0310_device *gc0310_dev = to_gc0310_sensor(sd);
> +       struct gc0310_device *sensor = to_gc0310_sensor(sd);
>  
> -       gpiod_set_value_cansleep(gc0310_dev->powerdown, 1);
> -       gpiod_set_value_cansleep(gc0310_dev->reset, 1);
> +       gpiod_set_value_cansleep(sensor->powerdown, 1);
> +       gpiod_set_value_cansleep(sensor->reset, 1);
>         return 0;
>  }
>  
>  static int gc0310_resume(struct device *dev)
>  {
>         struct v4l2_subdev *sd = dev_get_drvdata(dev);
> -       struct gc0310_device *gc0310_dev = to_gc0310_sensor(sd);
> +       struct gc0310_device *sensor = to_gc0310_sensor(sd);
>  
>         usleep_range(10000, 15000);
> -       gpiod_set_value_cansleep(gc0310_dev->reset, 0);
> +       gpiod_set_value_cansleep(sensor->reset, 0);
>         usleep_range(10000, 15000);
> -       gpiod_set_value_cansleep(gc0310_dev->powerdown, 0);
> +       gpiod_set_value_cansleep(sensor->powerdown, 0);
>  
>         return 0;
>  }
> -- 
> 2.49.0
>

[PATCH 02/23] media: atomisp: gc0310: Drop unused GC0310_FOCAL_LENGTH_NUM define

[Hans de Goede]

Drop the unused GC0310_FOCAL_LENGTH_NUM define, the focal-length
is a property of the sensor-module, not of the raw sensor itself.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index cc74e90a1457..f253b4b494f2 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -25,8 +25,6 @@
 #define GC0310_FPS				30
 #define GC0310_SKIP_FRAMES			3
 
-#define GC0310_FOCAL_LENGTH_NUM			278 /* 2.78mm */
-
 #define GC0310_ID				0xa310
 
 #define GC0310_RESET_RELATED			0xFE
-- 
2.49.0

Re: [PATCH 02/23] media: atomisp: gc0310: Drop unused GC0310_FOCAL_LENGTH_NUM define

[Kieran Bingham]

Quoting Hans de Goede (2025-05-17 12:40:45)
> Drop the unused GC0310_FOCAL_LENGTH_NUM define, the focal-length
> is a property of the sensor-module, not of the raw sensor itself.
> 

That's going to be an interesting one that we should identify for camera
tuning files in libcamera sometime too ...

But I agree it's not suitable here in the driver. Especially if it's
unused...


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

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> index cc74e90a1457..f253b4b494f2 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> @@ -25,8 +25,6 @@
>  #define GC0310_FPS                             30
>  #define GC0310_SKIP_FRAMES                     3
>  
> -#define GC0310_FOCAL_LENGTH_NUM                        278 /* 2.78mm */
> -
>  #define GC0310_ID                              0xa310
>  
>  #define GC0310_RESET_RELATED                   0xFE
> -- 
> 2.49.0
>

[PATCH 03/23] media: atomisp: gc0310: Modify vblank value to run at 30 fps

[Hans de Goede]

Currently the sensor is running 30.9 fps, increase vblank
to have it actually run at 30.0 fps.

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

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index f253b4b494f2..6b11f0ff6088 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -141,7 +141,7 @@ static const struct gc0310_reg gc0310_reset_register[] = {
 	{ 0x05, 0x00 },
 	{ 0x06, 0xb2 }, /* 0x0a //HB */
 	{ 0x07, 0x00 },
-	{ 0x08, 0x0c }, /* 0x89 //VB */
+	{ 0x08, 0x1b }, /* 0x89 //VB */
 	{ 0x09, 0x00 }, /* row start */
 	{ 0x0a, 0x00 },
 	{ 0x0b, 0x00 }, /* col start */
-- 
2.49.0

Re: [PATCH 03/23] media: atomisp: gc0310: Modify vblank value to run at 30 fps

[Kieran Bingham]

Quoting Hans de Goede (2025-05-17 12:40:46)
> Currently the sensor is running 30.9 fps, increase vblank
> to have it actually run at 30.0 fps.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> index f253b4b494f2..6b11f0ff6088 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> @@ -141,7 +141,7 @@ static const struct gc0310_reg gc0310_reset_register[] = {
>         { 0x05, 0x00 },
>         { 0x06, 0xb2 }, /* 0x0a //HB */
>         { 0x07, 0x00 },
> -       { 0x08, 0x0c }, /* 0x89 //VB */
> +       { 0x08, 0x1b }, /* 0x89 //VB */

I would wonder why we have 0x0a and 0x89 in the comments here ... but I
see that this later gets reworked to directly support setting
hblank/vblank controls so I'm not going to dwell on the specifics here.

I've no idea whether an RB or an Acked tag or something else is
appropriate here ;-) But I think this patch should be kept ;-)


>         { 0x09, 0x00 }, /* row start */
>         { 0x0a, 0x00 },
>         { 0x0b, 0x00 }, /* col start */
> -- 
> 2.49.0
>

[PATCH 04/23] media: atomisp: gc0310: Switch to CCI register access helpers

[Hans de Goede]

Switch the GC0310 driver over to the CCI register access helpers.

While at it also add a _REG prefix to all register address defines
to make clear they are register addresses and group register value
defines together with the address definition.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/i2c/Kconfig     |   1 +
 .../media/atomisp/i2c/atomisp-gc0310.c        | 143 +++++++-----------
 2 files changed, 55 insertions(+), 89 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/Kconfig b/drivers/staging/media/atomisp/i2c/Kconfig
index 4f46182da58b..ef2094c22347 100644
--- a/drivers/staging/media/atomisp/i2c/Kconfig
+++ b/drivers/staging/media/atomisp/i2c/Kconfig
@@ -31,6 +31,7 @@ config VIDEO_ATOMISP_GC0310
 	tristate "GC0310 sensor support"
 	depends on ACPI
 	depends on I2C && VIDEO_DEV
+	select V4L2_CCI_I2C
 	help
 	  This is a Video4Linux2 sensor-level driver for the Galaxycore
 	  GC0310 0.3MP sensor.
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index 6b11f0ff6088..ee039f3be4da 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -13,9 +13,11 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
+#include <linux/regmap.h>
 #include <linux/string.h>
 #include <linux/types.h>
 
+#include <media/v4l2-cci.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 
@@ -27,41 +29,32 @@
 
 #define GC0310_ID				0xa310
 
-#define GC0310_RESET_RELATED			0xFE
+#define GC0310_RESET_RELATED_REG		CCI_REG8(0xfe)
 #define GC0310_REGISTER_PAGE_0			0x0
 #define GC0310_REGISTER_PAGE_3			0x3
 
 /*
  * GC0310 System control registers
  */
-#define GC0310_SW_STREAM			0x10
-
-#define GC0310_SC_CMMN_CHIP_ID_H		0xf0
-#define GC0310_SC_CMMN_CHIP_ID_L		0xf1
-
-#define GC0310_AEC_PK_EXPO_H			0x03
-#define GC0310_AEC_PK_EXPO_L			0x04
-#define GC0310_AGC_ADJ				0x48
-#define GC0310_DGC_ADJ				0x71
-#define GC0310_GROUP_ACCESS			0x3208
-
-#define GC0310_H_CROP_START_H			0x09
-#define GC0310_H_CROP_START_L			0x0A
-#define GC0310_V_CROP_START_H			0x0B
-#define GC0310_V_CROP_START_L			0x0C
-#define GC0310_H_OUTSIZE_H			0x0F
-#define GC0310_H_OUTSIZE_L			0x10
-#define GC0310_V_OUTSIZE_H			0x0D
-#define GC0310_V_OUTSIZE_L			0x0E
-#define GC0310_H_BLANKING_H			0x05
-#define GC0310_H_BLANKING_L			0x06
-#define GC0310_V_BLANKING_H			0x07
-#define GC0310_V_BLANKING_L			0x08
-#define GC0310_SH_DELAY				0x11
+#define GC0310_SW_STREAM_REG			CCI_REG8(0x10)
 
 #define GC0310_START_STREAMING			0x94 /* 8-bit enable */
 #define GC0310_STOP_STREAMING			0x0 /* 8-bit disable */
 
+#define GC0310_SC_CMMN_CHIP_ID_REG		CCI_REG16(0xf0)
+
+#define GC0310_AEC_PK_EXPO_REG			CCI_REG16(0x03)
+#define GC0310_AGC_ADJ_REG			CCI_REG8(0x48)
+#define GC0310_DGC_ADJ_REG			CCI_REG8(0x71)
+
+#define GC0310_H_CROP_START_REG			CCI_REG16(0x09)
+#define GC0310_V_CROP_START_REG			CCI_REG16(0x0B)
+#define GC0310_H_OUTSIZE_REG			CCI_REG16(0x0F)
+#define GC0310_V_OUTSIZE_REG			CCI_REG16(0x0D)
+#define GC0310_H_BLANKING_REG			CCI_REG16(0x05)
+#define GC0310_V_BLANKING_REG			CCI_REG16(0x07)
+#define GC0310_SH_DELAY_REG			CCI_REG8(0x11)
+
 #define to_gc0310_sensor(x) container_of(x, struct gc0310_device, sd)
 
 struct gc0310_device {
@@ -71,6 +64,7 @@ struct gc0310_device {
 	struct mutex input_lock;
 	bool is_streaming;
 
+	struct regmap *regmap;
 	struct gpio_desc *reset;
 	struct gpio_desc *powerdown;
 
@@ -90,7 +84,7 @@ struct gc0310_reg {
 	u8 val;
 };
 
-static const struct gc0310_reg gc0310_reset_register[] = {
+static const struct reg_sequence gc0310_reset_register[] = {
 	/* System registers */
 	{ 0xfe, 0xf0 },
 	{ 0xfe, 0xf0 },
@@ -233,7 +227,7 @@ static const struct gc0310_reg gc0310_reset_register[] = {
 	{ 0xfe, 0x00 },
 };
 
-static const struct gc0310_reg gc0310_VGA_30fps[] = {
+static const struct reg_sequence gc0310_VGA_30fps[] = {
 	{ 0xfe, 0x00 },
 	{ 0x0d, 0x01 }, /* height */
 	{ 0x0e, 0xf2 }, /* 0xf7 //height */
@@ -257,41 +251,10 @@ static const struct gc0310_reg gc0310_VGA_30fps[] = {
 	{ 0xfe, 0x00 },
 };
 
-/*
- * gc0310_write_reg_array - Initializes a list of GC0310 registers
- * @client: i2c driver client structure
- * @reglist: list of registers to be written
- * @count: number of register, value pairs in the list
- */
-static int gc0310_write_reg_array(struct i2c_client *client,
-				  const struct gc0310_reg *reglist, int count)
-{
-	int i, err;
-
-	for (i = 0; i < count; i++) {
-		err = i2c_smbus_write_byte_data(client, reglist[i].reg, reglist[i].val);
-		if (err) {
-			dev_err(&client->dev, "write error: wrote 0x%x to offset 0x%x error %d",
-				reglist[i].val, reglist[i].reg, err);
-			return err;
-		}
-	}
-
-	return 0;
-}
-
-static int gc0310_exposure_set(struct gc0310_device *sensor, u32 exp)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
-
-	return i2c_smbus_write_word_swapped(client, GC0310_AEC_PK_EXPO_H, exp);
-}
-
 static int gc0310_gain_set(struct gc0310_device *sensor, u32 gain)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
 	u8 again, dgain;
-	int ret;
+	int ret = 0;
 
 	/* Taken from original driver, this never sets dgain lower then 32? */
 
@@ -306,11 +269,9 @@ static int gc0310_gain_set(struct gc0310_device *sensor, u32 gain)
 		dgain = gain / 2;
 	}
 
-	ret = i2c_smbus_write_byte_data(client, GC0310_AGC_ADJ, again);
-	if (ret)
-		return ret;
-
-	return i2c_smbus_write_byte_data(client, GC0310_DGC_ADJ, dgain);
+	cci_write(sensor->regmap, GC0310_AGC_ADJ_REG, again, &ret);
+	cci_write(sensor->regmap, GC0310_DGC_ADJ_REG, dgain, &ret);
+	return ret;
 }
 
 static int gc0310_s_ctrl(struct v4l2_ctrl *ctrl)
@@ -325,7 +286,8 @@ static int gc0310_s_ctrl(struct v4l2_ctrl *ctrl)
 
 	switch (ctrl->id) {
 	case V4L2_CID_EXPOSURE:
-		ret = gc0310_exposure_set(sensor, ctrl->val);
+		ret = cci_write(sensor->regmap, GC0310_AEC_PK_EXPO_REG,
+				ctrl->val, NULL);
 		break;
 	case V4L2_CID_GAIN:
 		ret = gc0310_gain_set(sensor, ctrl->val);
@@ -390,28 +352,30 @@ static int gc0310_get_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static int gc0310_detect(struct i2c_client *client)
+static int gc0310_detect(struct gc0310_device *sensor)
 {
-	struct i2c_adapter *adapter = client->adapter;
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
+	u64 val;
 	int ret;
 
-	if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
 		return -ENODEV;
 
 	ret = pm_runtime_get_sync(&client->dev);
 	if (ret >= 0)
-		ret = i2c_smbus_read_word_swapped(client, GC0310_SC_CMMN_CHIP_ID_H);
+		ret = cci_read(sensor->regmap, GC0310_SC_CMMN_CHIP_ID_REG,
+			       &val, NULL);
 	pm_runtime_put(&client->dev);
 	if (ret < 0) {
 		dev_err(&client->dev, "read sensor_id failed: %d\n", ret);
 		return -ENODEV;
 	}
 
-	dev_dbg(&client->dev, "sensor ID = 0x%x\n", ret);
+	dev_dbg(&client->dev, "sensor ID = 0x%llx\n", val);
 
-	if (ret != GC0310_ID) {
-		dev_err(&client->dev, "sensor ID error, read id = 0x%x, target id = 0x%x\n",
-			ret, GC0310_ID);
+	if (val != GC0310_ID) {
+		dev_err(&client->dev, "sensor ID error, read id = 0x%llx, target id = 0x%x\n",
+			val, GC0310_ID);
 		return -ENODEV;
 	}
 
@@ -436,12 +400,14 @@ static int gc0310_s_stream(struct v4l2_subdev *sd, int enable)
 
 		msleep(100);
 
-		ret = gc0310_write_reg_array(client, gc0310_reset_register,
+		ret = regmap_multi_reg_write(sensor->regmap,
+					     gc0310_reset_register,
 					     ARRAY_SIZE(gc0310_reset_register));
 		if (ret)
 			goto error_power_down;
 
-		ret = gc0310_write_reg_array(client, gc0310_VGA_30fps,
+		ret = regmap_multi_reg_write(sensor->regmap,
+					     gc0310_VGA_30fps,
 					     ARRAY_SIZE(gc0310_VGA_30fps));
 		if (ret)
 			goto error_power_down;
@@ -452,21 +418,16 @@ static int gc0310_s_stream(struct v4l2_subdev *sd, int enable)
 			goto error_power_down;
 
 		/* enable per frame MIPI and sensor ctrl reset  */
-		ret = i2c_smbus_write_byte_data(client, 0xFE, 0x30);
-		if (ret)
-			goto error_power_down;
+		cci_write(sensor->regmap, GC0310_RESET_RELATED_REG, 0x30, &ret);
 	}
 
-	ret = i2c_smbus_write_byte_data(client, GC0310_RESET_RELATED, GC0310_REGISTER_PAGE_3);
-	if (ret)
-		goto error_power_down;
-
-	ret = i2c_smbus_write_byte_data(client, GC0310_SW_STREAM,
-					enable ? GC0310_START_STREAMING : GC0310_STOP_STREAMING);
-	if (ret)
-		goto error_power_down;
-
-	ret = i2c_smbus_write_byte_data(client, GC0310_RESET_RELATED, GC0310_REGISTER_PAGE_0);
+	cci_write(sensor->regmap, GC0310_RESET_RELATED_REG,
+		  GC0310_REGISTER_PAGE_3, &ret);
+	cci_write(sensor->regmap, GC0310_SW_STREAM_REG,
+		  enable ? GC0310_START_STREAMING : GC0310_STOP_STREAMING,
+		  &ret);
+	cci_write(sensor->regmap, GC0310_RESET_RELATED_REG,
+		  GC0310_REGISTER_PAGE_0, &ret);
 	if (ret)
 		goto error_power_down;
 
@@ -627,12 +588,16 @@ static int gc0310_probe(struct i2c_client *client)
 	v4l2_i2c_subdev_init(&sensor->sd, client, &gc0310_ops);
 	gc0310_fill_format(&sensor->mode.fmt);
 
+	sensor->regmap = devm_cci_regmap_init_i2c(client, 8);
+	if (IS_ERR(sensor->regmap))
+		return PTR_ERR(sensor->regmap);
+
 	pm_runtime_set_suspended(&client->dev);
 	pm_runtime_enable(&client->dev);
 	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
 	pm_runtime_use_autosuspend(&client->dev);
 
-	ret = gc0310_detect(client);
+	ret = gc0310_detect(sensor);
 	if (ret) {
 		gc0310_remove(client);
 		return ret;
-- 
2.49.0

Re: [PATCH 04/23] media: atomisp: gc0310: Switch to CCI register access helpers

[Andy Shevchenko]

On Sat, May 17, 2025 at 01:40:47PM +0200, Hans de Goede wrote:
> Switch the GC0310 driver over to the CCI register access helpers.
> 
> While at it also add a _REG prefix to all register address defines
> to make clear they are register addresses and group register value
> defines together with the address definition.

...

> -#define GC0310_RESET_RELATED			0xFE
> +#define GC0310_RESET_RELATED_REG		CCI_REG8(0xfe)

Old style: CAPITAL
New style: small

...

> +#define GC0310_AGC_ADJ_REG			CCI_REG8(0x48)


> +#define GC0310_V_CROP_START_REG			CCI_REG16(0x0B)
> +#define GC0310_H_OUTSIZE_REG			CCI_REG16(0x0F)
> +#define GC0310_V_OUTSIZE_REG			CCI_REG16(0x0D)

But can we be consistent with the style all over?

...

> -	dev_dbg(&client->dev, "sensor ID = 0x%x\n", ret);
> +	dev_dbg(&client->dev, "sensor ID = 0x%llx\n", val);

Perhaps %#llx?

-- 
With Best Regards,
Andy Shevchenko

[PATCH 05/23] media: atomisp: gc0310: Use V4L2_CID_ANALOGUE_GAIN for gain control

[Hans de Goede]

Use V4L2_CID_ANALOGUE_GAIN for gain control, as expected by userspace.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index ee039f3be4da..756e56f639b7 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -289,7 +289,7 @@ static int gc0310_s_ctrl(struct v4l2_ctrl *ctrl)
 		ret = cci_write(sensor->regmap, GC0310_AEC_PK_EXPO_REG,
 				ctrl->val, NULL);
 		break;
-	case V4L2_CID_GAIN:
+	case V4L2_CID_ANALOGUE_GAIN:
 		ret = gc0310_gain_set(sensor, ctrl->val);
 		break;
 	default:
@@ -533,7 +533,7 @@ static int gc0310_init_controls(struct gc0310_device *sensor)
 
 	/* 32 steps at base gain 1 + 64 half steps at base gain 2 */
 	sensor->ctrls.gain =
-		v4l2_ctrl_new_std(hdl, &ctrl_ops, V4L2_CID_GAIN, 0, 95, 1, 31);
+		v4l2_ctrl_new_std(hdl, &ctrl_ops, V4L2_CID_ANALOGUE_GAIN, 0, 95, 1, 31);
 
 	return hdl->error;
 }
-- 
2.49.0

Re: [PATCH 05/23] media: atomisp: gc0310: Use V4L2_CID_ANALOGUE_GAIN for gain control

[Kieran Bingham]

Quoting Hans de Goede (2025-05-17 12:40:48)
> Use V4L2_CID_ANALOGUE_GAIN for gain control, as expected by userspace.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> index ee039f3be4da..756e56f639b7 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> @@ -289,7 +289,7 @@ static int gc0310_s_ctrl(struct v4l2_ctrl *ctrl)
>                 ret = cci_write(sensor->regmap, GC0310_AEC_PK_EXPO_REG,
>                                 ctrl->val, NULL);
>                 break;
> -       case V4L2_CID_GAIN:
> +       case V4L2_CID_ANALOGUE_GAIN:
>                 ret = gc0310_gain_set(sensor, ctrl->val);
>                 break;
>         default:
> @@ -533,7 +533,7 @@ static int gc0310_init_controls(struct gc0310_device *sensor)
>  
>         /* 32 steps at base gain 1 + 64 half steps at base gain 2 */

sounds like a curious gain model...

Will be interesting when we get the sensor calibration tools up and
running to plot this. (Or is there already a public datasheet
documenting this?)

Is there a split here between analogue gain and digital gain ? Or is it
all expected to be 'analogue gain' ?


>         sensor->ctrls.gain =
> -               v4l2_ctrl_new_std(hdl, &ctrl_ops, V4L2_CID_GAIN, 0, 95, 1, 31);
> +               v4l2_ctrl_new_std(hdl, &ctrl_ops, V4L2_CID_ANALOGUE_GAIN, 0, 95, 1, 31);
>  
>         return hdl->error;
>  }
> -- 
> 2.49.0
>

Re: [PATCH 05/23] media: atomisp: gc0310: Use V4L2_CID_ANALOGUE_GAIN for gain control

[Kieran Bingham]

Hi Hans,

+Cc: libcamera-devel

Digging in here I found this part interesting (i.e. perhaps we need to
clarify the expected behavours better)



Quoting Kieran Bingham (2025-05-17 22:09:13)
> Quoting Hans de Goede (2025-05-17 12:40:48)
> > Use V4L2_CID_ANALOGUE_GAIN for gain control, as expected by userspace.
> > 
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> > index ee039f3be4da..756e56f639b7 100644
> > --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> > +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> > @@ -289,7 +289,7 @@ static int gc0310_s_ctrl(struct v4l2_ctrl *ctrl)
> >                 ret = cci_write(sensor->regmap, GC0310_AEC_PK_EXPO_REG,
> >                                 ctrl->val, NULL);
> >                 break;
> > -       case V4L2_CID_GAIN:
> > +       case V4L2_CID_ANALOGUE_GAIN:
> >                 ret = gc0310_gain_set(sensor, ctrl->val);
> >                 break;
> >         default:
> > @@ -533,7 +533,7 @@ static int gc0310_init_controls(struct gc0310_device *sensor)
> >  
> >         /* 32 steps at base gain 1 + 64 half steps at base gain 2 */
> 
> sounds like a curious gain model...
> 
> Will be interesting when we get the sensor calibration tools up and
> running to plot this. (Or is there already a public datasheet
> documenting this?)
> 
> Is there a split here between analogue gain and digital gain ? Or is it
> all expected to be 'analogue gain' ?

I looked deeper, and this does seem to be a split between analogue and
digital gain. It also seems like this control might be doing additional
calculations which would then have to be accounted for as part of the
gain model in libcamera, so then instead of 'sensor specific' it would
be 'this linux sensor driver specific' - so maybe the gain functions
should be simplified more.

Adding in libcamera-devel - because I think we need to figure out what's
best for handling this (overall for all sensors with A+D gain)

There are some sensors I've seen where the digital gain can only be
applied 'on top' of the analogue gain, and so it does act like a single
control ...

But we probably want to be able to distinguish between analogue gain and
digital gain in libcamera / userspace.

However, even if we distinguish ... I suspect there are cases where if
we need more gain than just the analogue gain can provide - adding the
large steps at the sensor - and then only applying very small amounts of
fine-grain digital gain on an ISP would make things simpler or easier
overall.

So somehow I think we need to figure out and correctly document and
manage the splits between analogue and digital gains, and that will
likely have to have a corresponding mapping in either the camera sensor
helpers or the tuning files in some part.

--
Kieran


> 
> 
> >         sensor->ctrls.gain =
> > -               v4l2_ctrl_new_std(hdl, &ctrl_ops, V4L2_CID_GAIN, 0, 95, 1, 31);
> > +               v4l2_ctrl_new_std(hdl, &ctrl_ops, V4L2_CID_ANALOGUE_GAIN, 0, 95, 1, 31);
> >  
> >         return hdl->error;
> >  }
> > -- 
> > 2.49.0
> >

Re: [PATCH 05/23] media: atomisp: gc0310: Use V4L2_CID_ANALOGUE_GAIN for gain control

[Laurent Pinchart]

On Sun, May 18, 2025 at 10:42:56AM +0100, Kieran Bingham wrote:
> Hi Hans,
> 
> +Cc: libcamera-devel
> 
> Digging in here I found this part interesting (i.e. perhaps we need to
> clarify the expected behavours better)
> 
> Quoting Kieran Bingham (2025-05-17 22:09:13)
> > Quoting Hans de Goede (2025-05-17 12:40:48)
> > > Use V4L2_CID_ANALOGUE_GAIN for gain control, as expected by userspace.
> > > 
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > >  drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> > > index ee039f3be4da..756e56f639b7 100644
> > > --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> > > +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> > > @@ -289,7 +289,7 @@ static int gc0310_s_ctrl(struct v4l2_ctrl *ctrl)
> > >                 ret = cci_write(sensor->regmap, GC0310_AEC_PK_EXPO_REG,
> > >                                 ctrl->val, NULL);
> > >                 break;
> > > -       case V4L2_CID_GAIN:
> > > +       case V4L2_CID_ANALOGUE_GAIN:
> > >                 ret = gc0310_gain_set(sensor, ctrl->val);
> > >                 break;
> > >         default:
> > > @@ -533,7 +533,7 @@ static int gc0310_init_controls(struct gc0310_device *sensor)
> > >  
> > >         /* 32 steps at base gain 1 + 64 half steps at base gain 2 */
> > 
> > sounds like a curious gain model...
> > 
> > Will be interesting when we get the sensor calibration tools up and
> > running to plot this. (Or is there already a public datasheet
> > documenting this?)
> > 
> > Is there a split here between analogue gain and digital gain ? Or is it
> > all expected to be 'analogue gain' ?
> 
> I looked deeper, and this does seem to be a split between analogue and
> digital gain. It also seems like this control might be doing additional
> calculations which would then have to be accounted for as part of the
> gain model in libcamera, so then instead of 'sensor specific' it would
> be 'this linux sensor driver specific' - so maybe the gain functions
> should be simplified more.
> 
> Adding in libcamera-devel - because I think we need to figure out what's
> best for handling this (overall for all sensors with A+D gain)
> 
> There are some sensors I've seen where the digital gain can only be
> applied 'on top' of the analogue gain, and so it does act like a single
> control ...

I've seen other sensors (in particular the AR0830) that also combine
analogue and digital gains in a single register. The analogue gain is
typically quite coarse in that case, and the digital gain is used for
fine adjustments. In the ar0830 driver I'm writing, I've split the two
components in two separate controls.

> But we probably want to be able to distinguish between analogue gain and
> digital gain in libcamera / userspace.
> 
> However, even if we distinguish ... I suspect there are cases where if
> we need more gain than just the analogue gain can provide - adding the
> large steps at the sensor - and then only applying very small amounts of
> fine-grain digital gain on an ISP would make things simpler or easier
> overall.
> 
> So somehow I think we need to figure out and correctly document and
> manage the splits between analogue and digital gains, and that will
> likely have to have a corresponding mapping in either the camera sensor
> helpers or the tuning files in some part.
> 
> > >         sensor->ctrls.gain =
> > > -               v4l2_ctrl_new_std(hdl, &ctrl_ops, V4L2_CID_GAIN, 0, 95, 1, 31);
> > > +               v4l2_ctrl_new_std(hdl, &ctrl_ops, V4L2_CID_ANALOGUE_GAIN, 0, 95, 1, 31);
> > >  
> > >         return hdl->error;
> > >  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 05/23] media: atomisp: gc0310: Use V4L2_CID_ANALOGUE_GAIN for gain control

[Hans de Goede]

Hi Kieran,

Thank you for the reviews.

On 17-May-25 23:09, Kieran Bingham wrote:
> Quoting Hans de Goede (2025-05-17 12:40:48)
>> Use V4L2_CID_ANALOGUE_GAIN for gain control, as expected by userspace.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
>> index ee039f3be4da..756e56f639b7 100644
>> --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
>> +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
>> @@ -289,7 +289,7 @@ static int gc0310_s_ctrl(struct v4l2_ctrl *ctrl)
>>                 ret = cci_write(sensor->regmap, GC0310_AEC_PK_EXPO_REG,
>>                                 ctrl->val, NULL);
>>                 break;
>> -       case V4L2_CID_GAIN:
>> +       case V4L2_CID_ANALOGUE_GAIN:
>>                 ret = gc0310_gain_set(sensor, ctrl->val);
>>                 break;
>>         default:
>> @@ -533,7 +533,7 @@ static int gc0310_init_controls(struct gc0310_device *sensor)
>>  
>>         /* 32 steps at base gain 1 + 64 half steps at base gain 2 */
> 
> sounds like a curious gain model...
> 
> Will be interesting when we get the sensor calibration tools up and
> running to plot this. (Or is there already a public datasheet
> documenting this?)

There is a datasheet but it does not document much other then
register names.
 
> Is there a split here between analogue gain and digital gain ? Or is it
> all expected to be 'analogue gain' ?

here is the actual method setting the gain:

        /* Taken from original driver, this never sets dgain lower then 32? */

        /* Change 0 - 95 to 32 - 127 */
        gain += 32;

        if (gain < 64) {
                again = 0x0; /* sqrt(2) */
                dgain = gain;
        } else {
                again = 0x2; /* 2 * sqrt(2) */
                dgain = gain / 2;
        }

        cci_write(sensor->regmap, GC0310_AGC_ADJ_REG, again, &ret);
        cci_write(sensor->regmap, GC0310_DGC_ADJ_REG, dgain, &ret);

The 32 half steps come from the dgain = gain / 2 for steps 32 - 95 .

Note the again / dgain names here are confusing. The data sheet describes
the 2 registers which are being written as follows:

AGC:
P0:0x48 ANALOG_COL_gain-col_code 4 bit wide default 0x00 RO [7:4] ANALOG_COL_gain [2:0] col_code

DGC:
P0:0x71 Auto_pregain 8 bit wide default 0x20 RO Auto_pregain

Note DGC is described as read-only in the datasheet, maybe the datesheet is off by one
and we should actually look at register 0x70 in the datasheet:

P0:0x70 Global_gain 8 bit wide default 0x40 RW Global_gain

Either way based on the function calculations I get the feeling that
the first register is actually setting some fixed analog pre-multiplier
and the second register is the actual analog gain.

Despite the names used in the driver (inherited from Android kernels) it
does not feel to me like one of the 2 registers is a digital gain
register.

So I'm going to keep this as is for now and once we've gain calibration
tooling up and running make a plot with the current gain code and then
modify the driver to make this fit one of the standard gain models.

Regards,

Hans

[PATCH 06/23] media: atomisp: gc0310: Add selection API support

[Hans de Goede]

Add support for the selection API as expected by libcamera.

Note the driver only supports a single fixed resolution and
no cropping, so this is a simple read-only implementation.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/i2c/atomisp-gc0310.c        | 42 ++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index 756e56f639b7..7902e732a3ca 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -3,7 +3,7 @@
  * Support for GalaxyCore GC0310 VGA camera sensor.
  *
  * Copyright (c) 2013 Intel Corporation. All Rights Reserved.
- * Copyright (c) 2023 Hans de Goede <hdegoede@redhat.com>
+ * Copyright (c) 2023-2025 Hans de Goede <hansg@kernel.org>
  */
 
 #include <linux/delay.h>
@@ -352,6 +352,43 @@ static int gc0310_get_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int gc0310_get_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *state,
+				struct v4l2_subdev_selection *sel)
+{
+	/* Only the single fixed 656x496 mode is supported, without croping */
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP:
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+	case V4L2_SEL_TGT_NATIVE_SIZE:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = GC0310_NATIVE_WIDTH;
+		sel->r.height = GC0310_NATIVE_HEIGHT;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int gc0310_set_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_state *state,
+				struct v4l2_subdev_selection *sel)
+{
+	if (sel->target != V4L2_SEL_TGT_CROP)
+		return -EINVAL;
+
+	/* Only the single fixed 656x496 mode is supported, without croping */
+	sel->r.top = 0;
+	sel->r.left = 0;
+	sel->r.width = GC0310_NATIVE_WIDTH;
+	sel->r.height = GC0310_NATIVE_HEIGHT;
+	return 0;
+}
+
 static int gc0310_detect(struct gc0310_device *sensor)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
@@ -509,6 +546,8 @@ static const struct v4l2_subdev_pad_ops gc0310_pad_ops = {
 	.enum_frame_size = gc0310_enum_frame_size,
 	.get_fmt = gc0310_get_fmt,
 	.set_fmt = gc0310_set_fmt,
+	.get_selection = gc0310_get_selection,
+	.set_selection = gc0310_set_selection,
 	.get_frame_interval = gc0310_get_frame_interval,
 };
 
@@ -671,5 +710,6 @@ static struct i2c_driver gc0310_driver = {
 module_i2c_driver(gc0310_driver);
 
 MODULE_AUTHOR("Lai, Angie <angie.lai@intel.com>");
+MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
 MODULE_DESCRIPTION("A low-level driver for GalaxyCore GC0310 sensors");
 MODULE_LICENSE("GPL");
-- 
2.49.0

Re: [PATCH 06/23] media: atomisp: gc0310: Add selection API support

[Kieran Bingham]

Quoting Hans de Goede (2025-05-17 12:40:49)
> Add support for the selection API as expected by libcamera.
> 
> Note the driver only supports a single fixed resolution and
> no cropping, so this is a simple read-only implementation.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  .../media/atomisp/i2c/atomisp-gc0310.c        | 42 ++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> index 756e56f639b7..7902e732a3ca 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> @@ -3,7 +3,7 @@
>   * Support for GalaxyCore GC0310 VGA camera sensor.
>   *
>   * Copyright (c) 2013 Intel Corporation. All Rights Reserved.
> - * Copyright (c) 2023 Hans de Goede <hdegoede@redhat.com>
> + * Copyright (c) 2023-2025 Hans de Goede <hansg@kernel.org>
>   */
>  
>  #include <linux/delay.h>
> @@ -352,6 +352,43 @@ static int gc0310_get_fmt(struct v4l2_subdev *sd,
>         return 0;
>  }
>  
> +static int gc0310_get_selection(struct v4l2_subdev *sd,
> +                               struct v4l2_subdev_state *state,
> +                               struct v4l2_subdev_selection *sel)
> +{
> +       /* Only the single fixed 656x496 mode is supported, without croping */
> +       switch (sel->target) {
> +       case V4L2_SEL_TGT_CROP:
> +       case V4L2_SEL_TGT_CROP_BOUNDS:
> +       case V4L2_SEL_TGT_CROP_DEFAULT:
> +       case V4L2_SEL_TGT_NATIVE_SIZE:
> +               sel->r.top = 0;
> +               sel->r.left = 0;
> +               sel->r.width = GC0310_NATIVE_WIDTH;
> +               sel->r.height = GC0310_NATIVE_HEIGHT;
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +static int gc0310_set_selection(struct v4l2_subdev *sd,
> +                               struct v4l2_subdev_state *state,
> +                               struct v4l2_subdev_selection *sel)
> +{
> +       if (sel->target != V4L2_SEL_TGT_CROP)
> +               return -EINVAL;
> +
> +       /* Only the single fixed 656x496 mode is supported, without croping */
> +       sel->r.top = 0;
> +       sel->r.left = 0;
> +       sel->r.width = GC0310_NATIVE_WIDTH;
> +       sel->r.height = GC0310_NATIVE_HEIGHT;
> +       return 0;
> +}
> +
>  static int gc0310_detect(struct gc0310_device *sensor)
>  {
>         struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
> @@ -509,6 +546,8 @@ static const struct v4l2_subdev_pad_ops gc0310_pad_ops = {
>         .enum_frame_size = gc0310_enum_frame_size,
>         .get_fmt = gc0310_get_fmt,
>         .set_fmt = gc0310_set_fmt,
> +       .get_selection = gc0310_get_selection,
> +       .set_selection = gc0310_set_selection,

On other sensors I've worked on, we haven't implemented .set_selection()
unless it can be changed. I think this could be simplified here? - Just
the implementation in .get_selection should be enough I think ?

Saves a few lines when it's not configurable.

In imx283.c we have no implementation of .set_selection; though in
imx335.c - we simply set .set_selection = imx335_get_selection;

imx415.c also only sets the .get_selection callback ... so maybe I could
already simplify imx335 too!



>         .get_frame_interval = gc0310_get_frame_interval,
>  };
>  
> @@ -671,5 +710,6 @@ static struct i2c_driver gc0310_driver = {
>  module_i2c_driver(gc0310_driver);
>  
>  MODULE_AUTHOR("Lai, Angie <angie.lai@intel.com>");
> +MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
>  MODULE_DESCRIPTION("A low-level driver for GalaxyCore GC0310 sensors");
>  MODULE_LICENSE("GPL");
> -- 
> 2.49.0
>

Re: [PATCH 06/23] media: atomisp: gc0310: Add selection API support

[Hans de Goede]

Hi Kieran,

On 17-May-25 10:41 PM, Kieran Bingham wrote:
> Quoting Hans de Goede (2025-05-17 12:40:49)
>> Add support for the selection API as expected by libcamera.
>>
>> Note the driver only supports a single fixed resolution and
>> no cropping, so this is a simple read-only implementation.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  .../media/atomisp/i2c/atomisp-gc0310.c        | 42 ++++++++++++++++++-
>>  1 file changed, 41 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
>> index 756e56f639b7..7902e732a3ca 100644
>> --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
>> +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
>> @@ -3,7 +3,7 @@
>>   * Support for GalaxyCore GC0310 VGA camera sensor.
>>   *
>>   * Copyright (c) 2013 Intel Corporation. All Rights Reserved.
>> - * Copyright (c) 2023 Hans de Goede <hdegoede@redhat.com>
>> + * Copyright (c) 2023-2025 Hans de Goede <hansg@kernel.org>
>>   */
>>  
>>  #include <linux/delay.h>
>> @@ -352,6 +352,43 @@ static int gc0310_get_fmt(struct v4l2_subdev *sd,
>>         return 0;
>>  }
>>  
>> +static int gc0310_get_selection(struct v4l2_subdev *sd,
>> +                               struct v4l2_subdev_state *state,
>> +                               struct v4l2_subdev_selection *sel)
>> +{
>> +       /* Only the single fixed 656x496 mode is supported, without croping */
>> +       switch (sel->target) {
>> +       case V4L2_SEL_TGT_CROP:
>> +       case V4L2_SEL_TGT_CROP_BOUNDS:
>> +       case V4L2_SEL_TGT_CROP_DEFAULT:
>> +       case V4L2_SEL_TGT_NATIVE_SIZE:
>> +               sel->r.top = 0;
>> +               sel->r.left = 0;
>> +               sel->r.width = GC0310_NATIVE_WIDTH;
>> +               sel->r.height = GC0310_NATIVE_HEIGHT;
>> +               break;
>> +       default:
>> +               return -EINVAL;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int gc0310_set_selection(struct v4l2_subdev *sd,
>> +                               struct v4l2_subdev_state *state,
>> +                               struct v4l2_subdev_selection *sel)
>> +{
>> +       if (sel->target != V4L2_SEL_TGT_CROP)
>> +               return -EINVAL;
>> +
>> +       /* Only the single fixed 656x496 mode is supported, without croping */
>> +       sel->r.top = 0;
>> +       sel->r.left = 0;
>> +       sel->r.width = GC0310_NATIVE_WIDTH;
>> +       sel->r.height = GC0310_NATIVE_HEIGHT;
>> +       return 0;
>> +}
>> +
>>  static int gc0310_detect(struct gc0310_device *sensor)
>>  {
>>         struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
>> @@ -509,6 +546,8 @@ static const struct v4l2_subdev_pad_ops gc0310_pad_ops = {
>>         .enum_frame_size = gc0310_enum_frame_size,
>>         .get_fmt = gc0310_get_fmt,
>>         .set_fmt = gc0310_set_fmt,
>> +       .get_selection = gc0310_get_selection,
>> +       .set_selection = gc0310_set_selection,
> 
> On other sensors I've worked on, we haven't implemented .set_selection()
> unless it can be changed. I think this could be simplified here? - Just
> the implementation in .get_selection should be enough I think ?
> 
> Saves a few lines when it's not configurable.

Right. I'm working on merging this series now (minus the last
patch), preparing an atomisp pull-request for 6.17, addressing
review comments as I go.

I've gone with using get_selection for set_selection while
merging this.

Regards,

Hans





> 
> In imx283.c we have no implementation of .set_selection; though in
> imx335.c - we simply set .set_selection = imx335_get_selection;
> 
> imx415.c also only sets the .get_selection callback ... so maybe I could
> already simplify imx335 too!
> 
> 
> 
>>         .get_frame_interval = gc0310_get_frame_interval,
>>  };
>>  
>> @@ -671,5 +710,6 @@ static struct i2c_driver gc0310_driver = {
>>  module_i2c_driver(gc0310_driver);
>>  
>>  MODULE_AUTHOR("Lai, Angie <angie.lai@intel.com>");
>> +MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
>>  MODULE_DESCRIPTION("A low-level driver for GalaxyCore GC0310 sensors");
>>  MODULE_LICENSE("GPL");
>> -- 
>> 2.49.0
>>
> 

[PATCH 07/23] media: atomisp: gc0310: Add link-frequency and pixelrate controls

[Hans de Goede]

Add support for the pixelrate control as expected by libcamera,
while at it also add the link-frequency control.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/i2c/atomisp-gc0310.c        | 33 +++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index 7902e732a3ca..2bb309b51a3a 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -24,6 +24,17 @@
 #define GC0310_NATIVE_WIDTH			656
 #define GC0310_NATIVE_HEIGHT			496
 
+/*
+ * The actual PLL output rate is unknown, the datasheet
+ * says that the formula for the frame-time in pixels is:
+ * rowtime = win-width + hblank + sh-delay + 4
+ * frametime = rowtime * (win-height + vblank)
+ * Filling this in and multiplying by 30 fps gives:
+ * pixelrate = (660 + 178 + 42 + 4) * (498 + 27) * 30 = 13923000
+ */
+#define GC0310_PIXELRATE			13923000
+/* single lane, bus-format is 8 bpp, CSI-2 is double data rate */
+#define GC0310_LINK_FREQ			(GC0310_PIXELRATE * 8 / 2)
 #define GC0310_FPS				30
 #define GC0310_SKIP_FRAMES			3
 
@@ -76,6 +87,8 @@ struct gc0310_device {
 		struct v4l2_ctrl_handler handler;
 		struct v4l2_ctrl *exposure;
 		struct v4l2_ctrl *gain;
+		struct v4l2_ctrl *link_freq;
+		struct v4l2_ctrl *pixel_rate;
 	} ctrls;
 };
 
@@ -251,6 +264,10 @@ static const struct reg_sequence gc0310_VGA_30fps[] = {
 	{ 0xfe, 0x00 },
 };
 
+static const s64 link_freq_menu_items[] = {
+	GC0310_LINK_FREQ,
+};
+
 static int gc0310_gain_set(struct gc0310_device *sensor, u32 gain)
 {
 	u8 again, dgain;
@@ -561,7 +578,7 @@ static int gc0310_init_controls(struct gc0310_device *sensor)
 {
 	struct v4l2_ctrl_handler *hdl = &sensor->ctrls.handler;
 
-	v4l2_ctrl_handler_init(hdl, 2);
+	v4l2_ctrl_handler_init(hdl, 4);
 
 	/* Use the same lock for controls as for everything else */
 	hdl->lock = &sensor->input_lock;
@@ -574,7 +591,19 @@ static int gc0310_init_controls(struct gc0310_device *sensor)
 	sensor->ctrls.gain =
 		v4l2_ctrl_new_std(hdl, &ctrl_ops, V4L2_CID_ANALOGUE_GAIN, 0, 95, 1, 31);
 
-	return hdl->error;
+	sensor->ctrls.link_freq =
+		v4l2_ctrl_new_int_menu(hdl, NULL, V4L2_CID_LINK_FREQ,
+				       0, 0, link_freq_menu_items);
+	sensor->ctrls.pixel_rate =
+		v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE, 0,
+				  GC0310_PIXELRATE, 1, GC0310_PIXELRATE);
+
+	if (hdl->error)
+		return hdl->error;
+
+	sensor->ctrls.pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+	sensor->ctrls.link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+	return 0;
 }
 
 static void gc0310_remove(struct i2c_client *client)
-- 
2.49.0

Re: [PATCH 07/23] media: atomisp: gc0310: Add link-frequency and pixelrate controls

[Andy Shevchenko]

On Sat, May 17, 2025 at 01:40:50PM +0200, Hans de Goede wrote:
> Add support for the pixelrate control as expected by libcamera,
> while at it also add the link-frequency control.

...

> +/*
> + * The actual PLL output rate is unknown, the datasheet
> + * says that the formula for the frame-time in pixels is:
> + * rowtime = win-width + hblank + sh-delay + 4
> + * frametime = rowtime * (win-height + vblank)
> + * Filling this in and multiplying by 30 fps gives:
> + * pixelrate = (660 + 178 + 42 + 4) * (498 + 27) * 30 = 13923000
> + */
> +#define GC0310_PIXELRATE			13923000

Why not utilise the preprocessor to calculate the above?
I mean we can define an arithmetic expression instead of the result
(which is mentioned in the comments anyway).

...

> +/* single lane, bus-format is 8 bpp, CSI-2 is double data rate */
> +#define GC0310_LINK_FREQ			(GC0310_PIXELRATE * 8 / 2)

Hmm... I believe it won't be ever the case, but still the Q here is
if we expect rounded up value? In such a case perhaps BITS_TO_BYTES()
would make sense. OTOH the format theoretically can be not only 8bpp
in some cases, but let's say 9bpp, that macro wouldn't help then.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 07/23] media: atomisp: gc0310: Add link-frequency and pixelrate controls

[Hans de Goede]

Hi Andy,

On 19-May-25 1:30 PM, Andy Shevchenko wrote:
> On Sat, May 17, 2025 at 01:40:50PM +0200, Hans de Goede wrote:
>> Add support for the pixelrate control as expected by libcamera,
>> while at it also add the link-frequency control.
> 
> ...
> 
>> +/*
>> + * The actual PLL output rate is unknown, the datasheet
>> + * says that the formula for the frame-time in pixels is:
>> + * rowtime = win-width + hblank + sh-delay + 4
>> + * frametime = rowtime * (win-height + vblank)
>> + * Filling this in and multiplying by 30 fps gives:
>> + * pixelrate = (660 + 178 + 42 + 4) * (498 + 27) * 30 = 13923000
>> + */
>> +#define GC0310_PIXELRATE			13923000
> 
> Why not utilise the preprocessor to calculate the above?
> I mean we can define an arithmetic expression instead of the result
> (which is mentioned in the comments anyway).

I don't really see any added value in using the pre-processor
for this, it just makes the #define line unnecessarily long.

Regards,

Hans


> 
> ...
> 
>> +/* single lane, bus-format is 8 bpp, CSI-2 is double data rate */
>> +#define GC0310_LINK_FREQ			(GC0310_PIXELRATE * 8 / 2)
> 
> Hmm... I believe it won't be ever the case, but still the Q here is
> if we expect rounded up value? In such a case perhaps BITS_TO_BYTES()
> would make sense. OTOH the format theoretically can be not only 8bpp
> in some cases, but let's say 9bpp, that macro wouldn't help then.
> 

Re: [PATCH 07/23] media: atomisp: gc0310: Add link-frequency and pixelrate controls

[Sakari Ailus]

On Sun, Jul 06, 2025 at 11:57:17AM +0200, Hans de Goede wrote:
> Hi Andy,
> 
> On 19-May-25 1:30 PM, Andy Shevchenko wrote:
> > On Sat, May 17, 2025 at 01:40:50PM +0200, Hans de Goede wrote:
> >> Add support for the pixelrate control as expected by libcamera,
> >> while at it also add the link-frequency control.
> > 
> > ...
> > 
> >> +/*
> >> + * The actual PLL output rate is unknown, the datasheet
> >> + * says that the formula for the frame-time in pixels is:
> >> + * rowtime = win-width + hblank + sh-delay + 4
> >> + * frametime = rowtime * (win-height + vblank)
> >> + * Filling this in and multiplying by 30 fps gives:
> >> + * pixelrate = (660 + 178 + 42 + 4) * (498 + 27) * 30 = 13923000
> >> + */
> >> +#define GC0310_PIXELRATE			13923000
> > 
> > Why not utilise the preprocessor to calculate the above?
> > I mean we can define an arithmetic expression instead of the result
> > (which is mentioned in the comments anyway).
> 
> I don't really see any added value in using the pre-processor
> for this, it just makes the #define line unnecessarily long.

My suggestion would be the same as Andy's. If you think the calculated
value should appear there as well, then I'd put it in a comment.

-- 
Sakari Ailus

Re: [PATCH 07/23] media: atomisp: gc0310: Add link-frequency and pixelrate controls

[Hans de Goede]

Hi Sakari,

On 8-Jul-25 10:04 AM, Sakari Ailus wrote:
> On Sun, Jul 06, 2025 at 11:57:17AM +0200, Hans de Goede wrote:
>> Hi Andy,
>>
>> On 19-May-25 1:30 PM, Andy Shevchenko wrote:
>>> On Sat, May 17, 2025 at 01:40:50PM +0200, Hans de Goede wrote:
>>>> Add support for the pixelrate control as expected by libcamera,
>>>> while at it also add the link-frequency control.
>>>
>>> ...
>>>
>>>> +/*
>>>> + * The actual PLL output rate is unknown, the datasheet
>>>> + * says that the formula for the frame-time in pixels is:
>>>> + * rowtime = win-width + hblank + sh-delay + 4
>>>> + * frametime = rowtime * (win-height + vblank)
>>>> + * Filling this in and multiplying by 30 fps gives:
>>>> + * pixelrate = (660 + 178 + 42 + 4) * (498 + 27) * 30 = 13923000
>>>> + */
>>>> +#define GC0310_PIXELRATE			13923000
>>>
>>> Why not utilise the preprocessor to calculate the above?
>>> I mean we can define an arithmetic expression instead of the result
>>> (which is mentioned in the comments anyway).
>>
>> I don't really see any added value in using the pre-processor
>> for this, it just makes the #define line unnecessarily long.
> 
> My suggestion would be the same as Andy's. If you think the calculated
> value should appear there as well, then I'd put it in a comment.

I've already send out an atomisp pull-request for 6.17 keeping this
as is.

But we can fix this up with a follow-up patch I guess.

Regards,

Hans

[PATCH 08/23] media: atomisp: gc0310: Add vblank and hblank controls

[Hans de Goede]

Add support for the vblank and hblank controls, these controls
are mandatory for using the sensor driver with libcamera.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/i2c/atomisp-gc0310.c        | 38 +++++++++++++++++--
 1 file changed, 35 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index 2bb309b51a3a..568b501f7e87 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -62,9 +62,22 @@
 #define GC0310_V_CROP_START_REG			CCI_REG16(0x0B)
 #define GC0310_H_OUTSIZE_REG			CCI_REG16(0x0F)
 #define GC0310_V_OUTSIZE_REG			CCI_REG16(0x0D)
+
 #define GC0310_H_BLANKING_REG			CCI_REG16(0x05)
+/* Hblank-register + sh-delay + H-crop + 4 (from hw) */
+#define GC0310_H_BLANK_DEFAULT			(178 + 42 + 4 + 4)
+
 #define GC0310_V_BLANKING_REG			CCI_REG16(0x07)
+/* Vblank needs an offset compensate for the small V-crop done */
+#define GC0310_V_BLANK_OFFSET			2
+/* Vsync start time + 1 row vsync + vsync end time + offset */
+#define GC0310_V_BLANK_MIN			(9 + 1 + 4 + GC0310_V_BLANK_OFFSET)
+#define GC0310_V_BLANK_DEFAULT			(27 + GC0310_V_BLANK_OFFSET)
+#define GC0310_V_BLANK_MAX			(4095 - GC0310_NATIVE_HEIGHT)
+
 #define GC0310_SH_DELAY_REG			CCI_REG8(0x11)
+#define GC0310_VS_START_TIME_REG		CCI_REG8(0x12)
+#define GC0310_VS_END_TIME_REG			CCI_REG8(0x13)
 
 #define to_gc0310_sensor(x) container_of(x, struct gc0310_device, sd)
 
@@ -89,6 +102,8 @@ struct gc0310_device {
 		struct v4l2_ctrl *gain;
 		struct v4l2_ctrl *link_freq;
 		struct v4l2_ctrl *pixel_rate;
+		struct v4l2_ctrl *vblank;
+		struct v4l2_ctrl *hblank;
 	} ctrls;
 };
 
@@ -147,8 +162,7 @@ static const struct reg_sequence gc0310_reset_register[] = {
 	{ 0x04, 0xc0 }, /* 0xe8 //58 */
 	{ 0x05, 0x00 },
 	{ 0x06, 0xb2 }, /* 0x0a //HB */
-	{ 0x07, 0x00 },
-	{ 0x08, 0x1b }, /* 0x89 //VB */
+	/* Vblank (reg 0x07 + 0x08) gets set by the vblank ctrl */
 	{ 0x09, 0x00 }, /* row start */
 	{ 0x0a, 0x00 },
 	{ 0x0b, 0x00 }, /* col start */
@@ -309,6 +323,11 @@ static int gc0310_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_ANALOGUE_GAIN:
 		ret = gc0310_gain_set(sensor, ctrl->val);
 		break;
+	case V4L2_CID_VBLANK:
+		ret = cci_write(sensor->regmap, GC0310_V_BLANKING_REG,
+				ctrl->val - GC0310_V_BLANK_OFFSET,
+				NULL);
+		break;
 	default:
 		ret = -EINVAL;
 		break;
@@ -578,7 +597,7 @@ static int gc0310_init_controls(struct gc0310_device *sensor)
 {
 	struct v4l2_ctrl_handler *hdl = &sensor->ctrls.handler;
 
-	v4l2_ctrl_handler_init(hdl, 4);
+	v4l2_ctrl_handler_init(hdl, 6);
 
 	/* Use the same lock for controls as for everything else */
 	hdl->lock = &sensor->input_lock;
@@ -598,11 +617,24 @@ static int gc0310_init_controls(struct gc0310_device *sensor)
 		v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_PIXEL_RATE, 0,
 				  GC0310_PIXELRATE, 1, GC0310_PIXELRATE);
 
+	sensor->ctrls.vblank =
+		v4l2_ctrl_new_std(hdl, &ctrl_ops, V4L2_CID_VBLANK,
+				  GC0310_V_BLANK_MIN,
+				  GC0310_V_BLANK_MAX, 1,
+				  GC0310_V_BLANK_DEFAULT);
+
+	sensor->ctrls.hblank =
+		v4l2_ctrl_new_std(hdl, NULL, V4L2_CID_HBLANK,
+				  GC0310_H_BLANK_DEFAULT,
+				  GC0310_H_BLANK_DEFAULT, 1,
+				  GC0310_H_BLANK_DEFAULT);
+
 	if (hdl->error)
 		return hdl->error;
 
 	sensor->ctrls.pixel_rate->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 	sensor->ctrls.link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+	sensor->ctrls.hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 	return 0;
 }
 
-- 
2.49.0

Re: [PATCH 08/23] media: atomisp: gc0310: Add vblank and hblank controls

[Andy Shevchenko]

On Sat, May 17, 2025 at 01:40:51PM +0200, Hans de Goede wrote:
> Add support for the vblank and hblank controls, these controls
> are mandatory for using the sensor driver with libcamera.

...

>  #define GC0310_V_OUTSIZE_REG			CCI_REG16(0x0D)
> +
>  #define GC0310_H_BLANKING_REG			CCI_REG16(0x05)

This blank line can be added when you converted code to CCI. I believe it
belongs to there more than to here.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 09/23] media: atomisp: gc0310: Add camera orientation and sensor rotation controls

[Hans de Goede]

Add camera orientation and sensor rotation controls using
the v4l2_fwnode_device_parse() and v4l2_ctrl_new_fwnode_properties()
helpers.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index 568b501f7e87..889e03b89ee7 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -20,6 +20,7 @@
 #include <media/v4l2-cci.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
 
 #define GC0310_NATIVE_WIDTH			656
 #define GC0310_NATIVE_HEIGHT			496
@@ -595,9 +596,12 @@ static const struct v4l2_subdev_ops gc0310_ops = {
 
 static int gc0310_init_controls(struct gc0310_device *sensor)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
 	struct v4l2_ctrl_handler *hdl = &sensor->ctrls.handler;
+	struct v4l2_fwnode_device_properties props;
+	int ret;
 
-	v4l2_ctrl_handler_init(hdl, 6);
+	v4l2_ctrl_handler_init(hdl, 8);
 
 	/* Use the same lock for controls as for everything else */
 	hdl->lock = &sensor->input_lock;
@@ -629,6 +633,12 @@ static int gc0310_init_controls(struct gc0310_device *sensor)
 				  GC0310_H_BLANK_DEFAULT, 1,
 				  GC0310_H_BLANK_DEFAULT);
 
+	ret = v4l2_fwnode_device_parse(&client->dev, &props);
+	if (ret)
+		return ret;
+
+	v4l2_ctrl_new_fwnode_properties(hdl, &ctrl_ops, &props);
+
 	if (hdl->error)
 		return hdl->error;
 
-- 
2.49.0

[PATCH 10/23] media: atomisp: gc0310: Limit max exposure value to mode-height + vblank

[Hans de Goede]

When an exposure value > (mode-height + vblank) gets set the sensor will
automatically increase vblank, lowering the framerate.

This is not desirable, limit exposure the maximum exposure to mode-height +
vblank to avoid the unwanted framerate slowdown.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/i2c/atomisp-gc0310.c        | 21 +++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index 889e03b89ee7..e053982a6512 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -306,12 +306,27 @@ static int gc0310_gain_set(struct gc0310_device *sensor, u32 gain)
 	return ret;
 }
 
+static int gc0310_exposure_update_range(struct gc0310_device *sensor)
+{
+	int exp_max = GC0310_NATIVE_HEIGHT + sensor->ctrls.vblank->val;
+
+	return __v4l2_ctrl_modify_range(sensor->ctrls.exposure, 0, exp_max,
+					1, exp_max);
+}
+
 static int gc0310_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct gc0310_device *sensor =
 		container_of(ctrl->handler, struct gc0310_device, ctrls.handler);
 	int ret;
 
+	/* Update exposure range on vblank changes */
+	if (ctrl->id == V4L2_CID_VBLANK) {
+		ret = gc0310_exposure_update_range(sensor);
+		if (ret)
+			return ret;
+	}
+
 	/* Only apply changes to the controls if the device is powered up */
 	if (!pm_runtime_get_if_in_use(sensor->sd.dev))
 		return 0;
@@ -599,7 +614,7 @@ static int gc0310_init_controls(struct gc0310_device *sensor)
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
 	struct v4l2_ctrl_handler *hdl = &sensor->ctrls.handler;
 	struct v4l2_fwnode_device_properties props;
-	int ret;
+	int exp_max, ret;
 
 	v4l2_ctrl_handler_init(hdl, 8);
 
@@ -607,8 +622,10 @@ static int gc0310_init_controls(struct gc0310_device *sensor)
 	hdl->lock = &sensor->input_lock;
 	sensor->sd.ctrl_handler = hdl;
 
+	exp_max = GC0310_NATIVE_HEIGHT + GC0310_V_BLANK_DEFAULT;
 	sensor->ctrls.exposure =
-		v4l2_ctrl_new_std(hdl, &ctrl_ops, V4L2_CID_EXPOSURE, 0, 4095, 1, 1023);
+		v4l2_ctrl_new_std(hdl, &ctrl_ops, V4L2_CID_EXPOSURE, 0,
+				  exp_max, 1, exp_max);
 
 	/* 32 steps at base gain 1 + 64 half steps at base gain 2 */
 	sensor->ctrls.gain =
-- 
2.49.0

[PATCH 11/23] media: atomisp: gc0310: Add check_hwcfg() function

[Hans de Goede]

Add a check_hwcfg() function to check if the external clk-freq, CSI
link-freq and lane-count match the driver's expectations.

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

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index e053982a6512..8acb4517c67b 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -36,6 +36,7 @@
 #define GC0310_PIXELRATE			13923000
 /* single lane, bus-format is 8 bpp, CSI-2 is double data rate */
 #define GC0310_LINK_FREQ			(GC0310_PIXELRATE * 8 / 2)
+#define GC0310_MCLK_FREQ			19200000
 #define GC0310_FPS				30
 #define GC0310_SKIP_FRAMES			3
 
@@ -679,21 +680,68 @@ static void gc0310_remove(struct i2c_client *client)
 	pm_runtime_disable(&client->dev);
 }
 
-static int gc0310_probe(struct i2c_client *client)
+static int gc0310_check_hwcfg(struct device *dev)
 {
+	struct v4l2_fwnode_endpoint bus_cfg = {
+		.bus_type = V4L2_MBUS_CSI2_DPHY
+	};
 	struct fwnode_handle *ep_fwnode;
-	struct gc0310_device *sensor;
+	unsigned long link_freq_bitmap;
+	u32 mclk;
 	int ret;
 
 	/*
 	 * Sometimes the fwnode graph is initialized by the bridge driver.
 	 * Bridge drivers doing this may also add GPIO mappings, wait for this.
 	 */
-	ep_fwnode = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL);
+	ep_fwnode = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0, 0);
 	if (!ep_fwnode)
-		return dev_err_probe(&client->dev, -EPROBE_DEFER, "waiting for fwnode graph endpoint\n");
+		return dev_err_probe(dev, -EPROBE_DEFER,
+				     "waiting for fwnode graph endpoint\n");
 
+	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
+				       &mclk);
+	if (ret) {
+		fwnode_handle_put(ep_fwnode);
+		return dev_err_probe(dev, ret,
+				     "reading clock-frequency property\n");
+	}
+
+	if (mclk != GC0310_MCLK_FREQ) {
+		fwnode_handle_put(ep_fwnode);
+		return dev_err_probe(dev, -EINVAL,
+				     "external clock %u is not supported\n",
+				     mclk);
+	}
+
+	ret = v4l2_fwnode_endpoint_alloc_parse(ep_fwnode, &bus_cfg);
 	fwnode_handle_put(ep_fwnode);
+	if (ret)
+		return dev_err_probe(dev, ret, "parsing endpoint failed\n");
+
+	ret = v4l2_link_freq_to_bitmap(dev, bus_cfg.link_frequencies,
+				       bus_cfg.nr_of_link_frequencies,
+				       link_freq_menu_items,
+				       ARRAY_SIZE(link_freq_menu_items),
+				       &link_freq_bitmap);
+
+	if (ret == 0 && bus_cfg.bus.mipi_csi2.num_data_lanes != 1)
+		ret = dev_err_probe(dev, -EINVAL,
+				    "number of CSI2 data lanes %u is not supported\n",
+				    bus_cfg.bus.mipi_csi2.num_data_lanes);
+
+	v4l2_fwnode_endpoint_free(&bus_cfg);
+	return ret;
+}
+
+static int gc0310_probe(struct i2c_client *client)
+{
+	struct gc0310_device *sensor;
+	int ret;
+
+	ret = gc0310_check_hwcfg(&client->dev);
+	if (ret)
+		return ret;
 
 	sensor = devm_kzalloc(&client->dev, sizeof(*sensor), GFP_KERNEL);
 	if (!sensor)
-- 
2.49.0

Re: [PATCH 11/23] media: atomisp: gc0310: Add check_hwcfg() function

[Andy Shevchenko]

On Sat, May 17, 2025 at 01:40:54PM +0200, Hans de Goede wrote:
> Add a check_hwcfg() function to check if the external clk-freq, CSI
> link-freq and lane-count match the driver's expectations.

...

> +	struct v4l2_fwnode_endpoint bus_cfg = {
> +		.bus_type = V4L2_MBUS_CSI2_DPHY

Leave trailing comma, it might help in case we add anything later on.

> +	};

...

> +	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
> +				       &mclk);
> +	if (ret) {
> +		fwnode_handle_put(ep_fwnode);

Hmm... Can we switch driver to use __free() instead?

> +		return dev_err_probe(dev, ret,
> +				     "reading clock-frequency property\n");
> +	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 11/23] media: atomisp: gc0310: Add check_hwcfg() function

[Hans de Goede]

Hi,

On 19-May-25 1:35 PM, Andy Shevchenko wrote:
> On Sat, May 17, 2025 at 01:40:54PM +0200, Hans de Goede wrote:
>> Add a check_hwcfg() function to check if the external clk-freq, CSI
>> link-freq and lane-count match the driver's expectations.
> 
> ...
> 
>> +	struct v4l2_fwnode_endpoint bus_cfg = {
>> +		.bus_type = V4L2_MBUS_CSI2_DPHY
> 
> Leave trailing comma, it might help in case we add anything later on.

Ack I've fixed this while merging this series
for the atomisp pull-request for 6.17 which I'm preparing.

> 
>> +	};
> 
> ...
> 
>> +	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency",
>> +				       &mclk);
>> +	if (ret) {
>> +		fwnode_handle_put(ep_fwnode);
> 
> Hmm... Can we switch driver to use __free() instead?

None of the other sensor drivers do that and IIRC it is
also put a few lines below when we're done with it,
well before the end of the function making __free not
suitable.

Regards,

Hans




> 
>> +		return dev_err_probe(dev, ret,
>> +				     "reading clock-frequency property\n");
>> +	}
> 

[PATCH 12/23] media: atomisp: gc0310: Fix power on/off sleep times

[Hans de Goede]

Reduce the unnecessary long msleep(100) done on stream start to 10 ms and
move this to gc0310_resume() so that it is also done on the initial
power-up done by gc0310_detect(), which should fix gc0310_detect()
sometimes failing.

While at it switch the sleeps from msleep() / usleep_range() to fsleep().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index 8acb4517c67b..162b81789eca 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -488,8 +488,6 @@ static int gc0310_s_stream(struct v4l2_subdev *sd, int enable)
 		if (ret < 0)
 			goto error_power_down;
 
-		msleep(100);
-
 		ret = regmap_multi_reg_write(sensor->regmap,
 					     gc0310_reset_register,
 					     ARRAY_SIZE(gc0310_reset_register));
@@ -818,10 +816,11 @@ static int gc0310_resume(struct device *dev)
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct gc0310_device *sensor = to_gc0310_sensor(sd);
 
-	usleep_range(10000, 15000);
+	fsleep(10000);
 	gpiod_set_value_cansleep(sensor->reset, 0);
-	usleep_range(10000, 15000);
+	fsleep(10000);
 	gpiod_set_value_cansleep(sensor->powerdown, 0);
+	fsleep(10000);
 
 	return 0;
 }
-- 
2.49.0

Re: [PATCH 12/23] media: atomisp: gc0310: Fix power on/off sleep times

[Andy Shevchenko]

On Sat, May 17, 2025 at 01:40:55PM +0200, Hans de Goede wrote:
> Reduce the unnecessary long msleep(100) done on stream start to 10 ms and
> move this to gc0310_resume() so that it is also done on the initial
> power-up done by gc0310_detect(), which should fix gc0310_detect()
> sometimes failing.
> 
> While at it switch the sleeps from msleep() / usleep_range() to fsleep().

...

> +	fsleep(10000);

> +	fsleep(10000);

> +	fsleep(10000);

fsleep(10 * USEC_PER_MSEC) ?

This way it helps to get immediately that we want 10ms sleep.

-- 
With Best Regards,
Andy Shevchenko

[PATCH 13/23] media: atomisp: gc0310: Remove unused is_streaming variable

[Hans de Goede]

is_streaming is only set and never read, drop it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index 162b81789eca..948579c0373d 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -88,7 +88,6 @@ struct gc0310_device {
 	struct media_pad pad;
 	/* Protect against concurrent changes to controls */
 	struct mutex input_lock;
-	bool is_streaming;
 
 	struct regmap *regmap;
 	struct gpio_desc *reset;
@@ -522,13 +521,11 @@ static int gc0310_s_stream(struct v4l2_subdev *sd, int enable)
 	if (!enable)
 		pm_runtime_put(&client->dev);
 
-	sensor->is_streaming = enable;
 	mutex_unlock(&sensor->input_lock);
 	return 0;
 
 error_power_down:
 	pm_runtime_put(&client->dev);
-	sensor->is_streaming = false;
 	mutex_unlock(&sensor->input_lock);
 	return ret;
 }
-- 
2.49.0

[PATCH 14/23] media: atomisp: gc0310: Switch to {enable,disable}_streams

[Hans de Goede]

Switch from s_stream() to enable_streams() and disable_streams() pad
operations. They are preferred and required for streams support.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/i2c/atomisp-gc0310.c        | 94 +++++++++++--------
 1 file changed, 54 insertions(+), 40 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index 948579c0373d..5d25be0f4cf4 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -473,58 +473,70 @@ static int gc0310_detect(struct gc0310_device *sensor)
 	return 0;
 }
 
-static int gc0310_s_stream(struct v4l2_subdev *sd, int enable)
+static int gc0310_enable_streams(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_state *state,
+				 u32 pad, u64 streams_mask)
+{
+	struct gc0310_device *sensor = to_gc0310_sensor(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	int ret;
+
+	mutex_lock(&sensor->input_lock);
+
+	ret = pm_runtime_get_sync(&client->dev);
+	if (ret)
+		goto error_power_down;
+
+	ret = regmap_multi_reg_write(sensor->regmap,
+				     gc0310_reset_register,
+				     ARRAY_SIZE(gc0310_reset_register));
+	if (ret)
+		goto error_power_down;
+
+	ret = regmap_multi_reg_write(sensor->regmap,
+				     gc0310_VGA_30fps,
+				     ARRAY_SIZE(gc0310_VGA_30fps));
+	if (ret)
+		goto error_power_down;
+
+	/* restore value of all ctrls */
+	ret = __v4l2_ctrl_handler_setup(&sensor->ctrls.handler);
+
+	/* enable per frame MIPI and sensor ctrl reset  */
+	cci_write(sensor->regmap, GC0310_RESET_RELATED_REG, 0x30, &ret);
+
+	cci_write(sensor->regmap, GC0310_RESET_RELATED_REG,
+		  GC0310_REGISTER_PAGE_3, &ret);
+	cci_write(sensor->regmap, GC0310_SW_STREAM_REG,
+		  GC0310_START_STREAMING, &ret);
+	cci_write(sensor->regmap, GC0310_RESET_RELATED_REG,
+		  GC0310_REGISTER_PAGE_0, &ret);
+
+error_power_down:
+	if (ret)
+		pm_runtime_put(&client->dev);
+
+	mutex_unlock(&sensor->input_lock);
+	return ret;
+}
+
+static int gc0310_disable_streams(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_state *state,
+				  u32 pad, u64 streams_mask)
 {
 	struct gc0310_device *sensor = to_gc0310_sensor(sd);
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	int ret = 0;
 
-	dev_dbg(&client->dev, "%s S enable=%d\n", __func__, enable);
 	mutex_lock(&sensor->input_lock);
 
-	if (enable) {
-		ret = pm_runtime_get_sync(&client->dev);
-		if (ret < 0)
-			goto error_power_down;
-
-		ret = regmap_multi_reg_write(sensor->regmap,
-					     gc0310_reset_register,
-					     ARRAY_SIZE(gc0310_reset_register));
-		if (ret)
-			goto error_power_down;
-
-		ret = regmap_multi_reg_write(sensor->regmap,
-					     gc0310_VGA_30fps,
-					     ARRAY_SIZE(gc0310_VGA_30fps));
-		if (ret)
-			goto error_power_down;
-
-		/* restore value of all ctrls */
-		ret = __v4l2_ctrl_handler_setup(&sensor->ctrls.handler);
-		if (ret)
-			goto error_power_down;
-
-		/* enable per frame MIPI and sensor ctrl reset  */
-		cci_write(sensor->regmap, GC0310_RESET_RELATED_REG, 0x30, &ret);
-	}
-
 	cci_write(sensor->regmap, GC0310_RESET_RELATED_REG,
 		  GC0310_REGISTER_PAGE_3, &ret);
 	cci_write(sensor->regmap, GC0310_SW_STREAM_REG,
-		  enable ? GC0310_START_STREAMING : GC0310_STOP_STREAMING,
-		  &ret);
+		  GC0310_STOP_STREAMING, &ret);
 	cci_write(sensor->regmap, GC0310_RESET_RELATED_REG,
 		  GC0310_REGISTER_PAGE_0, &ret);
-	if (ret)
-		goto error_power_down;
 
-	if (!enable)
-		pm_runtime_put(&client->dev);
-
-	mutex_unlock(&sensor->input_lock);
-	return 0;
-
-error_power_down:
 	pm_runtime_put(&client->dev);
 	mutex_unlock(&sensor->input_lock);
 	return ret;
@@ -586,7 +598,7 @@ static const struct v4l2_subdev_sensor_ops gc0310_sensor_ops = {
 };
 
 static const struct v4l2_subdev_video_ops gc0310_video_ops = {
-	.s_stream = gc0310_s_stream,
+	.s_stream = v4l2_subdev_s_stream_helper,
 };
 
 static const struct v4l2_subdev_pad_ops gc0310_pad_ops = {
@@ -597,6 +609,8 @@ static const struct v4l2_subdev_pad_ops gc0310_pad_ops = {
 	.get_selection = gc0310_get_selection,
 	.set_selection = gc0310_set_selection,
 	.get_frame_interval = gc0310_get_frame_interval,
+	.enable_streams = gc0310_enable_streams,
+	.disable_streams = gc0310_disable_streams,
 };
 
 static const struct v4l2_subdev_ops gc0310_ops = {
-- 
2.49.0

Re: [PATCH 14/23] media: atomisp: gc0310: Switch to {enable,disable}_streams

[Andy Shevchenko]

On Sat, May 17, 2025 at 01:40:57PM +0200, Hans de Goede wrote:
> Switch from s_stream() to enable_streams() and disable_streams() pad
> operations. They are preferred and required for streams support.

...

(side note: that cleanup.h may help with locking as well)

> +	ret = pm_runtime_get_sync(&client->dev);
> +	if (ret)
> +		goto error_power_down;

Hmm... Don't you want rather to have pm_runtime_resume_and_get()?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 14/23] media: atomisp: gc0310: Switch to {enable,disable}_streams

[Hans de Goede]

Hi Andy,

On 19-May-25 1:43 PM, Andy Shevchenko wrote:
> On Sat, May 17, 2025 at 01:40:57PM +0200, Hans de Goede wrote:
>> Switch from s_stream() to enable_streams() and disable_streams() pad
>> operations. They are preferred and required for streams support.
> 
> ...
> 
> (side note: that cleanup.h may help with locking as well)
> 
>> +	ret = pm_runtime_get_sync(&client->dev);
>> +	if (ret)
>> +		goto error_power_down;
> 
> Hmm... Don't you want rather to have pm_runtime_resume_and_get()?

Ack I've fixed this while merging the series into
my media-atomisp branch for the upcoming atomisp pull-req
for 6.17 .

Regards,

Hans

[PATCH 15/23] media: atomisp: gc0310: Switch to using the sub-device state lock

[Hans de Goede]

Switch to using the sub-device state lock and properly call
v4l2_subdev_init_finalize() / v4l2_subdev_cleanup() on probe() /
remove().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/i2c/atomisp-gc0310.c        | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index 5d25be0f4cf4..a099f0975e1d 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -86,8 +86,6 @@
 struct gc0310_device {
 	struct v4l2_subdev sd;
 	struct media_pad pad;
-	/* Protect against concurrent changes to controls */
-	struct mutex input_lock;
 
 	struct regmap *regmap;
 	struct gpio_desc *reset;
@@ -481,8 +479,6 @@ static int gc0310_enable_streams(struct v4l2_subdev *sd,
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	int ret;
 
-	mutex_lock(&sensor->input_lock);
-
 	ret = pm_runtime_get_sync(&client->dev);
 	if (ret)
 		goto error_power_down;
@@ -516,7 +512,6 @@ static int gc0310_enable_streams(struct v4l2_subdev *sd,
 	if (ret)
 		pm_runtime_put(&client->dev);
 
-	mutex_unlock(&sensor->input_lock);
 	return ret;
 }
 
@@ -528,8 +523,6 @@ static int gc0310_disable_streams(struct v4l2_subdev *sd,
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	int ret = 0;
 
-	mutex_lock(&sensor->input_lock);
-
 	cci_write(sensor->regmap, GC0310_RESET_RELATED_REG,
 		  GC0310_REGISTER_PAGE_3, &ret);
 	cci_write(sensor->regmap, GC0310_SW_STREAM_REG,
@@ -538,7 +531,6 @@ static int gc0310_disable_streams(struct v4l2_subdev *sd,
 		  GC0310_REGISTER_PAGE_0, &ret);
 
 	pm_runtime_put(&client->dev);
-	mutex_unlock(&sensor->input_lock);
 	return ret;
 }
 
@@ -629,7 +621,6 @@ static int gc0310_init_controls(struct gc0310_device *sensor)
 	v4l2_ctrl_handler_init(hdl, 8);
 
 	/* Use the same lock for controls as for everything else */
-	hdl->lock = &sensor->input_lock;
 	sensor->sd.ctrl_handler = hdl;
 
 	exp_max = GC0310_NATIVE_HEIGHT + GC0310_V_BLANK_DEFAULT;
@@ -683,9 +674,9 @@ static void gc0310_remove(struct i2c_client *client)
 	dev_dbg(&client->dev, "gc0310_remove...\n");
 
 	v4l2_async_unregister_subdev(sd);
+	v4l2_subdev_cleanup(sd);
 	media_entity_cleanup(&sensor->sd.entity);
 	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
-	mutex_destroy(&sensor->input_lock);
 	pm_runtime_disable(&client->dev);
 }
 
@@ -768,7 +759,6 @@ static int gc0310_probe(struct i2c_client *client)
 				     "getting powerdown GPIO\n");
 	}
 
-	mutex_init(&sensor->input_lock);
 	v4l2_i2c_subdev_init(&sensor->sd, client, &gc0310_ops);
 	gc0310_fill_format(&sensor->mode.fmt);
 
@@ -803,6 +793,13 @@ static int gc0310_probe(struct i2c_client *client)
 		return ret;
 	}
 
+	sensor->sd.state_lock = sensor->ctrls.handler.lock;
+	ret = v4l2_subdev_init_finalize(&sensor->sd);
+	if (ret) {
+		gc0310_remove(client);
+		return ret;
+	}
+
 	ret = v4l2_async_register_subdev_sensor(&sensor->sd);
 	if (ret) {
 		gc0310_remove(client);
-- 
2.49.0

Re: [PATCH 15/23] media: atomisp: gc0310: Switch to using the sub-device state lock

[Andy Shevchenko]

On Sat, May 17, 2025 at 01:40:58PM +0200, Hans de Goede wrote:
> Switch to using the sub-device state lock and properly call
> v4l2_subdev_init_finalize() / v4l2_subdev_cleanup() on probe() /
> remove().

Ah, cool! But can this patch be put before the previous one which adds some
locking?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 15/23] media: atomisp: gc0310: Switch to using the sub-device state lock

[Hans de Goede]

Hi,

On 19-May-25 1:44 PM, Andy Shevchenko wrote:
> On Sat, May 17, 2025 at 01:40:58PM +0200, Hans de Goede wrote:
>> Switch to using the sub-device state lock and properly call
>> v4l2_subdev_init_finalize() / v4l2_subdev_cleanup() on probe() /
>> remove().
> 
> Ah, cool! But can this patch be put before the previous one which adds some
> locking?

Switching the order of these 2 patches up is a bit painful
(I tried) and the end result is the same so I'm going to keep
this as is.

Regards,

Hans

[PATCH 16/23] media: atomisp: gc0310: Implement internal_ops.init_state

[Hans de Goede]

Implement internal_ops.init_state to fill in the v4l2_mbus_framefmt
struct in newly allocated sd-state structs.

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

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index a099f0975e1d..7f655285bf62 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -611,6 +611,17 @@ static const struct v4l2_subdev_ops gc0310_ops = {
 	.sensor = &gc0310_sensor_ops,
 };
 
+static int gc0310_init_state(struct v4l2_subdev *sd,
+			     struct v4l2_subdev_state *sd_state)
+{
+	gc0310_fill_format(v4l2_subdev_state_get_format(sd_state, 0));
+	return 0;
+}
+
+static const struct v4l2_subdev_internal_ops gc0310_internal_ops = {
+	.init_state = gc0310_init_state,
+};
+
 static int gc0310_init_controls(struct gc0310_device *sensor)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
@@ -777,6 +788,7 @@ static int gc0310_probe(struct i2c_client *client)
 		return ret;
 	}
 
+	sensor->sd.internal_ops = &gc0310_internal_ops;
 	sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 	sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
 	sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
-- 
2.49.0

[PATCH 17/23] media: atomisp: gc0310: Use v4l2_subdev_get_fmt() as v4l2_subdev_pad_ops.get_fmt()

[Hans de Goede]

Now that the sd-state's fmt is properly initialized by
internal_ops.init_state(), the driver can be safely switched
to v4l2_subdev_get_fmt().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index 7f655285bf62..a9d0afbbe7ef 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -390,18 +390,6 @@ static int gc0310_set_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static int gc0310_get_fmt(struct v4l2_subdev *sd,
-			  struct v4l2_subdev_state *sd_state,
-			  struct v4l2_subdev_format *format)
-{
-	struct gc0310_device *sensor = to_gc0310_sensor(sd);
-	struct v4l2_mbus_framefmt *fmt;
-
-	fmt = gc0310_get_pad_format(sensor, sd_state, format->pad, format->which);
-	format->format = *fmt;
-	return 0;
-}
-
 static int gc0310_get_selection(struct v4l2_subdev *sd,
 				struct v4l2_subdev_state *state,
 				struct v4l2_subdev_selection *sel)
@@ -596,7 +584,7 @@ static const struct v4l2_subdev_video_ops gc0310_video_ops = {
 static const struct v4l2_subdev_pad_ops gc0310_pad_ops = {
 	.enum_mbus_code = gc0310_enum_mbus_code,
 	.enum_frame_size = gc0310_enum_frame_size,
-	.get_fmt = gc0310_get_fmt,
+	.get_fmt = v4l2_subdev_get_fmt,
 	.set_fmt = gc0310_set_fmt,
 	.get_selection = gc0310_get_selection,
 	.set_selection = gc0310_set_selection,
-- 
2.49.0

[PATCH 18/23] media: atomisp: gc0310: Switch to using sd.active_state fmt

[Hans de Goede]

Stop having a v4l2_mbus_framefmt mode.fmt driver-data member to store
the fmt for the active-state, instead use sd.active_state fmt.

This also removes the need for gc0310_get_pad_format() since
v4l2_subdev_state_get_format() now will return the correct
v4l2_mbus_framefmt for all whence values.

Instead of switching gc0310_set_fmt() from gc0310_get_pad_format() to
v4l2_subdev_state_get_format() just drop it entirely since there is only
1 fixed mode. Otherwise the new gc0310_set_fmt() would be 100% the same
as v4l2_subdev_get_fmt() after this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/i2c/atomisp-gc0310.c        | 32 +------------------
 1 file changed, 1 insertion(+), 31 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index a9d0afbbe7ef..5936e25fb8c7 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -91,10 +91,6 @@ struct gc0310_device {
 	struct gpio_desc *reset;
 	struct gpio_desc *powerdown;
 
-	struct gc0310_mode {
-		struct v4l2_mbus_framefmt fmt;
-	} mode;
-
 	struct gc0310_ctrls {
 		struct v4l2_ctrl_handler handler;
 		struct v4l2_ctrl *exposure;
@@ -355,17 +351,6 @@ static const struct v4l2_ctrl_ops ctrl_ops = {
 	.s_ctrl = gc0310_s_ctrl,
 };
 
-static struct v4l2_mbus_framefmt *
-gc0310_get_pad_format(struct gc0310_device *sensor,
-		      struct v4l2_subdev_state *state,
-		      unsigned int pad, enum v4l2_subdev_format_whence which)
-{
-	if (which == V4L2_SUBDEV_FORMAT_TRY)
-		return v4l2_subdev_state_get_format(state, pad);
-
-	return &sensor->mode.fmt;
-}
-
 /* The GC0310 currently only supports 1 fixed fmt */
 static void gc0310_fill_format(struct v4l2_mbus_framefmt *fmt)
 {
@@ -376,20 +361,6 @@ static void gc0310_fill_format(struct v4l2_mbus_framefmt *fmt)
 	fmt->code = MEDIA_BUS_FMT_SGRBG8_1X8;
 }
 
-static int gc0310_set_fmt(struct v4l2_subdev *sd,
-			  struct v4l2_subdev_state *sd_state,
-			  struct v4l2_subdev_format *format)
-{
-	struct gc0310_device *sensor = to_gc0310_sensor(sd);
-	struct v4l2_mbus_framefmt *fmt;
-
-	fmt = gc0310_get_pad_format(sensor, sd_state, format->pad, format->which);
-	gc0310_fill_format(fmt);
-
-	format->format = *fmt;
-	return 0;
-}
-
 static int gc0310_get_selection(struct v4l2_subdev *sd,
 				struct v4l2_subdev_state *state,
 				struct v4l2_subdev_selection *sel)
@@ -585,7 +556,7 @@ static const struct v4l2_subdev_pad_ops gc0310_pad_ops = {
 	.enum_mbus_code = gc0310_enum_mbus_code,
 	.enum_frame_size = gc0310_enum_frame_size,
 	.get_fmt = v4l2_subdev_get_fmt,
-	.set_fmt = gc0310_set_fmt,
+	.set_fmt = v4l2_subdev_get_fmt, /* Only 1 fixed mode supported */
 	.get_selection = gc0310_get_selection,
 	.set_selection = gc0310_set_selection,
 	.get_frame_interval = gc0310_get_frame_interval,
@@ -759,7 +730,6 @@ static int gc0310_probe(struct i2c_client *client)
 	}
 
 	v4l2_i2c_subdev_init(&sensor->sd, client, &gc0310_ops);
-	gc0310_fill_format(&sensor->mode.fmt);
 
 	sensor->regmap = devm_cci_regmap_init_i2c(client, 8);
 	if (IS_ERR(sensor->regmap))
-- 
2.49.0

[PATCH 19/23] media: atomisp: gc0310: Move and rename suspend/resume functions

[Hans de Goede]

Move the suspend()/resume() functions to above gc0310_detect() and rename
the functions to power_off()/power_on().

No functional changes, this is a preparation patch for reworking
the runtime-pm handling in probe() and remove().

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/i2c/atomisp-gc0310.c        | 51 ++++++++++---------
 1 file changed, 26 insertions(+), 25 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index 5936e25fb8c7..9532114d6139 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -398,6 +398,30 @@ static int gc0310_set_selection(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int gc0310_power_off(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct gc0310_device *sensor = to_gc0310_sensor(sd);
+
+	gpiod_set_value_cansleep(sensor->powerdown, 1);
+	gpiod_set_value_cansleep(sensor->reset, 1);
+	return 0;
+}
+
+static int gc0310_power_on(struct device *dev)
+{
+	struct v4l2_subdev *sd = dev_get_drvdata(dev);
+	struct gc0310_device *sensor = to_gc0310_sensor(sd);
+
+	fsleep(10000);
+	gpiod_set_value_cansleep(sensor->reset, 0);
+	fsleep(10000);
+	gpiod_set_value_cansleep(sensor->powerdown, 0);
+	fsleep(10000);
+
+	return 0;
+}
+
 static int gc0310_detect(struct gc0310_device *sensor)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
@@ -779,31 +803,8 @@ static int gc0310_probe(struct i2c_client *client)
 	return 0;
 }
 
-static int gc0310_suspend(struct device *dev)
-{
-	struct v4l2_subdev *sd = dev_get_drvdata(dev);
-	struct gc0310_device *sensor = to_gc0310_sensor(sd);
-
-	gpiod_set_value_cansleep(sensor->powerdown, 1);
-	gpiod_set_value_cansleep(sensor->reset, 1);
-	return 0;
-}
-
-static int gc0310_resume(struct device *dev)
-{
-	struct v4l2_subdev *sd = dev_get_drvdata(dev);
-	struct gc0310_device *sensor = to_gc0310_sensor(sd);
-
-	fsleep(10000);
-	gpiod_set_value_cansleep(sensor->reset, 0);
-	fsleep(10000);
-	gpiod_set_value_cansleep(sensor->powerdown, 0);
-	fsleep(10000);
-
-	return 0;
-}
-
-static DEFINE_RUNTIME_DEV_PM_OPS(gc0310_pm_ops, gc0310_suspend, gc0310_resume, NULL);
+static DEFINE_RUNTIME_DEV_PM_OPS(gc0310_pm_ops, gc0310_power_off,
+				 gc0310_power_on, NULL);
 
 static const struct acpi_device_id gc0310_acpi_match[] = {
 	{"INT0310"},
-- 
2.49.0

Re: [PATCH 19/23] media: atomisp: gc0310: Move and rename suspend/resume functions

[Andy Shevchenko]

On Sat, May 17, 2025 at 01:41:02PM +0200, Hans de Goede wrote:
> Move the suspend()/resume() functions to above gc0310_detect() and rename
> the functions to power_off()/power_on().
> 
> No functional changes, this is a preparation patch for reworking
> the runtime-pm handling in probe() and remove().

...

> +static DEFINE_RUNTIME_DEV_PM_OPS(gc0310_pm_ops, gc0310_power_off,
> +				 gc0310_power_on, NULL);

I prefer logical split

static DEFINE_RUNTIME_DEV_PM_OPS(gc0310_pm_ops,
				 gc0310_power_off, gc0310_power_on, NULL);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 19/23] media: atomisp: gc0310: Move and rename suspend/resume functions

[Hans de Goede]

Hi Andy,

On 19-May-25 1:50 PM, Andy Shevchenko wrote:
> On Sat, May 17, 2025 at 01:41:02PM +0200, Hans de Goede wrote:
>> Move the suspend()/resume() functions to above gc0310_detect() and rename
>> the functions to power_off()/power_on().
>>
>> No functional changes, this is a preparation patch for reworking
>> the runtime-pm handling in probe() and remove().
> 
> ...
> 
>> +static DEFINE_RUNTIME_DEV_PM_OPS(gc0310_pm_ops, gc0310_power_off,
>> +				 gc0310_power_on, NULL);
> 
> I prefer logical split
> 
> static DEFINE_RUNTIME_DEV_PM_OPS(gc0310_pm_ops,
> 				 gc0310_power_off, gc0310_power_on, NULL);

Ack, fixed while merging.

Regards,

Hans

[PATCH 20/23] media: atomisp: gc0310: runtime-PM fixes

[Hans de Goede]

The i2c-client's power-domain has already been powered up when probe()
gets called. So e.g. ACPI resources for regulators and clks have
already been enabled and only the GPIOs need to be set to the on state.

Instead of calling pm_runtime_set_suspended() while the domain is
already powered up and then have detect() do a pm_runtime_get()
to set the GPIOs do the following:

1. Call gc0310_power_on() to only set the GPIOs
2. Set the device's runtime-PM state to active instead of suspended
3. Avoid the device getting suspended as soon as pm_runtime_enable()
   gets called by calling pm_runtime_get() before _enable(), this means
   moving the pm_runtime_get() / _put() from detect() to probe ()

This fixes power_on() not getting called when runtime-PM is not
enabled in the Kconfig and this keeps the sensor powered-up while
registering it avoiding unnecessary power cycles.

Also modify gc0310_remove() to power-off the device if it is in
active state when gc0310_remove() runs.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../media/atomisp/i2c/atomisp-gc0310.c        | 56 +++++++++----------
 1 file changed, 28 insertions(+), 28 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index 9532114d6139..152835fa4226 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -431,11 +431,7 @@ static int gc0310_detect(struct gc0310_device *sensor)
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
 		return -ENODEV;
 
-	ret = pm_runtime_get_sync(&client->dev);
-	if (ret >= 0)
-		ret = cci_read(sensor->regmap, GC0310_SC_CMMN_CHIP_ID_REG,
-			       &val, NULL);
-	pm_runtime_put(&client->dev);
+	ret = cci_read(sensor->regmap, GC0310_SC_CMMN_CHIP_ID_REG, &val, NULL);
 	if (ret < 0) {
 		dev_err(&client->dev, "read sensor_id failed: %d\n", ret);
 		return -ENODEV;
@@ -672,6 +668,10 @@ static void gc0310_remove(struct i2c_client *client)
 	media_entity_cleanup(&sensor->sd.entity);
 	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
 	pm_runtime_disable(&client->dev);
+	if (!pm_runtime_status_suspended(&client->dev)) {
+		gc0310_power_off(&client->dev);
+		pm_runtime_set_suspended(&client->dev);
+	}
 }
 
 static int gc0310_check_hwcfg(struct device *dev)
@@ -759,16 +759,15 @@ static int gc0310_probe(struct i2c_client *client)
 	if (IS_ERR(sensor->regmap))
 		return PTR_ERR(sensor->regmap);
 
-	pm_runtime_set_suspended(&client->dev);
+	gc0310_power_on(&client->dev);
+
+	pm_runtime_set_active(&client->dev);
+	pm_runtime_get_noresume(&client->dev);
 	pm_runtime_enable(&client->dev);
-	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
-	pm_runtime_use_autosuspend(&client->dev);
 
 	ret = gc0310_detect(sensor);
-	if (ret) {
-		gc0310_remove(client);
-		return ret;
-	}
+	if (ret)
+		goto err_powerdown;
 
 	sensor->sd.internal_ops = &gc0310_internal_ops;
 	sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
@@ -776,31 +775,32 @@ static int gc0310_probe(struct i2c_client *client)
 	sensor->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
 
 	ret = gc0310_init_controls(sensor);
-	if (ret) {
-		gc0310_remove(client);
-		return ret;
-	}
+	if (ret)
+		goto err_powerdown;
 
 	ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
-	if (ret) {
-		gc0310_remove(client);
-		return ret;
-	}
+	if (ret)
+		goto err_powerdown;
 
 	sensor->sd.state_lock = sensor->ctrls.handler.lock;
 	ret = v4l2_subdev_init_finalize(&sensor->sd);
-	if (ret) {
-		gc0310_remove(client);
-		return ret;
-	}
+	if (ret)
+		goto err_powerdown;
 
 	ret = v4l2_async_register_subdev_sensor(&sensor->sd);
-	if (ret) {
-		gc0310_remove(client);
-		return ret;
-	}
+	if (ret)
+		goto err_powerdown;
+
+	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
+	pm_runtime_use_autosuspend(&client->dev);
+	pm_runtime_put_autosuspend(&client->dev);
 
 	return 0;
+
+err_powerdown:
+	pm_runtime_put_noidle(&client->dev);
+	gc0310_remove(client);
+	return ret;
 }
 
 static DEFINE_RUNTIME_DEV_PM_OPS(gc0310_pm_ops, gc0310_power_off,
-- 
2.49.0

[PATCH 21/23] media: atomisp: gc0310: Drop gc0310_get_frame_interval()

[Hans de Goede]

On raw camera sensors the framerate is controlled through vblank
(and optional) hblank controls.

Having a get_frame_interval makes no sense in this case, drop it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/i2c/atomisp-gc0310.c | 18 ------------------
 1 file changed, 18 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index 152835fa4226..73779c20ca68 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -513,23 +513,6 @@ static int gc0310_disable_streams(struct v4l2_subdev *sd,
 	return ret;
 }
 
-static int gc0310_get_frame_interval(struct v4l2_subdev *sd,
-				     struct v4l2_subdev_state *sd_state,
-				     struct v4l2_subdev_frame_interval *interval)
-{
-	/*
-	 * FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY, using the V4L2
-	 * subdev active state API.
-	 */
-	if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
-		return -EINVAL;
-
-	interval->interval.numerator = 1;
-	interval->interval.denominator = GC0310_FPS;
-
-	return 0;
-}
-
 static int gc0310_enum_mbus_code(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_mbus_code_enum *code)
@@ -579,7 +562,6 @@ static const struct v4l2_subdev_pad_ops gc0310_pad_ops = {
 	.set_fmt = v4l2_subdev_get_fmt, /* Only 1 fixed mode supported */
 	.get_selection = gc0310_get_selection,
 	.set_selection = gc0310_set_selection,
-	.get_frame_interval = gc0310_get_frame_interval,
 	.enable_streams = gc0310_enable_streams,
 	.disable_streams = gc0310_disable_streams,
 };
-- 
2.49.0

Re: [PATCH 21/23] media: atomisp: gc0310: Drop gc0310_get_frame_interval()

[Kieran Bingham]

Quoting Hans de Goede (2025-05-17 12:41:04)
> On raw camera sensors the framerate is controlled through vblank
> (and optional) hblank controls.
> 
> Having a get_frame_interval makes no sense in this case, drop it.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

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

> ---
>  .../staging/media/atomisp/i2c/atomisp-gc0310.c | 18 ------------------
>  1 file changed, 18 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> index 152835fa4226..73779c20ca68 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> @@ -513,23 +513,6 @@ static int gc0310_disable_streams(struct v4l2_subdev *sd,
>         return ret;
>  }
>  
> -static int gc0310_get_frame_interval(struct v4l2_subdev *sd,
> -                                    struct v4l2_subdev_state *sd_state,
> -                                    struct v4l2_subdev_frame_interval *interval)
> -{
> -       /*
> -        * FIXME: Implement support for V4L2_SUBDEV_FORMAT_TRY, using the V4L2
> -        * subdev active state API.
> -        */
> -       if (interval->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> -               return -EINVAL;
> -
> -       interval->interval.numerator = 1;
> -       interval->interval.denominator = GC0310_FPS;
> -
> -       return 0;
> -}
> -
>  static int gc0310_enum_mbus_code(struct v4l2_subdev *sd,
>                                  struct v4l2_subdev_state *sd_state,
>                                  struct v4l2_subdev_mbus_code_enum *code)
> @@ -579,7 +562,6 @@ static const struct v4l2_subdev_pad_ops gc0310_pad_ops = {
>         .set_fmt = v4l2_subdev_get_fmt, /* Only 1 fixed mode supported */
>         .get_selection = gc0310_get_selection,
>         .set_selection = gc0310_set_selection,
> -       .get_frame_interval = gc0310_get_frame_interval,
>         .enable_streams = gc0310_enable_streams,
>         .disable_streams = gc0310_disable_streams,
>  };
> -- 
> 2.49.0
>

[PATCH 22/23] media: atomisp: gc0310: Drop gc0310_g_skip_frames()

[Hans de Goede]

The g_skip_frames sensor-op is obsolete, drop it.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
index 73779c20ca68..edcad228272a 100644
--- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
+++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
@@ -541,16 +541,6 @@ static int gc0310_enum_frame_size(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static int gc0310_g_skip_frames(struct v4l2_subdev *sd, u32 *frames)
-{
-	*frames = GC0310_SKIP_FRAMES;
-	return 0;
-}
-
-static const struct v4l2_subdev_sensor_ops gc0310_sensor_ops = {
-	.g_skip_frames	= gc0310_g_skip_frames,
-};
-
 static const struct v4l2_subdev_video_ops gc0310_video_ops = {
 	.s_stream = v4l2_subdev_s_stream_helper,
 };
@@ -569,7 +559,6 @@ static const struct v4l2_subdev_pad_ops gc0310_pad_ops = {
 static const struct v4l2_subdev_ops gc0310_ops = {
 	.video = &gc0310_video_ops,
 	.pad = &gc0310_pad_ops,
-	.sensor = &gc0310_sensor_ops,
 };
 
 static int gc0310_init_state(struct v4l2_subdev *sd,
-- 
2.49.0

Re: [PATCH 22/23] media: atomisp: gc0310: Drop gc0310_g_skip_frames()

[Kieran Bingham]

Hi Hans,

Great looking series!
Makes me want to spend time cleaning up the OV5675 that I have here on
this laptop ... Oh to have more time ...


Quoting Hans de Goede (2025-05-17 12:41:05)
> The g_skip_frames sensor-op is obsolete, drop it.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 11 -----------
>  1 file changed, 11 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> index 73779c20ca68..edcad228272a 100644
> --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> @@ -541,16 +541,6 @@ static int gc0310_enum_frame_size(struct v4l2_subdev *sd,
>         return 0;
>  }
>  
> -static int gc0310_g_skip_frames(struct v4l2_subdev *sd, u32 *frames)
> -{
> -       *frames = GC0310_SKIP_FRAMES;

Skimming through - this seems to be the first comment I have ... :-)

I think GC0310_SKIP_FRAMES can be removed at this point.

We don't yet have a 'skip' frames parameter in libcamera sensor
properties - but especially after the libcamera workshop - we should add
the skip-frames (or other wise startup-frame named?) property as  '3' to
libcamera.

With that, 

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


> -       return 0;
> -}
> -
> -static const struct v4l2_subdev_sensor_ops gc0310_sensor_ops = {
> -       .g_skip_frames  = gc0310_g_skip_frames,
> -};
> -
>  static const struct v4l2_subdev_video_ops gc0310_video_ops = {
>         .s_stream = v4l2_subdev_s_stream_helper,
>  };
> @@ -569,7 +559,6 @@ static const struct v4l2_subdev_pad_ops gc0310_pad_ops = {
>  static const struct v4l2_subdev_ops gc0310_ops = {
>         .video = &gc0310_video_ops,
>         .pad = &gc0310_pad_ops,
> -       .sensor = &gc0310_sensor_ops,
>  };
>  
>  static int gc0310_init_state(struct v4l2_subdev *sd,
> -- 
> 2.49.0
>

[PATCH 23/23] media: Move gc0310 sensor drivers to drivers/media/i2c/

[Hans de Goede]

The atomisp gc0310 sensor driver has now been fully converted to
a standard v4l2 sensor driver. Move it to drivers/media/i2c/
to reflect this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 MAINTAINERS                                            |  6 ++++++
 drivers/media/i2c/Kconfig                              | 10 ++++++++++
 drivers/media/i2c/Makefile                             |  1 +
 .../i2c/atomisp-gc0310.c => media/i2c/gc0310.c}        |  0
 drivers/staging/media/atomisp/i2c/Kconfig              |  9 ---------
 drivers/staging/media/atomisp/i2c/Makefile             |  1 -
 6 files changed, 17 insertions(+), 10 deletions(-)
 rename drivers/{staging/media/atomisp/i2c/atomisp-gc0310.c => media/i2c/gc0310.c} (100%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 43a3d9afbb76..445120c55ad7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9800,6 +9800,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/media/i2c/galaxycore,gc0308.yaml
 F:	drivers/media/i2c/gc0308.c
 
+GALAXYCORE GC0310 CAMERA SENSOR DRIVER
+M:	Hans de Goede <hansg@kernel.org>
+L:	linux-media@vger.kernel.org
+S:	Maintained
+F:	drivers/media/i2c/gc0310.c
+
 GALAXYCORE GC05a2 CAMERA SENSOR DRIVER
 M:	Zhi Mao <zhi.mao@mediatek.com>
 L:	linux-media@vger.kernel.org
diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index b213cf27d4ea..bdfa41770090 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -70,6 +70,16 @@ config VIDEO_GC0308
 	  To compile this driver as a module, choose M here: the
 	  module will be called gc0308.
 
+config VIDEO_GC0310
+	tristate "GalaxyCore GC0310 sensor support"
+	select V4L2_CCI_I2C
+	help
+	  This is a Video4Linux2 sensor-level driver for the Galaxycore
+	  GC0310 0.3MP sensor.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called gc0310.
+
 config VIDEO_GC05A2
 	tristate "GalaxyCore gc05a2 sensor support"
 	select V4L2_CCI_I2C
diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
index 92c1c69b346b..01e9f7259289 100644
--- a/drivers/media/i2c/Makefile
+++ b/drivers/media/i2c/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_VIDEO_DW9768) += dw9768.o
 obj-$(CONFIG_VIDEO_DW9807_VCM) += dw9807-vcm.o
 obj-$(CONFIG_VIDEO_ET8EK8) += et8ek8/
 obj-$(CONFIG_VIDEO_GC0308) += gc0308.o
+obj-$(CONFIG_VIDEO_GC0310) += gc0310.o
 obj-$(CONFIG_VIDEO_GC05A2) += gc05a2.o
 obj-$(CONFIG_VIDEO_GC08A3) += gc08a3.o
 obj-$(CONFIG_VIDEO_GC2145) += gc2145.o
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/media/i2c/gc0310.c
similarity index 100%
rename from drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
rename to drivers/media/i2c/gc0310.c
diff --git a/drivers/staging/media/atomisp/i2c/Kconfig b/drivers/staging/media/atomisp/i2c/Kconfig
index ef2094c22347..7d447ba7a7e1 100644
--- a/drivers/staging/media/atomisp/i2c/Kconfig
+++ b/drivers/staging/media/atomisp/i2c/Kconfig
@@ -26,12 +26,3 @@ config VIDEO_ATOMISP_GC2235
 	  GC2235 is a 2M raw sensor.
 
 	  It currently only works with the atomisp driver.
-
-config VIDEO_ATOMISP_GC0310
-	tristate "GC0310 sensor support"
-	depends on ACPI
-	depends on I2C && VIDEO_DEV
-	select V4L2_CCI_I2C
-	help
-	  This is a Video4Linux2 sensor-level driver for the Galaxycore
-	  GC0310 0.3MP sensor.
diff --git a/drivers/staging/media/atomisp/i2c/Makefile b/drivers/staging/media/atomisp/i2c/Makefile
index e1637417e5c5..161aa542ef34 100644
--- a/drivers/staging/media/atomisp/i2c/Makefile
+++ b/drivers/staging/media/atomisp/i2c/Makefile
@@ -5,4 +5,3 @@
 
 obj-$(CONFIG_VIDEO_ATOMISP_GC2235)     += atomisp-gc2235.o
 obj-$(CONFIG_VIDEO_ATOMISP_OV2722)     += atomisp-ov2722.o
-obj-$(CONFIG_VIDEO_ATOMISP_GC0310)     += atomisp-gc0310.o
-- 
2.49.0

Re: [PATCH 23/23] media: Move gc0310 sensor drivers to drivers/media/i2c/

[Sakari Ailus]

Hi Hans,

Thanks for the set.

On Sat, May 17, 2025 at 01:41:06PM +0200, Hans de Goede wrote:
> The atomisp gc0310 sensor driver has now been fully converted to
> a standard v4l2 sensor driver. Move it to drivers/media/i2c/
> to reflect this.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  MAINTAINERS                                            |  6 ++++++
>  drivers/media/i2c/Kconfig                              | 10 ++++++++++
>  drivers/media/i2c/Makefile                             |  1 +
>  .../i2c/atomisp-gc0310.c => media/i2c/gc0310.c}        |  0

Looks good to me, with comments addressed. As this is a staging driver
before the last patch, I don't think the order of the patches matters that
much. As you'll be sending v2, could you also drop the debug print in
gc0310_remove() while at it?

-- 
Regards,

Sakari Ailus

Re: [PATCH 23/23] media: Move gc0310 sensor drivers to drivers/media/i2c/

[Dan Carpenter]

Hi Hans,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/media-atomisp-gc0310-Rename-dev-function-variable-to-sensor/20250517-194501
base:   git://linuxtv.org/sailus/media_tree.git master
patch link:    https://lore.kernel.org/r/20250517114106.43494-24-hdegoede%40redhat.com
patch subject: [PATCH 23/23] media: Move gc0310 sensor drivers to drivers/media/i2c/
config: loongarch-randconfig-r073-20250522 (https://download.01.org/0day-ci/archive/20250522/202505220651.U5WqBCdF-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 15.1.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202505220651.U5WqBCdF-lkp@intel.com/

New smatch warnings:
drivers/media/i2c/gc0310.c:462 gc0310_enable_streams() warn: pm_runtime_get_sync() also returns 1 on success

vim +462 drivers/media/i2c/gc0310.c

167e50747c4e3a drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2025-05-17  453  static int gc0310_enable_streams(struct v4l2_subdev *sd,
167e50747c4e3a drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2025-05-17  454  				 struct v4l2_subdev_state *state,
167e50747c4e3a drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2025-05-17  455  				 u32 pad, u64 streams_mask)
ad85094b293e40 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Mauro Carvalho Chehab 2020-04-19  456  {
1c468347b52136 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2025-05-17  457  	struct gc0310_device *sensor = to_gc0310_sensor(sd);
ad85094b293e40 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Mauro Carvalho Chehab 2020-04-19  458  	struct i2c_client *client = v4l2_get_subdevdata(sd);
167e50747c4e3a drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2025-05-17  459  	int ret;
ad85094b293e40 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Mauro Carvalho Chehab 2020-04-19  460  
2726c899fb6d57 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2023-02-05  461  	ret = pm_runtime_get_sync(&client->dev);
167e50747c4e3a drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2025-05-17 @462  	if (ret)

pm_runtime_get_sync() can return 1.  Use pm_runtime_resume_and_get()
instead.

2726c899fb6d57 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2023-02-05  463  		goto error_power_down;
b6763b2247ad43 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2023-02-05  464  
a285ed79b23e6f drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2025-05-17  465  	ret = regmap_multi_reg_write(sensor->regmap,
a285ed79b23e6f drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2025-05-17  466  				     gc0310_reset_register,
b6763b2247ad43 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2023-02-05  467  				     ARRAY_SIZE(gc0310_reset_register));
b6763b2247ad43 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2023-02-05  468  	if (ret)
b6763b2247ad43 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2023-02-05  469  		goto error_power_down;
b6763b2247ad43 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2023-02-05  470  
a285ed79b23e6f drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2025-05-17  471  	ret = regmap_multi_reg_write(sensor->regmap,
a285ed79b23e6f drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2025-05-17  472  				     gc0310_VGA_30fps,
b6763b2247ad43 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2023-02-05  473  				     ARRAY_SIZE(gc0310_VGA_30fps));
b6763b2247ad43 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2023-02-05  474  	if (ret)
b6763b2247ad43 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2023-02-05  475  		goto error_power_down;
b6763b2247ad43 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2023-02-05  476  
b6763b2247ad43 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2023-02-05  477  	/* restore value of all ctrls */
1c468347b52136 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2025-05-17  478  	ret = __v4l2_ctrl_handler_setup(&sensor->ctrls.handler);
b6763b2247ad43 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2023-02-05  479  
ad85094b293e40 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Mauro Carvalho Chehab 2020-04-19  480  	/* enable per frame MIPI and sensor ctrl reset  */
a285ed79b23e6f drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2025-05-17  481  	cci_write(sensor->regmap, GC0310_RESET_RELATED_REG, 0x30, &ret);
ad85094b293e40 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Mauro Carvalho Chehab 2020-04-19  482  
a285ed79b23e6f drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2025-05-17  483  	cci_write(sensor->regmap, GC0310_RESET_RELATED_REG,
a285ed79b23e6f drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2025-05-17  484  		  GC0310_REGISTER_PAGE_3, &ret);
a285ed79b23e6f drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2025-05-17  485  	cci_write(sensor->regmap, GC0310_SW_STREAM_REG,
167e50747c4e3a drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2025-05-17  486  		  GC0310_START_STREAMING, &ret);
a285ed79b23e6f drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2025-05-17  487  	cci_write(sensor->regmap, GC0310_RESET_RELATED_REG,
a285ed79b23e6f drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2025-05-17  488  		  GC0310_REGISTER_PAGE_0, &ret);
b6763b2247ad43 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2023-02-05  489  
167e50747c4e3a drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2025-05-17  490  error_power_down:
167e50747c4e3a drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2025-05-17  491  	if (ret)
2726c899fb6d57 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2023-02-05  492  		pm_runtime_put(&client->dev);
2b2297b11bb506 drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2023-02-05  493  
167e50747c4e3a drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2025-05-17  494  	return ret;
167e50747c4e3a drivers/staging/media/atomisp/i2c/atomisp-gc0310.c Hans de Goede         2025-05-17  495  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 23/23] media: Move gc0310 sensor drivers to drivers/media/i2c/

[Hans de Goede]

Hi Dan,
On 23-May-25 12:17 PM, Dan Carpenter wrote:
> Hi Hans,
> 
> kernel test robot noticed the following build warnings:

...

> New smatch warnings:
> drivers/media/i2c/gc0310.c:462 gc0310_enable_streams() warn: pm_runtime_get_sync() also returns 1 on success

Thank you for reporting this I've fixed this by switching to
pm_runtime_resume_and_get() as Andy suggested.

Regards,

Hans

Re: [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media

[Andy Shevchenko]

On Sat, May 17, 2025 at 01:40:43PM +0200, Hans de Goede wrote:
> Hi All,
> 
> Here is a patch series to bring the staging atomisp gc0310 driver inline
> with the expectations for new v4l2 raw camera sensor drivers and then
> after this move it to drivers/media/i2c since it is now just another
> normal modern v4l2 raw camera sensor driver.
> 
> I plan to send a pull-request for patches 1-22 to get added to
> media-commiters/next once they are reviewed. After that Sakari can review
> the final resulting gc0310.c file and merge the last patch moving it.

Reviewed-by: Andy Shevchenko <andy@kernel.org>

for non-commented by me or others (but if others okay with the changes,
I'm also okay).

Thanks for making this driver be aligned!

-- 
With Best Regards,
Andy Shevchenko