Re: [PATCH v1] regulator: Add Waveshare panel regulator driver

[Mark Brown]

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.

> +++ 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

Please keep this and the Kconfig sorted.

> @@ -0,0 +1,294 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2024 Waveshare International Limited
> + *

Please make the entire comment block a C++ one so things look more
intentional.

> +static const struct regmap_config waveshare_panel_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = REG_PWM,
> +};

Perhaps worth enabling caching to cut down on reads and hence improve
performance?

> +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);
> +	mutex_unlock(&state->pwr_lock);
> +
> +	return !!pwr_state;
> +}

This will only read the value that has been set but there's support in
the driver for configuring the GPIO as an input, how would that work?

> +static int waveshare_panel_gpio_set(struct gpio_chip *gc, unsigned int offset,
> +				     int value)
> +{
> +	struct waveshare_panel_lcd *state = gpiochip_get_data(gc);
> +	u16 last_val;
> +
> +	if (offset >= NUM_GPIO)
> +		return 0;

Shouldn't this be an error?

> +	ret = waveshare_panel_i2c_read(i2c, REG_ID, &data);
> +	if (ret == 0)
> +		dev_info(&i2c->dev, "waveshare panel hw id = 0x%x\n", data);
> +
> +	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);

Shouldn't we take I/O errors on these registers more seriously?

> +	state->direction_state = 0;
> +	state->poweron_state = BIT(9) | BIT(8); // Enable VCC
> +	regmap_write(regmap, REG_TP, state->poweron_state >> 8);
> +	regmap_write(regmap, REG_LCD, state->poweron_state & 0xff);
> +	msleep(20);

Generally the default for regulators is to leave the hardware alone
unless explicitly configured to control it.

> +static const struct of_device_id waveshare_panel_dt_ids[] = {
> +	{ .compatible = "waveshare,touchscreen-panel-regulator" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, waveshare_panel_dt_ids);

This DT binding needs to be documented.

> +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,
> +};

This doesn't actually seem to register a regulator, why put it in the
regulator subsystem?

Attachment: signature.asc
Description: PGP signature