Hello, On Sat, Mar 12, 2022 at 11:30:55AM +0100, Krzysztof Kozlowski wrote: > On 11/03/2022 19:00, Jacopo Mondi wrote: > > On Fri, Mar 11, 2022 at 05:11:47PM +0100, Krzysztof Kozlowski wrote: > >> On 11/03/2022 17:05, Jacopo Mondi wrote: > >>> On Thu, Mar 10, 2022 at 06:26:02PM +0100, Krzysztof Kozlowski wrote: > >>>> On 10/03/2022 18:16, Jacopo Mondi wrote: > >>>>> On Thu, Mar 10, 2022 at 03:29:24PM +0100, Krzysztof Kozlowski wrote: > >>>>>> On 10/03/2022 14:08, Jacopo Mondi wrote: > >>>>>>> Provide the bindings documentation for Omnivision OV5670 image sensor. > >>>>>>> > >>>>>>> Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx> > >>>>>>> --- > >>>>>>> .../devicetree/bindings/media/i2c/ov5670.yaml | 93 +++++++++++++++++++ > >>>>>> > >>>>>> Add the file to maintainers entry. > >>>>> > >>>>> Right > >>>>> > >>>>>>> 1 file changed, 93 insertions(+) > >>>>>>> create mode 100644 Documentation/devicetree/bindings/media/i2c/ov5670.yaml > >>>>>>> > >>>>>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml > >>>>>>> new file mode 100644 > >>>>>>> index 000000000000..dc4a3297bf6f > >>>>>>> --- /dev/null > >>>>>>> +++ b/Documentation/devicetree/bindings/media/i2c/ov5670.yaml > >>>>>> > >>>>>> Missing vendor prefix in file name. > >>>>> > >>>>> Right x2 > >>>>> > >>>>>>> @@ -0,0 +1,93 @@ > >>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >>>>>>> +%YAML 1.2 > >>>>>>> +--- > >>>>>>> +$id: http://devicetree.org/schemas/media/i2c/ov5670.yaml# > >>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >>>>>>> + > >>>>>>> +title: Omnivision OV5670 5 Megapixels raw image sensor > >>>>>>> + > >>>>>>> +maintainers: > >>>>>>> + - Jacopo Mondi <jacopo@xxxxxxxxxx> > >>>>>> > >>>>>> Please add also driver maintainer. > >>>>> > >>>>> I never got what the policy was, if the maintainer entries here only > >>>>> refer to the binding file or to the driver too > >>>> > >>>> It is a person responsible for the bindings, so indeed it might not feed > >>>> existing maintainer. > >>>> > >>>>>>> + > >>>>>>> +description: |- > >>>>>>> + The OV5670 is a 5 Megapixels raw image sensor which provides images in 10-bits > >>>>>>> + RAW BGGR Bayer format on a 2 data lanes MIPI CSI-2 serial interface and is > >>>>>>> + controlled through an I2C compatible control bus. > >>>>>>> + > >>>>>>> +properties: > >>>>>>> + compatible: > >>>>>>> + const: ovti,ov5670 > >>>>>>> + > >>>>>>> + reg: > >>>>>>> + maxItems: 1 > >>>>>>> + > >>>>>>> + clock-frequency: > >>>>>>> + description: Frequency of the xclk clock. > >>>>>> > >>>>>> Is the xclk external clock coming to the sensor? If yes, there should be > >>>>>> a "clocks" property. > >>>>> > >>>>> To be honest I was not sure about this, as clock-frequency is already > >>>>> used by the driver for the ACPI part, but it seems to in DT bindings > >>>>> it is a property meant to be specified in the clock providers, even if > >>>>> Documentation/devicetree/bindings/clock/clock-bindings.txt doesn't > >>>>> really clarify this > >>>>> > >>>>> Clock consumer should rather use 'clocks' and point to the provider's > >>>>> phandle if my understanding is right. > >>>> > >>>> This is a clock-frequency, not clock reference. For external clocks, a > >>> > >>> Yes, I was suggesting to replace clock-frequency with clocks, that > >>> accepts a phandle. > >>> > >>> The thing is, the driver parses 'clock-frequency' > >>> device_property_read_u32(&client->dev, "clock-frequency", &input_clk); > >>> > >>> which I assume comes from ACPI (as the driver was developed for an > >>> ACPI platform). > >>> > >>> If in DTS we don't use it, I then need to > >>> > >>> #ifdef CONFIG_ACPI > >>> > >>> #elif defined CONFIG_OF > >>> > >>> #endif > >>> > >>> Which I would really like to avoid. > >>> > >>> Anyone with ACPI experience that knows where clock-frequency comes > >>> from ? > >> > >> I would assume that ACPI simply does not support common clock framework, > >> so it had to use clock-frequency. Several of such drivers were added by > >> folks from Intel which use ACPI, not Devicetree. > >> > >>>> clock phandles + assigned-clock-rates should be rather used. However for > >>>> internal clocks, this is a perfectly valid property. > >>>> > >>>> Therefore the question is - what is the "xclk"? > >>> > >>> xclk is the clock fed to the sensor, which which all its internal > >>> clocks are generated, so it's indeed an 'external' clock. As I've > >>> said, clock-frequency seems to be meant for clock providers, and > >>> the image sensor is a clock consumer. > >> > >> Regardless whether clock-frequency stays or not, you need the clocks > >> property in such case. > > > > Yes, I will have to ifdef in the driver if no better alternatives > > I do not see the need of ifdefs... BTW, imx258 has exactly that case - > clock-frequency coming from ACPI world but not added to DT bindings. The driver can call clk_get_rate() when a clock is provided, and use the clock-frequency property otherwise. I also don't think conditional compilation is needed. -- Regards, Laurent Pinchart
