Re: [PATCH v7 1/2] drm/panel: Add support for Truly NT35597 panel driver
On Wed 19 Sep 19:55 PDT 2018, Abhinav Kumar wrote:
> +static int truly_nt35597_probe(struct mipi_dsi_device *dsi)
> +{
[..]
> + dsi1 = of_graph_get_remote_node(dsi->dev.of_node, 1, -1);
> + if (!dsi1) {
> + DRM_DEV_ERROR(dev,
> + "failed to get remote node for dsi1_device\n");
> + ret = -ENODEV;
> + goto err_get_remote;
> + }
> +
> + dsi1_host = of_find_mipi_dsi_host_by_node(dsi1);
> + if (!dsi1_host) {
> + DRM_DEV_ERROR(dev, "failed to find dsi host\n");
> + ret = -EPROBE_DEFER;
> + goto err_host;
> + }
> +
> + of_node_put(dsi1);
> +
> + /* register the second DSI device */
> + dsi1_device = mipi_dsi_device_register_full(dsi1_host, );
> + if (IS_ERR(dsi1_device)) {
[..]
> + goto err_dsi_device;
[..]
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev,
> + "dsi attach failed i = %d\n", i);
> + goto err_dsi_attach;
> + }
> + }
> +
> + return 0;
> +
> +err_dsi_attach:
> + drm_panel_remove(>panel);
> +err_panel_add:
> + mipi_dsi_device_unregister(dsi1_device);
> +err_dsi_device:
> +err_host:
> + of_node_put(dsi1);
dsi1 is already put if we came here through err_dsi_device et al.
You don't need to reference dsi1 beyond the call to
of_find_mipi_dsi_host_by_node() to put it before checking the dsi1_host.
> +err_get_remote:
> + return ret;
> +}
Regards,
Bjorn
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 1/2] drm/panel: Add support for Truly NT35597 panel driver
Hi Bjorn
Thanks for the comment.
Yes, I will move of_node_put(dsi1) to right after
of_find_mipi_dsi_host_by_node and remove it from err_dsi_device.
Thanks
Abhinav
On 2018-09-20 09:54, Bjorn Andersson wrote:
On Wed 19 Sep 19:55 PDT 2018, Abhinav Kumar wrote:
+static int truly_nt35597_probe(struct mipi_dsi_device *dsi)
+{
[..]
+ dsi1 = of_graph_get_remote_node(dsi->dev.of_node, 1, -1);
+ if (!dsi1) {
+ DRM_DEV_ERROR(dev,
+ "failed to get remote node for dsi1_device\n");
+ ret = -ENODEV;
+ goto err_get_remote;
+ }
+
+ dsi1_host = of_find_mipi_dsi_host_by_node(dsi1);
+ if (!dsi1_host) {
+ DRM_DEV_ERROR(dev, "failed to find dsi host\n");
+ ret = -EPROBE_DEFER;
+ goto err_host;
+ }
+
+ of_node_put(dsi1);
+
+ /* register the second DSI device */
+ dsi1_device = mipi_dsi_device_register_full(dsi1_host, );
+ if (IS_ERR(dsi1_device)) {
[..]
+ goto err_dsi_device;
[..]
+ if (ret < 0) {
+ DRM_DEV_ERROR(dev,
+ "dsi attach failed i = %d\n", i);
+ goto err_dsi_attach;
+ }
+ }
+
+ return 0;
+
+err_dsi_attach:
+ drm_panel_remove(>panel);
+err_panel_add:
+ mipi_dsi_device_unregister(dsi1_device);
+err_dsi_device:
+err_host:
+ of_node_put(dsi1);
dsi1 is already put if we came here through err_dsi_device et al.
You don't need to reference dsi1 beyond the call to
of_find_mipi_dsi_host_by_node() to put it before checking the
dsi1_host.
+err_get_remote:
+ return ret;
+}
Regards,
Bjorn
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 1/2] drm/panel: Add support for Truly NT35597 panel driver
Hi Sean
Sure, I will address these in v8.
Thanks
Abhinav
On 2018-09-20 08:52, Sean Paul wrote:
On Wed, Sep 19, 2018 at 07:55:39PM -0700, Abhinav Kumar wrote:
From: "abhin...@codeaurora.org"
Add support for Truly NT35597 panel driver used
in MSM reference platforms.
This panel driver supports both single DSI and dual DSI
modes.
However, this patch series adds support only for
dual DSI mode.
Changes in v7:
- Make the panel commands, name and mode as const
- Configure the reset pin as active low in the device
tree and adjust the reset sequence
- Remove the macros to call the functions
- Fix return conditions in the panel prepare call
- Change the compatible string to have only the
panel driver and the resolution details
Signed-off-by: Archit Taneja
Signed-off-by: Abhinav Kumar
Hey Abhinav,
Thanks for revising the patch. I'm a bit torn on sending more feedback
since
you're already on v7 and it's been on-list for a while. Unfortunately I
think
there's enough to warrant a respin, so apologies for keeping this thing
in
limbo, but hopefully we're converging.
---
drivers/gpu/drm/panel/Kconfig | 8 +
drivers/gpu/drm/panel/Makefile | 1 +
drivers/gpu/drm/panel/panel-truly-nt35597.c | 706
3 files changed, 715 insertions(+)
create mode 100644 drivers/gpu/drm/panel/panel-truly-nt35597.c
diff --git a/drivers/gpu/drm/panel/Kconfig
b/drivers/gpu/drm/panel/Kconfig
index 6020c30..7ae74c2 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -186,4 +186,12 @@ config DRM_PANEL_SITRONIX_ST7789V
Say Y here if you want to enable support for the Sitronix
ST7789V controller for 240x320 LCD panels
+config DRM_PANEL_TRULY_NT35597_WQXGA
+ tristate "Truly WQXGA"
+ depends on OF
+ depends on DRM_MIPI_DSI
+ select VIDEOMODE_HELPERS
I don't think you're using the videomode stuff any longer?
+ help
+ Say Y here if you want to enable support for Truly NT35597 WQXGA
Dual DSI
+ Video Mode panel
endmenu
diff --git a/drivers/gpu/drm/panel/Makefile
b/drivers/gpu/drm/panel/Makefile
index 5ccaaa9..80fd19f 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -19,3 +19,4 @@ obj-$(CONFIG_DRM_PANEL_SEIKO_43WVF1G) +=
panel-seiko-43wvf1g.o
obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) +=
panel-sharp-lq101r1sx01.o
obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) +=
panel-sharp-ls043t1le01.o
obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o
+obj-$(CONFIG_DRM_PANEL_TRULY_NT35597_WQXGA) += panel-truly-nt35597.o
diff --git a/drivers/gpu/drm/panel/panel-truly-nt35597.c
b/drivers/gpu/drm/panel/panel-truly-nt35597.c
new file mode 100644
index 000..a070d0f
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c
@@ -0,0 +1,706 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
Same comment here re: videomode
+
+#include
+#include
+#include
+
+static const char * const regulator_names[] = {
+ "vdda",
+ "vdispp",
+ "vdispn",
+};
+
+static unsigned long const regulator_enable_loads[] = {
+ 62000,
+ 10,
+ 10,
+};
+
+static unsigned long const regulator_disable_loads[] = {
+ 80,
+ 100,
+ 100,
+};
+
+struct nt35597_config {
+ u32 width_mm;
+ u32 height_mm;
+ const char *panel_name;
+ const void *panel_on_cmds;
+ u32 num_on_cmds;
+ const struct drm_display_mode *dm;
+};
+
+struct truly_nt35597 {
+ struct device *dev;
+ struct drm_panel panel;
+
+ struct regulator_bulk_data supplies[ARRAY_SIZE(regulator_names)];
+
+ struct gpio_desc *reset_gpio;
+ struct gpio_desc *mode_gpio;
+
+ struct backlight_device *backlight;
+
+ struct mipi_dsi_device *dsi[2];
+
+ const struct nt35597_config *config;
+ bool prepared;
+ bool enabled;
+};
+
+static inline struct truly_nt35597 *panel_to_ctx(struct drm_panel
*panel)
+{
+ return container_of(panel, struct truly_nt35597, panel);
+}
+
+#define MAX_LEN5
Aside from the ambiguous name, this is only used in one place and seems
like
it's 1 byte too big. Could you please just declare commands as 'u8
commands[4];'
below?
+#define SHORT_PACKET 2
+#define LONG_PACKET 4
SHORT_PACKET and LONG_PACKET aren't used anywhere
+
+struct cmd_set {
+ u8 commands[MAX_LEN];
+ u8 size;
+};
+
+static struct cmd_set qcom_2k_panel_magic_cmds[] = {
+ /* CMD2_P0 */
+ { { 0xff, 0x20 }, 2 },
+ { { 0xfb, 0x01 }, 2 },
+ { { 0x00, 0x01 }, 2 },
+ { { 0x01, 0x55 }, 2 },
+ { { 0x02, 0x45 }, 2 },
+ { { 0x05, 0x40 }, 2 },
+ { { 0x06, 0x19 }, 2 },
+ { { 0x07, 0x1e }, 2 },
+ { { 0x0b, 0x73 }, 2 },
+ { { 0x0c,
Re: [PATCH v7 1/2] drm/panel: Add support for Truly NT35597 panel driver
On Wed, Sep 19, 2018 at 07:55:39PM -0700, Abhinav Kumar wrote:
> From: "abhin...@codeaurora.org"
>
> Add support for Truly NT35597 panel driver used
> in MSM reference platforms.
>
> This panel driver supports both single DSI and dual DSI
> modes.
>
> However, this patch series adds support only for
> dual DSI mode.
>
> Changes in v7:
> - Make the panel commands, name and mode as const
> - Configure the reset pin as active low in the device
> tree and adjust the reset sequence
> - Remove the macros to call the functions
> - Fix return conditions in the panel prepare call
> - Change the compatible string to have only the
> panel driver and the resolution details
>
> Signed-off-by: Archit Taneja
> Signed-off-by: Abhinav Kumar
Hey Abhinav,
Thanks for revising the patch. I'm a bit torn on sending more feedback since
you're already on v7 and it's been on-list for a while. Unfortunately I think
there's enough to warrant a respin, so apologies for keeping this thing in
limbo, but hopefully we're converging.
> ---
> drivers/gpu/drm/panel/Kconfig | 8 +
> drivers/gpu/drm/panel/Makefile | 1 +
> drivers/gpu/drm/panel/panel-truly-nt35597.c | 706
>
> 3 files changed, 715 insertions(+)
> create mode 100644 drivers/gpu/drm/panel/panel-truly-nt35597.c
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 6020c30..7ae74c2 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -186,4 +186,12 @@ config DRM_PANEL_SITRONIX_ST7789V
> Say Y here if you want to enable support for the Sitronix
> ST7789V controller for 240x320 LCD panels
>
> +config DRM_PANEL_TRULY_NT35597_WQXGA
> + tristate "Truly WQXGA"
> + depends on OF
> + depends on DRM_MIPI_DSI
> + select VIDEOMODE_HELPERS
I don't think you're using the videomode stuff any longer?
> + help
> + Say Y here if you want to enable support for Truly NT35597 WQXGA Dual
> DSI
> + Video Mode panel
> endmenu
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index 5ccaaa9..80fd19f 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -19,3 +19,4 @@ obj-$(CONFIG_DRM_PANEL_SEIKO_43WVF1G) +=
> panel-seiko-43wvf1g.o
> obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
> obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) += panel-sharp-ls043t1le01.o
> obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o
> +obj-$(CONFIG_DRM_PANEL_TRULY_NT35597_WQXGA) += panel-truly-nt35597.o
> diff --git a/drivers/gpu/drm/panel/panel-truly-nt35597.c
> b/drivers/gpu/drm/panel/panel-truly-nt35597.c
> new file mode 100644
> index 000..a070d0f
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c
> @@ -0,0 +1,706 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
Same comment here re: videomode
> +
> +#include
> +#include
> +#include
> +
> +static const char * const regulator_names[] = {
> + "vdda",
> + "vdispp",
> + "vdispn",
> +};
> +
> +static unsigned long const regulator_enable_loads[] = {
> + 62000,
> + 10,
> + 10,
> +};
> +
> +static unsigned long const regulator_disable_loads[] = {
> + 80,
> + 100,
> + 100,
> +};
> +
> +struct nt35597_config {
> + u32 width_mm;
> + u32 height_mm;
> + const char *panel_name;
> + const void *panel_on_cmds;
> + u32 num_on_cmds;
> + const struct drm_display_mode *dm;
> +};
> +
> +struct truly_nt35597 {
> + struct device *dev;
> + struct drm_panel panel;
> +
> + struct regulator_bulk_data supplies[ARRAY_SIZE(regulator_names)];
> +
> + struct gpio_desc *reset_gpio;
> + struct gpio_desc *mode_gpio;
> +
> + struct backlight_device *backlight;
> +
> + struct mipi_dsi_device *dsi[2];
> +
> + const struct nt35597_config *config;
> + bool prepared;
> + bool enabled;
> +};
> +
> +static inline struct truly_nt35597 *panel_to_ctx(struct drm_panel *panel)
> +{
> + return container_of(panel, struct truly_nt35597, panel);
> +}
> +
> +#define MAX_LEN 5
Aside from the ambiguous name, this is only used in one place and seems like
it's 1 byte too big. Could you please just declare commands as 'u8 commands[4];'
below?
> +#define SHORT_PACKET 2
> +#define LONG_PACKET 4
SHORT_PACKET and LONG_PACKET aren't used anywhere
> +
> +struct cmd_set {
> + u8 commands[MAX_LEN];
> + u8 size;
> +};
> +
> +static struct cmd_set qcom_2k_panel_magic_cmds[] = {
> + /* CMD2_P0 */
> + { { 0xff, 0x20 }, 2 },
> + { { 0xfb, 0x01 }, 2 },
> + { { 0x00, 0x01 }, 2 },
> + { { 0x01, 0x55 }, 2 },
> + { { 0x02, 0x45 }, 2 },
> + { { 0x05, 0x40 }, 2 },
> + { { 0x06, 0x19
[PATCH v7 1/2] drm/panel: Add support for Truly NT35597 panel driver
From: "abhin...@codeaurora.org"
Add support for Truly NT35597 panel driver used
in MSM reference platforms.
This panel driver supports both single DSI and dual DSI
modes.
However, this patch series adds support only for
dual DSI mode.
Changes in v7:
- Make the panel commands, name and mode as const
- Configure the reset pin as active low in the device
tree and adjust the reset sequence
- Remove the macros to call the functions
- Fix return conditions in the panel prepare call
- Change the compatible string to have only the
panel driver and the resolution details
Signed-off-by: Archit Taneja
Signed-off-by: Abhinav Kumar
---
drivers/gpu/drm/panel/Kconfig | 8 +
drivers/gpu/drm/panel/Makefile | 1 +
drivers/gpu/drm/panel/panel-truly-nt35597.c | 706
3 files changed, 715 insertions(+)
create mode 100644 drivers/gpu/drm/panel/panel-truly-nt35597.c
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 6020c30..7ae74c2 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -186,4 +186,12 @@ config DRM_PANEL_SITRONIX_ST7789V
Say Y here if you want to enable support for the Sitronix
ST7789V controller for 240x320 LCD panels
+config DRM_PANEL_TRULY_NT35597_WQXGA
+ tristate "Truly WQXGA"
+ depends on OF
+ depends on DRM_MIPI_DSI
+ select VIDEOMODE_HELPERS
+ help
+ Say Y here if you want to enable support for Truly NT35597 WQXGA Dual
DSI
+ Video Mode panel
endmenu
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 5ccaaa9..80fd19f 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -19,3 +19,4 @@ obj-$(CONFIG_DRM_PANEL_SEIKO_43WVF1G) += panel-seiko-43wvf1g.o
obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o
obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) += panel-sharp-ls043t1le01.o
obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o
+obj-$(CONFIG_DRM_PANEL_TRULY_NT35597_WQXGA) += panel-truly-nt35597.o
diff --git a/drivers/gpu/drm/panel/panel-truly-nt35597.c
b/drivers/gpu/drm/panel/panel-truly-nt35597.c
new file mode 100644
index 000..a070d0f
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c
@@ -0,0 +1,706 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+#include
+#include
+#include
+
+static const char * const regulator_names[] = {
+ "vdda",
+ "vdispp",
+ "vdispn",
+};
+
+static unsigned long const regulator_enable_loads[] = {
+ 62000,
+ 10,
+ 10,
+};
+
+static unsigned long const regulator_disable_loads[] = {
+ 80,
+ 100,
+ 100,
+};
+
+struct nt35597_config {
+ u32 width_mm;
+ u32 height_mm;
+ const char *panel_name;
+ const void *panel_on_cmds;
+ u32 num_on_cmds;
+ const struct drm_display_mode *dm;
+};
+
+struct truly_nt35597 {
+ struct device *dev;
+ struct drm_panel panel;
+
+ struct regulator_bulk_data supplies[ARRAY_SIZE(regulator_names)];
+
+ struct gpio_desc *reset_gpio;
+ struct gpio_desc *mode_gpio;
+
+ struct backlight_device *backlight;
+
+ struct mipi_dsi_device *dsi[2];
+
+ const struct nt35597_config *config;
+ bool prepared;
+ bool enabled;
+};
+
+static inline struct truly_nt35597 *panel_to_ctx(struct drm_panel *panel)
+{
+ return container_of(panel, struct truly_nt35597, panel);
+}
+
+#define MAX_LEN5
+#define SHORT_PACKET 2
+#define LONG_PACKET 4
+
+struct cmd_set {
+ u8 commands[MAX_LEN];
+ u8 size;
+};
+
+static struct cmd_set qcom_2k_panel_magic_cmds[] = {
+ /* CMD2_P0 */
+ { { 0xff, 0x20 }, 2 },
+ { { 0xfb, 0x01 }, 2 },
+ { { 0x00, 0x01 }, 2 },
+ { { 0x01, 0x55 }, 2 },
+ { { 0x02, 0x45 }, 2 },
+ { { 0x05, 0x40 }, 2 },
+ { { 0x06, 0x19 }, 2 },
+ { { 0x07, 0x1e }, 2 },
+ { { 0x0b, 0x73 }, 2 },
+ { { 0x0c, 0x73 }, 2 },
+ { { 0x0e, 0xb0 }, 2 },
+ { { 0x0f, 0xae }, 2 },
+ { { 0x11, 0xb8 }, 2 },
+ { { 0x13, 0x00 }, 2 },
+ { { 0x58, 0x80 }, 2 },
+ { { 0x59, 0x01 }, 2 },
+ { { 0x5a, 0x00 }, 2 },
+ { { 0x5b, 0x01 }, 2 },
+ { { 0x5c, 0x80 }, 2 },
+ { { 0x5d, 0x81 }, 2 },
+ { { 0x5e, 0x00 }, 2 },
+ { { 0x5f, 0x01 }, 2 },
+ { { 0x72, 0x11 }, 2 },
+ { { 0x68, 0x03 }, 2 },
+ /* CMD2_P4 */
+ { { 0xFF, 0x24 }, 2 },
+ { { 0xFB, 0x01 }, 2 },
+ { { 0x00, 0x1C }, 2 },
+ { { 0x01, 0x0B }, 2 },
+ { { 0x02, 0x0C }, 2 },
+ { { 0x03, 0x01 }, 2 },
+ { { 0x04, 0x0F }, 2 },
+ { { 0x05, 0x10 }, 2 },
+ { { 0x06, 0x10 }, 2 },
+ { { 0x07, 0x10 }, 2 },
+
