[PATCH v4 0/3] drm/tiny: Add MIPI DBI compatible SPI driver

[PATCH v4 0/3] drm/tiny: Add MIPI DBI compatible SPI driver

[Noralf Trønnes]

Hi,

This patchset adds a driver that will work with most MIPI DBI compatible
SPI panels out there.

Maxime gave[1] a good overview of the situation with these displays and
proposed to make a driver that works with all MIPI DBI compatible
controllers and use a firmware file to provide the controller setup for
a particular panel.

Changes since version 3:
- There should only be two compatible (Maxime)
- s/panel-dbi-spi/panel-mipi-dbi-spi/in compatible
- Move driver to drm/tiny where the other drivers of its kind are located.
  The driver module will not be shared with a future DPI driver after all.

See wiki[2] for script to make command firmware files.

Noralf.

[1] https://lore.kernel.org/dri-devel/20211129093946.xhp22mvdut3m67sc@houat/
[2] https://github.com/notro/panel-mipi-dbi/wiki


Noralf Trønnes (3):
  dt-bindings: display: add bindings for MIPI DBI compatible SPI panels
  drm/mipi-dbi: Add driver_private member to struct mipi_dbi_dev
  drm/tiny: Add MIPI DBI compatible SPI driver

 .../display/panel/panel-mipi-dbi-spi.yaml     | 125 ++++++
 MAINTAINERS                                   |   8 +
 drivers/gpu/drm/tiny/Kconfig                  |  13 +
 drivers/gpu/drm/tiny/Makefile                 |   1 +
 drivers/gpu/drm/tiny/panel-mipi-dbi.c         | 413 ++++++++++++++++++
 include/drm/drm_mipi_dbi.h                    |   8 +
 6 files changed, 568 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
 create mode 100644 drivers/gpu/drm/tiny/panel-mipi-dbi.c

-- 
2.33.0

[PATCH v4 1/3] dt-bindings: display: add bindings for MIPI DBI compatible SPI panels

[Noralf Trønnes]

Add binding for MIPI DBI compatible SPI panels.

v4:
- There should only be two compatible (Maxime)
- s/panel-dbi-spi/panel-mipi-dbi-spi/in compatible

v3:
- Move properties to Device Tree (Maxime)
- Use contains for compatible (Maxime)
- Add backlight property to example
- Flesh out description

v2:
- Fix path for panel-common.yaml
- Use unevaluatedProperties
- Drop properties which are in the allOf section
- Drop model property (Rob)

Acked-by: Maxime Ripard <maxime@cerno.tech>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 .../display/panel/panel-mipi-dbi-spi.yaml     | 125 ++++++++++++++++++
 1 file changed, 125 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml

diff --git a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
new file mode 100644
index 000000000000..748c09113168
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
@@ -0,0 +1,125 @@
+# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/panel/panel-mipi-dbi-spi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MIPI DBI SPI Panel
+
+maintainers:
+  - Noralf Trønnes <noralf@tronnes.org>
+
+description: |
+  This binding is for display panels using a MIPI DBI compatible controller
+  in SPI mode.
+
+  The MIPI Alliance Standard for Display Bus Interface defines the electrical
+  and logical interfaces for display controllers historically used in mobile
+  phones. The standard defines 4 display architecture types and this binding is
+  for type 1 which has full frame memory. There are 3 interface types in the
+  standard and type C is the serial interface.
+
+  The standard defines the following interface signals for type C:
+  - Power:
+    - Vdd: Power supply for display module
+    - Vddi: Logic level supply for interface signals
+    Combined into one in this binding called: power-supply
+  - Interface:
+    - CSx: Chip select
+    - SCL: Serial clock
+    - Dout: Serial out
+    - Din: Serial in
+    - SDA: Bidrectional in/out
+    - D/CX: Data/command selection, high=data, low=command
+      Called dc-gpios in this binding.
+    - RESX: Reset when low
+      Called reset-gpios in this binding.
+
+  The type C interface has 3 options:
+
+    - Option 1: 9-bit mode and D/CX as the 9th bit
+      |              Command              |  the next command or following data  |
+      |<0><D7><D6><D5><D4><D3><D2><D1><D0>|<D/CX><D7><D6><D5><D4><D3><D2><D1><D0>|
+
+    - Option 2: 16-bit mode and D/CX as a 9th bit
+      |              Command or data                              |
+      |<X><X><X><X><X><X><X><D/CX><D7><D6><D5><D4><D3><D2><D1><D0>|
+
+    - Option 3: 8-bit mode and D/CX as a separate interface line
+      |        Command or data         |
+      |<D7><D6><D5><D4><D3><D2><D1><D0>|
+
+  The panel resolution is specified using the panel-timing node properties
+  hactive (width) and vactive (height). The other mandatory panel-timing
+  properties should be set to zero except clock-frequency which can be
+  optionally set to inform about the actual pixel clock frequency.
+
+  If the panel is wired to the controller at an offset specify this using
+  hback-porch (x-offset) and vback-porch (y-offset).
+
+allOf:
+  - $ref: panel-common.yaml#
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+  compatible:
+    items:
+      - {} # Panel Specific Compatible
+      - const: panel-mipi-dbi-spi
+
+  write-only:
+    type: boolean
+    description:
+      Controller is not readable (ie. MISO is not wired up).
+
+  dc-gpios:
+    maxItems: 1
+    description: |
+      Controller data/command selection (D/CX) in 4-line SPI mode.
+      If not set, the controller is in 3-line SPI mode.
+
+required:
+  - compatible
+  - reg
+  - panel-timing
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    spi {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            display@0{
+                    compatible = "sainsmart18", "panel-mipi-dbi-spi";
+                    reg = <0>;
+                    spi-max-frequency = <40000000>;
+
+                    dc-gpios = <&gpio 24 GPIO_ACTIVE_HIGH>;
+                    reset-gpios = <&gpio 25 GPIO_ACTIVE_HIGH>;
+                    write-only;
+
+                    backlight = <&backlight>;
+
+                    width-mm = <35>;
+                    height-mm = <28>;
+
+                    panel-timing {
+                        hactive = <160>;
+                        vactive = <128>;
+                        hback-porch = <0>;
+                        vback-porch = <0>;
+
+                        clock-frequency = <0>;
+                        hfront-porch = <0>;
+                        hsync-len = <0>;
+                        vfront-porch = <0>;
+                        vsync-len = <0>;
+                    };
+            };
+    };
+
+...
-- 
2.33.0

Re: [PATCH v4 1/3] dt-bindings: display: add bindings for MIPI DBI compatible SPI panels

[Sam Ravnborg]

Hi Noralf,

On Fri, Feb 18, 2022 at 04:11:08PM +0100, Noralf Trønnes wrote:
> Add binding for MIPI DBI compatible SPI panels.
> 
> v4:
> - There should only be two compatible (Maxime)
> - s/panel-dbi-spi/panel-mipi-dbi-spi/in compatible
> 
> v3:
> - Move properties to Device Tree (Maxime)
> - Use contains for compatible (Maxime)
> - Add backlight property to example
> - Flesh out description
> 
> v2:
> - Fix path for panel-common.yaml
> - Use unevaluatedProperties
> - Drop properties which are in the allOf section
> - Drop model property (Rob)
> 
> Acked-by: Maxime Ripard <maxime@cerno.tech>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  .../display/panel/panel-mipi-dbi-spi.yaml     | 125 ++++++++++++++++++
>  1 file changed, 125 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> new file mode 100644
> index 000000000000..748c09113168
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> @@ -0,0 +1,125 @@
> +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/panel/panel-mipi-dbi-spi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MIPI DBI SPI Panel
> +
> +maintainers:
> +  - Noralf Trønnes <noralf@tronnes.org>
> +
> +description: |
> +  This binding is for display panels using a MIPI DBI compatible controller
> +  in SPI mode.
> +
> +  The MIPI Alliance Standard for Display Bus Interface defines the electrical
> +  and logical interfaces for display controllers historically used in mobile
> +  phones. The standard defines 4 display architecture types and this binding is
> +  for type 1 which has full frame memory. There are 3 interface types in the
> +  standard and type C is the serial interface.
> +
> +  The standard defines the following interface signals for type C:
> +  - Power:
> +    - Vdd: Power supply for display module
> +    - Vddi: Logic level supply for interface signals
> +    Combined into one in this binding called: power-supply
> +  - Interface:
> +    - CSx: Chip select
> +    - SCL: Serial clock
> +    - Dout: Serial out
> +    - Din: Serial in
> +    - SDA: Bidrectional in/out
> +    - D/CX: Data/command selection, high=data, low=command
> +      Called dc-gpios in this binding.
> +    - RESX: Reset when low
> +      Called reset-gpios in this binding.
> +
> +  The type C interface has 3 options:
> +
> +    - Option 1: 9-bit mode and D/CX as the 9th bit
> +      |              Command              |  the next command or following data  |
> +      |<0><D7><D6><D5><D4><D3><D2><D1><D0>|<D/CX><D7><D6><D5><D4><D3><D2><D1><D0>|
> +
> +    - Option 2: 16-bit mode and D/CX as a 9th bit
> +      |              Command or data                              |
> +      |<X><X><X><X><X><X><X><D/CX><D7><D6><D5><D4><D3><D2><D1><D0>|
> +
> +    - Option 3: 8-bit mode and D/CX as a separate interface line
> +      |        Command or data         |
> +      |<D7><D6><D5><D4><D3><D2><D1><D0>|
> +
> +  The panel resolution is specified using the panel-timing node properties
> +  hactive (width) and vactive (height). The other mandatory panel-timing
> +  properties should be set to zero except clock-frequency which can be
> +  optionally set to inform about the actual pixel clock frequency.
> +
> +  If the panel is wired to the controller at an offset specify this using
> +  hback-porch (x-offset) and vback-porch (y-offset).
Very informative description - well done.

> +
> +allOf:
> +  - $ref: panel-common.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - {} # Panel Specific Compatible
> +      - const: panel-mipi-dbi-spi
> +
> +  write-only:
> +    type: boolean
> +    description:
> +      Controller is not readable (ie. MISO is not wired up).
It would be easier to understand if this comment refers to one of the
pins on the display described above. So maybe something like
(ie. Din (MSIO on the SPI interface) is not wired up).

With the comment updated to include a reference to Din,
Acked-by: Sam Ravnborg <sam@ravnborg.org>

Re: [PATCH v4 1/3] dt-bindings: display: add bindings for MIPI DBI compatible SPI panels

[Rob Herring]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 1642 bytes --]

On Fri, 18 Feb 2022 16:11:08 +0100, Noralf Trønnes wrote:
> Add binding for MIPI DBI compatible SPI panels.
> 
> v4:
> - There should only be two compatible (Maxime)
> - s/panel-dbi-spi/panel-mipi-dbi-spi/in compatible
> 
> v3:
> - Move properties to Device Tree (Maxime)
> - Use contains for compatible (Maxime)
> - Add backlight property to example
> - Flesh out description
> 
> v2:
> - Fix path for panel-common.yaml
> - Use unevaluatedProperties
> - Drop properties which are in the allOf section
> - Drop model property (Rob)
> 
> Acked-by: Maxime Ripard <maxime@cerno.tech>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  .../display/panel/panel-mipi-dbi-spi.yaml     | 125 ++++++++++++++++++
>  1 file changed, 125 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.example.dt.yaml:0:0: /example-0/spi/display@0: failed to match any schema with compatible: ['sainsmart18', 'panel-mipi-dbi-spi']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1594817

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v4 1/3] dt-bindings: display: add bindings for MIPI DBI compatible SPI panels

[Noralf Trønnes]

Den 18.02.2022 16.11, skrev Noralf Trønnes:
> Add binding for MIPI DBI compatible SPI panels.
> 
> v4:
> - There should only be two compatible (Maxime)
> - s/panel-dbi-spi/panel-mipi-dbi-spi/in compatible
> 
> v3:
> - Move properties to Device Tree (Maxime)
> - Use contains for compatible (Maxime)
> - Add backlight property to example
> - Flesh out description
> 
> v2:
> - Fix path for panel-common.yaml
> - Use unevaluatedProperties
> - Drop properties which are in the allOf section
> - Drop model property (Rob)
> 
> Acked-by: Maxime Ripard <maxime@cerno.tech>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  .../display/panel/panel-mipi-dbi-spi.yaml     | 125 ++++++++++++++++++
>  1 file changed, 125 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> new file mode 100644
> index 000000000000..748c09113168
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml

> +allOf:
> +  - $ref: panel-common.yaml#
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - {} # Panel Specific Compatible
> +      - const: panel-mipi-dbi-spi
> +

Rob's bot uses a -m flag I didn't use, and with that the compatible fails:

$ make DT_CHECKER_FLAGS=-m dt_binding_check
DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
  CHKDT   Documentation/devicetree/bindings/processed-schema-examples.json
  SCHEMA  Documentation/devicetree/bindings/processed-schema-examples.json
  DTEX
Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.example.dts
  DTC
Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.example.dt.yaml
  CHECK
Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.example.dt.yaml
Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.example.dt.yaml:0:0:
/example-0/spi/display@0: failed to match any schema with compatible:
['sainsmart18', 'panel-mipi-dbi-spi']

How can I write the compatible property to make the checker happy?

Noralf.

> +  write-only:
> +    type: boolean
> +    description:
> +      Controller is not readable (ie. MISO is not wired up).
> +
> +  dc-gpios:
> +    maxItems: 1
> +    description: |
> +      Controller data/command selection (D/CX) in 4-line SPI mode.
> +      If not set, the controller is in 3-line SPI mode.
> +
> +required:
> +  - compatible
> +  - reg
> +  - panel-timing
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    spi {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            display@0{
> +                    compatible = "sainsmart18", "panel-mipi-dbi-spi";
> +                    reg = <0>;
> +                    spi-max-frequency = <40000000>;
> +
> +                    dc-gpios = <&gpio 24 GPIO_ACTIVE_HIGH>;
> +                    reset-gpios = <&gpio 25 GPIO_ACTIVE_HIGH>;
> +                    write-only;
> +
> +                    backlight = <&backlight>;
> +
> +                    width-mm = <35>;
> +                    height-mm = <28>;
> +
> +                    panel-timing {
> +                        hactive = <160>;
> +                        vactive = <128>;
> +                        hback-porch = <0>;
> +                        vback-porch = <0>;
> +
> +                        clock-frequency = <0>;
> +                        hfront-porch = <0>;
> +                        hsync-len = <0>;
> +                        vfront-porch = <0>;
> +                        vsync-len = <0>;
> +                    };
> +            };
> +    };
> +
> +...

Re: [PATCH v4 1/3] dt-bindings: display: add bindings for MIPI DBI compatible SPI panels

[Rob Herring]

On Mon, Feb 21, 2022 at 12:31:08PM +0100, Noralf Trønnes wrote:
> 
> 
> Den 18.02.2022 16.11, skrev Noralf Trønnes:
> > Add binding for MIPI DBI compatible SPI panels.
> > 
> > v4:
> > - There should only be two compatible (Maxime)
> > - s/panel-dbi-spi/panel-mipi-dbi-spi/in compatible
> > 
> > v3:
> > - Move properties to Device Tree (Maxime)
> > - Use contains for compatible (Maxime)
> > - Add backlight property to example
> > - Flesh out description
> > 
> > v2:
> > - Fix path for panel-common.yaml
> > - Use unevaluatedProperties
> > - Drop properties which are in the allOf section
> > - Drop model property (Rob)
> > 
> > Acked-by: Maxime Ripard <maxime@cerno.tech>
> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> > ---
> >  .../display/panel/panel-mipi-dbi-spi.yaml     | 125 ++++++++++++++++++
> >  1 file changed, 125 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> > new file mode 100644
> > index 000000000000..748c09113168
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
> 
> > +allOf:
> > +  - $ref: panel-common.yaml#
> > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - {} # Panel Specific Compatible
> > +      - const: panel-mipi-dbi-spi
> > +
> 
> Rob's bot uses a -m flag I didn't use, and with that the compatible fails:
> 
> $ make DT_CHECKER_FLAGS=-m dt_binding_check
> DT_SCHEMA_FILES=Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
>   CHKDT   Documentation/devicetree/bindings/processed-schema-examples.json
>   SCHEMA  Documentation/devicetree/bindings/processed-schema-examples.json
>   DTEX
> Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.example.dts
>   DTC
> Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.example.dt.yaml
>   CHECK
> Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.example.dt.yaml
> Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.example.dt.yaml:0:0:
> /example-0/spi/display@0: failed to match any schema with compatible:
> ['sainsmart18', 'panel-mipi-dbi-spi']
> 
> How can I write the compatible property to make the checker happy?

You need to partition the schemas as I outlined before. Given the DBI 
spec does define power and reset, maybe you can get away with 1 schema 
just by changing the '- {}' entry above to an enum with a list of 
compatibles. But as soon as there is a panel with extra or different 
properties, this schema will have to be split. 

Rob

[PATCH v4 2/3] drm/mipi-dbi: Add driver_private member to struct mipi_dbi_dev

[Noralf Trønnes]

devm_drm_dev_alloc() can't allocate structures that embed a structure
which then again embeds drm_device. Workaround this by adding a
driver_private pointer to struct mipi_dbi_dev which the driver can use for
its additional state.

v3:
- Add documentation

Acked-by: Maxime Ripard <maxime@cerno.tech>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 include/drm/drm_mipi_dbi.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
index 6fe13cce2670..dad2f187b64b 100644
--- a/include/drm/drm_mipi_dbi.h
+++ b/include/drm/drm_mipi_dbi.h
@@ -130,6 +130,14 @@ struct mipi_dbi_dev {
 	 * @dbi: MIPI DBI interface
 	 */
 	struct mipi_dbi dbi;
+
+	/**
+	 * @driver_private: Driver private data.
+	 *                  Necessary for drivers with private data since devm_drm_dev_alloc()
+	 *                  can't allocate structures that embed a structure which then again
+	 *                  embeds drm_device.
+	 */
+	void *driver_private;
 };
 
 static inline struct mipi_dbi_dev *drm_to_mipi_dbi_dev(struct drm_device *drm)
-- 
2.33.0

Re: [PATCH v4 2/3] drm/mipi-dbi: Add driver_private member to struct mipi_dbi_dev

[Sam Ravnborg]

Hi Noralf,
On Fri, Feb 18, 2022 at 04:11:09PM +0100, Noralf Trønnes wrote:
> devm_drm_dev_alloc() can't allocate structures that embed a structure
> which then again embeds drm_device. Workaround this by adding a
> driver_private pointer to struct mipi_dbi_dev which the driver can use for
> its additional state.
> 
> v3:
> - Add documentation
> 
> Acked-by: Maxime Ripard <maxime@cerno.tech>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Acked-by: Sam Ravnborg <sam@ravnborg.org>

[PATCH v4 3/3] drm/tiny: Add MIPI DBI compatible SPI driver

[Noralf Trønnes]

Add a driver that will work with most MIPI DBI compatible SPI panels.
This avoids adding a driver for every new MIPI DBI compatible controller
that is to be used by Linux. The 'compatible' Device Tree property with
a '.bin' suffix will be used to load a firmware file that contains the
controller configuration.

Example (driver will load sainsmart18.bin):

display@0 {
	compatible = "sainsmart18", "panel-mipi-dbi-spi";
...
};

v4:
- Move driver to drm/tiny where the other drivers of its kind are located.
  The driver module will not be shared with a future DPI driver after all.

v3:
- Move properties to DT (Maxime)
- The MIPI DPI spec has optional support for DPI where the controller is
  configured over DBI. Rework the command functions so they can be moved
  to drm_mipi_dbi and shared with a future panel-mipi-dpi-spi driver

v2:
- Drop model property and use compatible instead (Rob)
- Add wiki entry in MAINTAINERS

Acked-by: Maxime Ripard <maxime@cerno.tech>
Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 MAINTAINERS                           |   8 +
 drivers/gpu/drm/tiny/Kconfig          |  13 +
 drivers/gpu/drm/tiny/Makefile         |   1 +
 drivers/gpu/drm/tiny/panel-mipi-dbi.c | 413 ++++++++++++++++++++++++++
 4 files changed, 435 insertions(+)
 create mode 100644 drivers/gpu/drm/tiny/panel-mipi-dbi.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 8e6e892f99f0..3a0e57f23ad0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6107,6 +6107,14 @@ T:	git git://anongit.freedesktop.org/drm/drm-misc
 F:	Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt
 F:	drivers/gpu/drm/tiny/mi0283qt.c
 
+DRM DRIVER FOR MIPI DBI compatible panels
+M:	Noralf Trønnes <noralf@tronnes.org>
+S:	Maintained
+W:	https://github.com/notro/panel-mipi-dbi/wiki
+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 MSM ADRENO GPU
 M:	Rob Clark <robdclark@gmail.com>
 M:	Sean Paul <sean@poorly.run>
diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
index 712e0004e96e..d552e1618da7 100644
--- a/drivers/gpu/drm/tiny/Kconfig
+++ b/drivers/gpu/drm/tiny/Kconfig
@@ -51,6 +51,19 @@ config DRM_GM12U320
 	 This is a KMS driver for projectors which use the GM12U320 chipset
 	 for video transfer over USB2/3, such as the Acer C120 mini projector.
 
+config DRM_PANEL_MIPI_DBI
+	tristate "DRM support for MIPI DBI compatible panels"
+	depends on DRM && SPI
+	select DRM_KMS_HELPER
+	select DRM_KMS_CMA_HELPER
+	select DRM_MIPI_DBI
+	select BACKLIGHT_CLASS_DEVICE
+	help
+	  Say Y here if you want to enable support for MIPI DBI compatible
+	  panels. The controller command setup can be provided using a
+	  firmware file.
+	  To compile this driver as a module, choose M here.
+
 config DRM_SIMPLEDRM
 	tristate "Simple framebuffer driver"
 	depends on DRM && MMU
diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
index 5d5505d40e7b..1d9d6227e7ab 100644
--- a/drivers/gpu/drm/tiny/Makefile
+++ b/drivers/gpu/drm/tiny/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_DRM_ARCPGU)		+= arcpgu.o
 obj-$(CONFIG_DRM_BOCHS)			+= bochs.o
 obj-$(CONFIG_DRM_CIRRUS_QEMU)		+= cirrus.o
 obj-$(CONFIG_DRM_GM12U320)		+= gm12u320.o
+obj-$(CONFIG_DRM_PANEL_MIPI_DBI)	+= panel-mipi-dbi.o
 obj-$(CONFIG_DRM_SIMPLEDRM)		+= simpledrm.o
 obj-$(CONFIG_TINYDRM_HX8357D)		+= hx8357d.o
 obj-$(CONFIG_TINYDRM_ILI9163)		+= ili9163.o
diff --git a/drivers/gpu/drm/tiny/panel-mipi-dbi.c b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
new file mode 100644
index 000000000000..9240fdec38d6
--- /dev/null
+++ b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
@@ -0,0 +1,413 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DRM driver for MIPI DBI compatible display panels
+ *
+ * Copyright 2022 Noralf Trønnes
+ */
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/firmware.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_fb_helper.h>
+#include <drm/drm_gem_atomic_helper.h>
+#include <drm/drm_gem_cma_helper.h>
+#include <drm/drm_managed.h>
+#include <drm/drm_mipi_dbi.h>
+#include <drm/drm_modeset_helper.h>
+
+#include <video/display_timing.h>
+#include <video/mipi_display.h>
+#include <video/of_display_timing.h>
+#include <video/videomode.h>
+
+static const u8 panel_mipi_dbi_magic[15] = { 'M', 'I', 'P', 'I', ' ', 'D', 'B', 'I',
+					     0, 0, 0, 0, 0, 0, 0 };
+
+/*
+ * The optional display controller configuration is stored in a firmware file.
+ * The Device Tree 'compatible' property value with a '.bin' suffix is passed
+ * to request_firmware() to fetch this file.
+ */
+struct panel_mipi_dbi_config {
+	/* Magic string: panel_mipi_dbi_magic */
+	u8 magic[15];
+
+	/* Config file format version */
+	u8 file_format_version;
+
+	/*
+	 * MIPI commands to execute when the display pipeline is enabled.
+	 * This is used to configure the display controller.
+	 *
+	 * The commands are stored in a byte array with the format:
+	 *     command, num_parameters, [ parameter, ...], command, ...
+	 *
+	 * Some commands require a pause before the next command can be received.
+	 * Inserting a delay in the command sequence is done by using the NOP command with one
+	 * parameter: delay in miliseconds (the No Operation command is part of the MIPI Display
+	 * Command Set where it has no parameters).
+	 *
+	 * Example:
+	 *     command 0x11
+	 *     sleep 120ms
+	 *     command 0xb1 parameters 0x01, 0x2c, 0x2d
+	 *     command 0x29
+	 *
+	 * Byte sequence:
+	 *     0x11 0x00
+	 *     0x00 0x01 0x78
+	 *     0xb1 0x03 0x01 0x2c 0x2d
+	 *     0x29 0x00
+	 */
+	u8 commands[];
+};
+
+struct panel_mipi_dbi_commands {
+	const u8 *buf;
+	size_t len;
+};
+
+static struct panel_mipi_dbi_commands *
+panel_mipi_dbi_check_commands(struct device *dev, const struct firmware *fw)
+{
+	const struct panel_mipi_dbi_config *config = (struct panel_mipi_dbi_config *)fw->data;
+	struct panel_mipi_dbi_commands *commands;
+	size_t size = fw->size, commands_len;
+	unsigned int i = 0;
+
+	if (size < sizeof(*config) + 2) { /* At least 1 command */
+		dev_err(dev, "config: file size=%zu is too small\n", size);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (memcmp(config->magic, panel_mipi_dbi_magic, sizeof(config->magic))) {
+		dev_err(dev, "config: Bad magic: %15ph\n", config->magic);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (config->file_format_version != 1) {
+		dev_err(dev, "config: version=%u is not supported\n", config->file_format_version);
+		return ERR_PTR(-EINVAL);
+	}
+
+	drm_dev_dbg(dev, DRM_UT_DRIVER, "size=%zu version=%u\n", size, config->file_format_version);
+
+	commands_len = size - sizeof(*config);
+
+	while ((i + 1) < commands_len) {
+		u8 command = config->commands[i++];
+		u8 num_parameters = config->commands[i++];
+		const u8 *parameters = &config->commands[i];
+
+		i += num_parameters;
+		if (i > commands_len) {
+			dev_err(dev, "config: command=0x%02x num_parameters=%u overflows\n",
+				command, num_parameters);
+			return ERR_PTR(-EINVAL);
+		}
+
+		if (command == 0x00 && num_parameters == 1)
+			drm_dev_dbg(dev, DRM_UT_DRIVER, "sleep %ums\n", parameters[0]);
+		else
+			drm_dev_dbg(dev, DRM_UT_DRIVER, "command %02x %*ph\n",
+				    command, num_parameters, parameters);
+	}
+
+	if (i != commands_len) {
+		dev_err(dev, "config: malformed command array\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	commands = devm_kzalloc(dev, sizeof(*commands), GFP_KERNEL);
+	if (!commands)
+		return ERR_PTR(-ENOMEM);
+
+	commands->len = commands_len;
+	commands->buf = devm_kmemdup(dev, config->commands, commands->len, GFP_KERNEL);
+	if (!commands->buf)
+		return ERR_PTR(-ENOMEM);
+
+	return commands;
+}
+
+static struct panel_mipi_dbi_commands *panel_mipi_dbi_commands_from_fw(struct device *dev)
+{
+	struct panel_mipi_dbi_commands *commands;
+	const struct firmware *fw;
+	const char *compatible;
+	struct property *prop;
+	char fw_name[40];
+	int ret;
+
+	of_property_for_each_string(dev->of_node, "compatible", prop, compatible) {
+		snprintf(fw_name, sizeof(fw_name), "%s.bin", compatible);
+		ret = firmware_request_nowarn(&fw, fw_name, dev);
+		if (ret) {
+			drm_dev_dbg(dev, DRM_UT_DRIVER,
+				    "No config file found for compatible: '%s' (error=%d)\n",
+				    compatible, ret);
+			continue;
+		}
+
+		commands = panel_mipi_dbi_check_commands(dev, fw);
+		release_firmware(fw);
+		return commands;
+	}
+
+	return NULL;
+}
+
+static void panel_mipi_dbi_commands_execute(struct mipi_dbi *dbi,
+					    struct panel_mipi_dbi_commands *commands)
+{
+	unsigned int i = 0;
+
+	if (!commands)
+		return;
+
+	while (i < commands->len) {
+		u8 command = commands->buf[i++];
+		u8 num_parameters = commands->buf[i++];
+		const u8 *parameters = &commands->buf[i];
+
+		if (command == 0x00 && num_parameters == 1)
+			msleep(parameters[0]);
+		else if (num_parameters)
+			mipi_dbi_command_stackbuf(dbi, command, parameters, num_parameters);
+		else
+			mipi_dbi_command(dbi, command);
+
+		i += num_parameters;
+	}
+}
+
+static void panel_mipi_dbi_enable(struct drm_simple_display_pipe *pipe,
+				  struct drm_crtc_state *crtc_state,
+				  struct drm_plane_state *plane_state)
+{
+	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
+	struct mipi_dbi *dbi = &dbidev->dbi;
+	int ret, idx;
+
+	if (!drm_dev_enter(pipe->crtc.dev, &idx))
+		return;
+
+	drm_dbg(pipe->crtc.dev, "\n");
+
+	ret = mipi_dbi_poweron_conditional_reset(dbidev);
+	if (ret < 0)
+		goto out_exit;
+	if (!ret)
+		panel_mipi_dbi_commands_execute(dbi, dbidev->driver_private);
+
+	mipi_dbi_enable_flush(dbidev, crtc_state, plane_state);
+out_exit:
+	drm_dev_exit(idx);
+}
+
+static const struct drm_simple_display_pipe_funcs panel_mipi_dbi_pipe_funcs = {
+	.enable = panel_mipi_dbi_enable,
+	.disable = mipi_dbi_pipe_disable,
+	.update = mipi_dbi_pipe_update,
+};
+
+DEFINE_DRM_GEM_CMA_FOPS(panel_mipi_dbi_fops);
+
+static const struct drm_driver panel_mipi_dbi_driver = {
+	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
+	.fops			= &panel_mipi_dbi_fops,
+	DRM_GEM_CMA_DRIVER_OPS_VMAP,
+	.debugfs_init		= mipi_dbi_debugfs_init,
+	.name			= "panel-mipi-dbi",
+	.desc			= "MIPI DBI compatible display panel",
+	.date			= "20220103",
+	.major			= 1,
+	.minor			= 0,
+};
+
+static int panel_mipi_dbi_get_mode(struct mipi_dbi_dev *dbidev, struct drm_display_mode *mode)
+{
+	struct device *dev = dbidev->drm.dev;
+	u32 width_mm = 0, height_mm = 0;
+	struct display_timing timing;
+	struct videomode vm;
+	int ret;
+
+	ret = of_get_display_timing(dev->of_node, "panel-timing", &timing);
+	if (ret) {
+		dev_err(dev, "%pOF: failed to get panel-timing (error=%d)\n", dev->of_node, ret);
+		return ret;
+	}
+
+	videomode_from_timing(&timing, &vm);
+
+	if (!vm.hactive || vm.hfront_porch || vm.hsync_len ||
+	    (vm.hback_porch + vm.hactive) > 0xffff ||
+	    !vm.vactive || vm.vfront_porch || vm.vsync_len ||
+	    (vm.vback_porch + vm.vactive) > 0xffff ||
+	    vm.flags) {
+		dev_err(dev, "%pOF: panel-timing out of bounds\n", dev->of_node);
+		return -EINVAL;
+	}
+
+	/* The driver doesn't use the pixel clock but it is mandatory so fake one if not set */
+	if (!vm.pixelclock)
+		vm.pixelclock = (vm.hback_porch + vm.hactive) * (vm.vback_porch + vm.vactive) * 60;
+
+	dbidev->top_offset = vm.vback_porch;
+	dbidev->left_offset = vm.hback_porch;
+
+	memset(mode, 0, sizeof(*mode));
+	drm_display_mode_from_videomode(&vm, mode);
+	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
+
+	ret = device_property_read_u32(dev, "width-mm", &width_mm);
+	if (ret && ret != -EINVAL)
+		return ret;
+
+	ret = device_property_read_u32(dev, "height-mm", &height_mm);
+	if (ret && ret != -EINVAL)
+		return ret;
+
+	mode->width_mm = width_mm;
+	mode->height_mm = height_mm;
+
+	drm_mode_debug_printmodeline(mode);
+
+	return 0;
+}
+
+static int panel_mipi_dbi_spi_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct drm_display_mode mode;
+	struct mipi_dbi_dev *dbidev;
+	struct drm_device *drm;
+	struct mipi_dbi *dbi;
+	struct gpio_desc *dc;
+	int ret;
+
+	dbidev = devm_drm_dev_alloc(dev, &panel_mipi_dbi_driver, struct mipi_dbi_dev, drm);
+	if (IS_ERR(dbidev))
+		return PTR_ERR(dbidev);
+
+	dbi = &dbidev->dbi;
+	drm = &dbidev->drm;
+
+	ret = panel_mipi_dbi_get_mode(dbidev, &mode);
+	if (ret)
+		return ret;
+
+	dbidev->regulator = devm_regulator_get(dev, "power");
+	if (IS_ERR(dbidev->regulator))
+		return dev_err_probe(dev, PTR_ERR(dbidev->regulator),
+				     "Failed to get regulator 'power'\n");
+
+	dbidev->backlight = devm_of_find_backlight(dev);
+	if (IS_ERR(dbidev->backlight))
+		return dev_err_probe(dev, PTR_ERR(dbidev->backlight), "Failed to get backlight\n");
+
+	dbi->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(dbi->reset))
+		return dev_err_probe(dev, PTR_ERR(dbi->reset), "Failed to get GPIO 'reset'\n");
+
+	dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
+	if (IS_ERR(dc))
+		return dev_err_probe(dev, PTR_ERR(dc), "Failed to get GPIO 'dc'\n");
+
+	ret = mipi_dbi_spi_init(spi, dbi, dc);
+	if (ret)
+		return ret;
+
+	if (device_property_present(dev, "write-only"))
+		dbi->read_commands = NULL;
+
+	dbidev->driver_private = panel_mipi_dbi_commands_from_fw(dev);
+	if (IS_ERR(dbidev->driver_private))
+		return PTR_ERR(dbidev->driver_private);
+
+	ret = mipi_dbi_dev_init(dbidev, &panel_mipi_dbi_pipe_funcs, &mode, 0);
+	if (ret)
+		return ret;
+
+	drm_mode_config_reset(drm);
+
+	ret = drm_dev_register(drm, 0);
+	if (ret)
+		return ret;
+
+	spi_set_drvdata(spi, drm);
+
+	drm_fbdev_generic_setup(drm, 0);
+
+	return 0;
+}
+
+static int panel_mipi_dbi_spi_remove(struct spi_device *spi)
+{
+	struct drm_device *drm = spi_get_drvdata(spi);
+
+	drm_dev_unplug(drm);
+	drm_atomic_helper_shutdown(drm);
+
+	return 0;
+}
+
+static void panel_mipi_dbi_spi_shutdown(struct spi_device *spi)
+{
+	drm_atomic_helper_shutdown(spi_get_drvdata(spi));
+}
+
+static int __maybe_unused panel_mipi_dbi_pm_suspend(struct device *dev)
+{
+	return drm_mode_config_helper_suspend(dev_get_drvdata(dev));
+}
+
+static int __maybe_unused panel_mipi_dbi_pm_resume(struct device *dev)
+{
+	drm_mode_config_helper_resume(dev_get_drvdata(dev));
+
+	return 0;
+}
+
+static const struct dev_pm_ops panel_mipi_dbi_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(panel_mipi_dbi_pm_suspend, panel_mipi_dbi_pm_resume)
+};
+
+static const struct of_device_id panel_mipi_dbi_spi_of_match[] = {
+	{ .compatible = "panel-mipi-dbi-spi" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, panel_mipi_dbi_spi_of_match);
+
+static const struct spi_device_id panel_mipi_dbi_spi_id[] = {
+	{ "panel-mipi-dbi-spi", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(spi, panel_mipi_dbi_spi_id);
+
+static struct spi_driver panel_mipi_dbi_spi_driver = {
+	.driver = {
+		.name = "panel-mipi-dbi-spi",
+		.owner = THIS_MODULE,
+		.of_match_table = panel_mipi_dbi_spi_of_match,
+		.pm = &panel_mipi_dbi_pm_ops,
+	},
+	.id_table = panel_mipi_dbi_spi_id,
+	.probe = panel_mipi_dbi_spi_probe,
+	.remove = panel_mipi_dbi_spi_remove,
+	.shutdown = panel_mipi_dbi_spi_shutdown,
+};
+module_spi_driver(panel_mipi_dbi_spi_driver);
+
+MODULE_DESCRIPTION("MIPI DBI compatible display panel driver");
+MODULE_AUTHOR("Noralf Trønnes");
+MODULE_LICENSE("GPL");
-- 
2.33.0

Re: [PATCH v4 3/3] drm/tiny: Add MIPI DBI compatible SPI driver

[Sam Ravnborg]

Hi Noralf,
On Fri, Feb 18, 2022 at 04:11:10PM +0100, Noralf Trønnes wrote:
> Add a driver that will work with most MIPI DBI compatible SPI panels.
> This avoids adding a driver for every new MIPI DBI compatible controller
> that is to be used by Linux. The 'compatible' Device Tree property with
> a '.bin' suffix will be used to load a firmware file that contains the
> controller configuration.
A solution where we have the command sequences as part of the DT-overlay
is IMO a much better way to solve this:
- The users needs to deal only with a single file, so there is less that
  goes wrong
- The user need not reading special instructions how to handle a .bin
  file, if the overlay is present everything is fine
- The file that contains the command sequences can be properly annotated
  with comments
- The people that creates the command sequences has no need for a special
  script to create the file for a display - this is all readable ascii.
- Using a DT-overlay the parsing of the DT-overlay can be done by
  well-tested functions, no need to invent something new to parse the
  file


The idea with a common mipi DBI SPI driver is super, it is the detail
with the .bin file that I am against.

With the above said, a few comments to the current implementation below.
As we know it from you - a very well-written driver.

	Sam

> Acked-by: Maxime Ripard <maxime@cerno.tech>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  MAINTAINERS                           |   8 +
>  drivers/gpu/drm/tiny/Kconfig          |  13 +
>  drivers/gpu/drm/tiny/Makefile         |   1 +
>  drivers/gpu/drm/tiny/panel-mipi-dbi.c | 413 ++++++++++++++++++++++++++
>  4 files changed, 435 insertions(+)
>  create mode 100644 drivers/gpu/drm/tiny/panel-mipi-dbi.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8e6e892f99f0..3a0e57f23ad0 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6107,6 +6107,14 @@ T:	git git://anongit.freedesktop.org/drm/drm-misc
>  F:	Documentation/devicetree/bindings/display/multi-inno,mi0283qt.txt
>  F:	drivers/gpu/drm/tiny/mi0283qt.c
>  
> +DRM DRIVER FOR MIPI DBI compatible panels
> +M:	Noralf Trønnes <noralf@tronnes.org>
> +S:	Maintained
> +W:	https://github.com/notro/panel-mipi-dbi/wiki
Nice with a wiki for this, I can see this will grow over time and be a
place to find how to support more panels.

> +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 MSM ADRENO GPU
>  M:	Rob Clark <robdclark@gmail.com>
>  M:	Sean Paul <sean@poorly.run>
> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
> index 712e0004e96e..d552e1618da7 100644
> --- a/drivers/gpu/drm/tiny/Kconfig
> +++ b/drivers/gpu/drm/tiny/Kconfig
> @@ -51,6 +51,19 @@ config DRM_GM12U320
>  	 This is a KMS driver for projectors which use the GM12U320 chipset
>  	 for video transfer over USB2/3, such as the Acer C120 mini projector.
>  
> +config DRM_PANEL_MIPI_DBI
> +	tristate "DRM support for MIPI DBI compatible panels"
> +	depends on DRM && SPI
> +	select DRM_KMS_HELPER
> +	select DRM_KMS_CMA_HELPER
This symbol is not present in my drm-misc-next tree (which is a few
weeks old, so it may be newer).

> +	select DRM_MIPI_DBI
> +	select BACKLIGHT_CLASS_DEVICE
> +	help
> +	  Say Y here if you want to enable support for MIPI DBI compatible
> +	  panels. The controller command setup can be provided using a
> +	  firmware file.
Consider adding a link to the wiki here - this may make it easier for
the user to find it.

> +	  To compile this driver as a module, choose M here.
> +
>  config DRM_SIMPLEDRM
>  	tristate "Simple framebuffer driver"
>  	depends on DRM && MMU
> diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
> index 5d5505d40e7b..1d9d6227e7ab 100644
> --- a/drivers/gpu/drm/tiny/Makefile
> +++ b/drivers/gpu/drm/tiny/Makefile
> @@ -4,6 +4,7 @@ obj-$(CONFIG_DRM_ARCPGU)		+= arcpgu.o
>  obj-$(CONFIG_DRM_BOCHS)			+= bochs.o
>  obj-$(CONFIG_DRM_CIRRUS_QEMU)		+= cirrus.o
>  obj-$(CONFIG_DRM_GM12U320)		+= gm12u320.o
> +obj-$(CONFIG_DRM_PANEL_MIPI_DBI)	+= panel-mipi-dbi.o
>  obj-$(CONFIG_DRM_SIMPLEDRM)		+= simpledrm.o
>  obj-$(CONFIG_TINYDRM_HX8357D)		+= hx8357d.o
>  obj-$(CONFIG_TINYDRM_ILI9163)		+= ili9163.o
> diff --git a/drivers/gpu/drm/tiny/panel-mipi-dbi.c b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
> new file mode 100644
> index 000000000000..9240fdec38d6
> --- /dev/null
> +++ b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
> @@ -0,0 +1,413 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DRM driver for MIPI DBI compatible display panels
> + *
> + * Copyright 2022 Noralf Trønnes
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/firmware.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_fb_helper.h>
> +#include <drm/drm_gem_atomic_helper.h>
> +#include <drm/drm_gem_cma_helper.h>
> +#include <drm/drm_managed.h>
> +#include <drm/drm_mipi_dbi.h>
> +#include <drm/drm_modeset_helper.h>
> +
> +#include <video/display_timing.h>
> +#include <video/mipi_display.h>
> +#include <video/of_display_timing.h>
> +#include <video/videomode.h>
videomode should not be used in new drivers, it is an fbdev artifact.
But that said - we are still missing a direct display_timing =>
display_mode - so maybe we need it here.

If it is needed Kconfig needs to be extended with:
select VIDEOMODE_HELPERS

> +
> +static const u8 panel_mipi_dbi_magic[15] = { 'M', 'I', 'P', 'I', ' ', 'D', 'B', 'I',
> +					     0, 0, 0, 0, 0, 0, 0 };
> +
> +/*
> + * The optional display controller configuration is stored in a firmware file.
> + * The Device Tree 'compatible' property value with a '.bin' suffix is passed
> + * to request_firmware() to fetch this file.
> + */
> +struct panel_mipi_dbi_config {
> +	/* Magic string: panel_mipi_dbi_magic */
> +	u8 magic[15];
> +
> +	/* Config file format version */
> +	u8 file_format_version;
> +
> +	/*
> +	 * MIPI commands to execute when the display pipeline is enabled.
> +	 * This is used to configure the display controller.
> +	 *
> +	 * The commands are stored in a byte array with the format:
> +	 *     command, num_parameters, [ parameter, ...], command, ...
> +	 *
> +	 * Some commands require a pause before the next command can be received.
> +	 * Inserting a delay in the command sequence is done by using the NOP command with one
> +	 * parameter: delay in miliseconds (the No Operation command is part of the MIPI Display
> +	 * Command Set where it has no parameters).
> +	 *
> +	 * Example:
> +	 *     command 0x11
> +	 *     sleep 120ms
> +	 *     command 0xb1 parameters 0x01, 0x2c, 0x2d
> +	 *     command 0x29
> +	 *
> +	 * Byte sequence:
> +	 *     0x11 0x00
> +	 *     0x00 0x01 0x78
> +	 *     0xb1 0x03 0x01 0x2c 0x2d
> +	 *     0x29 0x00
> +	 */
> +	u8 commands[];
> +};
> +
> +struct panel_mipi_dbi_commands {
> +	const u8 *buf;
> +	size_t len;
> +};
> +
> +static struct panel_mipi_dbi_commands *
> +panel_mipi_dbi_check_commands(struct device *dev, const struct firmware *fw)
> +{
> +	const struct panel_mipi_dbi_config *config = (struct panel_mipi_dbi_config *)fw->data;
> +	struct panel_mipi_dbi_commands *commands;
> +	size_t size = fw->size, commands_len;
> +	unsigned int i = 0;
> +
> +	if (size < sizeof(*config) + 2) { /* At least 1 command */
> +		dev_err(dev, "config: file size=%zu is too small\n", size);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	if (memcmp(config->magic, panel_mipi_dbi_magic, sizeof(config->magic))) {
> +		dev_err(dev, "config: Bad magic: %15ph\n", config->magic);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	if (config->file_format_version != 1) {
> +		dev_err(dev, "config: version=%u is not supported\n", config->file_format_version);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	drm_dev_dbg(dev, DRM_UT_DRIVER, "size=%zu version=%u\n", size, config->file_format_version);
> +
> +	commands_len = size - sizeof(*config);
> +
> +	while ((i + 1) < commands_len) {
> +		u8 command = config->commands[i++];
> +		u8 num_parameters = config->commands[i++];
> +		const u8 *parameters = &config->commands[i];
> +
> +		i += num_parameters;
> +		if (i > commands_len) {
> +			dev_err(dev, "config: command=0x%02x num_parameters=%u overflows\n",
> +				command, num_parameters);
> +			return ERR_PTR(-EINVAL);
> +		}
> +
> +		if (command == 0x00 && num_parameters == 1)
> +			drm_dev_dbg(dev, DRM_UT_DRIVER, "sleep %ums\n", parameters[0]);
> +		else
> +			drm_dev_dbg(dev, DRM_UT_DRIVER, "command %02x %*ph\n",
> +				    command, num_parameters, parameters);
> +	}
> +
> +	if (i != commands_len) {
> +		dev_err(dev, "config: malformed command array\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	commands = devm_kzalloc(dev, sizeof(*commands), GFP_KERNEL);
> +	if (!commands)
> +		return ERR_PTR(-ENOMEM);
> +
> +	commands->len = commands_len;
> +	commands->buf = devm_kmemdup(dev, config->commands, commands->len, GFP_KERNEL);
> +	if (!commands->buf)
> +		return ERR_PTR(-ENOMEM);
> +
> +	return commands;
> +}
> +
> +static struct panel_mipi_dbi_commands *panel_mipi_dbi_commands_from_fw(struct device *dev)
> +{
> +	struct panel_mipi_dbi_commands *commands;
> +	const struct firmware *fw;
> +	const char *compatible;
> +	struct property *prop;
> +	char fw_name[40];
> +	int ret;
> +
> +	of_property_for_each_string(dev->of_node, "compatible", prop, compatible) {
> +		snprintf(fw_name, sizeof(fw_name), "%s.bin", compatible);
> +		ret = firmware_request_nowarn(&fw, fw_name, dev);
> +		if (ret) {
> +			drm_dev_dbg(dev, DRM_UT_DRIVER,
> +				    "No config file found for compatible: '%s' (error=%d)\n",
> +				    compatible, ret);
It would be more helpful to spell out that we failed to find a file
named compatible.bin here as the user may not be aware that the .bin
file is needed.

> +			continue;
> +		}
> +
> +		commands = panel_mipi_dbi_check_commands(dev, fw);
> +		release_firmware(fw);
> +		return commands;
> +	}
> +
> +	return NULL;
> +}
> +
> +static void panel_mipi_dbi_commands_execute(struct mipi_dbi *dbi,
> +					    struct panel_mipi_dbi_commands *commands)
> +{
> +	unsigned int i = 0;
> +
> +	if (!commands)
> +		return;
> +
> +	while (i < commands->len) {
> +		u8 command = commands->buf[i++];
> +		u8 num_parameters = commands->buf[i++];
> +		const u8 *parameters = &commands->buf[i];
> +
> +		if (command == 0x00 && num_parameters == 1)
> +			msleep(parameters[0]);
> +		else if (num_parameters)
> +			mipi_dbi_command_stackbuf(dbi, command, parameters, num_parameters);
> +		else
> +			mipi_dbi_command(dbi, command);
> +
> +		i += num_parameters;
> +	}
> +}
> +
> +static void panel_mipi_dbi_enable(struct drm_simple_display_pipe *pipe,
> +				  struct drm_crtc_state *crtc_state,
> +				  struct drm_plane_state *plane_state)
> +{
> +	struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
> +	struct mipi_dbi *dbi = &dbidev->dbi;
> +	int ret, idx;
> +
> +	if (!drm_dev_enter(pipe->crtc.dev, &idx))
> +		return;
> +
> +	drm_dbg(pipe->crtc.dev, "\n");
> +
> +	ret = mipi_dbi_poweron_conditional_reset(dbidev);
> +	if (ret < 0)
> +		goto out_exit;
> +	if (!ret)
> +		panel_mipi_dbi_commands_execute(dbi, dbidev->driver_private);
> +
> +	mipi_dbi_enable_flush(dbidev, crtc_state, plane_state);
> +out_exit:
> +	drm_dev_exit(idx);
> +}
> +
> +static const struct drm_simple_display_pipe_funcs panel_mipi_dbi_pipe_funcs = {
> +	.enable = panel_mipi_dbi_enable,
> +	.disable = mipi_dbi_pipe_disable,
> +	.update = mipi_dbi_pipe_update,
> +};
> +
> +DEFINE_DRM_GEM_CMA_FOPS(panel_mipi_dbi_fops);
> +
> +static const struct drm_driver panel_mipi_dbi_driver = {
> +	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
> +	.fops			= &panel_mipi_dbi_fops,
> +	DRM_GEM_CMA_DRIVER_OPS_VMAP,
> +	.debugfs_init		= mipi_dbi_debugfs_init,
> +	.name			= "panel-mipi-dbi",
> +	.desc			= "MIPI DBI compatible display panel",
> +	.date			= "20220103",
> +	.major			= 1,
> +	.minor			= 0,
> +};
> +
> +static int panel_mipi_dbi_get_mode(struct mipi_dbi_dev *dbidev, struct drm_display_mode *mode)
> +{
> +	struct device *dev = dbidev->drm.dev;
> +	u32 width_mm = 0, height_mm = 0;
> +	struct display_timing timing;
> +	struct videomode vm;
> +	int ret;
> +
> +	ret = of_get_display_timing(dev->of_node, "panel-timing", &timing);
> +	if (ret) {
> +		dev_err(dev, "%pOF: failed to get panel-timing (error=%d)\n", dev->of_node, ret);
> +		return ret;
> +	}
> +
> +	videomode_from_timing(&timing, &vm);
> +
> +	if (!vm.hactive || vm.hfront_porch || vm.hsync_len ||
> +	    (vm.hback_porch + vm.hactive) > 0xffff ||
> +	    !vm.vactive || vm.vfront_porch || vm.vsync_len ||
> +	    (vm.vback_porch + vm.vactive) > 0xffff ||
> +	    vm.flags) {
> +		dev_err(dev, "%pOF: panel-timing out of bounds\n", dev->of_node);
> +		return -EINVAL;
> +	}
We should have a helper that implements this. Maybe the display_timing
=> display_mode helper could do it.

> +
> +	/* The driver doesn't use the pixel clock but it is mandatory so fake one if not set */
> +	if (!vm.pixelclock)
> +		vm.pixelclock = (vm.hback_porch + vm.hactive) * (vm.vback_porch + vm.vactive) * 60;
> +
> +	dbidev->top_offset = vm.vback_porch;
> +	dbidev->left_offset = vm.hback_porch;
> +
> +	memset(mode, 0, sizeof(*mode));
> +	drm_display_mode_from_videomode(&vm, mode);
> +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> +
> +	ret = device_property_read_u32(dev, "width-mm", &width_mm);
> +	if (ret && ret != -EINVAL)
> +		return ret;
> +
> +	ret = device_property_read_u32(dev, "height-mm", &height_mm);
> +	if (ret && ret != -EINVAL)
> +		return ret;
> +
> +	mode->width_mm = width_mm;
> +	mode->height_mm = height_mm;
> +
> +	drm_mode_debug_printmodeline(mode);
> +
> +	return 0;
> +}
> +
> +static int panel_mipi_dbi_spi_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct drm_display_mode mode;
> +	struct mipi_dbi_dev *dbidev;
> +	struct drm_device *drm;
> +	struct mipi_dbi *dbi;
> +	struct gpio_desc *dc;
> +	int ret;
> +
> +	dbidev = devm_drm_dev_alloc(dev, &panel_mipi_dbi_driver, struct mipi_dbi_dev, drm);
> +	if (IS_ERR(dbidev))
> +		return PTR_ERR(dbidev);
> +
> +	dbi = &dbidev->dbi;
> +	drm = &dbidev->drm;
> +
> +	ret = panel_mipi_dbi_get_mode(dbidev, &mode);
> +	if (ret)
> +		return ret;
> +
> +	dbidev->regulator = devm_regulator_get(dev, "power");
> +	if (IS_ERR(dbidev->regulator))
> +		return dev_err_probe(dev, PTR_ERR(dbidev->regulator),
> +				     "Failed to get regulator 'power'\n");
> +
> +	dbidev->backlight = devm_of_find_backlight(dev);
> +	if (IS_ERR(dbidev->backlight))
> +		return dev_err_probe(dev, PTR_ERR(dbidev->backlight), "Failed to get backlight\n");
> +
> +	dbi->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(dbi->reset))
> +		return dev_err_probe(dev, PTR_ERR(dbi->reset), "Failed to get GPIO 'reset'\n");
> +
> +	dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
> +	if (IS_ERR(dc))
> +		return dev_err_probe(dev, PTR_ERR(dc), "Failed to get GPIO 'dc'\n");
> +
> +	ret = mipi_dbi_spi_init(spi, dbi, dc);
> +	if (ret)
> +		return ret;
> +
> +	if (device_property_present(dev, "write-only"))
> +		dbi->read_commands = NULL;
read_commands are unused - so the write-only property is in practice
ignored.

> +
> +	dbidev->driver_private = panel_mipi_dbi_commands_from_fw(dev);
> +	if (IS_ERR(dbidev->driver_private))
> +		return PTR_ERR(dbidev->driver_private);
> +
> +	ret = mipi_dbi_dev_init(dbidev, &panel_mipi_dbi_pipe_funcs, &mode, 0);
> +	if (ret)
> +		return ret;
> +
> +	drm_mode_config_reset(drm);
> +
> +	ret = drm_dev_register(drm, 0);
> +	if (ret)
> +		return ret;
> +
> +	spi_set_drvdata(spi, drm);
> +
> +	drm_fbdev_generic_setup(drm, 0);
> +
> +	return 0;
> +}
> +
> +static int panel_mipi_dbi_spi_remove(struct spi_device *spi)
> +{
> +	struct drm_device *drm = spi_get_drvdata(spi);
> +
> +	drm_dev_unplug(drm);
> +	drm_atomic_helper_shutdown(drm);
> +
> +	return 0;
> +}
> +
> +static void panel_mipi_dbi_spi_shutdown(struct spi_device *spi)
> +{
> +	drm_atomic_helper_shutdown(spi_get_drvdata(spi));
> +}
> +
> +static int __maybe_unused panel_mipi_dbi_pm_suspend(struct device *dev)
> +{
> +	return drm_mode_config_helper_suspend(dev_get_drvdata(dev));
> +}
> +
> +static int __maybe_unused panel_mipi_dbi_pm_resume(struct device *dev)
> +{
> +	drm_mode_config_helper_resume(dev_get_drvdata(dev));
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops panel_mipi_dbi_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(panel_mipi_dbi_pm_suspend, panel_mipi_dbi_pm_resume)
> +};
> +
> +static const struct of_device_id panel_mipi_dbi_spi_of_match[] = {
> +	{ .compatible = "panel-mipi-dbi-spi" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, panel_mipi_dbi_spi_of_match);
> +
> +static const struct spi_device_id panel_mipi_dbi_spi_id[] = {
> +	{ "panel-mipi-dbi-spi", 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(spi, panel_mipi_dbi_spi_id);
> +
> +static struct spi_driver panel_mipi_dbi_spi_driver = {
> +	.driver = {
> +		.name = "panel-mipi-dbi-spi",
> +		.owner = THIS_MODULE,
> +		.of_match_table = panel_mipi_dbi_spi_of_match,
> +		.pm = &panel_mipi_dbi_pm_ops,
> +	},
> +	.id_table = panel_mipi_dbi_spi_id,
> +	.probe = panel_mipi_dbi_spi_probe,
> +	.remove = panel_mipi_dbi_spi_remove,
> +	.shutdown = panel_mipi_dbi_spi_shutdown,
> +};
> +module_spi_driver(panel_mipi_dbi_spi_driver);
> +
> +MODULE_DESCRIPTION("MIPI DBI compatible display panel driver");
> +MODULE_AUTHOR("Noralf Trønnes");
> +MODULE_LICENSE("GPL");
> -- 
> 2.33.0

Re: [PATCH v4 3/3] drm/tiny: Add MIPI DBI compatible SPI driver

[Sam Ravnborg]

Hi Noralf,

> > +static int panel_mipi_dbi_get_mode(struct mipi_dbi_dev *dbidev, struct drm_display_mode *mode)
> > +{
> > +	struct device *dev = dbidev->drm.dev;
> > +	u32 width_mm = 0, height_mm = 0;
> > +	struct display_timing timing;
> > +	struct videomode vm;
> > +	int ret;
> > +
> > +	ret = of_get_display_timing(dev->of_node, "panel-timing", &timing);
> > +	if (ret) {
> > +		dev_err(dev, "%pOF: failed to get panel-timing (error=%d)\n", dev->of_node, ret);
> > +		return ret;
> > +	}
> > +
> > +	videomode_from_timing(&timing, &vm);
> > +
> > +	if (!vm.hactive || vm.hfront_porch || vm.hsync_len ||
> > +	    (vm.hback_porch + vm.hactive) > 0xffff ||
> > +	    !vm.vactive || vm.vfront_porch || vm.vsync_len ||
> > +	    (vm.vback_porch + vm.vactive) > 0xffff ||
> > +	    vm.flags) {
> > +		dev_err(dev, "%pOF: panel-timing out of bounds\n", dev->of_node);
> > +		return -EINVAL;
> > +	}
> We should have a helper that implements this. Maybe the display_timing
> => display_mode helper could do it.

It would be nice with a drm_display_timing_to_mode() but that can come
later - the comment above should not be understood that I consider it
mandatory for this driver.

	Sam

Re: [PATCH v4 3/3] drm/tiny: Add MIPI DBI compatible SPI driver

[Noralf Trønnes]

Den 20.02.2022 11.04, skrev Sam Ravnborg:
> Hi Noralf,
> 
>>> +static int panel_mipi_dbi_get_mode(struct mipi_dbi_dev *dbidev, struct drm_display_mode *mode)
>>> +{
>>> +	struct device *dev = dbidev->drm.dev;
>>> +	u32 width_mm = 0, height_mm = 0;
>>> +	struct display_timing timing;
>>> +	struct videomode vm;
>>> +	int ret;
>>> +
>>> +	ret = of_get_display_timing(dev->of_node, "panel-timing", &timing);
>>> +	if (ret) {
>>> +		dev_err(dev, "%pOF: failed to get panel-timing (error=%d)\n", dev->of_node, ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	videomode_from_timing(&timing, &vm);
>>> +
>>> +	if (!vm.hactive || vm.hfront_porch || vm.hsync_len ||
>>> +	    (vm.hback_porch + vm.hactive) > 0xffff ||
>>> +	    !vm.vactive || vm.vfront_porch || vm.vsync_len ||
>>> +	    (vm.vback_porch + vm.vactive) > 0xffff ||
>>> +	    vm.flags) {
>>> +		dev_err(dev, "%pOF: panel-timing out of bounds\n", dev->of_node);
>>> +		return -EINVAL;
>>> +	}
>> We should have a helper that implements this. Maybe the display_timing
>> => display_mode helper could do it.
> 
> It would be nice with a drm_display_timing_to_mode() but that can come
> later - the comment above should not be understood that I consider it
> mandatory for this driver.
> 

I did consider adding an of_get_drm_panel_mode() fashioned after
of_get_drm_display_mode() but I didn't find any other driver that would
actually be able to use it and I would have to do some substraction to
get back the {h,v}front_porch values that I need and the optional pixel
clock calculation becomes more complex acting from a drm_display_mode so
I decided against it.

Looking at it now, what I could do is add a function like what
of_get_videomode() does for "display-timings":

/**
 * of_get_panel_videomode - get the panel-timing videomode from devicetree
 * @np: devicenode containing the panel-timing subnode
 * @vm: returns the videomode
 *
 * Returns:
 * Zero on success, negative error code on failure.
 **/
int of_get_panel_videomode(struct device_node *np, struct videomode *vm)
{
	struct display_timing timing;
	int ret;

	ret = of_get_display_timing(np, "panel-timing", &timing);
	if (ret)
		return ret;

	videomode_from_timing(&timing, vm);

	return 0;
}

This could also be used by panel-lvds and 2 fbdev drivers, the other
panel-timing users need/use the display_timing itself, some for bounds
checking.

Noralf.

Re: [PATCH v4 3/3] drm/tiny: Add MIPI DBI compatible SPI driver

[Noralf Trønnes]

> Den 20.02.2022 11.04, skrev Sam Ravnborg:
> > Hi Noralf,
> >
> >>> +static int panel_mipi_dbi_get_mode(struct mipi_dbi_dev *dbidev,
struct drm_display_mode *mode)
> >>> +{
> >>> +	struct device *dev = dbidev->drm.dev;
> >>> +	u32 width_mm = 0, height_mm = 0;
> >>> +	struct display_timing timing;
> >>> +	struct videomode vm;
> >>> +	int ret;
> >>> +
> >>> +	ret = of_get_display_timing(dev->of_node, "panel-timing", &timing);
> >>> +	if (ret) {
> >>> +		dev_err(dev, "%pOF: failed to get panel-timing (error=%d)\n",
dev->of_node, ret);
> >>> +		return ret;
> >>> +	}
> >>> +
> >>> +	videomode_from_timing(&timing, &vm);
> >>> +
> >>> +	if (!vm.hactive || vm.hfront_porch || vm.hsync_len ||
> >>> +	    (vm.hback_porch + vm.hactive) > 0xffff ||
> >>> +	    !vm.vactive || vm.vfront_porch || vm.vsync_len ||
> >>> +	    (vm.vback_porch + vm.vactive) > 0xffff ||
> >>> +	    vm.flags) {
> >>> +		dev_err(dev, "%pOF: panel-timing out of bounds\n", dev->of_node);
> >>> +		return -EINVAL;
> >>> +	}
> >> We should have a helper that implements this. Maybe the display_timing
> >> => display_mode helper could do it.
> >
> > It would be nice with a drm_display_timing_to_mode() but that can come
> > later - the comment above should not be understood that I consider it
> > mandatory for this driver.
> >
>
> I did consider adding an of_get_drm_panel_mode() fashioned after
> of_get_drm_display_mode() but I didn't find any other driver that would
> actually be able to use it and I would have to do some substraction to
> get back the {h,v}front_porch values that I need and the optional pixel
> clock calculation becomes more complex acting from a drm_display_mode so
> I decided against it.
>
> Looking at it now, what I could do is add a function like what
> of_get_videomode() does for "display-timings":
>
> /**
>  * of_get_panel_videomode - get the panel-timing videomode from devicetree
>  * @np: devicenode containing the panel-timing subnode
>  * @vm: returns the videomode
>  *
>  * Returns:
>  * Zero on success, negative error code on failure.
>  **/
> int of_get_panel_videomode(struct device_node *np, struct videomode *vm)
> {
> 	struct display_timing timing;
> 	int ret;
>
> 	ret = of_get_display_timing(np, "panel-timing", &timing);
> 	if (ret)
> 		return ret;
>
> 	videomode_from_timing(&timing, vm);
>
> 	return 0;
> }
>
> This could also be used by panel-lvds and 2 fbdev drivers, the other
> panel-timing users need/use the display_timing itself, some for bounds
> checking.

Scratch that, since videomode is to be avoided I tried adding a
drm_display_mode function and it didn't complicate matter as I though it
would so I'll do that instead:

static int panel_mipi_dbi_get_mode(struct mipi_dbi_dev *dbidev, struct
drm_display_mode *mode)
{
	struct device *dev = dbidev->drm.dev;
	u32 width_mm = 0, height_mm = 0;
	u16 hback_porch, vback_porch;
	struct videomode vm;
	int ret;

	ret = of_get_drm_panel_display_mode(dev->of_node, mode, NULL);
	if (ret) {
		dev_err(dev, "%pOF: failed to get panel-timing (error=%d)\n",
dev->of_node, ret);
		return ret;
	}

	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;

	hback_porch = mode->htotal - mode->hsync_end;
	vback_porch = mode->vtotal - mode->vsync_end;

	if (mode->hsync_end > mode->hdisplay || (hback_porch + mode->hdisplay)
> 0xffff ||
	    mode->vsync_end > mode->vdisplay || (vback_porch + mode->vdisplay)
> 0xffff ||
	    mode->flags) {
		dev_err(dev, "%pOF: panel-timing out of bounds\n", dev->of_node);
		return -EINVAL;
	}

	/* The driver doesn't use the pixel clock but it is mandatory so fake
one if not set */
	if (!mode->pixelclock)
		mode->pixelclock = mode->htotal * mode->vtotal * 60 / 1000;

	dbidev->top_offset = vback_porch;
	dbidev->left_offset = hback_porch;

	return 0;
}


int of_get_drm_panel_display_mode(struct device_node *np,
				  struct drm_display_mode *dmode, u32 *bus_flags)
{
	u32 width_mm = 0, height_mm = 0;
	struct display_timing timing;
	struct videomode vm;
	int ret;

	ret = of_get_display_timing(np, "panel-timing", &timing);
	if (ret)
		return ret;

	videomode_from_timing(&timing, vm);

	memset(dmode, 0, sizeof(*dmode));
	drm_display_mode_from_videomode(&vm, dmode);
	if (bus_flags)
		drm_bus_flags_from_videomode(&vm, bus_flags);

	ret = of_property_read_u32(np, "width-mm", &width_mm);
	if (ret && ret != -EINVAL)
		return ret;

	ret = of_property_read_u32(np, "height-mm", &height_mm);
	if (ret && ret != -EINVAL)
		return ret;

	mode->width_mm = width_mm;
	mode->height_mm = height_mm;

	drm_mode_debug_printmodeline(dmode);

	return 0;
}
EXPORT_SYMBOL_GPL(of_get_drm_display_mode);

Noralf.

Re: [PATCH v4 3/3] drm/tiny: Add MIPI DBI compatible SPI driver

[Sam Ravnborg]

Hi Noralf.

On Sun, Feb 20, 2022 at 07:11:14PM +0100, Noralf Trønnes wrote:
> > Den 20.02.2022 11.04, skrev Sam Ravnborg:
> > > Hi Noralf,
> > >
> > >>> +static int panel_mipi_dbi_get_mode(struct mipi_dbi_dev *dbidev,
> struct drm_display_mode *mode)
> > >>> +{
> > >>> +	struct device *dev = dbidev->drm.dev;
> > >>> +	u32 width_mm = 0, height_mm = 0;
> > >>> +	struct display_timing timing;
> > >>> +	struct videomode vm;
> > >>> +	int ret;
> > >>> +
> > >>> +	ret = of_get_display_timing(dev->of_node, "panel-timing", &timing);
> > >>> +	if (ret) {
> > >>> +		dev_err(dev, "%pOF: failed to get panel-timing (error=%d)\n",
> dev->of_node, ret);
> > >>> +		return ret;
> > >>> +	}
> > >>> +
> > >>> +	videomode_from_timing(&timing, &vm);
> > >>> +
> > >>> +	if (!vm.hactive || vm.hfront_porch || vm.hsync_len ||
> > >>> +	    (vm.hback_porch + vm.hactive) > 0xffff ||
> > >>> +	    !vm.vactive || vm.vfront_porch || vm.vsync_len ||
> > >>> +	    (vm.vback_porch + vm.vactive) > 0xffff ||
> > >>> +	    vm.flags) {
> > >>> +		dev_err(dev, "%pOF: panel-timing out of bounds\n", dev->of_node);
> > >>> +		return -EINVAL;
> > >>> +	}
> > >> We should have a helper that implements this. Maybe the display_timing
> > >> => display_mode helper could do it.
> > >
> > > It would be nice with a drm_display_timing_to_mode() but that can come
> > > later - the comment above should not be understood that I consider it
> > > mandatory for this driver.
> > >
> >
> > I did consider adding an of_get_drm_panel_mode() fashioned after
> > of_get_drm_display_mode() but I didn't find any other driver that would
> > actually be able to use it and I would have to do some substraction to
> > get back the {h,v}front_porch values that I need and the optional pixel
> > clock calculation becomes more complex acting from a drm_display_mode so
> > I decided against it.
> >
> > Looking at it now, what I could do is add a function like what
> > of_get_videomode() does for "display-timings":
> >
> > /**
> >  * of_get_panel_videomode - get the panel-timing videomode from devicetree
> >  * @np: devicenode containing the panel-timing subnode
> >  * @vm: returns the videomode
> >  *
> >  * Returns:
> >  * Zero on success, negative error code on failure.
> >  **/
> > int of_get_panel_videomode(struct device_node *np, struct videomode *vm)
> > {
> > 	struct display_timing timing;
> > 	int ret;
> >
> > 	ret = of_get_display_timing(np, "panel-timing", &timing);
> > 	if (ret)
> > 		return ret;
> >
> > 	videomode_from_timing(&timing, vm);
> >
> > 	return 0;
> > }
> >
> > This could also be used by panel-lvds and 2 fbdev drivers, the other
> > panel-timing users need/use the display_timing itself, some for bounds
> > checking.
> 
> Scratch that, since videomode is to be avoided I tried adding a
> drm_display_mode function and it didn't complicate matter as I though it
> would so I'll do that instead:

I like, but would like to get rid of video_mode entirely.

> 
> static int panel_mipi_dbi_get_mode(struct mipi_dbi_dev *dbidev, struct
> drm_display_mode *mode)
> {
> 	struct device *dev = dbidev->drm.dev;
> 	u32 width_mm = 0, height_mm = 0;
> 	u16 hback_porch, vback_porch;
> 	struct videomode vm;
> 	int ret;
> 
> 	ret = of_get_drm_panel_display_mode(dev->of_node, mode, NULL);
> 	if (ret) {
> 		dev_err(dev, "%pOF: failed to get panel-timing (error=%d)\n",
> dev->of_node, ret);
> 		return ret;
> 	}
> 
> 	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
> 
> 	hback_porch = mode->htotal - mode->hsync_end;
> 	vback_porch = mode->vtotal - mode->vsync_end;
The accesss functions I posed below could be used here - so we avoid
typing these (trivial) calculations in many places.

> 
> 	if (mode->hsync_end > mode->hdisplay || (hback_porch + mode->hdisplay)
> > 0xffff ||
> 	    mode->vsync_end > mode->vdisplay || (vback_porch + mode->vdisplay)
> > 0xffff ||
> 	    mode->flags) {
> 		dev_err(dev, "%pOF: panel-timing out of bounds\n", dev->of_node);
> 		return -EINVAL;
> 	}
With the display_timing => drm_display_mode I think the above is no
longer required.

> 
> 	/* The driver doesn't use the pixel clock but it is mandatory so fake
> one if not set */
> 	if (!mode->pixelclock)
> 		mode->pixelclock = mode->htotal * mode->vtotal * 60 / 1000;
> 
> 	dbidev->top_offset = vback_porch;
> 	dbidev->left_offset = hback_porch;
> 
> 	return 0;
> }
> 
> 
> int of_get_drm_panel_display_mode(struct device_node *np,
> 				  struct drm_display_mode *dmode, u32 *bus_flags)
> {
Not sure about the bus_flags argument here - seems misplaced.

> 	u32 width_mm = 0, height_mm = 0;
> 	struct display_timing timing;
> 	struct videomode vm;
> 	int ret;
> 
> 	ret = of_get_display_timing(np, "panel-timing", &timing);
> 	if (ret)
> 		return ret;
> 
> 	videomode_from_timing(&timing, vm);
> 
> 	memset(dmode, 0, sizeof(*dmode));
> 	drm_display_mode_from_videomode(&vm, dmode);
> 	if (bus_flags)
> 		drm_bus_flags_from_videomode(&vm, bus_flags);

This does a:
display_timing => video_mode => drm_display_display_mode

We could do a:
display_timing => drm_display_mode.

Sample implementation could look like this:
void drm_mode_from_display_timing(struct drm_display_mode *mode,
                                  const struct display_timing *dt)
{
	mode->hdisplay = dt->hactive.typ;
        mode->hsync_start = dt->hactive.typ + dt->hfront_porch.typ;
        mode->hsync_end = dt->hactive.typ + dt->hfront_porch.typ + dt->hsync_len.typ;
        mode->htotal = dt->hactive.typ + dt->hfront_porch.typ + dt->hsync_len.typ + dt->hback_porch.typ;

        mode->vdisplay = dt->vactive.typ;
        mode->vsync_start = dt->vactive.typ + dt->vfront_porch.typ;
        mode->vsync_end = dt->vactive.typ + dt->vfront_porch.typ + dt->vsync_len.typ;
        mode->vtotal = dt->vactive.typ + dt->vfront_porch.typ + dt->vsync_len.typ + dt->vback_porch.typ;

        mode->clock = dt->pixelclock.typ / 1000;

        mode->flags = 0;
        if (dt->flags & DISPLAY_FLAGS_HSYNC_HIGH)
                mode->flags |= DRM_MODE_FLAG_PHSYNC;
        else if (dt->flags & DISPLAY_FLAGS_HSYNC_LOW)
                mode->flags |= DRM_MODE_FLAG_NHSYNC;
        if (dt->flags & DISPLAY_FLAGS_VSYNC_HIGH)
                mode->flags |= DRM_MODE_FLAG_PVSYNC;
        else if (dt->flags & DISPLAY_FLAGS_VSYNC_LOW)
                mode->flags |= DRM_MODE_FLAG_NVSYNC;
        if (dt->flags & DISPLAY_FLAGS_INTERLACED)
                mode->flags |= DRM_MODE_FLAG_INTERLACE;
        if (dt->flags & DISPLAY_FLAGS_DOUBLESCAN)
                mode->flags |= DRM_MODE_FLAG_DBLSCAN;
        if (dt->flags & DISPLAY_FLAGS_DOUBLECLK)
                mode->flags |= DRM_MODE_FLAG_DBLCLK;
        drm_mode_set_name(mode);
}
EXPORT_SYMBOL_GPL(drm_mode_from_display_timing);

If we then on top of this would like easy access to the names we know we
could add a few access functions:

static inline u32 drm_mode_hactive(const struct drm_display_mode *mode)
{
        mode->hdisplay;
}

static inline u32 drm_mode_hfront_porch(const struct drm_display_mode *mode)
{
        mode->hsync_start - mode->hdisplay;
}

static inline u32 drm_mode_hback_porch(const struct drm_display_mode *mode)
{
        mode->htotal - mode->hsync_start;
}

static inline u32 drm_mode_hsync_len(const struct drm_display_mode *mode)
{
        return mode->hsync_end - mode->hsync_start;
}

static inline u32 drm_mode_vactive(const struct drm_display_mode *mode)
{
        return mode->vdisplay;
}

static inline u32 drm_mode_vfront_porch(const struct drm_display_mode *mode)
{
        return mode->vsync_start - mode->vdisplay;
}

static inline u32 drm_mode_vsync_len(const struct drm_display_mode *mode)
{
        return mode->vsync_end - mode->vsync_start;
}

static inline u32 drm_mode_vback_porch(const struct drm_display_mode *mode)
{
        return mode->vtotal - mode->vsync_end;
}


The above was something I just quickly typed. This was the easy part.
Writing kernel-.doc and fix it so it works is the time consuming part..

	Sam

Re: [PATCH v4 3/3] drm/tiny: Add MIPI DBI compatible SPI driver

[Noralf Trønnes]

Den 20.02.2022 20.57, skrev Sam Ravnborg:
> Hi Noralf.
> 
> On Sun, Feb 20, 2022 at 07:11:14PM +0100, Noralf Trønnes wrote:
>>> Den 20.02.2022 11.04, skrev Sam Ravnborg:
>>>> Hi Noralf,
>>>>
>>>>>> +static int panel_mipi_dbi_get_mode(struct mipi_dbi_dev *dbidev,
>> struct drm_display_mode *mode)
>>>>>> +{
>>>>>> +	struct device *dev = dbidev->drm.dev;
>>>>>> +	u32 width_mm = 0, height_mm = 0;
>>>>>> +	struct display_timing timing;
>>>>>> +	struct videomode vm;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	ret = of_get_display_timing(dev->of_node, "panel-timing", &timing);
>>>>>> +	if (ret) {
>>>>>> +		dev_err(dev, "%pOF: failed to get panel-timing (error=%d)\n",
>> dev->of_node, ret);
>>>>>> +		return ret;
>>>>>> +	}
>>>>>> +
>>>>>> +	videomode_from_timing(&timing, &vm);
>>>>>> +
>>>>>> +	if (!vm.hactive || vm.hfront_porch || vm.hsync_len ||
>>>>>> +	    (vm.hback_porch + vm.hactive) > 0xffff ||
>>>>>> +	    !vm.vactive || vm.vfront_porch || vm.vsync_len ||
>>>>>> +	    (vm.vback_porch + vm.vactive) > 0xffff ||
>>>>>> +	    vm.flags) {
>>>>>> +		dev_err(dev, "%pOF: panel-timing out of bounds\n", dev->of_node);
>>>>>> +		return -EINVAL;
>>>>>> +	}
>>>>> We should have a helper that implements this. Maybe the display_timing
>>>>> => display_mode helper could do it.
>>>>
>>>> It would be nice with a drm_display_timing_to_mode() but that can come
>>>> later - the comment above should not be understood that I consider it
>>>> mandatory for this driver.
>>>>
>>>
>>> I did consider adding an of_get_drm_panel_mode() fashioned after
>>> of_get_drm_display_mode() but I didn't find any other driver that would
>>> actually be able to use it and I would have to do some substraction to
>>> get back the {h,v}front_porch values that I need and the optional pixel
>>> clock calculation becomes more complex acting from a drm_display_mode so
>>> I decided against it.
>>>
>>> Looking at it now, what I could do is add a function like what
>>> of_get_videomode() does for "display-timings":
>>>
>>> /**
>>>  * of_get_panel_videomode - get the panel-timing videomode from devicetree
>>>  * @np: devicenode containing the panel-timing subnode
>>>  * @vm: returns the videomode
>>>  *
>>>  * Returns:
>>>  * Zero on success, negative error code on failure.
>>>  **/
>>> int of_get_panel_videomode(struct device_node *np, struct videomode *vm)
>>> {
>>> 	struct display_timing timing;
>>> 	int ret;
>>>
>>> 	ret = of_get_display_timing(np, "panel-timing", &timing);
>>> 	if (ret)
>>> 		return ret;
>>>
>>> 	videomode_from_timing(&timing, vm);
>>>
>>> 	return 0;
>>> }
>>>
>>> This could also be used by panel-lvds and 2 fbdev drivers, the other
>>> panel-timing users need/use the display_timing itself, some for bounds
>>> checking.
>>
>> Scratch that, since videomode is to be avoided I tried adding a
>> drm_display_mode function and it didn't complicate matter as I though it
>> would so I'll do that instead:
> 
> I like, but would like to get rid of video_mode entirely.
> 
>>
>> static int panel_mipi_dbi_get_mode(struct mipi_dbi_dev *dbidev, struct
>> drm_display_mode *mode)
>> {
>> 	struct device *dev = dbidev->drm.dev;
>> 	u32 width_mm = 0, height_mm = 0;
>> 	u16 hback_porch, vback_porch;
>> 	struct videomode vm;
>> 	int ret;
>>
>> 	ret = of_get_drm_panel_display_mode(dev->of_node, mode, NULL);
>> 	if (ret) {
>> 		dev_err(dev, "%pOF: failed to get panel-timing (error=%d)\n",
>> dev->of_node, ret);
>> 		return ret;
>> 	}
>>
>> 	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
>>
>> 	hback_porch = mode->htotal - mode->hsync_end;
>> 	vback_porch = mode->vtotal - mode->vsync_end;
> The accesss functions I posed below could be used here - so we avoid
> typing these (trivial) calculations in many places.
> 
>>
>> 	if (mode->hsync_end > mode->hdisplay || (hback_porch + mode->hdisplay)
>>> 0xffff ||
>> 	    mode->vsync_end > mode->vdisplay || (vback_porch + mode->vdisplay)
>>> 0xffff ||
>> 	    mode->flags) {
>> 		dev_err(dev, "%pOF: panel-timing out of bounds\n", dev->of_node);
>> 		return -EINVAL;
>> 	}
> With the display_timing => drm_display_mode I think the above is no
> longer required.
> 

I still need to verify the values to ensure that front_porch and
sync_len are zero. Maybe I need a comment now to tell what I'm checking
since I'm further away from the DT values:

/*
 * Make sure width and height are set and that only back porch and
 * pixelclock are set in the other timing values. Also check that
 * width and height don't exceed the 16-bit value specified by MIPI DCS.
 */

>>
>> 	/* The driver doesn't use the pixel clock but it is mandatory so fake
>> one if not set */
>> 	if (!mode->pixelclock)
>> 		mode->pixelclock = mode->htotal * mode->vtotal * 60 / 1000;
>>
>> 	dbidev->top_offset = vback_porch;
>> 	dbidev->left_offset = hback_porch;
>>
>> 	return 0;
>> }
>>
>>
>> int of_get_drm_panel_display_mode(struct device_node *np,
>> 				  struct drm_display_mode *dmode, u32 *bus_flags)
>> {
> Not sure about the bus_flags argument here - seems misplaced.
> 

I did the same as of_get_drm_display_mode(), don't panel drivers need
the bus flags?

>> 	u32 width_mm = 0, height_mm = 0;
>> 	struct display_timing timing;
>> 	struct videomode vm;
>> 	int ret;
>>
>> 	ret = of_get_display_timing(np, "panel-timing", &timing);
>> 	if (ret)
>> 		return ret;
>>
>> 	videomode_from_timing(&timing, vm);
>>
>> 	memset(dmode, 0, sizeof(*dmode));
>> 	drm_display_mode_from_videomode(&vm, dmode);
>> 	if (bus_flags)
>> 		drm_bus_flags_from_videomode(&vm, bus_flags);
> 
> This does a:
> display_timing => video_mode => drm_display_display_mode
> 
> We could do a:
> display_timing => drm_display_mode.
> 

I'll leave this to others to sort out. I want the function to look the
same as of_get_drm_display_mode() and it uses videomode. If videomode
goes away both can be fixed at the same time.

> Sample implementation could look like this:
> void drm_mode_from_display_timing(struct drm_display_mode *mode,
>                                   const struct display_timing *dt)
> {
> 	mode->hdisplay = dt->hactive.typ;
>         mode->hsync_start = dt->hactive.typ + dt->hfront_porch.typ;
>         mode->hsync_end = dt->hactive.typ + dt->hfront_porch.typ + dt->hsync_len.typ;
>         mode->htotal = dt->hactive.typ + dt->hfront_porch.typ + dt->hsync_len.typ + dt->hback_porch.typ;
> 
>         mode->vdisplay = dt->vactive.typ;
>         mode->vsync_start = dt->vactive.typ + dt->vfront_porch.typ;
>         mode->vsync_end = dt->vactive.typ + dt->vfront_porch.typ + dt->vsync_len.typ;
>         mode->vtotal = dt->vactive.typ + dt->vfront_porch.typ + dt->vsync_len.typ + dt->vback_porch.typ;
> 
>         mode->clock = dt->pixelclock.typ / 1000;
> 
>         mode->flags = 0;
>         if (dt->flags & DISPLAY_FLAGS_HSYNC_HIGH)
>                 mode->flags |= DRM_MODE_FLAG_PHSYNC;
>         else if (dt->flags & DISPLAY_FLAGS_HSYNC_LOW)
>                 mode->flags |= DRM_MODE_FLAG_NHSYNC;
>         if (dt->flags & DISPLAY_FLAGS_VSYNC_HIGH)
>                 mode->flags |= DRM_MODE_FLAG_PVSYNC;
>         else if (dt->flags & DISPLAY_FLAGS_VSYNC_LOW)
>                 mode->flags |= DRM_MODE_FLAG_NVSYNC;
>         if (dt->flags & DISPLAY_FLAGS_INTERLACED)
>                 mode->flags |= DRM_MODE_FLAG_INTERLACE;
>         if (dt->flags & DISPLAY_FLAGS_DOUBLESCAN)
>                 mode->flags |= DRM_MODE_FLAG_DBLSCAN;
>         if (dt->flags & DISPLAY_FLAGS_DOUBLECLK)
>                 mode->flags |= DRM_MODE_FLAG_DBLCLK;
>         drm_mode_set_name(mode);
> }
> EXPORT_SYMBOL_GPL(drm_mode_from_display_timing);
> 
> If we then on top of this would like easy access to the names we know we
> could add a few access functions:
> 

I don't think I'll do these either, it's more work than I can invest in
this.

Noralf.

> static inline u32 drm_mode_hactive(const struct drm_display_mode *mode)
> {
>         mode->hdisplay;
> }
> 
> static inline u32 drm_mode_hfront_porch(const struct drm_display_mode *mode)
> {
>         mode->hsync_start - mode->hdisplay;
> }
> 
> static inline u32 drm_mode_hback_porch(const struct drm_display_mode *mode)
> {
>         mode->htotal - mode->hsync_start;
> }
> 
> static inline u32 drm_mode_hsync_len(const struct drm_display_mode *mode)
> {
>         return mode->hsync_end - mode->hsync_start;
> }
> 
> static inline u32 drm_mode_vactive(const struct drm_display_mode *mode)
> {
>         return mode->vdisplay;
> }
> 
> static inline u32 drm_mode_vfront_porch(const struct drm_display_mode *mode)
> {
>         return mode->vsync_start - mode->vdisplay;
> }
> 
> static inline u32 drm_mode_vsync_len(const struct drm_display_mode *mode)
> {
>         return mode->vsync_end - mode->vsync_start;
> }
> 
> static inline u32 drm_mode_vback_porch(const struct drm_display_mode *mode)
> {
>         return mode->vtotal - mode->vsync_end;
> }
> 
> 
> The above was something I just quickly typed. This was the easy part.
> Writing kernel-.doc and fix it so it works is the time consuming part..
> 
> 	Sam

Re: [PATCH v4 3/3] drm/tiny: Add MIPI DBI compatible SPI driver

[Sam Ravnborg]

Hi Noralf,

> >> 	    mode->flags) {
> >> 		dev_err(dev, "%pOF: panel-timing out of bounds\n", dev->of_node);
> >> 		return -EINVAL;
> >> 	}
> > With the display_timing => drm_display_mode I think the above is no
> > longer required.
> > 
> 
> I still need to verify the values to ensure that front_porch and
> sync_len are zero. Maybe I need a comment now to tell what I'm checking
> since I'm further away from the DT values:
> 
> /*
>  * Make sure width and height are set and that only back porch and
>  * pixelclock are set in the other timing values. Also check that
>  * width and height don't exceed the 16-bit value specified by MIPI DCS.
>  */
Yes, that would be nice.
> 
> >>
> >> 	/* The driver doesn't use the pixel clock but it is mandatory so fake
> >> one if not set */
> >> 	if (!mode->pixelclock)
> >> 		mode->pixelclock = mode->htotal * mode->vtotal * 60 / 1000;
> >>
> >> 	dbidev->top_offset = vback_porch;
> >> 	dbidev->left_offset = hback_porch;
> >>
> >> 	return 0;
> >> }
> >>
> >>
> >> int of_get_drm_panel_display_mode(struct device_node *np,
> >> 				  struct drm_display_mode *dmode, u32 *bus_flags)
> >> {
> > Not sure about the bus_flags argument here - seems misplaced.
> > 
> 
> I did the same as of_get_drm_display_mode(), don't panel drivers need
> the bus flags?

In my haste I missed the display_timing combines flags for the bus and
the mode - so yes it is needed.


> 
> >> 	u32 width_mm = 0, height_mm = 0;
> >> 	struct display_timing timing;
> >> 	struct videomode vm;
> >> 	int ret;
> >>
> >> 	ret = of_get_display_timing(np, "panel-timing", &timing);
> >> 	if (ret)
> >> 		return ret;
> >>
> >> 	videomode_from_timing(&timing, vm);
> >>
> >> 	memset(dmode, 0, sizeof(*dmode));
> >> 	drm_display_mode_from_videomode(&vm, dmode);
> >> 	if (bus_flags)
> >> 		drm_bus_flags_from_videomode(&vm, bus_flags);
> > 
> > This does a:
> > display_timing => video_mode => drm_display_display_mode
> > 
> > We could do a:
> > display_timing => drm_display_mode.
> > 
> 
> I'll leave this to others to sort out. I want the function to look the
> same as of_get_drm_display_mode() and it uses videomode. If videomode
> goes away both can be fixed at the same time.

When I have dig myself out of the bridge hole I am in I may take a
look at this.

	Sam

Re: [PATCH v4 3/3] drm/tiny: Add MIPI DBI compatible SPI driver

[Noralf Trønnes]

Den 19.02.2022 23.10, skrev Sam Ravnborg:
> Hi Noralf,
> On Fri, Feb 18, 2022 at 04:11:10PM +0100, Noralf Trønnes wrote:
>> Add a driver that will work with most MIPI DBI compatible SPI panels.
>> This avoids adding a driver for every new MIPI DBI compatible controller
>> that is to be used by Linux. The 'compatible' Device Tree property with
>> a '.bin' suffix will be used to load a firmware file that contains the
>> controller configuration.
> A solution where we have the command sequences as part of the DT-overlay
> is IMO a much better way to solve this:
> - The users needs to deal only with a single file, so there is less that
>   goes wrong
> - The user need not reading special instructions how to handle a .bin
>   file, if the overlay is present everything is fine
> - The file that contains the command sequences can be properly annotated
>   with comments
> - The people that creates the command sequences has no need for a special
>   script to create the file for a display - this is all readable ascii.
> - Using a DT-overlay the parsing of the DT-overlay can be done by
>   well-tested functions, no need to invent something new to parse the
>   file
> 
> 
> The idea with a common mipi DBI SPI driver is super, it is the detail
> with the .bin file that I am against.
> 

The fbtft drivers has an init= DT property that contains the command
sequence. Example for the MZ61581 display:

				init = <0x10000b0 00
					0x1000011
					0x20000ff
					0x10000b3 0x02 0x00 0x00 0x00
					0x10000c0 0x13 0x3b 0x00 0x02 0x00 0x01 0x00 0x43
					0x10000c1 0x08 0x16 0x08 0x08
					0x10000c4 0x11 0x07 0x03 0x03
					0x10000c6 0x00
					0x10000c8 0x03 0x03 0x13 0x5c 0x03 0x07 0x14 0x08 0x00 0x21 0x08
0x14 0x07 0x53 0x0c 0x13 0x03 0x03 0x21 0x00
					0x1000035 0x00
					0x1000036 0xa0
					0x100003a 0x55
					0x1000044 0x00 0x01
					0x10000d0 0x07 0x07 0x1d 0x03
					0x10000d1 0x03 0x30 0x10
					0x10000d2 0x03 0x14 0x04
					0x1000029
					0x100002c>;

Parsed here:
https://elixir.bootlin.com/linux/latest/C/ident/fbtft_init_display_from_property

Before fbdev was closed for new drivers I looked at fixing up fbtft for
proper inclusion and asked on the Device Tree mailinglist about the init
property and how to handle the controller configuration generically.
I was politely told that this should be done in the driver, so when I
made my first DRM driver I made a driver specifically for a panel
(mi0283qt.c).

Later I found another post that in clear words stated that setting
random registers from DT was not acceptable.

So I think Maxime has provided a clever way of dealing with this problem
to let us have a generic driver: The OS is in charge of how to configure
the controller and in this case does it using a firmware file.

> With the above said, a few comments to the current implementation below.
> As we know it from you - a very well-written driver.
> 
> 	Sam
> 
>> Acked-by: Maxime Ripard <maxime@cerno.tech>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---

>> diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
>> index 712e0004e96e..d552e1618da7 100644
>> --- a/drivers/gpu/drm/tiny/Kconfig
>> +++ b/drivers/gpu/drm/tiny/Kconfig
>> @@ -51,6 +51,19 @@ config DRM_GM12U320
>>  	 This is a KMS driver for projectors which use the GM12U320 chipset
>>  	 for video transfer over USB2/3, such as the Acer C120 mini projector.
>>  
>> +config DRM_PANEL_MIPI_DBI
>> +	tristate "DRM support for MIPI DBI compatible panels"
>> +	depends on DRM && SPI
>> +	select DRM_KMS_HELPER
>> +	select DRM_KMS_CMA_HELPER
> This symbol is not present in my drm-misc-next tree (which is a few
> weeks old, so it may be newer).
> 

Turns out this was removed in 09717af7d13d.
This should be DRM_GEM_CMA_HELPER now.

>> +	select DRM_MIPI_DBI
>> +	select BACKLIGHT_CLASS_DEVICE
>> +	help
>> +	  Say Y here if you want to enable support for MIPI DBI compatible
>> +	  panels. The controller command setup can be provided using a
>> +	  firmware file.
> Consider adding a link to the wiki here - this may make it easier for
> the user to find it.
> 
>> +	  To compile this driver as a module, choose M here.
>> +
>>  config DRM_SIMPLEDRM
>>  	tristate "Simple framebuffer driver"
>>  	depends on DRM && MMU

>> diff --git a/drivers/gpu/drm/tiny/panel-mipi-dbi.c b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
>> new file mode 100644
>> index 000000000000..9240fdec38d6
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
>> @@ -0,0 +1,413 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * DRM driver for MIPI DBI compatible display panels
>> + *
>> + * Copyright 2022 Noralf Trønnes
>> + */
>> +
>> +#include <linux/backlight.h>
>> +#include <linux/delay.h>
>> +#include <linux/firmware.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/module.h>
>> +#include <linux/property.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/spi/spi.h>
>> +
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_drv.h>
>> +#include <drm/drm_fb_helper.h>
>> +#include <drm/drm_gem_atomic_helper.h>
>> +#include <drm/drm_gem_cma_helper.h>
>> +#include <drm/drm_managed.h>
>> +#include <drm/drm_mipi_dbi.h>
>> +#include <drm/drm_modeset_helper.h>
>> +
>> +#include <video/display_timing.h>
>> +#include <video/mipi_display.h>
>> +#include <video/of_display_timing.h>
>> +#include <video/videomode.h>
> videomode should not be used in new drivers, it is an fbdev artifact.
> But that said - we are still missing a direct display_timing =>
> display_mode - so maybe we need it here.
> 
> If it is needed Kconfig needs to be extended with:
> select VIDEOMODE_HELPERS
> 

Good catch!

>> +static int panel_mipi_dbi_spi_probe(struct spi_device *spi)
>> +{
>> +	struct device *dev = &spi->dev;
>> +	struct drm_display_mode mode;
>> +	struct mipi_dbi_dev *dbidev;
>> +	struct drm_device *drm;
>> +	struct mipi_dbi *dbi;
>> +	struct gpio_desc *dc;
>> +	int ret;
>> +
>> +	dbidev = devm_drm_dev_alloc(dev, &panel_mipi_dbi_driver, struct mipi_dbi_dev, drm);
>> +	if (IS_ERR(dbidev))
>> +		return PTR_ERR(dbidev);
>> +
>> +	dbi = &dbidev->dbi;
>> +	drm = &dbidev->drm;
>> +
>> +	ret = panel_mipi_dbi_get_mode(dbidev, &mode);
>> +	if (ret)
>> +		return ret;
>> +
>> +	dbidev->regulator = devm_regulator_get(dev, "power");
>> +	if (IS_ERR(dbidev->regulator))
>> +		return dev_err_probe(dev, PTR_ERR(dbidev->regulator),
>> +				     "Failed to get regulator 'power'\n");
>> +
>> +	dbidev->backlight = devm_of_find_backlight(dev);
>> +	if (IS_ERR(dbidev->backlight))
>> +		return dev_err_probe(dev, PTR_ERR(dbidev->backlight), "Failed to get backlight\n");
>> +
>> +	dbi->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(dbi->reset))
>> +		return dev_err_probe(dev, PTR_ERR(dbi->reset), "Failed to get GPIO 'reset'\n");
>> +
>> +	dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
>> +	if (IS_ERR(dc))
>> +		return dev_err_probe(dev, PTR_ERR(dc), "Failed to get GPIO 'dc'\n");
>> +
>> +	ret = mipi_dbi_spi_init(spi, dbi, dc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (device_property_present(dev, "write-only"))
>> +		dbi->read_commands = NULL;
> read_commands are unused - so the write-only property is in practice
> ignored.
> 

>read_commands is set to a default in mipi_dbi_spi_init() and we clear
it when write-only.

Noralf.

Re: [PATCH v4 3/3] drm/tiny: Add MIPI DBI compatible SPI driver

[Sam Ravnborg]

Hi Noralf,

On Sun, Feb 20, 2022 at 04:59:34PM +0100, Noralf Trønnes wrote:
> 
> 
> Den 19.02.2022 23.10, skrev Sam Ravnborg:
> > Hi Noralf,
> > On Fri, Feb 18, 2022 at 04:11:10PM +0100, Noralf Trønnes wrote:
> >> Add a driver that will work with most MIPI DBI compatible SPI panels.
> >> This avoids adding a driver for every new MIPI DBI compatible controller
> >> that is to be used by Linux. The 'compatible' Device Tree property with
> >> a '.bin' suffix will be used to load a firmware file that contains the
> >> controller configuration.
> > A solution where we have the command sequences as part of the DT-overlay
> > is IMO a much better way to solve this:
> > - The users needs to deal only with a single file, so there is less that
> >   goes wrong
> > - The user need not reading special instructions how to handle a .bin
> >   file, if the overlay is present everything is fine
> > - The file that contains the command sequences can be properly annotated
> >   with comments
> > - The people that creates the command sequences has no need for a special
> >   script to create the file for a display - this is all readable ascii.
> > - Using a DT-overlay the parsing of the DT-overlay can be done by
> >   well-tested functions, no need to invent something new to parse the
> >   file
> > 
> > 
> > The idea with a common mipi DBI SPI driver is super, it is the detail
> > with the .bin file that I am against.
> > 
> 
> The fbtft drivers has an init= DT property that contains the command
> sequence. Example for the MZ61581 display:
> 
> 				init = <0x10000b0 00
> 					0x1000011
> 					0x20000ff
> 					0x10000b3 0x02 0x00 0x00 0x00
> 					0x10000c0 0x13 0x3b 0x00 0x02 0x00 0x01 0x00 0x43
> 					0x10000c1 0x08 0x16 0x08 0x08
> 					0x10000c4 0x11 0x07 0x03 0x03
> 					0x10000c6 0x00
> 					0x10000c8 0x03 0x03 0x13 0x5c 0x03 0x07 0x14 0x08 0x00 0x21 0x08
> 0x14 0x07 0x53 0x0c 0x13 0x03 0x03 0x21 0x00
> 					0x1000035 0x00
> 					0x1000036 0xa0
> 					0x100003a 0x55
> 					0x1000044 0x00 0x01
> 					0x10000d0 0x07 0x07 0x1d 0x03
> 					0x10000d1 0x03 0x30 0x10
> 					0x10000d2 0x03 0x14 0x04
> 					0x1000029
> 					0x100002c>;
> 
> Parsed here:
> https://elixir.bootlin.com/linux/latest/C/ident/fbtft_init_display_from_property
> 
> Before fbdev was closed for new drivers I looked at fixing up fbtft for
> proper inclusion and asked on the Device Tree mailinglist about the init
> property and how to handle the controller configuration generically.
> I was politely told that this should be done in the driver, so when I
> made my first DRM driver I made a driver specifically for a panel
> (mi0283qt.c).
> 
> Later I found another post that in clear words stated that setting
> random registers from DT was not acceptable.
Understood and thanks for the explanation.
It is a shame that the users loose here :-(

	Sam

Re: [PATCH v4 3/3] drm/tiny: Add MIPI DBI compatible SPI driver

[Maxime Ripard]

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

On Sun, Feb 20, 2022 at 10:32:25PM +0100, Sam Ravnborg wrote:
> Hi Noralf,
> 
> On Sun, Feb 20, 2022 at 04:59:34PM +0100, Noralf Trønnes wrote:
> > 
> > 
> > Den 19.02.2022 23.10, skrev Sam Ravnborg:
> > > Hi Noralf,
> > > On Fri, Feb 18, 2022 at 04:11:10PM +0100, Noralf Trønnes wrote:
> > >> Add a driver that will work with most MIPI DBI compatible SPI panels.
> > >> This avoids adding a driver for every new MIPI DBI compatible controller
> > >> that is to be used by Linux. The 'compatible' Device Tree property with
> > >> a '.bin' suffix will be used to load a firmware file that contains the
> > >> controller configuration.
> > > A solution where we have the command sequences as part of the DT-overlay
> > > is IMO a much better way to solve this:
> > > - The users needs to deal only with a single file, so there is less that
> > >   goes wrong
> > > - The user need not reading special instructions how to handle a .bin
> > >   file, if the overlay is present everything is fine
> > > - The file that contains the command sequences can be properly annotated
> > >   with comments
> > > - The people that creates the command sequences has no need for a special
> > >   script to create the file for a display - this is all readable ascii.
> > > - Using a DT-overlay the parsing of the DT-overlay can be done by
> > >   well-tested functions, no need to invent something new to parse the
> > >   file
> > > 
> > > 
> > > The idea with a common mipi DBI SPI driver is super, it is the detail
> > > with the .bin file that I am against.
> > > 
> > 
> > The fbtft drivers has an init= DT property that contains the command
> > sequence. Example for the MZ61581 display:
> > 
> > 				init = <0x10000b0 00
> > 					0x1000011
> > 					0x20000ff
> > 					0x10000b3 0x02 0x00 0x00 0x00
> > 					0x10000c0 0x13 0x3b 0x00 0x02 0x00 0x01 0x00 0x43
> > 					0x10000c1 0x08 0x16 0x08 0x08
> > 					0x10000c4 0x11 0x07 0x03 0x03
> > 					0x10000c6 0x00
> > 					0x10000c8 0x03 0x03 0x13 0x5c 0x03 0x07 0x14 0x08 0x00 0x21 0x08
> > 0x14 0x07 0x53 0x0c 0x13 0x03 0x03 0x21 0x00
> > 					0x1000035 0x00
> > 					0x1000036 0xa0
> > 					0x100003a 0x55
> > 					0x1000044 0x00 0x01
> > 					0x10000d0 0x07 0x07 0x1d 0x03
> > 					0x10000d1 0x03 0x30 0x10
> > 					0x10000d2 0x03 0x14 0x04
> > 					0x1000029
> > 					0x100002c>;
> > 
> > Parsed here:
> > https://elixir.bootlin.com/linux/latest/C/ident/fbtft_init_display_from_property
> > 
> > Before fbdev was closed for new drivers I looked at fixing up fbtft for
> > proper inclusion and asked on the Device Tree mailinglist about the init
> > property and how to handle the controller configuration generically.
> > I was politely told that this should be done in the driver, so when I
> > made my first DRM driver I made a driver specifically for a panel
> > (mi0283qt.c).
> > 
> > Later I found another post that in clear words stated that setting
> > random registers from DT was not acceptable.
>
> Understood and thanks for the explanation.
> It is a shame that the users loose here :-(

From a user point-of-view, nothing prevents the overlays and the
firmware to be in the same package, provided by whatever distro or
build-system they would use.

The only case where it's a bit less efficient is for a panel that isn't
supported already, but it's just a documentation and tooling issue, and
Noralf has an awesome track record for both.

And the DT syntax throw so much people off that I'm not sure it's such a
benefit :)

Maxime

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