Re: [PATCH v1 1/2] dt-bindings: arm: qcom: Add waveshare MIPI-DSI panels support

[Krzysztof Kozlowski <krzk@kernel.org>]

On 11/11/2025 11:42, Sudarshan Shetty wrote:
> Device tree bindings for Waveshare MIPI-DSI panels
> of various sizes (5.0, 5.5, 7.0, 8.0, and 10.1).

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597

> These panels require proper power sequencing via an external
> regulator and a backlight node for brightness control.
> 
> Signed-off-by: Sudarshan Shetty <tessolveupstream@gmail.com>
> ---
>  .../display/panel/waveshare,dsi-panel.yaml    | 84 +++++++++++++++++++
>  ...waveshare,touchscreen-panel-regulator.yaml | 72 ++++++++++++++++

Do not mix up patches from different subsystems into one patchset.

>  2 files changed, 156 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/waveshare,dsi-panel.yaml
>  create mode 100644 Documentation/devicetree/bindings/regulator/waveshare,touchscreen-panel-regulator.yaml
> 
Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters


> diff --git a/Documentation/devicetree/bindings/display/panel/waveshare,dsi-panel.yaml b/Documentation/devicetree/bindings/display/panel/waveshare,dsi-panel.yaml
> new file mode 100644
> index 000000000000..a42ce065124f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/waveshare,dsi-panel.yaml
> @@ -0,0 +1,84 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/panel/waveshare,dsi-panel.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Waveshare 10.1" DSI Touch Display Panel
> +
> +maintainers:
> +  - Sudarshan Shetty <tessolveupstream@gmail.com>
> +
> +allOf:
> +  - $ref: panel-common.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - waveshare,12.3-dsi-touch-a,4lane
> +      - waveshare,10.1-dsi-touch-a
> +      - waveshare,10.1-dsi-touch-a-4lane
> +      - waveshare,10.1-dsi-touch-b
> +      - waveshare,10.1-dsi-touch-b,4lane
> +      - waveshare,9.0-dsi-touch-b
> +      - waveshare,9.0-dsi-touch-b,4lane
> +      - waveshare,8.8-dsi-touch-a
> +      - waveshare,8.0-dsi-touch-a
> +      - waveshare,8.0-dsi-touch-a-4lane
> +      - waveshare,7.0-dsi-touch-a
> +      - waveshare,7.0-dsi-touch-b
> +      - waveshare,5.5-dsi-touch-a
> +      - waveshare,5.0-dsi-touch-a
> +      - waveshare,4.0-dsi-touch-c
> +      - waveshare,3.4-dsi-touch-c

None of these come with any reasonable model names? How so?

> +
> +  reg:
> +    description: DSI virtual channel
> +    maxItems: 1
> +
> +  vdd-supply:
> +    description: Power supply regulator for the panel
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: GPIO to control panel reset
> +
> +  enable-gpios:
> +    maxItems: 1
> +    description: GPIO to control panel power enable
> +
> +required:
> +  - compatible
> +  - reg
> +  - vdd-supply
> +  - reset-gpios
> +  - enable-gpios
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    dsi@ae94000 {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        panel@1 {
> +            compatible = "waveshare,10.1-dsi-touch-a";
> +            reg = <1>;
> +            vdd-supply = <&vreg_l11a>;
> +            reset-gpios = <&display_mcu 1 GPIO_ACTIVE_HIGH>;
> +            enable-gpios = <&display_mcu 2 GPIO_ACTIVE_HIGH>;
> +
> +            port {
> +                panel_in: endpoint {
> +                    remote-endpoint = <&mdss_dsi0_out>;
> +                };
> +            };
> +        };
> +    };
> +
> +    mdss_dsi0_out: endpoint {
> +        remote-endpoint = <&panel_in>;
> +    };

Drop node, not relevant here.

> diff --git a/Documentation/devicetree/bindings/regulator/waveshare,touchscreen-panel-regulator.yaml b/Documentation/devicetree/bindings/regulator/waveshare,touchscreen-panel-regulator.yaml
> new file mode 100644
> index 000000000000..be81be5d2d74
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/waveshare,touchscreen-panel-regulator.yaml
> @@ -0,0 +1,72 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/waveshare,touchscreen-panel-regulator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Waveshare Touchscreen Panel Regulator
> +
> +maintainers:
> +  - Sudarshan Shetty <tessolveupstream@gmail.com>
> +
> +description: |

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

> +  Regulator driver for Waveshare touchscreen display units.

Driver as Linux driver?

> +  This regulator enables and disables panel power and provides
> +  backlight control over I2C.
> +
> +properties:
> +  compatible:
> +    const: waveshare,touchscreen-panel-regulator

That's way too generic. Description doesn't tell me much more what is
this hardware.

How is so that a device with specific programming model over I2C has no
name, no model, absolutely nothing which would identify that programming
model?

> +
> +  reg:
> +    maxItems: 1
> +    description: I2C address of the regulator device

Drop description, redundant.

> +
> +  vin-supply:
> +    description: Input supply regulator for the panel
> +
> +  enable-gpios:
> +    maxItems: 1
> +    description: GPIO to enable/disable regulator
> +
> +required:
> +  - compatible
> +  - reg
> +  - vin-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    /dts-v1/;
> +    /plugin/;
> +
> +    / {

Drop all this. Look at existing bindings first, how this is written.
There is no single file like that. Please do not come up with your own
style.

Best regards,
Krzysztof