It seems driver missed the point of proper use of device property APIs.
Correct this by updating headers and calls respectively.
Fixes: 5a04227326b0 ("drm/panel: Add ilitek ili9341 panel driver")
Signed-off-by: Andy Shevchenko <[email protected]>
---
drivers/gpu/drm/panel/Kconfig | 2 +-
drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 5 +++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index e54f6f5604ed..2d4515555820 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -177,7 +177,7 @@ config DRM_PANEL_ILITEK_IL9322
config DRM_PANEL_ILITEK_ILI9341
tristate "Ilitek ILI9341 240x320 QVGA panels"
- depends on OF && SPI
+ depends on SPI
select DRM_KMS_HELPER
select DRM_GEM_DMA_HELPER
depends on BACKLIGHT_CLASS_DEVICE
diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
index 3574681891e8..7584ddb0e441 100644
--- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
@@ -22,8 +22,9 @@
#include <linux/bitops.h>
#include <linux/delay.h>
#include <linux/gpio/consumer.h>
+#include <linux/mod_devicetable.h>
#include <linux/module.h>
-#include <linux/of.h>
+#include <linux/property.h>
#include <linux/regulator/consumer.h>
#include <linux/spi/spi.h>
@@ -691,7 +692,7 @@ static int ili9341_dpi_probe(struct spi_device *spi, struct gpio_desc *dc,
* Every new incarnation of this display must have a unique
* data entry for the system in this driver.
*/
- ili->conf = of_device_get_match_data(dev);
+ ili->conf = device_get_match_data(dev);
if (!ili->conf) {
dev_err(dev, "missing device configuration\n");
return -ENODEV;
--
2.43.0.rc1.1336.g36b5255a03ac
On 25/04/2024 16: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.
>
> Fixes: 5a04227326b0 ("drm/panel: Add ilitek ili9341 panel driver")
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/gpu/drm/panel/Kconfig | 2 +-
> drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 5 +++--
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index e54f6f5604ed..2d4515555820 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -177,7 +177,7 @@ config DRM_PANEL_ILITEK_IL9322
>
> config DRM_PANEL_ILITEK_ILI9341
> tristate "Ilitek ILI9341 240x320 QVGA panels"
> - depends on OF && SPI
> + depends on SPI
> select DRM_KMS_HELPER
> select DRM_GEM_DMA_HELPER
> depends on BACKLIGHT_CLASS_DEVICE
> diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
> index 3574681891e8..7584ddb0e441 100644
> --- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
> +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
> @@ -22,8 +22,9 @@
> #include <linux/bitops.h>
> #include <linux/delay.h>
> #include <linux/gpio/consumer.h>
> +#include <linux/mod_devicetable.h>
> #include <linux/module.h>
> -#include <linux/of.h>
> +#include <linux/property.h>
> #include <linux/regulator/consumer.h>
> #include <linux/spi/spi.h>
>
> @@ -691,7 +692,7 @@ static int ili9341_dpi_probe(struct spi_device *spi, struct gpio_desc *dc,
> * Every new incarnation of this display must have a unique
> * data entry for the system in this driver.
> */
> - ili->conf = of_device_get_match_data(dev);
> + ili->conf = device_get_match_data(dev);
> if (!ili->conf) {
> dev_err(dev, "missing device configuration\n");
> return -ENODEV;
Reviewed-by: Neil Armstrong <[email protected]>
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.
Please allow me to tell you the truth: This patch again has ZERO effect.
It fix nothing. And this patch is has the risks to be wrong.
Simple because the "ili9341_probe() ---> ili9341_dbi_prob()" code path
is DT dependent.
First of all, the devm_of_find_backlight() is called in ili9341_dbi_probe()
under *non-DT* environment, 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.
It is actually considered as fatal bug for *panels* if the backlight of
it is not light up, at least the brightness of *won't* be able to adjust.
What's worse, if there is no sane platform setup code at the firmware
or boot loader stage to set a proper initial state. The screen is complete
dark. Even though the itself panel is refreshing framebuffers, it can not
be seen by human's eye. Simple because of no backlight.
Second, the ili9341_dbi_probe() requires additional device properties to
be able to works very well on the rotation screen case. See the calling
of "device_property_read_u32(dev, "rotation", &rotation)" in
ili9341_dbi_probe() function.
Combine with those two factors, it is actually can conclude that the
panel-ilitek-ili9394 driver has the *implicit* dependency on 'OF'.
Removing the 'OF' dependency from its Kconfig just trigger the
leakage of such risks.
My software node related patches can help to reduce part of the potential
risks, but it still need some extra work. And it is not landed yet.
> Fixes: 5a04227326b0 ("drm/panel: Add ilitek ili9341 panel driver")
> Signed-off-by: Andy Shevchenko <[email protected]>
> Reviewed-by: Neil Armstrong <[email protected]>
> ---
> drivers/gpu/drm/panel/Kconfig | 2 +-
> drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 5 +++--
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index e54f6f5604ed..2d4515555820 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -177,7 +177,7 @@ config DRM_PANEL_ILITEK_IL9322
>
> config DRM_PANEL_ILITEK_ILI9341
> tristate "Ilitek ILI9341 240x320 QVGA panels"
> - depends on OF && SPI
> + depends on SPI
> select DRM_KMS_HELPER
> select DRM_GEM_DMA_HELPER
> depends on BACKLIGHT_CLASS_DEVICE
> diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
> index 3574681891e8..7584ddb0e441 100644
> --- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
> +++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
> @@ -22,8 +22,9 @@
> #include <linux/bitops.h>
> #include <linux/delay.h>
> #include <linux/gpio/consumer.h>
> +#include <linux/mod_devicetable.h>
> #include <linux/module.h>
> -#include <linux/of.h>
> +#include <linux/property.h>
> #include <linux/regulator/consumer.h>
> #include <linux/spi/spi.h>
>
> @@ -691,7 +692,7 @@ static int ili9341_dpi_probe(struct spi_device *spi, struct gpio_desc *dc,
> * Every new incarnation of this display must have a unique
> * data entry for the system in this driver.
> */
> - ili->conf = of_device_get_match_data(dev);
> + ili->conf = device_get_match_data(dev);
> if (!ili->conf) {
> dev_err(dev, "missing device configuration\n");
> return -ENODEV;
--
Best regards,
Sui
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.
On Fri, Apr 26, 2024 at 02:53:22AM +0800, Sui Jingfeng wrote:
> On 2024/4/26 02:08, Sui Jingfeng wrote:
Are you speaking to yourself? I'm totally lost.
Please, if you want to give a constructive feedback, try to understand
the topic from different aspects and then clearly express it.
--
With Best Regards,
Andy Shevchenko
Hi,
On 2024/4/26 03:12, Andy Shevchenko wrote:
> On Fri, Apr 26, 2024 at 02:53:22AM +0800, Sui Jingfeng wrote:
>> On 2024/4/26 02:08, Sui Jingfeng wrote:
> Are you speaking to yourself? I'm totally lost.
>
> Please, if you want to give a constructive feedback, try to understand
> the topic from different aspects and then clearly express it.
>
OK,
The previous email analysis the non-DT cases exhaustively, this email intend to
demonstrate the more frequently use case.
That is, in the *DT('OF')* based systems,
device_get_match_data() is completely equivalent to
of_device_get_match_data().
So the net results of applying this patch are "no gains and no lost".
Things will become clear if we divide the whole problem into two cases(DT and non-DT)
to discuss, that's it. That's all I can tell.
Sorry about my broken written and sorry for the noise. Thanks for your education in
the past.
--
Best regards,
Sui
On Fri, Apr 26, 2024 at 02:08:16AM +0800, Sui Jingfeng wrote:
> 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.
>
> Please allow me to tell you the truth: This patch again has ZERO effect.
> It fix nothing. And this patch is has the risks to be wrong.
Huh?! Really, stop commenting the stuff you do not understand.
> Simple because the "ili9341_probe() ---> ili9341_dbi_prob()" code path
> is DT dependent.
>
> First of all, the devm_of_find_backlight() is called in ili9341_dbi_probe()
> under *non-DT* environment, 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.
Is it a problem?
> It is actually considered as fatal bug for *panels* if the backlight of
> it is not light up, at least the brightness of *won't* be able to adjust.
> What's worse, if there is no sane platform setup code at the firmware
> or boot loader stage to set a proper initial state. The screen is complete
> dark. Even though the itself panel is refreshing framebuffers, it can not
> be seen by human's eye. Simple because of no backlight.
Can you imagine that I may have different hardware that considered
this is non-fatal error?
> Second, the ili9341_dbi_probe() requires additional device properties to
> be able to works very well on the rotation screen case. See the calling
> of "device_property_read_u32(dev, "rotation", &rotation)" in
> ili9341_dbi_probe() function.
Yes, exactly, and how does it object the purpose of this patch?
> Combine with those two factors, it is actually can conclude that the
> panel-ilitek-ili9394 driver has the *implicit* dependency on 'OF'.
> Removing the 'OF' dependency from its Kconfig just trigger the
> leakage of such risks.
What?!
> My software node related patches can help to reduce part of the potential
> risks, but it still need some extra work. And it is not landed yet.
Your patch has nothing to do with this series.
--
With Best Regards,
Andy Shevchenko
On Thu, Apr 25, 2024 at 05:26:17PM +0300, Andy Shevchenko wrote:
> It seems driver missed the point of proper use of device property APIs.
> Correct this by updating headers and calls respectively.
>
> Fixes: 5a04227326b0 ("drm/panel: Add ilitek ili9341 panel driver")
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> drivers/gpu/drm/panel/Kconfig | 2 +-
> drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 5 +++--
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On Fri, Apr 26, 2024 at 05:13:43AM +0800, Sui Jingfeng wrote:
> On 2024/4/26 03:12, Andy Shevchenko wrote:
> > On Fri, Apr 26, 2024 at 02:53:22AM +0800, Sui Jingfeng wrote:
> > > On 2024/4/26 02:08, Sui Jingfeng wrote:
..
> > Are you speaking to yourself? I'm totally lost.
> >
> > Please, if you want to give a constructive feedback, try to understand
> > the topic from different aspects and then clearly express it.
>
> OK,
>
> The previous email analysis the non-DT cases exhaustively, this email intend to
> demonstrate the more frequently use case.
>
> That is, in the *DT('OF')* based systems,
> device_get_match_data() is completely equivalent to
> of_device_get_match_data().
> So the net results of applying this patch are "no gains and no lost".
This is not true. It's only part of the cases, i.e. DT. So, I assume you meant
"So the net results of applying this patch are "no gains and no lost" in DT case".
> Things will become clear if we divide the whole problem into two cases(DT and non-DT)
> to discuss, that's it. That's all I can tell.
Not really. non-DT cases can also be divided to "fwnode backed or not", and
the former might be subdivided to "is it swnode backed or real fwnode one?"
--
With Best Regards,
Andy Shevchenko
On 2024/4/30 22:13, Andy Shevchenko wrote:
> On Fri, Apr 26, 2024 at 05:13:43AM +0800, Sui Jingfeng wrote:
>> On 2024/4/26 03:12, Andy Shevchenko wrote:
>>> On Fri, Apr 26, 2024 at 02:53:22AM +0800, Sui Jingfeng wrote:
>>>> On 2024/4/26 02:08, Sui Jingfeng wrote:
> ...
>
>>> Are you speaking to yourself? I'm totally lost.
>>>
>>> Please, if you want to give a constructive feedback, try to understand
>>> the topic from different aspects and then clearly express it.
>> OK,
>>
>> The previous email analysis the non-DT cases exhaustively, this email intend to
>> demonstrate the more frequently use case.
>>
>> That is, in the *DT('OF')* based systems,
>> device_get_match_data() is completely equivalent to
>> of_device_get_match_data().
>> So the net results of applying this patch are "no gains and no lost".
> This is not true.
Yes, I'm true.
I have mentionedin previous(earlier) paragraph with "in the DT(OF) based systems" or similar words.
> It's only part of the cases, i.e. DT. So, I assume you meant
>
> "So the net results of applying this patch are "no gains and no lost" in DT case".
Yeah,it is true that this patch are "no gains and no lost" in DT case.
>> Things will become clear if we divide the whole problem into two cases(DT and non-DT)
>> to discuss, that's it. That's all I can tell.
> Not really.
I'm very clear before our conversation, really!
> non-DT cases can also be divided to "fwnode backed or not",
For non fwnode backed case of non-DT cases, there is not decent way to acquire
large set of device properties. And is out of the scope of "Correct use of
device property APIs".
> and
> the former might be subdivided to "is it swnode backed or real fwnode one?"
>
Yeah,
On non-DT cases, it can be subdivided to swnode backed case and ACPI fwnode backed case.
- For swnode backed case: the device_get_match_data() don't has a implemented backend.
- For ACPI fwnode backed case: the device_get_match_data() has a implemented backend.
But the driver has *neither* software node support nor ACPI support, so that the rotation
property can not get and ili9341_dpi_probe() will fails. So in total, this is not a 100%
correct use of device property APIs.
But I'm fine that if you want to leave(ignore) those less frequent use cases temporarily,
there may have programmers want to post patches, to complete the missing in the future.
So, there do have some gains on non-DT cases.
- As you make it be able to compiled on X86 with the drm-misc-defconfig.
- You cleanup the code up (at least patch 2 in this series is no obvious problem).
- You allow people to modprobe it, and maybe half right and half undefined.
But you do helps moving something forward, so congratulations for the wake up.
--
Best regards,
Sui
On Wed, May 01, 2024 at 12:27:14AM +0800, Sui Jingfeng wrote:
> On 2024/4/30 22:13, Andy Shevchenko wrote:
> > On Fri, Apr 26, 2024 at 05:13:43AM +0800, Sui Jingfeng wrote:
..
> > the former might be subdivided to "is it swnode backed or real fwnode one?"
> >
> Yeah,
> On non-DT cases, it can be subdivided to swnode backed case and ACPI fwnode backed case.
>
> - For swnode backed case: the device_get_match_data() don't has a implemented backend.
> - For ACPI fwnode backed case: the device_get_match_data() has a implemented backend.
>
> But the driver has *neither* software node support
True.
> nor ACPI support,
Not true.
So, slow down and take your time to get into the code and understand how it works.
> so that the rotation property can not get and ili9341_dpi_probe() will fails.
> So in total, this is not a 100% correct use of device property APIs.
>
> But I'm fine that if you want to leave(ignore) those less frequent use cases temporarily,
> there may have programmers want to post patches, to complete the missing in the future.
>
> So, there do have some gains on non-DT cases.
>
> - As you make it be able to compiled on X86 with the drm-misc-defconfig.
> - You cleanup the code up (at least patch 2 in this series is no obvious problem).
> - You allow people to modprobe it, and maybe half right and half undefined.
>
> But you do helps moving something forward, so congratulations for the wake up.
--
With Best Regards,
Andy Shevchenko
Hi,
On 2024/5/2 16:32, Andy Shevchenko wrote:
> On Wed, May 01, 2024 at 12:27:14AM +0800, Sui Jingfeng wrote:
>> On 2024/4/30 22:13, Andy Shevchenko wrote:
>>> On Fri, Apr 26, 2024 at 05:13:43AM +0800, Sui Jingfeng wrote:
> ...
>
>>> the former might be subdivided to "is it swnode backed or real fwnode one?"
>>>
>> Yeah,
>> On non-DT cases, it can be subdivided to swnode backed case and ACPI fwnode backed case.
>>
>> - For swnode backed case: the device_get_match_data() don't has a implemented backend.
>> - For ACPI fwnode backed case: the device_get_match_data() has a implemented backend.
>>
>> But the driver has *neither* software node support
> True.
>
>> nor ACPI support,
> Not true.
Why this is not true? Are you means that the panel-ilitek-ili9341 driver has ACPI support?
I'm asking because I don't see struct acpi_device_id related stuff in that source file,
am I miss something?
> So, slow down and take your time to get into the code and understand how it works.
>
>> so that the rotation property can not get and ili9341_dpi_probe() will fails.
>> So in total, this is not a 100% correct use of device property APIs.
>>
>> But I'm fine that if you want to leave(ignore) those less frequent use cases temporarily,
>> there may have programmers want to post patches, to complete the missing in the future.
>>
>> So, there do have some gains on non-DT cases.
>>
>> - As you make it be able to compiled on X86 with the drm-misc-defconfig.
>> - You cleanup the code up (at least patch 2 in this series is no obvious problem).
>> - You allow people to modprobe it, and maybe half right and half undefined.
>>
>> But you do helps moving something forward, so congratulations for the wake up.
--
Best regards,
Sui
On Fri, May 03, 2024 at 12:25:17AM +0800, Sui Jingfeng wrote:
> On 2024/5/2 16:32, Andy Shevchenko wrote:
> > On Wed, May 01, 2024 at 12:27:14AM +0800, Sui Jingfeng wrote:
> > > On 2024/4/30 22:13, Andy Shevchenko wrote:
> > > > On Fri, Apr 26, 2024 at 05:13:43AM +0800, Sui Jingfeng wrote:
..
> > > > the former might be subdivided to "is it swnode backed or real fwnode one?"
> > > >
> > > Yeah,
> > > On non-DT cases, it can be subdivided to swnode backed case and ACPI fwnode backed case.
> > >
> > > - For swnode backed case: the device_get_match_data() don't has a implemented backend.
> > > - For ACPI fwnode backed case: the device_get_match_data() has a implemented backend.
> > >
> > > But the driver has *neither* software node support
> > True.
> >
> > > nor ACPI support,
> > Not true.
>
> Why this is not true? Are you means that the panel-ilitek-ili9341 driver has ACPI support?
That's the idea (as far as I see the copy of the code from tinyDRM),
but broken over the copy'n'paste. This patch rectifies that to be
in align with the original code, which *does* support ACPI.
> I'm asking because I don't see struct acpi_device_id related stuff in that source file,
> am I miss something?
Yes, you are. I leave it for you to research.
> > So, slow down and take your time to get into the code and understand how it works.
> >
> > > so that the rotation property can not get and ili9341_dpi_probe() will fails.
> > > So in total, this is not a 100% correct use of device property APIs.
> > >
> > > But I'm fine that if you want to leave(ignore) those less frequent use cases temporarily,
> > > there may have programmers want to post patches, to complete the missing in the future.
> > >
> > > So, there do have some gains on non-DT cases.
> > >
> > > - As you make it be able to compiled on X86 with the drm-misc-defconfig.
> > > - You cleanup the code up (at least patch 2 in this series is no obvious problem).
> > > - You allow people to modprobe it, and maybe half right and half undefined.
> > >
> > > But you do helps moving something forward, so congratulations for the wake up.
--
With Best Regards,
Andy Shevchenko
Hi,
On 2024/5/3 01:28, Andy Shevchenko wrote:
> On Fri, May 03, 2024 at 12:25:17AM +0800, Sui Jingfeng wrote:
>> On 2024/5/2 16:32, Andy Shevchenko wrote:
>>> On Wed, May 01, 2024 at 12:27:14AM +0800, Sui Jingfeng wrote:
>>>> On 2024/4/30 22:13, Andy Shevchenko wrote:
>>>>> On Fri, Apr 26, 2024 at 05:13:43AM +0800, Sui Jingfeng wrote:
> ...
>
>>>>> the former might be subdivided to "is it swnode backed or real fwnode one?"
>>>>>
>>>> Yeah,
>>>> On non-DT cases, it can be subdivided to swnode backed case and ACPI fwnode backed case.
>>>>
>>>> - For swnode backed case: the device_get_match_data() don't has a implemented backend.
>>>> - For ACPI fwnode backed case: the device_get_match_data() has a implemented backend.
>>>>
>>>> But the driver has *neither* software node support
>>> True.
>>>
>>>> nor ACPI support,
>>> Not true.
>> Why this is not true? Are you means that the panel-ilitek-ili9341 driver has ACPI support?
> That's the idea (as far as I see the copy of the code from tinyDRM),
> but broken over the copy'n'paste. This patch rectifies that to be
> in align with the original code, which *does* support ACPI.
>
>> I'm asking because I don't see struct acpi_device_id related stuff in that source file,
>> am I miss something?
> Yes, you are. I leave it for you to research.
>
After researching a few hours I still don't understand how does
the panel-ilitek-ili9341 driver has the ACPI support and be able
to ACPI probed when compiled as module.
As far as I know, drivers that has the ACPI support *must* has the
acpi_match_table hooked, so that be able to be probed when the
driver is compiled as a module.
For example, see commit 75a1b44a54bd9 ("spi: tegra210-quad: add acpi support")
to get a feel what a SPI device with *real* ACPI support looks like.
I have double checked the panel-ilitek-ili9341 driver, it doesn't
has acpi_match_table hooked, which means that this driver won't
even be able probed. And probed as pure SPI device still out of
the scope of "correct use of device property APIs". Because SPI
device specific method don't belong to the device property API.
I don't really know what's we are missing, but we already intend
to let it go, thanks.
>>> So, slow down and take your time to get into the code and understand how it works.
>>>
>>>> so that the rotation property can not get and ili9341_dpi_probe() will fails.
>>>> So in total, this is not a 100% correct use of device property APIs.
>>>>
>>>> But I'm fine that if you want to leave(ignore) those less frequent use cases temporarily,
>>>> there may have programmers want to post patches, to complete the missing in the future.
>>>>
>>>> So, there do have some gains on non-DT cases.
>>>>
>>>> - As you make it be able to compiled on X86 with the drm-misc-defconfig.
>>>> - You cleanup the code up (at least patch 2 in this series is no obvious problem).
>>>> - You allow people to modprobe it, and maybe half right and half undefined.
>>>>
>>>> But you do helps moving something forward, so congratulations for the wake up.
--
Best regards,
Sui
On Fri, May 03, 2024 at 12:57:33PM +0800, Sui Jingfeng wrote:
> On 2024/5/3 01:28, Andy Shevchenko wrote:
> > On Fri, May 03, 2024 at 12:25:17AM +0800, Sui Jingfeng wrote:
> > > On 2024/5/2 16:32, Andy Shevchenko wrote:
> > > > On Wed, May 01, 2024 at 12:27:14AM +0800, Sui Jingfeng wrote:
> > > > > On 2024/4/30 22:13, Andy Shevchenko wrote:
> > > > > > On Fri, Apr 26, 2024 at 05:13:43AM +0800, Sui Jingfeng wrote:
..
> > > > > > the former might be subdivided to "is it swnode backed or real fwnode one?"
> > > > > >
> > > > > Yeah,
> > > > > On non-DT cases, it can be subdivided to swnode backed case and ACPI fwnode backed case.
> > > > >
> > > > > - For swnode backed case: the device_get_match_data() don't has a implemented backend.
> > > > > - For ACPI fwnode backed case: the device_get_match_data() has a implemented backend.
> > > > >
> > > > > But the driver has *neither* software node support
> > > > True.
> > > >
> > > > > nor ACPI support,
> > > > Not true.
> > > Why this is not true? Are you means that the panel-ilitek-ili9341 driver has ACPI support?
> > That's the idea (as far as I see the copy of the code from tinyDRM),
> > but broken over the copy'n'paste. This patch rectifies that to be
> > in align with the original code, which *does* support ACPI.
> >
> > > I'm asking because I don't see struct acpi_device_id related stuff in that source file,
> > > am I miss something?
> > Yes, you are. I leave it for you to research.
>
> After researching a few hours I still don't understand how does
> the panel-ilitek-ili9341 driver has the ACPI support and be able
> to ACPI probed when compiled as module.
>
> As far as I know, drivers that has the ACPI support *must* has the
> .acpi_match_table hooked, so that be able to be probed when the
> driver is compiled as a module.
No, and this is the thing. Hint: there is a glue code to reuse compatible
strings from OF, that's why dependency to OF prevents *some* systems from
being able to use that. But it's easy to fix by enabling OF in the
configuration, however the ID tables are orthogonal to the environment.
That's why all those ACPI_PTR() and of_match_ptr() are design mistakes
that are going to be removed eventually (the work is ongoing, btw,
as well as killing specific *_device_get_match_data() calls).
> For example, see commit 75a1b44a54bd9 ("spi: tegra210-quad: add acpi support")
> to get a feel what a SPI device with *real* ACPI support looks like.
If under *real* you assume the allocated _HID in use, yes, that's how it's
done. But there is the other tricky way of achieving similar effect w/o
allocating a new / custom ACPI _HID.
Hint: it's all documented in kernel under firmware-guide/acpi/.
> I have double checked the panel-ilitek-ili9341 driver, it doesn't
> has acpi_match_table hooked, which means that this driver won't
> even be able probed. And probed as pure SPI device still out of
> the scope of "correct use of device property APIs". Because SPI
> device specific method don't belong to the device property API.
> I don't really know what's we are missing, but we already intend
> to let it go, thanks.
>
> > > > So, slow down and take your time to get into the code and understand how it works.
> > > >
> > > > > so that the rotation property can not get and ili9341_dpi_probe() will fails.
> > > > > So in total, this is not a 100% correct use of device property APIs.
> > > > >
> > > > > But I'm fine that if you want to leave(ignore) those less frequent use cases temporarily,
> > > > > there may have programmers want to post patches, to complete the missing in the future.
> > > > >
> > > > > So, there do have some gains on non-DT cases.
> > > > >
> > > > > - As you make it be able to compiled on X86 with the drm-misc-defconfig.
> > > > > - You cleanup the code up (at least patch 2 in this series is no obvious problem).
> > > > > - You allow people to modprobe it, and maybe half right and half undefined.
> > > > >
> > > > > But you do helps moving something forward, so congratulations for the wake up.
--
With Best Regards,
Andy Shevchenko