On Wed, Dec 15, 2021 at 10:02:22AM -0600, Guillaume Ranquet wrote: > 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=ZbaBbqBQ4EqpU2P0TF90gAWQeRsg@xxxxxxxxxxxxxx/ > > > > > > > > > > > > > > > > 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@xxxxxxxxxxxxxx/#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 > > 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. It's all good then, sorry for the noise Maxime
Attachment:
signature.asc
Description: PGP signature
