[PATCH 1/3] drm/bridge: ti-sn65dsi86: add link to datasheet

[PATCH 1/3] drm/bridge: ti-sn65dsi86: add link to datasheet

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

The bridge has pretty good docs, lets add a link to make them easier to
find.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index bcca9173c72a..f1a2493b86d9 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ * datasheet: http://www.ti.com/lit/ds/symlink/sn65dsi86.pdf
  */
 
 #include <linux/clk.h>
-- 
2.20.1

[PATCH 2/3] drm/bridge: ti-sn65dsi86: add debugfs

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Add a debugfs file to show status registers.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 42 +++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index f1a2493b86d9..a6f27648c015 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -5,6 +5,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/debugfs.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
 #include <linux/iopoll.h>
@@ -109,6 +110,7 @@ struct ti_sn_bridge {
 	struct drm_dp_aux		aux;
 	struct drm_bridge		bridge;
 	struct drm_connector		connector;
+	struct dentry			*debugfs;
 	struct device_node		*host_node;
 	struct mipi_dsi_device		*dsi;
 	struct clk			*refclk;
@@ -178,6 +180,42 @@ static const struct dev_pm_ops ti_sn_bridge_pm_ops = {
 	SET_RUNTIME_PM_OPS(ti_sn_bridge_suspend, ti_sn_bridge_resume, NULL)
 };
 
+static int status_show(struct seq_file *s, void *data)
+{
+	struct ti_sn_bridge *pdata = s->private;
+	unsigned int reg, val;
+
+	seq_puts(s, "STATUS REGISTERS:\n");
+
+	pm_runtime_get_sync(pdata->dev);
+
+	/* IRQ Status Registers, see Table 31 in datasheet */
+	for (reg = 0xf0; reg <= 0xf8; reg++) {
+		regmap_read(pdata->regmap, reg, &val);
+		seq_printf(s, "[0x%02x] = 0x%08x\n", reg, val);
+	}
+
+	pm_runtime_put(pdata->dev);
+
+	return 0;
+}
+
+DEFINE_SHOW_ATTRIBUTE(status);
+
+static void ti_sn_debugfs_init(struct ti_sn_bridge *pdata)
+{
+	pdata->debugfs = debugfs_create_dir("ti_sn65dsi86", NULL);
+
+	debugfs_create_file("status", 0600, pdata->debugfs, pdata,
+			&status_fops);
+}
+
+static void ti_sn_debugfs_remove(struct ti_sn_bridge *pdata)
+{
+	debugfs_remove_recursive(pdata->debugfs);
+	pdata->debugfs = NULL;
+}
+
 /* Connector funcs */
 static struct ti_sn_bridge *
 connector_to_ti_sn_bridge(struct drm_connector *connector)
@@ -869,6 +907,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
 
 	drm_bridge_add(&pdata->bridge);
 
+	ti_sn_debugfs_init(pdata);
+
 	return 0;
 }
 
@@ -879,6 +919,8 @@ static int ti_sn_bridge_remove(struct i2c_client *client)
 	if (!pdata)
 		return -EINVAL;
 
+	ti_sn_debugfs_remove(pdata);
+
 	of_node_put(pdata->host_node);
 
 	pm_runtime_disable(pdata->dev);
-- 
2.20.1

[PATCH 3/3] drm/bridge: ti-sn65dsi86: correct dsi mode_flags

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Noticed while comparing register dump of how bootloader configures DSI
vs how kernel configures.  It seems the bridge still works either way,
but fixing this clears the 'CHA_DATATYPE_ERR' error status bit.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index a6f27648c015..c8fb45e7b06d 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -342,8 +342,7 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge)
 	/* TODO: setting to 4 lanes always for now */
 	dsi->lanes = 4;
 	dsi->format = MIPI_DSI_FMT_RGB888;
-	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
-			  MIPI_DSI_MODE_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
 
 	/* check if continuous dsi clock is required or not */
 	pm_runtime_get_sync(pdata->dev);
-- 
2.20.1

Re: [PATCH 1/3] drm/bridge: ti-sn65dsi86: add link to datasheet

[Jeffrey Hugo]

On Tue, Jul 2, 2019 at 9:45 AM Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> The bridge has pretty good docs, lets add a link to make them easier to
> find.
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>

This is in the DT binding, but having it in the driver as well is a nice touch.

Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>

Re: [PATCH 2/3] drm/bridge: ti-sn65dsi86: add debugfs

[Jeffrey Hugo]

On Tue, Jul 2, 2019 at 9:46 AM Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> Add a debugfs file to show status registers.
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>

Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>

Re: [PATCH 3/3] drm/bridge: ti-sn65dsi86: correct dsi mode_flags

[Jeffrey Hugo]

On Tue, Jul 2, 2019 at 9:46 AM Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> Noticed while comparing register dump of how bootloader configures DSI
> vs how kernel configures.  It seems the bridge still works either way,
> but fixing this clears the 'CHA_DATATYPE_ERR' error status bit.
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index a6f27648c015..c8fb45e7b06d 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -342,8 +342,7 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge)
>         /* TODO: setting to 4 lanes always for now */
>         dsi->lanes = 4;
>         dsi->format = MIPI_DSI_FMT_RGB888;
> -       dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> -                         MIPI_DSI_MODE_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE;
> +       dsi->mode_flags = MIPI_DSI_MODE_VIDEO;

Did you check this against the datasheet?  Per my reading, EOT_PACKET
and VIDEO_HSE appear valid.  I don't know about VIDEO_SYNC_PULSE.

Re: [PATCH 3/3] drm/bridge: ti-sn65dsi86: correct dsi mode_flags

[Rob Clark]

On Tue, Jul 2, 2019 at 10:09 AM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote:
>
> On Tue, Jul 2, 2019 at 9:46 AM Rob Clark <robdclark@gmail.com> wrote:
> >
> > From: Rob Clark <robdclark@chromium.org>
> >
> > Noticed while comparing register dump of how bootloader configures DSI
> > vs how kernel configures.  It seems the bridge still works either way,
> > but fixing this clears the 'CHA_DATATYPE_ERR' error status bit.
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index a6f27648c015..c8fb45e7b06d 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -342,8 +342,7 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge)
> >         /* TODO: setting to 4 lanes always for now */
> >         dsi->lanes = 4;
> >         dsi->format = MIPI_DSI_FMT_RGB888;
> > -       dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> > -                         MIPI_DSI_MODE_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE;
> > +       dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
>
> Did you check this against the datasheet?  Per my reading, EOT_PACKET
> and VIDEO_HSE appear valid.  I don't know about VIDEO_SYNC_PULSE.

The EOT flat is badly named:

/* disable EoT packets in HS mode */
#define MIPI_DSI_MODE_EOT_PACKET    BIT(9)

I can double check out HSE, but this was one of the setting
differences between bootloader and kernel

BR,
-R

Re: [PATCH 3/3] drm/bridge: ti-sn65dsi86: correct dsi mode_flags

[Jeffrey Hugo]

On Tue, Jul 2, 2019 at 11:12 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Tue, Jul 2, 2019 at 10:09 AM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote:
> >
> > On Tue, Jul 2, 2019 at 9:46 AM Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > -       dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> > > -                         MIPI_DSI_MODE_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE;
> > > +       dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
> >
> > Did you check this against the datasheet?  Per my reading, EOT_PACKET
> > and VIDEO_HSE appear valid.  I don't know about VIDEO_SYNC_PULSE.
>
> The EOT flat is badly named:
>
> /* disable EoT packets in HS mode */
> #define MIPI_DSI_MODE_EOT_PACKET    BIT(9)
>
> I can double check out HSE, but this was one of the setting
> differences between bootloader and kernel

Ah yeah, you are right.  My eyes apparently skipped over the "disable".

If the bootloader is not setting the HSE, then I can't think of a
reason why we would be having an issue also not setting it.

Seems good to me

Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>

Re: [PATCH 1/3] drm/bridge: ti-sn65dsi86: add link to datasheet

[Andrzej Hajda]

On 02.07.2019 17:44, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
>
> The bridge has pretty good docs, lets add a link to make them easier to
> find.
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>


Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej


> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index bcca9173c72a..f1a2493b86d9 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
>   * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + * datasheet: http://www.ti.com/lit/ds/symlink/sn65dsi86.pdf
>   */
>  
>  #include <linux/clk.h>

Re: [PATCH 2/3] drm/bridge: ti-sn65dsi86: add debugfs

[Andrzej Hajda]

On 02.07.2019 17:44, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
>
> Add a debugfs file to show status registers.
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 42 +++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index f1a2493b86d9..a6f27648c015 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -5,6 +5,7 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/debugfs.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/i2c.h>
>  #include <linux/iopoll.h>
> @@ -109,6 +110,7 @@ struct ti_sn_bridge {
>  	struct drm_dp_aux		aux;
>  	struct drm_bridge		bridge;
>  	struct drm_connector		connector;
> +	struct dentry			*debugfs;
>  	struct device_node		*host_node;
>  	struct mipi_dsi_device		*dsi;
>  	struct clk			*refclk;
> @@ -178,6 +180,42 @@ static const struct dev_pm_ops ti_sn_bridge_pm_ops = {
>  	SET_RUNTIME_PM_OPS(ti_sn_bridge_suspend, ti_sn_bridge_resume, NULL)
>  };
>  
> +static int status_show(struct seq_file *s, void *data)
> +{
> +	struct ti_sn_bridge *pdata = s->private;
> +	unsigned int reg, val;
> +
> +	seq_puts(s, "STATUS REGISTERS:\n");
> +
> +	pm_runtime_get_sync(pdata->dev);
> +
> +	/* IRQ Status Registers, see Table 31 in datasheet */
> +	for (reg = 0xf0; reg <= 0xf8; reg++) {
> +		regmap_read(pdata->regmap, reg, &val);
> +		seq_printf(s, "[0x%02x] = 0x%08x\n", reg, val);
> +	}
> +
> +	pm_runtime_put(pdata->dev);
> +
> +	return 0;
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(status);
> +
> +static void ti_sn_debugfs_init(struct ti_sn_bridge *pdata)
> +{
> +	pdata->debugfs = debugfs_create_dir("ti_sn65dsi86", NULL);


If some day we will have board with two such bridges there will be a
problem.

Anyway:

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej



> +
> +	debugfs_create_file("status", 0600, pdata->debugfs, pdata,
> +			&status_fops);
> +}
> +
> +static void ti_sn_debugfs_remove(struct ti_sn_bridge *pdata)
> +{
> +	debugfs_remove_recursive(pdata->debugfs);
> +	pdata->debugfs = NULL;
> +}
> +
>  /* Connector funcs */
>  static struct ti_sn_bridge *
>  connector_to_ti_sn_bridge(struct drm_connector *connector)
> @@ -869,6 +907,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
>  
>  	drm_bridge_add(&pdata->bridge);
>  
> +	ti_sn_debugfs_init(pdata);
> +
>  	return 0;
>  }
>  
> @@ -879,6 +919,8 @@ static int ti_sn_bridge_remove(struct i2c_client *client)
>  	if (!pdata)
>  		return -EINVAL;
>  
> +	ti_sn_debugfs_remove(pdata);
> +
>  	of_node_put(pdata->host_node);
>  
>  	pm_runtime_disable(pdata->dev);

Re: [PATCH 1/3] drm/bridge: ti-sn65dsi86: add link to datasheet

[Laurent Pinchart]

Hi Rob,

Thank you for the patch.

On Tue, Jul 02, 2019 at 08:44:16AM -0700, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> The bridge has pretty good docs, lets add a link to make them easier to
> find.
> 
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index bcca9173c72a..f1a2493b86d9 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
>   * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + * datasheet: http://www.ti.com/lit/ds/symlink/sn65dsi86.pdf

It's in the DT bindings and it's easy to find so I'm not sure it adds
lots of value to mention it here, but I don't feel very strongly about
it, so if you think it can be useful I don't mind. I would however put a
blank line after the copyright line in that case, and
s/datasheet/Datasheet/

>   */
>  
>  #include <linux/clk.h>

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/3] drm/bridge: ti-sn65dsi86: correct dsi mode_flags

[Andrzej Hajda]

On 02.07.2019 19:23, Jeffrey Hugo wrote:
> On Tue, Jul 2, 2019 at 11:12 AM Rob Clark <robdclark@gmail.com> wrote:
>> On Tue, Jul 2, 2019 at 10:09 AM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote:
>>> On Tue, Jul 2, 2019 at 9:46 AM Rob Clark <robdclark@gmail.com> wrote:
>>>> -       dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
>>>> -                         MIPI_DSI_MODE_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE;
>>>> +       dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
>>> Did you check this against the datasheet?  Per my reading, EOT_PACKET
>>> and VIDEO_HSE appear valid.  I don't know about VIDEO_SYNC_PULSE.
>> The EOT flat is badly named:
>>
>> /* disable EoT packets in HS mode */
>> #define MIPI_DSI_MODE_EOT_PACKET    BIT(9)
>>
>> I can double check out HSE, but this was one of the setting
>> differences between bootloader and kernel
> Ah yeah, you are right.  My eyes apparently skipped over the "disable".
>
> If the bootloader is not setting the HSE, then I can't think of a
> reason why we would be having an issue also not setting it.
>
> Seems good to me
>
> Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
>
>
Yes, the flags require cleanup.

Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej

Re: [PATCH 2/3] drm/bridge: ti-sn65dsi86: add debugfs

[Laurent Pinchart]

Hello,

On Thu, Jul 04, 2019 at 02:31:20PM +0200, Andrzej Hajda wrote:
> On 02.07.2019 17:44, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > Add a debugfs file to show status registers.
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 42 +++++++++++++++++++++++++++
> >  1 file changed, 42 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index f1a2493b86d9..a6f27648c015 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -5,6 +5,7 @@
> >   */
> >  
> >  #include <linux/clk.h>
> > +#include <linux/debugfs.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/i2c.h>
> >  #include <linux/iopoll.h>
> > @@ -109,6 +110,7 @@ struct ti_sn_bridge {
> >  	struct drm_dp_aux		aux;
> >  	struct drm_bridge		bridge;
> >  	struct drm_connector		connector;
> > +	struct dentry			*debugfs;
> >  	struct device_node		*host_node;
> >  	struct mipi_dsi_device		*dsi;
> >  	struct clk			*refclk;
> > @@ -178,6 +180,42 @@ static const struct dev_pm_ops ti_sn_bridge_pm_ops = {
> >  	SET_RUNTIME_PM_OPS(ti_sn_bridge_suspend, ti_sn_bridge_resume, NULL)
> >  };
> >  
> > +static int status_show(struct seq_file *s, void *data)
> > +{
> > +	struct ti_sn_bridge *pdata = s->private;
> > +	unsigned int reg, val;
> > +
> > +	seq_puts(s, "STATUS REGISTERS:\n");

NO NEED TO SHOUT :-)

> > +
> > +	pm_runtime_get_sync(pdata->dev);
> > +
> > +	/* IRQ Status Registers, see Table 31 in datasheet */
> > +	for (reg = 0xf0; reg <= 0xf8; reg++) {
> > +		regmap_read(pdata->regmap, reg, &val);
> > +		seq_printf(s, "[0x%02x] = 0x%08x\n", reg, val);
> > +	}
> > +
> > +	pm_runtime_put(pdata->dev);
> > +
> > +	return 0;
> > +}
> > +
> > +DEFINE_SHOW_ATTRIBUTE(status);
> > +
> > +static void ti_sn_debugfs_init(struct ti_sn_bridge *pdata)
> > +{
> > +	pdata->debugfs = debugfs_create_dir("ti_sn65dsi86", NULL);
> 
> If some day we will have board with two such bridges there will be a
> problem.

Could we use the platform device name for this ?

> Anyway:
> 
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> 
> > +
> > +	debugfs_create_file("status", 0600, pdata->debugfs, pdata,
> > +			&status_fops);
> > +}
> > +
> > +static void ti_sn_debugfs_remove(struct ti_sn_bridge *pdata)
> > +{
> > +	debugfs_remove_recursive(pdata->debugfs);
> > +	pdata->debugfs = NULL;
> > +}
> > +

You need to conditionally-compile this based on CONFIG_DEBUG_FS.

> >  /* Connector funcs */
> >  static struct ti_sn_bridge *
> >  connector_to_ti_sn_bridge(struct drm_connector *connector)
> > @@ -869,6 +907,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client,
> >  
> >  	drm_bridge_add(&pdata->bridge);
> >  
> > +	ti_sn_debugfs_init(pdata);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -879,6 +919,8 @@ static int ti_sn_bridge_remove(struct i2c_client *client)
> >  	if (!pdata)
> >  		return -EINVAL;
> >  
> > +	ti_sn_debugfs_remove(pdata);
> > +
> >  	of_node_put(pdata->host_node);
> >  
> >  	pm_runtime_disable(pdata->dev);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] drm/bridge: ti-sn65dsi86: add debugfs

[Rob Clark]

On Thu, Jul 4, 2019 at 5:35 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hello,
>
> On Thu, Jul 04, 2019 at 02:31:20PM +0200, Andrzej Hajda wrote:
> > On 02.07.2019 17:44, Rob Clark wrote:
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > Add a debugfs file to show status registers.
> > >
> > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > ---
> > >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 42 +++++++++++++++++++++++++++
> > >  1 file changed, 42 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > index f1a2493b86d9..a6f27648c015 100644
> > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > @@ -5,6 +5,7 @@
> > >   */
> > >
> > >  #include <linux/clk.h>
> > > +#include <linux/debugfs.h>
> > >  #include <linux/gpio/consumer.h>
> > >  #include <linux/i2c.h>
> > >  #include <linux/iopoll.h>
> > > @@ -109,6 +110,7 @@ struct ti_sn_bridge {
> > >     struct drm_dp_aux               aux;
> > >     struct drm_bridge               bridge;
> > >     struct drm_connector            connector;
> > > +   struct dentry                   *debugfs;
> > >     struct device_node              *host_node;
> > >     struct mipi_dsi_device          *dsi;
> > >     struct clk                      *refclk;
> > > @@ -178,6 +180,42 @@ static const struct dev_pm_ops ti_sn_bridge_pm_ops = {
> > >     SET_RUNTIME_PM_OPS(ti_sn_bridge_suspend, ti_sn_bridge_resume, NULL)
> > >  };
> > >
> > > +static int status_show(struct seq_file *s, void *data)
> > > +{
> > > +   struct ti_sn_bridge *pdata = s->private;
> > > +   unsigned int reg, val;
> > > +
> > > +   seq_puts(s, "STATUS REGISTERS:\n");
>
> NO NEED TO SHOUT :-)
>
> > > +
> > > +   pm_runtime_get_sync(pdata->dev);
> > > +
> > > +   /* IRQ Status Registers, see Table 31 in datasheet */
> > > +   for (reg = 0xf0; reg <= 0xf8; reg++) {
> > > +           regmap_read(pdata->regmap, reg, &val);
> > > +           seq_printf(s, "[0x%02x] = 0x%08x\n", reg, val);
> > > +   }
> > > +
> > > +   pm_runtime_put(pdata->dev);
> > > +
> > > +   return 0;
> > > +}
> > > +
> > > +DEFINE_SHOW_ATTRIBUTE(status);
> > > +
> > > +static void ti_sn_debugfs_init(struct ti_sn_bridge *pdata)
> > > +{
> > > +   pdata->debugfs = debugfs_create_dir("ti_sn65dsi86", NULL);
> >
> > If some day we will have board with two such bridges there will be a
> > problem.
>
> Could we use the platform device name for this ?

hmm, yeah, that would solve the 2x bridges issue

> > Anyway:
> >
> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> >
> > > +
> > > +   debugfs_create_file("status", 0600, pdata->debugfs, pdata,
> > > +                   &status_fops);
> > > +}
> > > +
> > > +static void ti_sn_debugfs_remove(struct ti_sn_bridge *pdata)
> > > +{
> > > +   debugfs_remove_recursive(pdata->debugfs);
> > > +   pdata->debugfs = NULL;
> > > +}
> > > +
>
> You need to conditionally-compile this based on CONFIG_DEBUG_FS.

Hmm, is that really true?  Debugfs appears to be sufficently stub'd w/
inline no-ops in the !CONFIG_DEBUG_FS case

BR,
-R

Re: [PATCH 2/3] drm/bridge: ti-sn65dsi86: add debugfs

[Laurent Pinchart]

Hi Rob,

On Thu, Jul 04, 2019 at 06:56:56AM -0700, Rob Clark wrote:
> On Thu, Jul 4, 2019 at 5:35 AM Laurent Pinchartwrote:
> > On Thu, Jul 04, 2019 at 02:31:20PM +0200, Andrzej Hajda wrote:
> > > On 02.07.2019 17:44, Rob Clark wrote:
> > > > From: Rob Clark <robdclark@chromium.org>
> > > >
> > > > Add a debugfs file to show status registers.
> > > >
> > > > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > > > ---
> > > >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 42 +++++++++++++++++++++++++++
> > > >  1 file changed, 42 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > index f1a2493b86d9..a6f27648c015 100644
> > > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > > > @@ -5,6 +5,7 @@
> > > >   */
> > > >
> > > >  #include <linux/clk.h>
> > > > +#include <linux/debugfs.h>
> > > >  #include <linux/gpio/consumer.h>
> > > >  #include <linux/i2c.h>
> > > >  #include <linux/iopoll.h>
> > > > @@ -109,6 +110,7 @@ struct ti_sn_bridge {
> > > >     struct drm_dp_aux               aux;
> > > >     struct drm_bridge               bridge;
> > > >     struct drm_connector            connector;
> > > > +   struct dentry                   *debugfs;
> > > >     struct device_node              *host_node;
> > > >     struct mipi_dsi_device          *dsi;
> > > >     struct clk                      *refclk;
> > > > @@ -178,6 +180,42 @@ static const struct dev_pm_ops ti_sn_bridge_pm_ops = {
> > > >     SET_RUNTIME_PM_OPS(ti_sn_bridge_suspend, ti_sn_bridge_resume, NULL)
> > > >  };
> > > >
> > > > +static int status_show(struct seq_file *s, void *data)
> > > > +{
> > > > +   struct ti_sn_bridge *pdata = s->private;
> > > > +   unsigned int reg, val;
> > > > +
> > > > +   seq_puts(s, "STATUS REGISTERS:\n");
> >
> > NO NEED TO SHOUT :-)
> >
> > > > +
> > > > +   pm_runtime_get_sync(pdata->dev);
> > > > +
> > > > +   /* IRQ Status Registers, see Table 31 in datasheet */
> > > > +   for (reg = 0xf0; reg <= 0xf8; reg++) {
> > > > +           regmap_read(pdata->regmap, reg, &val);
> > > > +           seq_printf(s, "[0x%02x] = 0x%08x\n", reg, val);
> > > > +   }
> > > > +
> > > > +   pm_runtime_put(pdata->dev);
> > > > +
> > > > +   return 0;
> > > > +}
> > > > +
> > > > +DEFINE_SHOW_ATTRIBUTE(status);
> > > > +
> > > > +static void ti_sn_debugfs_init(struct ti_sn_bridge *pdata)
> > > > +{
> > > > +   pdata->debugfs = debugfs_create_dir("ti_sn65dsi86", NULL);
> > >
> > > If some day we will have board with two such bridges there will be a
> > > problem.
> >
> > Could we use the platform device name for this ?
> 
> hmm, yeah, that would solve the 2x bridges issue
> 
> > > Anyway:
> > >
> > > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> > >
> > > > +
> > > > +   debugfs_create_file("status", 0600, pdata->debugfs, pdata,
> > > > +                   &status_fops);
> > > > +}
> > > > +
> > > > +static void ti_sn_debugfs_remove(struct ti_sn_bridge *pdata)
> > > > +{
> > > > +   debugfs_remove_recursive(pdata->debugfs);
> > > > +   pdata->debugfs = NULL;
> > > > +}
> > > > +
> >
> > You need to conditionally-compile this based on CONFIG_DEBUG_FS.
> 
> Hmm, is that really true?  Debugfs appears to be sufficently stub'd w/
> inline no-ops in the !CONFIG_DEBUG_FS case

You're right, my bad. I wonder if the compiler will optimise the above
two functions out. It might warrant a CONFIG_DEBUG_FS check here for
that reason, but that's really bikeshedding. So with the 2x bridges
issue addressed, I think the patch will be good.

-- 
Regards,

Laurent Pinchart