| Message ID | 20250315201946.7428-2-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | [RFT,v2] drm/bridge: ite-it6505: make use of debugfs_init callback | expand |
On Sat, Mar 15, 2025 at 09:17:16PM +0100, Wolfram Sang wrote: > Do not create a custom directory in debugfs-root, but use the > debugfs_init callback to create a custom directory at the given place > for the bridge. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > > Only build tested, but following the same pattern as the tested > ti-sn65dsi86. > > Changes since v1: > * switch from 'client->debugfs' to DRM 'debugfs_init' callback > > drivers/gpu/drm/bridge/ite-it6505.c | 59 ++++++++++------------------- > 1 file changed, 19 insertions(+), 40 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c > index 88ef76a37fe6..803bfe9954e6 100644 > --- a/drivers/gpu/drm/bridge/ite-it6505.c > +++ b/drivers/gpu/drm/bridge/ite-it6505.c > @@ -302,7 +302,6 @@ > #define WORD_LENGTH_18BIT 1 > #define WORD_LENGTH_20BIT 2 > #define WORD_LENGTH_24BIT 3 > -#define DEBUGFS_DIR_NAME "it6505-debugfs" > #define READ_BUFFER_SIZE 400 > > /* Vendor option */ > @@ -478,7 +477,6 @@ struct it6505 { > struct device *codec_dev; > struct delayed_work delayed_audio; > struct it6505_audio_data audio; > - struct dentry *debugfs; > > /* it6505 driver hold option */ > bool enable_drv_hold; > @@ -3302,21 +3300,6 @@ static const struct drm_edid *it6505_bridge_edid_read(struct drm_bridge *bridge, > return drm_edid_dup(it6505->cached_edid); > } > > -static const struct drm_bridge_funcs it6505_bridge_funcs = { > - .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > - .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > - .atomic_reset = drm_atomic_helper_bridge_reset, > - .attach = it6505_bridge_attach, > - .detach = it6505_bridge_detach, > - .mode_valid = it6505_bridge_mode_valid, > - .atomic_enable = it6505_bridge_atomic_enable, > - .atomic_disable = it6505_bridge_atomic_disable, > - .atomic_pre_enable = it6505_bridge_atomic_pre_enable, > - .atomic_post_disable = it6505_bridge_atomic_post_disable, > - .detect = it6505_bridge_detect, > - .edid_read = it6505_bridge_edid_read, > -}; > - > static __maybe_unused int it6505_bridge_resume(struct device *dev) > { > struct it6505 *it6505 = dev_get_drvdata(dev); > @@ -3574,36 +3557,34 @@ static const struct debugfs_entries debugfs_entry[] = { > { NULL, NULL }, > }; > > -static void debugfs_create_files(struct it6505 *it6505) > +static void it6505_debugfs_init(struct drm_bridge *bridge, struct dentry *root) > { > + struct it6505 *it6505 = bridge_to_it6505(bridge); > + struct dentry *debugfs = debugfs_create_dir(dev_name(it6505->dev), root); > int i = 0; > > while (debugfs_entry[i].name && debugfs_entry[i].fops) { > debugfs_create_file(debugfs_entry[i].name, 0644, > - it6505->debugfs, it6505, > - debugfs_entry[i].fops); > + debugfs, it6505, debugfs_entry[i].fops); > i++; > } > } Ni: I'd say, it's better to move this function rather than moving it6505_bridge_funcs. LGTM otherwise. > > -static void debugfs_init(struct it6505 *it6505) > -{ > - struct device *dev = it6505->dev; > - > - it6505->debugfs = debugfs_create_dir(DEBUGFS_DIR_NAME, NULL); > - > - if (IS_ERR(it6505->debugfs)) { > - dev_err(dev, "failed to create debugfs root"); > - return; > - } > - > - debugfs_create_files(it6505); > -} > - > -static void it6505_debugfs_remove(struct it6505 *it6505) > -{ > - debugfs_remove_recursive(it6505->debugfs); > -} > +static const struct drm_bridge_funcs it6505_bridge_funcs = { > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, > + .atomic_reset = drm_atomic_helper_bridge_reset, > + .attach = it6505_bridge_attach, > + .detach = it6505_bridge_detach, > + .mode_valid = it6505_bridge_mode_valid, > + .atomic_enable = it6505_bridge_atomic_enable, > + .atomic_disable = it6505_bridge_atomic_disable, > + .atomic_pre_enable = it6505_bridge_atomic_pre_enable, > + .atomic_post_disable = it6505_bridge_atomic_post_disable, > + .detect = it6505_bridge_detect, > + .edid_read = it6505_bridge_edid_read, > + .debugfs_init = it6505_debugfs_init, > +}; > > static void it6505_shutdown(struct i2c_client *client) > { > @@ -3689,7 +3670,6 @@ static int it6505_i2c_probe(struct i2c_client *client) > it6505_poweron(it6505); > > DRM_DEV_DEBUG_DRIVER(dev, "it6505 device name: %s", dev_name(dev)); > - debugfs_init(it6505); > pm_runtime_enable(dev); > > it6505->aux.name = "DP-AUX"; > @@ -3712,7 +3692,6 @@ static void it6505_i2c_remove(struct i2c_client *client) > > drm_bridge_remove(&it6505->bridge); > drm_dp_aux_unregister(&it6505->aux); > - it6505_debugfs_remove(it6505); > it6505_poweroff(it6505); > it6505_remove_edid(it6505); > } > -- > 2.47.2 >
> > + struct dentry *debugfs = debugfs_create_dir(dev_name(it6505->dev), root); I will switch away from dev_name() here, too, of course. > > int i = 0; > > > > while (debugfs_entry[i].name && debugfs_entry[i].fops) { > > debugfs_create_file(debugfs_entry[i].name, 0644, > > - it6505->debugfs, it6505, > > - debugfs_entry[i].fops); > > + debugfs, it6505, debugfs_entry[i].fops); > > i++; > > } > > } > > Ni: I'd say, it's better to move this function rather than moving > it6505_bridge_funcs. LGTM otherwise. I started like this but it meant to move all the sysfs file ops code as well. That was quite a chunk. You still prefer to do it?
On Sun, Mar 16, 2025 at 07:26:07AM +0100, Wolfram Sang wrote: > > > > + struct dentry *debugfs = debugfs_create_dir(dev_name(it6505->dev), root); > > I will switch away from dev_name() here, too, of course. > > > > int i = 0; > > > > > > while (debugfs_entry[i].name && debugfs_entry[i].fops) { > > > debugfs_create_file(debugfs_entry[i].name, 0644, > > > - it6505->debugfs, it6505, > > > - debugfs_entry[i].fops); > > > + debugfs, it6505, debugfs_entry[i].fops); > > > i++; > > > } > > > } > > > > Ni: I'd say, it's better to move this function rather than moving > > it6505_bridge_funcs. LGTM otherwise. > > I started like this but it meant to move all the sysfs file ops code as > well. That was quite a chunk. You still prefer to do it? Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c index 88ef76a37fe6..803bfe9954e6 100644 --- a/drivers/gpu/drm/bridge/ite-it6505.c +++ b/drivers/gpu/drm/bridge/ite-it6505.c @@ -302,7 +302,6 @@ #define WORD_LENGTH_18BIT 1 #define WORD_LENGTH_20BIT 2 #define WORD_LENGTH_24BIT 3 -#define DEBUGFS_DIR_NAME "it6505-debugfs" #define READ_BUFFER_SIZE 400 /* Vendor option */ @@ -478,7 +477,6 @@ struct it6505 { struct device *codec_dev; struct delayed_work delayed_audio; struct it6505_audio_data audio; - struct dentry *debugfs; /* it6505 driver hold option */ bool enable_drv_hold; @@ -3302,21 +3300,6 @@ static const struct drm_edid *it6505_bridge_edid_read(struct drm_bridge *bridge, return drm_edid_dup(it6505->cached_edid); } -static const struct drm_bridge_funcs it6505_bridge_funcs = { - .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, - .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, - .atomic_reset = drm_atomic_helper_bridge_reset, - .attach = it6505_bridge_attach, - .detach = it6505_bridge_detach, - .mode_valid = it6505_bridge_mode_valid, - .atomic_enable = it6505_bridge_atomic_enable, - .atomic_disable = it6505_bridge_atomic_disable, - .atomic_pre_enable = it6505_bridge_atomic_pre_enable, - .atomic_post_disable = it6505_bridge_atomic_post_disable, - .detect = it6505_bridge_detect, - .edid_read = it6505_bridge_edid_read, -}; - static __maybe_unused int it6505_bridge_resume(struct device *dev) { struct it6505 *it6505 = dev_get_drvdata(dev); @@ -3574,36 +3557,34 @@ static const struct debugfs_entries debugfs_entry[] = { { NULL, NULL }, }; -static void debugfs_create_files(struct it6505 *it6505) +static void it6505_debugfs_init(struct drm_bridge *bridge, struct dentry *root) { + struct it6505 *it6505 = bridge_to_it6505(bridge); + struct dentry *debugfs = debugfs_create_dir(dev_name(it6505->dev), root); int i = 0; while (debugfs_entry[i].name && debugfs_entry[i].fops) { debugfs_create_file(debugfs_entry[i].name, 0644, - it6505->debugfs, it6505, - debugfs_entry[i].fops); + debugfs, it6505, debugfs_entry[i].fops); i++; } } -static void debugfs_init(struct it6505 *it6505) -{ - struct device *dev = it6505->dev; - - it6505->debugfs = debugfs_create_dir(DEBUGFS_DIR_NAME, NULL); - - if (IS_ERR(it6505->debugfs)) { - dev_err(dev, "failed to create debugfs root"); - return; - } - - debugfs_create_files(it6505); -} - -static void it6505_debugfs_remove(struct it6505 *it6505) -{ - debugfs_remove_recursive(it6505->debugfs); -} +static const struct drm_bridge_funcs it6505_bridge_funcs = { + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, + .atomic_reset = drm_atomic_helper_bridge_reset, + .attach = it6505_bridge_attach, + .detach = it6505_bridge_detach, + .mode_valid = it6505_bridge_mode_valid, + .atomic_enable = it6505_bridge_atomic_enable, + .atomic_disable = it6505_bridge_atomic_disable, + .atomic_pre_enable = it6505_bridge_atomic_pre_enable, + .atomic_post_disable = it6505_bridge_atomic_post_disable, + .detect = it6505_bridge_detect, + .edid_read = it6505_bridge_edid_read, + .debugfs_init = it6505_debugfs_init, +}; static void it6505_shutdown(struct i2c_client *client) { @@ -3689,7 +3670,6 @@ static int it6505_i2c_probe(struct i2c_client *client) it6505_poweron(it6505); DRM_DEV_DEBUG_DRIVER(dev, "it6505 device name: %s", dev_name(dev)); - debugfs_init(it6505); pm_runtime_enable(dev); it6505->aux.name = "DP-AUX"; @@ -3712,7 +3692,6 @@ static void it6505_i2c_remove(struct i2c_client *client) drm_bridge_remove(&it6505->bridge); drm_dp_aux_unregister(&it6505->aux); - it6505_debugfs_remove(it6505); it6505_poweroff(it6505); it6505_remove_edid(it6505); }
Do not create a custom directory in debugfs-root, but use the debugfs_init callback to create a custom directory at the given place for the bridge. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- Only build tested, but following the same pattern as the tested ti-sn65dsi86. Changes since v1: * switch from 'client->debugfs' to DRM 'debugfs_init' callback drivers/gpu/drm/bridge/ite-it6505.c | 59 ++++++++++------------------- 1 file changed, 19 insertions(+), 40 deletions(-)