Re: [PATCH v2 2/2] drm/bridge: tc358767: Add DPI to eDP bridge driver

[Archit Taneja <architt@codeaurora.org>]

Hi,

On 07/12/2016 10:51 PM, Philipp Zabel wrote:
> From: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
>
> Add a drm_bridge driver for the Toshiba TC358767 DPI/DSI to
> eDP/DP bridge. Currently only DPI input with 24-bit RGB is
> supported.
>
> Signed-off-by: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> Changes since v1:
>   - Replaced the regmap_read_poll_timeout macro with a function.
>   - Rebased on top of SiI902x driver merge to avoid Makefile/Kconfig conflicts.
>   - Fixed tc_pxl_pll_en as requested: removed unnecessary checks, whitespace
>     and comment improvements.
>   - Switched to atomic connector funcs.
>   - Moved the output port to port@2.
> ---
>   drivers/gpu/drm/bridge/Kconfig    |    8 +
>   drivers/gpu/drm/bridge/Makefile   |    1 +
>   drivers/gpu/drm/bridge/tc358767.c | 1421 +++++++++++++++++++++++++++++++++++++
>   3 files changed, 1430 insertions(+)
>   create mode 100644 drivers/gpu/drm/bridge/tc358767.c
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index a1419214..ebcad29 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -58,6 +58,14 @@ config DRM_SII902X
>   	---help---
>   	  Silicon Image sii902x bridge chip driver.
>
> +config DRM_TOSHIBA_TC358767
> +	tristate "Toshiba TC358767 eDP bridge"

Can we add a 'depends on OF ' here, or wrap around the
'tc->bridge.of_node' access with a CONFIG_OF check? It currently
breaks build for non-OF platforms.

> +	select DRM_KMS_HELPER
> +	select REGMAP_I2C
> +	select DRM_PANEL
> +	---help---
> +	  Toshiba TC358767 eDP bridge chip driver.
> +
>   source "drivers/gpu/drm/bridge/analogix/Kconfig"
>
>   endmenu
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index bfec9f8..42b853a 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -6,4 +6,5 @@ obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o
>   obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
>   obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>   obj-$(CONFIG_DRM_SII902X) += sii902x.o
> +obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>   obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> new file mode 100644
> index 0000000..f1e3a4b
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -0,0 +1,1421 @@
> +/*
> + * tc358767 eDP bridge driver
> + *
> + * Copyright (C) 2016 CogentEmbedded Inc
> + * Author: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
> + *
> + * Copyright (C) 2016 Pengutronix, Philipp Zabel <p.zabel@pengutronix.de>
> + *
> + * Initially based on: drivers/gpu/drm/i2c/tda998x_drv.c
> + *
> + * Copyright (C) 2012 Texas Instruments
> + * Author: Rob Clark <robdclark@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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/clk.h>
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_dp_helper.h>
> +#include <drm/drm_edid.h>
> +#include <drm/drm_of.h>
> +#include <drm/drm_panel.h>
> +
> +/* Registers */
> +
> +/* Display Parallel Interface */
> +#define DPIPXLFMT		0x0440
> +#define VS_POL_ACTIVE_LOW		(1 << 10)
> +#define HS_POL_ACTIVE_LOW		(1 << 9)
> +#define DE_POL_ACTIVE_HIGH		(0 << 8)
> +#define SUB_CFG_TYPE_CONFIG1		(0 << 2) /* LSB aligned */
> +#define SUB_CFG_TYPE_CONFIG2		(1 << 2) /* Loosely Packed */
> +#define SUB_CFG_TYPE_CONFIG3		(2 << 2) /* LSB aligned 8-bit */
> +#define DPI_BPP_RGB888			(0 << 0)
> +#define DPI_BPP_RGB666			(1 << 0)
> +#define DPI_BPP_RGB565			(2 << 0)
> +
> +/* Video Path */
> +#define VPCTRL0			0x0450
> +#define OPXLFMT_RGB666			(0 << 8)
> +#define OPXLFMT_RGB888			(1 << 8)
> +#define FRMSYNC_DISABLED		(0 << 4) /* Video Timing Gen Disabled */
> +#define FRMSYNC_ENABLED			(1 << 4) /* Video Timing Gen Enabled */
> +#define MSF_DISABLED			(0 << 0) /* Magic Square FRC disabled */
> +#define MSF_ENABLED			(1 << 0) /* Magic Square FRC enabled */
> +#define HTIM01			0x0454
> +#define HTIM02			0x0458
> +#define VTIM01			0x045c
> +#define VTIM02			0x0460
> +#define VFUEN0			0x0464
> +#define VFUEN				BIT(0)   /* Video Frame Timing Upload */
> +
> +/* System */
> +#define TC_IDREG		0x0500
> +#define SYSCTRL			0x0510
> +#define DP0_AUDSRC_NO_INPUT		(0 << 3)
> +#define DP0_AUDSRC_I2S_RX		(1 << 3)
> +#define DP0_VIDSRC_NO_INPUT		(0 << 0)
> +#define DP0_VIDSRC_DSI_RX		(1 << 0)
> +#define DP0_VIDSRC_DPI_RX		(2 << 0)
> +#define DP0_VIDSRC_COLOR_BAR		(3 << 0)
> +
> +/* Control */
> +#define DP0CTL			0x0600
> +#define VID_MN_GEN			BIT(6)   /* Auto-generate M/N values */
> +#define EF_EN				BIT(5)   /* Enable Enhanced Framing */
> +#define VID_EN				BIT(1)   /* Video transmission enable */
> +#define DP_EN				BIT(0)   /* Enable DPTX function */
> +
> +/* Clocks */
> +#define DP0_VIDMNGEN0		0x0610
> +#define DP0_VIDMNGEN1		0x0614
> +#define DP0_VMNGENSTATUS	0x0618
> +
> +/* Main Channel */
> +#define DP0_SECSAMPLE		0x0640
> +#define DP0_VIDSYNCDELAY	0x0644
> +#define DP0_TOTALVAL		0x0648
> +#define DP0_STARTVAL		0x064c
> +#define DP0_ACTIVEVAL		0x0650
> +#define DP0_SYNCVAL		0x0654
> +#define DP0_MISC		0x0658
> +#define TU_SIZE_RECOMMENDED		(0x3f << 16) /* LSCLK cycles per TU */
> +#define BPC_6				(0 << 5)
> +#define BPC_8				(1 << 5)
> +
> +/* AUX channel */
> +#define DP0_AUXCFG0		0x0660
> +#define DP0_AUXCFG1		0x0664
> +#define AUX_RX_FILTER_EN		BIT(16)
> +
> +#define DP0_AUXADDR		0x0668
> +#define DP0_AUXWDATA(i)		(0x066c + (i) * 4)
> +#define DP0_AUXRDATA(i)		(0x067c + (i) * 4)
> +#define DP0_AUXSTATUS		0x068c
> +#define AUX_STATUS_MASK			0xf0
> +#define AUX_STATUS_SHIFT		4
> +#define AUX_TIMEOUT			BIT(1)
> +#define AUX_BUSY			BIT(0)
> +#define DP0_AUXI2CADR		0x0698
> +
> +/* Link Training */
> +#define DP0_SRCCTRL		0x06a0
> +#define DP0_SRCCTRL_SCRMBLDIS		BIT(13)
> +#define DP0_SRCCTRL_EN810B		BIT(12)
> +#define DP0_SRCCTRL_NOTP		(0 << 8)
> +#define DP0_SRCCTRL_TP1			(1 << 8)
> +#define DP0_SRCCTRL_TP2			(2 << 8)
> +#define DP0_SRCCTRL_LANESKEW		BIT(7)
> +#define DP0_SRCCTRL_SSCG		BIT(3)
> +#define DP0_SRCCTRL_LANES_1		(0 << 2)
> +#define DP0_SRCCTRL_LANES_2		(1 << 2)
> +#define DP0_SRCCTRL_BW27		(1 << 1)
> +#define DP0_SRCCTRL_BW162		(0 << 1)
> +#define DP0_SRCCTRL_AUTOCORRECT		BIT(0)
> +#define DP0_LTSTAT		0x06d0
> +#define LT_LOOPDONE			BIT(13)
> +#define LT_STATUS_MASK			(0x1f << 8)
> +#define LT_CHANNEL1_EQ_BITS		(DP_CHANNEL_EQ_BITS << 4)
> +#define LT_INTERLANE_ALIGN_DONE		BIT(3)
> +#define LT_CHANNEL0_EQ_BITS		(DP_CHANNEL_EQ_BITS)
> +#define DP0_SNKLTCHGREQ		0x06d4
> +#define DP0_LTLOOPCTRL		0x06d8
> +#define DP0_SNKLTCTRL		0x06e4
> +
> +/* PHY */
> +#define DP_PHY_CTRL		0x0800
> +#define DP_PHY_RST			BIT(28)  /* DP PHY Global Soft Reset */
> +#define BGREN				BIT(25)  /* AUX PHY BGR Enable */
> +#define PWR_SW_EN			BIT(24)  /* PHY Power Switch Enable */
> +#define PHY_M1_RST			BIT(12)  /* Reset PHY1 Main Channel */
> +#define PHY_RDY				BIT(16)  /* PHY Main Channels Ready */
> +#define PHY_M0_RST			BIT(8)   /* Reset PHY0 Main Channel */
> +#define PHY_A0_EN			BIT(1)   /* PHY Aux Channel0 Enable */
> +#define PHY_M0_EN			BIT(0)   /* PHY Main Channel0 Enable */
> +
> +/* PLL */
> +#define DP0_PLLCTRL		0x0900
> +#define DP1_PLLCTRL		0x0904	/* not defined in DS */
> +#define PXL_PLLCTRL		0x0908
> +#define PLLUPDATE			BIT(2)
> +#define PLLBYP				BIT(1)
> +#define PLLEN				BIT(0)
> +#define PXL_PLLPARAM		0x0914
> +#define IN_SEL_REFCLK			(0 << 14)
> +#define SYS_PLLPARAM		0x0918
> +#define REF_FREQ_38M4			(0 << 8) /* 38.4 MHz */
> +#define REF_FREQ_19M2			(1 << 8) /* 19.2 MHz */
> +#define REF_FREQ_26M			(2 << 8) /* 26 MHz */
> +#define REF_FREQ_13M			(3 << 8) /* 13 MHz */
> +#define SYSCLK_SEL_LSCLK		(0 << 4)
> +#define LSCLK_DIV_1			(0 << 0)
> +#define LSCLK_DIV_2			(1 << 0)
> +
> +/* Test & Debug */
> +#define TSTCTL			0x0a00
> +#define PLL_DBG			0x0a04
> +
> +struct tc_edp_link {
> +	struct drm_dp_link	base;
> +	u8			assr;
> +	int			scrambler_dis;
> +	int			spread;
> +	int			coding8b10b;
> +	u8			swing;
> +	u8			preemp;
> +};
> +
> +struct tc_data {
> +	struct device		*dev;
> +	struct regmap		*regmap;
> +	struct drm_dp_aux	aux;
> +
> +	struct drm_bridge	bridge;
> +	struct drm_connector	connector;
> +	struct drm_panel	*panel;
> +
> +	/* link settings */
> +	struct tc_edp_link	link;
> +
> +	/* display edid */
> +	struct edid		*edid;
> +	/* current mode */
> +	struct drm_display_mode	*mode;
> +
> +	/* PLL pixelclock */
> +	u32			pll_clk;
> +	u32			pll_clk_real;
> +
> +	int			test_pattern;

This still doesn't seem to be initialized. Didn't we want this
to be a module param? If we're not sure yet, it would be fine
just to set tc->test_pattern explicitly to 0 and add a comment.

Patch looks good otherwise.

Thanks,
Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel