Quoting Maxime Ripard (2021-12-15 16:15:08)
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=ZbaBbqBQ4EqpU2... > > 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...
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
This is what I have today in the mt8195.dtsi I'm using for testing purpose. Provided by mediatek on their vendor branch, not sure if it has been submitted to the list yet:
edp_tx: edp_tx@1c500000 { status = "disabled"; compatible = "mediatek,mt8195-dp-tx"; reg = <0 0x1c500000 0 0x8000>; nvmem-cells = <&dp_calibration>; nvmem-cell-names = "dp_calibration_data"; power-domains = <&spm MT8195_POWER_DOMAIN_EPD_TX>; interrupts = <GIC_SPI 676 IRQ_TYPE_LEVEL_HIGH 0>; };
dp_tx: dp_tx@1c600000 { compatible = "mediatek,mt8195-dp-tx"; reg = <0 0x1c600000 0 0x8000>; nvmem-cells = <&dp_calibration>; nvmem-cell-names = "dp_calibration_data"; power-domains = <&spm MT8195_POWER_DOMAIN_DP_TX>; interrupts = <GIC_SPI 458 IRQ_TYPE_LEVEL_HIGH 0>; status = "disabled"; };
With no device tree node for the dp-phy. The phy range is included in the reg range of the dp-tx device.