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@BY5PR11MB418... 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