Message-ID: 
 <TY3PR01MB99822E6161ED319B4DE855B492042@TY3PR01MB9982.jpnprd01.prod.outlook.com>
Date: Tue, 17 Dec 2024 00:00:16 +0000
From: <yuji2.ishikawa@...hiba.co.jp>
To: <krzk@...nel.org>, <laurent.pinchart@...asonboard.com>,
        <mchehab@...nel.org>, <robh@...nel.org>, <krzk+dt@...nel.org>,
        <conor+dt@...nel.org>, <sakari.ailus@...ux.intel.com>,
        <hverkuil-cisco@...all.nl>, <nobuhiro1.iwamatsu@...hiba.co.jp>
CC: <linux-media@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <devicetree@...r.kernel.org>
Subject: RE: [PATCH v12 2/8] dt-bindings: media: platform: visconti: Add
 Toshiba Visconti Video Input Interface

Hello Krzysztof

Thank you for your review

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@...nel.org>
> Sent: Monday, November 25, 2024 7:08 PM
> To: ishikawa yuji(石川 悠司 ○ＲＤＣ□ＡＩＴＣ○ＥＡ開)
> <yuji2.ishikawa@...hiba.co.jp>; Laurent Pinchart
> <laurent.pinchart@...asonboard.com>; Mauro Carvalho Chehab
> <mchehab@...nel.org>; Rob Herring <robh@...nel.org>; Krzysztof Kozlowski
> <krzk+dt@...nel.org>; Conor Dooley <conor+dt@...nel.org>; Sakari Ailus
> <sakari.ailus@...ux.intel.com>; Hans Verkuil <hverkuil-cisco@...all.nl>;
> iwamatsu nobuhiro(岩松 信洋 ○ＤＩＴＣ□ＤＩＴ○ＯＳＴ)
> <nobuhiro1.iwamatsu@...hiba.co.jp>
> Cc: linux-media@...r.kernel.org; linux-kernel@...r.kernel.org;
> linux-arm-kernel@...ts.infradead.org; devicetree@...r.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@...hiba.co.jp>
> > Reviewed-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@...hiba.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.
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?

> > ---
> > 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?
> 

My comment was incorrect.
It should be "Driver does not use the information"

> > - 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.
>

Sorry for poor quality.
Several changes were brought about by the introduction of the CSI2RX driver.
The new driver was requested in the previous review.
The property "clock-noncontinuous" was pointed out as unnecessary (because HW supports both modes) in the previous review.
The property "link-frequencies" was dropped because It was found to be unused after re-check of the code.

> 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@...hiba.co.jp>'
> Grabbing thread from
> lore.kernel.org/all/20241125092146.1561901-3-yuji2.ishikawa@...hiba.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?
> 
> 

I will use b4 tool for further submits.
Also I'll add links to previous versions in the cover letter.

I ran b4 diff with the HEAD of media_stage tree and confirmed that it produced non-error results.
I needed to fetch the tree to a certain depth to ensure everything worked smoothly.

$ git clone https://git.linuxtv.org/media_stage.git --depth 1
$ git fetch --shallow-exclude v6.4
$ git log --oneline | head -5
40384c840ea1 Linux 6.13-rc1
a14bf463e7df Merge tag 'i2c-for-6.13-rc1-part3' of git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux
88862eeb4763 Merge tag 'trace-printf-v6.13' of git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace
f788b5ef1ca9 Merge tag 'timers_urgent_for_v6.13_rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip
63f4993b792e Merge tag 'irq_urgent_for_v6.13_rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip

$ b4 diff '<20241125092146.1561901-3-yuji2.ishikawa@...hiba.co.jp>'

> >
> >  .../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.y
> > +++ aml
> > @@ -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@...hiba.co.jp>
> > +
> > +description: |-
> 
> Since you ask for re-review, then:
> 
> Drop |-
> 

I'll drop "|-"

> Best regards,
> Krzysztof

Best regards,
Yuji Ishikawa

Powered by blists - more mailing lists