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

["Noralf Trønnes" <noralf@tronnes.org>]

Den 09.02.2022 10.04, skrev Maxime Ripard:
> Hi Noralf,
> 
> On Tue, Feb 08, 2022 at 01:16:44PM +0100, Noralf Trønnes wrote:
>> Den 08.02.2022 00.20, skrev Rob Herring:
>>> On Thu, Jan 27, 2022 at 10:37:22AM +0100, Maxime Ripard wrote:
>>>> Hi,
>>>>
>>>> On Tue, Jan 25, 2022 at 06:56:58PM +0100, Noralf Trønnes wrote:
>>>>> Add binding for MIPI DBI compatible SPI panels.
>>>>>
>>>>> v2:
>>>>> - Fix path for panel-common.yaml
>>>>> - Use unevaluatedProperties
>>>>> - Drop properties which are in the allOf section
>>>>> - Drop model property (Rob)
>>>>>
>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>> ---
>>>>>  .../display/panel/panel-mipi-dbi-spi.yaml     | 59 +++++++++++++++++++
>>>>>  1 file changed, 59 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..b7cbeea0f8aa
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml
>>>>> @@ -0,0 +1,59 @@
>>>>> +# SPDX-License-Identifier: GPL-2.0-only
>>>>> +%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 Panels Device Tree Bindings
>>>>> +
>>>>> +maintainers:
>>>>> +  - Noralf Trønnes <noralf@tronnes.org>
>>>>> +
>>>>> +description:
>>>>> +  This binding is for display panels using a MIPI DBI controller
>>>>> +  in SPI mode.
>>>>> +
>>>>> +allOf:
>>>>> +  - $ref: panel-common.yaml#
>>>>> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: panel-mipi-dbi-spi
>>>>
>>>> You need contains here, otherwise it will error out if you have two compatibles.
>>>
>>> Shouldn't it always have 2?
>>>
>>> Either way, this has to be split up between a common, shareable schema 
>>> and specific, complete schema(s). Like this:
>>>
>>> - A schema for everything common (that allows additional properties)
>>>
>>> - A schema for 'panel-mipi-dbi-spi' referencing the common schema plus 
>>>   'unevaluatedProperties: false'
>>>
>>> - Schemas for panels with their own additional properties (regulators, 
>>>   GPIOs, etc.)
>>>
>>> LVDS was restructured like this IIRC.
>>
>> The whole point of this exercise is to avoid the need for controller
>> specific bindings.
> 
> I'm not sure to follow you here, nothing that either Rob or I discussed
> would require a controller specific binding.
> 
> It would require a controller compatible, but the binding itself can
> just mandate a controller compatible in addition to the
> panel-mipi-dbi-spi compatible, without enforcing anything wrt the
> compatible itself.
> 
> And the driver will just match panel-mipi-dbi-spi so there won't be any
> driver change either?
> 
> In essence, it would be similar to the bindings of panel-lvds or the
> AT24 EEPROM binding: you have two compatibles, but the driver is generic
> and will just infer its behaviour based on the DT properties (and in our
> case will load a firmware based on the specific compatible).
> 
> Wouldn't that work?
> 

Oh, I misunderstood then.
I looked at the panel-lvds binding and since it didn't have any example
it looked like a schema that controllers/panels would include and not
something that would be used on its own.

>> This binding will cover all specifics about these
>> controllers except for one thing and that is the controller
>> configuration. Each controller has its own configuration commands. These
>> commands will be loaded as a firmware file based on the compatible and
>> applied by the driver.
>>
>> So this binding, the panel-common and spi-peripheral-props covers
>> everything except for the controller configuration.
>>
>> Here's a copy of the DBI spec: https://www.docin.com/p-219732497.html
>>
>> This is my current version of the binding:
>>
>> # 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:
>>     contains:
>>       const: panel-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-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>;
>>                     };
>>             };
>>     };
>>
>> ...
> 
> Yep, this looks good to me
> 

Thanks, I'll spin a new version.

Noralf.