[PATCH 0/4] auxdisplay: Add support for TM1637 7-segment display driver

[PATCH 0/4] auxdisplay: Add support for TM1637 7-segment display driver

[Siratul Islam]

This series adds a driver for the Titan Micro TM1637 7-segment display
controller.

The TM1637 uses a custom two-wire protocol (CLK/DIO) that resembles I2C
but lacks a slave address, requiring a bit-banging implementation.

The driver exposes standard sysfs attributes for 'message' (text) and
'brightness'. It handles standard ASCII-to-7-segment mapping using the
kernel's map_to_7segment utility.

Note on implementation:
This driver is implemented as a standalone platform driver rather than
using the auxiliary display 'linedisp' framework. The TM1637 protocol
requires manual handling of the decimal point (merging it with the
previous digit), which is difficult to achieve cleanly with the current
linedisp character buffer abstraction.

Siratul Islam (4):
  dt-bindings: vendor-prefixes: Add titanmec
  dt-bindings: auxdisplay: Add titanmec,tm1637
  auxdisplay: tm1637: Add driver for TM1637
  MAINTAINERS: Add entry for TM1637

 .../ABI/testing/sysfs-platform-tm1637         |  20 ++
 .../bindings/auxdisplay/titanmec,tm1637.yaml  |  43 +++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   7 +
 drivers/auxdisplay/Kconfig                    |  11 +
 drivers/auxdisplay/Makefile                   |   1 +
 drivers/auxdisplay/tm1637.c                   | 297 ++++++++++++++++++
 7 files changed, 381 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-tm1637
 create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm1637.yaml
 create mode 100644 drivers/auxdisplay/tm1637.c

--
2.47.3

[PATCH 1/4] dt-bindings: vendor-prefixes: Add titanmec

[Siratul Islam]

Add vendor prefix for Shenzhen Titan Micro Electronics Co., Ltd.

Signed-off-by: Siratul Islam <email@sirat.me>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index c7591b2aec2a..2fd273ccd66a 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1654,6 +1654,8 @@ patternProperties:
     description: Texas Instruments
   "^tianma,.*":
     description: Tianma Micro-electronics Co., Ltd.
+  "^titanmec,.*":
+    description: Shenzhen Titan Micro Electronics Co., Ltd.
   "^tlm,.*":
     description: Trusted Logic Mobility
   "^tmt,.*":
-- 
2.47.3

[PATCH 2/4] dt-bindings: auxdisplay: Add titanmec,tm1637

[Siratul Islam]

Add device tree binding for the Titanmec TM1637 7-segment display
controller.

The TM1637 is a LED drive controller with a two-wire serial interface
(CLK, DIO).

Signed-off-by: Siratul Islam <email@sirat.me>
---
 .../bindings/auxdisplay/titanmec,tm1637.yaml  | 43 +++++++++++++++++++
 1 file changed, 43 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/auxdisplay/titanmec,tm1637.yaml

diff --git a/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1637.yaml b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1637.yaml
new file mode 100644
index 000000000000..c39000b88676
--- /dev/null
+++ b/Documentation/devicetree/bindings/auxdisplay/titanmec,tm1637.yaml
@@ -0,0 +1,43 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/auxdisplay/titanmec,tm1637.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: TM1637 7-segment LED display controller
+
+maintainers:
+  - Siratul Islam <email@sirat.me>
+
+description:
+  The TM1637 is a 7-segment LED display controller with a two-wire serial
+  interface. It can drive up to 4 digits and includes brightness control.
+
+properties:
+  compatible:
+    const: titanmec,tm1637
+
+  clk-gpios:
+    maxItems: 1
+    description: GPIO for clock line
+
+  dio-gpios:
+    maxItems: 1
+    description: GPIO for data input/output line
+
+required:
+  - compatible
+  - clk-gpios
+  - dio-gpios
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    display {
+        compatible = "titanmec,tm1637";
+        clk-gpios = <&gpio 23 GPIO_ACTIVE_HIGH>;
+        dio-gpios = <&gpio 24 GPIO_ACTIVE_HIGH>;
+    };
-- 
2.47.3

[PATCH 3/4] auxdisplay: tm1637: Add driver for TM1637

[Siratul Islam]

Add a driver for the Titanmec TM1637 7-segment display controller.

The TM1637 uses a custom two-wire protocol (similar to I2C but without
a slave address) which requires bit-banging via GPIOs. This driver
implements the protocol manually as it does not fit the standard I2C
subsystem.

The driver exposes a character device interface via sysfs:
- 'message': Write a string to display (supports alphanumeric & dots).
- 'brightness': Control intensity (0-8).

Standard 7-segment mapping is handled using the kernel's map_to_7segment
utility.

Signed-off-by: Siratul Islam <email@sirat.me>
---
 .../ABI/testing/sysfs-platform-tm1637         |  20 ++
 drivers/auxdisplay/Kconfig                    |  11 +
 drivers/auxdisplay/Makefile                   |   1 +
 drivers/auxdisplay/tm1637.c                   | 297 ++++++++++++++++++
 4 files changed, 329 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-tm1637
 create mode 100644 drivers/auxdisplay/tm1637.c

diff --git a/Documentation/ABI/testing/sysfs-platform-tm1637 b/Documentation/ABI/testing/sysfs-platform-tm1637
new file mode 100644
index 000000000000..4941fd15518d
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-tm1637
@@ -0,0 +1,20 @@
+What:		/sys/bus/platform/devices/.../message
+Date:		January 2026
+KernelVersion:	6.19
+Contact:	Siratul Islam <email@sirat.me>
+Description:
+		Write a text string to display on the 7-segment display.
+		Supports alphanumeric characters and decimal points.
+		A decimal point can be added after any character by
+		following it with a dot (e.g., "12.34").
+
+		Reading returns the current segment data in hex format.
+
+What:		/sys/bus/platform/devices/.../brightness
+Date:		January 2026
+KernelVersion:	6.19
+Contact:	Siratul Islam <email@sirat.me>
+Description:
+		Control display brightness. Valid range is 0-8.
+		Writing 0 turns off the display.
+		Writing 1-8 sets brightness levels, with 8 being maximum.
diff --git a/drivers/auxdisplay/Kconfig b/drivers/auxdisplay/Kconfig
index bedc6133f970..1450591a0a25 100644
--- a/drivers/auxdisplay/Kconfig
+++ b/drivers/auxdisplay/Kconfig
@@ -526,6 +526,17 @@ config SEG_LED_GPIO
 	  This driver can also be built as a module. If so, the module
 	  will be called seg-led-gpio.
 
+config TM1637
+	tristate "Shenzhen Titan Micro Electronics TM1637 7-Segment Display"
+	depends on GPIOLIB || COMPILE_TEST
+	select AUXDISPLAY
+	help
+	  This driver provides support for the Titanmec TM1637 7-segment
+	  display controller connected via GPIO bit-banging.
+
+	  This driver exposes a character interface for controlling the
+	  display content and brightness via sysfs.
+
 #
 # Character LCD with non-conforming interface section
 #
diff --git a/drivers/auxdisplay/Makefile b/drivers/auxdisplay/Makefile
index f5c13ed1cd4f..5baa77da4343 100644
--- a/drivers/auxdisplay/Makefile
+++ b/drivers/auxdisplay/Makefile
@@ -16,3 +16,4 @@ obj-$(CONFIG_LINEDISP)		+= line-display.o
 obj-$(CONFIG_MAX6959)		+= max6959.o
 obj-$(CONFIG_PARPORT_PANEL)	+= panel.o
 obj-$(CONFIG_SEG_LED_GPIO)	+= seg-led-gpio.o
+obj-$(CONFIG_TM1637)		+= tm1637.o
diff --git a/drivers/auxdisplay/tm1637.c b/drivers/auxdisplay/tm1637.c
new file mode 100644
index 000000000000..f8623fea1d2a
--- /dev/null
+++ b/drivers/auxdisplay/tm1637.c
@@ -0,0 +1,297 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+/*
+ * TM1637 7-segment display driver
+ *
+ * Copyright (C) 2026 Siratul Islam <email@sirat.me>
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+#include <linux/map_to_7segment.h>
+
+/* Commands */
+#define TM1637_CMD_DATA_AUTO_INC 0x40
+#define TM1637_CMD_ADDR_BASE 0xC0
+#define TM1637_CMD_DISPLAY_CTRL 0x80
+
+/* Display control bits */
+#define TM1637_DISPLAY_ON BIT(3)
+#define TM1637_BRIGHTNESS_MASK GENMASK(2, 0)
+#define TM1637_BRIGHTNESS_MAX 7
+
+#define TM1637_SEG_DP BIT(7)
+
+/* Protocol timing */
+#define TM1637_BIT_DELAY_MIN 100
+#define TM1637_BIT_DELAY_MAX 120
+
+#define TM1637_DIGITS 4
+
+struct tm1637 {
+	struct device *dev;
+	struct gpio_desc *clk;
+	struct gpio_desc *dio;
+	struct mutex lock; /* Protects display buffer and brightness */
+	u8 brightness;
+	u8 buf[TM1637_DIGITS];
+};
+
+/* Defines a static const 'initial_map' variable */
+static const SEG7_DEFAULT_MAP(initial_map);
+
+static void tm1637_delay(void)
+{
+	usleep_range(TM1637_BIT_DELAY_MIN, TM1637_BIT_DELAY_MAX);
+}
+
+static void tm1637_start(struct tm1637 *tm)
+{
+	gpiod_direction_output(tm->dio, 1);
+	gpiod_set_value(tm->clk, 1);
+	tm1637_delay();
+	gpiod_set_value(tm->dio, 0);
+	tm1637_delay();
+	gpiod_set_value(tm->clk, 0);
+	tm1637_delay();
+}
+
+static void tm1637_stop(struct tm1637 *tm)
+{
+	gpiod_direction_output(tm->dio, 0);
+	gpiod_set_value(tm->clk, 1);
+	tm1637_delay();
+	gpiod_set_value(tm->dio, 1);
+	tm1637_delay();
+}
+
+static bool tm1637_write_byte(struct tm1637 *tm, u8 data)
+{
+	bool ack;
+	int i;
+
+	for (i = 0; i < 8; i++) {
+		gpiod_set_value(tm->clk, 0);
+		tm1637_delay();
+
+		if (data & BIT(i))
+			gpiod_direction_input(tm->dio);
+		else
+			gpiod_direction_output(tm->dio, 0);
+
+		tm1637_delay();
+		gpiod_set_value(tm->clk, 1);
+		tm1637_delay();
+	}
+
+	gpiod_set_value(tm->clk, 0);
+	gpiod_direction_input(tm->dio);
+	tm1637_delay();
+
+	gpiod_set_value(tm->clk, 1);
+	tm1637_delay();
+
+	ack = !gpiod_get_value(tm->dio);
+
+	if (!ack)
+		gpiod_direction_output(tm->dio, 0);
+
+	tm1637_delay();
+	gpiod_set_value(tm->clk, 0);
+
+	return ack;
+}
+
+static void tm1637_update_display_locked(struct tm1637 *tm)
+{
+	u8 ctrl_cmd;
+	int i;
+
+	tm1637_start(tm);
+	tm1637_write_byte(tm, TM1637_CMD_DATA_AUTO_INC);
+	tm1637_stop(tm);
+
+	tm1637_start(tm);
+	tm1637_write_byte(tm, TM1637_CMD_ADDR_BASE);
+	for (i = 0; i < TM1637_DIGITS; i++)
+		tm1637_write_byte(tm, tm->buf[i]);
+	tm1637_stop(tm);
+
+	if (tm->brightness == 0)
+		ctrl_cmd = 0;
+	else
+		ctrl_cmd = TM1637_DISPLAY_ON | ((tm->brightness - 1) & TM1637_BRIGHTNESS_MASK);
+
+	tm1637_start(tm);
+	tm1637_write_byte(tm, ctrl_cmd);
+	tm1637_stop(tm);
+}
+
+static void tm1637_update_display(struct tm1637 *tm)
+{
+	mutex_lock(&tm->lock);
+	tm1637_update_display_locked(tm);
+	mutex_unlock(&tm->lock);
+}
+
+static ssize_t message_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct tm1637 *tm = dev_get_drvdata(dev);
+	int i, pos = 0;
+
+	mutex_lock(&tm->lock);
+	for (i = 0; i < TM1637_DIGITS; i++) {
+		pos += sysfs_emit_at(buf, pos, "0x%02x", tm->buf[i]);
+		if (i < TM1637_DIGITS - 1)
+			pos += sysfs_emit_at(buf, pos, " ");
+	}
+	pos += sysfs_emit_at(buf, pos, "\n");
+	mutex_unlock(&tm->lock);
+
+	return pos;
+}
+
+static ssize_t message_store(struct device *dev, struct device_attribute *attr,
+			     const char *buf, size_t count)
+{
+	struct tm1637 *tm = dev_get_drvdata(dev);
+	size_t i, pos = 0;
+	size_t len;
+	u8 segment_data[TM1637_DIGITS] = {0};
+
+	len = count;
+	if (len > 0 && buf[len - 1] == '\n')
+		len--;
+
+	for (i = 0; i < len && pos < TM1637_DIGITS; i++) {
+		char c = buf[i];
+
+		if (c == '.')
+			continue;
+
+		segment_data[pos] = map_to_seg7(&initial_map, c);
+
+		if (i + 1 < len && buf[i + 1] == '.')
+			segment_data[pos] |= TM1637_SEG_DP;
+
+		pos++;
+	}
+
+	mutex_lock(&tm->lock);
+	memcpy(tm->buf, segment_data, sizeof(tm->buf));
+	tm1637_update_display_locked(tm);
+	mutex_unlock(&tm->lock);
+
+	return count;
+}
+static DEVICE_ATTR_RW(message);
+
+static ssize_t brightness_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct tm1637 *tm = dev_get_drvdata(dev);
+	unsigned int brightness;
+
+	mutex_lock(&tm->lock);
+	brightness = tm->brightness;
+	mutex_unlock(&tm->lock);
+
+	return sysfs_emit(buf, "%u\n", brightness);
+}
+
+static ssize_t brightness_store(struct device *dev, struct device_attribute *attr,
+				const char *buf, size_t count)
+{
+	struct tm1637 *tm = dev_get_drvdata(dev);
+	unsigned int brightness;
+	int ret;
+
+	ret = kstrtouint(buf, 10, &brightness);
+	if (ret)
+		return ret;
+
+	if (brightness > TM1637_BRIGHTNESS_MAX + 1)
+		brightness = TM1637_BRIGHTNESS_MAX + 1;
+
+	mutex_lock(&tm->lock);
+	if (tm->brightness != brightness) {
+		tm->brightness = brightness;
+		tm1637_update_display_locked(tm);
+	}
+	mutex_unlock(&tm->lock);
+
+	return count;
+}
+static DEVICE_ATTR_RW(brightness);
+
+static struct attribute *tm1637_attrs[] = {
+	&dev_attr_message.attr,
+	&dev_attr_brightness.attr,
+	NULL};
+ATTRIBUTE_GROUPS(tm1637);
+
+static int tm1637_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct tm1637 *tm;
+
+	tm = devm_kzalloc(dev, sizeof(*tm), GFP_KERNEL);
+	if (!tm)
+		return -ENOMEM;
+
+	tm->dev = dev;
+
+	tm->clk = devm_gpiod_get(dev, "clk", GPIOD_OUT_LOW);
+	if (IS_ERR(tm->clk))
+		return dev_err_probe(dev, PTR_ERR(tm->clk), "Failed to get clk GPIO\n");
+
+	tm->dio = devm_gpiod_get(dev, "dio", GPIOD_OUT_LOW);
+	if (IS_ERR(tm->dio))
+		return dev_err_probe(dev, PTR_ERR(tm->dio), "Failed to get dio GPIO\n");
+
+	mutex_init(&tm->lock);
+
+	tm->brightness = TM1637_BRIGHTNESS_MAX + 1;
+
+	platform_set_drvdata(pdev, tm);
+	tm1637_update_display(tm);
+
+	return 0;
+}
+
+static void tm1637_remove(struct platform_device *pdev)
+{
+	struct tm1637 *tm = platform_get_drvdata(pdev);
+
+	mutex_lock(&tm->lock);
+	tm->brightness = 0;
+	memset(tm->buf, 0, sizeof(tm->buf));
+	tm1637_update_display_locked(tm);
+	mutex_unlock(&tm->lock);
+
+	mutex_destroy(&tm->lock);
+}
+
+static const struct of_device_id tm1637_of_match[] = {
+	{.compatible = "titanmec,tm1637"},
+	{}};
+MODULE_DEVICE_TABLE(of, tm1637_of_match);
+
+static struct platform_driver tm1637_driver = {
+	.probe = tm1637_probe,
+	.remove = tm1637_remove,
+	.driver = {
+		.name = "tm1637",
+		.of_match_table = tm1637_of_match,
+		.dev_groups = tm1637_groups,
+	},
+};
+module_platform_driver(tm1637_driver);
+
+MODULE_DESCRIPTION("TM1637 7-segment display driver");
+MODULE_AUTHOR("Siratul Islam <email@sirat.me>");
+MODULE_LICENSE("Dual BSD/GPL");
-- 
2.47.3

[PATCH 4/4] MAINTAINERS: Add entry for TM1637

[Siratul Islam]

Add entry for the Titanmec TM1637 7-segment display driver.

Signed-off-by: Siratul Islam <email@sirat.me>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 32b5e41d9849..4f07c7b5d649 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -26312,6 +26312,13 @@ W:	http://sourceforge.net/projects/tlan/
 F:	Documentation/networking/device_drivers/ethernet/ti/tlan.rst
 F:	drivers/net/ethernet/ti/tlan.*
 
+TM1637 7-SEGMENT DISPLAY DRIVER
+M:	Siratul Islam <email@sirat.me>
+S:	Maintained
+F:	Documentation/ABI/testing/sysfs-platform-tm1637
+F:	Documentation/devicetree/bindings/auxdisplay/titanmec,tm1637.yaml
+F:	drivers/auxdisplay/tm1637.c
+
 TMIO/SDHI MMC DRIVER
 M:	Wolfram Sang <wsa+renesas@sang-engineering.com>
 L:	linux-mmc@vger.kernel.org
-- 
2.47.3

Re: [PATCH 0/4] auxdisplay: Add support for TM1637 7-segment display driver

[Andy Shevchenko]

On Tue, Jan 13, 2026 at 6:03 AM Siratul Islam <email@sirat.me> wrote:
>
> This series adds a driver for the Titan Micro TM1637 7-segment display
> controller.
>
> The TM1637 uses a custom two-wire protocol (CLK/DIO) that resembles I2C
> but lacks a slave address, requiring a bit-banging implementation.
>
> The driver exposes standard sysfs attributes for 'message' (text) and
> 'brightness'. It handles standard ASCII-to-7-segment mapping using the
> kernel's map_to_7segment utility.
>
> Note on implementation:
> This driver is implemented as a standalone platform driver rather than
> using the auxiliary display 'linedisp' framework. The TM1637 protocol
> requires manual handling of the decimal point (merging it with the
> previous digit), which is difficult to achieve cleanly with the current
> linedisp character buffer abstraction.

Wait, what happens to
https://lore.kernel.org/lkml/20251121145911.176033-1-jefflessard3@gmail.com/
?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] auxdisplay: Add support for TM1637 7-segment display driver

[Andy Shevchenko]

On Tue, Jan 13, 2026 at 6:03 AM Siratul Islam <email@sirat.me> wrote:

...

> Note on implementation:
> This driver is implemented as a standalone platform driver rather than
> using the auxiliary display 'linedisp' framework. The TM1637 protocol
> requires manual handling of the decimal point (merging it with the
> previous digit), which is difficult to achieve cleanly with the current
> linedisp character buffer abstraction.

We need this for 7-segment + dot point anyway, so consider extending
linedisp to cover this (via callback to the driver or better way).


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] auxdisplay: tm1637: Add driver for TM1637

[Andy Shevchenko]

On Tue, Jan 13, 2026 at 6:03 AM Siratul Islam <email@sirat.me> wrote:
>
> Add a driver for the Titanmec TM1637 7-segment display controller.
>
> The TM1637 uses a custom two-wire protocol (similar to I2C but without
> a slave address) which requires bit-banging via GPIOs. This driver
> implements the protocol manually as it does not fit the standard I2C
> subsystem.
>
> The driver exposes a character device interface via sysfs:
> - 'message': Write a string to display (supports alphanumeric & dots).
> - 'brightness': Control intensity (0-8).
>
> Standard 7-segment mapping is handled using the kernel's map_to_7segment
> utility.

Thanks for your contribution. I believe in the current form (design
wise) this is no go (we want linedisp and better understanding of the
protocol here), but below is just a shallow review to get you an idea
about some common mistakes people make (usually style related) to
avoid similar problems in the future. It means a free review for
making your code better in the future.

...

> +Date:          January 2026
> +KernelVersion: 6.19

At bare minimum this could be material for 6.21. Use
https://hansen.beer/~dave/phb/ prediction machine for this.

...

> +config TM1637
> +       tristate "Shenzhen Titan Micro Electronics TM1637 7-Segment Display"
> +       depends on GPIOLIB || COMPILE_TEST
> +       select AUXDISPLAY
> +       help
> +         This driver provides support for the Titanmec TM1637 7-segment
> +         display controller connected via GPIO bit-banging.
> +
> +         This driver exposes a character interface for controlling the
> +         display content and brightness via sysfs.

We also expect the name of the module being mentioned if the user chooses M.

...

The below block doesn't follow the IWYU (Include What You Use)
principle. E.g., bits.h is missing.

> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>

+ blank line

> +#include <linux/map_to_7segment.h>

...

> +/* Protocol timing */
> +#define TM1637_BIT_DELAY_MIN 100
> +#define TM1637_BIT_DELAY_MAX 120

What units are these? We do suffixes like _MS for milliseconds, for example.

...

> +struct tm1637 {
> +       struct device *dev;
> +       struct gpio_desc *clk;
> +       struct gpio_desc *dio;
> +       struct mutex lock; /* Protects display buffer and brightness */
> +       u8 brightness;
> +       u8 buf[TM1637_DIGITS];

Always think about alignment in the data types.  Definitely we want a
possibility for the code to use one assembly instruction to get a full
buffer to the register. With this layout it becomes impossible on
(some) 32-bit architectures. Also `pahole` should be your tool to
check structure layouts.

> +};

...

> +static void tm1637_delay(void)
> +{
> +       usleep_range(TM1637_BIT_DELAY_MIN, TM1637_BIT_DELAY_MAX);
> +}

Unneeded wrapper, just use fsleep() directly in the code with the
comment above on each case.

...

> +static void tm1637_start(struct tm1637 *tm)
> +{
> +       gpiod_direction_output(tm->dio, 1);
> +       gpiod_set_value(tm->clk, 1);
> +       tm1637_delay();
> +       gpiod_set_value(tm->dio, 0);
> +       tm1637_delay();
> +       gpiod_set_value(tm->clk, 0);
> +       tm1637_delay();
> +}
> +
> +static void tm1637_stop(struct tm1637 *tm)
> +{
> +       gpiod_direction_output(tm->dio, 0);
> +       gpiod_set_value(tm->clk, 1);
> +       tm1637_delay();
> +       gpiod_set_value(tm->dio, 1);
> +       tm1637_delay();
> +}
> +
> +static bool tm1637_write_byte(struct tm1637 *tm, u8 data)
> +{
> +       bool ack;
> +       int i;
> +
> +       for (i = 0; i < 8; i++) {
> +               gpiod_set_value(tm->clk, 0);
> +               tm1637_delay();
> +
> +               if (data & BIT(i))
> +                       gpiod_direction_input(tm->dio);
> +               else
> +                       gpiod_direction_output(tm->dio, 0);
> +
> +               tm1637_delay();
> +               gpiod_set_value(tm->clk, 1);
> +               tm1637_delay();
> +       }
> +
> +       gpiod_set_value(tm->clk, 0);
> +       gpiod_direction_input(tm->dio);
> +       tm1637_delay();
> +
> +       gpiod_set_value(tm->clk, 1);
> +       tm1637_delay();
> +
> +       ack = !gpiod_get_value(tm->dio);
> +
> +       if (!ack)
> +               gpiod_direction_output(tm->dio, 0);
> +
> +       tm1637_delay();
> +       gpiod_set_value(tm->clk, 0);
> +
> +       return ack;
> +}

This looks like the I2C bitbanging protocol. Have you checked that
one? And why I2C with raw transfers can't be used? I doubt this can't
be proper i2c peripheral driver.

...

> +       if (brightness > TM1637_BRIGHTNESS_MAX + 1)
> +               brightness = TM1637_BRIGHTNESS_MAX + 1;

min() / max()  (from minmax.h)?

...

> +       mutex_lock(&tm->lock);

> +       mutex_unlock(&tm->lock);

Use guard()() or scoped_guard() from cleanup.h.

...

> +static struct attribute *tm1637_attrs[] = {
> +       &dev_attr_message.attr,
> +       &dev_attr_brightness.attr,
> +       NULL};

Two lines on a single one. Split.

...

> +static int tm1637_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct tm1637 *tm;
> +
> +       tm = devm_kzalloc(dev, sizeof(*tm), GFP_KERNEL);
> +       if (!tm)
> +               return -ENOMEM;
> +
> +       tm->dev = dev;
> +
> +       tm->clk = devm_gpiod_get(dev, "clk", GPIOD_OUT_LOW);
> +       if (IS_ERR(tm->clk))
> +               return dev_err_probe(dev, PTR_ERR(tm->clk), "Failed to get clk GPIO\n");
> +
> +       tm->dio = devm_gpiod_get(dev, "dio", GPIOD_OUT_LOW);
> +       if (IS_ERR(tm->dio))
> +               return dev_err_probe(dev, PTR_ERR(tm->dio), "Failed to get dio GPIO\n");

> +       mutex_init(&tm->lock);

Use devm_mutex_init().

> +       tm->brightness = TM1637_BRIGHTNESS_MAX + 1;
> +
> +       platform_set_drvdata(pdev, tm);
> +       tm1637_update_display(tm);
> +
> +       return 0;
> +}

...

> +       mutex_destroy(&tm->lock);

Will be gone per above.

...

> +static const struct of_device_id tm1637_of_match[] = {
> +       {.compatible = "titanmec,tm1637"},

Missing spaces inside {}.

> +       {}};

Two lines on a single one. Please, split.

--
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/4] auxdisplay: Add support for TM1637 7-segment display driver

[Andy Shevchenko]

On Tue, Jan 13, 2026 at 03:19:32PM +0600, Sirat wrote:

> Regarding Jean-François's series: I did check that out. It seems focused on
> the TM16xx variants that speak proper I2C or SPI (like TM1650/TM1638).

> The TM1637 has no slave address,

IIRC we have drivers for I2C peripherals that do not support the address.
Seems gpio-pca9570.c is the one I remember.

> and data is sent LSB-first which makes it
> incompatible with standard I2C adapters,

Does it? What prevents from reversing bits? (We have bitrev.h APIs)

> so I can't really piggyback on that driver's I2C backend.

Please, reconsider.

...

And please, do not top-post!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] auxdisplay: tm1637: Add driver for TM1637

[Sirat]

Hi Andy,

Thanks for the review. appreciate the mentorship on the coding style.

On the I2C point: I did check, but the TM1637 is incompatible with
standard I2C adapters. It expects LSB-first data and there's no slave
address so bit-banging is the only option here.

I'll handle the rest in v2:
- Switching to `linedisp`.
- Fixing the struct alignment and locking with `guard()`.
- Cleaning up the headers (IWYU), units, and version numbers.

Thanks,
Sirat

On Tue, Jan 13, 2026 at 1:53 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Jan 13, 2026 at 6:03 AM Siratul Islam <email@sirat.me> wrote:
> >
> > Add a driver for the Titanmec TM1637 7-segment display controller.
> >
> > The TM1637 uses a custom two-wire protocol (similar to I2C but without
> > a slave address) which requires bit-banging via GPIOs. This driver
> > implements the protocol manually as it does not fit the standard I2C
> > subsystem.
> >
> > The driver exposes a character device interface via sysfs:
> > - 'message': Write a string to display (supports alphanumeric & dots).
> > - 'brightness': Control intensity (0-8).
> >
> > Standard 7-segment mapping is handled using the kernel's map_to_7segment
> > utility.
>
> Thanks for your contribution. I believe in the current form (design
> wise) this is no go (we want linedisp and better understanding of the
> protocol here), but below is just a shallow review to get you an idea
> about some common mistakes people make (usually style related) to
> avoid similar problems in the future. It means a free review for
> making your code better in the future.
>
> ...
>
> > +Date:          January 2026
> > +KernelVersion: 6.19
>
> At bare minimum this could be material for 6.21. Use
> https://hansen.beer/~dave/phb/ prediction machine for this.
>
> ...
>
> > +config TM1637
> > +       tristate "Shenzhen Titan Micro Electronics TM1637 7-Segment Display"
> > +       depends on GPIOLIB || COMPILE_TEST
> > +       select AUXDISPLAY
> > +       help
> > +         This driver provides support for the Titanmec TM1637 7-segment
> > +         display controller connected via GPIO bit-banging.
> > +
> > +         This driver exposes a character interface for controlling the
> > +         display content and brightness via sysfs.
>
> We also expect the name of the module being mentioned if the user chooses M.
>
> ...
>
> The below block doesn't follow the IWYU (Include What You Use)
> principle. E.g., bits.h is missing.
>
> > +#include <linux/delay.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/mutex.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/property.h>
> > +#include <linux/slab.h>
>
> + blank line
>
> > +#include <linux/map_to_7segment.h>
>
> ...
>
> > +/* Protocol timing */
> > +#define TM1637_BIT_DELAY_MIN 100
> > +#define TM1637_BIT_DELAY_MAX 120
>
> What units are these? We do suffixes like _MS for milliseconds, for example.
>
> ...
>
> > +struct tm1637 {
> > +       struct device *dev;
> > +       struct gpio_desc *clk;
> > +       struct gpio_desc *dio;
> > +       struct mutex lock; /* Protects display buffer and brightness */
> > +       u8 brightness;
> > +       u8 buf[TM1637_DIGITS];
>
> Always think about alignment in the data types.  Definitely we want a
> possibility for the code to use one assembly instruction to get a full
> buffer to the register. With this layout it becomes impossible on
> (some) 32-bit architectures. Also `pahole` should be your tool to
> check structure layouts.
>
> > +};
>
> ...
>
> > +static void tm1637_delay(void)
> > +{
> > +       usleep_range(TM1637_BIT_DELAY_MIN, TM1637_BIT_DELAY_MAX);
> > +}
>
> Unneeded wrapper, just use fsleep() directly in the code with the
> comment above on each case.
>
> ...
>
> > +static void tm1637_start(struct tm1637 *tm)
> > +{
> > +       gpiod_direction_output(tm->dio, 1);
> > +       gpiod_set_value(tm->clk, 1);
> > +       tm1637_delay();
> > +       gpiod_set_value(tm->dio, 0);
> > +       tm1637_delay();
> > +       gpiod_set_value(tm->clk, 0);
> > +       tm1637_delay();
> > +}
> > +
> > +static void tm1637_stop(struct tm1637 *tm)
> > +{
> > +       gpiod_direction_output(tm->dio, 0);
> > +       gpiod_set_value(tm->clk, 1);
> > +       tm1637_delay();
> > +       gpiod_set_value(tm->dio, 1);
> > +       tm1637_delay();
> > +}
> > +
> > +static bool tm1637_write_byte(struct tm1637 *tm, u8 data)
> > +{
> > +       bool ack;
> > +       int i;
> > +
> > +       for (i = 0; i < 8; i++) {
> > +               gpiod_set_value(tm->clk, 0);
> > +               tm1637_delay();
> > +
> > +               if (data & BIT(i))
> > +                       gpiod_direction_input(tm->dio);
> > +               else
> > +                       gpiod_direction_output(tm->dio, 0);
> > +
> > +               tm1637_delay();
> > +               gpiod_set_value(tm->clk, 1);
> > +               tm1637_delay();
> > +       }
> > +
> > +       gpiod_set_value(tm->clk, 0);
> > +       gpiod_direction_input(tm->dio);
> > +       tm1637_delay();
> > +
> > +       gpiod_set_value(tm->clk, 1);
> > +       tm1637_delay();
> > +
> > +       ack = !gpiod_get_value(tm->dio);
> > +
> > +       if (!ack)
> > +               gpiod_direction_output(tm->dio, 0);
> > +
> > +       tm1637_delay();
> > +       gpiod_set_value(tm->clk, 0);
> > +
> > +       return ack;
> > +}
>
> This looks like the I2C bitbanging protocol. Have you checked that
> one? And why I2C with raw transfers can't be used? I doubt this can't
> be proper i2c peripheral driver.
>
> ...
>
> > +       if (brightness > TM1637_BRIGHTNESS_MAX + 1)
> > +               brightness = TM1637_BRIGHTNESS_MAX + 1;
>
> min() / max()  (from minmax.h)?
>
> ...
>
> > +       mutex_lock(&tm->lock);
>
> > +       mutex_unlock(&tm->lock);
>
> Use guard()() or scoped_guard() from cleanup.h.
>
> ...
>
> > +static struct attribute *tm1637_attrs[] = {
> > +       &dev_attr_message.attr,
> > +       &dev_attr_brightness.attr,
> > +       NULL};
>
> Two lines on a single one. Split.
>
> ...
>
> > +static int tm1637_probe(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct tm1637 *tm;
> > +
> > +       tm = devm_kzalloc(dev, sizeof(*tm), GFP_KERNEL);
> > +       if (!tm)
> > +               return -ENOMEM;
> > +
> > +       tm->dev = dev;
> > +
> > +       tm->clk = devm_gpiod_get(dev, "clk", GPIOD_OUT_LOW);
> > +       if (IS_ERR(tm->clk))
> > +               return dev_err_probe(dev, PTR_ERR(tm->clk), "Failed to get clk GPIO\n");
> > +
> > +       tm->dio = devm_gpiod_get(dev, "dio", GPIOD_OUT_LOW);
> > +       if (IS_ERR(tm->dio))
> > +               return dev_err_probe(dev, PTR_ERR(tm->dio), "Failed to get dio GPIO\n");
>
> > +       mutex_init(&tm->lock);
>
> Use devm_mutex_init().
>
> > +       tm->brightness = TM1637_BRIGHTNESS_MAX + 1;
> > +
> > +       platform_set_drvdata(pdev, tm);
> > +       tm1637_update_display(tm);
> > +
> > +       return 0;
> > +}
>
> ...
>
> > +       mutex_destroy(&tm->lock);
>
> Will be gone per above.
>
> ...
>
> > +static const struct of_device_id tm1637_of_match[] = {
> > +       {.compatible = "titanmec,tm1637"},
>
> Missing spaces inside {}.
>
> > +       {}};
>
> Two lines on a single one. Please, split.
>
> --
> With Best Regards,
> Andy Shevchenko
>

Re: [PATCH 0/4] auxdisplay: Add support for TM1637 7-segment display driver

[Andy Shevchenko]

On Tue, Jan 13, 2026 at 03:19:32PM +0600, Sirat wrote:

> Regarding Jean-François's series: I did check that out. It seems focused on
> the TM16xx variants that speak proper I2C or SPI (like TM1650/TM1638).

Forgot to add. Please, Cc him and collaborate on the DT bindings as it seems
some potential clashes.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] auxdisplay: tm1637: Add driver for TM1637

[Andy Shevchenko]

On Tue, Jan 13, 2026 at 03:42:09PM +0600, Sirat wrote:
> Hi Andy,
> 
> Thanks for the review. appreciate the mentorship on the coding style.
> 
> On the I2C point: I did check, but the TM1637 is incompatible with
> standard I2C adapters. It expects LSB-first data and there's no slave
> address so bit-banging is the only option here.

As I replied earlier I do not see the problems there.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/4] auxdisplay: tm1637: Add driver for TM1637

[kernel test robot]

Hi Siratul,

kernel test robot noticed the following build errors:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linus/master v6.19-rc5 next-20260113]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Siratul-Islam/dt-bindings-vendor-prefixes-Add-titanmec/20260113-161216
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20260113040242.19156-4-email%40sirat.me
patch subject: [PATCH 3/4] auxdisplay: tm1637: Add driver for TM1637
config: riscv-randconfig-002-20260113 (attached as .config)
compiler: riscv64-linux-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260113/202601132253.Up9189vT-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202601132253.Up9189vT-lkp@intel.com/

All errors (new ones prefixed by >>):

>> error: recursive dependency detected!
   symbol AUXDISPLAY is selected by TM1637
   symbol TM1637 depends on AUXDISPLAY
   For a resolution refer to Documentation/kbuild/kconfig-language.rst
   subsection "Kconfig recursive dependency limitations"

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/4] dt-bindings: vendor-prefixes: Add titanmec

[Krzysztof Kozlowski]

On 13/01/2026 05:02, Siratul Islam wrote:
> Add vendor prefix for Shenzhen Titan Micro Electronics Co., Ltd.
> 
> Signed-off-by: Siratul Islam <email@sirat.me>
> ---


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

Best regards,
Krzysztof

Re: [PATCH 2/4] dt-bindings: auxdisplay: Add titanmec,tm1637

[Krzysztof Kozlowski]

On 13/01/2026 05:02, Siratul Islam wrote:
> +title: TM1637 7-segment LED display controller
> +
> +maintainers:
> +  - Siratul Islam <email@sirat.me>
> +
> +description:
> +  The TM1637 is a 7-segment LED display controller with a two-wire serial
> +  interface. It can drive up to 4 digits and includes brightness control.
> +
> +properties:
> +  compatible:
> +    const: titanmec,tm1637
> +
> +  clk-gpios:
> +    maxItems: 1
> +    description: GPIO for clock line
> +
> +  dio-gpios:
> +    maxItems: 1
> +    description: GPIO for data input/output line

So you are bitbanging in the kernel? This looks too tightly coupled to
the implementation. If this is some sort of standard interface, I would
expect following standard bus-child relationship, like i2c, spi, serial
or even 1-wire.

> +
Best regards,
Krzysztof