Re: [PATCH net-next 1/2] net: pcs: rzn1-miic: Make usage of miic_port_max consistent

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Kyle,

On Fri, 25 Sept 2026 at 17:20, Kyle Hendry via B4 Relay
<devnull+khendry.reliablecontrols.com@kernel.org> wrote:
> From: Kyle Hendry <khendry@reliablecontrols.com>
>
> miic_port_max is used both as the last port number and the port count
> which can be different depending on SoC numbering. Use compile time
> information to always set this as count and fix logic that was expecting
> the last port number.
>
> Signed-off-by: Kyle Hendry <khendry@reliablecontrols.com>

Thanks for your patch!

> --- a/drivers/net/pcs/pcs-rzn1-miic.c
> +++ b/drivers/net/pcs/pcs-rzn1-miic.c
> @@ -59,6 +59,8 @@
>
>  #define MIIC_MAX_NUM_RSTS              2
>
> +#define MIIC_PORT_END(x) ((x)->miic_port_start + (x)->miic_port_max - 1)
> +
>  /**
>   * struct modctrl_match - Matching table entry for  convctrl configuration
>   *                       See section 8.2.1 of manual.
> @@ -222,7 +224,7 @@ enum miic_type {
>   * @index_to_string: String representations of the index values
>   * @index_to_string_count: Number of entries in the index_to_string array
>   * @miic_port_start: MIIC port start number
> - * @miic_port_max: Maximum MIIC supported
> + * @miic_port_max: Count of total MIIC ports supported

miic_port_num_total?
"max" has a different meaning.

>   * @sw_mode_mask: Switch mode mask
>   * @reset_ids: Reset names array
>   * @reset_count: Number of entries in the reset_ids array
> @@ -482,7 +484,7 @@ struct phylink_pcs *miic_create(struct device *dev, struct device_node *np)
>
>         miic = platform_get_drvdata(pdev);
>         of_data = miic->of_data;
> -       if (port > of_data->miic_port_max || port < of_data->miic_port_start) {
> +       if (port > MIIC_PORT_END(of_data) || port < of_data->miic_port_start) {

IMHO the asymmetry makes the code harder to read.

As this changes the logic, I assume this is a fix?

>                 put_device(&pdev->dev);
>                 return ERR_PTR(-EINVAL);
>         }
> @@ -822,7 +824,7 @@ static struct miic_of_data rzn1_miic_of_data = {
>         .index_to_string = index_to_string,
>         .index_to_string_count = ARRAY_SIZE(index_to_string),
>         .miic_port_start = 1,
> -       .miic_port_max = 5,
> +       .miic_port_max = ARRAY_SIZE(index_to_string) - 1,

Why the -1? Oh, because the first entry of the array is not included.

>         .sw_mode_mask = GENMASK(4, 0),
>         .init_unlock_lock_regs = true,
>         .miic_write = miic_reg_writel_unlocked,
> @@ -838,7 +840,7 @@ static struct miic_of_data rzt2h_miic_of_data = {
>         .index_to_string = rzt2h_index_to_string,
>         .index_to_string_count = ARRAY_SIZE(rzt2h_index_to_string),
>         .miic_port_start = 0,
> -       .miic_port_max = 4,
> +       .miic_port_max = ARRAY_SIZE(rzt2h_index_to_string) - 1,

Why the -1? Oh, because the first entry of the array is not included.
And it is not related to .miic_port_start, which is zero here?

>         .sw_mode_mask = GENMASK(2, 0),
>         .reset_ids = rzt2h_reset_ids,
>         .reset_count = ARRAY_SIZE(rzt2h_reset_ids),

I'm not sure this is an improvement at all...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds