[PATCH 0/3] media: i2c: gc0310: cleanups and sensor clock handling improvements

[PATCH 0/3] media: i2c: gc0310: cleanups and sensor clock handling improvements

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

Hi all,

This patch series improves resource cleanup, error handling, and clock
management in the gc0310 camera sensor driver.

The changes focus on making the probe path more robust and easier to
reason about by fixing error unwind paths, reducing repeated access to
core structures, and adopting the common V4L2 sensor clock helper to
avoid manual clock handling in the driver.

Key highlights of this series:
- Fix probe error handling to ensure resources are properly released on
  failure paths.
- Use cached I2C client and device pointers consistently across the
  driver for improved readability and maintainability.
- Switch to devm_v4l2_sensor_clk_get() to standardise external clock
  handling and align with modern V4L2 sensor driver expectations.

No functional behavior changes are intended.

Testing:
  - Compiled with W=1
  - Build-tested on QEMU x86_64

Based on:
  <linux-v7.0-rc5>

Feedback and reviews are very welcome.

Thanks,
Sanjay Chitroda


Sanjay Chitroda (3):
  media: i2c: gc0310: fix probe error handling and unwind resources
    properly
  media: i2c: gc0310: use cached client and device pointers
  media: i2c: gc0310: Use devm_v4l2_sensor_clk_get()

 drivers/media/i2c/gc0310.c | 128 +++++++++++++++++++++----------------
 1 file changed, 73 insertions(+), 55 deletions(-)

-- 
2.34.1

[PATCH 1/3] media: i2c: gc0310: fix probe error handling and unwind resources properly

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

The GC0310 probe path currently performs error cleanup by jumping to a
common label that mirrors the driver's remove() callback. This is unsafe,
as remove() assumes that the subdevice has been fully registered with
the V4L2 framework, media and control resources have been initialized.

Calling remove() from probe can result in unregistering or cleaning up
subdevice, leading to resource leaks and subtle lifecycle bugs.

Rewrite the probe() error handling to unwind resources explicitly, using
fine‑grained goto labels along with appropriate error logs. Each failure
path now frees only successfully acquired resource, without remove().

This aligns the driver with standard V4L2 sensor lifecycle expectations
and avoids incorrect teardown during probe failures.

Fixes: 1e72afb5146a ("media: Move gc0310 sensor drivers to drivers/media/i2c/")
Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
---
 drivers/media/i2c/gc0310.c | 39 ++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/media/i2c/gc0310.c b/drivers/media/i2c/gc0310.c
index 7af4d66f42a0..72a82ad4118a 100644
--- a/drivers/media/i2c/gc0310.c
+++ b/drivers/media/i2c/gc0310.c
@@ -721,7 +721,7 @@ static int gc0310_probe(struct i2c_client *client)
 
 	ret = gc0310_detect(sensor);
 	if (ret)
-		goto err_power_down;
+		goto error_power_off;
 
 	sensor->sd.internal_ops = &gc0310_internal_ops;
 	sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
@@ -730,20 +730,27 @@ static int gc0310_probe(struct i2c_client *client)
 
 	ret = gc0310_init_controls(sensor);
 	if (ret)
-		goto err_power_down;
+		goto error_power_off;
 
 	ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
-	if (ret)
-		goto err_power_down;
+	if (ret) {
+		dev_err_probe(&client->dev, ret, "failed to init entity pads\n");
+		goto error_handler_free;
+	}
 
 	sensor->sd.state_lock = sensor->ctrls.handler.lock;
 	ret = v4l2_subdev_init_finalize(&sensor->sd);
-	if (ret)
-		goto err_power_down;
+	if (ret) {
+		dev_err_probe(&client->dev, ret, "subdev init error\n");
+		goto error_media_entity;
+	}
 
 	ret = v4l2_async_register_subdev_sensor(&sensor->sd);
-	if (ret)
-		goto err_power_down;
+	if (ret) {
+		dev_err_probe(&client->dev, ret,
+			      "failed to register gc0310 sub-device\n");
+		goto error_subdev_cleanup;
+	}
 
 	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
 	pm_runtime_use_autosuspend(&client->dev);
@@ -751,9 +758,21 @@ static int gc0310_probe(struct i2c_client *client)
 
 	return 0;
 
-err_power_down:
+error_subdev_cleanup:
+	v4l2_subdev_cleanup(&sensor->sd);
+	pm_runtime_disable(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+
+error_media_entity:
+	media_entity_cleanup(&sensor->sd.entity);
+
+error_handler_free:
+	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
+
+error_power_off:
 	pm_runtime_put_noidle(&client->dev);
-	gc0310_remove(client);
+	gc0310_power_off(&client->dev);
+
 	return ret;
 }
 
-- 
2.34.1

Re: [PATCH 1/3] media: i2c: gc0310: fix probe error handling and unwind resources properly

[Hans de Goede]

Hi,

On 1-Apr-26 20:16, Sanjay Chitroda wrote:
> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> 
> The GC0310 probe path currently performs error cleanup by jumping to a
> common label that mirrors the driver's remove() callback. This is unsafe,
> as remove() assumes that the subdevice has been fully registered with
> the V4L2 framework, media and control resources have been initialized.

That is simply not true, all functions called in remove() internally
check if their init counter-part has succeeded and if not are a no-op.

If you're aware of any specific calls in remove() where this is not
the case, please explicitly describe these cases and describe an
example exit-error path from probe() where things actually go wrong.

> Calling remove() from probe can result in unregistering or cleaning up
> subdevice, leading to resource leaks and subtle lifecycle bugs.
> 
> Rewrite the probe() error handling to unwind resources explicitly, using
> fine‑grained goto labels along with appropriate error logs. Each failure
> path now frees only successfully acquired resource, without remove().
> 
> This aligns the driver with standard V4L2 sensor lifecycle expectations
> and avoids incorrect teardown during probe failures.

The rest of this reads very much like this was AI generated.

Did you use AI to generate these patches ? If so please:

Make sure you actually understand what the patch is doing and
very yourself that it actually is correct, which in this case
I believe it is not.

Regards,

Hans


> 
> Fixes: 1e72afb5146a ("media: Move gc0310 sensor drivers to drivers/media/i2c/")
> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> ---
>  drivers/media/i2c/gc0310.c | 39 ++++++++++++++++++++++++++++----------
>  1 file changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/i2c/gc0310.c b/drivers/media/i2c/gc0310.c
> index 7af4d66f42a0..72a82ad4118a 100644
> --- a/drivers/media/i2c/gc0310.c
> +++ b/drivers/media/i2c/gc0310.c
> @@ -721,7 +721,7 @@ static int gc0310_probe(struct i2c_client *client)
>  
>  	ret = gc0310_detect(sensor);
>  	if (ret)
> -		goto err_power_down;
> +		goto error_power_off;
>  
>  	sensor->sd.internal_ops = &gc0310_internal_ops;
>  	sensor->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> @@ -730,20 +730,27 @@ static int gc0310_probe(struct i2c_client *client)
>  
>  	ret = gc0310_init_controls(sensor);
>  	if (ret)
> -		goto err_power_down;
> +		goto error_power_off;
>  
>  	ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
> -	if (ret)
> -		goto err_power_down;
> +	if (ret) {
> +		dev_err_probe(&client->dev, ret, "failed to init entity pads\n");
> +		goto error_handler_free;
> +	}
>  
>  	sensor->sd.state_lock = sensor->ctrls.handler.lock;
>  	ret = v4l2_subdev_init_finalize(&sensor->sd);
> -	if (ret)
> -		goto err_power_down;
> +	if (ret) {
> +		dev_err_probe(&client->dev, ret, "subdev init error\n");
> +		goto error_media_entity;
> +	}
>  
>  	ret = v4l2_async_register_subdev_sensor(&sensor->sd);
> -	if (ret)
> -		goto err_power_down;
> +	if (ret) {
> +		dev_err_probe(&client->dev, ret,
> +			      "failed to register gc0310 sub-device\n");
> +		goto error_subdev_cleanup;
> +	}
>  
>  	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
>  	pm_runtime_use_autosuspend(&client->dev);
> @@ -751,9 +758,21 @@ static int gc0310_probe(struct i2c_client *client)
>  
>  	return 0;
>  
> -err_power_down:
> +error_subdev_cleanup:
> +	v4l2_subdev_cleanup(&sensor->sd);
> +	pm_runtime_disable(&client->dev);
> +	pm_runtime_set_suspended(&client->dev);
> +
> +error_media_entity:
> +	media_entity_cleanup(&sensor->sd.entity);
> +
> +error_handler_free:
> +	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
> +
> +error_power_off:
>  	pm_runtime_put_noidle(&client->dev);
> -	gc0310_remove(client);
> +	gc0310_power_off(&client->dev);
> +
>  	return ret;
>  }
>  

Re: [PATCH 1/3] media: i2c: gc0310: fix probe error handling and unwind resources properly

[Sanjay Chitroda]

On 2 April 2026 12:36:33 am IST, Hans de Goede <hansg@kernel.org> wrote:
>Hi,
>
>On 1-Apr-26 20:16, Sanjay Chitroda wrote:
>> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
>> 
>> The GC0310 probe path currently performs error cleanup by jumping to a
>> common label that mirrors the driver's remove() callback. This is unsafe,
>> as remove() assumes that the subdevice has been fully registered with
>> the V4L2 framework, media and control resources have been initialized.
>
>That is simply not true, all functions called in remove() internally
>check if their init counter-part has succeeded and if not are a no-op.
>
>If you're aware of any specific calls in remove() where this is not
>the case, please explicitly describe these cases and describe an
>example exit-error path from probe() where things actually go wrong.
>

Hi Hans,

Thanks for the clarification - agreed, the remove helpers are defensively implemented and the existing code is not incorrect for a functional point. I should not have stated gc0310_remov() from a probe failure is unsafe.

>> Calling remove() from probe can result in unregistering or cleaning up
>> subdevice, leading to resource leaks and subtle lifecycle bugs.
>> 
>> Rewrite the probe() error handling to unwind resources explicitly, using
>> fine‑grained goto labels along with appropriate error logs. Each failure
>> path now frees only successfully acquired resource, without remove().
>> 
>> This aligns the driver with standard V4L2 sensor lifecycle expectations
>> and avoids incorrect teardown during probe failures.
>
>The rest of this reads very much like this was AI generated.
>
>Did you use AI to generate these patches ? If so please:
>
>Make sure you actually understand what the patch is doing and
>very yourself that it actually is correct, which in this case
>I believe it is not.
>
>Regards,
>
>Hans
>

Yes, I did use AI assistance to help draft the commit message, but the patch logic itself was written and reviewed by me. However, your feedback makes it clear that i did not sufficiently validate internals of existing remove() based cleanup.


I would like to propose commit message that align with change and existing kernel internals:

-------
media: i2c: gc0310: make probe error unwinding explicit

The gc0310 probe path unwinds failures by jumping to a single label remove-style cleanup.

Refactor the probe error handling so that resources are unwound explicitly and in reverse order of initialization using fine-grained goto labels.

This improves clarity and maintains symmetry with the probe initialization path.

No functional change intended.
-------

Kindly share your input on the same, according I will plan to resend v2 with an updated commit message as above.

>

Re: [PATCH 1/3] media: i2c: gc0310: fix probe error handling and unwind resources properly

[Hans de Goede]

Hi Sanjay,

On 5-Apr-26 12:16, Sanjay Chitroda wrote:
> 
> 
> On 2 April 2026 12:36:33 am IST, Hans de Goede <hansg@kernel.org> wrote:
>> Hi,
>>
>> On 1-Apr-26 20:16, Sanjay Chitroda wrote:
>>> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
>>>
>>> The GC0310 probe path currently performs error cleanup by jumping to a
>>> common label that mirrors the driver's remove() callback. This is unsafe,
>>> as remove() assumes that the subdevice has been fully registered with
>>> the V4L2 framework, media and control resources have been initialized.
>>
>> That is simply not true, all functions called in remove() internally
>> check if their init counter-part has succeeded and if not are a no-op.
>>
>> If you're aware of any specific calls in remove() where this is not
>> the case, please explicitly describe these cases and describe an
>> example exit-error path from probe() where things actually go wrong.
>>
> 
> Hi Hans,
> 
> Thanks for the clarification - agreed, the remove helpers are defensively implemented and the existing code is not incorrect for a functional point. I should not have stated gc0310_remov() from a probe failure is unsafe.
> 
>>> Calling remove() from probe can result in unregistering or cleaning up
>>> subdevice, leading to resource leaks and subtle lifecycle bugs.
>>>
>>> Rewrite the probe() error handling to unwind resources explicitly, using
>>> fine‑grained goto labels along with appropriate error logs. Each failure
>>> path now frees only successfully acquired resource, without remove().
>>>
>>> This aligns the driver with standard V4L2 sensor lifecycle expectations
>>> and avoids incorrect teardown during probe failures.
>>
>> The rest of this reads very much like this was AI generated.
>>
>> Did you use AI to generate these patches ? If so please:
>>
>> Make sure you actually understand what the patch is doing and
>> very yourself that it actually is correct, which in this case
>> I believe it is not.
>>
>> Regards,
>>
>> Hans
>>
> 
> Yes, I did use AI assistance to help draft the commit message, but the patch logic itself was written and reviewed by me. However, your feedback makes it clear that i did not sufficiently validate internals of existing remove() based cleanup.
> 
> 
> I would like to propose commit message that align with change and existing kernel internals:
> 
> -------
> media: i2c: gc0310: make probe error unwinding explicit
> 
> The gc0310 probe path unwinds failures by jumping to a single label remove-style cleanup.
> 
> Refactor the probe error handling so that resources are unwound explicitly and in reverse order of initialization using fine-grained goto labels.
> 
> This improves clarity and maintains symmetry with the probe initialization path.
> 
> No functional change intended.
> -------
> 
> Kindly share your input on the same, according I will plan to resend v2 with an updated commit message as above.

The problem is that your patch makes the code more complicated.

Since we know that gc0310_remove() is always safe to call
the simplest and cleanest code is to simply keep calling
gc0310_remove().

If you plan to do a v2 of this series please drop this patch.

Note I see no need for a v2. Patch 3/3 is good to have and
v1 of that can be merged. The other 2 patches should be dropped.

Regards,

Hans




> 
>>

[PATCH 2/3] media: i2c: gc0310: use cached client and device pointers

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

The driver was repeatedly retrieving the i2c_client using
v4l2_get_subdevdata() only to access the underlying struct device.

Replace this with the cached sensor->client and sensor->dev pointers,
which are already available in the sensor structure.

This simplifies the code, avoids redundant subdev lookups, and makes
the driver more consistent with common V4L2 sensor driver patterns.
No functional change intended.

Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
---
 drivers/media/i2c/gc0310.c | 72 ++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 35 deletions(-)

diff --git a/drivers/media/i2c/gc0310.c b/drivers/media/i2c/gc0310.c
index 72a82ad4118a..e538479fee2e 100644
--- a/drivers/media/i2c/gc0310.c
+++ b/drivers/media/i2c/gc0310.c
@@ -84,6 +84,9 @@
 #define to_gc0310_sensor(x) container_of(x, struct gc0310_device, sd)
 
 struct gc0310_device {
+	struct device *dev;
+	struct i2c_client *client;
+
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 
@@ -409,28 +412,27 @@ static int gc0310_power_on(struct device *dev)
 
 static int gc0310_detect(struct gc0310_device *sensor)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
 	u64 val;
 	int ret;
 
-	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+	if (!i2c_check_functionality(sensor->client->adapter, I2C_FUNC_I2C))
 		return -ENODEV;
 
 	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);
+		dev_err(sensor->dev, "read sensor_id failed: %d\n", ret);
 		return -ENODEV;
 	}
 
-	dev_dbg(&client->dev, "sensor ID = 0x%llx\n", val);
+	dev_dbg(sensor->dev, "sensor ID = 0x%llx\n", val);
 
 	if (val != GC0310_ID) {
-		dev_err(&client->dev, "sensor ID error, read id = 0x%llx, target id = 0x%x\n",
+		dev_err(sensor->dev, "sensor ID error, read id = 0x%llx, target id = 0x%x\n",
 			val, GC0310_ID);
 		return -ENODEV;
 	}
 
-	dev_dbg(&client->dev, "detect gc0310 success\n");
+	dev_dbg(sensor->dev, "detect gc0310 success\n");
 
 	return 0;
 }
@@ -440,10 +442,9 @@ static int gc0310_enable_streams(struct v4l2_subdev *sd,
 				 u32 pad, u64 streams_mask)
 {
 	struct gc0310_device *sensor = to_gc0310_sensor(sd);
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	int ret;
 
-	ret = pm_runtime_resume_and_get(&client->dev);
+	ret = pm_runtime_resume_and_get(sensor->dev);
 	if (ret)
 		return ret;
 
@@ -474,7 +475,7 @@ static int gc0310_enable_streams(struct v4l2_subdev *sd,
 
 error_power_down:
 	if (ret)
-		pm_runtime_put(&client->dev);
+		pm_runtime_put(sensor->dev);
 
 	return ret;
 }
@@ -484,7 +485,6 @@ static int gc0310_disable_streams(struct v4l2_subdev *sd,
 				  u32 pad, u64 streams_mask)
 {
 	struct gc0310_device *sensor = to_gc0310_sensor(sd);
-	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	int ret = 0;
 
 	cci_write(sensor->regmap, GC0310_RESET_RELATED_REG,
@@ -494,7 +494,7 @@ static int gc0310_disable_streams(struct v4l2_subdev *sd,
 	cci_write(sensor->regmap, GC0310_RESET_RELATED_REG,
 		  GC0310_REGISTER_PAGE_0, &ret);
 
-	pm_runtime_put(&client->dev);
+	pm_runtime_put(sensor->dev);
 	return ret;
 }
 
@@ -559,7 +559,6 @@ static const struct v4l2_subdev_internal_ops gc0310_internal_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 exp_max, ret;
@@ -597,7 +596,7 @@ 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);
+	ret = v4l2_fwnode_device_parse(sensor->dev, &props);
 	if (ret)
 		return ret;
 
@@ -621,10 +620,10 @@ static void gc0310_remove(struct i2c_client *client)
 	v4l2_subdev_cleanup(sd);
 	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);
+	pm_runtime_disable(sensor->dev);
+	if (!pm_runtime_status_suspended(sensor->dev)) {
+		gc0310_power_off(sensor->dev);
+		pm_runtime_set_suspended(sensor->dev);
 	}
 }
 
@@ -695,15 +694,18 @@ static int gc0310_probe(struct i2c_client *client)
 	if (!sensor)
 		return -ENOMEM;
 
-	sensor->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
+	sensor->client = client;
+	sensor->dev = &client->dev;
+
+	sensor->reset = devm_gpiod_get(sensor->dev, "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(sensor->reset)) {
-		return dev_err_probe(&client->dev, PTR_ERR(sensor->reset),
+		return dev_err_probe(sensor->dev, PTR_ERR(sensor->reset),
 				     "getting reset GPIO\n");
 	}
 
-	sensor->powerdown = devm_gpiod_get(&client->dev, "powerdown", GPIOD_OUT_HIGH);
+	sensor->powerdown = devm_gpiod_get(sensor->dev, "powerdown", GPIOD_OUT_HIGH);
 	if (IS_ERR(sensor->powerdown)) {
-		return dev_err_probe(&client->dev, PTR_ERR(sensor->powerdown),
+		return dev_err_probe(sensor->dev, PTR_ERR(sensor->powerdown),
 				     "getting powerdown GPIO\n");
 	}
 
@@ -713,11 +715,11 @@ static int gc0310_probe(struct i2c_client *client)
 	if (IS_ERR(sensor->regmap))
 		return PTR_ERR(sensor->regmap);
 
-	gc0310_power_on(&client->dev);
+	gc0310_power_on(sensor->dev);
 
-	pm_runtime_set_active(&client->dev);
-	pm_runtime_get_noresume(&client->dev);
-	pm_runtime_enable(&client->dev);
+	pm_runtime_set_active(sensor->dev);
+	pm_runtime_get_noresume(sensor->dev);
+	pm_runtime_enable(sensor->dev);
 
 	ret = gc0310_detect(sensor);
 	if (ret)
@@ -734,34 +736,34 @@ static int gc0310_probe(struct i2c_client *client)
 
 	ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
 	if (ret) {
-		dev_err_probe(&client->dev, ret, "failed to init entity pads\n");
+		dev_err_probe(sensor->dev, ret, "failed to init entity pads\n");
 		goto error_handler_free;
 	}
 
 	sensor->sd.state_lock = sensor->ctrls.handler.lock;
 	ret = v4l2_subdev_init_finalize(&sensor->sd);
 	if (ret) {
-		dev_err_probe(&client->dev, ret, "subdev init error\n");
+		dev_err_probe(sensor->dev, ret, "subdev init error\n");
 		goto error_media_entity;
 	}
 
 	ret = v4l2_async_register_subdev_sensor(&sensor->sd);
 	if (ret) {
-		dev_err_probe(&client->dev, ret,
+		dev_err_probe(sensor->dev, ret,
 			      "failed to register gc0310 sub-device\n");
 		goto error_subdev_cleanup;
 	}
 
-	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
-	pm_runtime_use_autosuspend(&client->dev);
-	pm_runtime_put_autosuspend(&client->dev);
+	pm_runtime_set_autosuspend_delay(sensor->dev, 1000);
+	pm_runtime_use_autosuspend(sensor->dev);
+	pm_runtime_put_autosuspend(sensor->dev);
 
 	return 0;
 
 error_subdev_cleanup:
 	v4l2_subdev_cleanup(&sensor->sd);
-	pm_runtime_disable(&client->dev);
-	pm_runtime_set_suspended(&client->dev);
+	pm_runtime_disable(sensor->dev);
+	pm_runtime_set_suspended(sensor->dev);
 
 error_media_entity:
 	media_entity_cleanup(&sensor->sd.entity);
@@ -770,8 +772,8 @@ static int gc0310_probe(struct i2c_client *client)
 	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
 
 error_power_off:
-	pm_runtime_put_noidle(&client->dev);
-	gc0310_power_off(&client->dev);
+	pm_runtime_put_noidle(sensor->dev);
+	gc0310_power_off(sensor->dev);
 
 	return ret;
 }
-- 
2.34.1

Re: [PATCH 2/3] media: i2c: gc0310: use cached client and device pointers

[Hans de Goede]

Hi,

On 1-Apr-26 20:16, Sanjay Chitroda wrote:
> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> 
> The driver was repeatedly retrieving the i2c_client using
> v4l2_get_subdevdata() only to access the underlying struct device.
> 
> Replace this with the cached sensor->client and sensor->dev pointers,
> which are already available in the sensor structure.
> 
> This simplifies the code, avoids redundant subdev lookups, and makes
> the driver more consistent with common V4L2 sensor driver patterns.
> No functional change intended.


So now you've grown struct gc0310_device by 16 bytes and
gained nothing much. And again this sound very much AI
generated.  Which is not necessarily a bad thing if the changes
are actually useful, but this seems like it is just code-churn
without any benefits.

Regards,

Hans



> 
> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> ---
>  drivers/media/i2c/gc0310.c | 72 ++++++++++++++++++++------------------
>  1 file changed, 37 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/media/i2c/gc0310.c b/drivers/media/i2c/gc0310.c
> index 72a82ad4118a..e538479fee2e 100644
> --- a/drivers/media/i2c/gc0310.c
> +++ b/drivers/media/i2c/gc0310.c
> @@ -84,6 +84,9 @@
>  #define to_gc0310_sensor(x) container_of(x, struct gc0310_device, sd)
>  
>  struct gc0310_device {
> +	struct device *dev;
> +	struct i2c_client *client;
> +
>  	struct v4l2_subdev sd;
>  	struct media_pad pad;
>  
> @@ -409,28 +412,27 @@ static int gc0310_power_on(struct device *dev)
>  
>  static int gc0310_detect(struct gc0310_device *sensor)
>  {
> -	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
>  	u64 val;
>  	int ret;
>  
> -	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> +	if (!i2c_check_functionality(sensor->client->adapter, I2C_FUNC_I2C))
>  		return -ENODEV;
>  
>  	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);
> +		dev_err(sensor->dev, "read sensor_id failed: %d\n", ret);
>  		return -ENODEV;
>  	}
>  
> -	dev_dbg(&client->dev, "sensor ID = 0x%llx\n", val);
> +	dev_dbg(sensor->dev, "sensor ID = 0x%llx\n", val);
>  
>  	if (val != GC0310_ID) {
> -		dev_err(&client->dev, "sensor ID error, read id = 0x%llx, target id = 0x%x\n",
> +		dev_err(sensor->dev, "sensor ID error, read id = 0x%llx, target id = 0x%x\n",
>  			val, GC0310_ID);
>  		return -ENODEV;
>  	}
>  
> -	dev_dbg(&client->dev, "detect gc0310 success\n");
> +	dev_dbg(sensor->dev, "detect gc0310 success\n");
>  
>  	return 0;
>  }
> @@ -440,10 +442,9 @@ static int gc0310_enable_streams(struct v4l2_subdev *sd,
>  				 u32 pad, u64 streams_mask)
>  {
>  	struct gc0310_device *sensor = to_gc0310_sensor(sd);
> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  	int ret;
>  
> -	ret = pm_runtime_resume_and_get(&client->dev);
> +	ret = pm_runtime_resume_and_get(sensor->dev);
>  	if (ret)
>  		return ret;
>  
> @@ -474,7 +475,7 @@ static int gc0310_enable_streams(struct v4l2_subdev *sd,
>  
>  error_power_down:
>  	if (ret)
> -		pm_runtime_put(&client->dev);
> +		pm_runtime_put(sensor->dev);
>  
>  	return ret;
>  }
> @@ -484,7 +485,6 @@ static int gc0310_disable_streams(struct v4l2_subdev *sd,
>  				  u32 pad, u64 streams_mask)
>  {
>  	struct gc0310_device *sensor = to_gc0310_sensor(sd);
> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
>  	int ret = 0;
>  
>  	cci_write(sensor->regmap, GC0310_RESET_RELATED_REG,
> @@ -494,7 +494,7 @@ static int gc0310_disable_streams(struct v4l2_subdev *sd,
>  	cci_write(sensor->regmap, GC0310_RESET_RELATED_REG,
>  		  GC0310_REGISTER_PAGE_0, &ret);
>  
> -	pm_runtime_put(&client->dev);
> +	pm_runtime_put(sensor->dev);
>  	return ret;
>  }
>  
> @@ -559,7 +559,6 @@ static const struct v4l2_subdev_internal_ops gc0310_internal_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 exp_max, ret;
> @@ -597,7 +596,7 @@ 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);
> +	ret = v4l2_fwnode_device_parse(sensor->dev, &props);
>  	if (ret)
>  		return ret;
>  
> @@ -621,10 +620,10 @@ static void gc0310_remove(struct i2c_client *client)
>  	v4l2_subdev_cleanup(sd);
>  	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);
> +	pm_runtime_disable(sensor->dev);
> +	if (!pm_runtime_status_suspended(sensor->dev)) {
> +		gc0310_power_off(sensor->dev);
> +		pm_runtime_set_suspended(sensor->dev);
>  	}
>  }
>  
> @@ -695,15 +694,18 @@ static int gc0310_probe(struct i2c_client *client)
>  	if (!sensor)
>  		return -ENOMEM;
>  
> -	sensor->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
> +	sensor->client = client;
> +	sensor->dev = &client->dev;
> +
> +	sensor->reset = devm_gpiod_get(sensor->dev, "reset", GPIOD_OUT_HIGH);
>  	if (IS_ERR(sensor->reset)) {
> -		return dev_err_probe(&client->dev, PTR_ERR(sensor->reset),
> +		return dev_err_probe(sensor->dev, PTR_ERR(sensor->reset),
>  				     "getting reset GPIO\n");
>  	}
>  
> -	sensor->powerdown = devm_gpiod_get(&client->dev, "powerdown", GPIOD_OUT_HIGH);
> +	sensor->powerdown = devm_gpiod_get(sensor->dev, "powerdown", GPIOD_OUT_HIGH);
>  	if (IS_ERR(sensor->powerdown)) {
> -		return dev_err_probe(&client->dev, PTR_ERR(sensor->powerdown),
> +		return dev_err_probe(sensor->dev, PTR_ERR(sensor->powerdown),
>  				     "getting powerdown GPIO\n");
>  	}
>  
> @@ -713,11 +715,11 @@ static int gc0310_probe(struct i2c_client *client)
>  	if (IS_ERR(sensor->regmap))
>  		return PTR_ERR(sensor->regmap);
>  
> -	gc0310_power_on(&client->dev);
> +	gc0310_power_on(sensor->dev);
>  
> -	pm_runtime_set_active(&client->dev);
> -	pm_runtime_get_noresume(&client->dev);
> -	pm_runtime_enable(&client->dev);
> +	pm_runtime_set_active(sensor->dev);
> +	pm_runtime_get_noresume(sensor->dev);
> +	pm_runtime_enable(sensor->dev);
>  
>  	ret = gc0310_detect(sensor);
>  	if (ret)
> @@ -734,34 +736,34 @@ static int gc0310_probe(struct i2c_client *client)
>  
>  	ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
>  	if (ret) {
> -		dev_err_probe(&client->dev, ret, "failed to init entity pads\n");
> +		dev_err_probe(sensor->dev, ret, "failed to init entity pads\n");
>  		goto error_handler_free;
>  	}
>  
>  	sensor->sd.state_lock = sensor->ctrls.handler.lock;
>  	ret = v4l2_subdev_init_finalize(&sensor->sd);
>  	if (ret) {
> -		dev_err_probe(&client->dev, ret, "subdev init error\n");
> +		dev_err_probe(sensor->dev, ret, "subdev init error\n");
>  		goto error_media_entity;
>  	}
>  
>  	ret = v4l2_async_register_subdev_sensor(&sensor->sd);
>  	if (ret) {
> -		dev_err_probe(&client->dev, ret,
> +		dev_err_probe(sensor->dev, ret,
>  			      "failed to register gc0310 sub-device\n");
>  		goto error_subdev_cleanup;
>  	}
>  
> -	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
> -	pm_runtime_use_autosuspend(&client->dev);
> -	pm_runtime_put_autosuspend(&client->dev);
> +	pm_runtime_set_autosuspend_delay(sensor->dev, 1000);
> +	pm_runtime_use_autosuspend(sensor->dev);
> +	pm_runtime_put_autosuspend(sensor->dev);
>  
>  	return 0;
>  
>  error_subdev_cleanup:
>  	v4l2_subdev_cleanup(&sensor->sd);
> -	pm_runtime_disable(&client->dev);
> -	pm_runtime_set_suspended(&client->dev);
> +	pm_runtime_disable(sensor->dev);
> +	pm_runtime_set_suspended(sensor->dev);
>  
>  error_media_entity:
>  	media_entity_cleanup(&sensor->sd.entity);
> @@ -770,8 +772,8 @@ static int gc0310_probe(struct i2c_client *client)
>  	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
>  
>  error_power_off:
> -	pm_runtime_put_noidle(&client->dev);
> -	gc0310_power_off(&client->dev);
> +	pm_runtime_put_noidle(sensor->dev);
> +	gc0310_power_off(sensor->dev);
>  
>  	return ret;
>  }

Re: [PATCH 2/3] media: i2c: gc0310: use cached client and device pointers

[Sanjay Chitroda]

On 2 April 2026 12:38:07 am IST, Hans de Goede <hansg@kernel.org> wrote:
>Hi,
>
>On 1-Apr-26 20:16, Sanjay Chitroda wrote:
>> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
>> 
>> The driver was repeatedly retrieving the i2c_client using
>> v4l2_get_subdevdata() only to access the underlying struct device.
>> 
>> Replace this with the cached sensor->client and sensor->dev pointers,
>> which are already available in the sensor structure.
>> 
>> This simplifies the code, avoids redundant subdev lookups, and makes
>> the driver more consistent with common V4L2 sensor driver patterns.
>> No functional change intended.
>
>
>So now you've grown struct gc0310_device by 16 bytes and
>gained nothing much. And again this sound very much AI
>generated.  Which is not necessarily a bad thing if the changes
>are actually useful, but this seems like it is just code-churn
>without any benefits.
>
>Regards,
>
>Hans
>

Thanks again for the review - I agree that, taken on its own, the change does not justify the added struct size and unnecessary churn for this driver.

For context, the motivation came from looking at recent sensor cleanups, for example Laurent's series: https://lore.kernel.org/all/20250812214620.30425-1-laurent.pinchart@ideasonboard.com/

where several sensor drivers were moved toward accessing the underlying device more directly instead of repeatedly going through subdev helpers. I attempted to explore a similar direction here.

That said, you're absolutely right that in the case of gc0310 this does not provide a concrete benefit, and the existing v412_get_subdevdata() usage is already clear and optimal.

I'll drop this patch instead of revising it.

Regards, Sanjay

>
>
>> 
>> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
>> ---
>>  drivers/media/i2c/gc0310.c | 72 ++++++++++++++++++++------------------
>>  1 file changed, 37 insertions(+), 35 deletions(-)
>> 
>> diff --git a/drivers/media/i2c/gc0310.c b/drivers/media/i2c/gc0310.c
>> index 72a82ad4118a..e538479fee2e 100644
>> --- a/drivers/media/i2c/gc0310.c
>> +++ b/drivers/media/i2c/gc0310.c
>> @@ -84,6 +84,9 @@
>>  #define to_gc0310_sensor(x) container_of(x, struct gc0310_device, sd)
>>  
>>  struct gc0310_device {
>> +	struct device *dev;
>> +	struct i2c_client *client;
>> +
>>  	struct v4l2_subdev sd;
>>  	struct media_pad pad;
>>  
>> @@ -409,28 +412,27 @@ static int gc0310_power_on(struct device *dev)
>>  
>>  static int gc0310_detect(struct gc0310_device *sensor)
>>  {
>> -	struct i2c_client *client = v4l2_get_subdevdata(&sensor->sd);
>>  	u64 val;
>>  	int ret;
>>  
>> -	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
>> +	if (!i2c_check_functionality(sensor->client->adapter, I2C_FUNC_I2C))
>>  		return -ENODEV;
>>  
>>  	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);
>> +		dev_err(sensor->dev, "read sensor_id failed: %d\n", ret);
>>  		return -ENODEV;
>>  	}
>>  
>> -	dev_dbg(&client->dev, "sensor ID = 0x%llx\n", val);
>> +	dev_dbg(sensor->dev, "sensor ID = 0x%llx\n", val);
>>  
>>  	if (val != GC0310_ID) {
>> -		dev_err(&client->dev, "sensor ID error, read id = 0x%llx, target id = 0x%x\n",
>> +		dev_err(sensor->dev, "sensor ID error, read id = 0x%llx, target id = 0x%x\n",
>>  			val, GC0310_ID);
>>  		return -ENODEV;
>>  	}
>>  
>> -	dev_dbg(&client->dev, "detect gc0310 success\n");
>> +	dev_dbg(sensor->dev, "detect gc0310 success\n");
>>  
>>  	return 0;
>>  }
>> @@ -440,10 +442,9 @@ static int gc0310_enable_streams(struct v4l2_subdev *sd,
>>  				 u32 pad, u64 streams_mask)
>>  {
>>  	struct gc0310_device *sensor = to_gc0310_sensor(sd);
>> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
>>  	int ret;
>>  
>> -	ret = pm_runtime_resume_and_get(&client->dev);
>> +	ret = pm_runtime_resume_and_get(sensor->dev);
>>  	if (ret)
>>  		return ret;
>>  
>> @@ -474,7 +475,7 @@ static int gc0310_enable_streams(struct v4l2_subdev *sd,
>>  
>>  error_power_down:
>>  	if (ret)
>> -		pm_runtime_put(&client->dev);
>> +		pm_runtime_put(sensor->dev);
>>  
>>  	return ret;
>>  }
>> @@ -484,7 +485,6 @@ static int gc0310_disable_streams(struct v4l2_subdev *sd,
>>  				  u32 pad, u64 streams_mask)
>>  {
>>  	struct gc0310_device *sensor = to_gc0310_sensor(sd);
>> -	struct i2c_client *client = v4l2_get_subdevdata(sd);
>>  	int ret = 0;
>>  
>>  	cci_write(sensor->regmap, GC0310_RESET_RELATED_REG,
>> @@ -494,7 +494,7 @@ static int gc0310_disable_streams(struct v4l2_subdev *sd,
>>  	cci_write(sensor->regmap, GC0310_RESET_RELATED_REG,
>>  		  GC0310_REGISTER_PAGE_0, &ret);
>>  
>> -	pm_runtime_put(&client->dev);
>> +	pm_runtime_put(sensor->dev);
>>  	return ret;
>>  }
>>  
>> @@ -559,7 +559,6 @@ static const struct v4l2_subdev_internal_ops gc0310_internal_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 exp_max, ret;
>> @@ -597,7 +596,7 @@ 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);
>> +	ret = v4l2_fwnode_device_parse(sensor->dev, &props);
>>  	if (ret)
>>  		return ret;
>>  
>> @@ -621,10 +620,10 @@ static void gc0310_remove(struct i2c_client *client)
>>  	v4l2_subdev_cleanup(sd);
>>  	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);
>> +	pm_runtime_disable(sensor->dev);
>> +	if (!pm_runtime_status_suspended(sensor->dev)) {
>> +		gc0310_power_off(sensor->dev);
>> +		pm_runtime_set_suspended(sensor->dev);
>>  	}
>>  }
>>  
>> @@ -695,15 +694,18 @@ static int gc0310_probe(struct i2c_client *client)
>>  	if (!sensor)
>>  		return -ENOMEM;
>>  
>> -	sensor->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
>> +	sensor->client = client;
>> +	sensor->dev = &client->dev;
>> +
>> +	sensor->reset = devm_gpiod_get(sensor->dev, "reset", GPIOD_OUT_HIGH);
>>  	if (IS_ERR(sensor->reset)) {
>> -		return dev_err_probe(&client->dev, PTR_ERR(sensor->reset),
>> +		return dev_err_probe(sensor->dev, PTR_ERR(sensor->reset),
>>  				     "getting reset GPIO\n");
>>  	}
>>  
>> -	sensor->powerdown = devm_gpiod_get(&client->dev, "powerdown", GPIOD_OUT_HIGH);
>> +	sensor->powerdown = devm_gpiod_get(sensor->dev, "powerdown", GPIOD_OUT_HIGH);
>>  	if (IS_ERR(sensor->powerdown)) {
>> -		return dev_err_probe(&client->dev, PTR_ERR(sensor->powerdown),
>> +		return dev_err_probe(sensor->dev, PTR_ERR(sensor->powerdown),
>>  				     "getting powerdown GPIO\n");
>>  	}
>>  
>> @@ -713,11 +715,11 @@ static int gc0310_probe(struct i2c_client *client)
>>  	if (IS_ERR(sensor->regmap))
>>  		return PTR_ERR(sensor->regmap);
>>  
>> -	gc0310_power_on(&client->dev);
>> +	gc0310_power_on(sensor->dev);
>>  
>> -	pm_runtime_set_active(&client->dev);
>> -	pm_runtime_get_noresume(&client->dev);
>> -	pm_runtime_enable(&client->dev);
>> +	pm_runtime_set_active(sensor->dev);
>> +	pm_runtime_get_noresume(sensor->dev);
>> +	pm_runtime_enable(sensor->dev);
>>  
>>  	ret = gc0310_detect(sensor);
>>  	if (ret)
>> @@ -734,34 +736,34 @@ static int gc0310_probe(struct i2c_client *client)
>>  
>>  	ret = media_entity_pads_init(&sensor->sd.entity, 1, &sensor->pad);
>>  	if (ret) {
>> -		dev_err_probe(&client->dev, ret, "failed to init entity pads\n");
>> +		dev_err_probe(sensor->dev, ret, "failed to init entity pads\n");
>>  		goto error_handler_free;
>>  	}
>>  
>>  	sensor->sd.state_lock = sensor->ctrls.handler.lock;
>>  	ret = v4l2_subdev_init_finalize(&sensor->sd);
>>  	if (ret) {
>> -		dev_err_probe(&client->dev, ret, "subdev init error\n");
>> +		dev_err_probe(sensor->dev, ret, "subdev init error\n");
>>  		goto error_media_entity;
>>  	}
>>  
>>  	ret = v4l2_async_register_subdev_sensor(&sensor->sd);
>>  	if (ret) {
>> -		dev_err_probe(&client->dev, ret,
>> +		dev_err_probe(sensor->dev, ret,
>>  			      "failed to register gc0310 sub-device\n");
>>  		goto error_subdev_cleanup;
>>  	}
>>  
>> -	pm_runtime_set_autosuspend_delay(&client->dev, 1000);
>> -	pm_runtime_use_autosuspend(&client->dev);
>> -	pm_runtime_put_autosuspend(&client->dev);
>> +	pm_runtime_set_autosuspend_delay(sensor->dev, 1000);
>> +	pm_runtime_use_autosuspend(sensor->dev);
>> +	pm_runtime_put_autosuspend(sensor->dev);
>>  
>>  	return 0;
>>  
>>  error_subdev_cleanup:
>>  	v4l2_subdev_cleanup(&sensor->sd);
>> -	pm_runtime_disable(&client->dev);
>> -	pm_runtime_set_suspended(&client->dev);
>> +	pm_runtime_disable(sensor->dev);
>> +	pm_runtime_set_suspended(sensor->dev);
>>  
>>  error_media_entity:
>>  	media_entity_cleanup(&sensor->sd.entity);
>> @@ -770,8 +772,8 @@ static int gc0310_probe(struct i2c_client *client)
>>  	v4l2_ctrl_handler_free(&sensor->ctrls.handler);
>>  
>>  error_power_off:
>> -	pm_runtime_put_noidle(&client->dev);
>> -	gc0310_power_off(&client->dev);
>> +	pm_runtime_put_noidle(sensor->dev);
>> +	gc0310_power_off(sensor->dev);
>>  
>>  	return ret;
>>  }
>

[PATCH 3/3] media: i2c: gc0310: Use devm_v4l2_sensor_clk_get()

[Sanjay Chitroda]

From: Sanjay Chitroda <sanjayembeddedse@gmail.com>

Several camera sensor drivers access the "clock-frequency" property
directly to retrieve the external clock rate or handle the external
clock manually in the driver. While this is valid on a subset of ACPI
platforms, implementing this logic directly in drivers is deprecated
and can lead to inconsistent behaviour across drivers.

This driver supports ACPI platforms only. It currently retrieves the
external clock rate from the "clock-frequency" property and fails
probing if the rate does not match the expected value, which is the
correct policy for ACPI platforms.

Switch to using the devm_v4l2_sensor_clk_get() helper to standardise
clock handling. This preserves the existing behaviour on ACPI
platforms that specify a clock-frequency property without providing
a clock. On platforms that provide a clock, the helper will program
the clock to the rate specified by clock-frequency, which is also
consistent with the driver's expectations.

Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>
---
 drivers/media/i2c/gc0310.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/media/i2c/gc0310.c b/drivers/media/i2c/gc0310.c
index e538479fee2e..e9e67bd73f51 100644
--- a/drivers/media/i2c/gc0310.c
+++ b/drivers/media/i2c/gc0310.c
@@ -6,6 +6,7 @@
  * Copyright (c) 2023-2025 Hans de Goede <hansg@kernel.org>
  */
 
+#include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/errno.h>
 #include <linux/gpio/consumer.h>
@@ -84,6 +85,7 @@
 #define to_gc0310_sensor(x) container_of(x, struct gc0310_device, sd)
 
 struct gc0310_device {
+	struct clk *clk;
 	struct device *dev;
 	struct i2c_client *client;
 
@@ -634,7 +636,6 @@ static int gc0310_check_hwcfg(struct device *dev)
 	};
 	struct fwnode_handle *ep_fwnode;
 	unsigned long link_freq_bitmap;
-	u32 mclk;
 	int ret;
 
 	/*
@@ -646,21 +647,6 @@ static int gc0310_check_hwcfg(struct device *dev)
 		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)
@@ -684,6 +670,7 @@ static int gc0310_check_hwcfg(struct device *dev)
 static int gc0310_probe(struct i2c_client *client)
 {
 	struct gc0310_device *sensor;
+	unsigned long freq;
 	int ret;
 
 	ret = gc0310_check_hwcfg(&client->dev);
@@ -697,6 +684,16 @@ static int gc0310_probe(struct i2c_client *client)
 	sensor->client = client;
 	sensor->dev = &client->dev;
 
+	sensor->clk = devm_v4l2_sensor_clk_get(sensor->dev, NULL);
+	if (IS_ERR(sensor->clk))
+		return dev_err_probe(sensor->dev, PTR_ERR(sensor->clk),
+				     "failed to get clock\n");
+
+	freq = clk_get_rate(sensor->clk);
+	if (freq != GC0310_MCLK_FREQ)
+		return dev_err_probe(sensor->dev, -EINVAL,
+				     "external clock %lu is not supported\n", freq);
+
 	sensor->reset = devm_gpiod_get(sensor->dev, "reset", GPIOD_OUT_HIGH);
 	if (IS_ERR(sensor->reset)) {
 		return dev_err_probe(sensor->dev, PTR_ERR(sensor->reset),
-- 
2.34.1

Re: [PATCH 3/3] media: i2c: gc0310: Use devm_v4l2_sensor_clk_get()

[Hans de Goede]

Hi,

On 1-Apr-26 20:16, Sanjay Chitroda wrote:
> From: Sanjay Chitroda <sanjayembeddedse@gmail.com>
> 
> Several camera sensor drivers access the "clock-frequency" property
> directly to retrieve the external clock rate or handle the external
> clock manually in the driver. While this is valid on a subset of ACPI
> platforms, implementing this logic directly in drivers is deprecated
> and can lead to inconsistent behaviour across drivers.
> 
> This driver supports ACPI platforms only. It currently retrieves the
> external clock rate from the "clock-frequency" property and fails
> probing if the rate does not match the expected value, which is the
> correct policy for ACPI platforms.
> 
> Switch to using the devm_v4l2_sensor_clk_get() helper to standardise
> clock handling. This preserves the existing behaviour on ACPI
> platforms that specify a clock-frequency property without providing
> a clock. On platforms that provide a clock, the helper will program
> the clock to the rate specified by clock-frequency, which is also
> consistent with the driver's expectations.
> 
> Signed-off-by: Sanjay Chitroda <sanjayembeddedse@gmail.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans



> ---
>  drivers/media/i2c/gc0310.c | 29 +++++++++++++----------------
>  1 file changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/i2c/gc0310.c b/drivers/media/i2c/gc0310.c
> index e538479fee2e..e9e67bd73f51 100644
> --- a/drivers/media/i2c/gc0310.c
> +++ b/drivers/media/i2c/gc0310.c
> @@ -6,6 +6,7 @@
>   * Copyright (c) 2023-2025 Hans de Goede <hansg@kernel.org>
>   */
>  
> +#include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/errno.h>
>  #include <linux/gpio/consumer.h>
> @@ -84,6 +85,7 @@
>  #define to_gc0310_sensor(x) container_of(x, struct gc0310_device, sd)
>  
>  struct gc0310_device {
> +	struct clk *clk;
>  	struct device *dev;
>  	struct i2c_client *client;
>  
> @@ -634,7 +636,6 @@ static int gc0310_check_hwcfg(struct device *dev)
>  	};
>  	struct fwnode_handle *ep_fwnode;
>  	unsigned long link_freq_bitmap;
> -	u32 mclk;
>  	int ret;
>  
>  	/*
> @@ -646,21 +647,6 @@ static int gc0310_check_hwcfg(struct device *dev)
>  		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)
> @@ -684,6 +670,7 @@ static int gc0310_check_hwcfg(struct device *dev)
>  static int gc0310_probe(struct i2c_client *client)
>  {
>  	struct gc0310_device *sensor;
> +	unsigned long freq;
>  	int ret;
>  
>  	ret = gc0310_check_hwcfg(&client->dev);
> @@ -697,6 +684,16 @@ static int gc0310_probe(struct i2c_client *client)
>  	sensor->client = client;
>  	sensor->dev = &client->dev;
>  
> +	sensor->clk = devm_v4l2_sensor_clk_get(sensor->dev, NULL);
> +	if (IS_ERR(sensor->clk))
> +		return dev_err_probe(sensor->dev, PTR_ERR(sensor->clk),
> +				     "failed to get clock\n");
> +
> +	freq = clk_get_rate(sensor->clk);
> +	if (freq != GC0310_MCLK_FREQ)
> +		return dev_err_probe(sensor->dev, -EINVAL,
> +				     "external clock %lu is not supported\n", freq);
> +
>  	sensor->reset = devm_gpiod_get(sensor->dev, "reset", GPIOD_OUT_HIGH);
>  	if (IS_ERR(sensor->reset)) {
>  		return dev_err_probe(sensor->dev, PTR_ERR(sensor->reset),