Message-ID: <CAM+2EuJKPO0iGPK2uqPhdTpLvR0-+Vwh_K-FWTMJ96+AvzLO1A@mail.gmail.com>
Date: Tue, 22 Apr 2025 20:55:50 +0530
From: Jagath Jog J <jagathjog1996@...il.com>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
Cc: mripard@...nel.org, tzimmermann@...e.de, airlied@...il.com, 
	simona@...ll.ch, mcanal@...lia.com, maarten.lankhorst@...ux.intel.com, 
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC 1/1] drm/mipi-dbi: Use drm_device for debugfs, drop
 drm_minor and .debugfs_init

Hi Dmitry

Thanks for the reply

On Mon, Apr 21, 2025 at 3:59 PM Dmitry Baryshkov
<dmitry.baryshkov@....qualcomm.com> wrote:
>
> On Mon, Apr 21, 2025 at 02:29:07PM +0530, Jagath Jog J wrote:
> > Refactor to use drm_device.debugfs_root instead of drm_minor for
> > debugfs file creation. The driver can now initialize debugfs directly
> > in probe(), before drm_dev_register(). This also removes the use of
> > .debugfs_init callback.
>
> Why? The callback was designed to add debugfs files. Likewise most if
> not all DRM drivers add files under the corresponding minor node.

My intention here was to follow the direction suggested in the DRM TODO list
https://docs.kernel.org/gpu/todo.html#clean-up-the-debugfs-support

It recommends using drm_device instead of drm_minor, transitioning towards
drm_device.debugfs_root, and avoiding .debugfs_init. The RFC patch was
an initial
step to gather feedback.

Related to this todo, some drivers use drm_debugfs_add_files() instead of
drm_debugfs_create_files(). For example

hdlcd - https://patchwork.freedesktop.org/patch/msgid/20221226155029.244355-4-mcanal@igalia.com
v3d - https://patchwork.freedesktop.org/patch/msgid/20221219120621.15086-6-mcanal@igalia.com
https://elixir.bootlin.com/linux/v6.14.3/source/include/drm/drm_device.h#L92

Please let me know your thoughts on this.


Regards
Jagath
>
> >
> > Signed-off-by: Jagath Jog J <jagathjog1996@...il.com>
> > ---
> >  drivers/gpu/drm/drm_mipi_dbi.c        | 8 ++++----
> >  drivers/gpu/drm/tiny/ili9163.c        | 3 ++-
> >  drivers/gpu/drm/tiny/panel-mipi-dbi.c | 3 ++-
> >  include/drm/drm_mipi_dbi.h            | 4 ++--
> >  4 files changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
> > index 89e05a5bed1d..66f292c48a78 100644
> > --- a/drivers/gpu/drm/drm_mipi_dbi.c
> > +++ b/drivers/gpu/drm/drm_mipi_dbi.c
> > @@ -1488,17 +1488,17 @@ static const struct file_operations mipi_dbi_debugfs_command_fops = {
> >   *
> >   * This function creates a 'command' debugfs file for sending commands to the
> >   * controller or getting the read command values.
> > - * Drivers can use this as their &drm_driver->debugfs_init callback.
> > + * Drivers can call this function before registering their device to drm.
> >   *
> >   */
> > -void mipi_dbi_debugfs_init(struct drm_minor *minor)
> > +void mipi_dbi_debugfs_init(struct mipi_dbi_dev *dbidev)
> >  {
> > -     struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(minor->dev);
> >       umode_t mode = S_IFREG | S_IWUSR;
> >
> >       if (dbidev->dbi.read_commands)
> >               mode |= S_IRUGO;
> > -     debugfs_create_file("command", mode, minor->debugfs_root, dbidev,
> > +
> > +     debugfs_create_file("command", mode, dbidev->drm.debugfs_root, dbidev,
> >                           &mipi_dbi_debugfs_command_fops);
> >  }
> >  EXPORT_SYMBOL(mipi_dbi_debugfs_init);
> > diff --git a/drivers/gpu/drm/tiny/ili9163.c b/drivers/gpu/drm/tiny/ili9163.c
> > index 62cadf5e033d..351d2a5b9f27 100644
> > --- a/drivers/gpu/drm/tiny/ili9163.c
> > +++ b/drivers/gpu/drm/tiny/ili9163.c
> > @@ -115,7 +115,6 @@ static struct drm_driver ili9163_driver = {
> >       .fops                   = &ili9163_fops,
> >       DRM_GEM_DMA_DRIVER_OPS_VMAP,
> >       DRM_FBDEV_DMA_DRIVER_OPS,
> > -     .debugfs_init           = mipi_dbi_debugfs_init,
> >       .name                   = "ili9163",
> >       .desc                   = "Ilitek ILI9163",
> >       .major                  = 1,
> > @@ -182,6 +181,8 @@ static int ili9163_probe(struct spi_device *spi)
> >
> >       drm_mode_config_reset(drm);
> >
> > +     mipi_dbi_debugfs_init(dbidev);
> > +
> >       ret = drm_dev_register(drm, 0);
> >       if (ret)
> >               return ret;
> > diff --git a/drivers/gpu/drm/tiny/panel-mipi-dbi.c b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
> > index 0460ecaef4bd..94466dd8db9f 100644
> > --- a/drivers/gpu/drm/tiny/panel-mipi-dbi.c
> > +++ b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
> > @@ -267,7 +267,6 @@ static const struct drm_driver panel_mipi_dbi_driver = {
> >       .fops                   = &panel_mipi_dbi_fops,
> >       DRM_GEM_DMA_DRIVER_OPS_VMAP,
> >       DRM_FBDEV_DMA_DRIVER_OPS,
> > -     .debugfs_init           = mipi_dbi_debugfs_init,
> >       .name                   = "panel-mipi-dbi",
> >       .desc                   = "MIPI DBI compatible display panel",
> >       .major                  = 1,
> > @@ -384,6 +383,8 @@ static int panel_mipi_dbi_spi_probe(struct spi_device *spi)
> >
> >       drm_mode_config_reset(drm);
> >
> > +     mipi_dbi_debugfs_init(dbidev);
> > +
> >       ret = drm_dev_register(drm, 0);
> >       if (ret)
> >               return ret;
> > diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
> > index f45f9612c0bc..88a9c87a1e99 100644
> > --- a/include/drm/drm_mipi_dbi.h
> > +++ b/include/drm/drm_mipi_dbi.h
> > @@ -230,9 +230,9 @@ int mipi_dbi_buf_copy(void *dst, struct iosys_map *src, struct drm_framebuffer *
> >  })
> >
> >  #ifdef CONFIG_DEBUG_FS
> > -void mipi_dbi_debugfs_init(struct drm_minor *minor);
> > +void mipi_dbi_debugfs_init(struct mipi_dbi_dev *dbidev);
> >  #else
> > -static inline void mipi_dbi_debugfs_init(struct drm_minor *minor) {}
> > +static inline void mipi_dbi_debugfs_init(struct mipi_dbi_dev *dbidev) {}
> >  #endif
> >
> >  /**
> > --
> > 2.20.1
> >
>
> --
> With best wishes
> Dmitry

Powered by blists - more mailing lists