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: <20250821-passionate-mouse-of-essence-2c5af4@kuoka>
Date: Thu, 21 Aug 2025 09:43:51 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Jean-François Lessard <jefflessard3@...il.com>
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

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?

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

Best regards,
Krzysztof