Re: [PATCH v4 3/4] rockchip: rk3399: Add ROC-PC-PLUS board detection

Hi Fabio,

On 8/12/26 1:01 AM, Fabio Estevam wrote:
> Hi Quentin,
> 
> On Tue, Aug 11, 2026 at 8:55 AM Quentin Schulz <[email protected]> wrote:
> 
>> Ideally, we should be able to automatically detect whether it's a
>> roc-pc-mezzanine-rk3399, but I couldn't find schematics for it and from
>> the Device Tree, there doesn't seem to be something we could use to
>> detect it like we do for ROC-PC-PLUS/ROC-PC. So instead I recommend to
>> check for CONFIG_IS_ENABLED(DM_I2C). If it ever gets enabled for
>> roc-pc-mezzanine-rk3399_defconfig, the logic will return that it's NOT a
>> ROC-PC-PLUS and thus use CONFIG_DEFAULT_FDT_FILE which is
>> rockchip/rk3399-roc-pc-mezzanine.dtb which is correct.
> 
> DM_I2C is also selected by roc-pc-mezzanine-rk3399_defconfig.
> 

Yes, but it's missing SPL_DM_I2C.

> We can use SPL_I2C to distinguish, like this:
> 

Unfortunately, we cannot simply rely on SPL_I2C as the function is only 
implemented in i2c-uclass.c which is only compiled when SPL_DM_I2C (and 
SPL_I2C) is enabled.

There's also an issue in the dependencies for SPL_DM_I2C and TPL_DM_I2C 
as they both depend on I2C (via the if at the top of 
drivers/i2c/Kconfig) and DM_I2C while they should depend on SPL_I2C 
(respectively TPL_I2C). I'm not sure why they depend on DM_I2C as it's 
for proper, not xPL...

TPL_SYS_I2C_LEGACY depends on !SPL_DM_I2C but I'm assuming it should 
rather depend on !TPL_DM_I2C.

Same for SPL_DM_I2C_GPIO, not sure why it depends on DM_I2C_GPIO. 
SPL_I2C_MUX also depends on I2C_MUX, for what reason?

VPL_DM_I2C should depend on VPL_I2C but it doesn't exist, so we probably 
should just remove the symbol since it won't do anything as is.

None are a blocker for this series.

> --- a/board/firefly/roc-pc-rk3399/roc-pc-rk3399.c
> +++ b/board/firefly/roc-pc-rk3399/roc-pc-rk3399.c
> @@ -68,6 +68,9 @@ static bool is_roc_pc_plus(void)
>   {
>          struct udevice *bus, *dev;
> 
> +       /* roc-pc-mezzanine-rk3399_defconfig does not select SPL_I2C */
> +       if (!CONFIG_IS_ENABLED(SPL_I2C))
> +               return false;

Please add a newline after the return false;

It should be !CONFIG_IS_ENABLED(DM_I2C) as CONFIG_IS_ENABLED is going to 
add the appropriate prefix based on the current stage being built (so 
definitely no SPL/TPL/VPL to be added manually) and CONFIG_SPL_I2C is 
not enough to build the dm_i2c_probe implementation, we need 
CONFIG_SPL_DM_I2C (well, and CONFIG_SPL_I2C because otherwise the 
drivers/i2c directory isn't traversed and because SPL_DM_I2C is missing 
a dependency on it).

Cheers,
Quentin