[PATCH v12 2/8] dt-bindings: media: platform: visconti: Add Toshiba Visconti Video Input Interface

Mon Dec 16 16:00:16 PST 2024

Hello Krzysztof

Thank you for your review

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

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 at toshiba.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 at toshiba.co.jp>
> > +
> > +description: |-
> 
> Since you ask for re-review, then:
> 
> Drop |-
> 

I'll drop "|-"

> Best regards,
> Krzysztof

Best regards,
Yuji Ishikawa