diff mbox series

[v10,1/6] dt-bindings: display: Add support for Intel KeemBay Display

Message ID 1604006877-20092-2-git-send-email-anitha.chrisanthus@intel.com
State New, archived
Headers show
Series Add support for KeemBay DRM driver | expand

Commit Message

Chrisanthus, Anitha Oct. 29, 2020, 9:27 p.m. UTC
This patch adds bindings for Intel KeemBay Display

v2: review changes from Rob Herring
v3: review changes from Sam Ravnborg (removed mipi dsi entries, and
    encoder entry, connect port to dsi)
    MSSCAM is part of the display submodule and its used to reset LCD
    and MIPI DSI clocks, so its best to be on this device tree.

Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 .../bindings/display/intel,keembay-display.yaml    | 75 ++++++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml

Comments

Sam Ravnborg Oct. 29, 2020, 10:20 p.m. UTC | #1
Hi Anitha.

On Thu, Oct 29, 2020 at 02:27:52PM -0700, Anitha Chrisanthus wrote:
> This patch adds bindings for Intel KeemBay Display
> 
> v2: review changes from Rob Herring
> v3: review changes from Sam Ravnborg (removed mipi dsi entries, and
>     encoder entry, connect port to dsi)
>     MSSCAM is part of the display submodule and its used to reset LCD
>     and MIPI DSI clocks, so its best to be on this device tree.
> 
> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Daniel Vetter <daniel@ffwll.ch>

Looks good - and the split betwwen the display and the mipi<->dsi parts
matches the understanding of the HW I have developed.

Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

> ---
>  .../bindings/display/intel,keembay-display.yaml    | 75 ++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml
> new file mode 100644
> index 0000000..8a8effe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml
> @@ -0,0 +1,75 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Devicetree bindings for Intel Keem Bay display controller
> +
> +maintainers:
> +  - Anitha Chrisanthus <anitha.chrisanthus@intel.com>
> +  - Edmond J Dea <edmund.j.dea@intel.com>
> +
> +properties:
> +  compatible:
> +    const: intel,keembay-display
> +
> +  reg:
> +    items:
> +      - description: LCD registers range
> +      - description: Msscam registers range
> +
> +  reg-names:
> +    items:
> +      - const: lcd
> +      - const: msscam
> +
> +  clocks:
> +    items:
> +      - description: LCD controller clock
> +      - description: pll0 clock
> +
> +  clock-names:
> +    items:
> +      - const: clk_lcd
> +      - const: clk_pll0
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  port:
> +    type: object
> +    description: Display output node to DSI.
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - clocks
> +  - clock-names
> +  - interrupts
> +  - port
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    display@20930000 {
> +        compatible = "intel,keembay-display";
> +        reg = <0x20930000 0x3000>,
> +              <0x20910000 0x30>;
> +        reg-names = "lcd", "msscam";
> +        interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&scmi_clk 0x83>,
> +                 <&scmi_clk 0x0>;
> +        clock-names = "clk_lcd", "clk_pll0";
> +
> +        port {
> +            disp_out: endpoint {
> +                remote-endpoint = <&dsi_in>;
> +            };
> +        };
> +    };
> -- 
> 2.7.4
Neil Armstrong Oct. 30, 2020, 8:31 a.m. UTC | #2
Hi,

On 29/10/2020 23:20, Sam Ravnborg wrote:
> Hi Anitha.
> 
> On Thu, Oct 29, 2020 at 02:27:52PM -0700, Anitha Chrisanthus wrote:
>> This patch adds bindings for Intel KeemBay Display
>>
>> v2: review changes from Rob Herring
>> v3: review changes from Sam Ravnborg (removed mipi dsi entries, and
>>     encoder entry, connect port to dsi)
>>     MSSCAM is part of the display submodule and its used to reset LCD
>>     and MIPI DSI clocks, so its best to be on this device tree.
>>
>> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>
>> Cc: Sam Ravnborg <sam@ravnborg.org>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Daniel Vetter <daniel@ffwll.ch>
> 
> Looks good - and the split betwwen the display and the mipi<->dsi parts
> matches the understanding of the HW I have developed.
> 
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
>> ---
>>  .../bindings/display/intel,keembay-display.yaml    | 75 ++++++++++++++++++++++
>>  1 file changed, 75 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml
>> new file mode 100644
>> index 0000000..8a8effe
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml
>> @@ -0,0 +1,75 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Devicetree bindings for Intel Keem Bay display controller
>> +
>> +maintainers:
>> +  - Anitha Chrisanthus <anitha.chrisanthus@intel.com>
>> +  - Edmond J Dea <edmund.j.dea@intel.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: intel,keembay-display
>> +
>> +  reg:
>> +    items:
>> +      - description: LCD registers range
>> +      - description: Msscam registers range
>> +

Indeed the split is much better, but as you replied on http://lore.kernel.org/r/BY5PR11MB41827DE07436DD0454E24E6E8C0A0@BY5PR11MB4182.namprd11.prod.outlook.com
the msscam seems to be shared with the camera subsystem block, if this is the case it should be handled.

If it's a shared register block, it could be defined as a "syscon" used by both subsystems.

Neil


>> +  reg-names:
>> +    items:
>> +      - const: lcd
>> +      - const: msscam
>> +
>> +  clocks:
>> +    items:
>> +      - description: LCD controller clock
>> +      - description: pll0 clock
>> +
>> +  clock-names:
>> +    items:
>> +      - const: clk_lcd
>> +      - const: clk_pll0
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  port:
>> +    type: object
>> +    description: Display output node to DSI.
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - reg-names
>> +  - clocks
>> +  - clock-names
>> +  - interrupts
>> +  - port
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/irq.h>
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +    display@20930000 {
>> +        compatible = "intel,keembay-display";
>> +        reg = <0x20930000 0x3000>,
>> +              <0x20910000 0x30>;
>> +        reg-names = "lcd", "msscam";
>> +        interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
>> +        clocks = <&scmi_clk 0x83>,
>> +                 <&scmi_clk 0x0>;
>> +        clock-names = "clk_lcd", "clk_pll0";
>> +
>> +        port {
>> +            disp_out: endpoint {
>> +                remote-endpoint = <&dsi_in>;
>> +            };
>> +        };
>> +    };
>> -- 
>> 2.7.4
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Sam Ravnborg Oct. 30, 2020, 9:15 p.m. UTC | #3
Hi Neil.

On Fri, Oct 30, 2020 at 09:31:36AM +0100, Neil Armstrong wrote:
> Hi,
> 
> On 29/10/2020 23:20, Sam Ravnborg wrote:
> > Hi Anitha.
> > 
> > On Thu, Oct 29, 2020 at 02:27:52PM -0700, Anitha Chrisanthus wrote:
> >> This patch adds bindings for Intel KeemBay Display
> >>
> >> v2: review changes from Rob Herring
> >> v3: review changes from Sam Ravnborg (removed mipi dsi entries, and
> >>     encoder entry, connect port to dsi)
> >>     MSSCAM is part of the display submodule and its used to reset LCD
> >>     and MIPI DSI clocks, so its best to be on this device tree.
> >>
> >> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>
> >> Cc: Sam Ravnborg <sam@ravnborg.org>
> >> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> >> Cc: Daniel Vetter <daniel@ffwll.ch>
> > 
> > Looks good - and the split betwwen the display and the mipi<->dsi parts
> > matches the understanding of the HW I have developed.
> > 
> > Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> > 
> >> ---
> >>  .../bindings/display/intel,keembay-display.yaml    | 75 ++++++++++++++++++++++
> >>  1 file changed, 75 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml
> >> new file mode 100644
> >> index 0000000..8a8effe
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml
> >> @@ -0,0 +1,75 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Devicetree bindings for Intel Keem Bay display controller
> >> +
> >> +maintainers:
> >> +  - Anitha Chrisanthus <anitha.chrisanthus@intel.com>
> >> +  - Edmond J Dea <edmund.j.dea@intel.com>
> >> +
> >> +properties:
> >> +  compatible:
> >> +    const: intel,keembay-display
> >> +
> >> +  reg:
> >> +    items:
> >> +      - description: LCD registers range
> >> +      - description: Msscam registers range
> >> +
> 
> Indeed the split is much better, but as you replied on http://lore.kernel.org/r/BY5PR11MB41827DE07436DD0454E24E6E8C0A0@BY5PR11MB4182.namprd11.prod.outlook.com
> the msscam seems to be shared with the camera subsystem block, if this is the case it should be handled.
> 
> If it's a shared register block, it could be defined as a "syscon" used by both subsystems.

I think I got it now.

msscam is used to enable clocks both for the display driver and the
mipi-dsi part.

So it should be pulled in to a dedicated node - for example like this:

mssscam: msscam@20910000 {
	compatible = "intel,keembay-msscam", "syscon";
	reg = <0x20910000, 0x30>;
	reg-io-width = <4>;
};

And ofc we need a binding file for that.


And then we could have code like this in the display driver:
	regmap *msscam = syscon_regmap_lookup_by_compatible("intel,keembay-msscam");
	if (IS_ERR(msscam))
		tell-and goto-out;

	regmap_write(msscam, MSS_LCD_MIPI_CFG, 1);
	regmap_write(msscam, MSS_LOOPBACK_CFG, 0);

And then in the kmb_dsi part:
	regmap *msscam = syscon_regmap_lookup_by_compatible("intel,keembay-msscam");
	if (IS_ERR(msscam))
		tell-and goto-out;

	regmap_write(msscam, MSS_MIPI_CIF_CFG, 1);


Did I finally get it?

	Sam
Rob Herring Nov. 2, 2020, 3:16 p.m. UTC | #4
On Fri, Oct 30, 2020 at 4:15 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Neil.
>
> On Fri, Oct 30, 2020 at 09:31:36AM +0100, Neil Armstrong wrote:
> > Hi,
> >
> > On 29/10/2020 23:20, Sam Ravnborg wrote:
> > > Hi Anitha.
> > >
> > > On Thu, Oct 29, 2020 at 02:27:52PM -0700, Anitha Chrisanthus wrote:
> > >> This patch adds bindings for Intel KeemBay Display
> > >>
> > >> v2: review changes from Rob Herring
> > >> v3: review changes from Sam Ravnborg (removed mipi dsi entries, and
> > >>     encoder entry, connect port to dsi)
> > >>     MSSCAM is part of the display submodule and its used to reset LCD
> > >>     and MIPI DSI clocks, so its best to be on this device tree.
> > >>
> > >> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>
> > >> Cc: Sam Ravnborg <sam@ravnborg.org>
> > >> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > >> Cc: Daniel Vetter <daniel@ffwll.ch>
> > >
> > > Looks good - and the split betwwen the display and the mipi<->dsi parts
> > > matches the understanding of the HW I have developed.
> > >
> > > Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> > >
> > >> ---
> > >>  .../bindings/display/intel,keembay-display.yaml    | 75 ++++++++++++++++++++++
> > >>  1 file changed, 75 insertions(+)
> > >>  create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml
> > >> new file mode 100644
> > >> index 0000000..8a8effe
> > >> --- /dev/null
> > >> +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml
> > >> @@ -0,0 +1,75 @@
> > >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > >> +%YAML 1.2
> > >> +---
> > >> +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml#
> > >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > >> +
> > >> +title: Devicetree bindings for Intel Keem Bay display controller
> > >> +
> > >> +maintainers:
> > >> +  - Anitha Chrisanthus <anitha.chrisanthus@intel.com>
> > >> +  - Edmond J Dea <edmund.j.dea@intel.com>
> > >> +
> > >> +properties:
> > >> +  compatible:
> > >> +    const: intel,keembay-display
> > >> +
> > >> +  reg:
> > >> +    items:
> > >> +      - description: LCD registers range
> > >> +      - description: Msscam registers range
> > >> +
> >
> > Indeed the split is much better, but as you replied on http://lore.kernel.org/r/BY5PR11MB41827DE07436DD0454E24E6E8C0A0@BY5PR11MB4182.namprd11.prod.outlook.com
> > the msscam seems to be shared with the camera subsystem block, if this is the case it should be handled.
> >
> > If it's a shared register block, it could be defined as a "syscon" used by both subsystems.
>
> I think I got it now.
>
> msscam is used to enable clocks both for the display driver and the
> mipi-dsi part.

If just clocks, then the msscam should be a clock provider possibly.
If not, then the below looks right.

>
> So it should be pulled in to a dedicated node - for example like this:
>
> mssscam: msscam@20910000 {
>         compatible = "intel,keembay-msscam", "syscon";
>         reg = <0x20910000, 0x30>;
>         reg-io-width = <4>;
> };
>
> And ofc we need a binding file for that.
>
>
> And then we could have code like this in the display driver:
>         regmap *msscam = syscon_regmap_lookup_by_compatible("intel,keembay-msscam");
>         if (IS_ERR(msscam))
>                 tell-and goto-out;
>
>         regmap_write(msscam, MSS_LCD_MIPI_CFG, 1);
>         regmap_write(msscam, MSS_LOOPBACK_CFG, 0);
>
> And then in the kmb_dsi part:
>         regmap *msscam = syscon_regmap_lookup_by_compatible("intel,keembay-msscam");
>         if (IS_ERR(msscam))
>                 tell-and goto-out;
>
>         regmap_write(msscam, MSS_MIPI_CIF_CFG, 1);
>
>
> Did I finally get it?
>
>         Sam
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Neil Armstrong Nov. 2, 2020, 4:38 p.m. UTC | #5
On 02/11/2020 16:16, Rob Herring wrote:
> On Fri, Oct 30, 2020 at 4:15 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>>
>> Hi Neil.
>>
>> On Fri, Oct 30, 2020 at 09:31:36AM +0100, Neil Armstrong wrote:
>>> Hi,
>>>
>>> On 29/10/2020 23:20, Sam Ravnborg wrote:
>>>> Hi Anitha.
>>>>
>>>> On Thu, Oct 29, 2020 at 02:27:52PM -0700, Anitha Chrisanthus wrote:
>>>>> This patch adds bindings for Intel KeemBay Display
>>>>>
>>>>> v2: review changes from Rob Herring
>>>>> v3: review changes from Sam Ravnborg (removed mipi dsi entries, and
>>>>>     encoder entry, connect port to dsi)
>>>>>     MSSCAM is part of the display submodule and its used to reset LCD
>>>>>     and MIPI DSI clocks, so its best to be on this device tree.
>>>>>
>>>>> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>
>>>>> Cc: Sam Ravnborg <sam@ravnborg.org>
>>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>>
>>>> Looks good - and the split betwwen the display and the mipi<->dsi parts
>>>> matches the understanding of the HW I have developed.
>>>>
>>>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>>>>
>>>>> ---
>>>>>  .../bindings/display/intel,keembay-display.yaml    | 75 ++++++++++++++++++++++
>>>>>  1 file changed, 75 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml
>>>>> new file mode 100644
>>>>> index 0000000..8a8effe
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml
>>>>> @@ -0,0 +1,75 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Devicetree bindings for Intel Keem Bay display controller
>>>>> +
>>>>> +maintainers:
>>>>> +  - Anitha Chrisanthus <anitha.chrisanthus@intel.com>
>>>>> +  - Edmond J Dea <edmund.j.dea@intel.com>
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: intel,keembay-display
>>>>> +
>>>>> +  reg:
>>>>> +    items:
>>>>> +      - description: LCD registers range
>>>>> +      - description: Msscam registers range
>>>>> +
>>>
>>> Indeed the split is much better, but as you replied on http://lore.kernel.org/r/BY5PR11MB41827DE07436DD0454E24E6E8C0A0@BY5PR11MB4182.namprd11.prod.outlook.com
>>> the msscam seems to be shared with the camera subsystem block, if this is the case it should be handled.
>>>
>>> If it's a shared register block, it could be defined as a "syscon" used by both subsystems.
>>
>> I think I got it now.
>>
>> msscam is used to enable clocks both for the display driver and the
>> mipi-dsi part.
> 
> If just clocks, then the msscam should be a clock provider possibly.
> If not, then the below looks right.

I agree

> 
>>
>> So it should be pulled in to a dedicated node - for example like this:
>>
>> mssscam: msscam@20910000 {
>>         compatible = "intel,keembay-msscam", "syscon";
>>         reg = <0x20910000, 0x30>;
>>         reg-io-width = <4>;
>> };
>>
>> And ofc we need a binding file for that.
>>
>>
>> And then we could have code like this in the display driver:
>>         regmap *msscam = syscon_regmap_lookup_by_compatible("intel,keembay-msscam");
>>         if (IS_ERR(msscam))
>>                 tell-and goto-out;

It's better to use a phandle in the display node, instead of looking for compatible nodes.

>>
>>         regmap_write(msscam, MSS_LCD_MIPI_CFG, 1);
>>         regmap_write(msscam, MSS_LOOPBACK_CFG, 0);
>>
>> And then in the kmb_dsi part:
>>         regmap *msscam = syscon_regmap_lookup_by_compatible("intel,keembay-msscam");
>>         if (IS_ERR(msscam))
>>                 tell-and goto-out;
>>
>>         regmap_write(msscam, MSS_MIPI_CIF_CFG, 1);
>>
>>
>> Did I finally get it?

Yes, this is what I was thinking about, thanks

Neil

>>
>>         Sam
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Sam Ravnborg Nov. 2, 2020, 6:04 p.m. UTC | #6
Hi Neil.

> >>>>> ---
> >>>>>  .../bindings/display/intel,keembay-display.yaml    | 75 ++++++++++++++++++++++
> >>>>>  1 file changed, 75 insertions(+)
> >>>>>  create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml
> >>>>> new file mode 100644
> >>>>> index 0000000..8a8effe
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml
> >>>>> @@ -0,0 +1,75 @@
> >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>> +%YAML 1.2
> >>>>> +---
> >>>>> +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml#
> >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>> +
> >>>>> +title: Devicetree bindings for Intel Keem Bay display controller
> >>>>> +
> >>>>> +maintainers:
> >>>>> +  - Anitha Chrisanthus <anitha.chrisanthus@intel.com>
> >>>>> +  - Edmond J Dea <edmund.j.dea@intel.com>
> >>>>> +
> >>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    const: intel,keembay-display
> >>>>> +
> >>>>> +  reg:
> >>>>> +    items:
> >>>>> +      - description: LCD registers range
> >>>>> +      - description: Msscam registers range
> >>>>> +
> >>>
> >>> Indeed the split is much better, but as you replied on http://lore.kernel.org/r/BY5PR11MB41827DE07436DD0454E24E6E8C0A0@BY5PR11MB4182.namprd11.prod.outlook.com
> >>> the msscam seems to be shared with the camera subsystem block, if this is the case it should be handled.
> >>>
> >>> If it's a shared register block, it could be defined as a "syscon" used by both subsystems.
> >>
> >> I think I got it now.
> >>
> >> msscam is used to enable clocks both for the display driver and the
> >> mipi-dsi part.
> > 
> > If just clocks, then the msscam should be a clock provider possibly.
> > If not, then the below looks right.

I am feeling a little clueless here - sorry.

Can you help with any example that does this?

Everything I looked up in bindings/clock/ had a "#clock-cells" which is
not relevant for msscam - or so I think at least.


	Sam
Rob Herring Nov. 3, 2020, 2:02 a.m. UTC | #7
On Mon, Nov 2, 2020 at 10:38 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> On 02/11/2020 16:16, Rob Herring wrote:
> > On Fri, Oct 30, 2020 at 4:15 PM Sam Ravnborg <sam@ravnborg.org> wrote:
> >>
> >> Hi Neil.
> >>
> >> On Fri, Oct 30, 2020 at 09:31:36AM +0100, Neil Armstrong wrote:
> >>> Hi,
> >>>
> >>> On 29/10/2020 23:20, Sam Ravnborg wrote:
> >>>> Hi Anitha.
> >>>>
> >>>> On Thu, Oct 29, 2020 at 02:27:52PM -0700, Anitha Chrisanthus wrote:
> >>>>> This patch adds bindings for Intel KeemBay Display
> >>>>>
> >>>>> v2: review changes from Rob Herring
> >>>>> v3: review changes from Sam Ravnborg (removed mipi dsi entries, and
> >>>>>     encoder entry, connect port to dsi)
> >>>>>     MSSCAM is part of the display submodule and its used to reset LCD
> >>>>>     and MIPI DSI clocks, so its best to be on this device tree.
> >>>>>
> >>>>> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>
> >>>>> Cc: Sam Ravnborg <sam@ravnborg.org>
> >>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
> >>>>
> >>>> Looks good - and the split betwwen the display and the mipi<->dsi parts
> >>>> matches the understanding of the HW I have developed.
> >>>>
> >>>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> >>>>
> >>>>> ---
> >>>>>  .../bindings/display/intel,keembay-display.yaml    | 75 ++++++++++++++++++++++
> >>>>>  1 file changed, 75 insertions(+)
> >>>>>  create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml
> >>>>> new file mode 100644
> >>>>> index 0000000..8a8effe
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml
> >>>>> @@ -0,0 +1,75 @@
> >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>> +%YAML 1.2
> >>>>> +---
> >>>>> +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml#
> >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>> +
> >>>>> +title: Devicetree bindings for Intel Keem Bay display controller
> >>>>> +
> >>>>> +maintainers:
> >>>>> +  - Anitha Chrisanthus <anitha.chrisanthus@intel.com>
> >>>>> +  - Edmond J Dea <edmund.j.dea@intel.com>
> >>>>> +
> >>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    const: intel,keembay-display
> >>>>> +
> >>>>> +  reg:
> >>>>> +    items:
> >>>>> +      - description: LCD registers range
> >>>>> +      - description: Msscam registers range
> >>>>> +
> >>>
> >>> Indeed the split is much better, but as you replied on http://lore.kernel.org/r/BY5PR11MB41827DE07436DD0454E24E6E8C0A0@BY5PR11MB4182.namprd11.prod.outlook.com
> >>> the msscam seems to be shared with the camera subsystem block, if this is the case it should be handled.
> >>>
> >>> If it's a shared register block, it could be defined as a "syscon" used by both subsystems.
> >>
> >> I think I got it now.
> >>
> >> msscam is used to enable clocks both for the display driver and the
> >> mipi-dsi part.
> >
> > If just clocks, then the msscam should be a clock provider possibly.
> > If not, then the below looks right.
>
> I agree
>
> >
> >>
> >> So it should be pulled in to a dedicated node - for example like this:
> >>
> >> mssscam: msscam@20910000 {
> >>         compatible = "intel,keembay-msscam", "syscon";
> >>         reg = <0x20910000, 0x30>;
> >>         reg-io-width = <4>;
> >> };
> >>
> >> And ofc we need a binding file for that.
> >>
> >>
> >> And then we could have code like this in the display driver:
> >>         regmap *msscam = syscon_regmap_lookup_by_compatible("intel,keembay-msscam");
> >>         if (IS_ERR(msscam))
> >>                 tell-and goto-out;
>
> It's better to use a phandle in the display node, instead of looking for compatible nodes.

No, you don't need it in DT unless there's more than one instance or
other parameters needed like register offsets/masks. The above is
actually faster too walking a list rather than a phandle look up
(though the phandle cache now may make it faster).

Rob
Rob Herring Nov. 3, 2020, 2:05 a.m. UTC | #8
On Mon, Nov 2, 2020 at 12:04 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Neil.
>
> > >>>>> ---
> > >>>>>  .../bindings/display/intel,keembay-display.yaml    | 75 ++++++++++++++++++++++
> > >>>>>  1 file changed, 75 insertions(+)
> > >>>>>  create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml
> > >>>>>
> > >>>>> diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml
> > >>>>> new file mode 100644
> > >>>>> index 0000000..8a8effe
> > >>>>> --- /dev/null
> > >>>>> +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml
> > >>>>> @@ -0,0 +1,75 @@
> > >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > >>>>> +%YAML 1.2
> > >>>>> +---
> > >>>>> +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml#
> > >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > >>>>> +
> > >>>>> +title: Devicetree bindings for Intel Keem Bay display controller
> > >>>>> +
> > >>>>> +maintainers:
> > >>>>> +  - Anitha Chrisanthus <anitha.chrisanthus@intel.com>
> > >>>>> +  - Edmond J Dea <edmund.j.dea@intel.com>
> > >>>>> +
> > >>>>> +properties:
> > >>>>> +  compatible:
> > >>>>> +    const: intel,keembay-display
> > >>>>> +
> > >>>>> +  reg:
> > >>>>> +    items:
> > >>>>> +      - description: LCD registers range
> > >>>>> +      - description: Msscam registers range
> > >>>>> +
> > >>>
> > >>> Indeed the split is much better, but as you replied on http://lore.kernel.org/r/BY5PR11MB41827DE07436DD0454E24E6E8C0A0@BY5PR11MB4182.namprd11.prod.outlook.com
> > >>> the msscam seems to be shared with the camera subsystem block, if this is the case it should be handled.
> > >>>
> > >>> If it's a shared register block, it could be defined as a "syscon" used by both subsystems.
> > >>
> > >> I think I got it now.
> > >>
> > >> msscam is used to enable clocks both for the display driver and the
> > >> mipi-dsi part.
> > >
> > > If just clocks, then the msscam should be a clock provider possibly.
> > > If not, then the below looks right.
>
> I am feeling a little clueless here - sorry.
>
> Can you help with any example that does this?

I'm pretty sure there's some DSI PHYs where they are also a clock provider.

> Everything I looked up in bindings/clock/ had a "#clock-cells" which is
> not relevant for msscam - or so I think at least.

That is precisely what needs to be added to msscam and then there
would be another 'clocks' entry here.

But it really depends if the register accesses here map to the
controls the clock API provides.

Rob
Neil Armstrong Nov. 3, 2020, 9:36 a.m. UTC | #9
On 03/11/2020 03:02, Rob Herring wrote:
> On Mon, Nov 2, 2020 at 10:38 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
>>
>> On 02/11/2020 16:16, Rob Herring wrote:
>>> On Fri, Oct 30, 2020 at 4:15 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>>>>
>>>> Hi Neil.
>>>>
>>>> On Fri, Oct 30, 2020 at 09:31:36AM +0100, Neil Armstrong wrote:
>>>>> Hi,
>>>>>
>>>>> On 29/10/2020 23:20, Sam Ravnborg wrote:
>>>>>> Hi Anitha.
>>>>>>
>>>>>> On Thu, Oct 29, 2020 at 02:27:52PM -0700, Anitha Chrisanthus wrote:
>>>>>>> This patch adds bindings for Intel KeemBay Display
>>>>>>>
>>>>>>> v2: review changes from Rob Herring
>>>>>>> v3: review changes from Sam Ravnborg (removed mipi dsi entries, and
>>>>>>>     encoder entry, connect port to dsi)
>>>>>>>     MSSCAM is part of the display submodule and its used to reset LCD
>>>>>>>     and MIPI DSI clocks, so its best to be on this device tree.
>>>>>>>
>>>>>>> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com>
>>>>>>> Cc: Sam Ravnborg <sam@ravnborg.org>
>>>>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>>>>
>>>>>> Looks good - and the split betwwen the display and the mipi<->dsi parts
>>>>>> matches the understanding of the HW I have developed.
>>>>>>
>>>>>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>>>>>>
>>>>>>> ---
>>>>>>>  .../bindings/display/intel,keembay-display.yaml    | 75 ++++++++++++++++++++++
>>>>>>>  1 file changed, 75 insertions(+)
>>>>>>>  create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml
>>>>>>> new file mode 100644
>>>>>>> index 0000000..8a8effe
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml
>>>>>>> @@ -0,0 +1,75 @@
>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>> +%YAML 1.2
>>>>>>> +---
>>>>>>> +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml#
>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>> +
>>>>>>> +title: Devicetree bindings for Intel Keem Bay display controller
>>>>>>> +
>>>>>>> +maintainers:
>>>>>>> +  - Anitha Chrisanthus <anitha.chrisanthus@intel.com>
>>>>>>> +  - Edmond J Dea <edmund.j.dea@intel.com>
>>>>>>> +
>>>>>>> +properties:
>>>>>>> +  compatible:
>>>>>>> +    const: intel,keembay-display
>>>>>>> +
>>>>>>> +  reg:
>>>>>>> +    items:
>>>>>>> +      - description: LCD registers range
>>>>>>> +      - description: Msscam registers range
>>>>>>> +
>>>>>
>>>>> Indeed the split is much better, but as you replied on http://lore.kernel.org/r/BY5PR11MB41827DE07436DD0454E24E6E8C0A0@BY5PR11MB4182.namprd11.prod.outlook.com
>>>>> the msscam seems to be shared with the camera subsystem block, if this is the case it should be handled.
>>>>>
>>>>> If it's a shared register block, it could be defined as a "syscon" used by both subsystems.
>>>>
>>>> I think I got it now.
>>>>
>>>> msscam is used to enable clocks both for the display driver and the
>>>> mipi-dsi part.
>>>
>>> If just clocks, then the msscam should be a clock provider possibly.
>>> If not, then the below looks right.
>>
>> I agree
>>
>>>
>>>>
>>>> So it should be pulled in to a dedicated node - for example like this:
>>>>
>>>> mssscam: msscam@20910000 {
>>>>         compatible = "intel,keembay-msscam", "syscon";
>>>>         reg = <0x20910000, 0x30>;
>>>>         reg-io-width = <4>;
>>>> };
>>>>
>>>> And ofc we need a binding file for that.
>>>>
>>>>
>>>> And then we could have code like this in the display driver:
>>>>         regmap *msscam = syscon_regmap_lookup_by_compatible("intel,keembay-msscam");
>>>>         if (IS_ERR(msscam))
>>>>                 tell-and goto-out;
>>
>> It's better to use a phandle in the display node, instead of looking for compatible nodes.
> 
> No, you don't need it in DT unless there's more than one instance or
> other parameters needed like register offsets/masks. The above is
> actually faster too walking a list rather than a phandle look up
> (though the phandle cache now may make it faster).

A phandle makes the dependency explicit, anyway it's only an advice.

Neil

> 
> Rob
>
Neil Armstrong Nov. 3, 2020, 9:37 a.m. UTC | #10
On 02/11/2020 19:04, Sam Ravnborg wrote:
> Hi Neil.
> 
>>>>>>> ---
>>>>>>>  .../bindings/display/intel,keembay-display.yaml    | 75 ++++++++++++++++++++++
>>>>>>>  1 file changed, 75 insertions(+)
>>>>>>>  create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml
>>>>>>> new file mode 100644
>>>>>>> index 0000000..8a8effe
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml
>>>>>>> @@ -0,0 +1,75 @@
>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>> +%YAML 1.2
>>>>>>> +---
>>>>>>> +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml#
>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>> +
>>>>>>> +title: Devicetree bindings for Intel Keem Bay display controller
>>>>>>> +
>>>>>>> +maintainers:
>>>>>>> +  - Anitha Chrisanthus <anitha.chrisanthus@intel.com>
>>>>>>> +  - Edmond J Dea <edmund.j.dea@intel.com>
>>>>>>> +
>>>>>>> +properties:
>>>>>>> +  compatible:
>>>>>>> +    const: intel,keembay-display
>>>>>>> +
>>>>>>> +  reg:
>>>>>>> +    items:
>>>>>>> +      - description: LCD registers range
>>>>>>> +      - description: Msscam registers range
>>>>>>> +
>>>>>
>>>>> Indeed the split is much better, but as you replied on http://lore.kernel.org/r/BY5PR11MB41827DE07436DD0454E24E6E8C0A0@BY5PR11MB4182.namprd11.prod.outlook.com
>>>>> the msscam seems to be shared with the camera subsystem block, if this is the case it should be handled.
>>>>>
>>>>> If it's a shared register block, it could be defined as a "syscon" used by both subsystems.
>>>>
>>>> I think I got it now.
>>>>
>>>> msscam is used to enable clocks both for the display driver and the
>>>> mipi-dsi part.
>>>
>>> If just clocks, then the msscam should be a clock provider possibly.
>>> If not, then the below looks right.
> 
> I am feeling a little clueless here - sorry.
> 
> Can you help with any example that does this?
> 
> Everything I looked up in bindings/clock/ had a "#clock-cells" which is
> not relevant for msscam - or so I think at least.

Looking at the code make it clear it's not relevant to implement it as clock controller.

The syscon is enough.

Neil

> 
> 
> 	Sam
>
Sam Ravnborg Nov. 3, 2020, 5:11 p.m. UTC | #11
Hi Neil,

> >>>>
> >>>> msscam is used to enable clocks both for the display driver and the
> >>>> mipi-dsi part.
> >>>
> >>> If just clocks, then the msscam should be a clock provider possibly.
> >>> If not, then the below looks right.
> > 
> > I am feeling a little clueless here - sorry.
> > 
> > Can you help with any example that does this?
> > 
> > Everything I looked up in bindings/clock/ had a "#clock-cells" which is
> > not relevant for msscam - or so I think at least.
> 
> Looking at the code make it clear it's not relevant to implement it as clock controller.
> 
> The syscon is enough.

Thanks, I think Anitha is going to send a v11 today with the sysconf
change.

	Sam
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml
new file mode 100644
index 0000000..8a8effe
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml
@@ -0,0 +1,75 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Devicetree bindings for Intel Keem Bay display controller
+
+maintainers:
+  - Anitha Chrisanthus <anitha.chrisanthus@intel.com>
+  - Edmond J Dea <edmund.j.dea@intel.com>
+
+properties:
+  compatible:
+    const: intel,keembay-display
+
+  reg:
+    items:
+      - description: LCD registers range
+      - description: Msscam registers range
+
+  reg-names:
+    items:
+      - const: lcd
+      - const: msscam
+
+  clocks:
+    items:
+      - description: LCD controller clock
+      - description: pll0 clock
+
+  clock-names:
+    items:
+      - const: clk_lcd
+      - const: clk_pll0
+
+  interrupts:
+    maxItems: 1
+
+  port:
+    type: object
+    description: Display output node to DSI.
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - clocks
+  - clock-names
+  - interrupts
+  - port
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    display@20930000 {
+        compatible = "intel,keembay-display";
+        reg = <0x20930000 0x3000>,
+              <0x20910000 0x30>;
+        reg-names = "lcd", "msscam";
+        interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&scmi_clk 0x83>,
+                 <&scmi_clk 0x0>;
+        clock-names = "clk_lcd", "clk_pll0";
+
+        port {
+            disp_out: endpoint {
+                remote-endpoint = <&dsi_in>;
+            };
+        };
+    };