| Message ID | 20230607131936.382406-3-tomm.merciai@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: Add support for alvium camera | expand |
On Wed, Jun 07, 2023 at 03:19:24PM +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@gmail.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > 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 > > .../media/i2c/alliedvision,alvium-csi2.yaml | 96 +++++++++++++++++++ > 1 file changed, 96 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..4726d0068229 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml > @@ -0,0 +1,96 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/i2c/alliedvision,alvium.yaml# This is still wrong, see Rob's bot reply in the previous version. > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Allied Vision Alvium Camera > + > +maintainers: > + - Tommaso Merciai <tomm.merciai@gmail.com> > + - Martin Hecht <martin.hecht@avnet.eu> > + > +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: > + 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. > + > + port: > + description: Digital Output Port > + $ref: /schemas/graph.yaml#/$defs/port-base > + additionalProperties: false > + > + properties: > + endpoint: > + $ref: /schemas/media/video-interfaces.yaml# > + unevaluatedProperties: false > + > + properties: > + link-frequencies: true > + > + data-lanes: > + minItems: 1 > + items: > + - const: 1 > + - const: 2 > + - const: 3 > + - const: 4 > + > + required: > + - data-lanes > + - link-frequencies > + > +required: > + - compatible > + - reg > + - vcc-ext-in-supply > + - port > + > +additionalProperties: false > + > +examples: > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + alvium: camera@3c { > + compatible = "alliedvision,alvium-csi2"; > + reg = <0x3c>; > + vcc-ext-in-supply = <®_vcc_ext_in>; > + alliedvision,lp2hs-delay-us = <20>; > + > + port { > + alvium_out: endpoint { > + remote-endpoint = <&mipi_csi_0_in>; > + data-lanes = <1 2 3 4>; > + link-frequencies = /bits/ 64 <681250000>; > + }; > + }; > + }; > + }; > + > +...
On 07/06/2023 15:19, 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@gmail.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > 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 You still did not test it before sending. Four versions of which none were tested :( Best regards, Krzysztof
On Wed, Jun 07, 2023 at 05:06:38PM +0300, Laurent Pinchart wrote: > On Wed, Jun 07, 2023 at 03:19:24PM +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@gmail.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > 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 > > > > .../media/i2c/alliedvision,alvium-csi2.yaml | 96 +++++++++++++++++++ > > 1 file changed, 96 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..4726d0068229 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml > > @@ -0,0 +1,96 @@ > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/i2c/alliedvision,alvium.yaml# > > This is still wrong, see Rob's bot reply in the previous version. Arg... :'( Thanks Laurent. Regards, Tommaso > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Allied Vision Alvium Camera > > + > > +maintainers: > > + - Tommaso Merciai <tomm.merciai@gmail.com> > > + - Martin Hecht <martin.hecht@avnet.eu> > > + > > +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: > > + 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. > > + > > + port: > > + description: Digital Output Port > > + $ref: /schemas/graph.yaml#/$defs/port-base > > + additionalProperties: false > > + > > + properties: > > + endpoint: > > + $ref: /schemas/media/video-interfaces.yaml# > > + unevaluatedProperties: false > > + > > + properties: > > + link-frequencies: true > > + > > + data-lanes: > > + minItems: 1 > > + items: > > + - const: 1 > > + - const: 2 > > + - const: 3 > > + - const: 4 > > + > > + required: > > + - data-lanes > > + - link-frequencies > > + > > +required: > > + - compatible > > + - reg > > + - vcc-ext-in-supply > > + - port > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + alvium: camera@3c { > > + compatible = "alliedvision,alvium-csi2"; > > + reg = <0x3c>; > > + vcc-ext-in-supply = <®_vcc_ext_in>; > > + alliedvision,lp2hs-delay-us = <20>; > > + > > + port { > > + alvium_out: endpoint { > > + remote-endpoint = <&mipi_csi_0_in>; > > + data-lanes = <1 2 3 4>; > > + link-frequencies = /bits/ 64 <681250000>; > > + }; > > + }; > > + }; > > + }; > > + > > +... > > -- > Regards, > > Laurent Pinchart
On Wed, 07 Jun 2023 15:19:24 +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@gmail.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > 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 > > .../media/i2c/alliedvision,alvium-csi2.yaml | 96 +++++++++++++++++++ > 1 file changed, 96 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: ./Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml: $id: relative path/filename doesn't match actual path or filename expected: http://devicetree.org/schemas/media/i2c/alliedvision,alvium-csi2.yaml# doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230607131936.382406-3-tomm.merciai@gmail.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
Hi Krzysztof, On Wed, Jun 07, 2023 at 04:18:48PM +0200, Krzysztof Kozlowski wrote: > On 07/06/2023 15:19, 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@gmail.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > 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 > > You still did not test it before sending. Four versions of which none > were tested :( You are right.. my bad. :'( After fixing id, as suggested by Laurent/bot into: $id: http://devicetree.org/schemas/media/i2c/alliedvision,alvium-csi2.yaml# I'm running the following test: make dt_binding_check DT_SCHEMA_FILES=alliedvision,alvium-csi2.yaml With the following result: LINT Documentation/devicetree/bindings CHKDT Documentation/devicetree/bindings/processed-schema.json SCHEMA Documentation/devicetree/bindings/processed-schema.json /home/tom/work/mainline/linux/Documentation/devicetree/bindings/media/i2c/.alliedvision,alvium-csi2.example.dts.pre.yaml: ignoring, error parsing file DTEX Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.example.dts DTC_CHK Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.example.dtb Is that correct? Let me know. Thanks in advance :) Regards, Tommaso > > Best regards, > Krzysztof >
On 07/06/2023 17:20, Tommaso Merciai wrote: > Hi Krzysztof, > > On Wed, Jun 07, 2023 at 04:18:48PM +0200, Krzysztof Kozlowski wrote: >> On 07/06/2023 15:19, 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@gmail.com> >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>> --- >>> 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 >> >> You still did not test it before sending. Four versions of which none >> were tested :( > > You are right.. my bad. :'( > > After fixing id, as suggested by Laurent/bot into: > > $id: http://devicetree.org/schemas/media/i2c/alliedvision,alvium-csi2.yaml# > > I'm running the following test: > > make dt_binding_check DT_SCHEMA_FILES=alliedvision,alvium-csi2.yaml > > With the following result: > > LINT Documentation/devicetree/bindings > CHKDT Documentation/devicetree/bindings/processed-schema.json > SCHEMA Documentation/devicetree/bindings/processed-schema.json > /home/tom/work/mainline/linux/Documentation/devicetree/bindings/media/i2c/.alliedvision,alvium-csi2.example.dts.pre.yaml: ignoring, error parsing file > DTEX Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.example.dts > DTC_CHK Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.example.dtb > > Is that correct? No, it doesn't look correct. You have error parsing your file. Check your yaml file and its example DTSI. Be sure you have also yamlling installed. Best regards, Krzysztof
Hey Tommaso, On Wed, Jun 07, 2023 at 03:19:24PM +0200, Tommaso Merciai wrote: > + alliedvision,lp2hs-delay-us: > + 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. Since this new version was posted before I got a chance to reply, I still don't think it makes sense to allow 0 & then special case it, when testing for the presence of a property is trivial. The property should describe some behaviour/property of the hardware, not be a mechanism to convey what you want to write into registers. I don't really get why you'd not do: If present, 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. > +static int alvium_get_dt_data(struct alvium_dev *alvium) > +{ > + struct device *dev = &alvium->i2c_client->dev; > + struct device_node *node = dev->of_node; > + struct fwnode_handle *endpoint; > + int ret = 0; > + > + if (!node) > + return -EINVAL; > + > + ret = fwnode_property_read_u32(dev_fwnode(dev), > + "alliedvision,lp2hs-delay-us", > + &alvium->lp2hs_delay); > + if (ret) > + dev_info(dev, "lp2hs-delay-us not found\n"); And this print, which I also don't understand the presence of as well behaving driver should be quiet, goes away. Cheers, Conor. > + > + endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL); > + if (!endpoint) { > + dev_err(dev, "endpoint node not found\n"); > + return -EINVAL; > + } > + > + if (v4l2_fwnode_endpoint_alloc_parse(endpoint, &alvium->ep)) { > + dev_err(dev, "could not parse endpoint\n"); > + return 0; > + } > + > + if (alvium->ep.bus_type != V4L2_MBUS_CSI2_DPHY) { > + dev_err(dev, "unsupported bus type\n"); > + return -EINVAL; > + } > + > + if (!alvium->ep.nr_of_link_frequencies) { > + dev_err(dev, "no link frequencies defined"); > + return -EINVAL; > + } > + > + dev_info(dev, "freq: %llu\n", > + alvium->ep.link_frequencies[0]); > + dev_info(dev, "lanes: %d\n", > + alvium->ep.bus.mipi_csi2.num_data_lanes); > + > + return 0; > +}
On Wed, Jun 07, 2023 at 06:11:54PM +0200, Krzysztof Kozlowski wrote: > On 07/06/2023 17:20, Tommaso Merciai wrote: > > Hi Krzysztof, > > > > On Wed, Jun 07, 2023 at 04:18:48PM +0200, Krzysztof Kozlowski wrote: > >> On 07/06/2023 15:19, 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@gmail.com> > >>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> --- > >>> 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 > >> > >> You still did not test it before sending. Four versions of which none > >> were tested :( > > > > You are right.. my bad. :'( > > > > After fixing id, as suggested by Laurent/bot into: > > > > $id: http://devicetree.org/schemas/media/i2c/alliedvision,alvium-csi2.yaml# > > > > I'm running the following test: > > > > make dt_binding_check DT_SCHEMA_FILES=alliedvision,alvium-csi2.yaml > > > > With the following result: > > > > LINT Documentation/devicetree/bindings > > CHKDT Documentation/devicetree/bindings/processed-schema.json > > SCHEMA Documentation/devicetree/bindings/processed-schema.json > > /home/tom/work/mainline/linux/Documentation/devicetree/bindings/media/i2c/.alliedvision,alvium-csi2.example.dts.pre.yaml: ignoring, error parsing file > > DTEX Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.example.dts > > DTC_CHK Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.example.dtb > > > > Is that correct? > > No, it doesn't look correct. You have error parsing your file. Check > your yaml file and its example DTSI. > > Be sure you have also yamlling installed. Thanks for the feedback! Regards, Tommaso > > Best regards, > Krzysztof >
Hi Conor, On Wed, Jun 07, 2023 at 06:16:19PM +0100, Conor Dooley wrote: > Hey Tommaso, > > On Wed, Jun 07, 2023 at 03:19:24PM +0200, Tommaso Merciai wrote: > > > + alliedvision,lp2hs-delay-us: > > + 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. > > Since this new version was posted before I got a chance to reply, I > still don't think it makes sense to allow 0 & then special case it, > when testing for the presence of a property is trivial. My bad, sry. My keyboard is too quick :P > The property should describe some behaviour/property of the hardware, > not be a mechanism to convey what you want to write into registers. > > I don't really get why you'd not do: > If present, 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. > You are suggesting the following solution: minimum: 1 maximum: 150000 Right? Personally I prefer to keep also 0 but never mind is ok also this solution. :) Let me know if I have understood correctly pls. Thanks in advance :) > > +static int alvium_get_dt_data(struct alvium_dev *alvium) > > +{ > > + struct device *dev = &alvium->i2c_client->dev; > > + struct device_node *node = dev->of_node; > > + struct fwnode_handle *endpoint; > > + int ret = 0; > > + > > + if (!node) > > + return -EINVAL; > > + > > + ret = fwnode_property_read_u32(dev_fwnode(dev), > > + "alliedvision,lp2hs-delay-us", > > + &alvium->lp2hs_delay); > > + if (ret) > > + dev_info(dev, "lp2hs-delay-us not found\n"); > > And this print, which I also don't understand the presence of as > well behaving driver should be quiet, goes away. Then you are suggesting to drop this print right? Thanks for your review! Regards, Tommaso > > Cheers, > Conor. > > > + > > + endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL); > > + if (!endpoint) { > > + dev_err(dev, "endpoint node not found\n"); > > + return -EINVAL; > > + } > > + > > + if (v4l2_fwnode_endpoint_alloc_parse(endpoint, &alvium->ep)) { > > + dev_err(dev, "could not parse endpoint\n"); > > + return 0; > > + } > > + > > + if (alvium->ep.bus_type != V4L2_MBUS_CSI2_DPHY) { > > + dev_err(dev, "unsupported bus type\n"); > > + return -EINVAL; > > + } > > + > > + if (!alvium->ep.nr_of_link_frequencies) { > > + dev_err(dev, "no link frequencies defined"); > > + return -EINVAL; > > + } > > + > > + dev_info(dev, "freq: %llu\n", > > + alvium->ep.link_frequencies[0]); > > + dev_info(dev, "lanes: %d\n", > > + alvium->ep.bus.mipi_csi2.num_data_lanes); > > + > > + return 0; > > +}
On Wed, Jun 07, 2023 at 09:49:18PM +0200, Tommaso Merciai wrote: > On Wed, Jun 07, 2023 at 06:16:19PM +0100, Conor Dooley wrote: > > On Wed, Jun 07, 2023 at 03:19:24PM +0200, Tommaso Merciai wrote: > You are suggesting the following solution: > > minimum: 1 > maximum: 150000 > > Right? > > Personally I prefer to keep also 0 but never mind > is ok also this solution. :) > > Let me know if I have understood correctly pls. > Thanks in advance :) Yup, you got it. > > > +static int alvium_get_dt_data(struct alvium_dev *alvium) > > > +{ > > > + struct device *dev = &alvium->i2c_client->dev; > > > + struct device_node *node = dev->of_node; > > > + struct fwnode_handle *endpoint; > > > + int ret = 0; > > > + > > > + if (!node) > > > + return -EINVAL; > > > + > > > + ret = fwnode_property_read_u32(dev_fwnode(dev), > > > + "alliedvision,lp2hs-delay-us", > > > + &alvium->lp2hs_delay); > > > + if (ret) > > > + dev_info(dev, "lp2hs-delay-us not found\n"); > > > > And this print, which I also don't understand the presence of as > > well behaving driver should be quiet, goes away. > > Then you are suggesting to drop this print right? Yeah, in general we do not want drivers to be printing things while they are behaving correctly & not having the property isn't an error ;) Cheers, Conor.
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..4726d0068229 --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/alliedvision,alvium-csi2.yaml @@ -0,0 +1,96 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/i2c/alliedvision,alvium.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Allied Vision Alvium Camera + +maintainers: + - Tommaso Merciai <tomm.merciai@gmail.com> + - Martin Hecht <martin.hecht@avnet.eu> + +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: + 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. + + port: + description: Digital Output Port + $ref: /schemas/graph.yaml#/$defs/port-base + additionalProperties: false + + properties: + endpoint: + $ref: /schemas/media/video-interfaces.yaml# + unevaluatedProperties: false + + properties: + link-frequencies: true + + data-lanes: + minItems: 1 + items: + - const: 1 + - const: 2 + - const: 3 + - const: 4 + + required: + - data-lanes + - link-frequencies + +required: + - compatible + - reg + - vcc-ext-in-supply + - port + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + alvium: camera@3c { + compatible = "alliedvision,alvium-csi2"; + reg = <0x3c>; + vcc-ext-in-supply = <®_vcc_ext_in>; + alliedvision,lp2hs-delay-us = <20>; + + port { + alvium_out: endpoint { + remote-endpoint = <&mipi_csi_0_in>; + data-lanes = <1 2 3 4>; + link-frequencies = /bits/ 64 <681250000>; + }; + }; + }; + }; + +...