[v3] porting pq compnent for MT8196

[PATCH v3 4/6] dt-bindings: display: mediatek: disp-tdshp: Add support for MT8196

Posted by Jay Liu 4 months, 1 week ago

Add disp-tdshp hardware description for MediaTek MT8196 SoC

Signed-off-by: Jay Liu <jay.liu@mediatek.com>
---
 .../display/mediatek/mediatek,disp-tdshp.yaml | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/mediatek/mediatek,disp-tdshp.yaml

diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp-tdshp.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp-tdshp.yaml
new file mode 100644
index 000000000000..94aa33a2a5ed
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp-tdshp.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/mediatek/mediatek,disp-tdshp.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek display 2D sharpness processor
+
+maintainers:
+  - Chun-Kuang Hu <chunkuang.hu@kernel.org>
+  - Philipp Zabel <p.zabel@pengutronix.de>
+
+description: |
+  MediaTek display 2D sharpness processor, namely TDSHP, provides a
+  operation used to adjust sharpness in display system.
+  TDSHP device node must be siblings to the central MMSYS_CONFIG node.
+  For a description of the MMSYS_CONFIG binding, see
+  Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
+  for details.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - mediatek,mt8196-disp-tdshp
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      disp-tdshp@321e0000 {
+          compatible = "mediatek,mt8196-disp-tdshp";
+          reg = <0 0x321e0000 0 0x1000>;
+          clocks = <&dispsys_config_clk 107>;
+      };
+    };
-- 
2.46.0

Re: [PATCH v3 4/6] dt-bindings: display: mediatek: disp-tdshp: Add support for MT8196

Posted by Krzysztof Kozlowski 4 months ago

On Fri, Aug 08, 2025 at 08:53:59PM +0800, Jay Liu wrote:
> Add disp-tdshp hardware description for MediaTek MT8196 SoC
> 
> Signed-off-by: Jay Liu <jay.liu@mediatek.com>
> ---
>  .../display/mediatek/mediatek,disp-tdshp.yaml | 52 +++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/mediatek/mediatek,disp-tdshp.yaml

We expect filename based on compatibles (see writing bindings).

> 
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp-tdshp.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp-tdshp.yaml
> new file mode 100644
> index 000000000000..94aa33a2a5ed
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp-tdshp.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/mediatek/mediatek,disp-tdshp.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek display 2D sharpness processor
> +
> +maintainers:
> +  - Chun-Kuang Hu <chunkuang.hu@kernel.org>
> +  - Philipp Zabel <p.zabel@pengutronix.de>
> +
> +description: |
> +  MediaTek display 2D sharpness processor, namely TDSHP, provides a
> +  operation used to adjust sharpness in display system.
> +  TDSHP device node must be siblings to the central MMSYS_CONFIG node.

Heh? Why would this have to be a sibling? This is really odd, because
you cannto actually rely on that.

You just added unverifiable unusual ABI with no explanation.


> +  For a description of the MMSYS_CONFIG binding, see
> +  Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.yaml
> +  for details.
> +
> +properties:
> +  compatible:
> +    items:

Drop, just enum.

> +      - enum:
> +          - mediatek,mt8196-disp-tdshp
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +

Drop blank line

> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;

messed indentation

> +
> +      disp-tdshp@321e0000 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

e.g. image-processor

Best regards,
Krzysztof

Re: [PATCH v3 4/6] dt-bindings: display: mediatek: disp-tdshp: Add support for MT8196

Posted by Jay Liu (刘博) 2 months, 3 weeks ago

On Mon, 2025-08-11 at 09:52 +0200, Krzysztof Kozlowski wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Fri, Aug 08, 2025 at 08:53:59PM +0800, Jay Liu wrote:
> > Add disp-tdshp hardware description for MediaTek MT8196 SoC
> > 
> > Signed-off-by: Jay Liu <jay.liu@mediatek.com>
> > ---
> >  .../display/mediatek/mediatek,disp-tdshp.yaml | 52
> > +++++++++++++++++++
> >  1 file changed, 52 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/display/mediatek/mediatek,disp-
> > tdshp.yaml
> 
> We expect filename based on compatibles (see writing bindings).
> 
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp-
> > tdshp.yaml
> > b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp-
> > tdshp.yaml
> > new file mode 100644
> > index 000000000000..94aa33a2a5ed
> > --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp-
> > tdshp.yaml
> > @@ -0,0 +1,52 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: 
> > https://urldefense.com/v3/__http://devicetree.org/schemas/display/mediatek/mediatek,disp-tdshp.yaml*__;Iw!!CTRNKA9wMg0ARbw!iQm_1HzyfT_nSaK-KNbXByLmW5dXythQCPEJzW-_ibXiLJyOKzw08MgQui4GKRmJ3QjHIj0zl1x1$
> > +$schema: 
> > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!iQm_1HzyfT_nSaK-KNbXByLmW5dXythQCPEJzW-_ibXiLJyOKzw08MgQui4GKRmJ3QjHIsmf4urv$
> > +
> > +title: MediaTek display 2D sharpness processor
> > +
> > +maintainers:
> > +  - Chun-Kuang Hu <chunkuang.hu@kernel.org>
> > +  - Philipp Zabel <p.zabel@pengutronix.de>
> > +
> > +description: |
> > +  MediaTek display 2D sharpness processor, namely TDSHP, provides
> > a
> > +  operation used to adjust sharpness in display system.
> > +  TDSHP device node must be siblings to the central MMSYS_CONFIG
> > node.
> 
> Heh? Why would this have to be a sibling? This is really odd, because
> you cannto actually rely on that.
> 
hi Krzysztof,
disp-tdshp is one of components of display pipeline. just like OVL,
color, etc. MMSYS_CONFIG controls the clock of display pipeline,

> You just added unverifiable unusual ABI with no explanation.
> 
> 
> > +  For a description of the MMSYS_CONFIG binding, see
> > +  Documentation/devicetree/bindings/arm/mediatek/mediatek,mmsys.ya
> > ml
> > +  for details.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> 
> Drop, just enum.
> 
> > +      - enum:
> > +          - mediatek,mt8196-disp-tdshp
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +
> 
> Drop blank line
> 
> > +    soc {
> > +      #address-cells = <2>;
> > +      #size-cells = <2>;
> 
> messed indentation
> 
> > +
> > +      disp-tdshp@321e0000 {
> 
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> 
https://urldefense.com/v3/__https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html*generic-names-recommendation__;Iw!!CTRNKA9wMg0ARbw!iQm_1HzyfT_nSaK-KNbXByLmW5dXythQCPEJzW-_ibXiLJyOKzw08MgQui4GKRmJ3QjHIjQOIUWC$
> 
> e.g. image-processor
> 
> Best regards,
> Krzysztof
>

Re: [PATCH v3 4/6] dt-bindings: display: mediatek: disp-tdshp: Add support for MT8196

Posted by Krzysztof Kozlowski 2 months, 1 week ago

On 21/09/2025 13:39, Jay Liu (刘博) wrote:
>>> +
>>> +maintainers:
>>> +  - Chun-Kuang Hu <chunkuang.hu@kernel.org>
>>> +  - Philipp Zabel <p.zabel@pengutronix.de>
>>> +
>>> +description: |
>>> +  MediaTek display 2D sharpness processor, namely TDSHP, provides
>>> a
>>> +  operation used to adjust sharpness in display system.
>>> +  TDSHP device node must be siblings to the central MMSYS_CONFIG
>>> node.
>>
>> Heh? Why would this have to be a sibling? This is really odd, because
>> you cannto actually rely on that.
>>
> hi Krzysztof,
> disp-tdshp is one of components of display pipeline. just like OVL,
> color, etc. MMSYS_CONFIG controls the clock of display pipeline,


Read feedback again.

Unfortunately Mediatek produces way too many poor quality patches and
review has so many obstacles like not addressing the comments. Example
above. You need to pay more attention and invest more time, before
pinging or replying to maintainers.

> 
>> You just added unverifiable unusual ABI with no explanation.

Best regards,
Krzysztof