[PATCH v2] dt-bindings: display: panel: add panel-mipi-dsi-bringup
Paulo Pavacic
pavacic.p at gmail.com
Tue May 16 22:08:14 UTC 2023
Hello, thank you for your time to review this patch and sorry for not
addressing all of the concerns, it was done unintentionally. This is my
first contribution to the Linux kernel and it is quite a process.
I have run those two scripts and haven't received any errors I have latest
master cloned so I will check what I did wrong.
The thing I would like to get approval on before I try anything else is the
name 'panel-mipi-dsi-bringup':
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.
Missing user of the bindings - driver or DTS. Please sent patches
together as patchset.
I wasn't sure how to name it and this name seemed fit. I'm not sure how to
be concise about this, but here is the full story as to why I have done
that:
I got a task to enable panel for which working driver wasn't available. I
have started testing raydium driver and modifying parts of it until I got
it working.
Driver was modified quite a lot, new functions, macros and structures were
added which resulted in a new driver.
Therefore I have made a simple driver which I have submitted for a review
which will probably be rejected now due tomany reasons I have noticed after
sending it:
https://lore.kernel.org/lkml/CAO9szn03msW6pu37Zws5EaFGL10rjp9ugPdCuDvOPuQRU72gVQ@mail.gmail.com/T/
While talking with manufacturers of the panel I have figured out that they
aren't that familiar with the Linux kernel.
They had previously only enabled it on bare metal (PLA?) and provided me
with the initialization sequences. Initialization sequences are hex values
sent over MIPI DSI to initialize panel controller.
Initialization sequences sometimes also require delays after certain
commands and for different panels it can be very different.
I believe I have simplified it so that someone can follow comments inside
of the driver and try to enable mipi dsi panel by copy pasting
initialization code from bare metal system and doing minor modifications.
Since I have targeted this at people who need to enable their panels for
the first time name seemed okay. I thought that since there is
panel-simple.yml that panel-mipi-dsi-bringup.yml would be acceptable name.
Best regards,
Paulo
uto, 16. svi 2023. u 17:57 Krzysztof Kozlowski <
krzysztof.kozlowski at linaro.org> napisao je:
>
> 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 at gmail.com>
> > ---
> > .../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 at gmail.com>
> > +
> > +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 at 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 at 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 at gmail.com>
> > +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 at gmail.com>
> > M: Abhinav Kumar <quic_abhinavk at quicinc.com>
>
> Best regards,
> Krzysztof
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230517/0c769139/attachment.htm>
More information about the dri-devel
mailing list