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

From: Tomi Valkeinen
Date: Mon Dec 09 2019 - 10:05:36 EST

On 09/12/2019 16:38, Andrey Smirnov wrote:
On Mon, Dec 9, 2019 at 1:38 AM Tomi Valkeinen <tomi.valkeinen@xxxxxx> 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

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.


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