On Tue, Aug 22, 2023 at 05:17:37PM +0300, Laurent Pinchart wrote: > Hi Dmitry, > > Thank you for the patches. > > On Thu, Aug 17, 2023 at 05:55:13PM +0300, Dmitry Baryshkov wrote: > > Supporting DP/USB-C can result in a chain of several transparent > > bridges (PHY, redrivers, mux, etc). This results in drivers having > > similar boilerplate code for such bridges. > > What do you mean by transparent bridge here ? Bridges are a DRM concept, > and as far as I can tell, a PHY isn't a bridge. Why does it need to be > handled as one, especially if it's completely transparent ? > > > Next, these drivers are susceptible to -EPROBE_DEFER loops: the next > > bridge can either be probed from the bridge->attach callback, when it is > > too late to return -EPROBE_DEFER, or from the probe() callback, when the > > next bridge might not yet be available, because it depends on the > > resources provided by the probing device. > > Can't device links help avoiding defer probing in those cases ? > > > Last, but not least, this results in the the internal knowledge of DRM > > subsystem slowly diffusing into other subsystems, like PHY or USB/TYPEC. > > Why so ? The PHY subsystem should provide a PHY, without considering > what subsystem it will be used by. This patch series seems to me to > actually create this DRM dependency in other subsystems, I was wrong on this one, there are indeed existing drm_bridge instances in drivers/usb/ and drivers/phy/. That's certainly not nice. Why do we even need drm_bridge there, why can't the PHYs be acquired by their consumers in DRM (and anywhere else) using the PHY API ? > which I don't > think is a very good idea. Resources should be registered in their own > subsystem with the appropriate API, not in a way that is tied to a > particular consumer. > > > To solve all these issues, define a separate DRM helper, which creates > > separate aux device just for the bridge. During probe such aux device > > doesn't result in the EPROBE_DEFER loops. Instead it allows the device > > drivers to probe properly, according to the actual resource > > dependencies. The bridge auxdevs are then probed when the next bridge > > becomes available, sparing drivers from drm_bridge_attach() returning > > -EPROBE_DEFER. > > I'm not thrilled :-( Let's discuss the questions above first. > > > Proposed merge strategy: immutable branch with the drm commit, which is > > then merged into PHY and USB subsystems together with the corresponding > > patch. > > > > Changes since v3: > > - Moved bridge driver to gpu/drm/bridge (Neil Armstrong) > > - Renamed it to aux-bridge (since there is already a simple_bridge driver) > > - Made CONFIG_OF mandatory for this driver (Neil Armstrong) > > - Added missing kfree and ida_free (Dan Carpenter) > > > > Changes since v2: > > - ifdef'ed bridge->of_node access (LKP) > > > > Changes since v1: > > - Added EXPORT_SYMBOL_GPL / MODULE_LICENSE / etc. to drm_simple_bridge > > > > Dmitry Baryshkov (3): > > drm/bridge: add transparent bridge helper > > phy: qcom: qmp-combo: switch to DRM_AUX_BRIDGE > > usb: typec: nb7vpq904m: switch to DRM_AUX_BRIDGE > > > > drivers/gpu/drm/bridge/Kconfig | 9 ++ > > drivers/gpu/drm/bridge/Makefile | 1 + > > drivers/gpu/drm/bridge/aux-bridge.c | 132 ++++++++++++++++++++++ > > drivers/phy/qualcomm/Kconfig | 2 +- > > drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 44 +------- > > drivers/usb/typec/mux/Kconfig | 2 +- > > drivers/usb/typec/mux/nb7vpq904m.c | 44 +------- > > include/drm/bridge/aux-bridge.h | 19 ++++ > > 8 files changed, 167 insertions(+), 86 deletions(-) > > create mode 100644 drivers/gpu/drm/bridge/aux-bridge.c > > create mode 100644 include/drm/bridge/aux-bridge.h -- Regards, Laurent Pinchart
