Message-ID: <CANn+LW+PkAcZ2ZVSUpAQwYhDucruiiN90cAakDMXnayRPW0jUQ@mail.gmail.com>
Date: Tue, 13 Jan 2026 15:42:09 +0600
From: Sirat <email@...at.me>
To: Andy Shevchenko <andy.shevchenko@...il.com>
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

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@...il.com> wrote:
>
> 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