On 11/03/2022 19:00, Jacopo Mondi wrote: > Hi Krzysztof > > On Fri, Mar 11, 2022 at 05:11:47PM +0100, Krzysztof Kozlowski wrote: >> On 11/03/2022 17:05, Jacopo Mondi wrote: >>> Hi Krzysztof, >>> >>> On Thu, Mar 10, 2022 at 06:26:02PM +0100, Krzysztof Kozlowski wrote: >>>> On 10/03/2022 18:16, Jacopo Mondi wrote: >>>>> Hi Krzysztof >>>>> >>>>> 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. Best regards, Krzysztof
