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]
Date: Wed, 10 Jan 2024 11:39:53 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Dharma Balasubiramani <dharma.b@...rochip.com>, sam@...nborg.org,
 bbrezillon@...nel.org, maarten.lankhorst@...ux.intel.com,
 mripard@...nel.org, tzimmermann@...e.de, airlied@...il.com, daniel@...ll.ch,
 robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
 nicolas.ferre@...rochip.com, alexandre.belloni@...tlin.com,
 claudiu.beznea@...on.dev, dri-devel@...ts.freedesktop.org,
 devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, lee@...nel.org, thierry.reding@...il.com,
 u.kleine-koenig@...gutronix.de, linux-pwm@...r.kernel.org
Subject: Re: [PATCH 1/3] dt-bindings: display: convert Atmel's HLCDC to DT
 schema

On 10/01/2024 11:25, Dharma Balasubiramani wrote:
> Convert the existing DT binding to DT schema of the Atmel's HLCDC display
> controller.
> 
> Signed-off-by: Dharma Balasubiramani <dharma.b@...rochip.com>
> ---
>  .../display/atmel/atmel,hlcdc-dc.yaml         | 133 ++++++++++++++++++
>  .../bindings/display/atmel/hlcdc-dc.txt       |  75 ----------
>  2 files changed, 133 insertions(+), 75 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/display/atmel/atmel,hlcdc-dc.yaml
>  delete mode 100644 Documentation/devicetree/bindings/display/atmel/hlcdc-dc.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/atmel/atmel,hlcdc-dc.yaml b/Documentation/devicetree/bindings/display/atmel/atmel,hlcdc-dc.yaml
> new file mode 100644
> index 000000000000..49ef28646c48
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/atmel/atmel,hlcdc-dc.yaml
> @@ -0,0 +1,133 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright (C) 2024 Microchip Technology, Inc. and its subsidiaries

What about original copyrights from TXT file? Although conversion is
quite independent, I could imagine some lawyer would call it a
derivative work of original TXT.

If you decide to add explicit copyrights (which anyway I do not
understand why), then please make it signed off by some of your lawyers
to be sure that you really claim that, in respect of other people
copyrights.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/atmel/atmel,hlcdc-dc.yaml#

Filename like compatible.

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Atmel's HLCDC (High LCD Controller) DRM driver

Driver as Linux driver? Not suitable for bindings, so please drop.

> +
> +maintainers:
> +  - Nicolas Ferre <nicolas.ferre@...rochip.com>
> +  - Alexandre Belloni <alexandre.belloni@...tlin.com>
> +  - Claudiu Beznea <claudiu.beznea@...on.dev>
> +
> +description: |
> +  Device-Tree bindings for Atmel's HLCDC DRM driver. The Atmel HLCDC Display

Drop entire first sentence and instead describe hardware.

> +  Controller is a subdevice of the HLCDC MFD device.
> +  # See ../../mfd/atmel,hlcdc.yaml for more details.

Full paths please.

> +
> +properties:
> +  compatible:
> +    const: atmel,hlcdc-display-controller
> +
> +  pinctrl-names:
> +    const: default
> +
> +  pinctrl-0: true

Why do you need these two? Are they really required?

> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  port@0:
> +    $ref: /schemas/graph.yaml#/$defs/port-base
> +    unevaluatedProperties: false
> +    description:
> +      Output endpoint of the controller, connecting the LCD panel signals.
> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +
> +      '#size-cells':
> +        const: 0
> +
> +      reg:
> +        maxItems: 1
> +
> +      endpoint:
> +        $ref: /schemas/graph.yaml#/$defs/endpoint-base

Hm, why do you reference endpoint-base? This looks oddly different than
all other bindings for such devices, so please explain why.

> +        unevaluatedProperties: false
> +        description:
> +          Endpoint connecting the LCD panel signals.
> +
> +        properties:
> +          bus-width:
> +            description: |
> +              Any endpoint grandchild node may specify a desired video interface according to
> +              ../../media/video-interfaces.yaml, specifically "bus-width" whose recognized

Drop redundant information. Don't you miss some $ref?


> +              values are <12>, <16>, <18> and <24>, and override any output mode selection
> +              heuristic, forcing "rgb444","rgb565", "rgb666" and "rgb888" respectively.
> +            enum: [ 12, 16, 18, 24 ]
> +
> +additionalProperties: false

This goes after required:

> +
> +required:
> +  - '#address-cells'
> +  - '#size-cells'
> +  - compatible
> +  - pinctrl-names
> +  - pinctrl-0
> +  - port@0
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/at91.h>
> +    #include <dt-bindings/dma/at91.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    //Example 1

Drop

> +    hlcdc: hlcdc@...30000 {

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


> +      compatible = "atmel,sama5d3-hlcdc";
> +      reg = <0xf0030000 0x2000>;
> +      interrupts = <36 IRQ_TYPE_LEVEL_HIGH 0>;
> +      clocks = <&lcdc_clk>, <&lcdck>, <&clk32k>;
> +      clock-names = "periph_clk","sys_clk", "slow_clk";

This part does not look related... If this is part of other device,
usually it is enough to have just one complete example.

Also, fix coding style - space after ,

> +
> +      hlcdc-display-controller {
> +        compatible = "atmel,hlcdc-display-controller";
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb888>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        port@0 {
> +          #address-cells = <1>;
> +          #size-cells = <0>;
> +          reg = <0>;
> +
> +          hlcdc_panel_output: endpoint@0 {
> +            reg = <0>;
> +            remote-endpoint = <&panel_input>;
> +          };
> +        };
> +      };
> +
> +      hlcdc_pwm: hlcdc-pwm {
> +        compatible = "atmel,hlcdc-pwm";

How is this related?

> +        pinctrl-names = "default";
> +        pinctrl-0 = <&pinctrl_lcd_pwm>;
> +        #pwm-cells = <3>;
> +      };
> +    };
> +  - |
> +    //Example 2 With a video interface override to force rgb565
> +    hlcdc-display-controller {
> +      pinctrl-names = "default";
> +      pinctrl-0 = <&pinctrl_lcd_base &pinctrl_lcd_rgb565>;

And how is this? Where is the compatible? Maybe just drop second
example, what are the differences?

Are you sure your Microchip folks reviewed it before?

Best regards,
Krzysztof


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ