On Thu, Dec 02, 2021 at 06:48:12AM -0800, Guillaume Ranquet wrote:

Hi,

Quoting Maxime Ripard (2021-11-25 15:30:34)

quoted

On Wed, Nov 24, 2021 at 01:45:21PM +0000, Guillaume Ranquet wrote:

quoted

Hi,
Thanks for all your input, really appreciated.

Quoting Maxime Ripard (2021-11-16 15:51:12)

quoted

Hi,

On Mon, Nov 15, 2021 at 09:33:52AM -0500, Guillaume Ranquet wrote:

quoted

Quoting Maxime Ripard (2021-11-15 11:11:29)

quoted

quoted

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/ (local)

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 (local)

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