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
