[PATCH RFT 0/2] drm/bridge: Use per-client debugfs entry

[PATCH RFT 0/2] drm/bridge: Use per-client debugfs entry

[Wolfram Sang]

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

 drivers/gpu/drm/bridge/ite-it6505.c   | 28 +++-----------------------
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 29 +--------------------------
 2 files changed, 4 insertions(+), 53 deletions(-)

-- 
2.45.2

[PATCH RFT 1/2] drm/bridge: it6505: Use per-client debugfs entry

[Wolfram Sang]

The I2C core now offers a debugfs-directory per client. Use it and
remove the custom handling. This also fixes the problem of a static
debugfs dir name which would break if two of these devices would be on
one system.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/gpu/drm/bridge/ite-it6505.c | 28 +++-------------------------
 1 file changed, 3 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 88ef76a37fe6..f32e3691a9ea 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;
@@ -3574,37 +3572,18 @@ static const struct debugfs_entries debugfs_entry[] = {
 	{ NULL, NULL },
 };
 
-static void debugfs_create_files(struct it6505 *it6505)
+static void it6505_debugfs_create_files(struct i2c_client *client, struct it6505 *it6505)
 {
 	int i = 0;
 
 	while (debugfs_entry[i].name && debugfs_entry[i].fops) {
 		debugfs_create_file(debugfs_entry[i].name, 0644,
-				    it6505->debugfs, it6505,
+				    client->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 void it6505_shutdown(struct i2c_client *client)
 {
 	struct it6505 *it6505 = dev_get_drvdata(&client->dev);
@@ -3689,7 +3668,7 @@ 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);
+	it6505_debugfs_create_files(client, it6505);
 	pm_runtime_enable(dev);
 
 	it6505->aux.name = "DP-AUX";
@@ -3712,7 +3691,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.45.2

[PATCH RFT 2/2] drm/bridge: ti-sn65dsi86: Use per-client debugfs entry

[Wolfram Sang]

The I2C core now offers a debugfs-directory per client. Use it and
remove the custom handling.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 29 +--------------------------
 1 file changed, 1 insertion(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index e4d9006b59f1..57a3f592c057 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -427,33 +427,6 @@ static int status_show(struct seq_file *s, void *data)
 
 DEFINE_SHOW_ATTRIBUTE(status);
 
-static void ti_sn65dsi86_debugfs_remove(void *data)
-{
-	debugfs_remove_recursive(data);
-}
-
-static void ti_sn65dsi86_debugfs_init(struct ti_sn65dsi86 *pdata)
-{
-	struct device *dev = pdata->dev;
-	struct dentry *debugfs;
-	int ret;
-
-	debugfs = debugfs_create_dir(dev_name(dev), NULL);
-
-	/*
-	 * We might get an error back if debugfs wasn't enabled in the kernel
-	 * so let's just silently return upon failure.
-	 */
-	if (IS_ERR_OR_NULL(debugfs))
-		return;
-
-	ret = devm_add_action_or_reset(dev, ti_sn65dsi86_debugfs_remove, debugfs);
-	if (ret)
-		return;
-
-	debugfs_create_file("status", 0600, debugfs, pdata, &status_fops);
-}
-
 /* -----------------------------------------------------------------------------
  * Auxiliary Devices (*not* AUX)
  */
@@ -1936,7 +1909,7 @@ static int ti_sn65dsi86_probe(struct i2c_client *client)
 	if (ret)
 		return ret;
 
-	ti_sn65dsi86_debugfs_init(pdata);
+	debugfs_create_file("status", 0600, client->debugfs, pdata, &status_fops);
 
 	/*
 	 * Break ourselves up into a collection of aux devices. The only real
-- 
2.45.2

Re: [PATCH RFT 0/2] drm/bridge: Use per-client debugfs entry

[Dmitry Baryshkov]

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.

-- 
With best wishes
Dmitry

Re: [PATCH RFT 0/2] drm/bridge: Use per-client debugfs entry

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1395 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFT 2/2] drm/bridge: ti-sn65dsi86: Use per-client debugfs entry

[Geert Uytterhoeven]

Hi Wolfram,

On Sat, 25 Jan 2025 at 13:53, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> The I2C core now offers a debugfs-directory per client. Use it and
> remove the custom handling.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks for your patch!

I gave it a try on Gray Hawk Single.

Old location:

    /sys/kernel/debug/1-002c
    `-- status

New location:

    /sys/kernel/debug/i2c/i2c-1/1-002c
    `-- status

Contents of the status file without a display connected:

    STATUS REGISTERS:
    [0xf0] = 0x00000000
    [0xf1] = 0x0000000b
    [0xf2] = 0x00000000
    [0xf3] = 0x00000000
    [0xf4] = 0x00000009
    [0xf5] = 0x00000020
    [0xf6] = 0x00000000
    [0xf7] = 0x00000000
    [0xf8] = 0x00000002

Contents of the status file with a 1920x1080 display connected:

    STATUS REGISTERS:
    [0xf0] = 0x00000000
    [0xf1] = 0x00000000
    [0xf2] = 0x00000000
    [0xf3] = 0x00000000
    [0xf4] = 0x00000001
    [0xf5] = 0x00000000
    [0xf6] = 0x00000002
    [0xf7] = 0x00000000
    [0xf8] = 0x00000001

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH RFT 0/2] drm/bridge: Use per-client debugfs entry

[Dmitry Baryshkov]

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
> 



-- 
With best wishes
Dmitry

Re: [PATCH RFT 0/2] drm/bridge: Use per-client debugfs entry

[Doug Anderson]

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

Re: [PATCH RFT 0/2] drm/bridge: Use per-client debugfs entry

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 506 bytes --]

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


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFT 2/2] drm/bridge: ti-sn65dsi86: Use per-client debugfs entry

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 97 bytes --]


> I gave it a try on Gray Hawk Single.

Oh, we have such a board? Nice! That would help a lot.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH RFT 2/2] drm/bridge: ti-sn65dsi86: Use per-client debugfs entry

[Geert Uytterhoeven]

Hi Wolfram,

On Tue, 28 Jan 2025 at 11:05, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > I gave it a try on Gray Hawk Single.
>
> Oh, we have such a board? Nice! That would help a lot.

We have several! ;-)

$ git grep ti,sn65dsi86 -- arch/*/boot/dts/renesas
arch/arm64/boot/dts/renesas/r8a779a0-falcon-cpu.dtsi:
compatible = "ti,sn65dsi86";
arch/arm64/boot/dts/renesas/r8a779g0-white-hawk.dts:
compatible = "ti,sn65dsi86";
arch/arm64/boot/dts/renesas/r8a779h0-gray-hawk-single.dts:
 compatible = "ti,sn65dsi86";
arch/arm64/boot/dts/renesas/white-hawk-cpu-common.dtsi:
compatible = "ti,sn65dsi86";

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds