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

[Sanjay Chitroda]

On 2 April 2026 12:50:20 am IST, Hans de Goede <hansg@xxxxxxxxxx> wrote:
>Hi,
>
>On 1-Apr-26 20:16, Sanjay Chitroda wrote:
>> From: Sanjay Chitroda <sanjayembeddedse@xxxxxxxxx>
>> 
>> 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@xxxxxxxxx>
>
>Thanks, patch looks good to me:
>
>Reviewed-by: Hans de Goede <johannes.goede@xxxxxxxxxxxxxxxx>
>
>Regards,
>
>Hans
>
>
Hi maintainer,

It looks this change is not reflected in the media-next, is there pending to update or information required on the same to proceed further ?

Thanks,
Sanjay Chitroda

>
>> ---
>>  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@xxxxxxxxxx>
>>   */
>>  
>> +#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),
>