| Message ID | 20250125125320.37285-4-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/bridge: Use per-client debugfs entry | expand |
On Sat, Jan 25, 2025 at 01:53:20PM +0100, Wolfram Sang wrote: > 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. > > Looking forward to comments. > > Wolfram Sang (2): > drm/bridge: it6505: Use per-client debugfs entry > drm/bridge: ti-sn65dsi86: Use per-client debugfs entry 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. Note, that callbacks gets connector's dentry as an argument, so bridges still should probably create a subdir for their own stuff.
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 - but they should - the new I2C client debugfs infrastructure is, thus, not needed here - 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? Or am I missing something? Happy hacking, Wolfram
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 > - but they should > - the new I2C client debugfs infrastructure is, thus, not needed here > - 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. > Or am I missing something? > > Happy hacking, > > Wolfram >
Hi, On Mon, Jan 27, 2025 at 7:34 AM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> 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
Hi Doug! > wanted it in panel-edp, but glad it's useful for other cases. ;-) :) > 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". The good news is that I learnt now that I can actually test the change myself on one of the Renesas boards. So, I can play around to find a good solution. Maybe even with a symlink somewhere. I have to try. Thanks for the feedback, Wolfram