From: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
To: Bartosz Golaszewski <brgl@kernel.org>
Cc: dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org,
Neil Armstrong <neil.armstrong@linaro.org>,
Jessica Zhang <jesszhan0024@gmail.com>,
David Airlie <airlied@gmail.com>, Simona Vetter <simona@ffwll.ch>,
Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
Maxime Ripard <mripard@kernel.org>,
Thomas Zimmermann <tzimmermann@suse.de>,
Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzk+dt@kernel.org>,
Conor Dooley <conor+dt@kernel.org>,
Cong Yang <yangcong5@huaqin.corp-partner.google.com>,
Ondrej Jirman <megi@xff.cz>,
Javier Martinez Canillas <javierm@redhat.com>,
Jagan Teki <jagan@edgeble.ai>,
Liam Girdwood <lgirdwood@gmail.com>,
Mark Brown <broonie@kernel.org>,
Linus Walleij <linusw@kernel.org>
Subject: Re: [PATCH 19/19] gpio: add GPIO controller found on Waveshare DSI TOUCH panels
Date: Thu, 9 Apr 2026 04:26:38 +0300 [thread overview]
Message-ID: <l6pezliurpgv2mopw2xl4gfgpki6r3v6ufpsaavj774qnxgept@h3jxxpco23oa> (raw)
In-Reply-To: <CAMRc=Mcusnm-k76e6jTiwrw5xJL7f-nWBsg4=QpD08cv8pPgMw@mail.gmail.com>
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@oss.qualcomm.com> 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@oss.qualcomm.com>
> > ---
> > 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
next prev parent reply other threads:[~2026-04-09 1:26 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-04-01 7:26 [PATCH 00/19] drm/panel: support Waveshare DSI TOUCH kits Dmitry Baryshkov
2026-04-01 7:26 ` [PATCH 01/19] dt-bindings: display/panel: himax,hx83102: describe Waveshare panel Dmitry Baryshkov
2026-04-02 8:30 ` Krzysztof Kozlowski
2026-04-07 9:38 ` Linus Walleij
2026-04-07 9:49 ` Krzysztof Kozlowski
2026-04-07 9:50 ` Krzysztof Kozlowski
2026-04-01 7:26 ` [PATCH 02/19] dt-bindings: display/panel: himax,hx8394: " Dmitry Baryshkov
2026-04-02 8:30 ` Krzysztof Kozlowski
2026-04-07 9:50 ` Krzysztof Kozlowski
2026-04-01 7:26 ` [PATCH 03/19] dt-bindings: display/panel: jadard,jd9365da-h3: " Dmitry Baryshkov
2026-04-07 10:46 ` Krzysztof Kozlowski
2026-04-01 7:26 ` [PATCH 04/19] dt-bindings: display/panel: ilitek,ili9881c: " Dmitry Baryshkov
2026-04-02 8:33 ` Krzysztof Kozlowski
2026-04-07 10:46 ` Krzysztof Kozlowski
2026-04-01 7:26 ` [PATCH 05/19] dt-bindings: dipslay/panel: describe panels using Focaltech OTA7290B Dmitry Baryshkov
2026-04-01 8:52 ` Rob Herring (Arm)
2026-04-01 7:26 ` [PATCH 06/19] drm/of: add helper to count data-lanes on a remote endpoint Dmitry Baryshkov
2026-04-09 9:15 ` Javier Martinez Canillas
2026-04-01 7:26 ` [PATCH 07/19] drm/panel: himax-hx83102: support Waveshare 12.3" DSI panel Dmitry Baryshkov
2026-04-07 20:12 ` Linus Walleij
2026-04-01 7:26 ` [PATCH 08/19] drm/panel: himax-hx8394: set prepare_prev_first Dmitry Baryshkov
2026-04-08 8:08 ` Linus Walleij
2026-04-09 9:16 ` Javier Martinez Canillas
2026-04-01 7:26 ` [PATCH 09/19] drm/panel: himax-hx8394: simplify hx8394_enable() Dmitry Baryshkov
2026-04-08 8:08 ` Linus Walleij
2026-04-09 9:20 ` Javier Martinez Canillas
2026-04-01 7:26 ` [PATCH 10/19] drm/panel: himax-hx8394: support Waveshare DSI panels Dmitry Baryshkov
2026-04-08 8:10 ` Linus Walleij
2026-04-09 0:33 ` Dmitry Baryshkov
2026-04-09 9:27 ` Javier Martinez Canillas
2026-04-01 7:26 ` [PATCH 11/19] drm/panel: jadard-jd9365da-h3: use drm_connector_helper_get_modes_fixed Dmitry Baryshkov
2026-04-08 8:10 ` Linus Walleij
2026-04-01 7:26 ` [PATCH 12/19] drm/panel: jadard-jd9365da-h3: support variable DSI configuration Dmitry Baryshkov
2026-04-07 12:51 ` Riccardo Mereu
2026-04-08 8:15 ` Linus Walleij
2026-04-09 0:35 ` Dmitry Baryshkov
2026-04-01 7:26 ` [PATCH 13/19] drm/panel: jadard-jd9365da-h3: set prepare_prev_first Dmitry Baryshkov
2026-04-07 12:51 ` Riccardo Mereu
2026-04-08 8:15 ` Linus Walleij
2026-04-01 7:26 ` [PATCH 14/19] drm/panel: jadard-jd9365da-h3: support Waveshare DSI panels Dmitry Baryshkov
2026-04-07 12:53 ` Riccardo Mereu
2026-04-08 8:24 ` Linus Walleij
2026-04-09 0:49 ` Dmitry Baryshkov
2026-04-09 8:50 ` Linus Walleij
2026-04-12 17:56 ` Dmitry Baryshkov
2026-04-01 7:26 ` [PATCH 15/19] drm/panel: ilitek-ili9881c: support Waveshare 7.0" DSI panel Dmitry Baryshkov
2026-04-08 8:26 ` Linus Walleij
2026-04-01 7:26 ` [PATCH 16/19] drm/panel: add devm_drm_panel_add() helper Dmitry Baryshkov
2026-04-08 8:25 ` Linus Walleij
2026-04-01 7:26 ` [PATCH 17/19] drm/panel: add driver for Waveshare 8.8" DSI TOUCH-A panel Dmitry Baryshkov
2026-04-08 8:27 ` Linus Walleij
2026-04-01 7:26 ` [PATCH 18/19] dt-bindings: gpio: describe Waveshare GPIO controller Dmitry Baryshkov
2026-04-01 8:52 ` Rob Herring (Arm)
2026-04-07 12:53 ` Riccardo Mereu
2026-04-01 7:26 ` [PATCH 19/19] gpio: add GPIO controller found on Waveshare DSI TOUCH panels Dmitry Baryshkov
2026-04-03 12:30 ` Bartosz Golaszewski
2026-04-09 1:26 ` Dmitry Baryshkov [this message]
2026-04-09 7:41 ` Bartosz Golaszewski
2026-04-07 12:53 ` Riccardo Mereu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=l6pezliurpgv2mopw2xl4gfgpki6r3v6ufpsaavj774qnxgept@h3jxxpco23oa \
--to=dmitry.baryshkov@oss.qualcomm.com \
--cc=airlied@gmail.com \
--cc=brgl@kernel.org \
--cc=broonie@kernel.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=jagan@edgeble.ai \
--cc=javierm@redhat.com \
--cc=jesszhan0024@gmail.com \
--cc=krzk+dt@kernel.org \
--cc=lgirdwood@gmail.com \
--cc=linusw@kernel.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maarten.lankhorst@linux.intel.com \
--cc=megi@xff.cz \
--cc=mripard@kernel.org \
--cc=neil.armstrong@linaro.org \
--cc=robh@kernel.org \
--cc=simona@ffwll.ch \
--cc=tzimmermann@suse.de \
--cc=yangcong5@huaqin.corp-partner.google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).