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@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
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