Re: [PATCH 1/6] media: dt-bindings: i2c: Document ov5670

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux