Hi, On Mon, Jan 27, 2025 at 7:34 AM Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> wrote: > > On Mon, Jan 27, 2025 at 08:54:38AM +0100, Wolfram Sang wrote: > > Hi Dmitry, > > > > thanks for the review! > > > > > > The I2C core now offers a debugfs-directory per client. Use it and > > > > remove the custom handling in drm bridge drivers. I don't have the > > > > hardware, so I hope I can find people willing to test here. Build bots > > > > are happy. And for it6505, it even fixes a problem. See the patch > > > > description there. > > > > > > I'd say, it should be done in a slightly different way: bridges have the > > > debugfs_init() callback, which is used by drm_bridge_connector (and can > > > be used by other bridge-created connetors) in order to create per-bridge > > > debugfs data. Please consider using it to create per-bridge debugfs data. > > > > ACK. > > > > > Note, that callbacks gets connector's dentry as an argument, so bridges > > > still should probably create a subdir for their own stuff. > > > > I wonder if this is necessary (I just looked at the code and have no > > hardware to test this, sadly). It looks to me as: > > > > - DRM has already debugfs infrastructure, yet those drivers don't use it Yeah, I think ti-sn65dsi86's debugfs code is a bit older (2019) and predates the debugfs infrastructure in drm_bridge (2022). I only added the debugfs code to drm bridge in order to get it for panels because I wanted it in panel-edp, but glad it's useful for other cases. ;-) > > - but they should > > - the new I2C client debugfs infrastructure is, thus, not needed here I don't personally have a strong opinion between using the i2c client infra vs. the drm_bridge infra. Both seem better than how we're doing it right now on ti-sn65dsi86 and just putting the debugfs directory at the top level. ;-) If Dmitry wants it to use the drm_bridge infra that sounds good to me. > > - DRM provides a dentry to the callbacks which drivers can "just use" > > - all drivers I looked at just put files there and never clean up > > (because the subsystem does it) > > > > So, from that, I should switch to the debugfs_init() callback and just > > use the dentry provided? > > Yes, please. Create a per-bridge subdir under that dentry, but I think > that was the case anyway. FWIW, when I look for the debugfs file created for panel-edp.c on my system, I see: /sys/kernel/debug/dri/ae01000.display-controller/eDP-1/panel/detected_panel The "panel" directory gets created in "drivers/gpu/drm/bridge/panel.c", so if a bridge created a file straight in the dentry passed it would have gone straight to "/sys/kernel/debug/dri/ae01000.display-controller/eDP-1". ...so you'd want some type of directory under there. In ti-sn65dsi86's case you could presumably keep the existing behavior where it creates a directory under there called "1-002c". -Doug
