Re: [PATCH v2 1/2] dt-bindings: display/panel: Add Lenovo NT36523W BOE panel

Re: [PATCH v2 1/2] dt-bindings: display/panel: Add Lenovo NT36523W BOE panel

[Linus Walleij]

On Tue, Mar 7, 2023 at 2:26 PM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:

> Add bindings for the 2000x1200px IPS panel found on Lenovo Tab P11/
> XiaoXin Pad devices.
>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

(...)
> +$id: http://devicetree.org/schemas/display/panel/lenovo,nt36523w-boe-j606.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NT36523W BOE panel found on Lenovo J606 devices

It's a Novatek NT36523 display controller-based device isn't it?

I would reflect that in the title or at least the description.

> +
> +maintainers:
> +  - Konrad Dybcio <konrad.dybcio@linaro.org>
> +
> +allOf:
> +  - $ref: panel-common.yaml#
> +
> +properties:
> +  compatible:
> +    const: lenovo,nt36523w-boe-j606
> +
> +  reg:
> +    maxItems: 1
> +    description: DSI virtual channel
> +
> +  vddio-supply: true
> +  reset-gpios: true
> +  rotation: true
> +  port: true

This is clearly (as can be seen from the magic in the driver) a
Novatek NT36523 display controller, just configured differently.
https://lore.kernel.org/lkml/20230220121258.10727-1-lujianhua000@gmail.com/T/

Why can't you just modify the existing nt36523 binding from
Jianhua Lu by e.g. making these two non-required:

 - vddpos-supply
 - vddneg-supply

It would not be helpful for driver writers to have two different bindings
for similar hardware hand having to write code to handle different
properties depending on which binding is used, so please unify into
one binding by cooperating with Jianhua.

Would it help if we merged Jianhua's binding so you can build on top?

Yours,
Linus Walleij

Re: [PATCH v2 2/2] gpu/drm/panel: Add Lenovo NT36523W BOE panel

[Linus Walleij]

On Tue, Mar 7, 2023 at 2:26 PM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:

> Introduce support for the BOE panel with a NT36523W touch/driver IC
> found on some Lenovo Tab P11 devices. It's a 2000x1200, 24bit RGB
> MIPI DSI panel with integrated DCS-controlled backlight (that expects
> big-endian communication).
>
> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

I will think this is some variant of the Novatek NT36523 display
controller packaged up with Lenovo electronics until proven how
wrong I am.

I will listen to reason if it can be demonstrated that NT36523 and
NT36523W are considerably different and need very different
drivers, but I seriously doubt it. (For reasons see below.)

>  drivers/gpu/drm/panel/panel-lenovo-nt36523w-boe.c | 747 ++++++++++++++++++++++

We usually share code with different displays using the
same display controller, so panel-novatek-nt36523.c should
be used as name.

> +config DRM_PANEL_LENOVO_NT36523W_BOE
> +       tristate "Lenovo NT36523W BOE panel"

Name it after the display controller like the other examples
in the Kconfig, DRM_PANEL_NOVATEK_NT36523

> +       mipi_dsi_dcs_write_seq(dsi, 0xff, 0x20);
> +       mipi_dsi_dcs_write_seq(dsi, 0xfb, 0x01);
> +       mipi_dsi_dcs_write_seq(dsi, 0x05, 0xd9);
> +       mipi_dsi_dcs_write_seq(dsi, 0x07, 0x78);
> +       mipi_dsi_dcs_write_seq(dsi, 0x08, 0x5a);
> +       mipi_dsi_dcs_write_seq(dsi, 0x0d, 0x63);
> +       mipi_dsi_dcs_write_seq(dsi, 0x0e, 0x91);
> +       mipi_dsi_dcs_write_seq(dsi, 0x0f, 0x73);
> +       mipi_dsi_dcs_write_seq(dsi, 0x95, 0xeb);
> +       mipi_dsi_dcs_write_seq(dsi, 0x96, 0xeb);
> +       mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SET_PARTIAL_ROWS, 0x11);

I think it looks very similar to Jianhua:s driver:
https://lore.kernel.org/lkml/20230220121258.10727-1-lujianhua000@gmail.com/T/

Can't you just add this special magic sequence into
that driver instead?

Would it help if we merge Jianhua's driver first so you can patch on
top of it?

Yours,
Linus Walleij

Re: [PATCH v2 1/2] dt-bindings: display/panel: Add Lenovo NT36523W BOE panel

[Konrad Dybcio]

On 7.03.2023 23:08, Linus Walleij wrote:
> On Tue, Mar 7, 2023 at 2:26 PM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> 
>> Add bindings for the 2000x1200px IPS panel found on Lenovo Tab P11/
>> XiaoXin Pad devices.
>>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> 
> (...)
>> +$id: http://devicetree.org/schemas/display/panel/lenovo,nt36523w-boe-j606.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: NT36523W BOE panel found on Lenovo J606 devices
> 
> It's a Novatek NT36523 display controller-based device isn't it?
> 
> I would reflect that in the title or at least the description.
> 
>> +
>> +maintainers:
>> +  - Konrad Dybcio <konrad.dybcio@linaro.org>
>> +
>> +allOf:
>> +  - $ref: panel-common.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    const: lenovo,nt36523w-boe-j606
>> +
>> +  reg:
>> +    maxItems: 1
>> +    description: DSI virtual channel
>> +
>> +  vddio-supply: true
>> +  reset-gpios: true
>> +  rotation: true
>> +  port: true
> 
> This is clearly (as can be seen from the magic in the driver) a
> Novatek NT36523 display controller, just configured differently.
> https://lore.kernel.org/lkml/20230220121258.10727-1-lujianhua000@gmail.com/T/
> 
> Why can't you just modify the existing nt36523 binding from
> Jianhua Lu by e.g. making these two non-required:
> 
>  - vddpos-supply
>  - vddneg-supply
> 
> It would not be helpful for driver writers to have two different bindings
> for similar hardware hand having to write code to handle different
> properties depending on which binding is used, so please unify into
> one binding by cooperating with Jianhua.
I'll look into Jianhua's patchset and try to work atop that!

> 
> Would it help if we merged Jianhua's binding so you can build on topYes please, the less out-of-tree dependencies the better..

> 
> Yours,
> Linus Walleij

Re: [PATCH v2 2/2] gpu/drm/panel: Add Lenovo NT36523W BOE panel

[Konrad Dybcio]

On 7.03.2023 23:18, Linus Walleij wrote:
> On Tue, Mar 7, 2023 at 2:26 PM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> 
>> Introduce support for the BOE panel with a NT36523W touch/driver IC
>> found on some Lenovo Tab P11 devices. It's a 2000x1200, 24bit RGB
>> MIPI DSI panel with integrated DCS-controlled backlight (that expects
>> big-endian communication).
>>
>> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> 
> I will think this is some variant of the Novatek NT36523 display
> controller packaged up with Lenovo electronics until proven how
> wrong I am.
> 
> I will listen to reason if it can be demonstrated that NT36523 and
> NT36523W are considerably different and need very different
> drivers, but I seriously doubt it. (For reasons see below.)
> 
>>  drivers/gpu/drm/panel/panel-lenovo-nt36523w-boe.c | 747 ++++++++++++++++++++++
> 
> We usually share code with different displays using the
> same display controller, so panel-novatek-nt36523.c should
> be used as name.
> 
>> +config DRM_PANEL_LENOVO_NT36523W_BOE
>> +       tristate "Lenovo NT36523W BOE panel"
> 
> Name it after the display controller like the other examples
> in the Kconfig, DRM_PANEL_NOVATEK_NT36523
> 
>> +       mipi_dsi_dcs_write_seq(dsi, 0xff, 0x20);
>> +       mipi_dsi_dcs_write_seq(dsi, 0xfb, 0x01);
>> +       mipi_dsi_dcs_write_seq(dsi, 0x05, 0xd9);
>> +       mipi_dsi_dcs_write_seq(dsi, 0x07, 0x78);
>> +       mipi_dsi_dcs_write_seq(dsi, 0x08, 0x5a);
>> +       mipi_dsi_dcs_write_seq(dsi, 0x0d, 0x63);
>> +       mipi_dsi_dcs_write_seq(dsi, 0x0e, 0x91);
>> +       mipi_dsi_dcs_write_seq(dsi, 0x0f, 0x73);
>> +       mipi_dsi_dcs_write_seq(dsi, 0x95, 0xeb);
>> +       mipi_dsi_dcs_write_seq(dsi, 0x96, 0xeb);
>> +       mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SET_PARTIAL_ROWS, 0x11);
> 
> I think it looks very similar to Jianhua:s driver:
> https://lore.kernel.org/lkml/20230220121258.10727-1-lujianhua000@gmail.com/T/
> 
> Can't you just add this special magic sequence into
> that driver instead?
Yeah I'll try doing that.

> 
> Would it help if we merge Jianhua's driver first so you can patch on
> top of it?
Definitely.

Konrad
> 
> Yours,
> Linus Walleij

Re: [PATCH v2 1/2] dt-bindings: display/panel: Add Lenovo NT36523W BOE panel

[Linus Walleij]

On Wed, Mar 8, 2023 at 12:02 PM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:

> > It would not be helpful for driver writers to have two different bindings
> > for similar hardware hand having to write code to handle different
> > properties depending on which binding is used, so please unify into
> > one binding by cooperating with Jianhua.
> I'll look into Jianhua's patchset and try to work atop that!

Jianhua's driver is merged to drm-misc-next so you can make
your addendum now!
https://cgit.freedesktop.org/drm/drm-misc/commit/?id=c61093b56a2ff15e449e8af56e96dc5a312baf25
https://cgit.freedesktop.org/drm/drm-misc/commit/?id=0993234a00451e0a5c3e47d8b0f2e01dac6cedbf

Yours,
Linus Walleij