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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Nov 2, 2020 at 10:38 AM Neil Armstrong <narmstrong@xxxxxxxxxxxx> wrote:
>
> On 02/11/2020 16:16, Rob Herring wrote:
> > On Fri, Oct 30, 2020 at 4:15 PM Sam Ravnborg <sam@xxxxxxxxxxxx> 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@xxxxxxxxx>
> >>>>> Cc: Sam Ravnborg <sam@xxxxxxxxxxxx>
> >>>>> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> >>>>> Cc: Daniel Vetter <daniel@xxxxxxxx>
> >>>>
> >>>> 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@xxxxxxxxxxxx>
> >>>>
> >>>>> ---
> >>>>>  .../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@xxxxxxxxx>
> >>>>> +  - Edmond J Dea <edmund.j.dea@xxxxxxxxx>
> >>>>> +
> >>>>> +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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> >>> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux