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.3Hi 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
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
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
>
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
© 2016 - 2026 Red Hat, Inc.