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

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