| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75VdCoHPp_ynzsrvbzx_LY_N5x4sMxvzQnDkatWiEe91j1A@mail.gmail.com>
Date: Tue, 13 Jan 2026 09:52:49 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Siratul Islam <email@...at.me>
Cc: andy@...nel.org, geert@...ux-m68k.org, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 3/4] auxdisplay: tm1637: Add driver for TM1637
On Tue, Jan 13, 2026 at 6:03 AM Siratul Islam <email@...at.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
Powered by blists - more mailing lists