[PATCH v3 0/2] tc358767 test mode

[PATCH v3 0/2] tc358767 test mode

[Andrey Smirnov]

Everyone:

This series is a couple of patches exposing TestCtl register of
tc358767, which can be pretty handy when troubleshooting link problems.

Changes since [v2]:

    - Series rebased on 5.4 kernel

Changes since [v1]:

    - Debugfs moved into a standalone directory and is now created as
      a part of probe()

    - Added tstctl_lock to ensure mutual exclusion of tstctl code and
      bridge's enable/disable methods

    - tc_tstctl_set() changed to function only if bridge was previosly
      enabled

    - Added comment explaining data format expected by "tstctl"

    - Debugfs permission changed to reflect write-only nature of this
      feature

    - Original commit split into two

    - Minor formatting changes

Thanks,
Andrey Smirnov

[v1] lore.kernel.org/r/20190826182524.5064-1-andrew.smirnov@gmail.com
[v2] lore.kernel.org/r/20190912013740.5638-1-andrew.smirnov@gmail.com

Andrey Smirnov (2):
  drm/bridge: tc358767: Introduce __tc_bridge_enable/disable()
  drm/bridge: tc358767: Expose test mode functionality via debugfs

 drivers/gpu/drm/bridge/tc358767.c | 184 ++++++++++++++++++++++++------
 1 file changed, 148 insertions(+), 36 deletions(-)

-- 
2.21.0

[PATCH v3 1/2] drm/bridge: tc358767: Introduce __tc_bridge_enable/disable()

[Andrey Smirnov]

Expose underlying implementation of bridge's enable/disable functions,
so it would be possible to use them in other parts of the driver.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Cory Tusar <cory.tusar@zii.aero>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/bridge/tc358767.c | 32 ++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 8a8d605021f0..3c252ae0ee6f 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1222,39 +1222,43 @@ static void tc_bridge_pre_enable(struct drm_bridge *bridge)
 	drm_panel_prepare(tc->panel);
 }
 
-static void tc_bridge_enable(struct drm_bridge *bridge)
+static int __tc_bridge_enable(struct tc_data *tc)
 {
-	struct tc_data *tc = bridge_to_tc(bridge);
 	int ret;
 
 	ret = tc_get_display_props(tc);
 	if (ret < 0) {
 		dev_err(tc->dev, "failed to read display props: %d\n", ret);
-		return;
+		return ret;
 	}
 
 	ret = tc_main_link_enable(tc);
 	if (ret < 0) {
 		dev_err(tc->dev, "main link enable error: %d\n", ret);
-		return;
+		return ret;
 	}
 
 	ret = tc_stream_enable(tc);
 	if (ret < 0) {
 		dev_err(tc->dev, "main link stream start error: %d\n", ret);
 		tc_main_link_disable(tc);
-		return;
+		return ret;
 	}
 
-	drm_panel_enable(tc->panel);
+	return 0;
 }
 
-static void tc_bridge_disable(struct drm_bridge *bridge)
+static void tc_bridge_enable(struct drm_bridge *bridge)
 {
 	struct tc_data *tc = bridge_to_tc(bridge);
-	int ret;
 
-	drm_panel_disable(tc->panel);
+	if (!__tc_bridge_enable(tc))
+		drm_panel_enable(tc->panel);
+}
+
+static int __tc_bridge_disable(struct tc_data *tc)
+{
+	int ret;
 
 	ret = tc_stream_disable(tc);
 	if (ret < 0)
@@ -1263,6 +1267,16 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
 	ret = tc_main_link_disable(tc);
 	if (ret < 0)
 		dev_err(tc->dev, "main link disable error: %d\n", ret);
+
+	return ret;
+}
+
+static void tc_bridge_disable(struct drm_bridge *bridge)
+{
+	struct tc_data *tc = bridge_to_tc(bridge);
+
+	drm_panel_disable(tc->panel);
+	__tc_bridge_disable(tc);
 }
 
 static void tc_bridge_post_disable(struct drm_bridge *bridge)
-- 
2.21.0

[PATCH v3 2/2] drm/bridge: tc358767: Expose test mode functionality via debugfs

[Andrey Smirnov]

Presently, the driver code artificially limits test pattern mode to a
single pattern with fixed color selection. It being a kernel module
parameter makes switching "test pattern" <-> "proper output" modes
on-the-fly clunky and outright impossible if the driver is built into
the kernel.

To improve the situation a bit, convert current test pattern code to
use debugfs instead by exposing "TestCtl" register. This way old
"tc_test_pattern=1" functionality can be emulated via:

    echo -n 0x78146302 > tstctl

and switch back to regular mode can be done with:

    echo -n 0x78146300 > tstctl

Note that switching to any of the test patterns, will NOT trigger link
re-establishment whereas switching to normal operation WILL. This is
done so:

a) we can isolate and verify (e)DP link functionality by switching to
   one of the test patters

b) trigger a link re-establishment by switching back to normal mode

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: Cory Tusar <cory.tusar@zii.aero>
Cc: Chris Healy <cphealy@gmail.com>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/bridge/tc358767.c | 152 ++++++++++++++++++++++++------
 1 file changed, 125 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 3c252ae0ee6f..12a8829e0ed1 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -17,6 +17,7 @@
 
 #include <linux/bitfield.h>
 #include <linux/clk.h>
+#include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/gpio/consumer.h>
 #include <linux/i2c.h>
@@ -221,11 +222,10 @@
 #define COLOR_B			GENMASK(15, 8)
 #define ENI2CFILTER		BIT(4)
 #define COLOR_BAR_MODE		GENMASK(1, 0)
+#define COLOR_BAR_MODE_NORMAL	0
 #define COLOR_BAR_MODE_BARS	2
-#define PLL_DBG			0x0a04
 
-static bool tc_test_pattern;
-module_param_named(test, tc_test_pattern, bool, 0644);
+#define PLL_DBG			0x0a04
 
 struct tc_edp_link {
 	struct drm_dp_link	base;
@@ -263,6 +263,9 @@ struct tc_data {
 
 	/* HPD pin number (0 or 1) or -ENODEV */
 	int			hpd_pin;
+
+	struct mutex		tstctl_lock;
+	bool			enabled;
 };
 
 static inline struct tc_data *aux_to_tc(struct drm_dp_aux *a)
@@ -791,16 +794,6 @@ static int tc_set_video_mode(struct tc_data *tc,
 	if (ret)
 		return ret;
 
-	/* Test pattern settings */
-	ret = regmap_write(tc->regmap, TSTCTL,
-			   FIELD_PREP(COLOR_R, 120) |
-			   FIELD_PREP(COLOR_G, 20) |
-			   FIELD_PREP(COLOR_B, 99) |
-			   ENI2CFILTER |
-			   FIELD_PREP(COLOR_BAR_MODE, COLOR_BAR_MODE_BARS));
-	if (ret)
-		return ret;
-
 	/* DP Main Stream Attributes */
 	vid_sync_dly = hsync_len + left_margin + mode->hdisplay;
 	ret = regmap_write(tc->regmap, DP0_VIDSYNCDELAY,
@@ -1152,14 +1145,6 @@ static int tc_stream_enable(struct tc_data *tc)
 
 	dev_dbg(tc->dev, "enable video stream\n");
 
-	/* PXL PLL setup */
-	if (tc_test_pattern) {
-		ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
-				    1000 * tc->mode.clock);
-		if (ret)
-			return ret;
-	}
-
 	ret = tc_set_video_mode(tc, &tc->mode);
 	if (ret)
 		return ret;
@@ -1188,12 +1173,8 @@ static int tc_stream_enable(struct tc_data *tc)
 	if (ret)
 		return ret;
 	/* Set input interface */
-	value = DP0_AUDSRC_NO_INPUT;
-	if (tc_test_pattern)
-		value |= DP0_VIDSRC_COLOR_BAR;
-	else
-		value |= DP0_VIDSRC_DPI_RX;
-	ret = regmap_write(tc->regmap, SYSCTRL, value);
+	ret = regmap_write(tc->regmap, SYSCTRL,
+			   DP0_AUDSRC_NO_INPUT | DP0_VIDSRC_DPI_RX);
 	if (ret)
 		return ret;
 
@@ -1252,8 +1233,13 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
 {
 	struct tc_data *tc = bridge_to_tc(bridge);
 
+	mutex_lock(&tc->tstctl_lock);
+
 	if (!__tc_bridge_enable(tc))
 		drm_panel_enable(tc->panel);
+
+	tc->enabled = true;
+	mutex_unlock(&tc->tstctl_lock);
 }
 
 static int __tc_bridge_disable(struct tc_data *tc)
@@ -1275,8 +1261,13 @@ static void tc_bridge_disable(struct drm_bridge *bridge)
 {
 	struct tc_data *tc = bridge_to_tc(bridge);
 
+	mutex_lock(&tc->tstctl_lock);
+
 	drm_panel_disable(tc->panel);
 	__tc_bridge_disable(tc);
+
+	tc->enabled = false;
+	mutex_unlock(&tc->tstctl_lock);
 }
 
 static void tc_bridge_post_disable(struct drm_bridge *bridge)
@@ -1388,6 +1379,99 @@ static enum drm_connector_status tc_connector_detect(struct drm_connector *conne
 		return connector_status_disconnected;
 }
 
+static int tc_tstctl_set(void *data, u64 val)
+{
+	struct tc_data *tc = data;
+	int ret;
+
+	mutex_lock(&tc->tstctl_lock);
+
+	if (!tc->enabled) {
+		dev_err(tc->dev, "bridge needs to be enabled first\n");
+		ret = -EBUSY;
+		goto unlock;
+	}
+
+	if (FIELD_GET(COLOR_BAR_MODE, val) == COLOR_BAR_MODE_NORMAL) {
+		ret = regmap_write(tc->regmap, SYSCTRL, DP0_VIDSRC_DPI_RX);
+		if (ret) {
+			dev_err(tc->dev,
+				"failed to select dpi video stream\n");
+			goto unlock;
+		}
+
+		ret = regmap_write(tc->regmap, TSTCTL, val | ENI2CFILTER);
+		if (ret) {
+			dev_err(tc->dev, "failed to set TSTCTL\n");
+			goto unlock;
+		}
+
+		ret = tc_pxl_pll_dis(tc);
+		if (ret) {
+			dev_err(tc->dev, "failed to disable PLL\n");
+			goto unlock;
+		}
+
+		/*
+		 * Re-establish DP link
+		 */
+		ret = __tc_bridge_disable(tc);
+		if (ret)
+			goto unlock;
+
+		ret = __tc_bridge_enable(tc);
+		if (ret)
+			goto unlock;
+	} else {
+		ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
+				    1000 * tc->mode.clock);
+		if (ret) {
+			dev_err(tc->dev, "failed to enable PLL\n");
+			goto unlock;
+		}
+
+		ret = regmap_write(tc->regmap, TSTCTL, val | ENI2CFILTER);
+		if (ret) {
+			dev_err(tc->dev, "failed to set TSTCTL\n");
+			goto unlock;
+		}
+
+		ret = regmap_write(tc->regmap, SYSCTRL, DP0_VIDSRC_COLOR_BAR);
+		if (ret) {
+			dev_err(tc->dev, "failed to set SYSCTRL\n");
+			goto unlock;
+		}
+	}
+
+unlock:
+	mutex_unlock(&tc->tstctl_lock);
+	return ret;
+}
+/*
+ * "tstctl" attribute has the following format:
+ *
+ *     RR_GG_BB_0_P
+ *
+ * "RR" is red, "GG" is green and "BB" is blue color components (byte
+ * each) used for various test patterns controlled by this register.
+ *
+ * "P" represents test pattern of the bridge. Valid values are:
+ *
+ *    "0" - normal operation
+ *    "1" - solid color test pattern
+ *    "2" - color bar test pattern
+ *    "3" - "checkers" test pattern
+ *
+ * This way old "tc_test_pattern=1" functionality can be emulated via:
+ *
+ *     echo -n 0x78146302 > tstctl
+ *
+ * and switch back to regular mode can be done with:
+ *
+ *     echo -n 0 > tstctl
+ */
+DEFINE_SIMPLE_ATTRIBUTE(tc_tstctl_fops, NULL, tc_tstctl_set, "%llu\n");
+
 static const struct drm_connector_funcs tc_connector_funcs = {
 	.detect = tc_connector_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
@@ -1530,9 +1614,15 @@ static irqreturn_t tc_irq_handler(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
+static void tc_remove_debugfs(void *data)
+{
+	debugfs_remove_recursive(data);
+}
+
 static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	struct device *dev = &client->dev;
+	struct dentry *debugfs;
 	struct tc_data *tc;
 	int ret;
 
@@ -1541,6 +1631,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		return -ENOMEM;
 
 	tc->dev = dev;
+	mutex_init(&tc->tstctl_lock);
 
 	/* port@2 is the output port */
 	ret = drm_of_find_panel_or_bridge(dev->of_node, 2, 0, &tc->panel, NULL);
@@ -1671,6 +1762,13 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	i2c_set_clientdata(client, tc);
 
+	debugfs = debugfs_create_dir(dev_name(dev), NULL);
+	if (!IS_ERR(debugfs)) {
+		debugfs_create_file_unsafe("tstctl", 0200, debugfs, tc,
+					   &tc_tstctl_fops);
+		devm_add_action_or_reset(dev, tc_remove_debugfs, debugfs);
+	}
+
 	return 0;
 }
 
-- 
2.21.0

Re: [PATCH v3 2/2] drm/bridge: tc358767: Expose test mode functionality via debugfs

[Tomi Valkeinen]

(Cc'ing Daniel for the last paragraph)

On 09/12/2019 07:08, Andrey Smirnov wrote:
> Presently, the driver code artificially limits test pattern mode to a
> single pattern with fixed color selection. It being a kernel module
> parameter makes switching "test pattern" <-> "proper output" modes
> on-the-fly clunky and outright impossible if the driver is built into
> the kernel.

That's not correct, /sys/module/tc358767/parameters/test is there even if the driver is built-in.

I think the bigger problems are that there's just one value, even if there are multiple devices, and 
that with kernel parameter the driver can't act on it dynamically (afaik).

> To improve the situation a bit, convert current test pattern code to
> use debugfs instead by exposing "TestCtl" register. This way old
> "tc_test_pattern=1" functionality can be emulated via:
> 
>      echo -n 0x78146302 > tstctl
> 
> and switch back to regular mode can be done with:
> 
>      echo -n 0x78146300 > tstctl

In the comment in the code you have 0 as return-to-regular-mode.

With my setup, enabling test mode seems to work, but when I return to regular mode, the first echo 
results in black display, but echoing 0 a second time will restore the display.

Hmm, actually, just echoing 0 to tstctl multiple times, it makes the screen go black and then 
restores it with every other echo.

> +	debugfs = debugfs_create_dir(dev_name(dev), NULL);
> +	if (!IS_ERR(debugfs)) {
> +		debugfs_create_file_unsafe("tstctl", 0200, debugfs, tc,
> +					   &tc_tstctl_fops);
> +		devm_add_action_or_reset(dev, tc_remove_debugfs, debugfs);
> +	}
> +

For me this creates debugfs/3-000f/tstctl. I don't think that's a clear or usable path, and could 
even cause a name conflict in the worst case.

Not sure what's a good solution here, but only two semi-good ones come to mind: have it in sysfs 
under the device's dir, or debugfs/dri/something. The latter probably needs some thought and common 
agreement on how to handle bridge and panel debugfs files. But that would be a good thing to have, 
as I'm sure there are other similar cases (at least I have a few).

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v3 2/2] drm/bridge: tc358767: Expose test mode functionality via debugfs

[Andrey Smirnov]

On Mon, Dec 9, 2019 at 1:38 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> (Cc'ing Daniel for the last paragraph)
>
> On 09/12/2019 07:08, Andrey Smirnov wrote:
> > Presently, the driver code artificially limits test pattern mode to a
> > single pattern with fixed color selection. It being a kernel module
> > parameter makes switching "test pattern" <-> "proper output" modes
> > on-the-fly clunky and outright impossible if the driver is built into
> > the kernel.
>
> That's not correct, /sys/module/tc358767/parameters/test is there even if the driver is built-in.
>

True, I'll drop the "impossible" part of the descrption. Having to
unbind and bind device to the driver to use that parameter definitely
falls under "clunky" for me still, so I'll just stick to that in the
description.

> I think the bigger problems are that there's just one value, even if there are multiple devices, and
> that with kernel parameter the driver can't act on it dynamically (afaik).
>
> > To improve the situation a bit, convert current test pattern code to
> > use debugfs instead by exposing "TestCtl" register. This way old
> > "tc_test_pattern=1" functionality can be emulated via:
> >
> >      echo -n 0x78146302 > tstctl
> >
> > and switch back to regular mode can be done with:
> >
> >      echo -n 0x78146300 > tstctl
>
> In the comment in the code you have 0 as return-to-regular-mode.

Both should work, but I'll modify commit message to match the code.

>
> With my setup, enabling test mode seems to work, but when I return to regular mode, the first echo
> results in black display, but echoing 0 a second time will restore the display.
>
> Hmm, actually, just echoing 0 to tstctl multiple times, it makes the screen go black and then
> restores it with every other echo.
>

Strange, works on my setup every time. No error messages in kernel log
I assume? Probably unrelated, but when you echo "0" and the screen
stays black, what do you see in DP_SINK_STATUS register:

dd if=/dev/drm_dp_aux0 bs=1 skip=$((0x205)) count=1 2>/dev/null | hexdump -Cv

? Note that this needs CONFIG_DRM_DP_AUX_CHARDEV to be enabled.

> > +     debugfs = debugfs_create_dir(dev_name(dev), NULL);
> > +     if (!IS_ERR(debugfs)) {
> > +             debugfs_create_file_unsafe("tstctl", 0200, debugfs, tc,
> > +                                        &tc_tstctl_fops);
> > +             devm_add_action_or_reset(dev, tc_remove_debugfs, debugfs);
> > +     }
> > +
>
> For me this creates debugfs/3-000f/tstctl. I don't think that's a clear or usable path, and could
> even cause a name conflict in the worst case.
>

I agree on usability aspect, but I am not sure I can see how a
conflict can happen. What scenario do you have in mind that would
cause that? My thinking was that the combination of I2C bus number +
I2C address should always be unique on the system, but maybe I am
missing something?

> Not sure what's a good solution here, but only two semi-good ones come to mind: have it in sysfs
> under the device's dir,

I'm fine with this option if it is the only path forward, but, given a
choice, I would _really_ rather not go the sysfs route.

Thanks,
Andrey Smirnov

Re: [PATCH v3 2/2] drm/bridge: tc358767: Expose test mode functionality via debugfs

[Tomi Valkeinen]

On 09/12/2019 16:38, Andrey Smirnov wrote:
> On Mon, Dec 9, 2019 at 1:38 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>
>> (Cc'ing Daniel for the last paragraph)
>>
>> On 09/12/2019 07:08, Andrey Smirnov wrote:
>>> Presently, the driver code artificially limits test pattern mode to a
>>> single pattern with fixed color selection. It being a kernel module
>>> parameter makes switching "test pattern" <-> "proper output" modes
>>> on-the-fly clunky and outright impossible if the driver is built into
>>> the kernel.
>>
>> That's not correct, /sys/module/tc358767/parameters/test is there even if the driver is built-in.
>>
> 
> True, I'll drop the "impossible" part of the descrption. Having to
> unbind and bind device to the driver to use that parameter definitely
> falls under "clunky" for me still, so I'll just stick to that in the
> description.

You don't need to re-bind. You can change the module parameter at runtime, and if the driver happens 
to use the value, then it uses the new value. If I recall right, changing the module parameter and 
then doing a full modeset from userspace made the driver to use the test mode (I'm not 100% sure, 
though).

In any case, I'm not advocating for the use of module parameter here =)

>> Hmm, actually, just echoing 0 to tstctl multiple times, it makes the screen go black and then
>> restores it with every other echo.
>>
> 
> Strange, works on my setup every time. No error messages in kernel log
> I assume? Probably unrelated, but when you echo "0" and the screen

No errors.

> stays black, what do you see in DP_SINK_STATUS register:
> 
> dd if=/dev/drm_dp_aux0 bs=1 skip=$((0x205)) count=1 2>/dev/null | hexdump -Cv
> 
> ? Note that this needs CONFIG_DRM_DP_AUX_CHARDEV to be enabled.

I'll check this later, and do a few more tests.

>>> +     debugfs = debugfs_create_dir(dev_name(dev), NULL);
>>> +     if (!IS_ERR(debugfs)) {
>>> +             debugfs_create_file_unsafe("tstctl", 0200, debugfs, tc,
>>> +                                        &tc_tstctl_fops);
>>> +             devm_add_action_or_reset(dev, tc_remove_debugfs, debugfs);
>>> +     }
>>> +
>>
>> For me this creates debugfs/3-000f/tstctl. I don't think that's a clear or usable path, and could
>> even cause a name conflict in the worst case.
>>
> 
> I agree on usability aspect, but I am not sure I can see how a
> conflict can happen. What scenario do you have in mind that would
> cause that? My thinking was that the combination of I2C bus number +
> I2C address should always be unique on the system, but maybe I am
> missing something?

Well, the dir name doesn't have "i2c" anywhere, so at least in theory, some other bus could have 
"3-000f" address too.

Maybe bigger problem is that it's not at all obvious what "3-000f" means. All the other debugfs dirs 
make sense when you look at the name, and "3-000f" looks very odd there.

  Tomi

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [PATCH v3 2/2] drm/bridge: tc358767: Expose test mode functionality via debugfs

[Andrey Smirnov]

On Mon, Dec 9, 2019 at 7:05 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>
> On 09/12/2019 16:38, Andrey Smirnov wrote:
> > On Mon, Dec 9, 2019 at 1:38 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >>
> >> (Cc'ing Daniel for the last paragraph)
> >>
> >> On 09/12/2019 07:08, Andrey Smirnov wrote:
> >>> Presently, the driver code artificially limits test pattern mode to a
> >>> single pattern with fixed color selection. It being a kernel module
> >>> parameter makes switching "test pattern" <-> "proper output" modes
> >>> on-the-fly clunky and outright impossible if the driver is built into
> >>> the kernel.
> >>
> >> That's not correct, /sys/module/tc358767/parameters/test is there even if the driver is built-in.
> >>
> >
> > True, I'll drop the "impossible" part of the descrption. Having to
> > unbind and bind device to the driver to use that parameter definitely
> > falls under "clunky" for me still, so I'll just stick to that in the
> > description.
>
> You don't need to re-bind. You can change the module parameter at runtime, and if the driver happens
> to use the value, then it uses the new value. If I recall right, changing the module parameter and
> then doing a full modeset from userspace made the driver to use the test mode (I'm not 100% sure,
> though).
>
> In any case, I'm not advocating for the use of module parameter here =)
>
> >> Hmm, actually, just echoing 0 to tstctl multiple times, it makes the screen go black and then
> >> restores it with every other echo.
> >>
> >
> > Strange, works on my setup every time. No error messages in kernel log
> > I assume? Probably unrelated, but when you echo "0" and the screen
>
> No errors.
>
> > stays black, what do you see in DP_SINK_STATUS register:
> >
> > dd if=/dev/drm_dp_aux0 bs=1 skip=$((0x205)) count=1 2>/dev/null | hexdump -Cv
> >
> > ? Note that this needs CONFIG_DRM_DP_AUX_CHARDEV to be enabled.
>
> I'll check this later, and do a few more tests.
>
> >>> +     debugfs = debugfs_create_dir(dev_name(dev), NULL);
> >>> +     if (!IS_ERR(debugfs)) {
> >>> +             debugfs_create_file_unsafe("tstctl", 0200, debugfs, tc,
> >>> +                                        &tc_tstctl_fops);
> >>> +             devm_add_action_or_reset(dev, tc_remove_debugfs, debugfs);
> >>> +     }
> >>> +
> >>
> >> For me this creates debugfs/3-000f/tstctl. I don't think that's a clear or usable path, and could
> >> even cause a name conflict in the worst case.
> >>
> >
> > I agree on usability aspect, but I am not sure I can see how a
> > conflict can happen. What scenario do you have in mind that would
> > cause that? My thinking was that the combination of I2C bus number +
> > I2C address should always be unique on the system, but maybe I am
> > missing something?
>
> Well, the dir name doesn't have "i2c" anywhere, so at least in theory, some other bus could have
> "3-000f" address too.
>
> Maybe bigger problem is that it's not at all obvious what "3-000f" means. All the other debugfs dirs
> make sense when you look at the name, and "3-000f" looks very odd there.
>

Fair enough, so what if we changed the name say "tc358767-3-000f" (i.
e. used "tc358767-" + dev_name(dev)), would that be a reasonable path
forward?

Thanks,
Andrey Smirnov