Message-ID:
 <DB7PR04MB4010B2B32D48F2288EF5C9FC9422A@DB7PR04MB4010.eurprd04.prod.outlook.com>
Date: Tue, 5 Aug 2025 02:23:54 +0000
From: Joseph Guo <qijian.guo@....com>
To: Ying Liu <victor.liu@....com>, Andrzej Hajda <andrzej.hajda@...el.com>,
	Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>, Jonas Karlman
	<jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>, David Airlie
	<airlied@...il.com>, Simona Vetter <simona@...ll.ch>, Maarten Lankhorst
	<maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
	Jessica Zhang <quic_jesszhan@...cinc.com>, Thierry Reding
	<thierry.reding@...il.com>, Sam Ravnborg <sam@...nborg.org>
CC: "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 3/3] drm: bridge: Add waveshare DSI2DPI unit driver

Hi Ying

> -----Original Message-----
> From: Ying Liu <victor.liu@....com>
> Sent: Tuesday, August 5, 2025 10:22 AM
> To: Joseph Guo <qijian.guo@....com>; Andrzej Hajda
> <andrzej.hajda@...el.com>; Neil Armstrong <neil.armstrong@...aro.org>;
> Robert Foss <rfoss@...nel.org>; Laurent Pinchart
> <laurent.pinchart@...asonboard.com>; Jonas Karlman <jonas@...boo.se>;
> Jernej Skrabec <jernej.skrabec@...il.com>; David Airlie
> <airlied@...il.com>; Simona Vetter <simona@...ll.ch>; Maarten Lankhorst
> <maarten.lankhorst@...ux.intel.com>; Maxime Ripard
> <mripard@...nel.org>; Thomas Zimmermann <tzimmermann@...e.de>;
> Rob Herring <robh@...nel.org>; Krzysztof Kozlowski <krzk+dt@...nel.org>;
> Conor Dooley <conor+dt@...nel.org>; Jessica Zhang
> <quic_jesszhan@...cinc.com>; Thierry Reding <thierry.reding@...il.com>;
> Sam Ravnborg <sam@...nborg.org>
> Cc: dri-devel@...ts.freedesktop.org; devicetree@...r.kernel.org; linux-
> kernel@...r.kernel.org
> Subject: Re: [PATCH v2 3/3] drm: bridge: Add waveshare DSI2DPI unit driver
> 
> Hi Joseph,
> 
> On 08/04/2025, Joseph Guo wrote:
> > Waveshare touchscreen consists of a DPI panel and a driver board.
> > The waveshare driver board consists of ICN6211 and a MCU to convert
> > DSI to DPI and control the backlight.
> > This driver treats the MCU and ICN6211 board as a whole unit.
> > It can support all resolution waveshare DSI2DPI based panel, the
> > timing table should come from 'panel-dpi' panel in the device tree.
> >
> > Signed-off-by: Joseph Guo <qijian.guo@....com>
> 
> For next version, you may add:
> Suggested-by: Liu Ying <victor.liu@....com>
> 
> > ---
> >  drivers/gpu/drm/bridge/Kconfig         |  11 ++
> >  drivers/gpu/drm/bridge/Makefile        |   1 +
> >  drivers/gpu/drm/bridge/waveshare-dsi.c | 210
> > +++++++++++++++++++++++++++++++++
> >  3 files changed, 222 insertions(+)
> 
> This patch doesn't apply to drm-misc-next cleanly.  I see conflicts in Kconfig
> and Makefile.  It seems that you generate the patch series based on NXP
> down stream kernel instead of the upstream kernel.
> 
[Joseph] OK, I will generate the patch based on upstream kernel in next version instead.
> >
> > diff --git a/drivers/gpu/drm/bridge/Kconfig
> > b/drivers/gpu/drm/bridge/Kconfig index
> >
> cb3b797fcea1c73e83c9187fef6582296b340305..26fec25c61ed7d950c094e0224
> f1
> > 196946079485 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -472,4 +472,15 @@ config DRM_ITE_IT6161
> >  	help
> >  	  ITE IT6161 bridge chip driver.
> >
> > +config DRM_WAVESHARE_BRIDGE
> 
> Sort the config names alphabetically.
> 
> > +	tristate "Waveshare DSI bridge"
> 
> depends on BACKLIGHT_CLASS_DEVICE
> 
> > +	depends on OF
> > +	select DRM_PANEL_BRIDGE
> > +	select DRM_KMS_HELPER
> > +	select DRM_MIPI_DSI
> > +	select REGMAP_I2C
> > +	help
> > +	  Driver for waveshare DSI to DPI bridge board.
> > +	  Please say Y if you have such hardware
> > +
> >  endmenu
> > diff --git a/drivers/gpu/drm/bridge/Makefile
> > b/drivers/gpu/drm/bridge/Makefile index
> >
> d1db90688a150fdc3a5fd40acebe740798c452b0..3caa4d8f71675804328aa5a51
> ec6
> > 7b2587938621 100644
> > --- a/drivers/gpu/drm/bridge/Makefile
> > +++ b/drivers/gpu/drm/bridge/Makefile
> > @@ -48,3 +48,4 @@ obj-$(CONFIG_DRM_ITE_IT6263) += it6263.o
> >  obj-$(CONFIG_DRM_ITE_IT6161) += it6161.o
> >  obj-$(CONFIG_DRM_SEC_MIPI_DSIM) += sec-dsim.o
> >  obj-$(CONFIG_DRM_NXP_SEIKO_43WVFIG) += nxp-seiko-43wvfig.o
> > +obj-$(CONFIG_DRM_WAVESHARE_BRIDGE) += waveshare-dsi.o
> 
> Sort the config names alphabetically with the best effort.
> 
> > diff --git a/drivers/gpu/drm/bridge/waveshare-dsi.c
> > b/drivers/gpu/drm/bridge/waveshare-dsi.c
> > new file mode 100644
> > index
> >
> 0000000000000000000000000000000000000000..efb3a2fc501b5725b02f498625
> 26
> > d1704a3a4b7b
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/waveshare-dsi.c
> > @@ -0,0 +1,210 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + *  Copyright 2025 NXP
> 
> Nit: Drop a space between * and C.
> 
> > + * Based on panel-raspberrypi-touchscreen by Broadcom */
> > +
> > +#include <linux/backlight.h>
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_graph.h>
> > +#include <linux/regmap.h>
> > +
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_bridge.h>
> > +#include <drm/drm_mipi_dsi.h>
> > +#include <drm/drm_of.h>
> > +#include <drm/drm_panel.h>
> > +#include <drm/drm_print.h>
> > +
> > +struct ws_bridge {
> > +	struct drm_bridge bridge;
> > +	struct drm_bridge *next_bridge;
> > +	struct backlight_device *backlight;
> > +	struct device *dev;
> > +	struct regmap *reg_map;
> > +};
> > +
> > +static const struct regmap_config ws_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +	.max_register = 0xff,
> > +	.disable_debugfs = true,
> 
> drivers/gpu/drm/bridge/waveshare-dsi.c:34:10: error: ‘const struct
> regmap_config’ has no member named ‘disable_debugfs’
>    34 |         .disable_debugfs = true,
>       |          ^~~~~~~~~~~~~~~
> 
> > +};
> > +
> > +static struct ws_bridge *bridge_to_ws_bridge(struct drm_bridge
> > +*bridge) {
> > +	return container_of(bridge, struct ws_bridge, bridge); }
> > +
> > +static int ws_bridge_attach_dsi(struct ws_bridge *ws) {
> > +	struct device_node *dsi_host_node;
> > +	struct mipi_dsi_host *host;
> > +	struct mipi_dsi_device *dsi;
> > +	const struct mipi_dsi_device_info info = {
> > +		.type = "ws-bridge",
> > +		.channel = 0,
> > +		.node = NULL,
> > +	};
> > +	struct device *dev = ws->dev;
> > +	int ret;
> 
> Nit: Sort these variables in reverse Christmas tree fashion.
> 
> > +
> > +	dsi_host_node = of_graph_get_remote_node(dev->of_node, 0, 0);
> > +	if (!dsi_host_node) {
> > +		dev_err(dev, "Failed to get remote port\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	host = of_find_mipi_dsi_host_by_node(dsi_host_node);
> > +
> 
> Nit: Drop this blank line.
> 
> > +	of_node_put(dsi_host_node);
> > +	if (!host)
> > +		return dev_err_probe(dev, -EPROBE_DEFER, "Failed to find
> > +dsi_host\n");
> > +
> > +	dsi = devm_mipi_dsi_device_register_full(dev, host, &info);
> > +
> 
> Ditto.
> 
> > +	if (IS_ERR(dsi))
> > +		return dev_err_probe(dev, PTR_ERR(dsi), "Failed to create
> dsi
> > +device\n");
> > +
> > +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO_HSE |
> MIPI_DSI_MODE_VIDEO |
> > +			  MIPI_DSI_CLOCK_NON_CONTINUOUS;
> > +	dsi->format = MIPI_DSI_FMT_RGB888;
> > +	dsi->lanes = 2;
> > +
> > +	ret = devm_mipi_dsi_attach(dev, dsi);
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "Failed to attach dsi to
> host\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static int ws_bridge_bridge_attach(struct drm_bridge *bridge,
> > +				   enum drm_bridge_attach_flags flags)
> 
> drivers/gpu/drm/bridge/waveshare-dsi.c:117:19: error: initialization of ‘int
> (*)(struct drm_bridge *, struct drm_encoder *, enum
> drm_bridge_attach_flags)’ from incompatible pointer type ‘int (*)(struct
> drm_bridge *, enum drm_bridge_attach_flags)’ [-Werror=incompatible-
> pointer-types]
[Joseph] It seems all the build error and warning came from the different version of downstream and upstream. I will resolve them all in v3. Thank you.

Regards,
Joseph
>   117 |         .attach = ws_bridge_bridge_attach,
>       |                   ^~~~~~~~~~~~~~~~~~~~~~~
> 
> > +{
> > +	struct ws_bridge *ws = bridge_to_ws_bridge(bridge);
> > +	int ret;
> > +
> > +	ret = ws_bridge_attach_dsi(ws);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return drm_bridge_attach(ws->bridge.encoder, ws->next_bridge,
> > +				 &ws->bridge, flags);
> > +}
> > +
> > +static void ws_bridge_bridge_enable(struct drm_bridge *bridge) {
> > +	struct ws_bridge *ws = bridge_to_ws_bridge(bridge);
> > +
> > +	regmap_write(ws->reg_map, 0xad, 0x01);
> > +	backlight_enable(ws->backlight);
> > +}
> > +
> > +static void ws_bridge_bridge_disable(struct drm_bridge *bridge) {
> > +	struct ws_bridge *ws = bridge_to_ws_bridge(bridge);
> > +
> > +	backlight_disable(ws->backlight);
> > +	regmap_write(ws->reg_map, 0xad, 0x00); }
> > +
> > +static const struct drm_bridge_funcs ws_bridge_bridge_funcs = {
> > +	.enable = ws_bridge_bridge_enable,
> > +	.disable = ws_bridge_bridge_disable,
> > +	.attach = ws_bridge_bridge_attach,
> > +};
> > +
> > +static int ws_bridge_bl_update_status(struct backlight_device *bl) {
> > +	struct ws_bridge *ws = bl_get_data(bl);
> > +
> > +	regmap_write(ws->reg_map, 0xab, 0xff -
> backlight_get_brightness(bl));
> > +	regmap_write(ws->reg_map, 0xaa, 0x01);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct backlight_ops ws_bridge_bl_ops = {
> > +	.update_status = ws_bridge_bl_update_status, };
> > +
> > +static struct backlight_device *ws_bridge_create_backlight(struct
> > +ws_bridge *ws) {
> > +	struct device *dev = ws->dev;
> > +	const struct backlight_properties props = {
> > +		.type = BACKLIGHT_RAW,
> > +		.brightness = 255,
> > +		.max_brightness = 255,
> > +	};
> 
> Nit: Sort these variables in reverse Christmas tree fashion.
> 
> > +
> > +	return devm_backlight_device_register(dev, dev_name(dev), dev,
> ws,
> > +					      &ws_bridge_bl_ops, &props); }
> > +
> > +static int ws_bridge_probe(struct i2c_client *i2c) {
> > +	struct device *dev = &i2c->dev;
> > +	struct ws_bridge *ws;
> > +	struct drm_panel *panel;
> > +	int ret;
> > +	struct backlight_device *backlight;
> 
> Ditto.
> 
> > +
> > +	ws = devm_kzalloc(dev, sizeof(*ws), GFP_KERNEL);
> 
> Recently upstream bridge drivers were changed to use
> devm_drm_bridge_alloc() to allocate the main structure which contains the
> DRM bridge member.
> devm_kzalloc() is no more allowed to be used.
> 
> > +	if (!ws)
> > +		return -ENOMEM;
> > +
> > +	ws->dev = dev;
> > +
> > +	ws->reg_map = devm_regmap_init_i2c(i2c, &ws_regmap_config);
> > +	if (IS_ERR(ws->reg_map))
> > +		return dev_err_probe(dev, PTR_ERR(ws->reg_map), "Failed
> to allocate
> > +regmap\n");
> > +
> > +	ret = drm_of_find_panel_or_bridge(dev->of_node, 1, -1, &panel,
> NULL);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to find remote
> panel\n");
> > +
> > +	ws->next_bridge = devm_drm_panel_bridge_add(dev, panel);
> > +	if (IS_ERR(ws->next_bridge))
> > +		return PTR_ERR(ws->next_bridge);
> > +
> > +	ws->backlight = ws_bridge_create_backlight(ws);
> > +	if (IS_ERR(backlight)) {
> > +		ret = PTR_ERR(backlight);
> > +		dev_err(dev, "Failed to create backlight: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	regmap_write(ws->reg_map, 0xc0, 0x01);
> > +	regmap_write(ws->reg_map, 0xc2, 0x01);
> > +	regmap_write(ws->reg_map, 0xac, 0x01);
> > +
> > +	ws->bridge.funcs = &ws_bridge_bridge_funcs;
> > +	ws->bridge.type = DRM_MODE_CONNECTOR_DPI;
> > +	ws->bridge.of_node = dev->of_node;
> > +	devm_drm_bridge_add(dev, &ws->bridge);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id ws_bridge_of_ids[] = {
> > +	{.compatible = "waveshare,dsi2dpi",},
> > +	{ }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, ws_bridge_of_ids);
> > +
> > +static struct i2c_driver ws_bridge_driver = {
> > +	.driver = {
> > +		.name = "ws_dsi2dpi",
> > +		.of_match_table = ws_bridge_of_ids,
> > +	},
> > +	.probe = ws_bridge_probe,
> > +};
> > +module_i2c_driver(ws_bridge_driver);
> > +
> > +MODULE_AUTHOR("Joseph Guo <qijian.guo@....com>");
> > +MODULE_DESCRIPTION("Waveshare DSI2DPI bridge driver");
> > +MODULE_LICENSE("GPL");
> >
> 
> 
> --
> Regards,
> Liu Ying

Powered by blists - more mailing lists