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

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

[Paulo Pavačić]

Add dt-bindings documentation for panel-mipi-dsi-bringup which currently
supports fannal,c3004 panel. Also added fannal to vendor-prefixes.

v2 changelog:
  - 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@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

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
+
+description: |
+  MIPI DSI Bringup Panel porting bindings to be used for a collection 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.
+
+maintainers:
+  - Paulo Pavacic <pavacic.p@gmail.com>
+
+allOf:
+  - $ref: panel-common.yaml#
+
+properties:
+
+  compatible:
+    enum:
+      # compatible must be listed in alphabetical order, ordered by compatible.
+      # The description in the comment is mandatory for each compatible.
+
+        # 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
+    mipi_dsi@32e10000 {
+        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@gmail.com>
+S:    Maintained
+C:    mipi-dsi-bringup:matrix.org
+F:    Documentation/devicetree/bindings/display/panel/panel-mipi-dsi-bringup.yaml
+
 DRM DRIVER FOR MSM ADRENO GPU
 M:    Rob Clark <robdclark@gmail.com>
 M:    Abhinav Kumar <quic_abhinavk@quicinc.com>
-- 
2.40.1

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@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@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@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@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@gmail.com>
>  M:    Abhinav Kumar <quic_abhinavk@quicinc.com>

Best regards,
Krzysztof

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

[Paulo Pavacic]

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@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@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@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@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@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@gmail.com>
> >  M:    Abhinav Kumar <quic_abhinavk@quicinc.com>
>
> Best regards,
> Krzysztof
>


-- 
Lijep pozdrav,
Paulo

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

[Krzysztof Kozlowski]

On 17/05/2023 00:13, Paulo Pavacic wrote:
> 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.

Bindings are for hardware, not driver, so they describe the hardware panel.

Best regards,
Krzysztof

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

[Paulo Pavacic]

Hello,

If I understood you correctly you'd prefer it to be named 
fannal,c3004.yaml? My logic is that if more panels were to be added that 
means that each one would have yaml files that would look exactly the 
same with the same user.

Best regards,

Paulo

On 5/17/23 09:03, Krzysztof Kozlowski wrote:
> On 17/05/2023 00:13, Paulo Pavacic wrote:
>> 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.
> Bindings are for hardware, not driver, so they describe the hardware panel.
>
> Best regards,
> Krzysztof
>

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

[Krzysztof Kozlowski]

On 17/05/2023 10:29, Paulo Pavacic wrote:
> Hello,
> 
> If I understood you correctly you'd prefer it to be named 
> fannal,c3004.yaml?

This is what I wrote:
"Judging by compatible, this should be fannal,c3004.yaml"

>  My logic is that if more panels were to be added that 
> means that each one would have yaml files that would look exactly the 
> same with the same user.

It's not a big deal. Although anyway why would other panels have exactly
the same supplies and all other properties?

BTW, you miss there supply.

Best regards,
Krzysztof

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

[Jagan Teki]

On Wed, May 17, 2023 at 12:39 PM Paulo Pavacic <pavacic.p@gmail.com> wrote:
>
> 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.

Just to add a few pieces of information for you to understand better
on the context of dsi panels. DSI panels can be part of panel-simple.c
or panel-<vendor-part>.c DSI panels whose init and exit sequence is
generic are suitable to add it in panel-simple and have bindings on
panel-simple.yml. Some DSI panels have specific init and exit
sequences in terms of power, reset and DCS then those have separate
drivers to handle and whose driver name must be panel-<vendor-part>.c
or similar and bindings also follow a similar naming convention.

Just decide which part of the category that your panel belongs and add
separate patches for binding and driver.

Thanks,
Jagan.

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

[Paulo]

On Wed, May 17 2023 at 05:50:22 PM +0530, Jagan Teki
<jagan@amarulasolutions.com> wrote:
 > Just to add a few pieces of information for you to understand better
 > on the context of dsi panels. DSI panels can be part of 
panel-simple.c
 > or panel-<vendor-part>.c DSI panels whose init and exit sequence is
 > generic are suitable to add it in panel-simple and have bindings on
 > panel-simple.yml.

This panel doesn't fit that well into panel-simple.c since it has
initialization sequence. For that reason it would fit more into
panel-sortofsimple.c which didn't exist so I have created new driver
and called it panel-mipi-dsi-bringup.c.

 > Some DSI panels have specific init and exit
 > sequences in terms of power, reset and DCS then those have separate
 > drivers to handle and whose driver name must be panel-<vendor-part>.c
 > or similar and bindings also follow a similar naming convention.

I have made a driver exactly for that purpose. Driver that allows
adding new panels which have specific init sequences (and of course 
timings and other stuff). fannal,c3004 can be seen as a working example.

Here is code snippet from the driver:
```
static const struct brup_panel_info brup_fannal_c3004_panel_info = {
 .display_mode = &brup_fannal_c3004_display_mode,
 .num_of_dsi_lanes = 2, //how many wires are connected to the panel
 .video_mode = BRUP_VIDEO_MODES[BRUP_SYNC_PULSE],
 .mipi_dsi_format = MIPI_DSI_FMT_RGB888,
 .mipi_dsi_mode_flags =
  MIPI_DSI_CLOCK_NON_CONTINUOUS | MIPI_DSI_MODE_VSYNC_FLUSH |
  MIPI_DSI_MODE_VIDEO_HSE | MIPI_DSI_MODE_NO_EOT_PACKET,
 .bus_flags = DRM_BUS_FLAG_DE_LOW | DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
 .panel_enable_function = &brup_panel_fannal_c3004_enable_function
};
```
where enable function is function with init sequence and other values 
are values that might be different for different displays.

All the inputs are appreciated as this is my first time submitting 
patch. If you see anything that is odd to you please reach out to me.
All in all I believe I now understand how should device tree look and 
the reasons/ideology behind it.

Best regards,
Paulo

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

[Jagan Teki]

Hi,

Please don't post, use inline replies.

On Wed, May 17, 2023 at 6:34 PM Paulo <pavacic.p@gmail.com> wrote:
>
> On Wed, May 17 2023 at 05:50:22 PM +0530, Jagan Teki
> <jagan@amarulasolutions.com> wrote:
>  > Just to add a few pieces of information for you to understand better
>  > on the context of dsi panels. DSI panels can be part of
> panel-simple.c
>  > or panel-<vendor-part>.c DSI panels whose init and exit sequence is
>  > generic are suitable to add it in panel-simple and have bindings on
>  > panel-simple.yml.
>
> This panel doesn't fit that well into panel-simple.c since it has
> initialization sequence. For that reason it would fit more into
> panel-sortofsimple.c which didn't exist so I have created new driver
> and called it panel-mipi-dsi-bringup.c.
>
>  > Some DSI panels have specific init and exit
>  > sequences in terms of power, reset and DCS then those have separate
>  > drivers to handle and whose driver name must be panel-<vendor-part>.c
>  > or similar and bindings also follow a similar naming convention.
>
> I have made a driver exactly for that purpose. Driver that allows
> adding new panels which have specific init sequences (and of course
> timings and other stuff). fannal,c3004 can be seen as a working example.
>
> Here is code snippet from the driver:
> ```
> static const struct brup_panel_info brup_fannal_c3004_panel_info = {
>  .display_mode = &brup_fannal_c3004_display_mode,
>  .num_of_dsi_lanes = 2, //how many wires are connected to the panel
>  .video_mode = BRUP_VIDEO_MODES[BRUP_SYNC_PULSE],
>  .mipi_dsi_format = MIPI_DSI_FMT_RGB888,
>  .mipi_dsi_mode_flags =
>   MIPI_DSI_CLOCK_NON_CONTINUOUS | MIPI_DSI_MODE_VSYNC_FLUSH |
>   MIPI_DSI_MODE_VIDEO_HSE | MIPI_DSI_MODE_NO_EOT_PACKET,
>  .bus_flags = DRM_BUS_FLAG_DE_LOW | DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE,
>  .panel_enable_function = &brup_panel_fannal_c3004_enable_function
> };
> ```
> where enable function is function with init sequence and other values
> are values that might be different for different displays.
>
> All the inputs are appreciated as this is my first time submitting
> patch. If you see anything that is odd to you please reach out to me.
> All in all I believe I now understand how should device tree look and
> the reasons/ideology behind it.

So, the driver has to be panel-fannal-c3004.c and binding to be
fannal,c3004.yaml.

Thanks,
Jagan.

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

[Paulo Pavacic]

> So, the driver has to be panel-fannal-c3004.c and binding to be fannal,c3004.yaml.

I believe I have covered this and all the other problems in following
patch which I will submit as a V3 of the conversation in a patchset
together with a driver:

Add dt-bindings documentation for panel-mipi-dsi-bringup which currently
supports fannal,c3004 panel. Also added fannal to vendor-prefixes.

Signed-off-by: Paulo Pavacic <pavacic.p@gmail.com>
---
v3 changelog:
  - renamed yml file
  - refactored yml file to describe fannal,c3004
  - added matrix URI to MAINTAINERS
v2 changelog:
  - 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
---
 .../bindings/display/panel/fannal,c3004.yaml  | 74 +++++++++++++++++++
 .../devicetree/bindings/vendor-prefixes.yaml  |  2 +
 MAINTAINERS                                   |  6 ++
 3 files changed, 82 insertions(+)
 create mode 100644
Documentation/devicetree/bindings/display/panel/fannal,c3004.yaml

diff --git a/Documentation/devicetree/bindings/display/panel/fannal,c3004.yaml
b/Documentation/devicetree/bindings/display/panel/fannal,c3004.yaml
new file mode 100644
index 000000000000..c2eea0fa8418
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/fannal,c3004.yaml
@@ -0,0 +1,74 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/fannal,c3004.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Fannal C3004373132019A MIPI-DSI
+
+maintainers:
+  - Paulo Pavacic <pavacic.p@gmail.com>
+
+description: |
+  Fannal C3004373132019A is a 480x800 panel which requires DSI DCS
+  initialization sequences.
+
+allOf:
+  - $ref: panel-common.yaml#
+
+properties:
+  compatible:
+    items:
+      - const: fannal,c3004
+
+  port: true
+  reg: true
+  reset-gpios: true
+
+  vdd-supply:
+    description: power supply voltage
+  vddio-supply:
+    description: power supply voltage for IO
+
+  width-mm:
+    description: physical panel width [mm]
+  height-mm:
+    description: physical panel height [mm]
+
+required:
+  - compatible
+  - reg
+  - reset-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    dsi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        panel@0 {
+            compatible = "fannal,c3004";
+            reg = <0>;
+            pinctrl-0 = <&pinctrl_mipi_dsi_rst>;
+            pinctrl-names = "default";
+            reset-gpios = <&gpio1 9 GPIO_ACTIVE_LOW>;
+            vdd-supply = <&reg1>;
+            vddio-supply = <&reg2>;
+            width-mm = <93>;
+            height-mm = <56>;
+            panel-timing {
+                clock-frequency = <27000000>;
+                hactive = <480>;
+                vactive = <800>;
+                hfront-porch = <30>;
+                hback-porch = <30>;
+                hsync-len = <8>;
+                vback-porch = <30>;
+                vfront-porch = <30>;
+                vsync-len = <8>;
+            };
+        };
+    };
+...
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..75879341dd0b 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@gmail.com>
+S:    Maintained
+C:    matrix:r/mipi-dsi-bringup:matrix.org
+F:    Documentation/devicetree/bindings/display/panel/panel-fannal,c3004.yaml
+
 DRM DRIVER FOR MSM ADRENO GPU
 M:    Rob Clark <robdclark@gmail.com>
 M:    Abhinav Kumar <quic_abhinavk@quicinc.com>
-- 
2.40.1

Best regards,
Paulo

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

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 358 bytes --]

On Thu, May 18, 2023 at 04:47:08PM +0200, Paulo Pavacic wrote:
> > So, the driver has to be panel-fannal-c3004.c and binding to be fannal,c3004.yaml.
> 
> I believe I have covered this and all the other problems in following
> patch which I will submit as a V3 of the conversation in a patchset
> together with a driver:

Please do, that'd be great.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Dave Stevenson]

Hi Paulo

On Tue, 16 May 2023 at 14:09, Paulo Pavačić <pavacic.p@gmail.com> wrote:
>
> Add dt-bindings documentation for panel-mipi-dsi-bringup which currently
> supports fannal,c3004 panel. Also added fannal to vendor-prefixes.
>
> v2 changelog:
>   - 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@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
>
> 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
> +
> +description: |
> +  MIPI DSI Bringup Panel porting bindings to be used for a collection 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.

Don't we already have support for DSI displays that only need a single
reset pin via panel-simple? [1]

The bit that confuses me is that the driver patch [2] is using DSI DCS
commands to configure the panel - that differs from this dt binding
description of the panel only needing a reset pin.

Potentially there is gain in having a template DSI panel driver
available for reference, but this driver/binding appears to be trying
to act as a generic thing.

  Dave

[1] https://elixir.bootlin.com/linux/v6.3.2/source/drivers/gpu/drm/panel/panel-simple.c#L4605
[2] https://lists.freedesktop.org/archives/dri-devel/2023-May/404775.html


> +
> +maintainers:
> +  - Paulo Pavacic <pavacic.p@gmail.com>
> +
> +allOf:
> +  - $ref: panel-common.yaml#
> +
> +properties:
> +
> +  compatible:
> +    enum:
> +      # compatible must be listed in alphabetical order, ordered by compatible.
> +      # The description in the comment is mandatory for each compatible.
> +
> +        # 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
> +    mipi_dsi@32e10000 {
> +        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@gmail.com>
> +S:    Maintained
> +C:    mipi-dsi-bringup:matrix.org
> +F:    Documentation/devicetree/bindings/display/panel/panel-mipi-dsi-bringup.yaml
> +
>  DRM DRIVER FOR MSM ADRENO GPU
>  M:    Rob Clark <robdclark@gmail.com>
>  M:    Abhinav Kumar <quic_abhinavk@quicinc.com>
> --
> 2.40.1
>