* [PATCH v1 0/2] Add kd101ne3 bindings and driver. @ 2024-04-18 8:15 lvzhaoxiong 2024-04-18 8:15 ` [PATCH v1 1/2] dt-bindings: display: panel: Add KD101NE3-40TI support lvzhaoxiong 2024-04-18 8:15 ` [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver lvzhaoxiong 0 siblings, 2 replies; 19+ messages in thread From: lvzhaoxiong @ 2024-04-18 8:15 UTC (permalink / raw To: mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt, dianders, hsinyi Cc: dri-devel, devicetree, linux-kernel, lvzhaoxiong Add bindings and driver for kd101ne3. lvzhaoxiong (2): dt-bindings: display: panel: Add KD101NE3-40TI support drm/panel: kd101ne3: add new panel driver .../panel/kingdisplay,kd101ne3-40ti.yaml | 63 ++ drivers/gpu/drm/panel/Kconfig | 9 + drivers/gpu/drm/panel/Makefile | 1 + .../drm/panel/panel-kingdisplay-kd101ne3.c | 607 ++++++++++++++++++ 4 files changed, 680 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/panel/kingdisplay,kd101ne3-40ti.yaml create mode 100644 drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c -- 2.17.1 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v1 1/2] dt-bindings: display: panel: Add KD101NE3-40TI support 2024-04-18 8:15 [PATCH v1 0/2] Add kd101ne3 bindings and driver lvzhaoxiong @ 2024-04-18 8:15 ` lvzhaoxiong 2024-04-18 22:59 ` Krzysztof Kozlowski 2024-04-18 8:15 ` [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver lvzhaoxiong 1 sibling, 1 reply; 19+ messages in thread From: lvzhaoxiong @ 2024-04-18 8:15 UTC (permalink / raw To: mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt, dianders, hsinyi Cc: dri-devel, devicetree, linux-kernel, lvzhaoxiong Create a new dt-scheam for the kd101ne3-40ti. Signed-off-by: lvzhaoxiong <lvzhaoxiong@huaqin.corp-partner.google.com> --- .../panel/kingdisplay,kd101ne3-40ti.yaml | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/panel/kingdisplay,kd101ne3-40ti.yaml diff --git a/Documentation/devicetree/bindings/display/panel/kingdisplay,kd101ne3-40ti.yaml b/Documentation/devicetree/bindings/display/panel/kingdisplay,kd101ne3-40ti.yaml new file mode 100644 index 000000000000..dc79a49eea3b --- /dev/null +++ b/Documentation/devicetree/bindings/display/panel/kingdisplay,kd101ne3-40ti.yaml @@ -0,0 +1,63 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/panel/kingdisplay,kd101ne3-40ti.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: King Display KD035G6-40TI based MIPI-DSI panels + +description: | + -This binding is for display panels using an JD9365DA controller + +maintainers: + - Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com> + +allOf: + - $ref: panel-common.yaml# + +properties: + compatible: + const: kingdisplay,kd101ne3-40ti + + backlight: true + port: true + pp3300-supply: true + reg: true + enable-gpios: true + rotation: true + +required: + - compatible + - reg + - enable-gpios + - pp3300-supply + - backlight + - port + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + dsi { + #address-cells = <1>; + #size-cells = <0>; + panel: panel@0 { + compatible = "kingdisplay,kd101ne3-40ti"; + reg = <0>; + enable-gpios = <&pio 98 0>; + pinctrl-names = "default"; + pinctrl-0 = <&panel_pins_default>; + pp3300-supply = <&en_pp6000_mipi_disp>; + backlight = <&backlight_lcd0>; + rotation = <90>; + port { + panel_in: endpoint { + remote-endpoint = <&dsi_out>; + }; + }; + }; + }; + +... -- 2.17.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v1 1/2] dt-bindings: display: panel: Add KD101NE3-40TI support 2024-04-18 8:15 ` [PATCH v1 1/2] dt-bindings: display: panel: Add KD101NE3-40TI support lvzhaoxiong @ 2024-04-18 22:59 ` Krzysztof Kozlowski 0 siblings, 0 replies; 19+ messages in thread From: Krzysztof Kozlowski @ 2024-04-18 22:59 UTC (permalink / raw To: lvzhaoxiong, mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt, dianders, hsinyi Cc: dri-devel, devicetree, linux-kernel On 18/04/2024 10:15, lvzhaoxiong wrote: > Create a new dt-scheam for the kd101ne3-40ti. There is another thread like this, which is confusing. Please version your patches. patman solves it. b4 as well. Read the guidelines provided to you by Google. > > Signed-off-by: lvzhaoxiong <lvzhaoxiong@huaqin.corp-partner.google.com> Same comment as for all your other patches: please use full name. > --- > .../panel/kingdisplay,kd101ne3-40ti.yaml | 63 +++++++++++++++++++ > 1 file changed, 63 insertions(+) > create mode 100644 Documentation/devicetree/bindings/display/panel/kingdisplay,kd101ne3-40ti.yaml > > diff --git a/Documentation/devicetree/bindings/display/panel/kingdisplay,kd101ne3-40ti.yaml b/Documentation/devicetree/bindings/display/panel/kingdisplay,kd101ne3-40ti.yaml > new file mode 100644 > index 000000000000..dc79a49eea3b > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/panel/kingdisplay,kd101ne3-40ti.yaml > @@ -0,0 +1,63 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/panel/kingdisplay,kd101ne3-40ti.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: King Display KD035G6-40TI based MIPI-DSI panels > + > +description: | > + -This binding is for display panels using an JD9365DA controller > + > +maintainers: > + - Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com> > + > +allOf: > + - $ref: panel-common.yaml# > + > +properties: > + compatible: > + const: kingdisplay,kd101ne3-40ti > + > + backlight: true > + port: true Drop both > + pp3300-supply: true > + reg: true > + enable-gpios: true > + rotation: true Drop these three. panel-common defines them. > + > +required: > + - compatible > + - reg > + - enable-gpios > + - pp3300-supply > + - backlight > + - port These can stay. > + > +unevaluatedProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + > + dsi { > + #address-cells = <1>; > + #size-cells = <0>; > + panel: panel@0 { > + compatible = "kingdisplay,kd101ne3-40ti"; > + reg = <0>; > + enable-gpios = <&pio 98 0>; You included the header, so use it. Best regards, Krzysztof ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver 2024-04-18 8:15 [PATCH v1 0/2] Add kd101ne3 bindings and driver lvzhaoxiong 2024-04-18 8:15 ` [PATCH v1 1/2] dt-bindings: display: panel: Add KD101NE3-40TI support lvzhaoxiong @ 2024-04-18 8:15 ` lvzhaoxiong 2024-04-18 11:46 ` Dmitry Baryshkov 1 sibling, 1 reply; 19+ messages in thread From: lvzhaoxiong @ 2024-04-18 8:15 UTC (permalink / raw To: mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt, dianders, hsinyi Cc: dri-devel, devicetree, linux-kernel, lvzhaoxiong The kingdisplay panel is based on JD9365DA controller. Add a driver for it. Signed-off-by: lvzhaoxiong <lvzhaoxiong@huaqin.corp-partner.google.com> --- drivers/gpu/drm/panel/Kconfig | 9 + drivers/gpu/drm/panel/Makefile | 1 + .../drm/panel/panel-kingdisplay-kd101ne3.c | 607 ++++++++++++++++++ 3 files changed, 617 insertions(+) create mode 100644 drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index 154f5bf82980..2c73086cf102 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -297,6 +297,15 @@ config DRM_PANEL_KINGDISPLAY_KD097D04 24 bit RGB per pixel. It provides a MIPI DSI interface to the host and has a built-in LED backlight. +config DRM_PANEL_KINGDISPLAY_KD101NE3 + tristate "Kingdisplay kd101ne3 panel" + depends on OF + depends on DRM_MIPI_DSI + depends on BACKLIGHT_CLASS_DEVICE + help + Say Y if you want to enable support for panels based on the + Kingdisplay kd101ne3 controller. + config DRM_PANEL_LEADTEK_LTK050H3146W tristate "Leadtek LTK050H3146W panel" depends on OF diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile index 24a02655d726..cbd414b98bb0 100644 --- a/drivers/gpu/drm/panel/Makefile +++ b/drivers/gpu/drm/panel/Makefile @@ -30,6 +30,7 @@ obj-$(CONFIG_DRM_PANEL_JDI_LPM102A188A) += panel-jdi-lpm102a188a.o obj-$(CONFIG_DRM_PANEL_JDI_R63452) += panel-jdi-fhd-r63452.o obj-$(CONFIG_DRM_PANEL_KHADAS_TS050) += panel-khadas-ts050.o obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD097D04) += panel-kingdisplay-kd097d04.o +obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD101NE3) += panel-kingdisplay-kd101ne3.o obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK050H3146W) += panel-leadtek-ltk050h3146w.o obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK500HD1829) += panel-leadtek-ltk500hd1829.o obj-$(CONFIG_DRM_PANEL_LG_LB035Q02) += panel-lg-lb035q02.o diff --git a/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c b/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c new file mode 100644 index 000000000000..dbf0992f8b81 --- /dev/null +++ b/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c @@ -0,0 +1,607 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Panels based on the JD9365DA display controller. + * Author: Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com> + */ + +#include <linux/delay.h> +#include <linux/gpio/consumer.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/regulator/consumer.h> + +#include <drm/drm_connector.h> +#include <drm/drm_crtc.h> +#include <drm/drm_mipi_dsi.h> +#include <drm/drm_panel.h> + +#include <video/mipi_display.h> + +struct panel_desc { + const struct drm_display_mode *modes; + unsigned int bpc; + + /** + * @width_mm: width of the panel's active display area + * @height_mm: height of the panel's active display area + */ + struct { + unsigned int width_mm; + unsigned int height_mm; + } size; + + unsigned long mode_flags; + enum mipi_dsi_pixel_format format; + const struct panel_init_cmd *init_cmds; + unsigned int lanes; + bool discharge_on_disable; + bool lp11_before_reset; +}; + +struct kingdisplay_panel { + struct drm_panel base; + struct mipi_dsi_device *dsi; + + const struct panel_desc *desc; + + enum drm_panel_orientation orientation; + struct regulator *pp3300; + struct gpio_desc *enable_gpio; +}; + +enum dsi_cmd_type { + INIT_DCS_CMD, + DELAY_CMD, +}; + +struct panel_init_cmd { + enum dsi_cmd_type type; + size_t len; + const char *data; +}; + +#define _INIT_DCS_CMD(...) { \ + .type = INIT_DCS_CMD, \ + .len = sizeof((char[]){__VA_ARGS__}), \ + .data = (char[]){__VA_ARGS__} } + +#define _INIT_DELAY_CMD(...) { \ + .type = DELAY_CMD,\ + .len = sizeof((char[]){__VA_ARGS__}), \ + .data = (char[]){__VA_ARGS__} } + +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = { + _INIT_DELAY_CMD(50), + _INIT_DCS_CMD(0xE0, 0x00), + _INIT_DCS_CMD(0xE1, 0x93), + _INIT_DCS_CMD(0xE2, 0x65), + _INIT_DCS_CMD(0xE3, 0xF8), + _INIT_DCS_CMD(0x80, 0x03), + _INIT_DCS_CMD(0xE0, 0x01), + _INIT_DCS_CMD(0x0C, 0x74), + _INIT_DCS_CMD(0x17, 0x00), + _INIT_DCS_CMD(0x18, 0xC7), + _INIT_DCS_CMD(0x19, 0x01), + _INIT_DCS_CMD(0x1A, 0x00), + _INIT_DCS_CMD(0x1B, 0xC7), + _INIT_DCS_CMD(0x1C, 0x01), + _INIT_DCS_CMD(0x24, 0xFE), + _INIT_DCS_CMD(0x37, 0x19), + _INIT_DCS_CMD(0x35, 0x28), + _INIT_DCS_CMD(0x38, 0x05), + _INIT_DCS_CMD(0x39, 0x08), + _INIT_DCS_CMD(0x3A, 0x12), + _INIT_DCS_CMD(0x3C, 0x7E), + _INIT_DCS_CMD(0x3D, 0xFF), + _INIT_DCS_CMD(0x3E, 0xFF), + _INIT_DCS_CMD(0x3F, 0x7F), + _INIT_DCS_CMD(0x40, 0x06), + _INIT_DCS_CMD(0x41, 0xA0), + _INIT_DCS_CMD(0x43, 0x1E), + _INIT_DCS_CMD(0x44, 0x0B), + _INIT_DCS_CMD(0x55, 0x02), + _INIT_DCS_CMD(0x57, 0x6A), + _INIT_DCS_CMD(0x59, 0x0A), + _INIT_DCS_CMD(0x5A, 0x2E), + _INIT_DCS_CMD(0x5B, 0x1A), + _INIT_DCS_CMD(0x5C, 0x15), + _INIT_DCS_CMD(0x5D, 0x7F), + _INIT_DCS_CMD(0x5E, 0x61), + _INIT_DCS_CMD(0x5F, 0x50), + _INIT_DCS_CMD(0x60, 0x43), + _INIT_DCS_CMD(0x61, 0x3F), + _INIT_DCS_CMD(0x62, 0x32), + _INIT_DCS_CMD(0x63, 0x35), + _INIT_DCS_CMD(0x64, 0x1F), + _INIT_DCS_CMD(0x65, 0x38), + _INIT_DCS_CMD(0x66, 0x36), + _INIT_DCS_CMD(0x67, 0x36), + _INIT_DCS_CMD(0x68, 0x54), + _INIT_DCS_CMD(0x69, 0x42), + _INIT_DCS_CMD(0x6A, 0x48), + _INIT_DCS_CMD(0x6B, 0x39), + _INIT_DCS_CMD(0x6C, 0x34), + _INIT_DCS_CMD(0x6D, 0x26), + _INIT_DCS_CMD(0x6E, 0x14), + _INIT_DCS_CMD(0x6F, 0x02), + _INIT_DCS_CMD(0x70, 0x7F), + _INIT_DCS_CMD(0x71, 0x61), + _INIT_DCS_CMD(0x72, 0x50), + _INIT_DCS_CMD(0x73, 0x43), + _INIT_DCS_CMD(0x74, 0x3F), + _INIT_DCS_CMD(0x75, 0x32), + _INIT_DCS_CMD(0x76, 0x35), + _INIT_DCS_CMD(0x77, 0x1F), + _INIT_DCS_CMD(0x78, 0x38), + _INIT_DCS_CMD(0x79, 0x36), + _INIT_DCS_CMD(0x7A, 0x36), + _INIT_DCS_CMD(0x7B, 0x54), + _INIT_DCS_CMD(0x7C, 0x42), + _INIT_DCS_CMD(0x7D, 0x48), + _INIT_DCS_CMD(0x7E, 0x39), + _INIT_DCS_CMD(0x7F, 0x34), + _INIT_DCS_CMD(0x80, 0x26), + _INIT_DCS_CMD(0x81, 0x14), + _INIT_DCS_CMD(0x82, 0x02), + _INIT_DCS_CMD(0xE0, 0x02), + _INIT_DCS_CMD(0x00, 0x52), + _INIT_DCS_CMD(0x01, 0x5F), + _INIT_DCS_CMD(0x02, 0x5F), + _INIT_DCS_CMD(0x03, 0x50), + _INIT_DCS_CMD(0x04, 0x77), + _INIT_DCS_CMD(0x05, 0x57), + _INIT_DCS_CMD(0x06, 0x5F), + _INIT_DCS_CMD(0x07, 0x4E), + _INIT_DCS_CMD(0x08, 0x4C), + _INIT_DCS_CMD(0x09, 0x5F), + _INIT_DCS_CMD(0x0A, 0x4A), + _INIT_DCS_CMD(0x0B, 0x48), + _INIT_DCS_CMD(0x0C, 0x5F), + _INIT_DCS_CMD(0x0D, 0x46), + _INIT_DCS_CMD(0x0E, 0x44), + _INIT_DCS_CMD(0x0F, 0x40), + _INIT_DCS_CMD(0x10, 0x5F), + _INIT_DCS_CMD(0x11, 0x5F), + _INIT_DCS_CMD(0x12, 0x5F), + _INIT_DCS_CMD(0x13, 0x5F), + _INIT_DCS_CMD(0x14, 0x5F), + _INIT_DCS_CMD(0x15, 0x5F), + _INIT_DCS_CMD(0x16, 0x53), + _INIT_DCS_CMD(0x17, 0x5F), + _INIT_DCS_CMD(0x18, 0x5F), + _INIT_DCS_CMD(0x19, 0x51), + _INIT_DCS_CMD(0x1A, 0x77), + _INIT_DCS_CMD(0x1B, 0x57), + _INIT_DCS_CMD(0x1C, 0x5F), + _INIT_DCS_CMD(0x1D, 0x4F), + _INIT_DCS_CMD(0x1E, 0x4D), + _INIT_DCS_CMD(0x1F, 0x5F), + _INIT_DCS_CMD(0x20, 0x4B), + _INIT_DCS_CMD(0x21, 0x49), + _INIT_DCS_CMD(0x22, 0x5F), + _INIT_DCS_CMD(0x23, 0x47), + _INIT_DCS_CMD(0x24, 0x45), + _INIT_DCS_CMD(0x25, 0x41), + _INIT_DCS_CMD(0x26, 0x5F), + _INIT_DCS_CMD(0x27, 0x5F), + _INIT_DCS_CMD(0x28, 0x5F), + _INIT_DCS_CMD(0x29, 0x5F), + _INIT_DCS_CMD(0x2A, 0x5F), + _INIT_DCS_CMD(0x2B, 0x5F), + _INIT_DCS_CMD(0x2C, 0x13), + _INIT_DCS_CMD(0x2D, 0x1F), + _INIT_DCS_CMD(0x2E, 0x1F), + _INIT_DCS_CMD(0x2F, 0x01), + _INIT_DCS_CMD(0x30, 0x17), + _INIT_DCS_CMD(0x31, 0x17), + _INIT_DCS_CMD(0x32, 0x1F), + _INIT_DCS_CMD(0x33, 0x0D), + _INIT_DCS_CMD(0x34, 0x0F), + _INIT_DCS_CMD(0x35, 0x1F), + _INIT_DCS_CMD(0x36, 0x05), + _INIT_DCS_CMD(0x37, 0x07), + _INIT_DCS_CMD(0x38, 0x1F), + _INIT_DCS_CMD(0x39, 0x09), + _INIT_DCS_CMD(0x3A, 0x0B), + _INIT_DCS_CMD(0x3B, 0x11), + _INIT_DCS_CMD(0x3C, 0x1F), + _INIT_DCS_CMD(0x3D, 0x1F), + _INIT_DCS_CMD(0x3E, 0x1F), + _INIT_DCS_CMD(0x3F, 0x1F), + _INIT_DCS_CMD(0x40, 0x1F), + _INIT_DCS_CMD(0x41, 0x1F), + _INIT_DCS_CMD(0x42, 0x12), + _INIT_DCS_CMD(0x43, 0x1F), + _INIT_DCS_CMD(0x44, 0x1F), + _INIT_DCS_CMD(0x45, 0x00), + _INIT_DCS_CMD(0x46, 0x17), + _INIT_DCS_CMD(0x47, 0x17), + _INIT_DCS_CMD(0x48, 0x1F), + _INIT_DCS_CMD(0x49, 0x0C), + _INIT_DCS_CMD(0x4A, 0x0E), + _INIT_DCS_CMD(0x4B, 0x1F), + _INIT_DCS_CMD(0x4C, 0x04), + _INIT_DCS_CMD(0x4D, 0x06), + _INIT_DCS_CMD(0x4E, 0x1F), + _INIT_DCS_CMD(0x4F, 0x08), + _INIT_DCS_CMD(0x50, 0x0A), + _INIT_DCS_CMD(0x51, 0x10), + _INIT_DCS_CMD(0x52, 0x1F), + _INIT_DCS_CMD(0x53, 0x1F), + _INIT_DCS_CMD(0x54, 0x1F), + _INIT_DCS_CMD(0x55, 0x1F), + _INIT_DCS_CMD(0x56, 0x1F), + _INIT_DCS_CMD(0x57, 0x1F), + _INIT_DCS_CMD(0x58, 0x40), + _INIT_DCS_CMD(0x5B, 0x10), + _INIT_DCS_CMD(0x5C, 0x06), + _INIT_DCS_CMD(0x5D, 0x40), + _INIT_DCS_CMD(0x5E, 0x00), + _INIT_DCS_CMD(0x5F, 0x00), + _INIT_DCS_CMD(0x60, 0x40), + _INIT_DCS_CMD(0x61, 0x03), + _INIT_DCS_CMD(0x62, 0x04), + _INIT_DCS_CMD(0x63, 0x6C), + _INIT_DCS_CMD(0x64, 0x6C), + _INIT_DCS_CMD(0x65, 0x75), + _INIT_DCS_CMD(0x66, 0x08), + _INIT_DCS_CMD(0x67, 0xB4), + _INIT_DCS_CMD(0x68, 0x08), + _INIT_DCS_CMD(0x69, 0x6C), + _INIT_DCS_CMD(0x6A, 0x6C), + _INIT_DCS_CMD(0x6B, 0x0C), + _INIT_DCS_CMD(0x6D, 0x00), + _INIT_DCS_CMD(0x6E, 0x00), + _INIT_DCS_CMD(0x6F, 0x88), + _INIT_DCS_CMD(0x75, 0xBB), + _INIT_DCS_CMD(0x76, 0x00), + _INIT_DCS_CMD(0x77, 0x05), + _INIT_DCS_CMD(0x78, 0x2A), + _INIT_DCS_CMD(0xE0, 0x04), + _INIT_DCS_CMD(0x00, 0x0E), + _INIT_DCS_CMD(0x02, 0xB3), + _INIT_DCS_CMD(0x09, 0x61), + _INIT_DCS_CMD(0x0E, 0x48), + + _INIT_DCS_CMD(0xE0, 0x00), + _INIT_DCS_CMD(0X11), + /* T6: 120ms */ + _INIT_DELAY_CMD(120), + _INIT_DCS_CMD(0X29), + _INIT_DELAY_CMD(20), + {}, +}; + +static inline struct kingdisplay_panel *to_kingdisplay_panel(struct drm_panel *panel) +{ + return container_of(panel, struct kingdisplay_panel, base); +} + +static int kingdisplay_panel_init_dcs_cmd(struct kingdisplay_panel *kingdisplay) +{ + struct mipi_dsi_device *dsi = kingdisplay->dsi; + struct drm_panel *panel = &kingdisplay->base; + int i, err = 0; + + if (kingdisplay->desc->init_cmds) { + const struct panel_init_cmd *init_cmds = kingdisplay->desc->init_cmds; + + for (i = 0; init_cmds[i].len != 0; i++) { + const struct panel_init_cmd *cmd = &init_cmds[i]; + + switch (cmd->type) { + case DELAY_CMD: + msleep(cmd->data[0]); + err = 0; + break; + + case INIT_DCS_CMD: + err = mipi_dsi_dcs_write(dsi, cmd->data[0], + cmd->len <= 1 ? NULL : + &cmd->data[1], + cmd->len - 1); + break; + + default: + err = -EINVAL; + } + + if (err < 0) { + dev_err(panel->dev, + "failed to write command %u\n", i); + return err; + } + } + } + return 0; +} + +static int kingdisplay_panel_enter_sleep_mode(struct kingdisplay_panel *kingdisplay) +{ + struct mipi_dsi_device *dsi = kingdisplay->dsi; + int ret; + + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; + + usleep_range(1000, 2000); + ret = mipi_dsi_dcs_set_display_off(dsi); + if (ret < 0) + return ret; + + msleep(50); + + ret = mipi_dsi_dcs_enter_sleep_mode(dsi); + if (ret < 0) + return ret; + + return 0; +} + +static int kingdisplay_panel_disable(struct drm_panel *panel) +{ + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel); + int ret; + + ret = kingdisplay_panel_enter_sleep_mode(kingdisplay); + if (ret < 0) { + dev_err(panel->dev, "failed to set panel off: %d\n", ret); + return ret; + } + + msleep(100); + + return 0; +} + +static int kingdisplay_panel_unprepare(struct drm_panel *panel) +{ + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel); + int err; + + gpiod_set_value_cansleep(kingdisplay->enable_gpio, 0); + + /* T15: 2ms */ + usleep_range(1000, 2000); + + err = regulator_disable(kingdisplay->pp3300); + if (err < 0) + return err; + + return 0; +} + +static int kingdisplay_panel_prepare(struct drm_panel *panel) +{ + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel); + int ret; + + gpiod_set_value(kingdisplay->enable_gpio, 0); + + ret = regulator_enable(kingdisplay->pp3300); + if (ret < 0) + return ret; + + /* T1: 5ms */ + usleep_range(5000, 6000); + + if (kingdisplay->desc->lp11_before_reset) { + mipi_dsi_dcs_nop(kingdisplay->dsi); + usleep_range(1000, 2000); + } + + /* T2: 10ms, T1 + T2 > 5ms*/ + usleep_range(10000, 11000); + + gpiod_set_value_cansleep(kingdisplay->enable_gpio, 1); + + ret = kingdisplay_panel_init_dcs_cmd(kingdisplay); + if (ret < 0) { + dev_err(panel->dev, "failed to init panel: %d\n", ret); + goto poweroff; + } + + return 0; + +poweroff: + regulator_disable(kingdisplay->pp3300); + /* T6: 2ms */ + usleep_range(1000, 2000); + gpiod_set_value(kingdisplay->enable_gpio, 0); + + return ret; +} + +static int kingdisplay_panel_enable(struct drm_panel *panel) +{ + msleep(130); + return 0; +} + +static const struct drm_display_mode kingdisplay_kd101ne3_40ti_default_mode = { + .clock = 70595, + .hdisplay = 800, + .hsync_start = 800 + 30, + .hsync_end = 800 + 30 + 30, + .htotal = 800 + 30 + 30 + 30, + .vdisplay = 1280, + .vsync_start = 1280 + 30, + .vsync_end = 1280 + 30 + 4, + .vtotal = 1280 + 30 + 4 + 8, + .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED, +}; + +static const struct panel_desc kingdisplay_kd101ne3_40ti_desc = { + .modes = &kingdisplay_kd101ne3_40ti_default_mode, + .bpc = 8, + .size = { + .width_mm = 135, + .height_mm = 216, + }, + .lanes = 4, + .format = MIPI_DSI_FMT_RGB888, + .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE | + MIPI_DSI_MODE_LPM, + .init_cmds = kingdisplay_kd101ne3_init_cmd, + .lp11_before_reset = true, +}; + +static int kingdisplay_panel_get_modes(struct drm_panel *panel, + struct drm_connector *connector) +{ + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel); + const struct drm_display_mode *m = kingdisplay->desc->modes; + struct drm_display_mode *mode; + + mode = drm_mode_duplicate(connector->dev, m); + if (!mode) { + dev_err(panel->dev, "failed to add mode %ux%u@%u\n", + m->hdisplay, m->vdisplay, drm_mode_vrefresh(m)); + return -ENOMEM; + } + + mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED; + drm_mode_set_name(mode); + drm_mode_probed_add(connector, mode); + + connector->display_info.width_mm = kingdisplay->desc->size.width_mm; + connector->display_info.height_mm = kingdisplay->desc->size.height_mm; + connector->display_info.bpc = kingdisplay->desc->bpc; + /* + * TODO: Remove once all drm drivers call + * drm_connector_set_orientation_from_panel() + */ + drm_connector_set_panel_orientation(connector, kingdisplay->orientation); + + return 1; +} + +static enum drm_panel_orientation kingdisplay_panel_get_orientation(struct drm_panel *panel) +{ + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel); + + return kingdisplay->orientation; +} + +static const struct drm_panel_funcs kingdisplay_panel_funcs = { + .disable = kingdisplay_panel_disable, + .unprepare = kingdisplay_panel_unprepare, + .prepare = kingdisplay_panel_prepare, + .enable = kingdisplay_panel_enable, + .get_modes = kingdisplay_panel_get_modes, + .get_orientation = kingdisplay_panel_get_orientation, +}; + +static int kingdisplay_panel_add(struct kingdisplay_panel *kingdisplay) +{ + struct device *dev = &kingdisplay->dsi->dev; + int err; + + kingdisplay->pp3300 = devm_regulator_get(dev, "pp3300"); + if (IS_ERR(kingdisplay->pp3300)) + return PTR_ERR(kingdisplay->pp3300); + + + kingdisplay->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW); + if (IS_ERR(kingdisplay->enable_gpio)) { + dev_err(dev, "cannot get reset-gpios %ld\n", + PTR_ERR(kingdisplay->enable_gpio)); + return PTR_ERR(kingdisplay->enable_gpio); + } + + gpiod_set_value(kingdisplay->enable_gpio, 0); + + drm_panel_init(&kingdisplay->base, dev, &kingdisplay_panel_funcs, + DRM_MODE_CONNECTOR_DSI); + err = of_drm_get_panel_orientation(dev->of_node, &kingdisplay->orientation); + if (err < 0) { + dev_err(dev, "%pOF: failed to get orientation %d\n", dev->of_node, err); + return err; + } + + err = drm_panel_of_backlight(&kingdisplay->base); + if (err) + return err; + + kingdisplay->base.funcs = &kingdisplay_panel_funcs; + kingdisplay->base.dev = &kingdisplay->dsi->dev; + + drm_panel_add(&kingdisplay->base); + + return 0; +} + +static int kingdisplay_panel_probe(struct mipi_dsi_device *dsi) +{ + struct kingdisplay_panel *kingdisplay; + int ret; + const struct panel_desc *desc; + + kingdisplay = devm_kzalloc(&dsi->dev, sizeof(*kingdisplay), GFP_KERNEL); + if (!kingdisplay) + return -ENOMEM; + + desc = of_device_get_match_data(&dsi->dev); + dsi->lanes = desc->lanes; + dsi->format = desc->format; + dsi->mode_flags = desc->mode_flags; + kingdisplay->desc = desc; + kingdisplay->dsi = dsi; + ret = kingdisplay_panel_add(kingdisplay); + if (ret < 0) + return ret; + + mipi_dsi_set_drvdata(dsi, kingdisplay); + + ret = mipi_dsi_attach(dsi); + if (ret) + drm_panel_remove(&kingdisplay->base); + + return ret; +} + +static void kingdisplay_panel_shutdown(struct mipi_dsi_device *dsi) +{ + struct kingdisplay_panel *kingdisplay = mipi_dsi_get_drvdata(dsi); + + drm_panel_disable(&kingdisplay->base); + drm_panel_unprepare(&kingdisplay->base); +} + +static void kingdisplay_panel_remove(struct mipi_dsi_device *dsi) +{ + struct kingdisplay_panel *kingdisplay = mipi_dsi_get_drvdata(dsi); + int ret; + + kingdisplay_panel_shutdown(dsi); + + ret = mipi_dsi_detach(dsi); + if (ret < 0) + dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret); + + if (kingdisplay->base.dev) + drm_panel_remove(&kingdisplay->base); +} + +static const struct of_device_id kingdisplay_of_match[] = { + { .compatible = "kingdisplay,kd101ne3-40ti", + .data = &kingdisplay_kd101ne3_40ti_desc + }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, kingdisplay_of_match); + +static struct mipi_dsi_driver kingdisplay_panel_driver = { + .driver = { + .name = "panel-kingdisplay-kd101ne3", + .of_match_table = kingdisplay_of_match, + }, + .probe = kingdisplay_panel_probe, + .remove = kingdisplay_panel_remove, + .shutdown = kingdisplay_panel_shutdown, +}; +module_mipi_dsi_driver(kingdisplay_panel_driver); + +MODULE_AUTHOR("Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com>"); +MODULE_DESCRIPTION("kingdisplay kd101ne3-40ti 800x1280 video mode panel driver"); +MODULE_LICENSE("GPL v2"); -- 2.17.1 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver 2024-04-18 8:15 ` [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver lvzhaoxiong @ 2024-04-18 11:46 ` Dmitry Baryshkov 2024-04-18 13:11 ` Hsin-Yi Wang 0 siblings, 1 reply; 19+ messages in thread From: Dmitry Baryshkov @ 2024-04-18 11:46 UTC (permalink / raw To: lvzhaoxiong Cc: mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt, dianders, hsinyi, dri-devel, devicetree, linux-kernel On Thu, Apr 18, 2024 at 04:15:48PM +0800, lvzhaoxiong wrote: > The kingdisplay panel is based on JD9365DA controller. > Add a driver for it. > > Signed-off-by: lvzhaoxiong <lvzhaoxiong@huaqin.corp-partner.google.com> > --- > drivers/gpu/drm/panel/Kconfig | 9 + > drivers/gpu/drm/panel/Makefile | 1 + > .../drm/panel/panel-kingdisplay-kd101ne3.c | 607 ++++++++++++++++++ > 3 files changed, 617 insertions(+) > create mode 100644 drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c > > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig > index 154f5bf82980..2c73086cf102 100644 > --- a/drivers/gpu/drm/panel/Kconfig > +++ b/drivers/gpu/drm/panel/Kconfig > @@ -297,6 +297,15 @@ config DRM_PANEL_KINGDISPLAY_KD097D04 > 24 bit RGB per pixel. It provides a MIPI DSI interface to > the host and has a built-in LED backlight. > > +config DRM_PANEL_KINGDISPLAY_KD101NE3 > + tristate "Kingdisplay kd101ne3 panel" > + depends on OF > + depends on DRM_MIPI_DSI > + depends on BACKLIGHT_CLASS_DEVICE > + help > + Say Y if you want to enable support for panels based on the > + Kingdisplay kd101ne3 controller. > + > config DRM_PANEL_LEADTEK_LTK050H3146W > tristate "Leadtek LTK050H3146W panel" > depends on OF > diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile > index 24a02655d726..cbd414b98bb0 100644 > --- a/drivers/gpu/drm/panel/Makefile > +++ b/drivers/gpu/drm/panel/Makefile > @@ -30,6 +30,7 @@ obj-$(CONFIG_DRM_PANEL_JDI_LPM102A188A) += panel-jdi-lpm102a188a.o > obj-$(CONFIG_DRM_PANEL_JDI_R63452) += panel-jdi-fhd-r63452.o > obj-$(CONFIG_DRM_PANEL_KHADAS_TS050) += panel-khadas-ts050.o > obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD097D04) += panel-kingdisplay-kd097d04.o > +obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD101NE3) += panel-kingdisplay-kd101ne3.o > obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK050H3146W) += panel-leadtek-ltk050h3146w.o > obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK500HD1829) += panel-leadtek-ltk500hd1829.o > obj-$(CONFIG_DRM_PANEL_LG_LB035Q02) += panel-lg-lb035q02.o > diff --git a/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c b/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c > new file mode 100644 > index 000000000000..dbf0992f8b81 > --- /dev/null > +++ b/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c > @@ -0,0 +1,607 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Panels based on the JD9365DA display controller. > + * Author: Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com> > + */ > + > +#include <linux/delay.h> > +#include <linux/gpio/consumer.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/regulator/consumer.h> > + > +#include <drm/drm_connector.h> > +#include <drm/drm_crtc.h> > +#include <drm/drm_mipi_dsi.h> > +#include <drm/drm_panel.h> > + > +#include <video/mipi_display.h> > + > +struct panel_desc { > + const struct drm_display_mode *modes; > + unsigned int bpc; > + > + /** > + * @width_mm: width of the panel's active display area > + * @height_mm: height of the panel's active display area > + */ > + struct { > + unsigned int width_mm; > + unsigned int height_mm; Please move to the declared mode; > + } size; > + > + unsigned long mode_flags; > + enum mipi_dsi_pixel_format format; > + const struct panel_init_cmd *init_cmds; > + unsigned int lanes; > + bool discharge_on_disable; > + bool lp11_before_reset; > +}; > + > +struct kingdisplay_panel { > + struct drm_panel base; > + struct mipi_dsi_device *dsi; > + > + const struct panel_desc *desc; > + > + enum drm_panel_orientation orientation; > + struct regulator *pp3300; > + struct gpio_desc *enable_gpio; > +}; > + > +enum dsi_cmd_type { > + INIT_DCS_CMD, > + DELAY_CMD, > +}; > + > +struct panel_init_cmd { > + enum dsi_cmd_type type; > + size_t len; > + const char *data; > +}; > + > +#define _INIT_DCS_CMD(...) { \ > + .type = INIT_DCS_CMD, \ > + .len = sizeof((char[]){__VA_ARGS__}), \ > + .data = (char[]){__VA_ARGS__} } > + > +#define _INIT_DELAY_CMD(...) { \ > + .type = DELAY_CMD,\ > + .len = sizeof((char[]){__VA_ARGS__}), \ > + .data = (char[]){__VA_ARGS__} } This is the third panel driver using the same appoach. Can you use mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer the table, we should extract this framework to a common helper. (my preference is shifted towards mipi_dsi_generic_write_seq()). > + > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = { > + _INIT_DELAY_CMD(50), > + _INIT_DCS_CMD(0xE0, 0x00), > + _INIT_DCS_CMD(0xE1, 0x93), > + _INIT_DCS_CMD(0xE2, 0x65), > + _INIT_DCS_CMD(0xE3, 0xF8), > + _INIT_DCS_CMD(0x80, 0x03), > + _INIT_DCS_CMD(0xE0, 0x01), > + _INIT_DCS_CMD(0x0C, 0x74), > + _INIT_DCS_CMD(0x17, 0x00), > + _INIT_DCS_CMD(0x18, 0xC7), > + _INIT_DCS_CMD(0x19, 0x01), > + _INIT_DCS_CMD(0x1A, 0x00), > + _INIT_DCS_CMD(0x1B, 0xC7), > + _INIT_DCS_CMD(0x1C, 0x01), > + _INIT_DCS_CMD(0x24, 0xFE), > + _INIT_DCS_CMD(0x37, 0x19), > + _INIT_DCS_CMD(0x35, 0x28), > + _INIT_DCS_CMD(0x38, 0x05), > + _INIT_DCS_CMD(0x39, 0x08), > + _INIT_DCS_CMD(0x3A, 0x12), > + _INIT_DCS_CMD(0x3C, 0x7E), > + _INIT_DCS_CMD(0x3D, 0xFF), > + _INIT_DCS_CMD(0x3E, 0xFF), > + _INIT_DCS_CMD(0x3F, 0x7F), > + _INIT_DCS_CMD(0x40, 0x06), > + _INIT_DCS_CMD(0x41, 0xA0), > + _INIT_DCS_CMD(0x43, 0x1E), > + _INIT_DCS_CMD(0x44, 0x0B), > + _INIT_DCS_CMD(0x55, 0x02), > + _INIT_DCS_CMD(0x57, 0x6A), > + _INIT_DCS_CMD(0x59, 0x0A), > + _INIT_DCS_CMD(0x5A, 0x2E), > + _INIT_DCS_CMD(0x5B, 0x1A), > + _INIT_DCS_CMD(0x5C, 0x15), > + _INIT_DCS_CMD(0x5D, 0x7F), > + _INIT_DCS_CMD(0x5E, 0x61), > + _INIT_DCS_CMD(0x5F, 0x50), > + _INIT_DCS_CMD(0x60, 0x43), > + _INIT_DCS_CMD(0x61, 0x3F), > + _INIT_DCS_CMD(0x62, 0x32), > + _INIT_DCS_CMD(0x63, 0x35), > + _INIT_DCS_CMD(0x64, 0x1F), > + _INIT_DCS_CMD(0x65, 0x38), > + _INIT_DCS_CMD(0x66, 0x36), > + _INIT_DCS_CMD(0x67, 0x36), > + _INIT_DCS_CMD(0x68, 0x54), > + _INIT_DCS_CMD(0x69, 0x42), > + _INIT_DCS_CMD(0x6A, 0x48), > + _INIT_DCS_CMD(0x6B, 0x39), > + _INIT_DCS_CMD(0x6C, 0x34), > + _INIT_DCS_CMD(0x6D, 0x26), > + _INIT_DCS_CMD(0x6E, 0x14), > + _INIT_DCS_CMD(0x6F, 0x02), > + _INIT_DCS_CMD(0x70, 0x7F), > + _INIT_DCS_CMD(0x71, 0x61), > + _INIT_DCS_CMD(0x72, 0x50), > + _INIT_DCS_CMD(0x73, 0x43), > + _INIT_DCS_CMD(0x74, 0x3F), > + _INIT_DCS_CMD(0x75, 0x32), > + _INIT_DCS_CMD(0x76, 0x35), > + _INIT_DCS_CMD(0x77, 0x1F), > + _INIT_DCS_CMD(0x78, 0x38), > + _INIT_DCS_CMD(0x79, 0x36), > + _INIT_DCS_CMD(0x7A, 0x36), > + _INIT_DCS_CMD(0x7B, 0x54), > + _INIT_DCS_CMD(0x7C, 0x42), > + _INIT_DCS_CMD(0x7D, 0x48), > + _INIT_DCS_CMD(0x7E, 0x39), > + _INIT_DCS_CMD(0x7F, 0x34), > + _INIT_DCS_CMD(0x80, 0x26), > + _INIT_DCS_CMD(0x81, 0x14), > + _INIT_DCS_CMD(0x82, 0x02), > + _INIT_DCS_CMD(0xE0, 0x02), > + _INIT_DCS_CMD(0x00, 0x52), > + _INIT_DCS_CMD(0x01, 0x5F), > + _INIT_DCS_CMD(0x02, 0x5F), > + _INIT_DCS_CMD(0x03, 0x50), > + _INIT_DCS_CMD(0x04, 0x77), > + _INIT_DCS_CMD(0x05, 0x57), > + _INIT_DCS_CMD(0x06, 0x5F), > + _INIT_DCS_CMD(0x07, 0x4E), > + _INIT_DCS_CMD(0x08, 0x4C), > + _INIT_DCS_CMD(0x09, 0x5F), > + _INIT_DCS_CMD(0x0A, 0x4A), > + _INIT_DCS_CMD(0x0B, 0x48), > + _INIT_DCS_CMD(0x0C, 0x5F), > + _INIT_DCS_CMD(0x0D, 0x46), > + _INIT_DCS_CMD(0x0E, 0x44), > + _INIT_DCS_CMD(0x0F, 0x40), > + _INIT_DCS_CMD(0x10, 0x5F), > + _INIT_DCS_CMD(0x11, 0x5F), > + _INIT_DCS_CMD(0x12, 0x5F), > + _INIT_DCS_CMD(0x13, 0x5F), > + _INIT_DCS_CMD(0x14, 0x5F), > + _INIT_DCS_CMD(0x15, 0x5F), > + _INIT_DCS_CMD(0x16, 0x53), > + _INIT_DCS_CMD(0x17, 0x5F), > + _INIT_DCS_CMD(0x18, 0x5F), > + _INIT_DCS_CMD(0x19, 0x51), > + _INIT_DCS_CMD(0x1A, 0x77), > + _INIT_DCS_CMD(0x1B, 0x57), > + _INIT_DCS_CMD(0x1C, 0x5F), > + _INIT_DCS_CMD(0x1D, 0x4F), > + _INIT_DCS_CMD(0x1E, 0x4D), > + _INIT_DCS_CMD(0x1F, 0x5F), > + _INIT_DCS_CMD(0x20, 0x4B), > + _INIT_DCS_CMD(0x21, 0x49), > + _INIT_DCS_CMD(0x22, 0x5F), > + _INIT_DCS_CMD(0x23, 0x47), > + _INIT_DCS_CMD(0x24, 0x45), > + _INIT_DCS_CMD(0x25, 0x41), > + _INIT_DCS_CMD(0x26, 0x5F), > + _INIT_DCS_CMD(0x27, 0x5F), > + _INIT_DCS_CMD(0x28, 0x5F), > + _INIT_DCS_CMD(0x29, 0x5F), > + _INIT_DCS_CMD(0x2A, 0x5F), > + _INIT_DCS_CMD(0x2B, 0x5F), > + _INIT_DCS_CMD(0x2C, 0x13), > + _INIT_DCS_CMD(0x2D, 0x1F), > + _INIT_DCS_CMD(0x2E, 0x1F), > + _INIT_DCS_CMD(0x2F, 0x01), > + _INIT_DCS_CMD(0x30, 0x17), > + _INIT_DCS_CMD(0x31, 0x17), > + _INIT_DCS_CMD(0x32, 0x1F), > + _INIT_DCS_CMD(0x33, 0x0D), > + _INIT_DCS_CMD(0x34, 0x0F), > + _INIT_DCS_CMD(0x35, 0x1F), > + _INIT_DCS_CMD(0x36, 0x05), > + _INIT_DCS_CMD(0x37, 0x07), > + _INIT_DCS_CMD(0x38, 0x1F), > + _INIT_DCS_CMD(0x39, 0x09), > + _INIT_DCS_CMD(0x3A, 0x0B), > + _INIT_DCS_CMD(0x3B, 0x11), > + _INIT_DCS_CMD(0x3C, 0x1F), > + _INIT_DCS_CMD(0x3D, 0x1F), > + _INIT_DCS_CMD(0x3E, 0x1F), > + _INIT_DCS_CMD(0x3F, 0x1F), > + _INIT_DCS_CMD(0x40, 0x1F), > + _INIT_DCS_CMD(0x41, 0x1F), > + _INIT_DCS_CMD(0x42, 0x12), > + _INIT_DCS_CMD(0x43, 0x1F), > + _INIT_DCS_CMD(0x44, 0x1F), > + _INIT_DCS_CMD(0x45, 0x00), > + _INIT_DCS_CMD(0x46, 0x17), > + _INIT_DCS_CMD(0x47, 0x17), > + _INIT_DCS_CMD(0x48, 0x1F), > + _INIT_DCS_CMD(0x49, 0x0C), > + _INIT_DCS_CMD(0x4A, 0x0E), > + _INIT_DCS_CMD(0x4B, 0x1F), > + _INIT_DCS_CMD(0x4C, 0x04), > + _INIT_DCS_CMD(0x4D, 0x06), > + _INIT_DCS_CMD(0x4E, 0x1F), > + _INIT_DCS_CMD(0x4F, 0x08), > + _INIT_DCS_CMD(0x50, 0x0A), > + _INIT_DCS_CMD(0x51, 0x10), > + _INIT_DCS_CMD(0x52, 0x1F), > + _INIT_DCS_CMD(0x53, 0x1F), > + _INIT_DCS_CMD(0x54, 0x1F), > + _INIT_DCS_CMD(0x55, 0x1F), > + _INIT_DCS_CMD(0x56, 0x1F), > + _INIT_DCS_CMD(0x57, 0x1F), > + _INIT_DCS_CMD(0x58, 0x40), > + _INIT_DCS_CMD(0x5B, 0x10), > + _INIT_DCS_CMD(0x5C, 0x06), > + _INIT_DCS_CMD(0x5D, 0x40), > + _INIT_DCS_CMD(0x5E, 0x00), > + _INIT_DCS_CMD(0x5F, 0x00), > + _INIT_DCS_CMD(0x60, 0x40), > + _INIT_DCS_CMD(0x61, 0x03), > + _INIT_DCS_CMD(0x62, 0x04), > + _INIT_DCS_CMD(0x63, 0x6C), > + _INIT_DCS_CMD(0x64, 0x6C), > + _INIT_DCS_CMD(0x65, 0x75), > + _INIT_DCS_CMD(0x66, 0x08), > + _INIT_DCS_CMD(0x67, 0xB4), > + _INIT_DCS_CMD(0x68, 0x08), > + _INIT_DCS_CMD(0x69, 0x6C), > + _INIT_DCS_CMD(0x6A, 0x6C), > + _INIT_DCS_CMD(0x6B, 0x0C), > + _INIT_DCS_CMD(0x6D, 0x00), > + _INIT_DCS_CMD(0x6E, 0x00), > + _INIT_DCS_CMD(0x6F, 0x88), > + _INIT_DCS_CMD(0x75, 0xBB), > + _INIT_DCS_CMD(0x76, 0x00), > + _INIT_DCS_CMD(0x77, 0x05), > + _INIT_DCS_CMD(0x78, 0x2A), > + _INIT_DCS_CMD(0xE0, 0x04), > + _INIT_DCS_CMD(0x00, 0x0E), > + _INIT_DCS_CMD(0x02, 0xB3), > + _INIT_DCS_CMD(0x09, 0x61), > + _INIT_DCS_CMD(0x0E, 0x48), > + > + _INIT_DCS_CMD(0xE0, 0x00), > + _INIT_DCS_CMD(0X11), > + /* T6: 120ms */ > + _INIT_DELAY_CMD(120), > + _INIT_DCS_CMD(0X29), > + _INIT_DELAY_CMD(20), > + {}, > +}; > + > +static inline struct kingdisplay_panel *to_kingdisplay_panel(struct drm_panel *panel) > +{ > + return container_of(panel, struct kingdisplay_panel, base); > +} > + > +static int kingdisplay_panel_init_dcs_cmd(struct kingdisplay_panel *kingdisplay) > +{ > + struct mipi_dsi_device *dsi = kingdisplay->dsi; > + struct drm_panel *panel = &kingdisplay->base; > + int i, err = 0; > + > + if (kingdisplay->desc->init_cmds) { > + const struct panel_init_cmd *init_cmds = kingdisplay->desc->init_cmds; > + > + for (i = 0; init_cmds[i].len != 0; i++) { > + const struct panel_init_cmd *cmd = &init_cmds[i]; > + > + switch (cmd->type) { > + case DELAY_CMD: > + msleep(cmd->data[0]); > + err = 0; > + break; > + > + case INIT_DCS_CMD: > + err = mipi_dsi_dcs_write(dsi, cmd->data[0], > + cmd->len <= 1 ? NULL : > + &cmd->data[1], > + cmd->len - 1); > + break; > + > + default: > + err = -EINVAL; > + } > + > + if (err < 0) { > + dev_err(panel->dev, > + "failed to write command %u\n", i); > + return err; > + } > + } > + } > + return 0; > +} > + > +static int kingdisplay_panel_enter_sleep_mode(struct kingdisplay_panel *kingdisplay) > +{ > + struct mipi_dsi_device *dsi = kingdisplay->dsi; > + int ret; > + > + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; > + > + usleep_range(1000, 2000); > + ret = mipi_dsi_dcs_set_display_off(dsi); > + if (ret < 0) > + return ret; > + > + msleep(50); > + > + ret = mipi_dsi_dcs_enter_sleep_mode(dsi); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static int kingdisplay_panel_disable(struct drm_panel *panel) > +{ > + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel); > + int ret; > + > + ret = kingdisplay_panel_enter_sleep_mode(kingdisplay); > + if (ret < 0) { > + dev_err(panel->dev, "failed to set panel off: %d\n", ret); > + return ret; > + } > + > + msleep(100); > + > + return 0; > +} > + > +static int kingdisplay_panel_unprepare(struct drm_panel *panel) > +{ > + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel); > + int err; > + > + gpiod_set_value_cansleep(kingdisplay->enable_gpio, 0); > + > + /* T15: 2ms */ > + usleep_range(1000, 2000); > + > + err = regulator_disable(kingdisplay->pp3300); > + if (err < 0) > + return err; > + > + return 0; > +} > + > +static int kingdisplay_panel_prepare(struct drm_panel *panel) > +{ > + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel); > + int ret; > + > + gpiod_set_value(kingdisplay->enable_gpio, 0); > + > + ret = regulator_enable(kingdisplay->pp3300); > + if (ret < 0) > + return ret; > + > + /* T1: 5ms */ > + usleep_range(5000, 6000); > + > + if (kingdisplay->desc->lp11_before_reset) { > + mipi_dsi_dcs_nop(kingdisplay->dsi); > + usleep_range(1000, 2000); > + } > + > + /* T2: 10ms, T1 + T2 > 5ms*/ > + usleep_range(10000, 11000); > + > + gpiod_set_value_cansleep(kingdisplay->enable_gpio, 1); > + > + ret = kingdisplay_panel_init_dcs_cmd(kingdisplay); > + if (ret < 0) { > + dev_err(panel->dev, "failed to init panel: %d\n", ret); > + goto poweroff; > + } > + > + return 0; > + > +poweroff: > + regulator_disable(kingdisplay->pp3300); > + /* T6: 2ms */ > + usleep_range(1000, 2000); > + gpiod_set_value(kingdisplay->enable_gpio, 0); > + > + return ret; > +} > + > +static int kingdisplay_panel_enable(struct drm_panel *panel) > +{ > + msleep(130); > + return 0; > +} > + > +static const struct drm_display_mode kingdisplay_kd101ne3_40ti_default_mode = { > + .clock = 70595, > + .hdisplay = 800, > + .hsync_start = 800 + 30, > + .hsync_end = 800 + 30 + 30, > + .htotal = 800 + 30 + 30 + 30, > + .vdisplay = 1280, > + .vsync_start = 1280 + 30, > + .vsync_end = 1280 + 30 + 4, > + .vtotal = 1280 + 30 + 4 + 8, > + .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED, > +}; > + > +static const struct panel_desc kingdisplay_kd101ne3_40ti_desc = { > + .modes = &kingdisplay_kd101ne3_40ti_default_mode, > + .bpc = 8, > + .size = { > + .width_mm = 135, > + .height_mm = 216, > + }, > + .lanes = 4, > + .format = MIPI_DSI_FMT_RGB888, > + .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE | > + MIPI_DSI_MODE_LPM, > + .init_cmds = kingdisplay_kd101ne3_init_cmd, > + .lp11_before_reset = true, > +}; > + > +static int kingdisplay_panel_get_modes(struct drm_panel *panel, > + struct drm_connector *connector) > +{ > + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel); > + const struct drm_display_mode *m = kingdisplay->desc->modes; > + struct drm_display_mode *mode; > + > + mode = drm_mode_duplicate(connector->dev, m); > + if (!mode) { > + dev_err(panel->dev, "failed to add mode %ux%u@%u\n", > + m->hdisplay, m->vdisplay, drm_mode_vrefresh(m)); > + return -ENOMEM; > + } > + > + mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED; > + drm_mode_set_name(mode); > + drm_mode_probed_add(connector, mode); > + > + connector->display_info.width_mm = kingdisplay->desc->size.width_mm; > + connector->display_info.height_mm = kingdisplay->desc->size.height_mm; Please use drm_connector_helper_get_modes_fixed() > + connector->display_info.bpc = kingdisplay->desc->bpc; > + /* > + * TODO: Remove once all drm drivers call > + * drm_connector_set_orientation_from_panel() What is your usecase / platform? Are you using drm_bridge_connector? If not, why? > + */ > + drm_connector_set_panel_orientation(connector, kingdisplay->orientation); > + > + return 1; > +} > + > +static enum drm_panel_orientation kingdisplay_panel_get_orientation(struct drm_panel *panel) > +{ > + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel); > + > + return kingdisplay->orientation; > +} > + > +static const struct drm_panel_funcs kingdisplay_panel_funcs = { > + .disable = kingdisplay_panel_disable, > + .unprepare = kingdisplay_panel_unprepare, > + .prepare = kingdisplay_panel_prepare, > + .enable = kingdisplay_panel_enable, > + .get_modes = kingdisplay_panel_get_modes, > + .get_orientation = kingdisplay_panel_get_orientation, > +}; > + > +static int kingdisplay_panel_add(struct kingdisplay_panel *kingdisplay) > +{ > + struct device *dev = &kingdisplay->dsi->dev; > + int err; > + > + kingdisplay->pp3300 = devm_regulator_get(dev, "pp3300"); > + if (IS_ERR(kingdisplay->pp3300)) > + return PTR_ERR(kingdisplay->pp3300); > + > + > + kingdisplay->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW); > + if (IS_ERR(kingdisplay->enable_gpio)) { > + dev_err(dev, "cannot get reset-gpios %ld\n", > + PTR_ERR(kingdisplay->enable_gpio)); > + return PTR_ERR(kingdisplay->enable_gpio); > + } > + > + gpiod_set_value(kingdisplay->enable_gpio, 0); > + > + drm_panel_init(&kingdisplay->base, dev, &kingdisplay_panel_funcs, > + DRM_MODE_CONNECTOR_DSI); > + err = of_drm_get_panel_orientation(dev->of_node, &kingdisplay->orientation); > + if (err < 0) { > + dev_err(dev, "%pOF: failed to get orientation %d\n", dev->of_node, err); > + return err; > + } > + > + err = drm_panel_of_backlight(&kingdisplay->base); > + if (err) > + return err; > + > + kingdisplay->base.funcs = &kingdisplay_panel_funcs; > + kingdisplay->base.dev = &kingdisplay->dsi->dev; > + > + drm_panel_add(&kingdisplay->base); > + > + return 0; > +} > + > +static int kingdisplay_panel_probe(struct mipi_dsi_device *dsi) > +{ > + struct kingdisplay_panel *kingdisplay; > + int ret; > + const struct panel_desc *desc; > + > + kingdisplay = devm_kzalloc(&dsi->dev, sizeof(*kingdisplay), GFP_KERNEL); > + if (!kingdisplay) > + return -ENOMEM; > + > + desc = of_device_get_match_data(&dsi->dev); > + dsi->lanes = desc->lanes; > + dsi->format = desc->format; > + dsi->mode_flags = desc->mode_flags; > + kingdisplay->desc = desc; > + kingdisplay->dsi = dsi; > + ret = kingdisplay_panel_add(kingdisplay); > + if (ret < 0) > + return ret; > + > + mipi_dsi_set_drvdata(dsi, kingdisplay); > + > + ret = mipi_dsi_attach(dsi); > + if (ret) > + drm_panel_remove(&kingdisplay->base); > + > + return ret; > +} > + > +static void kingdisplay_panel_shutdown(struct mipi_dsi_device *dsi) > +{ > + struct kingdisplay_panel *kingdisplay = mipi_dsi_get_drvdata(dsi); > + > + drm_panel_disable(&kingdisplay->base); > + drm_panel_unprepare(&kingdisplay->base); > +} > + > +static void kingdisplay_panel_remove(struct mipi_dsi_device *dsi) > +{ > + struct kingdisplay_panel *kingdisplay = mipi_dsi_get_drvdata(dsi); > + int ret; > + > + kingdisplay_panel_shutdown(dsi); > + > + ret = mipi_dsi_detach(dsi); > + if (ret < 0) > + dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret); > + > + if (kingdisplay->base.dev) > + drm_panel_remove(&kingdisplay->base); You are adding it unconditionally, so there should be no condition on removal of the panel. > +} > + > +static const struct of_device_id kingdisplay_of_match[] = { > + { .compatible = "kingdisplay,kd101ne3-40ti", > + .data = &kingdisplay_kd101ne3_40ti_desc > + }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, kingdisplay_of_match); > + > +static struct mipi_dsi_driver kingdisplay_panel_driver = { > + .driver = { > + .name = "panel-kingdisplay-kd101ne3", > + .of_match_table = kingdisplay_of_match, > + }, > + .probe = kingdisplay_panel_probe, > + .remove = kingdisplay_panel_remove, > + .shutdown = kingdisplay_panel_shutdown, > +}; > +module_mipi_dsi_driver(kingdisplay_panel_driver); > + > +MODULE_AUTHOR("Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com>"); > +MODULE_DESCRIPTION("kingdisplay kd101ne3-40ti 800x1280 video mode panel driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.17.1 > -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver 2024-04-18 11:46 ` Dmitry Baryshkov @ 2024-04-18 13:11 ` Hsin-Yi Wang 2024-04-18 14:11 ` Dmitry Baryshkov 0 siblings, 1 reply; 19+ messages in thread From: Hsin-Yi Wang @ 2024-04-18 13:11 UTC (permalink / raw To: Dmitry Baryshkov Cc: lvzhaoxiong, mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt, dianders, dri-devel, devicetree, linux-kernel On Thu, Apr 18, 2024 at 7:46 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Thu, Apr 18, 2024 at 04:15:48PM +0800, lvzhaoxiong wrote: > > The kingdisplay panel is based on JD9365DA controller. > > Add a driver for it. > > > > Signed-off-by: lvzhaoxiong <lvzhaoxiong@huaqin.corp-partner.google.com> > > --- > > drivers/gpu/drm/panel/Kconfig | 9 + > > drivers/gpu/drm/panel/Makefile | 1 + > > .../drm/panel/panel-kingdisplay-kd101ne3.c | 607 ++++++++++++++++++ > > 3 files changed, 617 insertions(+) > > create mode 100644 drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c > > > > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig > > index 154f5bf82980..2c73086cf102 100644 > > --- a/drivers/gpu/drm/panel/Kconfig > > +++ b/drivers/gpu/drm/panel/Kconfig > > @@ -297,6 +297,15 @@ config DRM_PANEL_KINGDISPLAY_KD097D04 > > 24 bit RGB per pixel. It provides a MIPI DSI interface to > > the host and has a built-in LED backlight. > > > > +config DRM_PANEL_KINGDISPLAY_KD101NE3 > > + tristate "Kingdisplay kd101ne3 panel" > > + depends on OF > > + depends on DRM_MIPI_DSI > > + depends on BACKLIGHT_CLASS_DEVICE > > + help > > + Say Y if you want to enable support for panels based on the > > + Kingdisplay kd101ne3 controller. > > + > > config DRM_PANEL_LEADTEK_LTK050H3146W > > tristate "Leadtek LTK050H3146W panel" > > depends on OF > > diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile > > index 24a02655d726..cbd414b98bb0 100644 > > --- a/drivers/gpu/drm/panel/Makefile > > +++ b/drivers/gpu/drm/panel/Makefile > > @@ -30,6 +30,7 @@ obj-$(CONFIG_DRM_PANEL_JDI_LPM102A188A) += panel-jdi-lpm102a188a.o > > obj-$(CONFIG_DRM_PANEL_JDI_R63452) += panel-jdi-fhd-r63452.o > > obj-$(CONFIG_DRM_PANEL_KHADAS_TS050) += panel-khadas-ts050.o > > obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD097D04) += panel-kingdisplay-kd097d04.o > > +obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD101NE3) += panel-kingdisplay-kd101ne3.o > > obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK050H3146W) += panel-leadtek-ltk050h3146w.o > > obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK500HD1829) += panel-leadtek-ltk500hd1829.o > > obj-$(CONFIG_DRM_PANEL_LG_LB035Q02) += panel-lg-lb035q02.o > > diff --git a/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c b/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c > > new file mode 100644 > > index 000000000000..dbf0992f8b81 > > --- /dev/null > > +++ b/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c > > @@ -0,0 +1,607 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Panels based on the JD9365DA display controller. > > + * Author: Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com> > > + */ > > + > > +#include <linux/delay.h> > > +#include <linux/gpio/consumer.h> > > +#include <linux/module.h> > > +#include <linux/of.h> > > +#include <linux/of_device.h> > > +#include <linux/regulator/consumer.h> > > + > > +#include <drm/drm_connector.h> > > +#include <drm/drm_crtc.h> > > +#include <drm/drm_mipi_dsi.h> > > +#include <drm/drm_panel.h> > > + > > +#include <video/mipi_display.h> > > + > > +struct panel_desc { > > + const struct drm_display_mode *modes; > > + unsigned int bpc; > > + > > + /** > > + * @width_mm: width of the panel's active display area > > + * @height_mm: height of the panel's active display area > > + */ > > + struct { > > + unsigned int width_mm; > > + unsigned int height_mm; > > Please move to the declared mode; > > > + } size; > > + > > + unsigned long mode_flags; > > + enum mipi_dsi_pixel_format format; > > + const struct panel_init_cmd *init_cmds; > > + unsigned int lanes; > > + bool discharge_on_disable; > > + bool lp11_before_reset; > > +}; > > + > > +struct kingdisplay_panel { > > + struct drm_panel base; > > + struct mipi_dsi_device *dsi; > > + > > + const struct panel_desc *desc; > > + > > + enum drm_panel_orientation orientation; > > + struct regulator *pp3300; > > + struct gpio_desc *enable_gpio; > > +}; > > + > > +enum dsi_cmd_type { > > + INIT_DCS_CMD, > > + DELAY_CMD, > > +}; > > + > > +struct panel_init_cmd { > > + enum dsi_cmd_type type; > > + size_t len; > > + const char *data; > > +}; > > + > > +#define _INIT_DCS_CMD(...) { \ > > + .type = INIT_DCS_CMD, \ > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > + .data = (char[]){__VA_ARGS__} } > > + > > +#define _INIT_DELAY_CMD(...) { \ > > + .type = DELAY_CMD,\ > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > + .data = (char[]){__VA_ARGS__} } > > This is the third panel driver using the same appoach. Can you use > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer > the table, we should extract this framework to a common helper. > (my preference is shifted towards mipi_dsi_generic_write_seq()). > The drawback of mipi_dsi_generic_write_seq() is that it can cause the kernel size grows a lot since every sequence will be expanded. Similar discussion in here: https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/ This patch would increase the module size from 157K to 572K. scripts/bloat-o-meter shows chg +235.95%. So maybe the common helper is better regarding the kernel module size? > > + > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = { > > + _INIT_DELAY_CMD(50), > > + _INIT_DCS_CMD(0xE0, 0x00), > > + _INIT_DCS_CMD(0xE1, 0x93), > > + _INIT_DCS_CMD(0xE2, 0x65), > > + _INIT_DCS_CMD(0xE3, 0xF8), > > + _INIT_DCS_CMD(0x80, 0x03), > > + _INIT_DCS_CMD(0xE0, 0x01), > > + _INIT_DCS_CMD(0x0C, 0x74), > > + _INIT_DCS_CMD(0x17, 0x00), > > + _INIT_DCS_CMD(0x18, 0xC7), > > + _INIT_DCS_CMD(0x19, 0x01), > > + _INIT_DCS_CMD(0x1A, 0x00), > > + _INIT_DCS_CMD(0x1B, 0xC7), > > + _INIT_DCS_CMD(0x1C, 0x01), > > + _INIT_DCS_CMD(0x24, 0xFE), > > + _INIT_DCS_CMD(0x37, 0x19), > > + _INIT_DCS_CMD(0x35, 0x28), > > + _INIT_DCS_CMD(0x38, 0x05), > > + _INIT_DCS_CMD(0x39, 0x08), > > + _INIT_DCS_CMD(0x3A, 0x12), > > + _INIT_DCS_CMD(0x3C, 0x7E), > > + _INIT_DCS_CMD(0x3D, 0xFF), > > + _INIT_DCS_CMD(0x3E, 0xFF), > > + _INIT_DCS_CMD(0x3F, 0x7F), > > + _INIT_DCS_CMD(0x40, 0x06), > > + _INIT_DCS_CMD(0x41, 0xA0), > > + _INIT_DCS_CMD(0x43, 0x1E), > > + _INIT_DCS_CMD(0x44, 0x0B), > > + _INIT_DCS_CMD(0x55, 0x02), > > + _INIT_DCS_CMD(0x57, 0x6A), > > + _INIT_DCS_CMD(0x59, 0x0A), > > + _INIT_DCS_CMD(0x5A, 0x2E), > > + _INIT_DCS_CMD(0x5B, 0x1A), > > + _INIT_DCS_CMD(0x5C, 0x15), > > + _INIT_DCS_CMD(0x5D, 0x7F), > > + _INIT_DCS_CMD(0x5E, 0x61), > > + _INIT_DCS_CMD(0x5F, 0x50), > > + _INIT_DCS_CMD(0x60, 0x43), > > + _INIT_DCS_CMD(0x61, 0x3F), > > + _INIT_DCS_CMD(0x62, 0x32), > > + _INIT_DCS_CMD(0x63, 0x35), > > + _INIT_DCS_CMD(0x64, 0x1F), > > + _INIT_DCS_CMD(0x65, 0x38), > > + _INIT_DCS_CMD(0x66, 0x36), > > + _INIT_DCS_CMD(0x67, 0x36), > > + _INIT_DCS_CMD(0x68, 0x54), > > + _INIT_DCS_CMD(0x69, 0x42), > > + _INIT_DCS_CMD(0x6A, 0x48), > > + _INIT_DCS_CMD(0x6B, 0x39), > > + _INIT_DCS_CMD(0x6C, 0x34), > > + _INIT_DCS_CMD(0x6D, 0x26), > > + _INIT_DCS_CMD(0x6E, 0x14), > > + _INIT_DCS_CMD(0x6F, 0x02), > > + _INIT_DCS_CMD(0x70, 0x7F), > > + _INIT_DCS_CMD(0x71, 0x61), > > + _INIT_DCS_CMD(0x72, 0x50), > > + _INIT_DCS_CMD(0x73, 0x43), > > + _INIT_DCS_CMD(0x74, 0x3F), > > + _INIT_DCS_CMD(0x75, 0x32), > > + _INIT_DCS_CMD(0x76, 0x35), > > + _INIT_DCS_CMD(0x77, 0x1F), > > + _INIT_DCS_CMD(0x78, 0x38), > > + _INIT_DCS_CMD(0x79, 0x36), > > + _INIT_DCS_CMD(0x7A, 0x36), > > + _INIT_DCS_CMD(0x7B, 0x54), > > + _INIT_DCS_CMD(0x7C, 0x42), > > + _INIT_DCS_CMD(0x7D, 0x48), > > + _INIT_DCS_CMD(0x7E, 0x39), > > + _INIT_DCS_CMD(0x7F, 0x34), > > + _INIT_DCS_CMD(0x80, 0x26), > > + _INIT_DCS_CMD(0x81, 0x14), > > + _INIT_DCS_CMD(0x82, 0x02), > > + _INIT_DCS_CMD(0xE0, 0x02), > > + _INIT_DCS_CMD(0x00, 0x52), > > + _INIT_DCS_CMD(0x01, 0x5F), > > + _INIT_DCS_CMD(0x02, 0x5F), > > + _INIT_DCS_CMD(0x03, 0x50), > > + _INIT_DCS_CMD(0x04, 0x77), > > + _INIT_DCS_CMD(0x05, 0x57), > > + _INIT_DCS_CMD(0x06, 0x5F), > > + _INIT_DCS_CMD(0x07, 0x4E), > > + _INIT_DCS_CMD(0x08, 0x4C), > > + _INIT_DCS_CMD(0x09, 0x5F), > > + _INIT_DCS_CMD(0x0A, 0x4A), > > + _INIT_DCS_CMD(0x0B, 0x48), > > + _INIT_DCS_CMD(0x0C, 0x5F), > > + _INIT_DCS_CMD(0x0D, 0x46), > > + _INIT_DCS_CMD(0x0E, 0x44), > > + _INIT_DCS_CMD(0x0F, 0x40), > > + _INIT_DCS_CMD(0x10, 0x5F), > > + _INIT_DCS_CMD(0x11, 0x5F), > > + _INIT_DCS_CMD(0x12, 0x5F), > > + _INIT_DCS_CMD(0x13, 0x5F), > > + _INIT_DCS_CMD(0x14, 0x5F), > > + _INIT_DCS_CMD(0x15, 0x5F), > > + _INIT_DCS_CMD(0x16, 0x53), > > + _INIT_DCS_CMD(0x17, 0x5F), > > + _INIT_DCS_CMD(0x18, 0x5F), > > + _INIT_DCS_CMD(0x19, 0x51), > > + _INIT_DCS_CMD(0x1A, 0x77), > > + _INIT_DCS_CMD(0x1B, 0x57), > > + _INIT_DCS_CMD(0x1C, 0x5F), > > + _INIT_DCS_CMD(0x1D, 0x4F), > > + _INIT_DCS_CMD(0x1E, 0x4D), > > + _INIT_DCS_CMD(0x1F, 0x5F), > > + _INIT_DCS_CMD(0x20, 0x4B), > > + _INIT_DCS_CMD(0x21, 0x49), > > + _INIT_DCS_CMD(0x22, 0x5F), > > + _INIT_DCS_CMD(0x23, 0x47), > > + _INIT_DCS_CMD(0x24, 0x45), > > + _INIT_DCS_CMD(0x25, 0x41), > > + _INIT_DCS_CMD(0x26, 0x5F), > > + _INIT_DCS_CMD(0x27, 0x5F), > > + _INIT_DCS_CMD(0x28, 0x5F), > > + _INIT_DCS_CMD(0x29, 0x5F), > > + _INIT_DCS_CMD(0x2A, 0x5F), > > + _INIT_DCS_CMD(0x2B, 0x5F), > > + _INIT_DCS_CMD(0x2C, 0x13), > > + _INIT_DCS_CMD(0x2D, 0x1F), > > + _INIT_DCS_CMD(0x2E, 0x1F), > > + _INIT_DCS_CMD(0x2F, 0x01), > > + _INIT_DCS_CMD(0x30, 0x17), > > + _INIT_DCS_CMD(0x31, 0x17), > > + _INIT_DCS_CMD(0x32, 0x1F), > > + _INIT_DCS_CMD(0x33, 0x0D), > > + _INIT_DCS_CMD(0x34, 0x0F), > > + _INIT_DCS_CMD(0x35, 0x1F), > > + _INIT_DCS_CMD(0x36, 0x05), > > + _INIT_DCS_CMD(0x37, 0x07), > > + _INIT_DCS_CMD(0x38, 0x1F), > > + _INIT_DCS_CMD(0x39, 0x09), > > + _INIT_DCS_CMD(0x3A, 0x0B), > > + _INIT_DCS_CMD(0x3B, 0x11), > > + _INIT_DCS_CMD(0x3C, 0x1F), > > + _INIT_DCS_CMD(0x3D, 0x1F), > > + _INIT_DCS_CMD(0x3E, 0x1F), > > + _INIT_DCS_CMD(0x3F, 0x1F), > > + _INIT_DCS_CMD(0x40, 0x1F), > > + _INIT_DCS_CMD(0x41, 0x1F), > > + _INIT_DCS_CMD(0x42, 0x12), > > + _INIT_DCS_CMD(0x43, 0x1F), > > + _INIT_DCS_CMD(0x44, 0x1F), > > + _INIT_DCS_CMD(0x45, 0x00), > > + _INIT_DCS_CMD(0x46, 0x17), > > + _INIT_DCS_CMD(0x47, 0x17), > > + _INIT_DCS_CMD(0x48, 0x1F), > > + _INIT_DCS_CMD(0x49, 0x0C), > > + _INIT_DCS_CMD(0x4A, 0x0E), > > + _INIT_DCS_CMD(0x4B, 0x1F), > > + _INIT_DCS_CMD(0x4C, 0x04), > > + _INIT_DCS_CMD(0x4D, 0x06), > > + _INIT_DCS_CMD(0x4E, 0x1F), > > + _INIT_DCS_CMD(0x4F, 0x08), > > + _INIT_DCS_CMD(0x50, 0x0A), > > + _INIT_DCS_CMD(0x51, 0x10), > > + _INIT_DCS_CMD(0x52, 0x1F), > > + _INIT_DCS_CMD(0x53, 0x1F), > > + _INIT_DCS_CMD(0x54, 0x1F), > > + _INIT_DCS_CMD(0x55, 0x1F), > > + _INIT_DCS_CMD(0x56, 0x1F), > > + _INIT_DCS_CMD(0x57, 0x1F), > > + _INIT_DCS_CMD(0x58, 0x40), > > + _INIT_DCS_CMD(0x5B, 0x10), > > + _INIT_DCS_CMD(0x5C, 0x06), > > + _INIT_DCS_CMD(0x5D, 0x40), > > + _INIT_DCS_CMD(0x5E, 0x00), > > + _INIT_DCS_CMD(0x5F, 0x00), > > + _INIT_DCS_CMD(0x60, 0x40), > > + _INIT_DCS_CMD(0x61, 0x03), > > + _INIT_DCS_CMD(0x62, 0x04), > > + _INIT_DCS_CMD(0x63, 0x6C), > > + _INIT_DCS_CMD(0x64, 0x6C), > > + _INIT_DCS_CMD(0x65, 0x75), > > + _INIT_DCS_CMD(0x66, 0x08), > > + _INIT_DCS_CMD(0x67, 0xB4), > > + _INIT_DCS_CMD(0x68, 0x08), > > + _INIT_DCS_CMD(0x69, 0x6C), > > + _INIT_DCS_CMD(0x6A, 0x6C), > > + _INIT_DCS_CMD(0x6B, 0x0C), > > + _INIT_DCS_CMD(0x6D, 0x00), > > + _INIT_DCS_CMD(0x6E, 0x00), > > + _INIT_DCS_CMD(0x6F, 0x88), > > + _INIT_DCS_CMD(0x75, 0xBB), > > + _INIT_DCS_CMD(0x76, 0x00), > > + _INIT_DCS_CMD(0x77, 0x05), > > + _INIT_DCS_CMD(0x78, 0x2A), > > + _INIT_DCS_CMD(0xE0, 0x04), > > + _INIT_DCS_CMD(0x00, 0x0E), > > + _INIT_DCS_CMD(0x02, 0xB3), > > + _INIT_DCS_CMD(0x09, 0x61), > > + _INIT_DCS_CMD(0x0E, 0x48), > > + > > + _INIT_DCS_CMD(0xE0, 0x00), > > + _INIT_DCS_CMD(0X11), > > + /* T6: 120ms */ > > + _INIT_DELAY_CMD(120), > > + _INIT_DCS_CMD(0X29), > > + _INIT_DELAY_CMD(20), > > + {}, > > +}; > > + > > +static inline struct kingdisplay_panel *to_kingdisplay_panel(struct drm_panel *panel) > > +{ > > + return container_of(panel, struct kingdisplay_panel, base); > > +} > > + > > +static int kingdisplay_panel_init_dcs_cmd(struct kingdisplay_panel *kingdisplay) > > +{ > > + struct mipi_dsi_device *dsi = kingdisplay->dsi; > > + struct drm_panel *panel = &kingdisplay->base; > > + int i, err = 0; > > + > > + if (kingdisplay->desc->init_cmds) { > > + const struct panel_init_cmd *init_cmds = kingdisplay->desc->init_cmds; > > + > > + for (i = 0; init_cmds[i].len != 0; i++) { > > + const struct panel_init_cmd *cmd = &init_cmds[i]; > > + > > + switch (cmd->type) { > > + case DELAY_CMD: > > + msleep(cmd->data[0]); > > + err = 0; > > + break; > > + > > + case INIT_DCS_CMD: > > + err = mipi_dsi_dcs_write(dsi, cmd->data[0], > > + cmd->len <= 1 ? NULL : > > + &cmd->data[1], > > + cmd->len - 1); > > + break; > > + > > + default: > > + err = -EINVAL; > > + } > > + > > + if (err < 0) { > > + dev_err(panel->dev, > > + "failed to write command %u\n", i); > > + return err; > > + } > > + } > > + } > > + return 0; > > +} > > + > > +static int kingdisplay_panel_enter_sleep_mode(struct kingdisplay_panel *kingdisplay) > > +{ > > + struct mipi_dsi_device *dsi = kingdisplay->dsi; > > + int ret; > > + > > + dsi->mode_flags &= ~MIPI_DSI_MODE_LPM; > > + > > + usleep_range(1000, 2000); > > + ret = mipi_dsi_dcs_set_display_off(dsi); > > + if (ret < 0) > > + return ret; > > + > > + msleep(50); > > + > > + ret = mipi_dsi_dcs_enter_sleep_mode(dsi); > > + if (ret < 0) > > + return ret; > > + > > + return 0; > > +} > > + > > +static int kingdisplay_panel_disable(struct drm_panel *panel) > > +{ > > + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel); > > + int ret; > > + > > + ret = kingdisplay_panel_enter_sleep_mode(kingdisplay); > > + if (ret < 0) { > > + dev_err(panel->dev, "failed to set panel off: %d\n", ret); > > + return ret; > > + } > > + > > + msleep(100); > > + > > + return 0; > > +} > > + > > +static int kingdisplay_panel_unprepare(struct drm_panel *panel) > > +{ > > + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel); > > + int err; > > + > > + gpiod_set_value_cansleep(kingdisplay->enable_gpio, 0); > > + > > + /* T15: 2ms */ > > + usleep_range(1000, 2000); > > + > > + err = regulator_disable(kingdisplay->pp3300); > > + if (err < 0) > > + return err; > > + > > + return 0; > > +} > > + > > +static int kingdisplay_panel_prepare(struct drm_panel *panel) > > +{ > > + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel); > > + int ret; > > + > > + gpiod_set_value(kingdisplay->enable_gpio, 0); > > + > > + ret = regulator_enable(kingdisplay->pp3300); > > + if (ret < 0) > > + return ret; > > + > > + /* T1: 5ms */ > > + usleep_range(5000, 6000); > > + > > + if (kingdisplay->desc->lp11_before_reset) { > > + mipi_dsi_dcs_nop(kingdisplay->dsi); > > + usleep_range(1000, 2000); > > + } > > + > > + /* T2: 10ms, T1 + T2 > 5ms*/ > > + usleep_range(10000, 11000); > > + > > + gpiod_set_value_cansleep(kingdisplay->enable_gpio, 1); > > + > > + ret = kingdisplay_panel_init_dcs_cmd(kingdisplay); > > + if (ret < 0) { > > + dev_err(panel->dev, "failed to init panel: %d\n", ret); > > + goto poweroff; > > + } > > + > > + return 0; > > + > > +poweroff: > > + regulator_disable(kingdisplay->pp3300); > > + /* T6: 2ms */ > > + usleep_range(1000, 2000); > > + gpiod_set_value(kingdisplay->enable_gpio, 0); > > + > > + return ret; > > +} > > + > > +static int kingdisplay_panel_enable(struct drm_panel *panel) > > +{ > > + msleep(130); > > + return 0; > > +} > > + > > +static const struct drm_display_mode kingdisplay_kd101ne3_40ti_default_mode = { > > + .clock = 70595, > > + .hdisplay = 800, > > + .hsync_start = 800 + 30, > > + .hsync_end = 800 + 30 + 30, > > + .htotal = 800 + 30 + 30 + 30, > > + .vdisplay = 1280, > > + .vsync_start = 1280 + 30, > > + .vsync_end = 1280 + 30 + 4, > > + .vtotal = 1280 + 30 + 4 + 8, > > + .type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED, > > +}; > > + > > +static const struct panel_desc kingdisplay_kd101ne3_40ti_desc = { > > + .modes = &kingdisplay_kd101ne3_40ti_default_mode, > > + .bpc = 8, > > + .size = { > > + .width_mm = 135, > > + .height_mm = 216, > > + }, > > + .lanes = 4, > > + .format = MIPI_DSI_FMT_RGB888, > > + .mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE | > > + MIPI_DSI_MODE_LPM, > > + .init_cmds = kingdisplay_kd101ne3_init_cmd, > > + .lp11_before_reset = true, > > +}; > > + > > +static int kingdisplay_panel_get_modes(struct drm_panel *panel, > > + struct drm_connector *connector) > > +{ > > + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel); > > + const struct drm_display_mode *m = kingdisplay->desc->modes; > > + struct drm_display_mode *mode; > > + > > + mode = drm_mode_duplicate(connector->dev, m); > > + if (!mode) { > > + dev_err(panel->dev, "failed to add mode %ux%u@%u\n", > > + m->hdisplay, m->vdisplay, drm_mode_vrefresh(m)); > > + return -ENOMEM; > > + } > > + > > + mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED; > > + drm_mode_set_name(mode); > > + drm_mode_probed_add(connector, mode); > > + > > + connector->display_info.width_mm = kingdisplay->desc->size.width_mm; > > + connector->display_info.height_mm = kingdisplay->desc->size.height_mm; > > Please use drm_connector_helper_get_modes_fixed() > > > + connector->display_info.bpc = kingdisplay->desc->bpc; > > + /* > > + * TODO: Remove once all drm drivers call > > + * drm_connector_set_orientation_from_panel() > > What is your usecase / platform? Are you using drm_bridge_connector? If not, why? > > > + */ > > + drm_connector_set_panel_orientation(connector, kingdisplay->orientation); > > + > > + return 1; > > +} > > + > > +static enum drm_panel_orientation kingdisplay_panel_get_orientation(struct drm_panel *panel) > > +{ > > + struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel); > > + > > + return kingdisplay->orientation; > > +} > > + > > +static const struct drm_panel_funcs kingdisplay_panel_funcs = { > > + .disable = kingdisplay_panel_disable, > > + .unprepare = kingdisplay_panel_unprepare, > > + .prepare = kingdisplay_panel_prepare, > > + .enable = kingdisplay_panel_enable, > > + .get_modes = kingdisplay_panel_get_modes, > > + .get_orientation = kingdisplay_panel_get_orientation, > > +}; > > + > > +static int kingdisplay_panel_add(struct kingdisplay_panel *kingdisplay) > > +{ > > + struct device *dev = &kingdisplay->dsi->dev; > > + int err; > > + > > + kingdisplay->pp3300 = devm_regulator_get(dev, "pp3300"); > > + if (IS_ERR(kingdisplay->pp3300)) > > + return PTR_ERR(kingdisplay->pp3300); > > + > > + > > + kingdisplay->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW); > > + if (IS_ERR(kingdisplay->enable_gpio)) { > > + dev_err(dev, "cannot get reset-gpios %ld\n", > > + PTR_ERR(kingdisplay->enable_gpio)); > > + return PTR_ERR(kingdisplay->enable_gpio); > > + } > > + > > + gpiod_set_value(kingdisplay->enable_gpio, 0); > > + > > + drm_panel_init(&kingdisplay->base, dev, &kingdisplay_panel_funcs, > > + DRM_MODE_CONNECTOR_DSI); > > + err = of_drm_get_panel_orientation(dev->of_node, &kingdisplay->orientation); > > + if (err < 0) { > > + dev_err(dev, "%pOF: failed to get orientation %d\n", dev->of_node, err); > > + return err; > > + } > > + > > + err = drm_panel_of_backlight(&kingdisplay->base); > > + if (err) > > + return err; > > + > > + kingdisplay->base.funcs = &kingdisplay_panel_funcs; > > + kingdisplay->base.dev = &kingdisplay->dsi->dev; > > + > > + drm_panel_add(&kingdisplay->base); > > + > > + return 0; > > +} > > + > > +static int kingdisplay_panel_probe(struct mipi_dsi_device *dsi) > > +{ > > + struct kingdisplay_panel *kingdisplay; > > + int ret; > > + const struct panel_desc *desc; > > + > > + kingdisplay = devm_kzalloc(&dsi->dev, sizeof(*kingdisplay), GFP_KERNEL); > > + if (!kingdisplay) > > + return -ENOMEM; > > + > > + desc = of_device_get_match_data(&dsi->dev); > > + dsi->lanes = desc->lanes; > > + dsi->format = desc->format; > > + dsi->mode_flags = desc->mode_flags; > > + kingdisplay->desc = desc; > > + kingdisplay->dsi = dsi; > > + ret = kingdisplay_panel_add(kingdisplay); > > + if (ret < 0) > > + return ret; > > + > > + mipi_dsi_set_drvdata(dsi, kingdisplay); > > + > > + ret = mipi_dsi_attach(dsi); > > + if (ret) > > + drm_panel_remove(&kingdisplay->base); > > + > > + return ret; > > +} > > + > > +static void kingdisplay_panel_shutdown(struct mipi_dsi_device *dsi) > > +{ > > + struct kingdisplay_panel *kingdisplay = mipi_dsi_get_drvdata(dsi); > > + > > + drm_panel_disable(&kingdisplay->base); > > + drm_panel_unprepare(&kingdisplay->base); > > +} > > + > > +static void kingdisplay_panel_remove(struct mipi_dsi_device *dsi) > > +{ > > + struct kingdisplay_panel *kingdisplay = mipi_dsi_get_drvdata(dsi); > > + int ret; > > + > > + kingdisplay_panel_shutdown(dsi); > > + > > + ret = mipi_dsi_detach(dsi); > > + if (ret < 0) > > + dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret); > > + > > + if (kingdisplay->base.dev) > > + drm_panel_remove(&kingdisplay->base); > > You are adding it unconditionally, so there should be no condition on > removal of the panel. > > > +} > > + > > +static const struct of_device_id kingdisplay_of_match[] = { > > + { .compatible = "kingdisplay,kd101ne3-40ti", > > + .data = &kingdisplay_kd101ne3_40ti_desc > > + }, > > + { /* sentinel */ } > > +}; > > +MODULE_DEVICE_TABLE(of, kingdisplay_of_match); > > + > > +static struct mipi_dsi_driver kingdisplay_panel_driver = { > > + .driver = { > > + .name = "panel-kingdisplay-kd101ne3", > > + .of_match_table = kingdisplay_of_match, > > + }, > > + .probe = kingdisplay_panel_probe, > > + .remove = kingdisplay_panel_remove, > > + .shutdown = kingdisplay_panel_shutdown, > > +}; > > +module_mipi_dsi_driver(kingdisplay_panel_driver); > > + > > +MODULE_AUTHOR("Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com>"); > > +MODULE_DESCRIPTION("kingdisplay kd101ne3-40ti 800x1280 video mode panel driver"); > > +MODULE_LICENSE("GPL v2"); > > -- > > 2.17.1 > > > > -- > With best wishes > Dmitry ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver 2024-04-18 13:11 ` Hsin-Yi Wang @ 2024-04-18 14:11 ` Dmitry Baryshkov 2024-04-23 18:10 ` Hsin-Yi Wang 0 siblings, 1 reply; 19+ messages in thread From: Dmitry Baryshkov @ 2024-04-18 14:11 UTC (permalink / raw To: Hsin-Yi Wang Cc: lvzhaoxiong, mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt, dianders, dri-devel, devicetree, linux-kernel On Thu, Apr 18, 2024 at 09:11:37PM +0800, Hsin-Yi Wang wrote: > On Thu, Apr 18, 2024 at 7:46 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Thu, Apr 18, 2024 at 04:15:48PM +0800, lvzhaoxiong wrote: > > > The kingdisplay panel is based on JD9365DA controller. > > > Add a driver for it. > > > > > > Signed-off-by: lvzhaoxiong <lvzhaoxiong@huaqin.corp-partner.google.com> > > > --- > > > drivers/gpu/drm/panel/Kconfig | 9 + > > > drivers/gpu/drm/panel/Makefile | 1 + > > > .../drm/panel/panel-kingdisplay-kd101ne3.c | 607 ++++++++++++++++++ > > > 3 files changed, 617 insertions(+) > > > create mode 100644 drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c > > > > > > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig > > > index 154f5bf82980..2c73086cf102 100644 > > > --- a/drivers/gpu/drm/panel/Kconfig > > > +++ b/drivers/gpu/drm/panel/Kconfig > > > @@ -297,6 +297,15 @@ config DRM_PANEL_KINGDISPLAY_KD097D04 > > > 24 bit RGB per pixel. It provides a MIPI DSI interface to > > > the host and has a built-in LED backlight. > > > > > > +config DRM_PANEL_KINGDISPLAY_KD101NE3 > > > + tristate "Kingdisplay kd101ne3 panel" > > > + depends on OF > > > + depends on DRM_MIPI_DSI > > > + depends on BACKLIGHT_CLASS_DEVICE > > > + help > > > + Say Y if you want to enable support for panels based on the > > > + Kingdisplay kd101ne3 controller. > > > + > > > config DRM_PANEL_LEADTEK_LTK050H3146W > > > tristate "Leadtek LTK050H3146W panel" > > > depends on OF > > > diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile > > > index 24a02655d726..cbd414b98bb0 100644 > > > --- a/drivers/gpu/drm/panel/Makefile > > > +++ b/drivers/gpu/drm/panel/Makefile > > > @@ -30,6 +30,7 @@ obj-$(CONFIG_DRM_PANEL_JDI_LPM102A188A) += panel-jdi-lpm102a188a.o > > > obj-$(CONFIG_DRM_PANEL_JDI_R63452) += panel-jdi-fhd-r63452.o > > > obj-$(CONFIG_DRM_PANEL_KHADAS_TS050) += panel-khadas-ts050.o > > > obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD097D04) += panel-kingdisplay-kd097d04.o > > > +obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD101NE3) += panel-kingdisplay-kd101ne3.o > > > obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK050H3146W) += panel-leadtek-ltk050h3146w.o > > > obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK500HD1829) += panel-leadtek-ltk500hd1829.o > > > obj-$(CONFIG_DRM_PANEL_LG_LB035Q02) += panel-lg-lb035q02.o > > > diff --git a/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c b/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c > > > new file mode 100644 > > > index 000000000000..dbf0992f8b81 > > > --- /dev/null > > > +++ b/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c > > > @@ -0,0 +1,607 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Panels based on the JD9365DA display controller. > > > + * Author: Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com> > > > + */ > > > + > > > +#include <linux/delay.h> > > > +#include <linux/gpio/consumer.h> > > > +#include <linux/module.h> > > > +#include <linux/of.h> > > > +#include <linux/of_device.h> > > > +#include <linux/regulator/consumer.h> > > > + > > > +#include <drm/drm_connector.h> > > > +#include <drm/drm_crtc.h> > > > +#include <drm/drm_mipi_dsi.h> > > > +#include <drm/drm_panel.h> > > > + > > > +#include <video/mipi_display.h> > > > + > > > +struct panel_desc { > > > + const struct drm_display_mode *modes; > > > + unsigned int bpc; > > > + > > > + /** > > > + * @width_mm: width of the panel's active display area > > > + * @height_mm: height of the panel's active display area > > > + */ > > > + struct { > > > + unsigned int width_mm; > > > + unsigned int height_mm; > > > > Please move to the declared mode; > > > > > + } size; > > > + > > > + unsigned long mode_flags; > > > + enum mipi_dsi_pixel_format format; > > > + const struct panel_init_cmd *init_cmds; > > > + unsigned int lanes; > > > + bool discharge_on_disable; > > > + bool lp11_before_reset; > > > +}; > > > + > > > +struct kingdisplay_panel { > > > + struct drm_panel base; > > > + struct mipi_dsi_device *dsi; > > > + > > > + const struct panel_desc *desc; > > > + > > > + enum drm_panel_orientation orientation; > > > + struct regulator *pp3300; > > > + struct gpio_desc *enable_gpio; > > > +}; > > > + > > > +enum dsi_cmd_type { > > > + INIT_DCS_CMD, > > > + DELAY_CMD, > > > +}; > > > + > > > +struct panel_init_cmd { > > > + enum dsi_cmd_type type; > > > + size_t len; > > > + const char *data; > > > +}; > > > + > > > +#define _INIT_DCS_CMD(...) { \ > > > + .type = INIT_DCS_CMD, \ > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > + .data = (char[]){__VA_ARGS__} } > > > + > > > +#define _INIT_DELAY_CMD(...) { \ > > > + .type = DELAY_CMD,\ > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > + .data = (char[]){__VA_ARGS__} } > > > > This is the third panel driver using the same appoach. Can you use > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer > > the table, we should extract this framework to a common helper. > > (my preference is shifted towards mipi_dsi_generic_write_seq()). > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the > kernel size grows a lot since every sequence will be expanded. > > Similar discussion in here: > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/ > > This patch would increase the module size from 157K to 572K. > scripts/bloat-o-meter shows chg +235.95%. > > So maybe the common helper is better regarding the kernel module size? Yes, let's get a framework done in a useful way. I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be used instead (and it's up to the developer to select correct delay function). > > > > + > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = { > > > + _INIT_DELAY_CMD(50), > > > + _INIT_DCS_CMD(0xE0, 0x00), [skipped the body of the table] > > > + _INIT_DCS_CMD(0x0E, 0x48), > > > + > > > + _INIT_DCS_CMD(0xE0, 0x00), > > > + _INIT_DCS_CMD(0X11), Also, at least this is mipi_dsi_dcs_exit_sleep_mode(). > > > + /* T6: 120ms */ > > > + _INIT_DELAY_CMD(120), > > > + _INIT_DCS_CMD(0X29), And this is mipi_dsi_dcs_set_display_on(). Having a single table enourages people to put known commands into the table, the practice that must be frowned upon and forbidden. We have functions for some of the standard DCS commands. So, maybe instead of adding a single-table based approach we can improve mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the error handling to a common part of enable() / prepare() function. > > > + _INIT_DELAY_CMD(20), > > > + {}, > > > +}; -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver 2024-04-18 14:11 ` Dmitry Baryshkov @ 2024-04-23 18:10 ` Hsin-Yi Wang 2024-04-23 20:41 ` Doug Anderson 0 siblings, 1 reply; 19+ messages in thread From: Hsin-Yi Wang @ 2024-04-23 18:10 UTC (permalink / raw To: Dmitry Baryshkov Cc: lvzhaoxiong, mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt, dianders, dri-devel, devicetree, linux-kernel On Thu, Apr 18, 2024 at 7:11 AM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Thu, Apr 18, 2024 at 09:11:37PM +0800, Hsin-Yi Wang wrote: > > On Thu, Apr 18, 2024 at 7:46 PM Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > On Thu, Apr 18, 2024 at 04:15:48PM +0800, lvzhaoxiong wrote: > > > > The kingdisplay panel is based on JD9365DA controller. > > > > Add a driver for it. > > > > > > > > Signed-off-by: lvzhaoxiong <lvzhaoxiong@huaqin.corp-partner.google.com> > > > > --- > > > > drivers/gpu/drm/panel/Kconfig | 9 + > > > > drivers/gpu/drm/panel/Makefile | 1 + > > > > .../drm/panel/panel-kingdisplay-kd101ne3.c | 607 ++++++++++++++++++ > > > > 3 files changed, 617 insertions(+) > > > > create mode 100644 drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c > > > > > > > > diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig > > > > index 154f5bf82980..2c73086cf102 100644 > > > > --- a/drivers/gpu/drm/panel/Kconfig > > > > +++ b/drivers/gpu/drm/panel/Kconfig > > > > @@ -297,6 +297,15 @@ config DRM_PANEL_KINGDISPLAY_KD097D04 > > > > 24 bit RGB per pixel. It provides a MIPI DSI interface to > > > > the host and has a built-in LED backlight. > > > > > > > > +config DRM_PANEL_KINGDISPLAY_KD101NE3 > > > > + tristate "Kingdisplay kd101ne3 panel" > > > > + depends on OF > > > > + depends on DRM_MIPI_DSI > > > > + depends on BACKLIGHT_CLASS_DEVICE > > > > + help > > > > + Say Y if you want to enable support for panels based on the > > > > + Kingdisplay kd101ne3 controller. > > > > + > > > > config DRM_PANEL_LEADTEK_LTK050H3146W > > > > tristate "Leadtek LTK050H3146W panel" > > > > depends on OF > > > > diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile > > > > index 24a02655d726..cbd414b98bb0 100644 > > > > --- a/drivers/gpu/drm/panel/Makefile > > > > +++ b/drivers/gpu/drm/panel/Makefile > > > > @@ -30,6 +30,7 @@ obj-$(CONFIG_DRM_PANEL_JDI_LPM102A188A) += panel-jdi-lpm102a188a.o > > > > obj-$(CONFIG_DRM_PANEL_JDI_R63452) += panel-jdi-fhd-r63452.o > > > > obj-$(CONFIG_DRM_PANEL_KHADAS_TS050) += panel-khadas-ts050.o > > > > obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD097D04) += panel-kingdisplay-kd097d04.o > > > > +obj-$(CONFIG_DRM_PANEL_KINGDISPLAY_KD101NE3) += panel-kingdisplay-kd101ne3.o > > > > obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK050H3146W) += panel-leadtek-ltk050h3146w.o > > > > obj-$(CONFIG_DRM_PANEL_LEADTEK_LTK500HD1829) += panel-leadtek-ltk500hd1829.o > > > > obj-$(CONFIG_DRM_PANEL_LG_LB035Q02) += panel-lg-lb035q02.o > > > > diff --git a/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c b/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c > > > > new file mode 100644 > > > > index 000000000000..dbf0992f8b81 > > > > --- /dev/null > > > > +++ b/drivers/gpu/drm/panel/panel-kingdisplay-kd101ne3.c > > > > @@ -0,0 +1,607 @@ > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > +/* > > > > + * Panels based on the JD9365DA display controller. > > > > + * Author: Zhaoxiong Lv <lvzhaoxiong@huaqin.corp-partner.google.com> > > > > + */ > > > > + > > > > +#include <linux/delay.h> > > > > +#include <linux/gpio/consumer.h> > > > > +#include <linux/module.h> > > > > +#include <linux/of.h> > > > > +#include <linux/of_device.h> > > > > +#include <linux/regulator/consumer.h> > > > > + > > > > +#include <drm/drm_connector.h> > > > > +#include <drm/drm_crtc.h> > > > > +#include <drm/drm_mipi_dsi.h> > > > > +#include <drm/drm_panel.h> > > > > + > > > > +#include <video/mipi_display.h> > > > > + > > > > +struct panel_desc { > > > > + const struct drm_display_mode *modes; > > > > + unsigned int bpc; > > > > + > > > > + /** > > > > + * @width_mm: width of the panel's active display area > > > > + * @height_mm: height of the panel's active display area > > > > + */ > > > > + struct { > > > > + unsigned int width_mm; > > > > + unsigned int height_mm; > > > > > > Please move to the declared mode; > > > > > > > + } size; > > > > + > > > > + unsigned long mode_flags; > > > > + enum mipi_dsi_pixel_format format; > > > > + const struct panel_init_cmd *init_cmds; > > > > + unsigned int lanes; > > > > + bool discharge_on_disable; > > > > + bool lp11_before_reset; > > > > +}; > > > > + > > > > +struct kingdisplay_panel { > > > > + struct drm_panel base; > > > > + struct mipi_dsi_device *dsi; > > > > + > > > > + const struct panel_desc *desc; > > > > + > > > > + enum drm_panel_orientation orientation; > > > > + struct regulator *pp3300; > > > > + struct gpio_desc *enable_gpio; > > > > +}; > > > > + > > > > +enum dsi_cmd_type { > > > > + INIT_DCS_CMD, > > > > + DELAY_CMD, > > > > +}; > > > > + > > > > +struct panel_init_cmd { > > > > + enum dsi_cmd_type type; > > > > + size_t len; > > > > + const char *data; > > > > +}; > > > > + > > > > +#define _INIT_DCS_CMD(...) { \ > > > > + .type = INIT_DCS_CMD, \ > > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > > + .data = (char[]){__VA_ARGS__} } > > > > + > > > > +#define _INIT_DELAY_CMD(...) { \ > > > > + .type = DELAY_CMD,\ > > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > > + .data = (char[]){__VA_ARGS__} } > > > > > > This is the third panel driver using the same appoach. Can you use > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer > > > the table, we should extract this framework to a common helper. > > > (my preference is shifted towards mipi_dsi_generic_write_seq()). > > > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the > > kernel size grows a lot since every sequence will be expanded. > > > > Similar discussion in here: > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/ > > > > This patch would increase the module size from 157K to 572K. > > scripts/bloat-o-meter shows chg +235.95%. > > > > So maybe the common helper is better regarding the kernel module size? > > Yes, let's get a framework done in a useful way. > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be > used instead (and it's up to the developer to select correct delay > function). > > > > > > > + > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = { > > > > + _INIT_DELAY_CMD(50), > > > > + _INIT_DCS_CMD(0xE0, 0x00), > > [skipped the body of the table] > > > > > + _INIT_DCS_CMD(0x0E, 0x48), > > > > + > > > > + _INIT_DCS_CMD(0xE0, 0x00), > > > > > + _INIT_DCS_CMD(0X11), > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode(). > > > > > + /* T6: 120ms */ > > > > + _INIT_DELAY_CMD(120), > > > > + _INIT_DCS_CMD(0X29), > > And this is mipi_dsi_dcs_set_display_on(). > > Having a single table enourages people to put known commands into the > table, the practice that must be frowned upon and forbidden. > > We have functions for some of the standard DCS commands. So, maybe > instead of adding a single-table based approach we can improve > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the > error handling to a common part of enable() / prepare() function. > For this panel, I think it can also refer to how panel-kingdisplay-kd097d04.c does. Create the table for init cmd data, not what operation to use, and use mipi_dsi_generic_write_seq() when looping through the table. > > > > + _INIT_DELAY_CMD(20), > > > > + {}, > > > > +}; > > -- > With best wishes > Dmitry ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver 2024-04-23 18:10 ` Hsin-Yi Wang @ 2024-04-23 20:41 ` Doug Anderson 2024-04-23 21:20 ` Dmitry Baryshkov 0 siblings, 1 reply; 19+ messages in thread From: Doug Anderson @ 2024-04-23 20:41 UTC (permalink / raw To: Hsin-Yi Wang Cc: Dmitry Baryshkov, lvzhaoxiong, mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt, dri-devel, devicetree, linux-kernel, cong yang Hi, On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang <hsinyi@google.com> wrote: > > > > > > +#define _INIT_DCS_CMD(...) { \ > > > > > + .type = INIT_DCS_CMD, \ > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > > > + .data = (char[]){__VA_ARGS__} } > > > > > + > > > > > +#define _INIT_DELAY_CMD(...) { \ > > > > > + .type = DELAY_CMD,\ > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > > > + .data = (char[]){__VA_ARGS__} } > > > > > > > > This is the third panel driver using the same appoach. Can you use > > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer > > > > the table, we should extract this framework to a common helper. > > > > (my preference is shifted towards mipi_dsi_generic_write_seq()). > > > > > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the > > > kernel size grows a lot since every sequence will be expanded. > > > > > > Similar discussion in here: > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/ > > > > > > This patch would increase the module size from 157K to 572K. > > > scripts/bloat-o-meter shows chg +235.95%. > > > > > > So maybe the common helper is better regarding the kernel module size? > > > > Yes, let's get a framework done in a useful way. > > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be > > used instead (and it's up to the developer to select correct delay > > function). > > > > > > > > > > + > > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = { > > > > > + _INIT_DELAY_CMD(50), > > > > > + _INIT_DCS_CMD(0xE0, 0x00), > > > > [skipped the body of the table] > > > > > > > + _INIT_DCS_CMD(0x0E, 0x48), > > > > > + > > > > > + _INIT_DCS_CMD(0xE0, 0x00), > > > > > > > + _INIT_DCS_CMD(0X11), > > > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode(). > > > > > > > + /* T6: 120ms */ > > > > > + _INIT_DELAY_CMD(120), > > > > > + _INIT_DCS_CMD(0X29), > > > > And this is mipi_dsi_dcs_set_display_on(). > > > > Having a single table enourages people to put known commands into the > > table, the practice that must be frowned upon and forbidden. > > > > We have functions for some of the standard DCS commands. So, maybe > > instead of adding a single-table based approach we can improve > > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the > > error handling to a common part of enable() / prepare() function. > > > > For this panel, I think it can also refer to how > panel-kingdisplay-kd097d04.c does. Create the table for init cmd data, > not what operation to use, and use mipi_dsi_generic_write_seq() when > looping through the table. Even more similar discussion: https://lore.kernel.org/r/CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver 2024-04-23 20:41 ` Doug Anderson @ 2024-04-23 21:20 ` Dmitry Baryshkov 2024-04-24 21:04 ` Doug Anderson 0 siblings, 1 reply; 19+ messages in thread From: Dmitry Baryshkov @ 2024-04-23 21:20 UTC (permalink / raw To: Doug Anderson Cc: Hsin-Yi Wang, lvzhaoxiong, mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt, dri-devel, devicetree, linux-kernel, cong yang On Tue, Apr 23, 2024 at 01:41:59PM -0700, Doug Anderson wrote: > Hi, > > On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang <hsinyi@google.com> wrote: > > > > > > > > +#define _INIT_DCS_CMD(...) { \ > > > > > > + .type = INIT_DCS_CMD, \ > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > > > > + .data = (char[]){__VA_ARGS__} } > > > > > > + > > > > > > +#define _INIT_DELAY_CMD(...) { \ > > > > > > + .type = DELAY_CMD,\ > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > > > > + .data = (char[]){__VA_ARGS__} } > > > > > > > > > > This is the third panel driver using the same appoach. Can you use > > > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer > > > > > the table, we should extract this framework to a common helper. > > > > > (my preference is shifted towards mipi_dsi_generic_write_seq()). > > > > > > > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the > > > > kernel size grows a lot since every sequence will be expanded. > > > > > > > > Similar discussion in here: > > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/ > > > > > > > > This patch would increase the module size from 157K to 572K. > > > > scripts/bloat-o-meter shows chg +235.95%. > > > > > > > > So maybe the common helper is better regarding the kernel module size? > > > > > > Yes, let's get a framework done in a useful way. > > > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be > > > used instead (and it's up to the developer to select correct delay > > > function). > > > > > > > > > > > > > + > > > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = { > > > > > > + _INIT_DELAY_CMD(50), > > > > > > + _INIT_DCS_CMD(0xE0, 0x00), > > > > > > [skipped the body of the table] > > > > > > > > > + _INIT_DCS_CMD(0x0E, 0x48), > > > > > > + > > > > > > + _INIT_DCS_CMD(0xE0, 0x00), > > > > > > > > > + _INIT_DCS_CMD(0X11), > > > > > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode(). > > > > > > > > > + /* T6: 120ms */ > > > > > > + _INIT_DELAY_CMD(120), > > > > > > + _INIT_DCS_CMD(0X29), > > > > > > And this is mipi_dsi_dcs_set_display_on(). > > > > > > Having a single table enourages people to put known commands into the > > > table, the practice that must be frowned upon and forbidden. > > > > > > We have functions for some of the standard DCS commands. So, maybe > > > instead of adding a single-table based approach we can improve > > > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the > > > error handling to a common part of enable() / prepare() function. > > > > > > > For this panel, I think it can also refer to how > > panel-kingdisplay-kd097d04.c does. Create the table for init cmd data, > > not what operation to use, and use mipi_dsi_generic_write_seq() when > > looping through the table. > > Even more similar discussion: > > https://lore.kernel.org/r/CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com It seems I skipped that thread. I'd still suggest a code-based solution compared to table-based one, for the reasons I've outlined before. Having a tables puts a pressure on the developer to put commands there for which we already have a command-specific function. -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver 2024-04-23 21:20 ` Dmitry Baryshkov @ 2024-04-24 21:04 ` Doug Anderson 2024-04-24 21:18 ` Hsin-Yi Wang 2024-04-24 21:49 ` Dmitry Baryshkov 0 siblings, 2 replies; 19+ messages in thread From: Doug Anderson @ 2024-04-24 21:04 UTC (permalink / raw To: Dmitry Baryshkov Cc: Hsin-Yi Wang, lvzhaoxiong, mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt, dri-devel, devicetree, linux-kernel, cong yang Hi, On Tue, Apr 23, 2024 at 2:20 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Tue, Apr 23, 2024 at 01:41:59PM -0700, Doug Anderson wrote: > > Hi, > > > > On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang <hsinyi@google.com> wrote: > > > > > > > > > > +#define _INIT_DCS_CMD(...) { \ > > > > > > > + .type = INIT_DCS_CMD, \ > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > > > > > + .data = (char[]){__VA_ARGS__} } > > > > > > > + > > > > > > > +#define _INIT_DELAY_CMD(...) { \ > > > > > > > + .type = DELAY_CMD,\ > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > > > > > + .data = (char[]){__VA_ARGS__} } > > > > > > > > > > > > This is the third panel driver using the same appoach. Can you use > > > > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer > > > > > > the table, we should extract this framework to a common helper. > > > > > > (my preference is shifted towards mipi_dsi_generic_write_seq()). > > > > > > > > > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the > > > > > kernel size grows a lot since every sequence will be expanded. > > > > > > > > > > Similar discussion in here: > > > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/ > > > > > > > > > > This patch would increase the module size from 157K to 572K. > > > > > scripts/bloat-o-meter shows chg +235.95%. > > > > > > > > > > So maybe the common helper is better regarding the kernel module size? > > > > > > > > Yes, let's get a framework done in a useful way. > > > > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be > > > > used instead (and it's up to the developer to select correct delay > > > > function). > > > > > > > > > > > > > > > > + > > > > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = { > > > > > > > + _INIT_DELAY_CMD(50), > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00), > > > > > > > > [skipped the body of the table] > > > > > > > > > > > + _INIT_DCS_CMD(0x0E, 0x48), > > > > > > > + > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00), > > > > > > > > > > > + _INIT_DCS_CMD(0X11), > > > > > > > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode(). > > > > > > > > > > > + /* T6: 120ms */ > > > > > > > + _INIT_DELAY_CMD(120), > > > > > > > + _INIT_DCS_CMD(0X29), > > > > > > > > And this is mipi_dsi_dcs_set_display_on(). > > > > > > > > Having a single table enourages people to put known commands into the > > > > table, the practice that must be frowned upon and forbidden. > > > > > > > > We have functions for some of the standard DCS commands. So, maybe > > > > instead of adding a single-table based approach we can improve > > > > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the > > > > error handling to a common part of enable() / prepare() function. > > > > > > > > > > For this panel, I think it can also refer to how > > > panel-kingdisplay-kd097d04.c does. Create the table for init cmd data, > > > not what operation to use, and use mipi_dsi_generic_write_seq() when > > > looping through the table. > > > > Even more similar discussion: > > > > https://lore.kernel.org/r/CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com > > It seems I skipped that thread. > > I'd still suggest a code-based solution compared to table-based one, for > the reasons I've outlined before. Having a tables puts a pressure on the > developer to put commands there for which we already have a > command-specific function. The problem is that with these panels that need big init sequences the code based solution is _a lot_ bigger. If it were a few bytes or a 1-2KB then fine, but when Hsin-Yi measured Linus W's attempt to move from a table to code it was 100K difference in code [1]. I would also say that having these long init sequences done as separate commands encourages people to skip checking the return values of each of the transfer functions and I don't love that idea. It would be ideal if these panels didn't need these long init sequences, but I don't have any inside knowledge here saying that they could be removed. So assume we can't get rid of the init sequences it feels like we have to find some way to make the tables work for at least the large chunks of init code and encourage people to make the tables readable... [1] https://lore.kernel.org/r/CAD=FV=UFa_AoJQvUT3BTiRs19WCA2xLVeQOU=+nYu_HaE0_c6Q@mail.gmail.com ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver 2024-04-24 21:04 ` Doug Anderson @ 2024-04-24 21:18 ` Hsin-Yi Wang 2024-04-24 21:49 ` Dmitry Baryshkov 1 sibling, 0 replies; 19+ messages in thread From: Hsin-Yi Wang @ 2024-04-24 21:18 UTC (permalink / raw To: Doug Anderson Cc: Dmitry Baryshkov, lvzhaoxiong, mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt, dri-devel, devicetree, linux-kernel, cong yang On Wed, Apr 24, 2024 at 2:05 PM Doug Anderson <dianders@google.com> wrote: > > Hi, > > On Tue, Apr 23, 2024 at 2:20 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Tue, Apr 23, 2024 at 01:41:59PM -0700, Doug Anderson wrote: > > > Hi, > > > > > > On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang <hsinyi@google.com> wrote: > > > > > > > > > > > > +#define _INIT_DCS_CMD(...) { \ > > > > > > > > + .type = INIT_DCS_CMD, \ > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > > > > > > + .data = (char[]){__VA_ARGS__} } > > > > > > > > + > > > > > > > > +#define _INIT_DELAY_CMD(...) { \ > > > > > > > > + .type = DELAY_CMD,\ > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > > > > > > + .data = (char[]){__VA_ARGS__} } > > > > > > > > > > > > > > This is the third panel driver using the same appoach. Can you use > > > > > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer > > > > > > > the table, we should extract this framework to a common helper. > > > > > > > (my preference is shifted towards mipi_dsi_generic_write_seq()). > > > > > > > > > > > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the > > > > > > kernel size grows a lot since every sequence will be expanded. > > > > > > > > > > > > Similar discussion in here: > > > > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/ > > > > > > > > > > > > This patch would increase the module size from 157K to 572K. > > > > > > scripts/bloat-o-meter shows chg +235.95%. > > > > > > > > > > > > So maybe the common helper is better regarding the kernel module size? > > > > > > > > > > Yes, let's get a framework done in a useful way. > > > > > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be > > > > > used instead (and it's up to the developer to select correct delay > > > > > function). > > > > > > > > > > > > > > > > > > > + > > > > > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = { > > > > > > > > + _INIT_DELAY_CMD(50), > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00), > > > > > > > > > > [skipped the body of the table] > > > > > > > > > > > > > + _INIT_DCS_CMD(0x0E, 0x48), > > > > > > > > + > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00), > > > > > > > > > > > > > + _INIT_DCS_CMD(0X11), > > > > > > > > > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode(). > > > > > > > > > > > > > + /* T6: 120ms */ > > > > > > > > + _INIT_DELAY_CMD(120), > > > > > > > > + _INIT_DCS_CMD(0X29), > > > > > > > > > > And this is mipi_dsi_dcs_set_display_on(). > > > > > > > > > > Having a single table enourages people to put known commands into the > > > > > table, the practice that must be frowned upon and forbidden. > > > > > > > > > > We have functions for some of the standard DCS commands. So, maybe > > > > > instead of adding a single-table based approach we can improve > > > > > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the > > > > > error handling to a common part of enable() / prepare() function. > > > > > > > > > > > > > For this panel, I think it can also refer to how > > > > panel-kingdisplay-kd097d04.c does. Create the table for init cmd data, > > > > not what operation to use, and use mipi_dsi_generic_write_seq() when > > > > looping through the table. > > > > > > Even more similar discussion: > > > > > > https://lore.kernel.org/r/CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com > > > > It seems I skipped that thread. > > > > I'd still suggest a code-based solution compared to table-based one, for > > the reasons I've outlined before. Having a tables puts a pressure on the > > developer to put commands there for which we already have a > > command-specific function. > > The problem is that with these panels that need big init sequences the > code based solution is _a lot_ bigger. If it were a few bytes or a > 1-2KB then fine, but when Hsin-Yi measured Linus W's attempt to move > from a table to code it was 100K difference in code [1]. I would also > say that having these long init sequences done as separate commands > encourages people to skip checking the return values of each of the > transfer functions and I don't love that idea. > > It would be ideal if these panels didn't need these long init > sequences, but I don't have any inside knowledge here saying that they > could be removed. So assume we can't get rid of the init sequences it > feels like we have to find some way to make the tables work for at > least the large chunks of init code and encourage people to make the > tables readable... > For the init sequence of the panel from this patch, using the table approach, we can still use mipi_dsi_generic_write_seq() and not invent new macro or make the code complicated. > > [1] https://lore.kernel.org/r/CAD=FV=UFa_AoJQvUT3BTiRs19WCA2xLVeQOU=+nYu_HaE0_c6Q@mail.gmail.com ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver 2024-04-24 21:04 ` Doug Anderson 2024-04-24 21:18 ` Hsin-Yi Wang @ 2024-04-24 21:49 ` Dmitry Baryshkov 2024-04-24 22:15 ` Hsin-Yi Wang 2024-04-24 22:27 ` Doug Anderson 1 sibling, 2 replies; 19+ messages in thread From: Dmitry Baryshkov @ 2024-04-24 21:49 UTC (permalink / raw To: Doug Anderson Cc: Hsin-Yi Wang, lvzhaoxiong, mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt, dri-devel, devicetree, linux-kernel, cong yang On Thu, 25 Apr 2024 at 00:04, Doug Anderson <dianders@google.com> wrote: > > Hi, > > On Tue, Apr 23, 2024 at 2:20 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Tue, Apr 23, 2024 at 01:41:59PM -0700, Doug Anderson wrote: > > > Hi, > > > > > > On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang <hsinyi@google.com> wrote: > > > > > > > > > > > > +#define _INIT_DCS_CMD(...) { \ > > > > > > > > + .type = INIT_DCS_CMD, \ > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > > > > > > + .data = (char[]){__VA_ARGS__} } > > > > > > > > + > > > > > > > > +#define _INIT_DELAY_CMD(...) { \ > > > > > > > > + .type = DELAY_CMD,\ > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > > > > > > + .data = (char[]){__VA_ARGS__} } > > > > > > > > > > > > > > This is the third panel driver using the same appoach. Can you use > > > > > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer > > > > > > > the table, we should extract this framework to a common helper. > > > > > > > (my preference is shifted towards mipi_dsi_generic_write_seq()). > > > > > > > > > > > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the > > > > > > kernel size grows a lot since every sequence will be expanded. > > > > > > > > > > > > Similar discussion in here: > > > > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/ > > > > > > > > > > > > This patch would increase the module size from 157K to 572K. > > > > > > scripts/bloat-o-meter shows chg +235.95%. > > > > > > > > > > > > So maybe the common helper is better regarding the kernel module size? > > > > > > > > > > Yes, let's get a framework done in a useful way. > > > > > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be > > > > > used instead (and it's up to the developer to select correct delay > > > > > function). > > > > > > > > > > > > > > > > > > > + > > > > > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = { > > > > > > > > + _INIT_DELAY_CMD(50), > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00), > > > > > > > > > > [skipped the body of the table] > > > > > > > > > > > > > + _INIT_DCS_CMD(0x0E, 0x48), > > > > > > > > + > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00), > > > > > > > > > > > > > + _INIT_DCS_CMD(0X11), > > > > > > > > > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode(). > > > > > > > > > > > > > + /* T6: 120ms */ > > > > > > > > + _INIT_DELAY_CMD(120), > > > > > > > > + _INIT_DCS_CMD(0X29), > > > > > > > > > > And this is mipi_dsi_dcs_set_display_on(). > > > > > > > > > > Having a single table enourages people to put known commands into the > > > > > table, the practice that must be frowned upon and forbidden. > > > > > > > > > > We have functions for some of the standard DCS commands. So, maybe > > > > > instead of adding a single-table based approach we can improve > > > > > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the > > > > > error handling to a common part of enable() / prepare() function. > > > > > > > > > > > > > For this panel, I think it can also refer to how > > > > panel-kingdisplay-kd097d04.c does. Create the table for init cmd data, > > > > not what operation to use, and use mipi_dsi_generic_write_seq() when > > > > looping through the table. > > > > > > Even more similar discussion: > > > > > > https://lore.kernel.org/r/CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com > > > > It seems I skipped that thread. > > > > I'd still suggest a code-based solution compared to table-based one, for > > the reasons I've outlined before. Having a tables puts a pressure on the > > developer to put commands there for which we already have a > > command-specific function. > > The problem is that with these panels that need big init sequences the > code based solution is _a lot_ bigger. If it were a few bytes or a > 1-2KB then fine, but when Hsin-Yi measured Linus W's attempt to move > from a table to code it was 100K difference in code [1]. I would also > say that having these long init sequences done as separate commands > encourages people to skip checking the return values of each of the > transfer functions and I don't love that idea. > > It would be ideal if these panels didn't need these long init > sequences, but I don't have any inside knowledge here saying that they > could be removed. So assume we can't get rid of the init sequences it > feels like we have to find some way to make the tables work for at > least the large chunks of init code and encourage people to make the > tables readable... I did a quick check on the boe-tv101wum-nl6 driver by converting the writes to use the following macro: #define mipi_dsi_dcs_write_cmd_seq(dsi, cmd, seq...) \ do { \ static const u8 d[] = { cmd, seq }; \ ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \ if (ret < 0) \ goto err; \ } while (0) And then at the end of the init funciton having err: dev_err(panel->dev, "failed to write command %d\n", ret); return ret; } Size comparison: text data bss dec hex filename before 34109 10410 18 44537 adf9 ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o making init data const 44359 184 0 44543 adff ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o with new macros 44353 184 0 44537 adf9 ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o As you can see, there is literally no size difference with this macro in place. The only drawback is that the init stops on the first write rather than going through the sequence. WDYT? I can turn this into a proper patch if you think this makes sense. > > > [1] https://lore.kernel.org/r/CAD=FV=UFa_AoJQvUT3BTiRs19WCA2xLVeQOU=+nYu_HaE0_c6Q@mail.gmail.com -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver 2024-04-24 21:49 ` Dmitry Baryshkov @ 2024-04-24 22:15 ` Hsin-Yi Wang 2024-04-24 22:50 ` Dmitry Baryshkov 2024-04-24 22:27 ` Doug Anderson 1 sibling, 1 reply; 19+ messages in thread From: Hsin-Yi Wang @ 2024-04-24 22:15 UTC (permalink / raw To: Dmitry Baryshkov Cc: Doug Anderson, lvzhaoxiong, mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt, dri-devel, devicetree, linux-kernel, cong yang On Wed, Apr 24, 2024 at 2:49 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Thu, 25 Apr 2024 at 00:04, Doug Anderson <dianders@google.com> wrote: > > > > Hi, > > > > On Tue, Apr 23, 2024 at 2:20 PM Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > On Tue, Apr 23, 2024 at 01:41:59PM -0700, Doug Anderson wrote: > > > > Hi, > > > > > > > > On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang <hsinyi@google.com> wrote: > > > > > > > > > > > > > > +#define _INIT_DCS_CMD(...) { \ > > > > > > > > > + .type = INIT_DCS_CMD, \ > > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > > > > > > > + .data = (char[]){__VA_ARGS__} } > > > > > > > > > + > > > > > > > > > +#define _INIT_DELAY_CMD(...) { \ > > > > > > > > > + .type = DELAY_CMD,\ > > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > > > > > > > + .data = (char[]){__VA_ARGS__} } > > > > > > > > > > > > > > > > This is the third panel driver using the same appoach. Can you use > > > > > > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer > > > > > > > > the table, we should extract this framework to a common helper. > > > > > > > > (my preference is shifted towards mipi_dsi_generic_write_seq()). > > > > > > > > > > > > > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the > > > > > > > kernel size grows a lot since every sequence will be expanded. > > > > > > > > > > > > > > Similar discussion in here: > > > > > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/ > > > > > > > > > > > > > > This patch would increase the module size from 157K to 572K. > > > > > > > scripts/bloat-o-meter shows chg +235.95%. > > > > > > > > > > > > > > So maybe the common helper is better regarding the kernel module size? > > > > > > > > > > > > Yes, let's get a framework done in a useful way. > > > > > > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be > > > > > > used instead (and it's up to the developer to select correct delay > > > > > > function). > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = { > > > > > > > > > + _INIT_DELAY_CMD(50), > > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00), > > > > > > > > > > > > [skipped the body of the table] > > > > > > > > > > > > > > > + _INIT_DCS_CMD(0x0E, 0x48), > > > > > > > > > + > > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00), > > > > > > > > > > > > > > > + _INIT_DCS_CMD(0X11), > > > > > > > > > > > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode(). > > > > > > > > > > > > > > > + /* T6: 120ms */ > > > > > > > > > + _INIT_DELAY_CMD(120), > > > > > > > > > + _INIT_DCS_CMD(0X29), > > > > > > > > > > > > And this is mipi_dsi_dcs_set_display_on(). > > > > > > > > > > > > Having a single table enourages people to put known commands into the > > > > > > table, the practice that must be frowned upon and forbidden. > > > > > > > > > > > > We have functions for some of the standard DCS commands. So, maybe > > > > > > instead of adding a single-table based approach we can improve > > > > > > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the > > > > > > error handling to a common part of enable() / prepare() function. > > > > > > > > > > > > > > > > For this panel, I think it can also refer to how > > > > > panel-kingdisplay-kd097d04.c does. Create the table for init cmd data, > > > > > not what operation to use, and use mipi_dsi_generic_write_seq() when > > > > > looping through the table. > > > > > > > > Even more similar discussion: > > > > > > > > https://lore.kernel.org/r/CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com > > > > > > It seems I skipped that thread. > > > > > > I'd still suggest a code-based solution compared to table-based one, for > > > the reasons I've outlined before. Having a tables puts a pressure on the > > > developer to put commands there for which we already have a > > > command-specific function. > > > > The problem is that with these panels that need big init sequences the > > code based solution is _a lot_ bigger. If it were a few bytes or a > > 1-2KB then fine, but when Hsin-Yi measured Linus W's attempt to move > > from a table to code it was 100K difference in code [1]. I would also > > say that having these long init sequences done as separate commands > > encourages people to skip checking the return values of each of the > > transfer functions and I don't love that idea. > > > > It would be ideal if these panels didn't need these long init > > sequences, but I don't have any inside knowledge here saying that they > > could be removed. So assume we can't get rid of the init sequences it > > feels like we have to find some way to make the tables work for at > > least the large chunks of init code and encourage people to make the > > tables readable... > > > I did a quick check on the boe-tv101wum-nl6 driver by converting the > writes to use the following macro: > > #define mipi_dsi_dcs_write_cmd_seq(dsi, cmd, seq...) > \ > do { \ > static const u8 d[] = { cmd, seq }; \ > ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \ > if (ret < 0) \ > goto err; \ > } while (0) > > And then at the end of the init funciton having > > err: > dev_err(panel->dev, > "failed to write command %d\n", ret); > return ret; > } > I'm not sure about the coding style rule here, would it be considered unclear that caller of mipi_dsi_dcs_write_cmd_seq() needs to have err block, but the block may not be directly used in that caller and is only jumped from the macro? > Size comparison: > text data bss dec hex filename > before > 34109 10410 18 44537 adf9 > ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o > making init data const > 44359 184 0 44543 adff > ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o > with new macros > 44353 184 0 44537 adf9 > ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o > > As you can see, there is literally no size difference with this macro in place. > The only drawback is that the init stops on the first write rather > than going through the sequence. > > WDYT? I can turn this into a proper patch if you think this makes sense. > > > > > > > [1] https://lore.kernel.org/r/CAD=FV=UFa_AoJQvUT3BTiRs19WCA2xLVeQOU=+nYu_HaE0_c6Q@mail.gmail.com > > > > -- > With best wishes > Dmitry ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver 2024-04-24 22:15 ` Hsin-Yi Wang @ 2024-04-24 22:50 ` Dmitry Baryshkov 2024-04-24 23:25 ` Doug Anderson 0 siblings, 1 reply; 19+ messages in thread From: Dmitry Baryshkov @ 2024-04-24 22:50 UTC (permalink / raw To: Hsin-Yi Wang Cc: Doug Anderson, lvzhaoxiong, mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt, dri-devel, devicetree, linux-kernel, cong yang On Thu, 25 Apr 2024 at 01:15, Hsin-Yi Wang <hsinyi@google.com> wrote: > > On Wed, Apr 24, 2024 at 2:49 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Thu, 25 Apr 2024 at 00:04, Doug Anderson <dianders@google.com> wrote: > > > > > > Hi, > > > > > > On Tue, Apr 23, 2024 at 2:20 PM Dmitry Baryshkov > > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > > > On Tue, Apr 23, 2024 at 01:41:59PM -0700, Doug Anderson wrote: > > > > > Hi, > > > > > > > > > > On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang <hsinyi@google.com> wrote: > > > > > > > > > > > > > > > > +#define _INIT_DCS_CMD(...) { \ > > > > > > > > > > + .type = INIT_DCS_CMD, \ > > > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > > > > > > > > + .data = (char[]){__VA_ARGS__} } > > > > > > > > > > + > > > > > > > > > > +#define _INIT_DELAY_CMD(...) { \ > > > > > > > > > > + .type = DELAY_CMD,\ > > > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > > > > > > > > + .data = (char[]){__VA_ARGS__} } > > > > > > > > > > > > > > > > > > This is the third panel driver using the same appoach. Can you use > > > > > > > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer > > > > > > > > > the table, we should extract this framework to a common helper. > > > > > > > > > (my preference is shifted towards mipi_dsi_generic_write_seq()). > > > > > > > > > > > > > > > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the > > > > > > > > kernel size grows a lot since every sequence will be expanded. > > > > > > > > > > > > > > > > Similar discussion in here: > > > > > > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/ > > > > > > > > > > > > > > > > This patch would increase the module size from 157K to 572K. > > > > > > > > scripts/bloat-o-meter shows chg +235.95%. > > > > > > > > > > > > > > > > So maybe the common helper is better regarding the kernel module size? > > > > > > > > > > > > > > Yes, let's get a framework done in a useful way. > > > > > > > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be > > > > > > > used instead (and it's up to the developer to select correct delay > > > > > > > function). > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = { > > > > > > > > > > + _INIT_DELAY_CMD(50), > > > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00), > > > > > > > > > > > > > > [skipped the body of the table] > > > > > > > > > > > > > > > > > + _INIT_DCS_CMD(0x0E, 0x48), > > > > > > > > > > + > > > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00), > > > > > > > > > > > > > > > > > + _INIT_DCS_CMD(0X11), > > > > > > > > > > > > > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode(). > > > > > > > > > > > > > > > > > + /* T6: 120ms */ > > > > > > > > > > + _INIT_DELAY_CMD(120), > > > > > > > > > > + _INIT_DCS_CMD(0X29), > > > > > > > > > > > > > > And this is mipi_dsi_dcs_set_display_on(). > > > > > > > > > > > > > > Having a single table enourages people to put known commands into the > > > > > > > table, the practice that must be frowned upon and forbidden. > > > > > > > > > > > > > > We have functions for some of the standard DCS commands. So, maybe > > > > > > > instead of adding a single-table based approach we can improve > > > > > > > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the > > > > > > > error handling to a common part of enable() / prepare() function. > > > > > > > > > > > > > > > > > > > For this panel, I think it can also refer to how > > > > > > panel-kingdisplay-kd097d04.c does. Create the table for init cmd data, > > > > > > not what operation to use, and use mipi_dsi_generic_write_seq() when > > > > > > looping through the table. > > > > > > > > > > Even more similar discussion: > > > > > > > > > > https://lore.kernel.org/r/CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com > > > > > > > > It seems I skipped that thread. > > > > > > > > I'd still suggest a code-based solution compared to table-based one, for > > > > the reasons I've outlined before. Having a tables puts a pressure on the > > > > developer to put commands there for which we already have a > > > > command-specific function. > > > > > > The problem is that with these panels that need big init sequences the > > > code based solution is _a lot_ bigger. If it were a few bytes or a > > > 1-2KB then fine, but when Hsin-Yi measured Linus W's attempt to move > > > from a table to code it was 100K difference in code [1]. I would also > > > say that having these long init sequences done as separate commands > > > encourages people to skip checking the return values of each of the > > > transfer functions and I don't love that idea. > > > > > > It would be ideal if these panels didn't need these long init > > > sequences, but I don't have any inside knowledge here saying that they > > > could be removed. So assume we can't get rid of the init sequences it > > > feels like we have to find some way to make the tables work for at > > > least the large chunks of init code and encourage people to make the > > > tables readable... > > > > > > I did a quick check on the boe-tv101wum-nl6 driver by converting the > > writes to use the following macro: > > > > #define mipi_dsi_dcs_write_cmd_seq(dsi, cmd, seq...) > > \ > > do { \ > > static const u8 d[] = { cmd, seq }; \ > > ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \ > > if (ret < 0) \ > > goto err; \ > > } while (0) > > > > And then at the end of the init funciton having > > > > err: > > dev_err(panel->dev, > > "failed to write command %d\n", ret); > > return ret; > > } > > > > I'm not sure about the coding style rule here, would it be considered > unclear that caller of mipi_dsi_dcs_write_cmd_seq() needs to have err > block, but the block may not be directly used in that caller and is > only jumped from the macro? I'm also not sure here. It was a quick and dirty test. We might as well do something like ret = mipi_dsi_dcs_write_cmd_seq(dsi, ...); if (ret) goto err; all over the place. > > > > Size comparison: > > text data bss dec hex filename > > before > > 34109 10410 18 44537 adf9 > > ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o > > making init data const > > 44359 184 0 44543 adff > > ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o > > with new macros > > 44353 184 0 44537 adf9 > > ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o > > > > As you can see, there is literally no size difference with this macro in place. > > The only drawback is that the init stops on the first write rather > > than going through the sequence. > > > > WDYT? I can turn this into a proper patch if you think this makes sense. > > > > > > > > > > > [1] https://lore.kernel.org/r/CAD=FV=UFa_AoJQvUT3BTiRs19WCA2xLVeQOU=+nYu_HaE0_c6Q@mail.gmail.com > > > > > > > > -- > > With best wishes > > Dmitry -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver 2024-04-24 22:50 ` Dmitry Baryshkov @ 2024-04-24 23:25 ` Doug Anderson 2024-04-24 23:37 ` Dmitry Baryshkov 0 siblings, 1 reply; 19+ messages in thread From: Doug Anderson @ 2024-04-24 23:25 UTC (permalink / raw To: Dmitry Baryshkov Cc: Hsin-Yi Wang, lvzhaoxiong, mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt, dri-devel, devicetree, linux-kernel, cong yang Hi, On Wed, Apr 24, 2024 at 3:51 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Thu, 25 Apr 2024 at 01:15, Hsin-Yi Wang <hsinyi@google.com> wrote: > > > > On Wed, Apr 24, 2024 at 2:49 PM Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > On Thu, 25 Apr 2024 at 00:04, Doug Anderson <dianders@google.com> wrote: > > > > > > > > Hi, > > > > > > > > On Tue, Apr 23, 2024 at 2:20 PM Dmitry Baryshkov > > > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > > > > > On Tue, Apr 23, 2024 at 01:41:59PM -0700, Doug Anderson wrote: > > > > > > Hi, > > > > > > > > > > > > On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang <hsinyi@google.com> wrote: > > > > > > > > > > > > > > > > > > +#define _INIT_DCS_CMD(...) { \ > > > > > > > > > > > + .type = INIT_DCS_CMD, \ > > > > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > > > > > > > > > + .data = (char[]){__VA_ARGS__} } > > > > > > > > > > > + > > > > > > > > > > > +#define _INIT_DELAY_CMD(...) { \ > > > > > > > > > > > + .type = DELAY_CMD,\ > > > > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > > > > > > > > > + .data = (char[]){__VA_ARGS__} } > > > > > > > > > > > > > > > > > > > > This is the third panel driver using the same appoach. Can you use > > > > > > > > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer > > > > > > > > > > the table, we should extract this framework to a common helper. > > > > > > > > > > (my preference is shifted towards mipi_dsi_generic_write_seq()). > > > > > > > > > > > > > > > > > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the > > > > > > > > > kernel size grows a lot since every sequence will be expanded. > > > > > > > > > > > > > > > > > > Similar discussion in here: > > > > > > > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/ > > > > > > > > > > > > > > > > > > This patch would increase the module size from 157K to 572K. > > > > > > > > > scripts/bloat-o-meter shows chg +235.95%. > > > > > > > > > > > > > > > > > > So maybe the common helper is better regarding the kernel module size? > > > > > > > > > > > > > > > > Yes, let's get a framework done in a useful way. > > > > > > > > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be > > > > > > > > used instead (and it's up to the developer to select correct delay > > > > > > > > function). > > > > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = { > > > > > > > > > > > + _INIT_DELAY_CMD(50), > > > > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00), > > > > > > > > > > > > > > > > [skipped the body of the table] > > > > > > > > > > > > > > > > > > > + _INIT_DCS_CMD(0x0E, 0x48), > > > > > > > > > > > + > > > > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00), > > > > > > > > > > > > > > > > > > > + _INIT_DCS_CMD(0X11), > > > > > > > > > > > > > > > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode(). > > > > > > > > > > > > > > > > > > > + /* T6: 120ms */ > > > > > > > > > > > + _INIT_DELAY_CMD(120), > > > > > > > > > > > + _INIT_DCS_CMD(0X29), > > > > > > > > > > > > > > > > And this is mipi_dsi_dcs_set_display_on(). > > > > > > > > > > > > > > > > Having a single table enourages people to put known commands into the > > > > > > > > table, the practice that must be frowned upon and forbidden. > > > > > > > > > > > > > > > > We have functions for some of the standard DCS commands. So, maybe > > > > > > > > instead of adding a single-table based approach we can improve > > > > > > > > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the > > > > > > > > error handling to a common part of enable() / prepare() function. > > > > > > > > > > > > > > > > > > > > > > For this panel, I think it can also refer to how > > > > > > > panel-kingdisplay-kd097d04.c does. Create the table for init cmd data, > > > > > > > not what operation to use, and use mipi_dsi_generic_write_seq() when > > > > > > > looping through the table. > > > > > > > > > > > > Even more similar discussion: > > > > > > > > > > > > https://lore.kernel.org/r/CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com > > > > > > > > > > It seems I skipped that thread. > > > > > > > > > > I'd still suggest a code-based solution compared to table-based one, for > > > > > the reasons I've outlined before. Having a tables puts a pressure on the > > > > > developer to put commands there for which we already have a > > > > > command-specific function. > > > > > > > > The problem is that with these panels that need big init sequences the > > > > code based solution is _a lot_ bigger. If it were a few bytes or a > > > > 1-2KB then fine, but when Hsin-Yi measured Linus W's attempt to move > > > > from a table to code it was 100K difference in code [1]. I would also > > > > say that having these long init sequences done as separate commands > > > > encourages people to skip checking the return values of each of the > > > > transfer functions and I don't love that idea. > > > > > > > > It would be ideal if these panels didn't need these long init > > > > sequences, but I don't have any inside knowledge here saying that they > > > > could be removed. So assume we can't get rid of the init sequences it > > > > feels like we have to find some way to make the tables work for at > > > > least the large chunks of init code and encourage people to make the > > > > tables readable... > > > > > > > > > I did a quick check on the boe-tv101wum-nl6 driver by converting the > > > writes to use the following macro: > > > > > > #define mipi_dsi_dcs_write_cmd_seq(dsi, cmd, seq...) > > > \ > > > do { \ > > > static const u8 d[] = { cmd, seq }; \ > > > ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \ > > > if (ret < 0) \ > > > goto err; \ > > > } while (0) > > > > > > And then at the end of the init funciton having > > > > > > err: > > > dev_err(panel->dev, > > > "failed to write command %d\n", ret); > > > return ret; > > > } > > > > > > > I'm not sure about the coding style rule here, would it be considered > > unclear that caller of mipi_dsi_dcs_write_cmd_seq() needs to have err > > block, but the block may not be directly used in that caller and is > > only jumped from the macro? > > I'm also not sure here. It was a quick and dirty test. > We might as well do something like > > ret = mipi_dsi_dcs_write_cmd_seq(dsi, ...); > if (ret) > goto err; > > all over the place. Oh. This is actually very simple to fix and requires no code changes to clients. :-P We just need to hoist the printing out of the macro and into "drm_mipi_dsi.c". Let me double-confirm and then I'll post a patch. -Doug ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver 2024-04-24 23:25 ` Doug Anderson @ 2024-04-24 23:37 ` Dmitry Baryshkov 2024-04-25 0:21 ` Doug Anderson 0 siblings, 1 reply; 19+ messages in thread From: Dmitry Baryshkov @ 2024-04-24 23:37 UTC (permalink / raw To: Doug Anderson Cc: Hsin-Yi Wang, lvzhaoxiong, mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt, dri-devel, devicetree, linux-kernel, cong yang On Thu, 25 Apr 2024 at 02:25, Doug Anderson <dianders@google.com> wrote: > > Hi, > > On Wed, Apr 24, 2024 at 3:51 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Thu, 25 Apr 2024 at 01:15, Hsin-Yi Wang <hsinyi@google.com> wrote: > > > > > > On Wed, Apr 24, 2024 at 2:49 PM Dmitry Baryshkov > > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > > > On Thu, 25 Apr 2024 at 00:04, Doug Anderson <dianders@google.com> wrote: > > > > > > > > > > Hi, > > > > > > > > > > On Tue, Apr 23, 2024 at 2:20 PM Dmitry Baryshkov > > > > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > > > > > > > On Tue, Apr 23, 2024 at 01:41:59PM -0700, Doug Anderson wrote: > > > > > > > Hi, > > > > > > > > > > > > > > On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang <hsinyi@google.com> wrote: > > > > > > > > > > > > > > > > > > > > +#define _INIT_DCS_CMD(...) { \ > > > > > > > > > > > > + .type = INIT_DCS_CMD, \ > > > > > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > > > > > > > > > > + .data = (char[]){__VA_ARGS__} } > > > > > > > > > > > > + > > > > > > > > > > > > +#define _INIT_DELAY_CMD(...) { \ > > > > > > > > > > > > + .type = DELAY_CMD,\ > > > > > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > > > > > > > > > > + .data = (char[]){__VA_ARGS__} } > > > > > > > > > > > > > > > > > > > > > > This is the third panel driver using the same appoach. Can you use > > > > > > > > > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer > > > > > > > > > > > the table, we should extract this framework to a common helper. > > > > > > > > > > > (my preference is shifted towards mipi_dsi_generic_write_seq()). > > > > > > > > > > > > > > > > > > > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the > > > > > > > > > > kernel size grows a lot since every sequence will be expanded. > > > > > > > > > > > > > > > > > > > > Similar discussion in here: > > > > > > > > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/ > > > > > > > > > > > > > > > > > > > > This patch would increase the module size from 157K to 572K. > > > > > > > > > > scripts/bloat-o-meter shows chg +235.95%. > > > > > > > > > > > > > > > > > > > > So maybe the common helper is better regarding the kernel module size? > > > > > > > > > > > > > > > > > > Yes, let's get a framework done in a useful way. > > > > > > > > > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be > > > > > > > > > used instead (and it's up to the developer to select correct delay > > > > > > > > > function). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = { > > > > > > > > > > > > + _INIT_DELAY_CMD(50), > > > > > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00), > > > > > > > > > > > > > > > > > > [skipped the body of the table] > > > > > > > > > > > > > > > > > > > > > + _INIT_DCS_CMD(0x0E, 0x48), > > > > > > > > > > > > + > > > > > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00), > > > > > > > > > > > > > > > > > > > > > + _INIT_DCS_CMD(0X11), > > > > > > > > > > > > > > > > > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode(). > > > > > > > > > > > > > > > > > > > > > + /* T6: 120ms */ > > > > > > > > > > > > + _INIT_DELAY_CMD(120), > > > > > > > > > > > > + _INIT_DCS_CMD(0X29), > > > > > > > > > > > > > > > > > > And this is mipi_dsi_dcs_set_display_on(). > > > > > > > > > > > > > > > > > > Having a single table enourages people to put known commands into the > > > > > > > > > table, the practice that must be frowned upon and forbidden. > > > > > > > > > > > > > > > > > > We have functions for some of the standard DCS commands. So, maybe > > > > > > > > > instead of adding a single-table based approach we can improve > > > > > > > > > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the > > > > > > > > > error handling to a common part of enable() / prepare() function. > > > > > > > > > > > > > > > > > > > > > > > > > For this panel, I think it can also refer to how > > > > > > > > panel-kingdisplay-kd097d04.c does. Create the table for init cmd data, > > > > > > > > not what operation to use, and use mipi_dsi_generic_write_seq() when > > > > > > > > looping through the table. > > > > > > > > > > > > > > Even more similar discussion: > > > > > > > > > > > > > > https://lore.kernel.org/r/CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com > > > > > > > > > > > > It seems I skipped that thread. > > > > > > > > > > > > I'd still suggest a code-based solution compared to table-based one, for > > > > > > the reasons I've outlined before. Having a tables puts a pressure on the > > > > > > developer to put commands there for which we already have a > > > > > > command-specific function. > > > > > > > > > > The problem is that with these panels that need big init sequences the > > > > > code based solution is _a lot_ bigger. If it were a few bytes or a > > > > > 1-2KB then fine, but when Hsin-Yi measured Linus W's attempt to move > > > > > from a table to code it was 100K difference in code [1]. I would also > > > > > say that having these long init sequences done as separate commands > > > > > encourages people to skip checking the return values of each of the > > > > > transfer functions and I don't love that idea. > > > > > > > > > > It would be ideal if these panels didn't need these long init > > > > > sequences, but I don't have any inside knowledge here saying that they > > > > > could be removed. So assume we can't get rid of the init sequences it > > > > > feels like we have to find some way to make the tables work for at > > > > > least the large chunks of init code and encourage people to make the > > > > > tables readable... > > > > > > > > > > > > I did a quick check on the boe-tv101wum-nl6 driver by converting the > > > > writes to use the following macro: > > > > > > > > #define mipi_dsi_dcs_write_cmd_seq(dsi, cmd, seq...) > > > > \ > > > > do { \ > > > > static const u8 d[] = { cmd, seq }; \ > > > > ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \ > > > > if (ret < 0) \ > > > > goto err; \ > > > > } while (0) > > > > > > > > And then at the end of the init funciton having > > > > > > > > err: > > > > dev_err(panel->dev, > > > > "failed to write command %d\n", ret); > > > > return ret; > > > > } > > > > > > > > > > I'm not sure about the coding style rule here, would it be considered > > > unclear that caller of mipi_dsi_dcs_write_cmd_seq() needs to have err > > > block, but the block may not be directly used in that caller and is > > > only jumped from the macro? > > > > I'm also not sure here. It was a quick and dirty test. > > We might as well do something like > > > > ret = mipi_dsi_dcs_write_cmd_seq(dsi, ...); > > if (ret) > > goto err; > > > > all over the place. > > Oh. This is actually very simple to fix and requires no code changes > to clients. :-P We just need to hoist the printing out of the macro > and into "drm_mipi_dsi.c". Let me double-confirm and then I'll post a > patch. Sounds good. I have converted boe-tv101wum-nl6, ilitek-ili9882t and innolux-p079zca drivers. Once you post your patch I can post those too. -- With best wishes Dmitry ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver 2024-04-24 23:37 ` Dmitry Baryshkov @ 2024-04-25 0:21 ` Doug Anderson 0 siblings, 0 replies; 19+ messages in thread From: Doug Anderson @ 2024-04-25 0:21 UTC (permalink / raw To: Dmitry Baryshkov Cc: Hsin-Yi Wang, lvzhaoxiong, mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt, dri-devel, devicetree, linux-kernel, cong yang Hi, On Wed, Apr 24, 2024 at 4:37 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Thu, 25 Apr 2024 at 02:25, Doug Anderson <dianders@google.com> wrote: > > > > Hi, > > > > On Wed, Apr 24, 2024 at 3:51 PM Dmitry Baryshkov > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > On Thu, 25 Apr 2024 at 01:15, Hsin-Yi Wang <hsinyi@google.com> wrote: > > > > > > > > On Wed, Apr 24, 2024 at 2:49 PM Dmitry Baryshkov > > > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > > > > > On Thu, 25 Apr 2024 at 00:04, Doug Anderson <dianders@google.com> wrote: > > > > > > > > > > > > Hi, > > > > > > > > > > > > On Tue, Apr 23, 2024 at 2:20 PM Dmitry Baryshkov > > > > > > <dmitry.baryshkov@linaro.org> wrote: > > > > > > > > > > > > > > On Tue, Apr 23, 2024 at 01:41:59PM -0700, Doug Anderson wrote: > > > > > > > > Hi, > > > > > > > > > > > > > > > > On Tue, Apr 23, 2024 at 11:10 AM Hsin-Yi Wang <hsinyi@google.com> wrote: > > > > > > > > > > > > > > > > > > > > > > +#define _INIT_DCS_CMD(...) { \ > > > > > > > > > > > > > + .type = INIT_DCS_CMD, \ > > > > > > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > > > > > > > > > > > + .data = (char[]){__VA_ARGS__} } > > > > > > > > > > > > > + > > > > > > > > > > > > > +#define _INIT_DELAY_CMD(...) { \ > > > > > > > > > > > > > + .type = DELAY_CMD,\ > > > > > > > > > > > > > + .len = sizeof((char[]){__VA_ARGS__}), \ > > > > > > > > > > > > > + .data = (char[]){__VA_ARGS__} } > > > > > > > > > > > > > > > > > > > > > > > > This is the third panel driver using the same appoach. Can you use > > > > > > > > > > > > mipi_dsi_generic_write_seq() instead of the huge table? Or if you prefer > > > > > > > > > > > > the table, we should extract this framework to a common helper. > > > > > > > > > > > > (my preference is shifted towards mipi_dsi_generic_write_seq()). > > > > > > > > > > > > > > > > > > > > > > > The drawback of mipi_dsi_generic_write_seq() is that it can cause the > > > > > > > > > > > kernel size grows a lot since every sequence will be expanded. > > > > > > > > > > > > > > > > > > > > > > Similar discussion in here: > > > > > > > > > > > https://lore.kernel.org/dri-devel/CAD=FV=Wju3WS45=EpXMUg7FjYDh3-=mvm_jS7TF1tsaAzbb4Uw@mail.gmail.com/ > > > > > > > > > > > > > > > > > > > > > > This patch would increase the module size from 157K to 572K. > > > > > > > > > > > scripts/bloat-o-meter shows chg +235.95%. > > > > > > > > > > > > > > > > > > > > > > So maybe the common helper is better regarding the kernel module size? > > > > > > > > > > > > > > > > > > > > Yes, let's get a framework done in a useful way. > > > > > > > > > > I'd say, drop the _INIT_DELAY_CMD. msleep() and usleep_range() should be > > > > > > > > > > used instead (and it's up to the developer to select correct delay > > > > > > > > > > function). > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > > > > > > +static const struct panel_init_cmd kingdisplay_kd101ne3_init_cmd[] = { > > > > > > > > > > > > > + _INIT_DELAY_CMD(50), > > > > > > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00), > > > > > > > > > > > > > > > > > > > > [skipped the body of the table] > > > > > > > > > > > > > > > > > > > > > > > + _INIT_DCS_CMD(0x0E, 0x48), > > > > > > > > > > > > > + > > > > > > > > > > > > > + _INIT_DCS_CMD(0xE0, 0x00), > > > > > > > > > > > > > > > > > > > > > > > + _INIT_DCS_CMD(0X11), > > > > > > > > > > > > > > > > > > > > Also, at least this is mipi_dsi_dcs_exit_sleep_mode(). > > > > > > > > > > > > > > > > > > > > > > > + /* T6: 120ms */ > > > > > > > > > > > > > + _INIT_DELAY_CMD(120), > > > > > > > > > > > > > + _INIT_DCS_CMD(0X29), > > > > > > > > > > > > > > > > > > > > And this is mipi_dsi_dcs_set_display_on(). > > > > > > > > > > > > > > > > > > > > Having a single table enourages people to put known commands into the > > > > > > > > > > table, the practice that must be frowned upon and forbidden. > > > > > > > > > > > > > > > > > > > > We have functions for some of the standard DCS commands. So, maybe > > > > > > > > > > instead of adding a single-table based approach we can improve > > > > > > > > > > mipi_dsi_generic_write_seq() to reduce the bloat. E.g. by moving the > > > > > > > > > > error handling to a common part of enable() / prepare() function. > > > > > > > > > > > > > > > > > > > > > > > > > > > > For this panel, I think it can also refer to how > > > > > > > > > panel-kingdisplay-kd097d04.c does. Create the table for init cmd data, > > > > > > > > > not what operation to use, and use mipi_dsi_generic_write_seq() when > > > > > > > > > looping through the table. > > > > > > > > > > > > > > > > Even more similar discussion: > > > > > > > > > > > > > > > > https://lore.kernel.org/r/CAD=FV=UGDbNvAMjzWSOvxybGikQcvW9JsRtbxHVg8_97YPEQCA@mail.gmail.com > > > > > > > > > > > > > > It seems I skipped that thread. > > > > > > > > > > > > > > I'd still suggest a code-based solution compared to table-based one, for > > > > > > > the reasons I've outlined before. Having a tables puts a pressure on the > > > > > > > developer to put commands there for which we already have a > > > > > > > command-specific function. > > > > > > > > > > > > The problem is that with these panels that need big init sequences the > > > > > > code based solution is _a lot_ bigger. If it were a few bytes or a > > > > > > 1-2KB then fine, but when Hsin-Yi measured Linus W's attempt to move > > > > > > from a table to code it was 100K difference in code [1]. I would also > > > > > > say that having these long init sequences done as separate commands > > > > > > encourages people to skip checking the return values of each of the > > > > > > transfer functions and I don't love that idea. > > > > > > > > > > > > It would be ideal if these panels didn't need these long init > > > > > > sequences, but I don't have any inside knowledge here saying that they > > > > > > could be removed. So assume we can't get rid of the init sequences it > > > > > > feels like we have to find some way to make the tables work for at > > > > > > least the large chunks of init code and encourage people to make the > > > > > > tables readable... > > > > > > > > > > > > > > > I did a quick check on the boe-tv101wum-nl6 driver by converting the > > > > > writes to use the following macro: > > > > > > > > > > #define mipi_dsi_dcs_write_cmd_seq(dsi, cmd, seq...) > > > > > \ > > > > > do { \ > > > > > static const u8 d[] = { cmd, seq }; \ > > > > > ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \ > > > > > if (ret < 0) \ > > > > > goto err; \ > > > > > } while (0) > > > > > > > > > > And then at the end of the init funciton having > > > > > > > > > > err: > > > > > dev_err(panel->dev, > > > > > "failed to write command %d\n", ret); > > > > > return ret; > > > > > } > > > > > > > > > > > > > I'm not sure about the coding style rule here, would it be considered > > > > unclear that caller of mipi_dsi_dcs_write_cmd_seq() needs to have err > > > > block, but the block may not be directly used in that caller and is > > > > only jumped from the macro? > > > > > > I'm also not sure here. It was a quick and dirty test. > > > We might as well do something like > > > > > > ret = mipi_dsi_dcs_write_cmd_seq(dsi, ...); > > > if (ret) > > > goto err; > > > > > > all over the place. > > > > Oh. This is actually very simple to fix and requires no code changes > > to clients. :-P We just need to hoist the printing out of the macro > > and into "drm_mipi_dsi.c". Let me double-confirm and then I'll post a > > patch. > > Sounds good. I have converted boe-tv101wum-nl6, ilitek-ili9882t and > innolux-p079zca drivers. Once you post your patch I can post those > too. I sent out a patch for it, though I didn't have time to do testing on real hardware: https://lore.kernel.org/r/20240424172017.1.Id15fae80582bc74a0d4f1338987fa375738f45b9@changeid I can poke more at it tomorrow... -Doug ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver 2024-04-24 21:49 ` Dmitry Baryshkov 2024-04-24 22:15 ` Hsin-Yi Wang @ 2024-04-24 22:27 ` Doug Anderson 1 sibling, 0 replies; 19+ messages in thread From: Doug Anderson @ 2024-04-24 22:27 UTC (permalink / raw To: Dmitry Baryshkov Cc: Hsin-Yi Wang, lvzhaoxiong, mripard, airlied, daniel, robh, krzysztof.kozlowski+dt, conor+dt, dri-devel, devicetree, linux-kernel, cong yang Hi, On Wed, Apr 24, 2024 at 2:49 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > > The problem is that with these panels that need big init sequences the > > code based solution is _a lot_ bigger. If it were a few bytes or a > > 1-2KB then fine, but when Hsin-Yi measured Linus W's attempt to move > > from a table to code it was 100K difference in code [1]. I would also > > say that having these long init sequences done as separate commands > > encourages people to skip checking the return values of each of the > > transfer functions and I don't love that idea. > > > > It would be ideal if these panels didn't need these long init > > sequences, but I don't have any inside knowledge here saying that they > > could be removed. So assume we can't get rid of the init sequences it > > feels like we have to find some way to make the tables work for at > > least the large chunks of init code and encourage people to make the > > tables readable... > > > I did a quick check on the boe-tv101wum-nl6 driver by converting the > writes to use the following macro: > > #define mipi_dsi_dcs_write_cmd_seq(dsi, cmd, seq...) > \ > do { \ > static const u8 d[] = { cmd, seq }; \ > ret = mipi_dsi_dcs_write_buffer(dsi, d, ARRAY_SIZE(d)); \ > if (ret < 0) \ > goto err; \ > } while (0) > > And then at the end of the init funciton having > > err: > dev_err(panel->dev, > "failed to write command %d\n", ret); > return ret; > } > > Size comparison: > text data bss dec hex filename > before > 34109 10410 18 44537 adf9 > ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o > making init data const > 44359 184 0 44543 adff > ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o > with new macros > 44353 184 0 44537 adf9 > ../build-64/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.o > > As you can see, there is literally no size difference with this macro in place. > The only drawback is that the init stops on the first write rather > than going through the sequence. > > WDYT? I can turn this into a proper patch if you think this makes sense. Ah, so what you're saying is that the big bloat from using the existing mipi_dsi_dcs_write_seq() is the error printing. That makes sense. ...and by relying on the caller to provide an error handling label we can get rid of the overhead and still get the error prints. Yes, that seems pretty reasonable to me. I guess I'd perhaps make the error label a parameter to the macro (so it's obvious that the caller needs to define it) and maybe name it in such a way to make it obvious the difference between this macro and mipi_dsi_dcs_write_seq(). With that and your measurements then this seems perfectly reasonable to me and I'm good with fully moving away from the table-based approach. I'd be happy if you sent a patch for it and happy to review it. -Doug ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2024-04-25 0:22 UTC | newest] Thread overview: 19+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-04-18 8:15 [PATCH v1 0/2] Add kd101ne3 bindings and driver lvzhaoxiong 2024-04-18 8:15 ` [PATCH v1 1/2] dt-bindings: display: panel: Add KD101NE3-40TI support lvzhaoxiong 2024-04-18 22:59 ` Krzysztof Kozlowski 2024-04-18 8:15 ` [PATCH v1 2/2] drm/panel: kd101ne3: add new panel driver lvzhaoxiong 2024-04-18 11:46 ` Dmitry Baryshkov 2024-04-18 13:11 ` Hsin-Yi Wang 2024-04-18 14:11 ` Dmitry Baryshkov 2024-04-23 18:10 ` Hsin-Yi Wang 2024-04-23 20:41 ` Doug Anderson 2024-04-23 21:20 ` Dmitry Baryshkov 2024-04-24 21:04 ` Doug Anderson 2024-04-24 21:18 ` Hsin-Yi Wang 2024-04-24 21:49 ` Dmitry Baryshkov 2024-04-24 22:15 ` Hsin-Yi Wang 2024-04-24 22:50 ` Dmitry Baryshkov 2024-04-24 23:25 ` Doug Anderson 2024-04-24 23:37 ` Dmitry Baryshkov 2024-04-25 0:21 ` Doug Anderson 2024-04-24 22:27 ` Doug Anderson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).