Message-ID: <E66BF0B3-C061-4D5E-A929-8E940F3336ED@gmail.com>
Date: Mon, 30 Jun 2025 21:02:41 -0400
From: Jean-François Lessard <jefflessard3@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, 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

Le 30 juin 2025 02 h 12 min 16 s HAE, Krzysztof Kozlowski <krzk@...nel.org> a écrit :
>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
>

Well noted.

>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
>

Well noted.

>
>> - 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.
>

Well noted. Will drop all usage of clang-format and use kernel style only.

>
>> +	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.
>

Well noted.

>> +	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.
>

Well noted.

>
>
>Best regards,
>Krzysztof

Jean-François Lessard

Powered by blists - more mailing lists