Messages in this thread | | | Date | Fri, 26 Apr 2024 02:08:16 +0800 | Subject | Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs | From | Sui Jingfeng <> |
| |
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 <andriy.shevchenko@linux.intel.com> > Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org> > --- > 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
| |