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

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

 



On 15/03/2022 08:59, Sakari Ailus wrote:
> Hi Krzysztof, Jacopo,
> 
> On Tue, Mar 15, 2022 at 08:32:58AM +0100, Krzysztof Kozlowski wrote:
>> On 14/03/2022 17:27, Jacopo Mondi wrote:
>>> Provide the bindings documentation for Omnivision OV5670 image sensor.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
>>>
>>> ---
>>> v1->v2 (comments from Krzysztof)
>>>
>>> - Rename to include manufacturer name
>>> - Add entry to MAINTAINERS
>>> - Add maxItems: to -gpios properties
>>> - Use common clock properties
>>> - Use enum: [1, 2] for data lanes
>>> - Fix whitespace issue in example
>>> ---
>>>
>>>  .../bindings/media/i2c/ovti,ov5670.yaml       | 99 +++++++++++++++++++
>>>  MAINTAINERS                                   |  1 +
>>>  2 files changed, 100 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
>>> new file mode 100644
>>> index 000000000000..73cf72203f17
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5670.yaml
>>> @@ -0,0 +1,99 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/media/i2c/ovti,ov5670.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Omnivision OV5670 5 Megapixels raw image sensor
>>> +
>>> +maintainers:
>>> +  - Jacopo Mondi <jacopo@xxxxxxxxxx>
>>> +
>>> +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
>>> +
>>> +  assigned-clocks: true
>>> +  assigned-clock-parents: true
>>> +  assigned-clock-rates: true
>>
>> You should not need these. These are coming with schema. You can add
>> these to example schema below and double-check.
> 
> They should probably be required actually.

Why required? The hardware can work with different clocks, get their
rate and configure internal PLLs/clocks to new value. Having it required
might have sense for current implementation of driver but this is
independent of bindings. Bindings do not describe driver, but hardware.

>>
>>> +
>>> +  clocks:
>>> +    description: System clock. From 6 to 27 MHz.
>>> +    maxItems: 1
>>> +
>>> +  pwdn-gpios:
>>> +    description: Reference to the GPIO connected to the PWDNB pin. Active low.
>>
>> This does not look like a standard property, so you need a vendor prefix.
> 
> The similarly named property exists elsewhere. I wouldn't use a vendor
> prefix, also for the reason that the functionality is quite common. I guess
> alternative name would be possible, too --- "shutdown" seems to be more
> common.

It exists in three bindings, so it is not a standard property...
although something closer to standard is "powerdown-gpios" so maybe just
use that one?

Best regards,
Krzysztof



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux