| Message ID | 20241125092146.1561901-3-yuji2.ishikawa@toshiba.co.jp |
|---|---|
| State | New |
| Headers | show |
| Series | Add Toshiba Visconti Video Input Interface driver | expand |
On 25/11/2024 10:21, Yuji Ishikawa wrote: > Adds the Device Tree binding documentation that allows to describe > the Video Input Interface found in Toshiba Visconti SoCs. > > Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@toshiba.co.jp> > Reviewed-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> Why this tag stayed and other was removed? What was the reason of tag removal? > --- > Changelog v2: > - no change > > Changelog v3: > - no change > > Changelog v4: > - fix style problems at the v3 patch > - remove "index" member > - update example > > Changelog v5: > - no change > > Changelog v6: > - add register definition of BUS-IF and MPU > > Changelog v7: > - remove trailing "bindings" from commit header message > - remove trailing "Device Tree Bindings" from title > - fix text wrapping of description > - change compatible to visconti5-viif > - explicitly define allowed properties for port::endpoint > > Changelog v8: > - Suggestion from Krzysztof Kozlowski > - rename bindings description file > - use block style array instead of inline style > - remove clock-lane (as it is fixed at position 0) > - update sample node's name > - use lowercase hex for literals > - Suggestion from Laurent Pinchart > - update description message port::description > - remove port::endpoint::bus-type as it is fixed to <4> > - remove port::endpoint::clock-lanes from example > - add port::endpoint::data-lanes to required parameters list > - fix sequence of data-lanes: <1 2 3 4> because current driver does not support data reordering > - update port::endpoint::data-lanes::description > - remove redundant type definition for port::endpoint::data-lanes > > Changelog v9: > - place "required" after "properties" > - dictionary ordering of properties > > Changelog v10: > - no change > > Changelog v11: > - no change > > Changelog v12: > - remove property "clock-noncontinuous" as VIIF switches both modes automatically > - remove property "link-frequencies" as VIIF does not use the information Driver does not use or hardware supports only one frequency? > - remove reg[2] and interrupts[3] which are used for CSI2RX driver > - update example to refer csi2rx for remote-endpoint Were these the reasons? I am really surprised that after 11 versions this binding still is being totally reshaped and you need us to re-review. Also, start using b4 tool, so: 1. your cover letter will have proper links to previous versions 2. b4 diff would work. Look, try by yourself: b4 diff '<20241125092146.1561901-3-yuji2.ishikawa@toshiba.co.jp>' Grabbing thread from lore.kernel.org/all/20241125092146.1561901-3-yuji2.ishikawa@toshiba.co.jp/t.mbox.gz Checking for older revisions Grabbing search results from lore.kernel.org Added from v11: 7 patches --- Analyzing 144 messages in the thread Looking for additional code-review trailers on lore.kernel.org Analyzing 13 code-review messages Preparing fake-am for v11: Add Toshiba Visconti Video Input Interface driver range: e16e149ced15..4efb0d6768a6 Preparing fake-am for v12: dt-bindings: media: platform: visconti: Add Toshiba Visconti MIPI CSI-2 Receiver ERROR: v12 series incomplete; unable to create a fake-am range --- Could not create fake-am range for upper series v12 How can I verify what happened here without too much effort? > > .../media/toshiba,visconti5-viif.yaml | 95 +++++++++++++++++++ > 1 file changed, 95 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/toshiba,visconti5-viif.yaml > > diff --git a/Documentation/devicetree/bindings/media/toshiba,visconti5-viif.yaml b/Documentation/devicetree/bindings/media/toshiba,visconti5-viif.yaml > new file mode 100644 > index 000000000000..ef0452a47e98 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/toshiba,visconti5-viif.yaml > @@ -0,0 +1,95 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/toshiba,visconti5-viif.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Toshiba Visconti5 SoC Video Input Interface > + > +maintainers: > + - Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> > + > +description: |- Since you ask for re-review, then: Drop |- Best regards, Krzysztof
On 17/12/2024 10:45, Laurent Pinchart wrote: > On Tue, Dec 17, 2024 at 06:43:22AM +0100, Krzysztof Kozlowski wrote: >> On 17/12/2024 01:00, yuji2.ishikawa@toshiba.co.jp wrote: >>> Hello Krzysztof >>> >>> Thank you for your review >>> >>>> -----Original Message----- >>>> From: Krzysztof Kozlowski <krzk@kernel.org> >>>> Sent: Monday, November 25, 2024 7:08 PM >>>> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開) >>>> <yuji2.ishikawa@toshiba.co.jp>; Laurent Pinchart >>>> <laurent.pinchart@ideasonboard.com>; Mauro Carvalho Chehab >>>> <mchehab@kernel.org>; Rob Herring <robh@kernel.org>; Krzysztof Kozlowski >>>> <krzk+dt@kernel.org>; Conor Dooley <conor+dt@kernel.org>; Sakari Ailus >>>> <sakari.ailus@linux.intel.com>; Hans Verkuil <hverkuil-cisco@xs4all.nl>; >>>> iwamatsu nobuhiro(岩松 信洋 ○ＤＩＴＣ□ＤＩＴ○ＯＳＴ) >>>> <nobuhiro1.iwamatsu@toshiba.co.jp> >>>> Cc: linux-media@vger.kernel.org; linux-kernel@vger.kernel.org; >>>> linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org >>>> Subject: Re: [PATCH v12 2/8] dt-bindings: media: platform: visconti: Add >>>> Toshiba Visconti Video Input Interface >>>> >>>> On 25/11/2024 10:21, Yuji Ishikawa wrote: >>>>> Adds the Device Tree binding documentation that allows to describe the >>>>> Video Input Interface found in Toshiba Visconti SoCs. >>>>> >>>>> Signed-off-by: Yuji Ishikawa <yuji2.ishikawa@toshiba.co.jp> >>>>> Reviewed-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> >>>> >>>> Why this tag stayed and other was removed? What was the reason of tag >>>> removal? >>>> >>> >>> The stayed tag is due to internal review. >> >> Did the internal review really happened? How is it that immediately new >> version has internal review without any traces? >> >> I have doubts this review happened in the context of reviewer's >> statement of oversight. >> >>> The removed tag is due to code's change (split of csi2rx part) after the last review. >>> If the code is largely changed following the instruction of another reviewer >>> after obtaining the tags, how should the tags be handled? >> >> Drop all reviews and perform reviews on the list. >> >> Such internal review appearing afterwards is rather a proof it you are >> adding just the tags to satisfy your process. I have no way to even >> verify whether that person performed any reasonable review or maybe just >> acked your patch. > > How do you verify that for public reviews ? By quality or amount of comments. Or timing. Or reviewing cover letter without any feedback on individual patches. There are many, many ways. Considering how many companies were adding fake manager-review-tags in the past (or fake SoBs), I am pretty picky on that. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/media/toshiba,visconti5-viif.yaml b/Documentation/devicetree/bindings/media/toshiba,visconti5-viif.yaml new file mode 100644 index 000000000000..ef0452a47e98 --- /dev/null +++ b/Documentation/devicetree/bindings/media/toshiba,visconti5-viif.yaml @@ -0,0 +1,95 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/media/toshiba,visconti5-viif.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Toshiba Visconti5 SoC Video Input Interface + +maintainers: + - Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp> + +description: |- + Toshiba Visconti5 SoC Video Input Interface (VIIF) receives videostream + from MIPI CSI-2 receiver device, processes the stream with image signal + processors (L1ISP, L2ISP), then stores pictures to main memory. + +properties: + compatible: + const: toshiba,visconti5-viif + + reg: + items: + - description: Registers for capture control + - description: Registers for bus interface unit control + - description: Registers for Memory Protection Unit + + interrupts: + items: + - description: Sync Interrupt + - description: Status (Error) Interrupt + - description: L1ISP Interrupt + + port: + $ref: /schemas/graph.yaml#/$defs/port-base + unevaluatedProperties: false + description: CSI-2 input port, with a single endpoint connected to the CSI-2 transmitter. + + properties: + endpoint: + $ref: video-interfaces.yaml# + additionalProperties: false + + properties: + data-lanes: + description: VIIF supports 1, 2, 3 or 4 data lanes + minItems: 1 + items: + - const: 1 + - const: 2 + - const: 3 + - const: 4 + + remote-endpoint: true + + required: + - data-lanes + - remote-endpoint + +required: + - compatible + - reg + - interrupts + - port + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/interrupt-controller/irq.h> + + soc { + #address-cells = <2>; + #size-cells = <2>; + + video@1c000000 { + compatible = "toshiba,visconti5-viif"; + reg = <0 0x1c000000 0 0x6000>, + <0 0x1c00e000 0 0x1000>, + <0 0x2417a000 0 0x1000>; + interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>; + + port { + #address-cells = <1>; + #size-cells = <0>; + + csi_in0: endpoint { + data-lanes = <1 2>; + remote-endpoint = <&csi2rx_out0>; + }; + }; + }; + };