Re: [PATCH 2/4] drm/panel: Add driver for Novatek NT37705 panel

["Luca Weiss" <luca.weiss@fairphone.com>]

On Fri May 15, 2026 at 4:36 PM CEST, Thomas Zimmermann wrote:
> Hi
>
> Am 15.05.26 um 16:31 schrieb Luca Weiss:
>> On Fri May 8, 2026 at 10:06 AM CEST, Thomas Zimmermann wrote:
>>> Hi
>>>
>>> Am 01.05.26 um 15:52 schrieb Luca Weiss:
>>>> Add support for the 2484x1116 AMOLED panel from BOE (BJ631JHM-T71-D900)
>>>> bundled with a NT37705 driver IC, as found on the Fairphone (Gen. 6)
>>>> smartphone.
>>>>
>>>> The panel can also be configured in 10-bit (RGB101010) mode, however
>>>> currently it's configured in 8-bit (RGB888) since there's some issues in
>>>> the Qualcomm DPU driver when driving this panel in 10-bit.
>>>>
>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>> ---
>>>>    drivers/gpu/drm/panel/Kconfig                 |  11 +
>>>>    drivers/gpu/drm/panel/Makefile                |   1 +
>>>>    drivers/gpu/drm/panel/panel-novatek-nt37705.c | 413 ++++++++++++++++++++++++++
>>>>    3 files changed, 425 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
>>>> index 979109c27b9b..59ab3f29d8ef 100644
>>>> --- a/drivers/gpu/drm/panel/Kconfig
>>>> +++ b/drivers/gpu/drm/panel/Kconfig
>>>> @@ -624,6 +624,17 @@ config DRM_PANEL_NOVATEK_NT37700F
>>>>    	  Say Y here if you want to enable support for Novatek NT37700F DSI
>>>>    	  panel module. The panel has a resolution of 1080x2160.
>>>>    
>>>> +config DRM_PANEL_NOVATEK_NT37705
>>>> +	tristate "Novatek NT37705-based DSI panel"
>>>> +	depends on OF
>>>> +	depends on DRM_MIPI_DSI
>>>> +	depends on BACKLIGHT_CLASS_DEVICE
>>>> +	select DRM_KMS_HELPER
>>>> +	help
>>>> +	  Say Y here if you want to enable support for Novatek NT37705-based
>>>> +	  display panels, such as the one found in the The Fairphone (Gen. 6)
>>> Duplicate 'the'
>> Marketing really wanted to have it be "The Fairphone". Will change and
>> make it saner.
>
> How about "as the one found in Gen. 6 of The Fairphone." ?

I'll just make it

  display panels, such as the one found in the Fairphone (Gen. 6)
  smartphone.

>>>> +	  smartphone.
>>>> +
>>>>    config DRM_PANEL_NOVATEK_NT37801
>>>>    	tristate "Novatek NT37801/NT37810 AMOLED DSI panel"
>>>>    	depends on OF
>>>> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
>>>> index 0d694acbfbb6..94639bc58ca8 100644
>>>> --- a/drivers/gpu/drm/panel/Makefile
>>>> +++ b/drivers/gpu/drm/panel/Makefile
>>>> @@ -61,6 +61,7 @@ obj-$(CONFIG_DRM_PANEL_NOVATEK_NT36523) += panel-novatek-nt36523.o
>>>>    obj-$(CONFIG_DRM_PANEL_NOVATEK_NT36672A) += panel-novatek-nt36672a.o
>>>>    obj-$(CONFIG_DRM_PANEL_NOVATEK_NT36672E) += panel-novatek-nt36672e.o
>>>>    obj-$(CONFIG_DRM_PANEL_NOVATEK_NT37700F) += panel-novatek-nt37700f.o
>>>> +obj-$(CONFIG_DRM_PANEL_NOVATEK_NT37705) += panel-novatek-nt37705.o
>>>>    obj-$(CONFIG_DRM_PANEL_NOVATEK_NT37801) += panel-novatek-nt37801.o
>>>>    obj-$(CONFIG_DRM_PANEL_NOVATEK_NT39016) += panel-novatek-nt39016.o
>>>>    obj-$(CONFIG_DRM_PANEL_MANTIX_MLAF057WE51) += panel-mantix-mlaf057we51.o
>>>> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt37705.c b/drivers/gpu/drm/panel/panel-novatek-nt37705.c
>>>> new file mode 100644
>>>> index 000000000000..27bd8072ccd1
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/panel/panel-novatek-nt37705.c
>>>> @@ -0,0 +1,413 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Generated with linux-mdss-dsi-panel-driver-generator from vendor device tree.
>>>> + * Copyright (c) 2026 Luca Weiss <luca.weiss@fairphone.com>
>>>> + */
>>>> +
>>>> +#include <linux/backlight.h>
>>>> +#include <linux/delay.h>
>>>> +#include <linux/gpio/consumer.h>
>>>> +#include <linux/mod_devicetable.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +
>>>> +#include <video/mipi_display.h>
>>>> +
>>>> +#include <drm/display/drm_dsc.h>
>>>> +#include <drm/display/drm_dsc_helper.h>
>>> IIRC this requires
>>>
>>>    select DRM_DISPLAY_DSC_HELPER
>>>
>>> in the Kconfig. Maybe double-check.
>> Will check. Always difficult to figure out the proper dependencies in a
>> fully-featured defconfig build.
>>
>>>> +#include <drm/drm_mipi_dsi.h>
>>>> +#include <drm/drm_modes.h>
>>>> +#include <drm/drm_panel.h>
>>>> +#include <drm/drm_probe_helper.h>
>>>> +
>>>> +struct nt37705_panel {
>>>> +	struct drm_panel panel;
>>>> +	struct mipi_dsi_device *dsi;
>>>> +	struct drm_dsc_config dsc;
>>>> +	struct regulator_bulk_data *supplies;
>>>> +	struct gpio_desc *reset_gpio;
>>>> +};
>>>> +
>>>> +static const struct regulator_bulk_data nt37705_supplies[] = {
>>>> +	{ .supply = "vddio" },
>>>> +	{ .supply = "dvdd" },
>>>> +	{ .supply = "vci" },
>>>> +};
>>>> +
>>>> +static inline struct nt37705_panel *to_nt37705_panel(struct drm_panel *panel)
>>>> +{
>>>> +	return container_of_const(panel, struct nt37705_panel, panel);
>>> Either just use container_of or build something that respects the
>>> input's const-ness.
>> I really don't get what you mean here? Why is container_of_const() bad
>> here?
>
> Because nothing is const here. It looks like an oversight or as if 
> something should be const.

I checked and include/linux/container_of.h states:

  Do not use container_of() in new code.

and

  Always prefer container_of_const() instead of container_of() in new code.

So sounds like using container_of_const() is the correct way.

>>>> +
>>>> +	ret = regulator_bulk_enable(ARRAY_SIZE(nt37705_supplies), ctx->supplies);
>>>> +	if (ret < 0) {
>>> Common style is to check for errors with
>>>
>>>     if (ret)
>>>
>>> Here and everywhere else.
>> At least for regulator_bulk_enable() "ret < 0" is actually more popular
>> than just "ret".
>>
>> Kernel doc says "Return: 0 on success or a negative error number on
>> failure." so a positive integer should in theory never happen so they're
>> equivalent.
>>
>> (git grep -h -A2 regulator_bulk_enable | grep if | sed 's|^[ \t]\+||' | sed 's| {$||' | sort | uniq -c)
>
> It's just nitpicking, not a blocker.

I was also curious so I dug around a bit :)

Regards
Luca