Hello Jani, On Tue, 25 Feb 2025 18:36:41 +0200 Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote: > On Tue, 25 Feb 2025, Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx> wrote: > > In preparation to expose more info about bridges in debugfs, which will > > require more insight into drm_bridge data structures, move the bridges_show > > code to drm_bridge.c. > > > > Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx> > > Signed-off-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx> > > I hate myself for doing this on a patch that's at v7... but here goes. Please don't! :-) This patch is new in v7, and a different (and definitely worse) approach was present in v6, but there was nothing before. > Perhaps consider moving the bridges debugfs creation and fops to > drm_bridge.c instead of just adding > drm_bridge_debugfs_show_encoder_bridges(). > > For example, add drm_bridge_debugfs_add(struct drm_encoder *encoder), > which then contains the debugfs_create_file() call. I think it should go in drm_encoder.c, not drm_bridge.c, right? Here we are showing the bridges attached to an encoder, so the entry point is each encoder. On the other hand in patch 2 we should move the drm_debugfs_global_add() code to drm_bridge.c, as it's showing bridges ina encoder-independent way. And finally drm_bridge should export the common drm_bridge_debugfs_show_bridge() function to drm_encoder.c. Do you think this is correct? > Interestingly, this lets you drop a lot of #ifdef CONFIG_DEBUG_FS. The > compiler optimizes the fops struct and the functions away when > debugfs_create_file() becomes a stub for CONFIG_DEBUG_FS=n. It becomes > all around cleaner. This surely makes the idea interesting. Cleaner code is always welcome. Luca -- Luca Ceresoli, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
