Date:   Wed, 15 Dec 2021 16:15:08 +0100
From:   Maxime Ripard <maxime@...no.tech>
To:     Guillaume Ranquet <granquet@...libre.com>
Cc:     Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Chun-Kuang Hu <chunkuang.hu@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Markus Schneider-Pargmann <msp@...libre.com>,
        kernel test robot <lkp@...el.com>,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linux-mediatek@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v6 7/7] drm/mediatek: Add mt8195 DisplayPort driver

Hi,

On Wed, Dec 15, 2021 at 09:03:01AM -0600, Guillaume Ranquet wrote:
> Quoting Maxime Ripard (2021-12-13 17:54:22)
> > On Thu, Dec 02, 2021 at 06:48:12AM -0800, Guillaume Ranquet wrote:
> > > Hi,
> > >
> > > Quoting Maxime Ripard (2021-11-25 15:30:34)
> > > > On Wed, Nov 24, 2021 at 01:45:21PM +0000, Guillaume Ranquet wrote:
> > > > > Hi,
> > > > > Thanks for all your input, really appreciated.
> > > > >
> > > > > Quoting Maxime Ripard (2021-11-16 15:51:12)
> > > > > > Hi,
> > > > > >
> > > > > > On Mon, Nov 15, 2021 at 09:33:52AM -0500, Guillaume Ranquet wrote:
> > > > > > > Quoting Maxime Ripard (2021-11-15 11:11:29)
> > > > > > > > > The driver creates a child device for the phy. The child device will
> > > > > > > > > never exist without the parent being active. As they are sharing a
> > > > > > > > > register range, the parent passes a regmap pointer to the child so that
> > > > > > > > > both can work with the same register range. The phy driver sets device
> > > > > > > > > data that is read by the parent to get the phy device that can be used
> > > > > > > > > to control the phy properties.
> > > > > > > >
> > > > > > > > If the PHY is in the same register space than the DP controller, why do
> > > > > > > > you need a separate PHY driver in the first place?
> > > > > > >
> > > > > > > This has been asked by Chun-Kuang Hu in a previous revision of the series:
> > > > > > >
> > > > > > > https://lore.kernel.org/linux-mediatek/CAAOTY_-+T-wRCH2yw2XSm=ZbaBbqBQ4EqpU2P0TF90gAWQeRsg@mail.gmail.com/
> > > > > >
> > > > > > It's a bit of a circular argument though :)
> > > > > >
> > > > > > It's a separate phy driver because it needs to go through another
> > > > > > maintainer's tree, but it needs to go through another maintainer's tree
> > > > > > because it's a separate phy driver.
> > > > > >
> > > > > > It doesn't explain why it needs to be a separate phy driver? Why can't
> > > > > > the phy setup be done directly in the DP driver, if it's essentially a
> > > > > > single device?
> > > > > >
> > > > > > That being said, usually what those kind of questions mean is that
> > > > > > you're missing a comment or something in the commit log to provide that
> > > > > > context in the first place, so it would be great to add that context
> > > > > > here.
> > > > > >
> > > > > > And it will avoid the situation we're now in where multiple reviewers
> > > > > > ask the same questions over and over again :)
> > > > > >
> > > > > At first I didn't understand your reply, then I realized I gave you
> > > > > the wrong link...
> > > > > my bad! I'm struggling a bit with mail reviews, but I'll get there eventually.
> > > > >
> > > > > The driver and phy were a single driver until v2 of this patch series
> > > > > and the phy setup
> > > > > was done directly in the driver (single driver, single C file).
> > > > > Here's the relevant link to the discussion between Chun-Kuang and Markus
> > > > >
> > > > > https://lore.kernel.org/linux-mediatek/CAAOTY__cJMqcAieEraJ2sz4gi0Zs-aiNXz38_x7dPQea6HvYEg@mail.gmail.com/#t
> > > > >
> > > > > I'll try to find a way to make it clearer for v7.
> > > >
> > > > OK, it makes sense then :)
> > > >
> > > > There's something weird though: the devices definitely look like they're
> > > > in a separate register range, yet you mention a regmap to handle the
> > > > shared register range. That range doesn't seem described anywhere in the
> > > > device tree though? What is it for?
> > >
> > > My understanding is that 0x1000 to 0x1fff controls the phy
> > > functionalities and 0x2000 to 0x4fff controls "non-phy"
> > > functionalities. And you are right, there's no description of that in
> > > the device tree whatsoever. The ranges are in the same actual device
> > > and thus it has been decided to not have dt-bindings for the phy
> > > device.
> >
> > Sure, that last part makes sense, but then I'm not sure why you don't
> > have the full register range in the device node you have in the DT?
> >
> > > The phy driver is a child of the DP driver that we register using
> > > platform_device_register_data() and we pass along the same regmap as
> > > the DP driver in its platform data.
> >
> > Especially if it's used by something, it should be described in the DT
> > somewhere.
> >
> > Maxime
> 
> 
> So, to make things crystal clear to a newbie (like me).
> Would you describe it like this:
> compatible = "mediatek,mt8195-dp-tx";
> reg = <0 0x1c501000 0 0x0fff>,
> 	<0 0x1c502000 0 0x2fff>;
> 
> instead of the current description:
> compatible = "mediatek,mt8195-dp-tx";
> reg = <0 0x1c500000 0 0x8000>;
> 
> I haven't checked what the rest of the 0x8000 range is used for though...

I'm confused, is that what you had before?

I recall you had a DTSI somewhere where you have two devices, and the
dp-tx device not having the phy range?

If the latter is what you have, and there's no overlapping ranges over
multiple nodes, then it's fine already.

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists