Re: [PATCH v2] dt-bindings: display: panel: add panel-mipi-dsi-bringup

[Krzysztof Kozlowski]

On 16/05/2023 15:09, Paulo Pavačić wrote:
> Add dt-bindings documentation for panel-mipi-dsi-bringup which currently
> supports fannal,c3004 panel. Also added fannal to vendor-prefixes.

Thank you for your patch. There is something to discuss/improve.

> 
> v2 changelog:

Please put changelog after ---

Missing user of the bindings - driver or DTS. Please sent patches
together as patchset.



>   - revised driver title, now describes purpose
>   - revised description, now describes hw
>   - revised maintainers, now has only 1 mail
>   - removed diacritics from commit/commit author
>   - properties/compatible is now enum
>   - compatible using only lowercase
>   - revised dts example
>   - modified MAINTAINERS in this commit (instead of driver commit)
>   - dt_bindings_check checked yml
>   - checkpatch warning fixed
> 
> Signed-off-by: Paulo Pavacic <pavacic.p@xxxxxxxxx>
> ---
>  .../display/panel/panel-mipi-dsi-bringup.yaml | 54 +++++++++++++++++++
>  .../devicetree/bindings/vendor-prefixes.yaml  |  2 +
>  MAINTAINERS                                   |  6 +++
>  3 files changed, 62 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/display/panel/panel-mipi-dsi-bringup.yaml

Still wrong filename. You did not respond to my previous comments, so I
don't really understand what's this.

Judging by compatible, this should be fannal,c3004.yaml

If not, explain please.

> 
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-mipi-dsi-bringup.yaml
> b/Documentation/devicetree/bindings/display/panel/panel-mipi-dsi-bringup.yaml
> new file mode 100644
> index 000000000000..c9e2b545657e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dsi-bringup.yaml
> @@ -0,0 +1,54 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/panel/panel-mipi-dsi-bringup.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MIPI DSI Bringup Panel Porting Bindings

Drop Bindings. I don't understand what is "Porting" in the terms of
hardware. If it these are bindings for Panel, please write here proper
hardware.

> +
> +description: |
> +  MIPI DSI Bringup Panel porting bindings to be used for a collection of panels

I have no clue what is "Bringup panel". Is it technical term for some
type of panels?

> +  from different manufacturers which don't require backlight control from the
> +  driver and have a single reset pin which is required to be passed as an
> +  argument.

Drop "driver"

> +
> +maintainers:
> +  - Paulo Pavacic <pavacic.p@xxxxxxxxx>
> +
> +allOf:
> +  - $ref: panel-common.yaml#
> +
> +properties:
> +

Drop blank line.

> +  compatible:
> +    enum:
> +      # compatible must be listed in alphabetical order, ordered by compatible.
> +      # The description in the comment is mandatory for each compatible.

Drop above comment.

> +
> +        # Fannal 480x800 panel
> +      - fannal,c3004
> +
> +  reg: true
> +  reset-gpios: true
> +
> +required:
> +  - compatible
> +  - reg
> +  - reset-gpios
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    //example on IMX8MM where GPIO pin 9 is used as a reset pin

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.

I asked to drop the comment.

> +    mipi_dsi@32e10000 {

dsi {

There is no way it was correct in current form.

It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.

> +        panel@0 {
> +            compatible = "fannal,c3004";
> +            reg = <0>;
> +            pinctrl-0 = <&pinctrl_mipi_dsi_rst>;
> +            pinctrl-names = "default";
> +            reset-gpios = <&gpio1 9 GPIO_ACTIVE_LOW>;
> +        };
> +    };
> +...
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 82d39ab0231b..f962750f630a 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -462,6 +462,8 @@ patternProperties:
>      description: Facebook
>    "^fairphone,.*":
>      description: Fairphone B.V.
> +  "^fannal,.*":
> +    description: Fannal Electronics Co., Ltd
>    "^faraday,.*":
>      description: Faraday Technology Corporation
>    "^fastrax,.*":
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e0ad886d3163..46f988ee60bd 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6566,6 +6566,12 @@ T:    git git://anongit.freedesktop.org/drm/drm-misc
>  F:    Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
>  F:    drivers/gpu/drm/tiny/panel-mipi-dbi.c
> 
> +DRM DRIVER FOR MIPI DSI BRINGUP
> +M:    Paulo Pavacic <pavacic.p@xxxxxxxxx>
> +S:    Maintained
> +C:    mipi-dsi-bringup:matrix.org

Missing protocol. See explanation of C: entry at the beginning.

> +F:    Documentation/devicetree/bindings/display/panel/panel-mipi-dsi-bringup.yaml
> +
>  DRM DRIVER FOR MSM ADRENO GPU
>  M:    Rob Clark <robdclark@xxxxxxxxx>
>  M:    Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>

Best regards,
Krzysztof