Re: [PATCH v5 2/3] media: dt-bindings: alvium: add document YAML binding

[Sakari Ailus]

Hi Tommaso,

On Fri, Jun 09, 2023 at 11:41:16AM +0200, Tommaso Merciai wrote:
> Hi Sakari,
> Thanks for your feedback.
> 
> On Fri, Jun 09, 2023 at 08:35:20AM +0000, Sakari Ailus wrote:
> > Hi Tommaso,
> > 
> > On Thu, Jun 08, 2023 at 10:31:15AM +0200, Tommaso Merciai wrote:
> > > Add documentation of device tree in YAML schema for the ALVIUM
> > > Camera from Allied Vision Inc.
> > > 
> > > References:
> > >  - https://www.alliedvision.com/en/products/embedded-vision-solutions
> > > 
> > > Signed-off-by: Tommaso Merciai <tomm.merciai@xxxxxxxxx>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > > ---
> > > Changes since v1:
> > >  - Fixed build error as suggested by RHerring bot
> > > 
> > > Changes since v2:
> > >  - Fixed License as suggested by KKozlowski/CDooley
> > >  - Removed rotation property as suggested by CDooley/LPinchart
> > >  - Fixed example node name as suggested by CDooley
> > >  - Fixed title as suggested by LPinchart
> > >  - Fixed compatible name as suggested by LPinchart
> > >  - Removed clock as suggested by LPinchart
> > >  - Removed gpios not as suggested by LPinchart
> > >  - Renamed property name streamon-delay into alliedvision,lp2hs-delay-us
> > >  - Fixed vendor prefix, unit append as suggested by KKozlowski
> > >  - Fixed data-lanes
> > >  - Fixed blank space + example indentation (from 6 -> 4 space) as suggested by KKozlowski
> > >  - Dropped status into example  as suggested by KKozlowski
> > >  - Added vcc-ext-in supply as suggested by LPinchart
> > >  - Dropped pinctrl into example as suggested by LPinchart
> > > 
> > > Changes since v3:
> > >  - Fixed vcc-ext-in-supply description as suggested by LPinchart
> > >  - Fixed alliedvision,lp2hs-delay-us description as suggested by LPinchart
> > >  - Added maximum to alliedvision,lp2hs-delay-us as suggested by LPinchart
> > >  - Collected Reviewed-by tag from LPinchart
> > > 
> > > Changes since v4:
> > >  - Fixed id as reported by RHerring bot and LPinchart
> > >  - Add minimum into alliedvision,lp2hs-delay-us as suggested by CDooley
> > >  - Tested using make dt_binding_check DT_SCHEMA_FILES=alliedvision,alvium-csi2.yaml
> > > 
> > >  .../media/i2c/alliedvision,alvium-csi2.yaml   | 97 +++++++++++++++++++
> > >  1 file changed, 97 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml b/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml
> > > new file mode 100644
> > > index 000000000000..c771e5039641
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml
> > > @@ -0,0 +1,97 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/media/i2c/alliedvision,alvium-csi2.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Allied Vision Alvium Camera
> > > +
> > > +maintainers:
> > > +  - Tommaso Merciai <tomm.merciai@xxxxxxxxx>
> > > +  - Martin Hecht <martin.hecht@xxxxxxxx>
> > > +
> > > +allOf:
> > > +  - $ref: /schemas/media/video-interface-devices.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: alliedvision,alvium-csi2
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  vcc-ext-in-supply:
> > > +    description: |
> > > +      The regulator that supplies power to the VCC_EXT_IN pins.
> > > +
> > > +  alliedvision,lp2hs-delay-us:
> > > +    minimum: 1
> > > +    maximum: 150000
> > > +    description: |
> > > +      Low power to high speed delay time.
> > > +
> > > +      If the value is larger than 0, the camera forces a reset of all
> > > +      D-PHY lanes for the duration specified by this property. All lanes
> > > +      will transition to the low-power state and back to the high-speed
> > > +      state after the delay. Otherwise the lanes will transition to and
> > > +      remain in the high-speed state immediately after power on.
> > > +
> > > +      This is meant to help CSI-2 receivers synchronizing their D-PHY
> > > +      RX.
> > 
> > Why do you need this? D-PHY TX is obviously controlled by the driver.
> > Explicit control of PHY initialisation is of course best option when
> > combined with some receivers, done via driver's pre_streamon and s_stream
> > callbacks. Can't your hardware do that?
> 
> You mean calculate this time in some dynamical way and program this
> during pre_streamon or s_stream?
> 
> If yes, unfortunately hw can't do this.

This shouldn't be based on timing but rather progress of the bus
initialisation sequence. That's how it works on most sensors nowadays.

The description is also lacking details on what delay this exactly is. What
is the bus state during that delay?

-- 
Regards,

Sakari Ailus