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

[Rob Herring]

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).

Rob