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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87r10bo1k5.fsf@gmail.com>
Date:   Fri, 16 Sep 2022 16:44:31 +0300
From:   Mikhail Rudenko <mike.rudenko@...il.com>
To:     Tommaso Merciai <tommaso.merciai@...rulasolutions.com>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Hans Verkuil <hverkuil-cisco@...all.nl>,
        Jacopo Mondi <jacopo@...ndi.org>,
        Shawn Tu <shawnx.tu@...el.com>,
        Randy Dunlap <rdunlap@...radead.org>,
        Daniel Scally <djrscally@...il.com>,
        Christian Hemp <c.hemp@...tec.de>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Marek Vasut <marex@...x.de>, linux-media@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] media: i2c: add support for ov4689


On 2022-09-16 at 15:34 +02, Tommaso Merciai <tommaso.merciai@...rulasolutions.com> 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@...rulasolutions.com> 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@amarulasolutions.com/
>
> 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