Re: [DPU PATCH 1/2] drm/panel: Add Truly Dual DSI video mode panel

[abhinavk@codeaurora.org]

On 2018-04-08 09:36, Archit Taneja wrote:
> Hi Abhinav,
> 
> Thanks for posting this driver. Some comments below.
> 
> On Saturday 07 April 2018 12:36 PM, Abhinav Kumar wrote:
>> From: Archit Taneja <architt@codeaurora.org>
>> 
>> Add support for truly dual DSI video mode panel
>> panel used in MSM reference platforms >
>> Signed-off-by: Archit Taneja <architt@codeaurora.org>
>> Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
>> ---
>>   .../bindings/display/truly,dual_wqxga.txt          |  47 ++
>>   drivers/gpu/drm/panel/Kconfig                      |   7 +
>>   drivers/gpu/drm/panel/Makefile                     |   1 +
>>   drivers/gpu/drm/panel/panel-truly-dual-dsi.c       | 530 
>> +++++++++++++++++++++
>>   4 files changed, 585 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/display/truly,dual_wqxga.txt
>>   create mode 100644 drivers/gpu/drm/panel/panel-truly-dual-dsi.c
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/display/truly,dual_wqxga.txt 
>> b/Documentation/devicetree/bindings/display/truly,dual_wqxga.txt
>> new file mode 100644
>> index 0000000..a1b24c1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/truly,dual_wqxga.txt
>> @@ -0,0 +1,47 @@
>> +Truly model NT35597 1440x2560 DSI Panel
>> +
>> +Required properties:
>> +- compatible: should be "truly,dual_wqxga"
> 
> The compatible string, kernel config and the driver file should be 
> based
> on the panel model no. There can be many truly based panels that
> support wqxga. Something like "truly,nt35597" would be better.
> 
[Abhinav] Yes, will change it in v2.

>> +- vdda-supply: phandle of the regulator that provides the supply 
>> voltage
>> +  Power IC supply
>> +- lab-supply: phandle of the regulator that provides the supply 
>> voltage
>> +  for LCD bias
>> +- ibb-supply: phandle of the regulator that provides the supply 
>> voltage
>> +  for LCD bias
> 
> Both seem to have the same description. Aren't lab and ibb qualcomm
> specific terms? Could we use the pin names specified in the panel's
> data sheet?
> 
[Abhinav] There isnt a number specified on the data sheet. I will use 
vdispp-supply and
vdispn-supply . Does that work?


>> +- reset-gpios: phandle of gpio for reset line
>> +  This should be 8mA, gpio can be configured using mux, pinctrl, 
>> pinctrl-names
>> +- mode-gpios: phandle of the gpio for choosing the mode of the 
>> display
>> +  for single DSI or Dual DSI
> 
> Could we describe here how to use this gpio? I.e, whether we need to 
> set
> it to low for dual DSI, etc?
> 
[Abhinav] Yes, will do this in v2.
>> +- display-timings: Node for the Panel timings
>> +- link2: phandle to the secondary node of the panel
> 
> The link2 binding was a temporary hack we used. We should use the
> of-graph bindings to represent the two DSI input ports of the panel.
> 
[Abhinav] Alright will start using of-graph in the next patchset.
>> +
>> +Example:
>> +
>> +	dsi@ae94000 {
>> +		panel@0 {
>> +			compatible = "truly,dual_wqxga";
>> +			reg = <0>;
>> +			link2 = <&link2>;
>> +			vdda-supply = <&pm8998_l14>;
>> +
>> +			pinctrl-names = "default", "suspend";
>> +			pinctrl-0 = <&sde_dsi_active>;
>> +			pinctrl-1 = <&sde_dsi_suspend>;
>> +
>> +			reset-gpios = <&tlmm 6 0>;
>> +			mode-gpios = <&tlmm 52 0>;
>> +			display-timings {
>> +				timing0: timing-0 {
>> +					clock-frequency = <268316138>;
>> +					hactive = <1440>;
>> +					vactive = <2560>;
>> +					hfront-porch = <200>;
>> +					hback-porch = <64>;
>> +					hsync-len = <32>;
>> +					vfront-porch = <8>;
>> +					vback-porch = <7>;
>> +					vsync-len = <1>;
>> +				};
>> +			};
>> +		};
>> +	};
>> diff --git a/drivers/gpu/drm/panel/Kconfig 
>> b/drivers/gpu/drm/panel/Kconfig
>> index 988048e..a63c3f7 100644
>> --- a/drivers/gpu/drm/panel/Kconfig
>> +++ b/drivers/gpu/drm/panel/Kconfig
>> @@ -168,4 +168,11 @@ 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_WQXGA
>> +	tristate "Truly WQXGA"
>> +	depends on OF
>> +	depends on DRM_MIPI_DSI
>> +	help
>> +	  Say Y here if you want to enable support for Truly WQXGA Dual DSI
>> +	  Video Mode panel
>>   endmenu
[Abhinav] These configs will be updated too
>> diff --git a/drivers/gpu/drm/panel/Makefile 
>> b/drivers/gpu/drm/panel/Makefile
>> index 3d2a88d..64891f6 100644
>> --- a/drivers/gpu/drm/panel/Makefile
>> +++ b/drivers/gpu/drm/panel/Makefile
>> @@ -17,3 +17,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_WQXGA) += panel-truly-dual-dsi.o
>> diff --git a/drivers/gpu/drm/panel/panel-truly-dual-dsi.c 
>> b/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
>> new file mode 100644
>> index 0000000..47891ee
>> --- /dev/null
>> +++ b/drivers/gpu/drm/panel/panel-truly-dual-dsi.c
>> @@ -0,0 +1,530 @@
>> +/* Copyright (c) 2018, The Linux Foundation. All rights reserved.
> 
> We should use the SPDX license headers here.
> 
[Abhinav] Is there a reference I can use? Need to make sure of this.
>> + *
>> + * This program is free software; you can redistribute it and/or 
>> modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/pinctrl/consumer.h>
>> +
>> +#include <video/mipi_display.h>
>> +#include <video/of_videomode.h>
>> +#include <video/videomode.h>
>> +
>> +#include <drm/drmP.h>
>> +#include <drm/drm_panel.h>
>> +#include <drm/drm_mipi_dsi.h>
>> +
>> +struct truly_wqxga {
>> +	struct device *dev;
>> +	struct drm_panel panel;
>> +
>> +	struct regulator_bulk_data supplies[4];
>> +
>> +	struct gpio_desc *reset_gpio;
>> +	struct gpio_desc *mode_gpio;
>> +
>> +	struct backlight_device *backlight;
>> +	struct videomode vm;
>> +
>> +	struct mipi_dsi_device *dsi[2];
>> +
>> +	bool prepared;
>> +	bool enabled;
>> +};
>> +
>> +static inline struct truly_wqxga *panel_to_truly_wqxga(struct 
>> drm_panel *panel)
>> +{
>> +	return container_of(panel, struct truly_wqxga, panel);
>> +}
>> +
>> +static int truly_wqxga_power_on(struct truly_wqxga *ctx)
>> +{
>> +	int ret;
>> +
>> +	ret = regulator_set_load(ctx->supplies[1].consumer, 62000);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regulator_set_load(ctx->supplies[2].consumer, 100000);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regulator_set_load(ctx->supplies[3].consumer, 100000);
>> +	if (ret)
>> +		return ret;
> 
> We could make a const static struct array specifying the regulator
> names (to be used during probe) and their corresponding power on and
> power off loads. That should make things a bit cleaner.
> 
[Abhinav] Yes, will do it in the next patchset.
>> +
>> +	ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), 
>> ctx->supplies);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	msleep(20);
>> +	gpiod_set_value(ctx->reset_gpio, 1);
>> +	usleep_range(20000, 20100);
>> +	gpiod_set_value(ctx->reset_gpio, 0);
>> +	usleep_range(20000, 20100);
>> +	gpiod_set_value(ctx->reset_gpio, 1);
>> +	usleep_range(50000, 50100);
>> +
>> +	/* dual port */
>> +	gpiod_set_value(ctx->mode_gpio, 0);
>> +
>> +	return 0;
>> +}
>> +
>> +static int truly_wqxga_power_off(struct truly_wqxga *ctx)
>> +{
>> +	int ret;
>> +
>> +	gpiod_set_value(ctx->reset_gpio, 0);
>> +	ret = regulator_set_load(ctx->supplies[1].consumer, 62000);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), 
>> ctx->supplies);
>> +}
>> +
>> +static int truly_wqxga_disable(struct drm_panel *panel)
>> +{
>> +	struct truly_wqxga *ctx = panel_to_truly_wqxga(panel);
>> +
>> +	if (!ctx->enabled)
>> +		return 0;
>> +
>> +	if (ctx->backlight) {
>> +		ctx->backlight->props.power = FB_BLANK_POWERDOWN;
>> +		backlight_update_status(ctx->backlight);
>> +	}
>> +
>> +	ctx->enabled = false;
>> +	return 0;
>> +}
>> +
>> +static int truly_wqxga_unprepare(struct drm_panel *panel)
>> +{
>> +	struct truly_wqxga *ctx = panel_to_truly_wqxga(panel);
>> +	struct mipi_dsi_device **dsis = ctx->dsi;
>> +	int ret = 0, i;
>> +
>> +	if (!ctx->prepared)
>> +		return 0;
>> +
>> +	dsis[0]->mode_flags = 0;
>> +	dsis[1]->mode_flags = 0;
>> +
>> +	for (i = 0; i < 2; i++)
>> +		if (mipi_dsi_dcs_set_display_off(dsis[i]) < 0)
>> +			ret = -ECOMM;
>> +	msleep(78);
>> +
>> +	for (i = 0; i < 2; i++)
>> +		if (mipi_dsi_dcs_enter_sleep_mode(dsis[i]) < 0)
>> +			ret = -ECOMM;
>> +	msleep(78);
>> +
>> +	truly_wqxga_power_off(ctx);
>> +
>> +	ctx->prepared = false;
>> +	return ret;
>> +}
>> +
>> +#define MAX_LEN	5
>> +struct {
>> +	u8 commands[MAX_LEN];
>> +	int size;
>> +} panel_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 },
>> +		   { { 0x08, 0x89 }, 2 },
>> +		   { { 0x09, 0x8A }, 2 },
>> +		   { { 0x0A, 0x13 }, 2 },
>> +		   { { 0x0B, 0x13 }, 2 },
>> +		   { { 0x0C, 0x15 }, 2 },
>> +		   { { 0x0D, 0x15 }, 2 },
>> +		   { { 0x0E, 0x17 }, 2 },
>> +		   { { 0x0F, 0x17 }, 2 },
>> +		   { { 0x10, 0x1C }, 2 },
>> +		   { { 0x11, 0x0B }, 2 },
>> +		   { { 0x12, 0x0C }, 2 },
>> +		   { { 0x13, 0x01 }, 2 },
>> +		   { { 0x14, 0x0F }, 2 },
>> +		   { { 0x15, 0x10 }, 2 },
>> +		   { { 0x16, 0x10 }, 2 },
>> +		   { { 0x17, 0x10 }, 2 },
>> +		   { { 0x18, 0x89 }, 2 },
>> +		   { { 0x19, 0x8A }, 2 },
>> +		   { { 0x1A, 0x13 }, 2 },
>> +		   { { 0x1B, 0x13 }, 2 },
>> +		   { { 0x1C, 0x15 }, 2 },
>> +		   { { 0x1D, 0x15 }, 2 },
>> +		   { { 0x1E, 0x17 }, 2 },
>> +		   { { 0x1F, 0x17 }, 2 },
>> +		   /* STV */
>> +		   { { 0x20, 0x40 }, 2 },
>> +		   { { 0x21, 0x01 }, 2 },
>> +		   { { 0x22, 0x00 }, 2 },
>> +		   { { 0x23, 0x40 }, 2 },
>> +		   { { 0x24, 0x40 }, 2 },
>> +		   { { 0x25, 0x6D }, 2 },
>> +		   { { 0x26, 0x40 }, 2 },
>> +		   { { 0x27, 0x40 }, 2 },
>> +		   /* Vend */
>> +		   { { 0xE0, 0x00 }, 2 },
>> +		   { { 0xDC, 0x21 }, 2 },
>> +		   { { 0xDD, 0x22 }, 2 },
>> +		   { { 0xDE, 0x07 }, 2 },
>> +		   { { 0xDF, 0x07 }, 2 },
>> +		   { { 0xE3, 0x6D }, 2 },
>> +		   { { 0xE1, 0x07 }, 2 },
>> +		   { { 0xE2, 0x07 }, 2 },
>> +		   /* UD */
>> +		   { { 0x29, 0xD8 }, 2 },
>> +		   { { 0x2A, 0x2A }, 2 },
>> +		   /* CLK */
>> +		   { { 0x4B, 0x03 }, 2 },
>> +		   { { 0x4C, 0x11 }, 2 },
>> +		   { { 0x4D, 0x10 }, 2 },
>> +		   { { 0x4E, 0x01 }, 2 },
>> +		   { { 0x4F, 0x01 }, 2 },
>> +		   { { 0x50, 0x10 }, 2 },
>> +		   { { 0x51, 0x00 }, 2 },
>> +		   { { 0x52, 0x80 }, 2 },
>> +		   { { 0x53, 0x00 }, 2 },
>> +		   { { 0x56, 0x00 }, 2 },
>> +		   { { 0x54, 0x07 }, 2 },
>> +		   { { 0x58, 0x07 }, 2 },
>> +		   { { 0x55, 0x25 }, 2 },
>> +		   /* Reset XDONB */
>> +		   { { 0x5B, 0x43 }, 2 },
>> +		   { { 0x5C, 0x00 }, 2 },
>> +		   { { 0x5F, 0x73 }, 2 },
>> +		   { { 0x60, 0x73 }, 2 },
>> +		   { { 0x63, 0x22 }, 2 },
>> +		   { { 0x64, 0x00 }, 2 },
>> +		   { { 0x67, 0x08 }, 2 },
>> +		   { { 0x68, 0x04 }, 2 },
>> +		   /* Resolution:1440x2560 */
>> +		   { { 0x72, 0x02 }, 2 },
>> +		   /* mux */
>> +		   { { 0x7A, 0x80 }, 2 },
>> +		   { { 0x7B, 0x91 }, 2 },
>> +		   { { 0x7C, 0xD8 }, 2 },
>> +		   { { 0x7D, 0x60 }, 2 },
>> +		   { { 0x7F, 0x15 }, 2 },
>> +		   { { 0x75, 0x15 }, 2 },
>> +		   /* ABOFF */
>> +		   { { 0xB3, 0xC0 }, 2 },
>> +		   { { 0xB4, 0x00 }, 2 },
>> +		   { { 0xB5, 0x00 }, 2 },
>> +		   /* Source EQ */
>> +		   { { 0x78, 0x00 }, 2 },
>> +		   { { 0x79, 0x00 }, 2 },
>> +		   { { 0x80, 0x00 }, 2 },
>> +		   { { 0x83, 0x00 }, 2 },
>> +		   /* FP BP */
>> +		   { { 0x93, 0x0A }, 2 },
>> +		   { { 0x94, 0x0A }, 2 },
>> +		   /* Inversion Type */
>> +		   { { 0x8A, 0x00 }, 2 },
>> +		   { { 0x9B, 0xFF }, 2 },
>> +		   /* IMGSWAP =1 @PortSwap=1 */
>> +		   { { 0x9D, 0xB0 }, 2 },
>> +		   { { 0x9F, 0x63 }, 2 },
>> +		   { { 0x98, 0x10 }, 2 },
>> +		   /* FRM */
>> +		   { { 0xEC, 0x00 }, 2 },
>> +		   /* CMD1 */
>> +		   { { 0xFF, 0x10 }, 2 },
>> +		    /* VBP+VSA=,VFP = 10H */
>> +		   { { 0x3B, 0x03, 0x0A, 0x0A, }, 4 },
>> +		   /* FTE on */
>> +		   { { 0x35, 0x00 }, 2 },
>> +		   /* EN_BK =1(auto black) */
>> +		   { { 0xE5, 0x01 }, 2 },
>> +		   /* CMD mode(10) VDO mode(03) */
>> +		   { { 0xBB, 0x03 }, 2 },
>> +		   /* Non Reload MTP */
>> +		   { { 0xFB, 0x01 }, 2 },
>> +		   /* SlpOut + DispOn */
>> +		   //05 01 00 00 78 00 02 11 00
>> +		   //05 01 00 00 78 00 02 29 00
> 
> We can drop the commented lines. If possible, it would be nice
> to have some more description about the registers, but I can imagine
> that info being hard to retrieve.
[Abhinav] Yes, will remove the commented lines. Yes will be hard to give
more info on the regs.
> 
>> +};
>> +
>> +static int truly_wqxga_prepare(struct drm_panel *panel)
>> +{
>> +	struct truly_wqxga *ctx = panel_to_truly_wqxga(panel);
>> +	struct mipi_dsi_device **dsis = ctx->dsi;
>> +	struct mipi_dsi_device *d;
>> +	int ret, i, j;
>> +
>> +	if (ctx->prepared)
>> +		return 0;
>> +
>> +	ret = truly_wqxga_power_on(ctx);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	dsis[0]->mode_flags |= MIPI_DSI_MODE_LPM;
>> +	dsis[1]->mode_flags |= MIPI_DSI_MODE_LPM;
>> +
>> +	for (j = 0; j < ARRAY_SIZE(panel_cmds); j++) {
>> +		for (i = 0; i < 2; i++) {
>> +			d = dsis[i];
>> +			ret = mipi_dsi_dcs_write_buffer(dsis[i],
>> +					panel_cmds[j].commands,
>> +					panel_cmds[j].size);
>> +			if (ret < 0) {
>> +				/* continue anyway */
>> +				dev_err(ctx->dev, "failed to cmd no %d, err: %d\n",
>> +						j, ret);
> 
> This may have been done for debug purposes. It would be probably better
> to bail out.
[Abhinav] Ok, will bail out here.
> 
>> +			}
>> +		}
>> +	}
>> +
>> +
>> +	for (i = 0; i < 2; i++)
>> +		if (mipi_dsi_dcs_exit_sleep_mode(dsis[i]) < 0) {
>> +			dev_err(ctx->dev, "failed to exit sleep mode\n");
>> +			return -ECOMM;
>> +		}
>> +	msleep(78);
>> +
>> +	for (i = 0; i < 2; i++)
>> +		if (mipi_dsi_dcs_set_display_on(dsis[i]) < 0) {
>> +			dev_err(ctx->dev, "failed to send display on\n");
>> +			return -ECOMM;
>> +		}
>> +	msleep(78);
>> +
>> +	ctx->prepared = true;
>> +
>> +	return 0;
>> +}
>> +
>> +static int truly_wqxga_enable(struct drm_panel *panel)
>> +{
>> +	struct truly_wqxga *ctx = panel_to_truly_wqxga(panel);
>> +
>> +	if (ctx->enabled)
>> +		return 0;
>> +
>> +	if (ctx->backlight) {
>> +		ctx->backlight->props.power = FB_BLANK_UNBLANK;
>> +		backlight_update_status(ctx->backlight);
>> +	}
>> +	ctx->enabled = true;
>> +
>> +	return 0;
>> +}
>> +
>> +static int truly_wqxga_get_modes(struct drm_panel *panel)
>> +{
>> +	struct drm_connector *connector = panel->connector;
>> +	struct truly_wqxga *ctx = panel_to_truly_wqxga(panel);
>> +	struct drm_display_mode *mode;
>> +
>> +	mode = drm_mode_create(connector->dev);
>> +	if (!mode) {
>> +		dev_err(ctx->dev, "failed to create a new display mode\n");
>> +		return 0;
>> +	}
>> +
>> +	drm_display_mode_from_videomode(&ctx->vm, mode);
>> +	connector->display_info.width_mm = 74;
>> +	connector->display_info.height_mm = 131;
>> +	mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
>> +	drm_mode_probed_add(connector, mode);
>> +
>> +	return 1;
>> +}
>> +
>> +static const struct drm_panel_funcs truly_wqxga_drm_funcs = {
>> +	.disable = truly_wqxga_disable,
>> +	.unprepare = truly_wqxga_unprepare,
>> +	.prepare = truly_wqxga_prepare,
>> +	.enable = truly_wqxga_enable,
>> +	.get_modes = truly_wqxga_get_modes,
>> +};
>> +
>> +static int truly_wqxga_panel_add(struct truly_wqxga *ctx)
>> +{
>> +	struct device *dev = ctx->dev;
>> +	int ret;
>> +
>> +	ctx->supplies[0].supply = "vddio";
>> +	ctx->supplies[1].supply = "vdda";
>> +	ctx->supplies[2].supply = "lab";
>> +	ctx->supplies[3].supply = "ibb";
>> +	ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(ctx->supplies),
>> +				      ctx->supplies);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(ctx->reset_gpio)) {
>> +		dev_err(dev, "cannot get reset-gpios %ld\n",
>> +			PTR_ERR(ctx->reset_gpio));
>> +		return PTR_ERR(ctx->reset_gpio);
>> +	}
>> +
>> +	ctx->mode_gpio = devm_gpiod_get(dev, "mode", GPIOD_OUT_LOW);
>> +	if (IS_ERR(ctx->mode_gpio)) {
>> +		dev_err(dev, "cannot get mode gpio %ld\n",
>> +			PTR_ERR(ctx->mode_gpio));
>> +		ctx->mode_gpio = NULL;
>> +		return PTR_ERR(ctx->mode_gpio);
>> +	}
>> +
>> +	ret = pinctrl_pm_select_default_state(dev);
>> +	if (ret) {
>> +		dev_err(dev, "%s: failed to set pinctrl default state, %d\n",
>> +			__func__, ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = of_get_videomode(dev->of_node, &ctx->vm, 0);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	drm_panel_init(&ctx->panel);
>> +	ctx->panel.dev = dev;
>> +	ctx->panel.funcs = &truly_wqxga_drm_funcs;
>> +	drm_panel_add(&ctx->panel);
>> +
>> +	return 0;
>> +}
>> +
>> +static void truly_wqxga_panel_del(struct truly_wqxga *ctx)
>> +{
>> +	if (ctx->panel.dev)
>> +		drm_panel_remove(&ctx->panel);
>> +
>> +	if (ctx->backlight)
>> +		put_device(&ctx->backlight->dev);
>> +
>> +	if (ctx->dsi[1])
>> +		put_device(&ctx->dsi[1]->dev);
>> +}
>> +
>> +static int truly_wqxga_probe(struct mipi_dsi_device *dsi)
>> +{
>> +	struct device *dev = &dsi->dev;
>> +	struct truly_wqxga *ctx;
>> +	struct mipi_dsi_device *secondary = NULL;
>> +	struct device_node *np;
>> +	int ret;
>> +
>> +	dsi->lanes = 4;
>> +	dsi->format = MIPI_DSI_FMT_RGB888;
>> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | 
>> MIPI_DSI_CLOCK_NON_CONTINUOUS |
>> +			  MIPI_DSI_MODE_LPM;
>> +
>> +	/* Only DSI-LINK1 node has "link2" entry. */
>> +	np = of_parse_phandle(dsi->dev.of_node, "link2", 0);
>> +	if (np) {
>> +		secondary = of_find_mipi_dsi_device_by_node(np);
>> +		of_node_put(np);
>> +
>> +		if (!secondary)
>> +			return -EPROBE_DEFER;
>> +
>> +		ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>> +		if (!ctx) {
>> +			put_device(&secondary->dev);
>> +			return -ENOMEM;
>> +		}
>> +		mipi_dsi_set_drvdata(dsi, ctx);
>> +
>> +		ctx->dev = dev;
>> +		ctx->dsi[0] = dsi;
>> +		ctx->dsi[1] = secondary;
>> +
>> +		ret = truly_wqxga_panel_add(ctx);
>> +		if (ret) {
>> +			put_device(&secondary->dev);
>> +			return ret;
>> +		}
>> +	}
> 
> This should go when we use the of-graph bindings. The second
> mipi_dsi_device needs to be created manually in the driver
> using mipi_dsi_device_register_full(). You could look at the
> ADV7511 bridge driver as an example. We would eventually need
> to call mipi_dsi_attach() on the second device too.
> 
> Thanks,
> Archit
> 
[Abhinav] I dont think we should follow bridge's example.
In our DT, the panel nodes are listed as child nodes.
The mipi_dsi_host_register API adds all the children of using
of_mipi_dsi_device_add. This calls a mipi_dsi_device_register_full().
twice because the panel is listed as a child on both the DSIs.

In bridge, the bridge driver is not listed as a child
of the DSI node, and hence an explicit call to 
mipi_dsi_device_register_full()
is required.

Hence, yes I will use of-graph bindings and get rid of "link2" but the 
part
to register the panel only for the master controller and allocate ctx 
only then
seems required to me.

Also, an explicit call to mipi_dsi_device_register_full() doesnt seem 
necessary.

Can you please comment whether our understanding is aligned with yours 
here?

>> +
>> +	ret = mipi_dsi_attach(dsi);
>> +	if (ret < 0) {
>> +		if (secondary)
>> +			truly_wqxga_panel_del(ctx);
>> +		return ret;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int truly_wqxga_remove(struct mipi_dsi_device *dsi)
>> +{
>> +	struct truly_wqxga *ctx = mipi_dsi_get_drvdata(dsi);
>> +
>> +	mipi_dsi_detach(dsi);
>> +
>> +	/* delete panel only for the DSI1 interface */
>> +	if (ctx)
>> +		truly_wqxga_panel_del(ctx);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id truly_wqxga_of_match[] = {
>> +	{ .compatible = "truly,dual_wqxga", },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, truly_wqxga_of_match);
>> +
>> +static struct mipi_dsi_driver truly_wqxga_driver = {
>> +	.driver = {
>> +		.name = "panel_truly_wqxga",
>> +		.of_match_table = truly_wqxga_of_match,
>> +	},
>> +	.probe = truly_wqxga_probe,
>> +	.remove = truly_wqxga_remove,
>> +};
>> +module_mipi_dsi_driver(truly_wqxga_driver);
>> +
>> +MODULE_DESCRIPTION("truly wqxga DSI Panel Driver");
>> +MODULE_LICENSE("GPL v2");
>> 
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel