linux-kernel - Re: [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <13E7CEAE-6865-485B-9486-7EBEEE46E285@gmail.com>
Date: Thu, 21 Aug 2025 12:42:21 -0400
From: Jean-François Lessard <jefflessard3@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
CC: Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 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>,
 Martin Blumenstingl <martin.blumenstingl@...glemail.com>
Subject: Re: [PATCH v3 3/4] auxdisplay: Add TM16xx 7-segment LED matrix display controllers driver

Le 21 août 2025 03 h 43 min 51 s HAE, Krzysztof Kozlowski <krzk@...nel.org> a écrit :
>On Wed, Aug 20, 2025 at 12:31:16PM -0400, Jean-François Lessard wrote:
>> +/**
>> + * tm16xx_map_seg7_store() - Sysfs: set 7-segment character map (binary blob)
>> + * @dev: pointer to device
>> + * @attr: device attribute (unused)
>> + * @buf: new mapping (must match size of map_seg7)
>> + * @cnt: buffer length
>> + *
>> + * Return: cnt on success, negative errno on failure
>> + */
>> +static ssize_t tm16xx_map_seg7_store(struct device *dev,
>> +				     struct device_attribute *attr,
>> +				     const char *buf, size_t cnt)
>> +{
>> +	if (cnt != sizeof(map_seg7))
>> +		return -EINVAL;
>> +	memcpy(&map_seg7, buf, cnt);
>> +	return cnt;
>> +}
>> +
>> +static DEVICE_ATTR(value, 0644, tm16xx_value_show, tm16xx_value_store);
>> +static DEVICE_ATTR(num_digits, 0444, tm16xx_num_digits_show, NULL);
>> +static DEVICE_ATTR(map_seg7, 0644, tm16xx_map_seg7_show, tm16xx_map_seg7_store);
>
>Where did you document the ABI?
>

Currently, there is no formal ABI documentation for the TM16xx sysfs attributes.
While map_seg7 follows existing auxdisplay conventions (which lack ABI docs),
I plan to add TM16xx-specific ABI documentation under
Documentation/ABI/testing/sysfs-class-leds-tm16xx in the v4 submission.

>> +
>> +static struct attribute *tm16xx_main_led_attrs[] = {
>> +	&dev_attr_value.attr,
>> +	&dev_attr_num_digits.attr,
>> +	&dev_attr_map_seg7.attr,
>> +	NULL,
>> +};
>> +ATTRIBUTE_GROUPS(tm16xx_main_led);
>> +
>
>...
>
>> +#if IS_ENABLED(CONFIG_I2C)
>> +/**
>> + * tm16xx_i2c_probe() - Probe callback for I2C-attached controllers
>> + * @client: pointer to i2c_client
>> + *
>> + * Return: 0 on success, negative error code on failure
>> + */
>> +static int tm16xx_i2c_probe(struct i2c_client *client)
>> +{
>> +	const struct tm16xx_controller *controller;
>> +	struct tm16xx_display *display;
>> +	int ret;
>> +
>> +	controller = i2c_get_match_data(client);
>> +	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;
>> +}
>> +
>> +/**
>> + * tm16xx_i2c_remove() - Remove callback for I2C-attached controllers
>> + * @client: pointer to i2c_client
>
>Please don't ever add comments, especially kerneldocs, for such obvious
>driver API. This function cannot be anything else than what you
>described. Why? Linux core driver model tells that. You never have to
>repeat what Linux core driver model is...
>

Well received, thank you. I will drop these trivial kernel-doc comments.

>Best regards,
>Krzysztof
>

Best regards,

Jean-François Lessard