Re: [PATCH 1/2] dt-bindings: media: i2c: Add IMX296 CMOS sensor binding

[Sakari Ailus]

Hi Manivannan, Rob,

On Wed, Oct 16, 2019 at 02:07:48PM +0530, Manivannan Sadhasivam wrote:
> Hi Rob,
> 
> On Tue, Oct 15, 2019 at 05:45:54PM -0500, Rob Herring wrote:
> > On Fri, Oct 11, 2019 at 09:26:12AM +0530, Manivannan Sadhasivam wrote:
> > > Add devicetree binding for IMX296 CMOS image sensor.
> > > 
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > > ---
> > >  .../devicetree/bindings/media/i2c/imx296.txt  | 55 +++++++++++++++++++
> > >  1 file changed, 55 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/imx296.txt
> > 
> > You should know by now, use DT schema format please.
> > 
> 
> I know for other subsystems but by having a vague look at the existing bindings
> I thought media subsystem is still using .txt. But I now see few yaml bindings
> in linux-next and will switch over this.
> 
> Btw, is it mandatory now to use YAML bindings for all subsystems? I don't
> see any issue (instead I prefer) but I remember that you defer to the preference
> of the subsystem maintainers before!
> 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/imx296.txt b/Documentation/devicetree/bindings/media/i2c/imx296.txt
> > > new file mode 100644
> > > index 000000000000..25d3b15162c1
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/imx296.txt
> > > @@ -0,0 +1,55 @@
> > > +* Sony IMX296 1/2.8-Inch CMOS Image Sensor
> > > +
> > > +The Sony IMX296 is a 1/2.9-Inch active pixel type CMOS Solid-state image
> > > +sensor with square pixel array and 1.58 M effective pixels. This chip features
> > > +a global shutter with variable charge-integration time. It is programmable
> > > +through I2C and 4-wire interfaces. The sensor output is available via CSI-2
> > > +serial data output (1 Lane).
> > > +
> > > +Required Properties:
> > > +- compatible: Should be "sony,imx296"
> > > +- reg: I2C bus address of the device
> > > +- clocks: Reference to the mclk clock.
> > > +- clock-names: Should be "mclk".
> > > +- clock-frequency: Frequency of the mclk clock in Hz.
> > > +- vddo-supply: Interface power supply.
> > > +- vdda-supply: Analog power supply.
> > > +- vddd-supply: Digital power supply.
> > > +
> > > +Optional Properties:
> > > +- reset-gpios: Sensor reset GPIO
> > > +
> > > +The imx296 device node should contain one 'port' child node with
> > > +an 'endpoint' subnode. For further reading on port node refer to
> > > +Documentation/devicetree/bindings/media/video-interfaces.txt.
> > > +
> > > +Required Properties on endpoint:
> > > +- data-lanes: check ../video-interfaces.txt
> > 
> > This should only be required when not using all the lanes on the device.
> > 
> 
> This is a bit weird! How will someone know how many lanes the device is using
> by looking at the binding? He can anyway refer the datasheet but still...

Many current bindings document data-lanes as mandatory. Nothing prevents
making all lanes are connected the default though, thus making data-lanes
optional.

The V4L2 fwnode framework supports easy parsing of that, too, by driver
providing that default value before letting V4L2 fwnode framework to parse
the endpoint properties.

Looking at this particular sensor --- doesn't it only have a single lane,
and thus nothing to configure here?

-- 
Regards,

Sakari Ailus