Re: [PATCH v2 2/2] media: i2c: add support for ov4689

[Mikhail Rudenko]

On 2022-09-16 at 15:34 +02, Tommaso Merciai <tommaso.merciai@xxxxxxxxxxxxxxxxxxxx> wrote:
> Hi Mikhail,
>
> On Thu, Sep 15, 2022 at 11:50:23PM +0300, Mikhail Rudenko wrote:
>>
>> Hi Tommaso,
>>
>> On 2022-09-14 at 17:51 +02, Tommaso Merciai <tommaso.merciai@xxxxxxxxxxxxxxxxxxxx> wrote:
>> > Hi Mikhail,
>> > I do a first round on reviewing your driver :)
>> >
>> > On Sun, Sep 11, 2022 at 11:01:35PM +0300, Mikhail Rudenko wrote:

<snip>

>> >> +
>> >> +	ov4689->xvclk = devm_clk_get(dev, "xvclk");
>> >> +	if (IS_ERR(ov4689->xvclk)) {
>> >> +		dev_err(dev, "Failed to get xvclk\n");
>> >> +		return -EINVAL;
>> >> +	}
>> >
>> > ^ I think is better to use devm_clk_get_optional instead of clck_get.
>> > clck_get can fail in CPU's that use ACPI
>> >
>> >> +
>> >> +	ret = clk_set_rate(ov4689->xvclk, OV4689_XVCLK_FREQ);
>> >> +	if (ret < 0) {
>> >> +		dev_err(dev, "Failed to set xvclk rate (24MHz)\n");
>> >> +		return ret;
>> >> +	}
>> >> +	if (clk_get_rate(ov4689->xvclk) != OV4689_XVCLK_FREQ)
>> >> +		dev_warn(dev, "xvclk mismatched, modes are based on 24MHz\n");
>> >
>> >
>> > What do you think about?
>> > Thanks.
>>
>> Unfortunately, I have no experience with ACPI-based devices. :(
>>
>> Do you mean that in the case of an ACPI device and devm_clk_get_optional
>> returning NULL we should assume that the clock is already enabled and
>> will stay enabled during sensor operation? How should we distinguish it
>> from the case of an OF-based system and clock just missing from device
>> tree?
>
> Not exaclty :)
>
> I copy comment from [1]
>
> if you use ov5693->xvclk to identify the ACPI vs OF use case shouldn't
> you use the get_optionl() version ? Otherwise in the ACPI case you will have
> -ENOENT if there's not 'xvclk' property and bail out.
>
> Unless my understanding is wrong on ACPI we have "clock-frequency" and
> on OF "xvclk" with an "assigned-clock-rates",
>
> [1] https://patchwork.linuxtv.org/project/linux-media/patch/20220627150453.220292-5-tommaso.merciai@xxxxxxxxxxxxxxxxxxxx/
>
> Let me know if you need more details.

Thanks for the pointer! I'll try to implement something along the lines
of your ov5693 series.

But I'm not sure that will be enough to support ACPI systems
correctly. What about lanes number and link frequency checks? Should
they be made conditional on CONFIG_OF? Anything else I don't know?

>
> Regards,
> Tommaso
>
--
Best regards,
Mikhail Rudenko