Message-ID: <CAO9szn1EsbuPSRrOW8CLqhp+QUcL=9NE93FAwsg2n3htd_aJTw@mail.gmail.com>
Date:   Wed, 17 May 2023 00:13:06 +0200
From:   Paulo Pavacic <pavacic.p@...il.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc:     krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        neil.armstrong@...aro.org, sam@...nborg.org, airlied@...il.com,
        robh+dt@...nel.org, daniel@...ll.ch,
        dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] dt-bindings: display: panel: add panel-mipi-dsi-bringup

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@...aro.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@...il.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@...il.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@...10000 {
>
> 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@...il.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@...il.com>
> >  M:    Abhinav Kumar <quic_abhinavk@...cinc.com>
>
> Best regards,
> Krzysztof
>


-- 
Lijep pozdrav,
Paulo

Powered by blists - more mailing lists