* [PATCH 01/23] media: atomisp: gc0310: Rename "dev" function variable to "sensor"
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
@ 2025-05-17 11:40 ` Hans de Goede
2025-05-17 20:42 ` Kieran Bingham
2025-05-17 11:40 ` [PATCH 02/23] media: atomisp: gc0310: Drop unused GC0310_FOCAL_LENGTH_NUM define Hans de Goede
` (22 subsequent siblings)
23 siblings, 1 reply; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:40 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH 01/23] media: atomisp: gc0310: Rename "dev" function variable to "sensor"
2025-05-17 11:40 ` [PATCH 01/23] media: atomisp: gc0310: Rename "dev" function variable to "sensor" Hans de Goede
@ 2025-05-17 20:42 ` Kieran Bingham
0 siblings, 0 replies; 56+ messages in thread
From: Kieran Bingham @ 2025-05-17 20:42 UTC (permalink / raw)
To: Andy Shevchenko, Hans de Goede, Sakari Ailus
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
>
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH 02/23] media: atomisp: gc0310: Drop unused GC0310_FOCAL_LENGTH_NUM define
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
2025-05-17 11:40 ` [PATCH 01/23] media: atomisp: gc0310: Rename "dev" function variable to "sensor" Hans de Goede
@ 2025-05-17 11:40 ` Hans de Goede
2025-05-17 20:43 ` Kieran Bingham
2025-05-17 11:40 ` [PATCH 03/23] media: atomisp: gc0310: Modify vblank value to run at 30 fps Hans de Goede
` (21 subsequent siblings)
23 siblings, 1 reply; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:40 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH 02/23] media: atomisp: gc0310: Drop unused GC0310_FOCAL_LENGTH_NUM define
2025-05-17 11:40 ` [PATCH 02/23] media: atomisp: gc0310: Drop unused GC0310_FOCAL_LENGTH_NUM define Hans de Goede
@ 2025-05-17 20:43 ` Kieran Bingham
0 siblings, 0 replies; 56+ messages in thread
From: Kieran Bingham @ 2025-05-17 20:43 UTC (permalink / raw)
To: Andy Shevchenko, Hans de Goede, Sakari Ailus
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
>
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH 03/23] media: atomisp: gc0310: Modify vblank value to run at 30 fps
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
2025-05-17 11:40 ` [PATCH 01/23] media: atomisp: gc0310: Rename "dev" function variable to "sensor" Hans de Goede
2025-05-17 11:40 ` [PATCH 02/23] media: atomisp: gc0310: Drop unused GC0310_FOCAL_LENGTH_NUM define Hans de Goede
@ 2025-05-17 11:40 ` Hans de Goede
2025-05-17 20:45 ` Kieran Bingham
2025-05-17 11:40 ` [PATCH 04/23] media: atomisp: gc0310: Switch to CCI register access helpers Hans de Goede
` (20 subsequent siblings)
23 siblings, 1 reply; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:40 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH 03/23] media: atomisp: gc0310: Modify vblank value to run at 30 fps
2025-05-17 11:40 ` [PATCH 03/23] media: atomisp: gc0310: Modify vblank value to run at 30 fps Hans de Goede
@ 2025-05-17 20:45 ` Kieran Bingham
0 siblings, 0 replies; 56+ messages in thread
From: Kieran Bingham @ 2025-05-17 20:45 UTC (permalink / raw)
To: Andy Shevchenko, Hans de Goede, Sakari Ailus
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
>
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH 04/23] media: atomisp: gc0310: Switch to CCI register access helpers
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
` (2 preceding siblings ...)
2025-05-17 11:40 ` [PATCH 03/23] media: atomisp: gc0310: Modify vblank value to run at 30 fps Hans de Goede
@ 2025-05-17 11:40 ` Hans de Goede
2025-05-19 11:09 ` Andy Shevchenko
2025-05-17 11:40 ` [PATCH 05/23] media: atomisp: gc0310: Use V4L2_CID_ANALOGUE_GAIN for gain control Hans de Goede
` (19 subsequent siblings)
23 siblings, 1 reply; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:40 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH 04/23] media: atomisp: gc0310: Switch to CCI register access helpers
2025-05-17 11:40 ` [PATCH 04/23] media: atomisp: gc0310: Switch to CCI register access helpers Hans de Goede
@ 2025-05-19 11:09 ` Andy Shevchenko
0 siblings, 0 replies; 56+ messages in thread
From: Andy Shevchenko @ 2025-05-19 11:09 UTC (permalink / raw)
To: Hans de Goede
Cc: Sakari Ailus, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH 05/23] media: atomisp: gc0310: Use V4L2_CID_ANALOGUE_GAIN for gain control
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
` (3 preceding siblings ...)
2025-05-17 11:40 ` [PATCH 04/23] media: atomisp: gc0310: Switch to CCI register access helpers Hans de Goede
@ 2025-05-17 11:40 ` Hans de Goede
2025-05-17 21:09 ` Kieran Bingham
2025-05-17 11:40 ` [PATCH 06/23] media: atomisp: gc0310: Add selection API support Hans de Goede
` (18 subsequent siblings)
23 siblings, 1 reply; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:40 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH 05/23] media: atomisp: gc0310: Use V4L2_CID_ANALOGUE_GAIN for gain control
2025-05-17 11:40 ` [PATCH 05/23] media: atomisp: gc0310: Use V4L2_CID_ANALOGUE_GAIN for gain control Hans de Goede
@ 2025-05-17 21:09 ` Kieran Bingham
2025-05-18 9:42 ` Kieran Bingham
2025-07-04 20:53 ` Hans de Goede
0 siblings, 2 replies; 56+ messages in thread
From: Kieran Bingham @ 2025-05-17 21:09 UTC (permalink / raw)
To: Andy Shevchenko, Hans de Goede, Sakari Ailus
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
>
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 05/23] media: atomisp: gc0310: Use V4L2_CID_ANALOGUE_GAIN for gain control
2025-05-17 21:09 ` Kieran Bingham
@ 2025-05-18 9:42 ` Kieran Bingham
2025-05-21 14:04 ` Laurent Pinchart
2025-07-04 20:53 ` Hans de Goede
1 sibling, 1 reply; 56+ messages in thread
From: Kieran Bingham @ 2025-05-18 9:42 UTC (permalink / raw)
To: Andy Shevchenko, Hans de Goede, Sakari Ailus
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging,
libcamera-devel
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
> >
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 05/23] media: atomisp: gc0310: Use V4L2_CID_ANALOGUE_GAIN for gain control
2025-05-18 9:42 ` Kieran Bingham
@ 2025-05-21 14:04 ` Laurent Pinchart
0 siblings, 0 replies; 56+ messages in thread
From: Laurent Pinchart @ 2025-05-21 14:04 UTC (permalink / raw)
To: Kieran Bingham
Cc: Andy Shevchenko, Hans de Goede, Sakari Ailus,
Mauro Carvalho Chehab, linux-media, linux-staging,
libcamera-devel
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
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 05/23] media: atomisp: gc0310: Use V4L2_CID_ANALOGUE_GAIN for gain control
2025-05-17 21:09 ` Kieran Bingham
2025-05-18 9:42 ` Kieran Bingham
@ 2025-07-04 20:53 ` Hans de Goede
1 sibling, 0 replies; 56+ messages in thread
From: Hans de Goede @ 2025-07-04 20:53 UTC (permalink / raw)
To: Kieran Bingham, Andy Shevchenko, Hans de Goede, Sakari Ailus
Cc: Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH 06/23] media: atomisp: gc0310: Add selection API support
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
` (4 preceding siblings ...)
2025-05-17 11:40 ` [PATCH 05/23] media: atomisp: gc0310: Use V4L2_CID_ANALOGUE_GAIN for gain control Hans de Goede
@ 2025-05-17 11:40 ` Hans de Goede
2025-05-17 20:41 ` Kieran Bingham
2025-05-17 11:40 ` [PATCH 07/23] media: atomisp: gc0310: Add link-frequency and pixelrate controls Hans de Goede
` (17 subsequent siblings)
23 siblings, 1 reply; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:40 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH 06/23] media: atomisp: gc0310: Add selection API support
2025-05-17 11:40 ` [PATCH 06/23] media: atomisp: gc0310: Add selection API support Hans de Goede
@ 2025-05-17 20:41 ` Kieran Bingham
2025-07-06 9:52 ` Hans de Goede
0 siblings, 1 reply; 56+ messages in thread
From: Kieran Bingham @ 2025-05-17 20:41 UTC (permalink / raw)
To: Andy Shevchenko, Hans de Goede, Sakari Ailus
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
>
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH 06/23] media: atomisp: gc0310: Add selection API support
2025-05-17 20:41 ` Kieran Bingham
@ 2025-07-06 9:52 ` Hans de Goede
0 siblings, 0 replies; 56+ messages in thread
From: Hans de Goede @ 2025-07-06 9:52 UTC (permalink / raw)
To: Kieran Bingham, Andy Shevchenko, Hans de Goede, Sakari Ailus
Cc: Mauro Carvalho Chehab, linux-media, linux-staging
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
>>
>
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH 07/23] media: atomisp: gc0310: Add link-frequency and pixelrate controls
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
` (5 preceding siblings ...)
2025-05-17 11:40 ` [PATCH 06/23] media: atomisp: gc0310: Add selection API support Hans de Goede
@ 2025-05-17 11:40 ` Hans de Goede
2025-05-19 11:30 ` Andy Shevchenko
2025-05-17 11:40 ` [PATCH 08/23] media: atomisp: gc0310: Add vblank and hblank controls Hans de Goede
` (16 subsequent siblings)
23 siblings, 1 reply; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:40 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH 07/23] media: atomisp: gc0310: Add link-frequency and pixelrate controls
2025-05-17 11:40 ` [PATCH 07/23] media: atomisp: gc0310: Add link-frequency and pixelrate controls Hans de Goede
@ 2025-05-19 11:30 ` Andy Shevchenko
2025-07-06 9:57 ` Hans de Goede
0 siblings, 1 reply; 56+ messages in thread
From: Andy Shevchenko @ 2025-05-19 11:30 UTC (permalink / raw)
To: Hans de Goede
Cc: Sakari Ailus, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 07/23] media: atomisp: gc0310: Add link-frequency and pixelrate controls
2025-05-19 11:30 ` Andy Shevchenko
@ 2025-07-06 9:57 ` Hans de Goede
2025-07-08 8:04 ` Sakari Ailus
0 siblings, 1 reply; 56+ messages in thread
From: Hans de Goede @ 2025-07-06 9:57 UTC (permalink / raw)
To: Andy Shevchenko, Hans de Goede
Cc: Sakari Ailus, Mauro Carvalho Chehab, linux-media, linux-staging
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.
>
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 07/23] media: atomisp: gc0310: Add link-frequency and pixelrate controls
2025-07-06 9:57 ` Hans de Goede
@ 2025-07-08 8:04 ` Sakari Ailus
2025-07-08 9:47 ` Hans de Goede
0 siblings, 1 reply; 56+ messages in thread
From: Sakari Ailus @ 2025-07-08 8:04 UTC (permalink / raw)
To: Hans de Goede
Cc: Andy Shevchenko, Hans de Goede, Mauro Carvalho Chehab,
linux-media, linux-staging
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
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 07/23] media: atomisp: gc0310: Add link-frequency and pixelrate controls
2025-07-08 8:04 ` Sakari Ailus
@ 2025-07-08 9:47 ` Hans de Goede
0 siblings, 0 replies; 56+ messages in thread
From: Hans de Goede @ 2025-07-08 9:47 UTC (permalink / raw)
To: Sakari Ailus
Cc: Andy Shevchenko, Hans de Goede, Mauro Carvalho Chehab,
linux-media, linux-staging
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
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH 08/23] media: atomisp: gc0310: Add vblank and hblank controls
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
` (6 preceding siblings ...)
2025-05-17 11:40 ` [PATCH 07/23] media: atomisp: gc0310: Add link-frequency and pixelrate controls Hans de Goede
@ 2025-05-17 11:40 ` Hans de Goede
2025-05-19 11:32 ` Andy Shevchenko
2025-05-17 11:40 ` [PATCH 09/23] media: atomisp: gc0310: Add camera orientation and sensor rotation controls Hans de Goede
` (15 subsequent siblings)
23 siblings, 1 reply; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:40 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH 09/23] media: atomisp: gc0310: Add camera orientation and sensor rotation controls
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
` (7 preceding siblings ...)
2025-05-17 11:40 ` [PATCH 08/23] media: atomisp: gc0310: Add vblank and hblank controls Hans de Goede
@ 2025-05-17 11:40 ` Hans de Goede
2025-05-17 11:40 ` [PATCH 10/23] media: atomisp: gc0310: Limit max exposure value to mode-height + vblank Hans de Goede
` (14 subsequent siblings)
23 siblings, 0 replies; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:40 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH 10/23] media: atomisp: gc0310: Limit max exposure value to mode-height + vblank
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
` (8 preceding siblings ...)
2025-05-17 11:40 ` [PATCH 09/23] media: atomisp: gc0310: Add camera orientation and sensor rotation controls Hans de Goede
@ 2025-05-17 11:40 ` Hans de Goede
2025-05-17 11:40 ` [PATCH 11/23] media: atomisp: gc0310: Add check_hwcfg() function Hans de Goede
` (13 subsequent siblings)
23 siblings, 0 replies; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:40 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH 11/23] media: atomisp: gc0310: Add check_hwcfg() function
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
` (9 preceding siblings ...)
2025-05-17 11:40 ` [PATCH 10/23] media: atomisp: gc0310: Limit max exposure value to mode-height + vblank Hans de Goede
@ 2025-05-17 11:40 ` Hans de Goede
2025-05-19 11:35 ` Andy Shevchenko
2025-05-17 11:40 ` [PATCH 12/23] media: atomisp: gc0310: Fix power on/off sleep times Hans de Goede
` (12 subsequent siblings)
23 siblings, 1 reply; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:40 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH 11/23] media: atomisp: gc0310: Add check_hwcfg() function
2025-05-17 11:40 ` [PATCH 11/23] media: atomisp: gc0310: Add check_hwcfg() function Hans de Goede
@ 2025-05-19 11:35 ` Andy Shevchenko
2025-07-06 9:58 ` Hans de Goede
0 siblings, 1 reply; 56+ messages in thread
From: Andy Shevchenko @ 2025-05-19 11:35 UTC (permalink / raw)
To: Hans de Goede
Cc: Sakari Ailus, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH 11/23] media: atomisp: gc0310: Add check_hwcfg() function
2025-05-19 11:35 ` Andy Shevchenko
@ 2025-07-06 9:58 ` Hans de Goede
0 siblings, 0 replies; 56+ messages in thread
From: Hans de Goede @ 2025-07-06 9:58 UTC (permalink / raw)
To: Andy Shevchenko, Hans de Goede
Cc: Sakari Ailus, Mauro Carvalho Chehab, linux-media, linux-staging
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");
>> + }
>
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH 12/23] media: atomisp: gc0310: Fix power on/off sleep times
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
` (10 preceding siblings ...)
2025-05-17 11:40 ` [PATCH 11/23] media: atomisp: gc0310: Add check_hwcfg() function Hans de Goede
@ 2025-05-17 11:40 ` Hans de Goede
2025-05-19 11:40 ` Andy Shevchenko
2025-05-17 11:40 ` [PATCH 13/23] media: atomisp: gc0310: Remove unused is_streaming variable Hans de Goede
` (11 subsequent siblings)
23 siblings, 1 reply; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:40 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH 12/23] media: atomisp: gc0310: Fix power on/off sleep times
2025-05-17 11:40 ` [PATCH 12/23] media: atomisp: gc0310: Fix power on/off sleep times Hans de Goede
@ 2025-05-19 11:40 ` Andy Shevchenko
0 siblings, 0 replies; 56+ messages in thread
From: Andy Shevchenko @ 2025-05-19 11:40 UTC (permalink / raw)
To: Hans de Goede
Cc: Sakari Ailus, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH 13/23] media: atomisp: gc0310: Remove unused is_streaming variable
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
` (11 preceding siblings ...)
2025-05-17 11:40 ` [PATCH 12/23] media: atomisp: gc0310: Fix power on/off sleep times Hans de Goede
@ 2025-05-17 11:40 ` Hans de Goede
2025-05-17 11:40 ` [PATCH 14/23] media: atomisp: gc0310: Switch to {enable,disable}_streams Hans de Goede
` (10 subsequent siblings)
23 siblings, 0 replies; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:40 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH 14/23] media: atomisp: gc0310: Switch to {enable,disable}_streams
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
` (12 preceding siblings ...)
2025-05-17 11:40 ` [PATCH 13/23] media: atomisp: gc0310: Remove unused is_streaming variable Hans de Goede
@ 2025-05-17 11:40 ` Hans de Goede
2025-05-19 11:43 ` Andy Shevchenko
2025-05-17 11:40 ` [PATCH 15/23] media: atomisp: gc0310: Switch to using the sub-device state lock Hans de Goede
` (9 subsequent siblings)
23 siblings, 1 reply; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:40 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH 14/23] media: atomisp: gc0310: Switch to {enable,disable}_streams
2025-05-17 11:40 ` [PATCH 14/23] media: atomisp: gc0310: Switch to {enable,disable}_streams Hans de Goede
@ 2025-05-19 11:43 ` Andy Shevchenko
2025-07-06 13:45 ` Hans de Goede
0 siblings, 1 reply; 56+ messages in thread
From: Andy Shevchenko @ 2025-05-19 11:43 UTC (permalink / raw)
To: Hans de Goede
Cc: Sakari Ailus, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply [flat|nested] 56+ messages in thread* Re: [PATCH 14/23] media: atomisp: gc0310: Switch to {enable,disable}_streams
2025-05-19 11:43 ` Andy Shevchenko
@ 2025-07-06 13:45 ` Hans de Goede
0 siblings, 0 replies; 56+ messages in thread
From: Hans de Goede @ 2025-07-06 13:45 UTC (permalink / raw)
To: Andy Shevchenko, Hans de Goede
Cc: Sakari Ailus, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH 15/23] media: atomisp: gc0310: Switch to using the sub-device state lock
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
` (13 preceding siblings ...)
2025-05-17 11:40 ` [PATCH 14/23] media: atomisp: gc0310: Switch to {enable,disable}_streams Hans de Goede
@ 2025-05-17 11:40 ` Hans de Goede
2025-05-19 11:44 ` Andy Shevchenko
2025-05-17 11:40 ` [PATCH 16/23] media: atomisp: gc0310: Implement internal_ops.init_state Hans de Goede
` (8 subsequent siblings)
23 siblings, 1 reply; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:40 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH 15/23] media: atomisp: gc0310: Switch to using the sub-device state lock
2025-05-17 11:40 ` [PATCH 15/23] media: atomisp: gc0310: Switch to using the sub-device state lock Hans de Goede
@ 2025-05-19 11:44 ` Andy Shevchenko
2025-07-06 13:51 ` Hans de Goede
0 siblings, 1 reply; 56+ messages in thread
From: Andy Shevchenko @ 2025-05-19 11:44 UTC (permalink / raw)
To: Hans de Goede
Cc: Sakari Ailus, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 15/23] media: atomisp: gc0310: Switch to using the sub-device state lock
2025-05-19 11:44 ` Andy Shevchenko
@ 2025-07-06 13:51 ` Hans de Goede
0 siblings, 0 replies; 56+ messages in thread
From: Hans de Goede @ 2025-07-06 13:51 UTC (permalink / raw)
To: Andy Shevchenko, Hans de Goede
Cc: Sakari Ailus, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH 16/23] media: atomisp: gc0310: Implement internal_ops.init_state
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
` (14 preceding siblings ...)
2025-05-17 11:40 ` [PATCH 15/23] media: atomisp: gc0310: Switch to using the sub-device state lock Hans de Goede
@ 2025-05-17 11:40 ` Hans de Goede
2025-05-17 11:41 ` [PATCH 17/23] media: atomisp: gc0310: Use v4l2_subdev_get_fmt() as v4l2_subdev_pad_ops.get_fmt() Hans de Goede
` (7 subsequent siblings)
23 siblings, 0 replies; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:40 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH 17/23] media: atomisp: gc0310: Use v4l2_subdev_get_fmt() as v4l2_subdev_pad_ops.get_fmt()
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
` (15 preceding siblings ...)
2025-05-17 11:40 ` [PATCH 16/23] media: atomisp: gc0310: Implement internal_ops.init_state Hans de Goede
@ 2025-05-17 11:41 ` Hans de Goede
2025-05-17 11:41 ` [PATCH 18/23] media: atomisp: gc0310: Switch to using sd.active_state fmt Hans de Goede
` (6 subsequent siblings)
23 siblings, 0 replies; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:41 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH 18/23] media: atomisp: gc0310: Switch to using sd.active_state fmt
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
` (16 preceding siblings ...)
2025-05-17 11:41 ` [PATCH 17/23] media: atomisp: gc0310: Use v4l2_subdev_get_fmt() as v4l2_subdev_pad_ops.get_fmt() Hans de Goede
@ 2025-05-17 11:41 ` Hans de Goede
2025-05-17 11:41 ` [PATCH 19/23] media: atomisp: gc0310: Move and rename suspend/resume functions Hans de Goede
` (5 subsequent siblings)
23 siblings, 0 replies; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:41 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH 19/23] media: atomisp: gc0310: Move and rename suspend/resume functions
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
` (17 preceding siblings ...)
2025-05-17 11:41 ` [PATCH 18/23] media: atomisp: gc0310: Switch to using sd.active_state fmt Hans de Goede
@ 2025-05-17 11:41 ` Hans de Goede
2025-05-19 11:50 ` Andy Shevchenko
2025-05-17 11:41 ` [PATCH 20/23] media: atomisp: gc0310: runtime-PM fixes Hans de Goede
` (4 subsequent siblings)
23 siblings, 1 reply; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:41 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH 19/23] media: atomisp: gc0310: Move and rename suspend/resume functions
2025-05-17 11:41 ` [PATCH 19/23] media: atomisp: gc0310: Move and rename suspend/resume functions Hans de Goede
@ 2025-05-19 11:50 ` Andy Shevchenko
2025-07-06 14:01 ` Hans de Goede
0 siblings, 1 reply; 56+ messages in thread
From: Andy Shevchenko @ 2025-05-19 11:50 UTC (permalink / raw)
To: Hans de Goede
Cc: Sakari Ailus, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 19/23] media: atomisp: gc0310: Move and rename suspend/resume functions
2025-05-19 11:50 ` Andy Shevchenko
@ 2025-07-06 14:01 ` Hans de Goede
0 siblings, 0 replies; 56+ messages in thread
From: Hans de Goede @ 2025-07-06 14:01 UTC (permalink / raw)
To: Andy Shevchenko, Hans de Goede
Cc: Sakari Ailus, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH 20/23] media: atomisp: gc0310: runtime-PM fixes
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
` (18 preceding siblings ...)
2025-05-17 11:41 ` [PATCH 19/23] media: atomisp: gc0310: Move and rename suspend/resume functions Hans de Goede
@ 2025-05-17 11:41 ` Hans de Goede
2025-05-17 11:41 ` [PATCH 21/23] media: atomisp: gc0310: Drop gc0310_get_frame_interval() Hans de Goede
` (3 subsequent siblings)
23 siblings, 0 replies; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:41 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* [PATCH 21/23] media: atomisp: gc0310: Drop gc0310_get_frame_interval()
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
` (19 preceding siblings ...)
2025-05-17 11:41 ` [PATCH 20/23] media: atomisp: gc0310: runtime-PM fixes Hans de Goede
@ 2025-05-17 11:41 ` Hans de Goede
2025-05-18 9:44 ` Kieran Bingham
2025-05-17 11:41 ` [PATCH 22/23] media: atomisp: gc0310: Drop gc0310_g_skip_frames() Hans de Goede
` (2 subsequent siblings)
23 siblings, 1 reply; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:41 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH 21/23] media: atomisp: gc0310: Drop gc0310_get_frame_interval()
2025-05-17 11:41 ` [PATCH 21/23] media: atomisp: gc0310: Drop gc0310_get_frame_interval() Hans de Goede
@ 2025-05-18 9:44 ` Kieran Bingham
0 siblings, 0 replies; 56+ messages in thread
From: Kieran Bingham @ 2025-05-18 9:44 UTC (permalink / raw)
To: Andy Shevchenko, Hans de Goede, Sakari Ailus
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
>
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH 22/23] media: atomisp: gc0310: Drop gc0310_g_skip_frames()
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
` (20 preceding siblings ...)
2025-05-17 11:41 ` [PATCH 21/23] media: atomisp: gc0310: Drop gc0310_get_frame_interval() Hans de Goede
@ 2025-05-17 11:41 ` Hans de Goede
2025-05-17 14:12 ` Kieran Bingham
2025-05-17 11:41 ` [PATCH 23/23] media: Move gc0310 sensor drivers to drivers/media/i2c/ Hans de Goede
2025-05-19 11:54 ` [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Andy Shevchenko
23 siblings, 1 reply; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:41 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH 22/23] media: atomisp: gc0310: Drop gc0310_g_skip_frames()
2025-05-17 11:41 ` [PATCH 22/23] media: atomisp: gc0310: Drop gc0310_g_skip_frames() Hans de Goede
@ 2025-05-17 14:12 ` Kieran Bingham
0 siblings, 0 replies; 56+ messages in thread
From: Kieran Bingham @ 2025-05-17 14:12 UTC (permalink / raw)
To: Andy Shevchenko, Hans de Goede, Sakari Ailus
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
>
^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH 23/23] media: Move gc0310 sensor drivers to drivers/media/i2c/
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
` (21 preceding siblings ...)
2025-05-17 11:41 ` [PATCH 22/23] media: atomisp: gc0310: Drop gc0310_g_skip_frames() Hans de Goede
@ 2025-05-17 11:41 ` Hans de Goede
2025-05-19 12:19 ` Sakari Ailus
2025-05-19 11:54 ` [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Andy Shevchenko
23 siblings, 1 reply; 56+ messages in thread
From: Hans de Goede @ 2025-05-17 11:41 UTC (permalink / raw)
To: Sakari Ailus, Andy Shevchenko
Cc: Hans de Goede, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply related [flat|nested] 56+ messages in thread* Re: [PATCH 23/23] media: Move gc0310 sensor drivers to drivers/media/i2c/
2025-05-17 11:41 ` [PATCH 23/23] media: Move gc0310 sensor drivers to drivers/media/i2c/ Hans de Goede
@ 2025-05-19 12:19 ` Sakari Ailus
0 siblings, 0 replies; 56+ messages in thread
From: Sakari Ailus @ 2025-05-19 12:19 UTC (permalink / raw)
To: Hans de Goede
Cc: Sakari Ailus, Andy Shevchenko, Mauro Carvalho Chehab, linux-media,
linux-staging
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
^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media
2025-05-17 11:40 [PATCH 00/23] media: atomisp: gc0310: Modernize and move to drivers/media Hans de Goede
` (22 preceding siblings ...)
2025-05-17 11:41 ` [PATCH 23/23] media: Move gc0310 sensor drivers to drivers/media/i2c/ Hans de Goede
@ 2025-05-19 11:54 ` Andy Shevchenko
23 siblings, 0 replies; 56+ messages in thread
From: Andy Shevchenko @ 2025-05-19 11:54 UTC (permalink / raw)
To: Hans de Goede
Cc: Sakari Ailus, Mauro Carvalho Chehab, linux-media, linux-staging
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
^ permalink raw reply [flat|nested] 56+ messages in thread