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

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

 



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



[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