Re: [PATCH 5/8] can: rcar_canfd: Do not set registers selecting the CAN mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch adds the `mode_select_na` flag to support the RZ/G3S SoC, but fails to provide the RZ/G3S hardware info structure with the flag set, rendering the logic as dead code.
--

--- Patch [5]: [PATCH 5/8] can: rcar_canfd: Do not set registers selecting the CAN mode ---
commit c89aa117ce922f6dfb13d5f51bb4146e8af1be2c
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

can: rcar_canfd: Do not set registers selecting the CAN mode

This patch adds the mode_select_na flag to struct rcar_canfd_hw_info to avoid
writing to unavailable CAN mode bits on the Renesas RZ/G3S SoC. The flag is
initialized to 0 for existing hardware structures, and a conditional check
is added in the controller reset path to skip mode selection if the flag is
set.

Link: https://lore.kernel.org/all/1d9719e3-10ff-4cd8-b729-55fea93c37ce@wanadoo.fr

> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index 6ec2d7a031238..7f01afe60a4c2 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -445,6 +445,7 @@ struct rcar_canfd_hw_info {
>  	unsigned shared_can_regs:1;	/* Has shared classical can registers */
>  	unsigned external_clk:1;	/* Has external clock */
>  	unsigned fcan_pclk:1;		/* Has fcan sourced from pclk. */
> +	unsigned mode_select_na:1;	/* Has no bits for selecting the mode. */
>  };
>  
>  /* Channel priv data */

[Severity: Medium]
Was the RZ/G3S hardware info structure missing from this patch series?

The commit message mentions adding this flag to cover the RZ/G3S SoC, but
all existing structures are updated to set this flag to 0, and no new
structure is added that sets it to 1.

[ ... ]

> @@ -846,6 +852,9 @@ static int rcar_canfd_reset_controller(struct rcar_canfd_global *gpriv)
>  		}
>  
>  		/* Set the controller into appropriate mode */
> +		if (gpriv->info->mode_select_na)
> +			continue;
> +
>  		if (gpriv->info->ch_interface_mode) {
>  			/* Do not set CLOE and FDOE simultaneously */
>  			if (!gpriv->fdmode) {

[Severity: Medium]
Since mode_select_na is never set to 1 anywhere in the driver, doesn't this
conditional evaluate to false for all devices, resulting in dead code?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707102418.1646159-1-claudiu.beznea+renesas@tuxon.dev?part=5