From: Tomi Valkeinen <tomi.valkeinen@ti.com>
To: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: <dri-devel@lists.freedesktop.org>,
Andrzej Hajda <a.hajda@samsung.com>,
Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
Cory Tusar <cory.tusar@zii.aero>, Chris Healy <cphealy@gmail.com>,
Lucas Stach <l.stach@pengutronix.de>,
linux-kernel <linux-kernel@vger.kernel.org>,
Daniel Vetter <daniel@ffwll.ch>
Subject: Re: [PATCH v3 2/2] drm/bridge: tc358767: Expose test mode functionality via debugfs
Date: Mon, 9 Dec 2019 17:05:19 +0200 [thread overview]
Message-ID: <f873e4de-eabf-2746-8ad8-3d3c7d64a270@ti.com> (raw)
In-Reply-To: <CAHQ1cqH8XTYysd1Nv2Q0EziXfJWeemZeyyZZ3OKoCv8=XrHZWA@mail.gmail.com>
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
next prev parent reply other threads:[~2019-12-09 15:05 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-09 5:08 [PATCH v3 0/2] tc358767 test mode Andrey Smirnov
2019-12-09 5:08 ` [PATCH v3 1/2] drm/bridge: tc358767: Introduce __tc_bridge_enable/disable() Andrey Smirnov
2019-12-09 5:08 ` [PATCH v3 2/2] drm/bridge: tc358767: Expose test mode functionality via debugfs Andrey Smirnov
2019-12-09 9:38 ` Tomi Valkeinen
2019-12-09 14:38 ` Andrey Smirnov
2019-12-09 15:05 ` Tomi Valkeinen [this message]
2019-12-09 15:24 ` Andrey Smirnov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f873e4de-eabf-2746-8ad8-3d3c7d64a270@ti.com \
--to=tomi.valkeinen@ti.com \
--cc=Laurent.pinchart@ideasonboard.com \
--cc=a.hajda@samsung.com \
--cc=andrew.smirnov@gmail.com \
--cc=cory.tusar@zii.aero \
--cc=cphealy@gmail.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=l.stach@pengutronix.de \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).