Message-ID: <pu7zh6a23yjeahxtviernffnlqurwwj3px2pqbesyqugnh2vcu@f5msqjuggrfs>
Date: Mon, 17 Nov 2025 17:46:11 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Sudarshan Shetty <tessolveupstream@...il.com>
Cc: lgirdwood@...il.com, broonie@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] regulator: Add Waveshare panel regulator driver

On Tue, Nov 11, 2025 at 04:13:20PM +0530, Sudarshan Shetty wrote:
> This patch adds a regulator driver for Waveshare panels.
> The regulator provides controlled power sequencing and is also used
> to enable or disable the display backlight.
> 
> Features:
>  - I2C interface to control panel-specific regulator registers
>  - GPIO-based enable/disable support
>  - Integration with the Linux regulator framework
> 
> This driver is required for proper initialization of Waveshare
> MIPI-DSI panels supported by the companion DRM panel driver.
> 

This patch should have been sent together with the binding patch
https://lore.kernel.org/all/20251111104245.3420041-1-tessolveupstream@gmail.com/

> Signed-off-by: Sudarshan Shetty <tessolveupstream@...il.com>
> ---
>  arch/arm64/configs/defconfig                  |   1 +
>  drivers/regulator/Kconfig                     |  11 +
>  drivers/regulator/Makefile                    |   1 +
>  drivers/regulator/waveshare-panel-regulator.c | 294 ++++++++++++++++++
>  4 files changed, 307 insertions(+)
>  create mode 100644 drivers/regulator/waveshare-panel-regulator.c
> 
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index e3a2d37bd104..a1e564024be2 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -1823,3 +1823,4 @@ CONFIG_CORESIGHT_STM=m
>  CONFIG_CORESIGHT_CPU_DEBUG=m
>  CONFIG_CORESIGHT_CTI=m
>  CONFIG_MEMTEST=y
> +CONFIG_REGULATOR_WAVESHARE_TOUCHSCREEN=y

This is not in the right place (it shouldn't be in this patch and it
shouldn't be at this place in the defconfig).

> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index e6ea2d6f46a4..287ec02220f2 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -1184,6 +1184,17 @@ config REGULATOR_RASPBERRYPI_TOUCHSCREEN_V2
>  	  unit. The regulator is used to enable power to the display and to
>  	  control backlight PWM.
>  
> +config REGULATOR_WAVESHARE_TOUCHSCREEN
> +	tristate "Waveshare touchscreen panel regulator"
> +	depends on BACKLIGHT_CLASS_DEVICE
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  Enable support for Waveshare DSI touchscreen panels,
> +	  This driver supports regulator on the waveshare
> +	  touchscreen unit. The regulator is used to enable power to the
> +	  display and to control backlight.
> +
>  config REGULATOR_RC5T583
>  	tristate "RICOH RC5T583 Power regulators"
>  	depends on MFD_RC5T583
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index b5befee45379..4c4011be74cd 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -138,6 +138,7 @@ obj-$(CONFIG_REGULATOR_PBIAS) += pbias-regulator.o
>  obj-$(CONFIG_REGULATOR_PCAP) += pcap-regulator.o
>  obj-$(CONFIG_REGULATOR_RAA215300) += raa215300.o
>  obj-$(CONFIG_REGULATOR_RASPBERRYPI_TOUCHSCREEN_ATTINY)  += rpi-panel-attiny-regulator.o
> +obj-$(CONFIG_REGULATOR_WAVESHARE_TOUCHSCREEN)  += waveshare-panel-regulator.o
>  obj-$(CONFIG_REGULATOR_RASPBERRYPI_TOUCHSCREEN_V2)  += rpi-panel-v2-regulator.o
>  obj-$(CONFIG_REGULATOR_RC5T583)  += rc5t583-regulator.o
>  obj-$(CONFIG_REGULATOR_RK808)   += rk808-regulator.o
> diff --git a/drivers/regulator/waveshare-panel-regulator.c b/drivers/regulator/waveshare-panel-regulator.c
> new file mode 100644
> index 000000000000..eba068e9592a
> --- /dev/null
> +++ b/drivers/regulator/waveshare-panel-regulator.c
> @@ -0,0 +1,294 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2024 Waveshare International Limited
> + *
> + * Based on rpi-panel-v2-regulator.c by Dave Stevenson <dave.stevenson@...pberrypi.com>
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/err.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/of.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
> +
> +#define NUM_GPIO 16 /* Treat BL_ENABLE, LCD_RESET, TP_RESET as GPIOs */

After a lot of digging, I determined that I need to drive GPIO 0, 1, 2
and 4 high to get my Waveshare display to light up, and I need GPIO 9
high to get the touchscreen to work.

What are these pins?!

Pin 0 and 4 are referred to by "regulator sounding names" in the
Waveshare "test" driver. So those sounds like regulators. The way you
toggle pin 2, also sounds like a regulator.

And then we have the 3.3V and 5V supply coming in to this thing...

> +
> +struct waveshare_panel_lcd {
> +	struct mutex dir_lock;
> +	struct mutex pwr_lock;
> +	struct regmap *regmap;
> +	u16 poweron_state;
> +	u16 direction_state;
> +
> +	struct gpio_chip gc;
> +	struct gpio_desc *enable;
> +};
> +
> +static const struct regmap_config waveshare_panel_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = REG_PWM,
> +};
> +
> +static int waveshare_panel_gpio_direction_in(struct gpio_chip *gc,
> +					     unsigned int offset)
> +{
> +	struct waveshare_panel_lcd *state = gpiochip_get_data(gc);
> +
> +	mutex_lock(&state->dir_lock);
> +	state->direction_state |= BIT(offset);

You don't actually do any direction handling, this is just kept in the
driver and returned upon request.

> +	mutex_unlock(&state->dir_lock);
> +
> +	return 0;
> +}
> +
> +static int waveshare_panel_gpio_direction_out(struct gpio_chip *gc,
> +					      unsigned int offset, int val)
> +{
> +	struct waveshare_panel_lcd *state = gpiochip_get_data(gc);
> +	u16 last_val;
> +
> +	mutex_lock(&state->dir_lock);
> +	state->direction_state &= ~BIT(offset);
> +	mutex_unlock(&state->dir_lock);
> +
> +	mutex_lock(&state->pwr_lock);
> +	last_val = state->poweron_state;
> +	if (val)
> +		last_val |= BIT(offset);
> +	else
> +		last_val &= ~BIT(offset);
> +
> +	state->poweron_state = last_val;
> +	mutex_unlock(&state->pwr_lock);
> +
> +	regmap_write(state->regmap, REG_TP, last_val >> 8);
> +	regmap_write(state->regmap, REG_LCD, last_val & 0xff);
> +
> +	return 0;
> +}
> +
> +static int waveshare_panel_gpio_get_direction(struct gpio_chip *gc,
> +					      unsigned int offset)
> +{
> +	struct waveshare_panel_lcd *state = gpiochip_get_data(gc);
> +	u16 last_val;
> +
> +	mutex_lock(&state->dir_lock);
> +	last_val = state->direction_state;
> +	mutex_unlock(&state->dir_lock);
> +
> +	if (last_val & BIT(offset))
> +		return GPIO_LINE_DIRECTION_IN;
> +	else
> +		return GPIO_LINE_DIRECTION_OUT;
> +}
> +
> +static int waveshare_panel_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
> +	struct waveshare_panel_lcd *state = gpiochip_get_data(gc);
> +	u16 pwr_state;
> +
> +	mutex_lock(&state->pwr_lock);
> +	pwr_state = state->poweron_state & BIT(offset);

Is it possible to read this from the hardware?

> +	mutex_unlock(&state->pwr_lock);
> +
> +	return !!pwr_state;
> +}
> +
> +static int waveshare_panel_gpio_set(struct gpio_chip *gc, unsigned int offset,
> +				     int value)

This is exactly the same function as
waveshare_panel_gpio_direction_out()

> +{
> +	struct waveshare_panel_lcd *state = gpiochip_get_data(gc);
> +	u16 last_val;
> +
> +	if (offset >= NUM_GPIO)
> +		return 0;
> +
> +	mutex_lock(&state->pwr_lock);
> +
> +	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);
> +
> +	mutex_unlock(&state->pwr_lock);
> +
> +	return 0;
> +}
> +
> +static int waveshare_panel_update_status(struct backlight_device *bl)
> +{
> +	struct waveshare_panel_lcd *state = bl_get_data(bl);
> +	int brightness = bl->props.brightness;
> +
> +	if (bl->props.power != FB_BLANK_UNBLANK ||

FB_BLANK_UNBLANK was renamed in v6.11, so you haven't compiled and/or
tested this on a modern kernel.

All contributions must be done base on the most recent kernel version,
or newer.

> +	    bl->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK))
> +		brightness = 0;
> +
> +	if (state->enable)
> +		gpiod_set_value_cansleep(state->enable, !!brightness);
> +
> +	return regmap_write(state->regmap, REG_PWM, brightness);
> +}
> +
> +static const struct backlight_ops waveshare_panel_bl = {
> +	.update_status = waveshare_panel_update_status,
> +};
> +
> +static int waveshare_panel_i2c_read(struct i2c_client *client, u8 reg,
> +				    unsigned int *buf)
> +{
> +	int val;
> +
> +	val = i2c_smbus_read_byte_data(client, reg);
> +	if (val < 0)
> +		return val;
> +
> +	*buf = val;
> +
> +	return 0;
> +}
> +
> +static int waveshare_panel_i2c_probe(struct i2c_client *i2c)
> +{
> +	struct backlight_properties props = {};
> +	struct backlight_device *bl;
> +	struct waveshare_panel_lcd *state;
> +	struct regmap *regmap;
> +	unsigned int data;
> +	int ret;
> +
> +	state = devm_kzalloc(&i2c->dev, sizeof(*state), GFP_KERNEL);
> +	if (!state)
> +		return -ENOMEM;
> +
> +	mutex_init(&state->dir_lock);
> +	mutex_init(&state->pwr_lock);
> +
> +	i2c_set_clientdata(i2c, state);
> +
> +	regmap = devm_regmap_init_i2c(i2c, &waveshare_panel_regmap_config);
> +	if (IS_ERR(regmap)) {
> +		ret = PTR_ERR(regmap);
> +		dev_err(&i2c->dev, "Failed to allocate register map: %d\n",
> +			ret);
> +		goto error;
> +	}
> +
> +	ret = waveshare_panel_i2c_read(i2c, REG_ID, &data);

Why do you roll your own i2c_read() instead of just using the regmap?!

> +	if (ret == 0)
> +		dev_info(&i2c->dev, "waveshare panel hw id = 0x%x\n", data);

This is possibly interested to you while you work on bringing up this
driver, after that it's noise. dev_dbg() or perhaps even remove it.

> +
> +	ret = waveshare_panel_i2c_read(i2c, REG_SIZE, &data);
> +	if (ret == 0)
> +		dev_info(&i2c->dev, "waveshare panel size = %d\n", data);
> +
> +	ret = waveshare_panel_i2c_read(i2c, REG_VERSION, &data);
> +	if (ret == 0)
> +		dev_info(&i2c->dev, "waveshare panel mcu version = 0x%x\n",
> +			 data);
> +
> +	state->direction_state = 0;
> +	state->poweron_state = BIT(9) | BIT(8); // Enable VCC

Are you sure? As I wrote above, Waveshare list pin 9 as "reset-gpios" in
the touchscreen node.

But, this means that we have mystery pins 0, 1, 2, 4, 8, and 9...

> +	regmap_write(regmap, REG_TP, state->poweron_state >> 8);
> +	regmap_write(regmap, REG_LCD, state->poweron_state & 0xff);

This is the 3rd time you write these two lines.

> +	msleep(20);
> +
> +	state->regmap = regmap;
> +	state->gc.parent = &i2c->dev;
> +	state->gc.label = i2c->name;
> +	state->gc.owner = THIS_MODULE;
> +	state->gc.base = -1;
> +	state->gc.ngpio = NUM_GPIO;
> +
> +	state->gc.get = waveshare_panel_gpio_get;
> +	state->gc.set = waveshare_panel_gpio_set;
> +	state->gc.direction_input = waveshare_panel_gpio_direction_in;
> +	state->gc.direction_output = waveshare_panel_gpio_direction_out;
> +	state->gc.get_direction = waveshare_panel_gpio_get_direction;
> +	state->gc.can_sleep = true;
> +
> +	ret = devm_gpiochip_add_data(&i2c->dev, &state->gc, state);

You should be able to follow drivers/regulator/rpi-panel-v2-regulator.c
and use devm_gpio_regmap_register() instead of writing your own
functions for all this.

> +	if (ret) {
> +		dev_err(&i2c->dev, "Failed to create gpiochip: %d\n", ret);
> +		goto error;
> +	}
> +
> +	state->enable =
> +		devm_gpiod_get_optional(&i2c->dev, "enable", GPIOD_OUT_LOW);

No, there's no point in having the driver reach for DeviceTree in order
to get to pin 2 in its own GPIO controller.

Perhaps pin 2 is one of the master switches? In which case it shouldn't
be exposed through the gpio-controller, but handled internally, when the
PWM or other regulators are enabled?

> +	if (IS_ERR(state->enable))
> +		return dev_err_probe(&i2c->dev, PTR_ERR(state->enable),
> +				     "Couldn't get enable GPIO\n");
> +
> +	props.type = BACKLIGHT_RAW;
> +	props.max_brightness = 255;
> +	bl = devm_backlight_device_register(&i2c->dev, dev_name(&i2c->dev),
> +					    &i2c->dev, state,
> +					    &waveshare_panel_bl, &props);
> +	if (IS_ERR(bl))
> +		return PTR_ERR(bl);
> +
> +	bl->props.brightness = 255;
> +
> +	return 0;
> +
> +error:
> +	mutex_destroy(&state->dir_lock);
> +	mutex_destroy(&state->pwr_lock);
> +	return ret;
> +}
> +
> +static void waveshare_panel_i2c_remove(struct i2c_client *client)
> +{
> +	struct waveshare_panel_lcd *state = i2c_get_clientdata(client);
> +
> +	mutex_destroy(&state->dir_lock);
> +	mutex_destroy(&state->pwr_lock);
> +}
> +
> +static void waveshare_panel_i2c_shutdown(struct i2c_client *client)
> +{
> +	struct waveshare_panel_lcd *state = i2c_get_clientdata(client);
> +
> +	regmap_write(state->regmap, REG_PWM, 0);

What about the regulators?

Regards,
Bjorn

> +}
> +
> +static const struct of_device_id waveshare_panel_dt_ids[] = {
> +	{ .compatible = "waveshare,touchscreen-panel-regulator" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, waveshare_panel_dt_ids);
> +
> +static struct i2c_driver waveshare_panel_regulator_driver = {
> +	.driver = {
> +		.name = "waveshare_touchscreen",
> +		.of_match_table = of_match_ptr(waveshare_panel_dt_ids),
> +	},
> +	.probe = waveshare_panel_i2c_probe,
> +	.remove	= waveshare_panel_i2c_remove,
> +	.shutdown = waveshare_panel_i2c_shutdown,
> +};
> +
> +module_i2c_driver(waveshare_panel_regulator_driver);
> +
> +MODULE_AUTHOR("Waveshare Team <support@...eshare.com>");
> +MODULE_DESCRIPTION("Regulator device driver for Waveshare touchscreen");
> +MODULE_LICENSE("GPL");
> -- 
> 2.34.1
> 

Powered by blists - more mailing lists