Re: [PATCH net-next v2 3/8] mfd: add X-Powers AC200 support

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The driver uses PLATFORM_DEVID_NONE instead of PLATFORM_DEVID_AUTO when registering MFD children, risking sysfs naming collisions.
- [Low] The commit message prefix violates MFD subsystem capitalization and formatting rules.
- [Low] The `i2c_device_id` array uses implicit positional initializers instead of named initializers.
--

--- Patch [3]: [PATCH net-next v2 3/8] mfd: add X-Powers AC200 support ---
commit 98ebae297c54eb1823cd6d77c3a20fc90dde205b
Author: James Hilliard <[email protected]>

mfd: add X-Powers AC200 support

This commit introduces the MFD core driver for the X-Powers AC200 mixed-signal
companion IC. It initializes shared regmap access over I2C, applies vendor
resets, and registers Ethernet PHY control children.

> mfd: add X-Powers AC200 support

[Severity: Low]
This isn't a bug, but should the commit message prefix follow the standard
MFD capitalization and naming format? The subsystem guidelines require
capitalizing the description and using the driver name, such as
mfd: ac200: Add X-Powers AC200 support.

> The X-Powers AC200 is a mixed-signal companion IC with a paged register
> map accessed over I2C.

[ ... ]

> --- /dev/null
> +++ b/drivers/mfd/ac200.c
> @@ -0,0 +1,207 @@

[ ... ]

> +	ephy_node = of_get_compatible_child(dev->of_node,
> +					    "x-powers,ac200-ephy-ctl");
> +	if (!ephy_node)
> +		return 0;
> +
> +	ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, ac200_cells,
> +				   ARRAY_SIZE(ac200_cells), NULL, 0, NULL);

[Severity: Medium]
Could using PLATFORM_DEVID_NONE here cause sysfs naming collisions if
multiple instances of the AC200 are present on different I2C buses? The MFD
subsystem guidelines suggest preferring PLATFORM_DEVID_AUTO for automatic
cell indexing to prevent driver load failures.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to add MFD devices\n");
> +
> +	return 0;
> +}

[ ... ]

> +static const struct of_device_id ac200_of_match[] = {
> +	{ .compatible = "x-powers,ac200" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ac200_of_match);
> +
> +static const struct i2c_device_id ac200_i2c_ids[] = {
> +	{ "ac200" },

[Severity: Low]
This isn't a bug, but does the I2C subsystem require initialized arrays of
type struct i2c_device_id to use named initializers instead of implicit
positional ones? It looks like this should be { .name = "ac200" }.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3