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

[Neil Armstrong]

On 02/11/2020 16:16, Rob Herring wrote:
> On Fri, Oct 30, 2020 at 4:15 PM Sam Ravnborg <sam at 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 at intel.com>
>>>>> Cc: Sam Ravnborg <sam at ravnborg.org>
>>>>> Cc: Thomas Zimmermann <tzimmermann at suse.de>
>>>>> Cc: Daniel Vetter <daniel at 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 at 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 at intel.com>
>>>>> +  - Edmond J Dea <edmund.j.dea at 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 at 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel