Hi Paul, On Wed, Nov 04, 2020 at 11:26:43AM +0100, Paul Kocialkowski wrote: > Hi Sakari and thanks for the review! > > On Tue 03 Nov 20, 01:24, Sakari Ailus wrote: > > On Fri, Oct 23, 2020 at 07:54:04PM +0200, Paul Kocialkowski wrote: > > > This introduces YAML bindings documentation for the OV8865 > > > image sensor. > > > > > > Co-developed-by: Kévin L'hôpital <kevin.lhopital@xxxxxxxxxxx> > > > Signed-off-by: Kévin L'hôpital <kevin.lhopital@xxxxxxxxxxx> > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx> > > > --- > > > .../bindings/media/i2c/ovti,ov8865.yaml | 124 ++++++++++++++++++ > > > 1 file changed, 124 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml > > > new file mode 100644 > > > index 000000000000..807f1a94afae > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml > > > @@ -0,0 +1,124 @@ > > > +# SPDX-License-Identifier: GPL-2.0 > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/media/i2c/ovti,ov8865.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: OmniVision OV8865 Image Sensor Device Tree Bindings > > > + > > > +maintainers: > > > + - Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx> > > > + > > > +properties: > > > + compatible: > > > + const: ovti,ov8865 > > > + > > > + reg: > > > + maxItems: 1 > > > + > > > + clocks: > > > + items: > > > + - description: EXTCLK Clock > > > + > > > + clock-names: > > > + items: > > > + - const: extclk > > > > Is this needed with a single clock? > > Yes I think so: we grab the clock with devm_clk_get which takes a name string > that matches the clock-names property. That argument may be NULL. > > > And... shouldn't this also come with assigned-clock-rates etc., to set the > > clock frequency? > > I'm a bit confused why we would need to do that in the device-tree rather than > setting the clock rate with clk_set_rate in the driver, like any other driver > does. I think this was discussed before (on the initial ov8865 series) and the > conclusion was that there is no particular reason for media i2c drivers to > behave differently. So I believe this is the correct approach. I'm not exactly sure about that conclusion. You can use clk_set_rate() if you get the frequency from DT, but we recently did conclude that camera sensor drivers can expect to get the frequency indicated by assigned-clock-rate property. In other words, the driver may not be specific to a particular board and SoC you have. Please also read Documentation/driver-api/media/camera-sensor.rst . > > > > + > > > + dvdd-supply: > > > + description: Digital Domain Power Supply > > > + > > > + avdd-supply: > > > + description: Analog Domain Power Supply (internal AVDD is used if missing) > > > + > > > + dovdd-supply: > > > + description: I/O Domain Power Supply > > > + > > > + powerdown-gpios: > > > + maxItems: 1 > > > + description: Power Down Pin GPIO Control (active low) > > > + > > > + reset-gpios: > > > + maxItems: 1 > > > + description: Reset Pin GPIO Control (active low) > > > + > > > + port: > > > + type: object > > > + description: Input port, connect to a MIPI CSI-2 receiver > > > + > > > + properties: > > > + endpoint: > > > + type: object > > > + > > > + properties: > > > + remote-endpoint: true > > > + > > > + bus-type: > > > + const: 4 > > > + > > > + clock-lanes: > > > + maxItems: 1 > > > > I believe you can drop clock-lanes and bus-type; these are both constants. > > I don't understand why bus-type should be dropped because it is constant: > if bus-type is set to something else, the driver will definitely not probe > since we're requesting V4L2_MBUS_CSI2_DPHY for v4l2_fwnode_endpoint_parse. > So I think it's quite important for the bindings to reflect this. This driver is for a particular device that has MIPI CSI-2 on D-PHY as the data bus. You can assume that in the driver. > > > I presume the device does not support lane remapping? > > That's correct so this is indeed not something we can configure. > But shouldn't we instead specift clock-lanes = <0> as a const rather than > getting rid of it? Why would you put redundant information to DT? > > > Could you also add link-frequencies, to list which frequencies are known to > > be good? > > Ah right, I had missed it. I'm a bit unsure about what I should do with the > information from the driver though: should I refuse to use link frequencies that > are not in the list? Yes, please. -- Regards, Sakari Ailus
