Re: [PATCH 19/19] gpio: add GPIO controller found on Waveshare DSI TOUCH panels

[Dmitry Baryshkov]

On Fri, Apr 03, 2026 at 08:30:22AM -0400, Bartosz Golaszewski wrote:
> On Wed, 1 Apr 2026 09:26:38 +0200, Dmitry Baryshkov
> <dmitry.baryshkov@xxxxxxxxxxxxxxxx> said:
> > The Waveshare DSI TOUCH family of panels has separate on-board GPIO
> > controller, which controls power supplies to the panel and the touch
> > screen and provides reset pins for both the panel and the touchscreen.
> > Also it provides a simple PWM controller for panel backlight. Add
> > support for this GPIO controller.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxxxxxxxx>
> > ---
> >  drivers/gpio/Kconfig              |  10 ++
> >  drivers/gpio/Makefile             |   1 +
> >  drivers/gpio/gpio-waveshare-dsi.c | 220 ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 231 insertions(+)
> >
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 4c3f6ec336c1..f0bb5cdebf9b 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -804,6 +804,16 @@ config GPIO_VISCONTI
> >  	help
> >  	  Say yes here to support GPIO on Tohisba Visconti.
> >
> > +config GPIO_WAVESHARE_DSI_TOUCH
> > +	tristate "Waveshare GPIO controller for DSI panels"
> > +	depends on BACKLIGHT_CLASS_DEVICE
> > +	depends on I2C
> > +	select REGMAP_I2C
> > +	help
> > +	  Enable support for the GPIO and PWM controller found on Waveshare DSI
> > +	  TOUCH panel kits. It provides GPIOs (used for regulator control and
> > +          resets) and backlight support.
> > +
> >  config GPIO_WCD934X
> >  	tristate "Qualcomm Technologies Inc WCD9340/WCD9341 GPIO controller driver"
> >  	depends on MFD_WCD934X
> > diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> > index 20d4a57afdaa..75ce89fc3b93 100644
> > --- a/drivers/gpio/Makefile
> > +++ b/drivers/gpio/Makefile
> > @@ -207,6 +207,7 @@ obj-$(CONFIG_GPIO_VIRTUSER)		+= gpio-virtuser.o
> >  obj-$(CONFIG_GPIO_VIRTIO)		+= gpio-virtio.o
> >  obj-$(CONFIG_GPIO_VISCONTI)		+= gpio-visconti.o
> >  obj-$(CONFIG_GPIO_VX855)		+= gpio-vx855.o
> > +obj-$(CONFIG_GPIO_WAVESHARE_DSI_TOUCH)	+= gpio-waveshare-dsi.o
> >  obj-$(CONFIG_GPIO_WCD934X)		+= gpio-wcd934x.o
> >  obj-$(CONFIG_GPIO_WHISKEY_COVE)		+= gpio-wcove.o
> >  obj-$(CONFIG_GPIO_WINBOND)		+= gpio-winbond.o
> > diff --git a/drivers/gpio/gpio-waveshare-dsi.c b/drivers/gpio/gpio-waveshare-dsi.c
> > new file mode 100644
> > index 000000000000..30fe7569c150
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-waveshare-dsi.c
> > @@ -0,0 +1,220 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2024 Waveshare International Limited
> > + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> > + */
> > +
> > +#include <linux/backlight.h>
> > +#include <linux/err.h>
> > +#include <linux/fb.h>
> > +#include <linux/gpio/driver.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/regmap.h>
> > +
> > +/* I2C registers of the microcontroller. */
> > +#define REG_TP		0x94
> > +#define REG_LCD		0x95
> > +#define REG_PWM		0x96
> > +#define REG_SIZE	0x97
> > +#define REG_ID		0x98
> > +#define REG_VERSION	0x99
> > +
> > +enum {
> > +	GPIO_AVDD = 0,
> > +	GPIO_PANEL_RESET = 1,
> > +	GPIO_BL_ENABLE = 2,
> > +	GPIO_IOVCC = 4,
> > +	GPIO_VCC = 8,
> > +	GPIO_TS_RESET = 9,
> > +	NUM_GPIO = 16,
> 
> Why is this part of an enum?

I'll move this out of the enum.

> 
> > +static int waveshare_gpio_set(struct waveshare_gpio *state, unsigned int offset, int value)
> > +{
> > +	u16 last_val;
> > +
> > +	mutex_lock(&state->pwr_lock);
> 
> Can you use guards for locks?

Yes

> 
> > +
> > +	last_val = state->poweron_state;
> > +	if (value)
> > +		last_val |= BIT(offset);
> > +	else
> > +		last_val &= ~BIT(offset);
> > +
> > +	state->poweron_state = last_val;
> > +
> > +	regmap_write(state->regmap, REG_TP, last_val >> 8);
> > +	regmap_write(state->regmap, REG_LCD, last_val & 0xff);
> 
> I2C regmap writes can fail and their return value should be checked.

Ack.

> 
> > +
> > +	mutex_unlock(&state->pwr_lock);
> > +
> > +	return 0;
> > +}
> > +

[...]

> > +
> > +static int waveshare_gpio_update_status(struct backlight_device *bl)
> > +{
> > +	struct waveshare_gpio *state = bl_get_data(bl);
> > +	int brightness = backlight_get_brightness(bl);
> > +
> > +	waveshare_gpio_set(state, GPIO_BL_ENABLE, brightness);
> > +
> > +	return regmap_write(state->regmap, REG_PWM, brightness);
> > +}
> > +

[...]

> > +static int waveshare_gpio_probe(struct i2c_client *i2c)
> > +{

[...]
> > +
> > +	dev_dbg(dev, "waveshare panel mcu version = 0x%x\n", data);
> > +
> > +	state->poweron_state = BIT(GPIO_TS_RESET);
> > +	regmap_write(regmap, REG_TP, state->poweron_state >> 8);
> > +	regmap_write(regmap, REG_LCD, state->poweron_state & 0xff);

And this can become waveshare_gpio_set().

> > +	msleep(20);
> > +
> > +	state->regmap = regmap;
> > +	state->gc.parent = dev;
> > +	state->gc.label = i2c->name;
> > +	state->gc.owner = THIS_MODULE;
> > +	state->gc.base = -1;
> > +	state->gc.ngpio = NUM_GPIO;
> > +
> > +	/* it is output only */
> > +	state->gc.get = waveshare_gpio_gpio_get;
> > +	state->gc.set = waveshare_gpio_gpio_set;
> > +	state->gc.get_direction = waveshare_gpio_gpio_get_direction;
> > +	state->gc.can_sleep = true;
> > +
> > +	ret = devm_gpiochip_add_data(dev, &state->gc, state);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to create gpiochip\n");
> > +
> 
> This driver looks like it could be easily converted to use gpio-regmap and
> become much shorter in the process. Could you please take a look at
> linux/gpio/regmap.h?

I took a glance. It is a nice wrapper, but I think being able to call
waveshare_gpio_set() internally without extra troubles overweights the
bonuses of the wrapper. Also, I'd agree if there were extra complexity
here (e.g. the stride or the in/out handling), but having just the out
GPIOs doesn't seem to warrant it.

An alternative would be to split away the backlight into a separate
pwm-backlight device. Then having waveshare_gpio_set() isn't that
important and thus I could switch to GPIO_REGMAP. But then... We don't
have real control over the PWM. We are really programming some values,
with the actual PWM duty cycle calculations being handled internally.

With all that in mind, unless you really insist, I'd prefer to leave
this part the driver as is.

> 
> > +	props.type = BACKLIGHT_RAW;
> > +	props.max_brightness = 255;
> > +	props.brightness = 255;
> > +	bl = devm_backlight_device_register(dev, dev_name(dev), dev, state,
> > +					    &waveshare_gpio_bl, &props);
> > +	return PTR_ERR_OR_ZERO(bl);
> > +}
> > +

-- 
With best wishes
Dmitry