linux-kernel - Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

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: <YXBjX2vUwrKVOd78@valkosipuli.retiisi.eu>
Date:   Wed, 20 Oct 2021 21:43:43 +0300
From:   Sakari Ailus <sakari.ailus@....fi>
To:     Krzysztof Hałasa <khalasa@...p.pl>
Cc:     Jacopo Mondi <jacopo@...ndi.org>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Matteo Lisi <matteo.lisi@...icam.com>
Subject: Re: [PATCH v5] Driver for ON Semi AR0521 camera sensor

Hi Krzysztof,

On Wed, Oct 13, 2021 at 07:39:07AM +0200, Krzysztof Hałasa wrote:
> Hi Sakari,
> 
> > 	https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html
> 
> Ok:
> "8.2.2. Devicetree
> 
> The currently preferred way to achieve this is using assigned-clocks,
> assigned-clock-parents and assigned-clock-rates properties. See
> Documentation/devicetree/bindings/clock/clock-bindings.txt for more
> information. The driver then gets the frequency using clk_get_rate()."
> 
> Let's see:
> Documentation/devicetree/bindings/clock/clock-bindings.txt:
> 
> "==Assigned clock parents and rates==
> 
> Some platforms may require initial configuration of default parent clocks
> and clock frequencies. Such a configuration can be specified in a device tree
> node through assigned-clocks, assigned-clock-parents and assigned-clock-rates
> properties. The assigned-clock-parents property should contain a list of parent
> clocks in the form of a phandle and clock specifier pair and the
> assigned-clock-rates property should contain a list of frequencies in Hz. Both
> these properties should correspond to the clocks listed in the assigned-clocks
> property."
> 
> So I'm after "assigned-clock-rates", right?
> 
> "Configuring a clock's parent and rate through the device node that consumes
> the clock can be done only for clocks that have a single user. Specifying
> conflicting parent or rate configuration in multiple consumer nodes for
> a shared clock is forbidden."
> 
> This sounds a bit problematic, the clock I use is at least potentially
> shared by multiple parts of the system, depending on current (run time)
> configuration. I am/was getting different frequencies depending of the
> particular system (all based on the same i.MX6* SoC, but with different
> peripherals used/enabled). I think it's quite a common situation.

This was discussed some time ago before I wrote the documentation. The
conclusion back then was that it's just fine, and such cases would need to
be addressed when they turn up. We haven't had any yet as far as I know.

> 
> > Generally camera sensor drivers that set the clock in drivers themselves
> > are (very) old.
> 
> Let's have a look... ov9282 is (one of) the newest drivers. It does:
> #define OV9282_INCLK_RATE    24000000
> 
>         /* Get sensor input clock */
>         ov9282->inclk = devm_clk_get(ov9282->dev, NULL);
>         if (IS_ERR(ov9282->inclk)) {
>                 dev_err(ov9282->dev, "could not get inclk");
>                 return PTR_ERR(ov9282->inclk);
>         }
> 
>         rate = clk_get_rate(ov9282->inclk);
>         if (rate != OV9282_INCLK_RATE) {
>                 dev_err(ov9282->dev, "inclk frequency mismatch");
>                 return -EINVAL;
>         }
> 
> $ git grep -l ov9282
> Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml
> MAINTAINERS
> drivers/media/i2c/Kconfig
> drivers/media/i2c/Makefile
> drivers/media/i2c/ov9282.c
> 
>   clocks:
>     description: Clock frequency from 6 to 27MHz
> 
> No in-tree DTS exists, but the single frequency (both in the driver -
> this one can be fixed - and in the DTS) is rather limiting. Maybe
> another:
> 
> imx412, imx335, imx334, imx258 - same here.
> imx208 is ACPI-based.
> 
> Which driver should I consult?

The drivers you're looking at are based on register lists so they usually
support just a single frequency. The sensors are not limited to this
frequency however, which is why you see the frequency in DT bindings, too.

-- 
Regards,

Sakari Ailus