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

[Sam Ravnborg]

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.

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;

	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?

	Sam