Re: [PATCH v2 3/8] Add Advantech EIO Hardware Monitor driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Use of bespoke parent-child accessors instead of standard kernel APIs.
- [Low] Static tables missing `const` qualifier and containing entirely unused fields.
--

--- Patch [3]: [PATCH v2 3/8] Add Advantech EIO Hardware Monitor driver ---
commit 63ead2676c728959d0dbe217692c0d9b6f79eaa3
Author: Ramiro Oliveira <ramiro.oliveira@advantech.com>

Add Advantech EIO Hardware Monitor driver

This driver controls the Hardware Monitor block of the Advantech EIO chip.
There is a separate driver for the thermal device driver since it's a use
case we support currently.

> diff --git a/drivers/hwmon/eio-hwmon.c b/drivers/hwmon/eio-hwmon.c
> index 0000000000000..dd877664e9e89
> --- /dev/null
> +++ b/drivers/hwmon/eio-hwmon.c
[ ... ]
> +static struct {
> +	u8 cmd;
> +	u8 max;
> +	signed int shift;
> +	char name[MAX_NAME];
> +	u8 ctrl[16];
> +	u16 multi[16];
> +	char item[16][MAX_NAME];
> +	char labels[32][MAX_NAME];
> +} sen_info[] = {

[Severity: Low]
This isn't a bug, but should this array be declared as static const so it is
placed in the .rodata section rather than .data?

Also, it looks like the name and item fields are initialized but never read
anywhere in this driver. Can they be removed to reduce the memory footprint?

[ ... ]
> +static struct {
> +	enum _sen_type type;
> +	u8   ctrl;
> +	int  size;
> +	bool write;
> +} ctrl_para[] = {

[Severity: Low]
Similarly, should this lookup table be declared as static const?

[ ... ]
> +static int pmc_read(struct device *mfd, enum _sen_type type, u8 dev_id, u8 ctrl, void *data)
> +{
> +	int idx = para_idx(type, ctrl);
> +	int ret = 0;
> +
> +	if (idx == 0)
> +		return -EINVAL;
> +
> +	if (WARN_ON(!data))
> +		return -EINVAL;
> +
> +	struct pmc_op op = {
> +		 .cmd       = sen_info[type].cmd | EIO_FLAG_PMC_READ,
> +		 .control   = ctrl,
> +		 .device_id = dev_id,
> +		 .size	    = ctrl_para[idx].size,
> +		 .payload   = (u8 *)data,
> +	};
> +
> +	ret = eio_core_pmc_operation(mfd, &op);

[Severity: Medium]
Does this bypass standard kernel APIs by using a custom parent-child accessor?

The MFD subsystem guidelines recommend avoiding bespoke accessors or helper
functions in the parent to pass state to child devices. Instead, child
drivers are encouraged to use standard subsystem APIs to prevent rigid,
tightly-coupled designs.

Could this communication be refactored to use standard APIs like regmap?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714-upstream-v2-v2-0-76e5e41026db@advantech.com?part=3