linux-kernel - Re: [PATCH v5 1/9] dt-bindings: display: bindings for starfive,JH7110 display pipeline

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fza6u77alzm6ovzptkh7isztycn72pzae2f62kqeyj3f7ognou@lmp7cg4tr3yw>
Date: Thu, 21 Nov 2024 09:23:03 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: keith zhao <keith.zhao@...rfivetech.com>
Cc: devicetree@...r.kernel.org, dri-devel@...ts.freedesktop.org, 
	andrzej.hajda@...el.com, neil.armstrong@...aro.org, rfoss@...nel.org, 
	Laurent.pinchart@...asonboard.com, jonas@...boo.se, jernej.skrabec@...il.com, 
	maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de, airlied@...il.com, 
	simona@...ll.ch, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org, 
	hjc@...k-chips.com, heiko@...ech.de, andy.yan@...k-chips.com, 
	william.qiu@...rfivetech.com, xingyu.wu@...rfivetech.com, kernel@...il.dk, 
	paul.walmsley@...ive.com, palmer@...belt.com, aou@...s.berkeley.edu, 
	p.zabel@...gutronix.de, changhuang.liang@...rfivetech.com, jack.zhu@...rfivetech.com, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 1/9] dt-bindings: display: bindings for
 starfive,JH7110 display pipeline

On Wed, Nov 20, 2024 at 02:18:40PM +0800, keith zhao wrote:
> - Added bindings to support the display subsystem on the JH7110 SoC.

Please do not use "This commit/patch/change" (implied) and past tense,
but imperative mood. See longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> 
> - Included the DC8200 display controller and Inno HDMI controller.
> 
> - Created innosilicon,inno-hdmi.yaml schema containing common properties
>   for the Inno DesignWare HDMI TX controller.
>   This isn't a full device tree binding specification,
>   but is intended to be referenced by platform-specific bindings
>   for the IP core.
> 
> Signed-off-by: keith zhao <keith.zhao@...rfivetech.com>
> ---
>  .../display/bridge/innosilicon,inno-hdmi.yaml |  45 +++++
>  .../display/rockchip/rockchip,inno-hdmi.yaml  |  27 +--
>  .../starfive/starfive,jh7110-dc8200.yaml      | 176 ++++++++++++++++++
>  .../starfive/starfive,jh7110-inno-hdmi.yaml   |  91 +++++++++
>  .../soc/starfive/starfive,jh7110-syscon.yaml  |   1 +

I do not see how you addressed my feedback. I asked you to split the
patch. Where is the split?

Your answer to my request to split was "Background is ...", yeah, so
what? You are not going to split?


>  MAINTAINERS                                   |   8 +
>  6 files changed, 323 insertions(+), 25 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/innosilicon,inno-hdmi.yaml
>  create mode 100644 Documentation/devicetree/bindings/display/starfive/starfive,jh7110-dc8200.yaml
>  create mode 100644 Documentation/devicetree/bindings/display/starfive/starfive,jh7110-inno-hdmi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/innosilicon,inno-hdmi.yaml b/Documentation/devicetree/bindings/display/bridge/innosilicon,inno-hdmi.yaml
> new file mode 100644
> index 000000000000..f2543aebc312
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/innosilicon,inno-hdmi.yaml
> @@ -0,0 +1,45 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/innosilicon,inno-hdmi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Common Properties for Innosilicon HDMI TX IP
> +
> +maintainers:
> +  - keith zhao <keith.zhao@...rfivetech.com>
> +
> +description: |

Do not need '|' unless you need to preserve formatting.



> +  Innosilicon HDMI TX is an HDMI transmission device integrated into the zap SoC.

Keep and extend.

> +  This document specifies the device tree properties for the INNO HDMI IP core.

Keep... but what is INNO HDMI IP core? Another name? Different block?


> +  It is intended to be referenced by platform-specific device tree bindings,
> +  which will determine the necessity of each property.

Not much improved here. Last two sentences are almost useless. Again -
document the hardware. Drop this sentence.

I already complained about this. I already complained that you keep
ignoring my comments and this does not improve much.

Since you did not implement my main feedback I am going to NAK the rest
except one more thing:

...

> +
> +properties:
> +  compatible:
> +    const: "starfive,jh7110-inno-hdmi"

I could not be more specific in my previous feedback. Improvements? No.
Stop wasting our time.

NAK.

Best regards,
Krzysztof