From: Sui Jingfeng <sui.jingfeng@linux.dev>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
Neil Armstrong <neil.armstrong@linaro.org>,
Randy Dunlap <rdunlap@infradead.org>,
dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Cc: Jessica Zhang <quic_jesszhan@quicinc.com>,
Sam Ravnborg <sam@ravnborg.org>,
Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
Maxime Ripard <mripard@kernel.org>,
Thomas Zimmermann <tzimmermann@suse.de>,
David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>
Subject: Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
Date: Fri, 26 Apr 2024 02:53:22 +0800 [thread overview]
Message-ID: <ce6a480d-80b3-46b0-a32d-26bc6480d02f@linux.dev> (raw)
In-Reply-To: <c2d41916-0b6c-43b5-98eb-514eb0511f84@linux.dev>
Hi,
On 2024/4/26 02:08, Sui Jingfeng wrote:
> Hi,
>
>
> On 2024/4/25 22:26, Andy Shevchenko wrote:
>> It seems driver missed the point of proper use of device property APIs.
>> Correct this by updating headers and calls respectively.
>
> You are using the 'seems' here exactly saying that you are not 100% sure.
>
Using the word 'seems' here exactly saying that you are not 100% sure.
> And this patch is has the risks to be wrong.
>
This patch has the risks of to be wrong.
> Simple because the "ili9341_probe() ---> ili9341_dbi_prob()" code path
> is DT dependent.
>
Simply because part of the driver is DT dependent, plus its code(implementation)
is deep(tight) coupling, as a result, it is became total DT dependent.
> First of all, the devm_of_find_backlight() is called in
> ili9341_dbi_probe()
,
> under *non-DT* environment,
Under *non-DT* environment, the use case probably should take into consideration.
Since you remove it, then we can't stop imagining. But if we really care about
the usage case on DT based systems, There is *NO* difference between the
device_get_match_data() and the of_device_get_match_data(). This is the reason
why I'm saying that you patch has the *ZERO* effects.
And again, on non-DT systems, if there is no acpi_device_id stuff, calling
the device_get_match_data() function will just get NULL. Which is nearly a
undefined behavior. So the 'OF 'removal is don't really make much sense.
But there is a way to save the awkward situation, that is, helps to get
this patch[1] merged. Then we still tenable both at the practice side
and at the concept side.
[1] https://patchwork.freedesktop.org/patch/590653/?series=131296&rev=2
> devm_of_find_backlight() is just a just a no-op and will return NULL.
> NULL is not an error code, so ili9341_dbi_probe()
> won't rage quit.
[...]
> But the several side effect is that the backlight will NOT works at all.
>
s/several/severe
> It is actually considered as fatal bug for *panels* if the backlight of
> it is not light up,
It's fatal error if backlight is not adjustable or not light-up at all.
>
[...]
> Even though the itself panel is refreshing framebuffers,
Even though the panel itself is consuming frame-buffers and displaying.
But if the backlight not work, the screen is extremely dark, we can not
see as well.
Besides the ili9341_dbi_probe() requires additional device properties to
able to work very well. Especially on the rotate screen use case.
next prev parent reply other threads:[~2024-04-25 18:53 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-25 14:26 [PATCH v1 0/3] drm/panel: ili9341: Obvious fixes Andy Shevchenko
2024-04-25 14:26 ` [PATCH v1 1/3] drm/panel: ili9341: Correct use of device property APIs Andy Shevchenko
2024-04-25 16:00 ` Neil Armstrong
2024-04-25 18:08 ` [v1,1/3] " Sui Jingfeng
2024-04-25 18:53 ` Sui Jingfeng [this message]
2024-04-25 19:12 ` Andy Shevchenko
2024-04-25 21:13 ` Sui Jingfeng
2024-04-30 14:13 ` Andy Shevchenko
2024-04-30 16:27 ` Sui Jingfeng
2024-05-02 8:32 ` Andy Shevchenko
2024-05-02 16:25 ` Sui Jingfeng
2024-05-02 17:28 ` Andy Shevchenko
2024-05-03 4:57 ` Sui Jingfeng
2024-05-03 15:46 ` Andy Shevchenko
2024-04-25 19:10 ` Andy Shevchenko
2024-04-25 20:43 ` Sui Jingfeng
2024-04-25 21:27 ` Sui Jingfeng
2024-04-26 6:23 ` Maxime Ripard
2024-04-26 10:11 ` Sui Jingfeng
2024-04-27 5:57 ` Sui Jingfeng
2024-04-29 11:55 ` Maxime Ripard
2024-04-29 16:54 ` Sui Jingfeng
2024-04-30 9:34 ` Maxime Ripard
2024-05-02 7:34 ` Neil Armstrong
2024-05-02 8:33 ` Andy Shevchenko
2024-05-02 17:28 ` Sui Jingfeng
2024-05-02 17:38 ` Andy Shevchenko
2024-04-30 14:32 ` Andy Shevchenko
2024-04-30 17:24 ` Sui Jingfeng
2024-04-30 10:19 ` [PATCH v1 1/3] " Dmitry Baryshkov
2024-04-25 14:26 ` [PATCH v1 2/3] drm/panel: ili9341: Respect deferred probe Andy Shevchenko
2024-04-25 16:00 ` Neil Armstrong
2024-04-30 10:21 ` Dmitry Baryshkov
2024-04-30 16:50 ` [v1,2/3] " Sui Jingfeng
2024-04-25 14:26 ` [PATCH v1 3/3] drm/panel: ili9341: Use predefined error codes Andy Shevchenko
2024-04-25 16:00 ` Neil Armstrong
2024-04-30 16:54 ` [v1,3/3] " Sui Jingfeng
2024-04-25 15:08 ` [PATCH v1 0/3] drm/panel: ili9341: Obvious fixes Andy Shevchenko
2024-05-02 7:43 ` Neil Armstrong
2024-05-02 8:33 ` Andy Shevchenko
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=ce6a480d-80b3-46b0-a32d-26bc6480d02f@linux.dev \
--to=sui.jingfeng@linux.dev \
--cc=airlied@gmail.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maarten.lankhorst@linux.intel.com \
--cc=mripard@kernel.org \
--cc=neil.armstrong@linaro.org \
--cc=quic_jesszhan@quicinc.com \
--cc=rdunlap@infradead.org \
--cc=sam@ravnborg.org \
--cc=tzimmermann@suse.de \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.