Re: [PATCH net-next v2 2/3] net: pcs: rzn1-miic: Fix miic register initialization loop

[Lad, Prabhakar]

Hi Kyle,

On Thu, Oct 1, 2026 at 1:28 PM <netdev-bot+sashiko@xxxxxxxxxx> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] The Fixes tag and commit message of "net: pcs: rzn1-miic: Fix
>   miic register initialization loop" blame the wrong commit and describe…
>
> --
>
> net: pcs: rzn1-miic: Fix miic register initialization loop
>
> This commit changes the loop in miic_init_hw() that writes default
> register values. The loop now runs for miic_port_max - miic_port_start + 1
> iterations instead of miic_port_max, so the count comes from the SoC port
> range.
>
> > The function to write default values to miic registers was looping from
> > zero to miic_port_max, which might not cover all ports depending on SoC
> > numbering. Determine the port count from SoC data and loop over that.
> >
> > Fixes: c112520de041758e ("net: pcs: rzn1-miic: move port range handling into SoC data")
>
> [Severity: Medium]
> Is this Fixes tag pointing at the right commit?
>
> Before this series, the loop in miic_init_hw() already covered every
> converter on both supported SoCs:
>
>     for (port = 0; port < miic->of_data->miic_port_max; port++) {
>
> RZ/N1 uses miic_port_start = 1 and miic_port_max = 5, so the loop walks
> ports 0..4. That is 5 converters.
>
> RZ/T2H uses miic_port_start = 0 and miic_port_max = 4 (from 08f89e42121d),
> so the loop walks ports 0..3. That is CONV0..CONV3.
>
> c112520de041 only added the RZ/N1 data.
>
> The loop stops covering CONV3 only after the previous patch in this
> series, "net: pcs: rzn1-miic: Fix port numbering on RZ/T2H". That patch
> changes rzt2h_miic_of_data.miic_port_max from 4 to 3. So this patch seems
> to fix a regression from earlier in the same series, not a problem from
> c112520de041.
>
> This leaves a bisect window. At the previous patch, miic_init_hw() on
> RZ/T2H runs only 3 times. The CONVRST bit for CONV3 is never cleared
> during init.
>
> The two patches also carry different Fixes tags (08f89e42121d and
> c112520de041), so stable trees could pick up one without the other. If a
> tree takes only this patch, RZ/T2H still has miic_port_max = 4:
>
>     num_ports = miic->of_data->miic_port_max -
>         miic->of_data->miic_port_start + 1;    /* 4 - 0 + 1 = 5 */
>
> The loop would then clear MIIC_CONVRST_PHYIF_RST(4), which is for a
> converter RZ/T2H does not have. If a tree takes only the previous patch,
> CONV3 is skipped as described above.
>
> Could the miic_port_max change and the num_ports change be squashed into
> one patch with Fixes: 08f89e42121d ("net: pcs: rzn1-miic: Add RZ/T2H MIIC
> support")? If not, could the commit message at least state the dependency
> on the previous patch and drop the c112520de041 attribution?
>
I totally agree with this feedback.

Cheers,
Prabhakar

> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928-miic-validate-dtb-v2-0-1ee3d33748e9%40reliablecontrols.com
>