Re: [PATCH v6 2/2] display/drm/bridge: TC358775 DSI/LVDS driver
Hi Vinay. On Thu, Jul 02, 2020 at 06:06:34PM +0530, Vinay Simha BN wrote: > This driver is tested with two panels individually with Apq8016-IFC6309 board > https://www.inforcecomputing.com/products/single-board-computers-sbc/qualcomm-snapdragon-410-inforce-6309-micro-sbc > > 1. 1366x768@60 auo,b101xtn01 data-mapping = "jeida-24" > 2. 800x480@60 innolux,at070tn92 data-mapping = "vesa-24" > > Signed-off-by: Vinay Simha BN > > --- > v1: > Initial version > > v2: > * Andrzej Hajda review comments incorporated > SPDX identifier > development debug removed > alphabetic order headers > u32 instead of unit32_t > magic numbers to macros for CLRSI and mux registers > ignored return value > > * Laurent Pinchart review comments incorporated > mdelay to usleep_range > bus_formats added > > v3: > * Andrzej Hajda review comments incorporated > drm_connector_status removed > u32 rev removed and local variabl is used > regulator enable disable with proper orders and delays > as per the spec > devm_drm_panel_bridge_add method used instead of panel > description modified > dual port implemented > > v4: > * Sam Ravnborg review comments incorporated > panel->connector_type removed > > * Reported-by: kernel test robot > parse_dt to static function > removed the if (endpoint), since data-lanes has to be > present for dsi dts ports > > v5: > ~vsdelay dynamic value set based on the > calculation of dsi speed, output speed, blanking > > v6: > * Sam Ravnborg review comments incorporated > help modified > display_timings naming local variables > check for bus_formats unsupported > error handling enpoint data-lanes There are some details that I missed in last review - see below. Trivial stuff only, I do not have knowledge to review all of the driver properly. Sam > --- > drivers/gpu/drm/bridge/Kconfig| 10 + > drivers/gpu/drm/bridge/Makefile | 1 + > drivers/gpu/drm/bridge/tc358775.c | 766 ++ > 3 files changed, 777 insertions(+) > create mode 100644 drivers/gpu/drm/bridge/tc358775.c > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index 43271c21d3fc..99abda4459ab 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -181,6 +181,16 @@ config DRM_TOSHIBA_TC358768 > help > Toshiba TC358768AXBG/TC358778XBG DSI bridge chip driver. > > +config DRM_TOSHIBA_TC358775 > +tristate "Toshiba TC358775 DSI/LVDS bridge" > +depends on OF > +select DRM_KMS_HELPER > +select REGMAP_I2C > +select DRM_PANEL > + select DRM_MIPI_DSI > +help > + Toshiba TC358775 DSI/LVDS bridge chip driver. Use one tab for indent, not spaces. Help text needs to have an indent of one tab + two spaces. > + > config DRM_TI_TFP410 > tristate "TI TFP410 DVI/HDMI bridge" > depends on OF > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > index d63d4b7e4347..23c770b3bfe4 100644 > --- a/drivers/gpu/drm/bridge/Makefile > +++ b/drivers/gpu/drm/bridge/Makefile > @@ -15,6 +15,7 @@ obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o > obj-$(CONFIG_DRM_TOSHIBA_TC358764) += tc358764.o > obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o > obj-$(CONFIG_DRM_TOSHIBA_TC358768) += tc358768.o > +obj-$(CONFIG_DRM_TOSHIBA_TC358775) += tc358775.o > obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/ > obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o > obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o > diff --git a/drivers/gpu/drm/bridge/tc358775.c > b/drivers/gpu/drm/bridge/tc358775.c > new file mode 100644 > index ..88a05b9645e8 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/tc358775.c > @@ -0,0 +1,766 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * tc358775 DSI to LVDS bridge driver > + * > + * Copyright (C) 2020 SMART Wireless Computing > + * Author: Vinay Simha BN > + * > + */ > +/* #define DEBUG */ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define FLD_MASK(start, end)(((1 << ((start) - (end) + 1)) - 1) << (end)) > +#define FLD_VAL(val, start, end) (((val) << (end)) & FLD_MASK(start, end)) I did no dechiper the above, but it looks like an opencoded variant of GENMASK() If it is so, then consider using GENMASK() as this is the official way to do it. (linux/bits.h) (Yes, I know FLD_* is used on other bridge drivers). > + > +/* Registers */ > + > +/* DSI D-PHY Layer Registers */ > +#define D0W_DPHYCONTTX 0x0004 /* Data Lane 0 DPHY Tx Control */ > +#define CLW_DPHYCONTRX 0x0020 /* Clock Lane DPHY Rx Control */ > +#define D0W_DPHYCONTRX 0x0024 /* Data Lane 0 DPHY Rx Control */ > +#define D1W_DPHYCONTRX 0x0028 /* Data Lane 1 DPHY Rx Control */ > +#define D2W_DPHYCONTRX 0x002C /* Data Lane 2 DPHY Rx
Re: [PATCH v6 2/2] display/drm/bridge: TC358775 DSI/LVDS driver
Hi Vinay,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.8-rc3 next-20200702]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Vinay-Simha-BN/dt-binding-Add-DSI-LVDS-TC358775-bridge-bindings/20200702-203915
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
cd77006e01b3198c75fb7819b3d0ff89709539bb
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project
003a086ffc0d1affbb8300b36225fb8150a2d40a)
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/bridge/tc358775.c:457:2: warning: variable 'bpc' is used
>> uninitialized whenever switch default is taken [-Wsometimes-uninitialized]
default:
^~~
drivers/gpu/drm/bridge/tc358775.c:464:34: note: uninitialized use occurs here
dsiclk = mode->crtc_clock * 3 * bpc / tc->num_dsi_lanes / 1000;
^~~
drivers/gpu/drm/bridge/tc358775.c:387:14: note: initialize the variable
'bpc' to silence this warning
u8 link, bpc;
^
= '\0'
>> drivers/gpu/drm/bridge/tc358775.c:527:1: warning: no previous prototype for
>> function 'tc_mode_valid' [-Wmissing-prototypes]
tc_mode_valid(struct drm_bridge *bridge,
^
drivers/gpu/drm/bridge/tc358775.c:526:1: note: declare 'static' if the
function is not intended to be used outside of this translation unit
enum drm_mode_status
^
static
drivers/gpu/drm/bridge/tc358775.c:566:8: warning: variable 'len' is used
uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (endpoint) {
^~~~
drivers/gpu/drm/bridge/tc358775.c:579:22: note: uninitialized use occurs here
tc->num_dsi_lanes = len / sizeof(u32);
^~~
drivers/gpu/drm/bridge/tc358775.c:566:4: note: remove the 'if' if its
condition is always true
if (endpoint) {
^~
drivers/gpu/drm/bridge/tc358775.c:562:7: warning: variable 'len' is used
uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (parent) {
^~
drivers/gpu/drm/bridge/tc358775.c:579:22: note: uninitialized use occurs here
tc->num_dsi_lanes = len / sizeof(u32);
^~~
drivers/gpu/drm/bridge/tc358775.c:562:3: note: remove the 'if' if its
condition is always true
if (parent) {
^~~~
drivers/gpu/drm/bridge/tc358775.c:558:6: warning: variable 'len' is used
uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (endpoint) {
^~~~
drivers/gpu/drm/bridge/tc358775.c:579:22: note: uninitialized use occurs here
tc->num_dsi_lanes = len / sizeof(u32);
^~~
drivers/gpu/drm/bridge/tc358775.c:558:2: note: remove the 'if' if its
condition is always true
if (endpoint) {
^~
drivers/gpu/drm/bridge/tc358775.c:550:9: note: initialize the variable 'len'
to silence this warning
int len;
^
= 0
drivers/gpu/drm/bridge/tc358775.c:662:16: error: incompatible function
pointer types initializing 'enum drm_mode_status (*)(struct drm_bridge *, const
struct drm_display_mode *)' with an expression of type 'enum drm_mode_status
(struct drm_bridge *, const struct drm_display_info *, const struct
drm_display_mode *)' [-Werror,-Wincompatible-function-pointer-types]
.mode_valid = tc_mode_valid,
^
5 warnings and 1 error generated.
vim +/bpc +457 drivers/gpu/drm/bridge/tc358775.c
379
380 static void tc_bridge_enable(struct drm_bridge *bridge)
381 {
382 struct tc_data *tc = bridge_to_tc(bridge);
383 u32 hback_porch, hsync_len, hfront_porch, hactive, htime1,
htime2;
384 u32 vback_porch, vsync_len, vfront_porch, vactive, vtime1,
vtime2;
385 u32 val = 0, val_lvcfg = 0;
386 u16 dsiclk,
Re: [PATCH v6 2/2] display/drm/bridge: TC358775 DSI/LVDS driver
Hi Vinay,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.8-rc3 next-20200702]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]
url:
https://github.com/0day-ci/linux/commits/Vinay-Simha-BN/dt-binding-Add-DSI-LVDS-TC358775-bridge-bindings/20200702-203915
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
cd77006e01b3198c75fb7819b3d0ff89709539bb
config: mips-allyesconfig (attached as .config)
compiler: mips-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross
ARCH=mips
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot
All warnings (new ones prefixed by >>):
>> drivers/gpu/drm/bridge/tc358775.c:527:1: warning: no previous prototype for
>> 'tc_mode_valid' [-Wmissing-prototypes]
527 | tc_mode_valid(struct drm_bridge *bridge,
| ^
drivers/gpu/drm/bridge/tc358775.c:662:16: error: initialization of 'enum
drm_mode_status (*)(struct drm_bridge *, const struct drm_display_mode *)' from
incompatible pointer type 'enum drm_mode_status (*)(struct drm_bridge *, const
struct drm_display_info *, const struct drm_display_mode *)'
[-Werror=incompatible-pointer-types]
662 | .mode_valid = tc_mode_valid,
|^
drivers/gpu/drm/bridge/tc358775.c:662:16: note: (near initialization for
'tc_bridge_funcs.mode_valid')
cc1: some warnings being treated as errors
vim +/tc_mode_valid +527 drivers/gpu/drm/bridge/tc358775.c
525
526 enum drm_mode_status
> 527 tc_mode_valid(struct drm_bridge *bridge,
528const struct drm_display_info *info,
529const struct drm_display_mode *mode)
530 {
531 struct tc_data *tc = bridge_to_tc(bridge);
532
533 /*
534 * Maximum pixel clock speed 135MHz for single-link
535 * 270MHz for dual-link
536 */
537 if ((mode->clock > 135000 && tc->dual_link) ||
538 (mode->clock > 27 && tc->dual_link))
539 return MODE_CLOCK_HIGH;
540
541 return MODE_OK;
542 }
543
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org
.config.gz
Description: application/gzip
[PATCH v6 2/2] display/drm/bridge: TC358775 DSI/LVDS driver
This driver is tested with two panels individually with Apq8016-IFC6309 board https://www.inforcecomputing.com/products/single-board-computers-sbc/qualcomm-snapdragon-410-inforce-6309-micro-sbc 1. 1366x768@60 auo,b101xtn01 data-mapping = "jeida-24" 2. 800x480@60 innolux,at070tn92 data-mapping = "vesa-24" Signed-off-by: Vinay Simha BN --- v1: Initial version v2: * Andrzej Hajda review comments incorporated SPDX identifier development debug removed alphabetic order headers u32 instead of unit32_t magic numbers to macros for CLRSI and mux registers ignored return value * Laurent Pinchart review comments incorporated mdelay to usleep_range bus_formats added v3: * Andrzej Hajda review comments incorporated drm_connector_status removed u32 rev removed and local variabl is used regulator enable disable with proper orders and delays as per the spec devm_drm_panel_bridge_add method used instead of panel description modified dual port implemented v4: * Sam Ravnborg review comments incorporated panel->connector_type removed * Reported-by: kernel test robot parse_dt to static function removed the if (endpoint), since data-lanes has to be present for dsi dts ports v5: ~vsdelay dynamic value set based on the calculation of dsi speed, output speed, blanking v6: * Sam Ravnborg review comments incorporated help modified display_timings naming local variables check for bus_formats unsupported error handling enpoint data-lanes --- drivers/gpu/drm/bridge/Kconfig| 10 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/tc358775.c | 766 ++ 3 files changed, 777 insertions(+) create mode 100644 drivers/gpu/drm/bridge/tc358775.c diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 43271c21d3fc..99abda4459ab 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -181,6 +181,16 @@ config DRM_TOSHIBA_TC358768 help Toshiba TC358768AXBG/TC358778XBG DSI bridge chip driver. +config DRM_TOSHIBA_TC358775 +tristate "Toshiba TC358775 DSI/LVDS bridge" +depends on OF +select DRM_KMS_HELPER +select REGMAP_I2C +select DRM_PANEL + select DRM_MIPI_DSI +help + Toshiba TC358775 DSI/LVDS bridge chip driver. + config DRM_TI_TFP410 tristate "TI TFP410 DVI/HDMI bridge" depends on OF diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile index d63d4b7e4347..23c770b3bfe4 100644 --- a/drivers/gpu/drm/bridge/Makefile +++ b/drivers/gpu/drm/bridge/Makefile @@ -15,6 +15,7 @@ obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o obj-$(CONFIG_DRM_TOSHIBA_TC358764) += tc358764.o obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o obj-$(CONFIG_DRM_TOSHIBA_TC358768) += tc358768.o +obj-$(CONFIG_DRM_TOSHIBA_TC358775) += tc358775.o obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/ obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o diff --git a/drivers/gpu/drm/bridge/tc358775.c b/drivers/gpu/drm/bridge/tc358775.c new file mode 100644 index ..88a05b9645e8 --- /dev/null +++ b/drivers/gpu/drm/bridge/tc358775.c @@ -0,0 +1,766 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * tc358775 DSI to LVDS bridge driver + * + * Copyright (C) 2020 SMART Wireless Computing + * Author: Vinay Simha BN + * + */ +/* #define DEBUG */ +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include + +#define FLD_MASK(start, end)(((1 << ((start) - (end) + 1)) - 1) << (end)) +#define FLD_VAL(val, start, end) (((val) << (end)) & FLD_MASK(start, end)) + +/* Registers */ + +/* DSI D-PHY Layer Registers */ +#define D0W_DPHYCONTTX 0x0004 /* Data Lane 0 DPHY Tx Control */ +#define CLW_DPHYCONTRX 0x0020 /* Clock Lane DPHY Rx Control */ +#define D0W_DPHYCONTRX 0x0024 /* Data Lane 0 DPHY Rx Control */ +#define D1W_DPHYCONTRX 0x0028 /* Data Lane 1 DPHY Rx Control */ +#define D2W_DPHYCONTRX 0x002C /* Data Lane 2 DPHY Rx Control */ +#define D3W_DPHYCONTRX 0x0030 /* Data Lane 3 DPHY Rx Control */ +#define COM_DPHYCONTRX 0x0038 /* DPHY Rx Common Control */ +#define CLW_CNTRL 0x0040 /* Clock Lane Control */ +#define D0W_CNTRL 0x0044 /* Data Lane 0 Control */ +#define D1W_CNTRL 0x0048 /* Data Lane 1 Control */ +#define D2W_CNTRL 0x004C /* Data Lane 2 Control */ +#define D3W_CNTRL 0x0050 /* Data Lane 3 Control */ +#define DFTMODE_CNTRL 0x0054 /* DFT Mode Control */ + +/* DSI PPI Layer Registers */ +#define PPI_STARTPPI0x0104 /* START control bit of PPI-TX function. */ +#define PPI_START_FUNCTION 1 + +#define PPI_BUSYPPI 0x0108 +#define PPI_LINEINITCNT 0x0110 /* Line Initialization Wait Counter */ +#define PPI_LPTXTIMECNT 0x0114 +#define PPI_LANEENABLE 0x0134 /* Enables each lane at
