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

[Neil Armstrong]

On 03/11/2020 03:02, Rob Herring wrote:
> On Mon, Nov 2, 2020 at 10:38 AM Neil Armstrong <narmstrong at baylibre.com> wrote:
>>
>> 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.
> 
> 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
>