Re: [PATCH v6 03/11] HID: spi-hid: add transport driver skeleton for HID over SPI bus

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] HID: spi-hid: multi-value sysfs attributes and missing ABI documentation
--

commit 4c66f3d16c395b79d0997b29357fb80b5a6f2708
Author: Angela Czubak <acz@semihalf.com>

HID: spi-hid: add transport driver skeleton for HID over SPI bus

This commit creates the spi-hid directory with its Kconfig and Makefile, and
adds the initial core driver skeleton including basic device structures,
sysfs attributes, and probe/remove functions.

> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> new file mode 100644
> index 0000000000000..02a7608c4b882
> --- /dev/null
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
> @@ -0,0 +1,213 @@

[ ... ]

> +static ssize_t bus_error_count_show(struct device *dev,
> +				    struct device_attribute *attr, char *buf)
> +{
> +	struct spi_hid *shid = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%u (%d)\n",
> +			  shid->bus_error_count, shid->bus_last_error);
> +}
> +static DEVICE_ATTR_RO(bus_error_count);
> +
> +static ssize_t regulator_error_count_show(struct device *dev,
> +					  struct device_attribute *attr,
> +					  char *buf)
> +{
> +	struct spi_hid *shid = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%u (%d)\n",
> +			  shid->regulator_error_count,
> +			  shid->regulator_last_error);
> +}
> +static DEVICE_ATTR_RO(regulator_error_count);
> +
> +static ssize_t device_initiated_reset_count_show(struct device *dev,
> +						 struct device_attribute *attr,
> +						 char *buf)
> +{
> +	struct spi_hid *shid = dev_get_drvdata(dev);
> +
> +	return sysfs_emit(buf, "%u\n", shid->dir_count);
> +}
> +static DEVICE_ATTR_RO(device_initiated_reset_count);
> +
> +static struct attribute *spi_hid_attrs[] = {
> +	&dev_attr_bus_error_count.attr,
> +	&dev_attr_regulator_error_count.attr,
> +	&dev_attr_device_initiated_reset_count.attr,
> +	NULL	/* Terminator */
> +};
> +
> +static const struct attribute_group spi_hid_group = {
> +	.attrs = spi_hid_attrs,
> +};
> +
> +const struct attribute_group *spi_hid_groups[] = {
> +	&spi_hid_group,
> +	NULL
> +};
> +EXPORT_SYMBOL_GPL(spi_hid_groups);

[Severity: Low]
In drivers/hid/spi-hid/spi-hid-core.c, bus_error_count_show() and
regulator_error_count_show() format both an unsigned error count and a signed
last error code in parentheses ("%u (%d)\n") into a single sysfs file when
userspace reads bus_error_count or regulator_error_count via spi_hid_groups
(which is attached to .dev_groups in the later ACPI and Device Tree support
commits in this series).

Per Documentation/filesystems/sysfs.rst, sysfs attributes should export one
value per file without custom formatting or mixing types, and new sysfs
attributes should be documented in Documentation/ABI/.

Should bus_last_error and regulator_last_error be split into separate sysfs
attributes (or moved to debugfs), and should Documentation/ABI/ entries be
added for the attributes in spi_hid_attrs?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-send-upstream-v6-0-cb9ba4b3d6fa@chromium.org?part=3