Re: [PATCH v9 1/6] mfd: khadas-mcu: Add per-variant configuration infrastructure and VIM4 support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Passing child-specific configuration (fan registers and cooling levels) from the core MFD parent driver to the child violates MFD separation of concerns.
- [Medium] The patch claims to remove hardcoded constants and add VIM4 support but fails to include the corresponding C code modifications, leaving the feature entirely unimplemented.
--

--- Patch [1]: [PATCH v9 1/6] mfd: khadas-mcu: Add per-variant configuration infrastructure and VIM4 support ---
commit 9c30c9c20ad7d8a42c271a8ebbe94020ab58ae90
Author: Ronald Claveau <linux-kernel-dev@aliel.fr>

mfd: khadas-mcu: Add per-variant configuration infrastructure and VIM4 support

This commit adds per-variant configuration structures and enums to manage
different MCU variants, specifically adding VIM4 support. It also introduces
a structure to pass fan configuration to the fan sub-driver to remove
hardcoded constants.

> diff --git a/include/linux/mfd/khadas-mcu.h b/include/linux/mfd/khadas-mcu.h
> index a99ba2ed0e4e0..e4044c251b97e 100644
> --- a/include/linux/mfd/khadas-mcu.h
> +++ b/include/linux/mfd/khadas-mcu.h
> @@ -70,6 +70,13 @@
>  #define KHADAS_MCU_WOL_INIT_START_REG		0x87 /* WO */
>  #define KHADAS_MCU_CMD_FAN_STATUS_CTRL_REG	0x88 /* WO */
>  
> +/* VIM4 specific registers */
> +#define KHADAS_MCU_VIM4_REST_CONF_REG		0x2c /* WO - reset EEPROM */
> +#define KHADAS_MCU_VIM4_LED_ON_RAM_REG		0x89 /* WO - LED volatile */
> +#define KHADAS_MCU_VIM4_FAN_CTRL_REG		0x8a /* WO */
> +#define KHADAS_MCU_VIM4_WDT_EN_REG		0x8b /* WO */
> +#define KHADAS_MCU_VIM4_SYS_RST_REG		0x91 /* WO */

[Severity: Medium]
Will these new VIM4 specific registers exceed the unchanged max_register
(0x88) in the core driver's regmap configuration?

It looks like the C file modifications are missing from this patch, meaning
these new registers might be rejected by the regmap API if the configuration
is not updated to allow access up to 0x91.

[ ... ]

> @@ -88,4 +95,21 @@ struct khadas_mcu {
>  	struct regmap *regmap;
>  };
>  
> +/**
> + * struct khadas_mcu_fan_pdata - fan sub-driver configuration
> + * @fan_reg: register address to write the fan level
> + * @levels: table mapping logical cooling-device states to raw fan levels
> + * @nlevels: number of entries in @levels
> + */
> +struct khadas_mcu_fan_pdata {
> +	unsigned int fan_reg;
> +	const unsigned int *levels;
> +	unsigned int nlevels;
> +};

[Severity: Medium]
Does passing child-specific configuration like fan registers and cooling
levels from the core MFD parent driver to the child violate MFD separation
of concerns?

The MFD subsystem guidelines suggest that the core MFD parent driver should
only handle shared core resources. Private resources consumed exclusively by
a single child driver should typically be initialized directly within that
child rather than passed from the parent.

[Severity: Medium]
The commit message mentions removing hardcoded constants and passing these
parameters to the fan sub-driver, but is this structure actually used
anywhere in the code?

The corresponding C file modifications appear to be missing from the commit,
which leaves this structure entirely unused.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805-add-mcu-fan-khadas-vim4-v9-0-75df78710b9e@aliel.fr?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic