Message-ID: <47d24e31-1c6f-4299-aeaf-669c474c4459@kernel.org>
Date: Mon, 30 Jun 2025 08:12:16 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Jean-François Lessard <jefflessard3@...il.com>,
 Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>
Cc: Geert Uytterhoeven <geert@...ux-m68k.org>, devicetree@...r.kernel.org,
 linux-leds@...r.kernel.org, linux-kernel@...r.kernel.org,
 Andreas Färber <afaerber@...e.de>,
 Boris Gjenero <boris.gjenero@...il.com>,
 Christian Hewitt <christianshewitt@...il.com>,
 Heiner Kallweit <hkallweit1@...il.com>,
 Paolo Sabatino <paolo.sabatino@...il.com>
Subject: Re: [PATCH v2 7/8] auxdisplay: Add Titanmec TM16xx 7-segment display
 controllers driver

On 29/06/2025 15:18, Jean-François Lessard wrote:
> This patch introduces a new auxiliary display driver for TM16XX family LED controllers and compatible chips:

Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597


> - Shenzhen Titan Micro Electronics: TM1618, TM1620, TM1628, TM1650
> - Fuzhou Fuda Hisi Microelectronics: FD620, FD628, FD650, FD655, FD6551
> - Wuxi i-Core Electronics: AiP650, AiP1618, AiP1628
> - Princeton Technology: PT6964
> - Shenzhen Winrise Technology: HBS658
> 


...

> + * tm16xx_parse_dt - Parse device tree data
> + * @dev: Pointer to device structure
> + * @display: Pointer to tm16xx_display structure
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int tm16xx_parse_dt(struct device *dev, struct tm16xx_display *display)
> +{
> +	struct fwnode_handle *child;
> +	int ret, i, max_grid = 0;
> +	u8 *digits;
> +
> +	display->transpose_display_data =
> +		device_property_read_bool(dev, "titanmec,transposed");

Wrong wrapping.

> +
> +	ret = device_property_count_u8(dev, "titanmec,digits");
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to count 'titanmec,digits' property: %d\n", ret);
> +		return ret;
> +	}
> +
> +	display->num_digits = ret;
> +	dev_dbg(dev, "Number of digits: %d\n", display->num_digits);
> +
> +	digits = devm_kcalloc(dev, display->num_digits, sizeof(*digits), GFP_KERNEL);
> +	if (!digits)
> +		return -ENOMEM;
> +
> +	ret = device_property_read_u8_array(dev, "titanmec,digits", digits,
> +					    display->num_digits);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to read 'titanmec,digits' property: %d\n", ret);
> +		return ret;
> +	}
> +
> +	display->digits = devm_kcalloc(dev, display->num_digits, sizeof(*display->digits),
> +				       GFP_KERNEL);
> +	if (!display->digits)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < display->num_digits; i++) {
> +		if (digits[i] >= display->controller->max_grids) {
> +			dev_err(dev, "Digit grid %d exceeds controller max_grids %d\n",
> +				digits[i], display->controller->max_grids);
> +			return -EINVAL;
> +		}
> +
> +		display->digits[i].grid = digits[i];
> +		max_grid = umax(max_grid, digits[i]);
> +	}
> +
> +	devm_kfree(dev, digits);
> +
> +	ret = device_property_read_u8_array(dev, "titanmec,segment-mapping",
> +					    display->segment_mapping, DIGIT_SEGMENTS);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to read 'titanmec,segment-mapping' property: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	display->digit_bitmask = 0;
> +	for (i = 0; i < DIGIT_SEGMENTS; i++) {
> +		if (display->segment_mapping[i] < MIN_SEGMENT ||
> +		    display->segment_mapping[i] > MAX_SEGMENT) {
> +			dev_err(dev,
> +				"Invalid 'titanmec,segment-mapping' value: %d (must be between %d and %d)\n",
> +				display->segment_mapping[i], MIN_SEGMENT, MAX_SEGMENT);
> +			return -EINVAL;
> +		}
> +
> +		display->digit_bitmask |= BIT(display->segment_mapping[i]);
> +	}
> +
> +	display->num_leds = 0;
> +	device_for_each_child_node(dev, child) {
> +		u32 reg[2];
> +
> +		ret = fwnode_property_read_u32_array(child, "reg", reg, 2);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to read 'reg' property of led node: %d\n",
> +				ret);
> +			fwnode_handle_put(child);
> +			return ret;
> +		}
> +
> +		if (reg[0] >= display->controller->max_grids) {
> +			dev_err(dev, "LED grid %d exceeds controller max_grids %d\n",
> +				reg[0], display->controller->max_grids);
> +			fwnode_handle_put(child);
> +			return -EINVAL;
> +		}
> +
> +		if (reg[1] < MIN_SEGMENT || reg[1] > MAX_SEGMENT) {
> +			dev_err(dev,
> +				"LED segment %d is invalid (must be between %d and %d)\n",
> +				reg[1], MIN_SEGMENT, MAX_SEGMENT);
> +			fwnode_handle_put(child);
> +			return -EINVAL;
> +		}
> +
> +		max_grid = umax(max_grid, reg[0]);
> +		display->num_leds++;
> +	}
> +
> +	dev_dbg(dev, "Number of LEDs: %d\n", display->num_leds);
> +
> +	display->display_data_len = max_grid + 1;
> +	dev_dbg(dev, "Number of display grids: %zu\n", display->display_data_len);
> +
> +	display->display_data = devm_kcalloc(dev, display->display_data_len,
> +					     sizeof(*display->display_data), GFP_KERNEL);
> +	if (!display->display_data)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +/**
> + * tm16xx_probe - Probe function for tm16xx devices
> + * @display: Pointer to tm16xx_display structure
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int tm16xx_probe(struct tm16xx_display *display)
> +{
> +	struct device *dev = display->dev;
> +	struct fwnode_handle *child;
> +	int ret, i;
> +
> +	ret = tm16xx_parse_dt(dev, display);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to parse device tree: %d\n", ret);
> +		return ret;
> +	}
> +
> +	mutex_init(&display->lock);
> +	INIT_WORK(&display->flush_init, tm16xx_display_flush_init);
> +
> +	/* Initialize work structure with appropriate flush function */
> +	if (display->transpose_display_data) {
> +		INIT_WORK(&display->flush_display, tm16xx_display_flush_data_transposed);
> +		dev_info(display->dev, "Operating in transposed mode\n");
> +	} else {
> +		INIT_WORK(&display->flush_display, tm16xx_display_flush_data);
> +	}
> +
> +	display->main_led.name = TM16XX_DEVICE_NAME;
> +	display->main_led.brightness = display->controller->max_brightness;
> +	display->main_led.max_brightness = display->controller->max_brightness;
> +	display->main_led.brightness_set = tm16xx_brightness_set;
> +	display->main_led.groups = tm16xx_main_led_groups;
> +	display->main_led.flags = LED_RETAIN_AT_SHUTDOWN | LED_CORE_SUSPENDRESUME;
> +
> +	ret = devm_led_classdev_register(dev, &display->main_led);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to register main LED: %d\n", ret);
> +		return ret;
> +	}
> +
> +	display->leds =
> +		devm_kcalloc(dev, display->num_leds, sizeof(*display->leds), GFP_KERNEL);

Wrong wrapping. Use kernel style, not clang style.


> +	if (!display->leds)
> +		return -ENOMEM;
> +
> +	i = 0;
> +	device_for_each_child_node(dev, child) {
> +		struct tm16xx_led *led = &display->leds[i];
> +		struct led_init_data led_init = {
> +			.fwnode = child,
> +			.devicename = display->main_led.name,
> +			.devname_mandatory = true,
> +		};
> +		u32 reg[2];
> +
> +		ret = fwnode_property_read_u32_array(child, "reg", reg, 2);
> +		if (ret < 0) {
> +			fwnode_handle_put(child);
> +			dev_err(dev, "Failed to read LED reg property: %d\n", ret);
> +			return ret;
> +		}
> +
> +		led->grid = reg[0];
> +		led->segment = reg[1];
> +
> +		led->cdev.max_brightness = 1;
> +		led->cdev.brightness_set = tm16xx_led_set;
> +		led->cdev.flags = LED_RETAIN_AT_SHUTDOWN | LED_CORE_SUSPENDRESUME;
> +
> +		ret = devm_led_classdev_register_ext(dev, &led->cdev, &led_init);
> +		if (ret < 0) {
> +			fwnode_handle_put(child);
> +			dev_err(dev, "Failed to register LED %s: %d\n", led->cdev.name,
> +				ret);
> +			return ret;
> +		}
> +
> +		i++;
> +	}
> +
> +	ret = tm16xx_display_init(display);
> +	if (ret < 0) {
> +		dev_err(display->dev, "Failed to initialize display: %d\n", ret);
> +		return ret;
> +	}
> +
> +	dev_info(display->dev, "Display initialized successfully\n");

Drop, drivers should be silent on success. See coding style.

> +	return 0;
> +}
> +
> +/* SPI specific code */
> +static int tm16xx_spi_probe(struct spi_device *spi)
> +{
> +	const struct tm16xx_controller *controller;
> +	struct tm16xx_display *display;
> +	int ret;
> +
> +	controller = of_device_get_match_data(&spi->dev);
> +	if (!controller)
> +		return -EINVAL;
> +
> +	display = devm_kzalloc(&spi->dev, sizeof(*display), GFP_KERNEL);
> +	if (!display)
> +		return -ENOMEM;
> +
> +	display->client.spi = spi;
> +	display->dev = &spi->dev;
> +	display->controller = controller;
> +
> +	spi_set_drvdata(spi, display);
> +
> +	ret = tm16xx_probe(display);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static void tm16xx_spi_remove(struct spi_device *spi)
> +{
> +	struct tm16xx_display *display = spi_get_drvdata(spi);
> +
> +	tm16xx_display_remove(display);
> +}
> +
> +// clang-format off

Drop

> +static const struct of_device_id tm16xx_spi_of_match[] = {
> +	{ .compatible = "titanmec,tm1618", .data = &tm1618_controller },
> +	{ .compatible = "titanmec,tm1620", .data = &tm1628_controller },
> +	{ .compatible = "titanmec,tm1628", .data = &tm1628_controller },
> +	{ .compatible = "fdhisi,fd620", .data = &tm1628_controller },
> +	{ .compatible = "fdhisi,fd628", .data = &tm1628_controller },
> +	{ .compatible = "icore,aip1618", .data = &tm1618_controller },
> +	{ .compatible = "icore,aip1628", .data = &tm1628_controller },
> +	{ .compatible = "princeton,pt6964", .data = &tm1628_controller },
> +	{ .compatible = "winrise,hbs658", .data = &hbs658_controller },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, tm16xx_spi_of_match);
> +
> +static const struct spi_device_id tm16xx_spi_id[] = {
> +	{ "tm1618", 0 },
> +	{ "tm1620", 0 },
> +	{ "tm1628", 0 },
> +	{ "fd620", 0 },
> +	{ "fd628", 0 },
> +	{ "aip1618", 0 },
> +	{ "aip1628", 0 },
> +	{ "pt6964", 0 },
> +	{ "hbs658", 0 },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(spi, tm16xx_spi_id);
> +// clang-format on

Drop

> +
> +static struct spi_driver tm16xx_spi_driver = {
> +	.driver = {
> +		.name = TM16XX_DRIVER_NAME,
> +		.of_match_table = tm16xx_spi_of_match,
> +	},
> +	.probe = tm16xx_spi_probe,
> +	.remove = tm16xx_spi_remove,
> +	.shutdown = tm16xx_spi_remove,
> +	.id_table = tm16xx_spi_id,
> +};
> +
> +/* I2C specific code */
> +static int tm16xx_i2c_probe(struct i2c_client *client)
> +{
> +	const struct tm16xx_controller *controller;
> +	struct tm16xx_display *display;
> +	int ret;
> +
> +	controller = of_device_get_match_data(&client->dev);
> +	if (!controller)
> +		return -EINVAL;
> +
> +	display = devm_kzalloc(&client->dev, sizeof(*display), GFP_KERNEL);
> +	if (!display)
> +		return -ENOMEM;
> +
> +	display->client.i2c = client;
> +	display->dev = &client->dev;
> +	display->controller = controller;
> +
> +	i2c_set_clientdata(client, display);
> +
> +	ret = tm16xx_probe(display);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static void tm16xx_i2c_remove(struct i2c_client *client)
> +{
> +	struct tm16xx_display *display = i2c_get_clientdata(client);
> +
> +	tm16xx_display_remove(display);
> +}
> +
> +// clang-format off

Drop

> +static const struct of_device_id tm16xx_i2c_of_match[] = {
> +	{ .compatible = "titanmec,tm1650", .data = &tm1650_controller },
> +	{ .compatible = "icore,aip650", .data = &tm1650_controller },
> +	{ .compatible = "fdhisi,fd650", .data = &tm1650_controller },
> +	{ .compatible = "fdhisi,fd6551", .data = &fd6551_controller },
> +	{ .compatible = "fdhisi,fd655", .data = &fd655_controller },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, tm16xx_i2c_of_match);
> +
> +static const struct i2c_device_id tm16xx_i2c_id[] = {
> +	{ "tm1650", 0 },
> +	{ "aip650", 0 },
> +	{ "fd650", 0 },
> +	{ "fd6551", 0 },
> +	{ "fd655", 0 },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, tm16xx_i2c_id);
> +// clang-format on

Drop.

> +
> +static struct i2c_driver tm16xx_i2c_driver = {
> +	.driver = {
> +		.name = TM16XX_DRIVER_NAME,
> +		.of_match_table = tm16xx_i2c_of_match,
> +	},
> +	.probe = tm16xx_i2c_probe,
> +	.remove = tm16xx_i2c_remove,
> +	.shutdown = tm16xx_i2c_remove,
> +	.id_table = tm16xx_i2c_id,
> +};
> +
> +static int __init tm16xx_init(void)
> +{
> +	int ret;
> +
> +	ret = spi_register_driver(&tm16xx_spi_driver);
> +	if (ret)
> +		return ret;
> +
> +	ret = i2c_add_driver(&tm16xx_i2c_driver);
> +	if (ret) {
> +		spi_unregister_driver(&tm16xx_spi_driver);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void __exit tm16xx_exit(void)
> +{
> +	i2c_del_driver(&tm16xx_i2c_driver);
> +	spi_unregister_driver(&tm16xx_spi_driver);
> +}
> +
> +module_init(tm16xx_init);
> +module_exit(tm16xx_exit);


> +
> +MODULE_AUTHOR("Jean-François Lessard");
> +MODULE_DESCRIPTION("TM16XX Compatible LED Display Controllers");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("spi:tm16xx");
> +MODULE_ALIAS("i2c:tm16xx");

Drop these two.



Best regards,
Krzysztof

Powered by blists - more mailing lists