Re: [PATCH v1 41/55] media: i2c: ov8865: Use the v4l2 helper for obtaining the clock

[Hans de Goede <hansg@kernel.org>]

Hi,

On 21-Jun-25 9:58 AM, Sakari Ailus wrote:
> Hi Laurent,
> 
> On Fri, Jun 20, 2025 at 12:59:51AM +0300, Laurent Pinchart wrote:
>> On Fri, Jun 20, 2025 at 12:57:46AM +0300, Laurent Pinchart wrote:
>>> Hi Mehdi,
>>>
>>> Thank you for the patch.
>>>
>>> On Thu, Jun 19, 2025 at 07:59:34PM +0200, Mehdi Djait wrote:
>>>> devm_clk_get() fails on ACPI-based platforms, where firmware does not
>>>> provide a direct reference to the clock producer.
>>>>
>>>> Replace devm_clk_get() with the new v4l2 helper
>>>> devm_v4l2_sensor_clk_get() that works on both DT- and ACPI-based
>>>> platforms to retrieve a reference to the clock producer from firmware.
>>>>
>>>> Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
>>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>
>>>>
>>>> diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
>>>> index 95ffe7536aa6..5cc278c3e169 100644
>>>> --- a/drivers/media/i2c/ov8865.c
>>>> +++ b/drivers/media/i2c/ov8865.c
>>>> @@ -2956,7 +2956,6 @@ static int ov8865_probe(struct i2c_client *client)
>>>>  	struct ov8865_sensor *sensor;
>>>>  	struct v4l2_subdev *subdev;
>>>>  	struct media_pad *pad;
>>>> -	unsigned int rate = 0;
>>>>  	unsigned int i;
>>>>  	int ret;
>>>>  
>>>> @@ -3019,39 +3018,14 @@ static int ov8865_probe(struct i2c_client *client)
>>>>  
>>>>  	/* External Clock */
>>>>  
>>>> -	sensor->extclk = devm_clk_get(dev, NULL);
>>>> -	if (PTR_ERR(sensor->extclk) == -ENOENT) {
>>>> -		dev_info(dev, "no external clock found, continuing...\n");
>>>> -		sensor->extclk = NULL;
>>>> -	} else if (IS_ERR(sensor->extclk)) {
>>>> +	sensor->extclk = devm_v4l2_sensor_clk_get(dev, NULL);
>>>> +	if (IS_ERR(sensor->extclk)) {
>>>>  		dev_err(dev, "failed to get external clock\n");
>>>>  		ret = PTR_ERR(sensor->extclk);
>>
>> Actually, I'd take this as an opportunity to write
>>
>>  		ret = dev_err_probe(dev, PTR_ERR(sensor->extclk),
>> 				    "failed to get external clock\n");
> 
> Wouldn't printing this kind of a message fit for devm_v4l2_sensor_clk_get()
> and consequently be removed from drivers?

+1 to that suggestion having the helper always log an error on errors
and then being able to just directly return an error without needing
to log would be great.

Note the helper really should use dev_err_probe() for these errors,
so that if devm_clk_get() ever starts returning -EPROBE_DEFER that
will be handled properly.

Otherwise this looks good to me. Mehdi thank you for your work on this!

Regards,

Hans