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

["Jean-François Lessard" <jefflessard3@gmail.com>]

Le 21 août 2025 03 h 43 min 51 s HAE, Krzysztof Kozlowski <krzk@kernel.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