Re: [PATCH v6 2/3] platform/arm64: add Lenovo Yoga Slim 7x Gen 11 EC driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] hwmon: registering hardware monitoring device outside drivers/hwmon/ violates layering
- [Low] i2c: struct i2c_device_id array uses positional initialization instead of named initializers
--

commit ebc39e385bee7e339220d234fab99f84ba72d285
Author: Oleg Keri <okerixx@gmail.com>

platform/arm64: add Lenovo Yoga Slim 7x Gen 11 EC driver

This commit adds the Lenovo Yoga Slim 7x Gen 11 embedded controller driver.
It exposes thermistors, CPU fan speed, keyboard backlight LED, hotkeys, and
an fn-lock sysfs attribute.

> diff --git a/drivers/platform/arm64/lenovo-yoga-slim7x-gen11-ec.c b/drivers/platform/arm64/lenovo-yoga-slim7x-gen11-ec.c
> new file mode 100644
> index 0000000000000..53b0e7b325d35
> --- /dev/null
> +++ b/drivers/platform/arm64/lenovo-yoga-slim7x-gen11-ec.c

[ ... ]

> +static int slim7x11_ec_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct device *hwmon;
> +	struct slim7x11_ec *ec;
> +	unsigned int i;
> +	int ret;

[ ... ]

> +	dev_dbg(dev, "thermistors present: %#lx\n", ec->temp_present);
> +
> +	hwmon = devm_hwmon_device_register_with_info(dev, "yoga_slim7x_ec", ec,
> +						     &slim7x11_ec_chip_info, NULL);

[Severity: Low]
Does this code register a hardware monitoring device directly from outside the
drivers/hwmon/ directory? 

According to the hwmon subsystem guidelines, if the main functionality of a
chip is not hardware monitoring, its hardware monitoring functionality should
be implemented as an auxiliary device driver, and the hardware monitoring
driver itself should reside in drivers/hwmon/.

> +	if (IS_ERR(hwmon))
> +		return PTR_ERR(hwmon);
> +

[ ... ]

> +static const struct of_device_id slim7x11_ec_of_match[] = {
> +	{ .compatible = "lenovo,yoga-slim7x-gen11-ec" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, slim7x11_ec_of_match);
> +
> +static const struct i2c_device_id slim7x11_ec_i2c_id[] = {
> +	{ "slim7x-gen11-ec" },

[Severity: Low]
Is this using a positional initializer?

The I2C subsystem guidelines state that initialized arrays of type
struct i2c_device_id must be declared const and use named initializers
(such as .name = "slim7x-gen11-ec").

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, slim7x11_ec_i2c_id);
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014132315.4659-1-okerixx@gmail.com?part=2