Re: [PATCH net-next v5 03/11] net: dsa: realtek: convert variants into real drivers

[Vladimir Oltean]

On Tue, Jan 30, 2024 at 08:13:22PM -0300, Luiz Angelo Daros de Luca wrote:
> Previously, the interface modules realtek-smi and realtek-mdio served as
> a platform and an MDIO driver, respectively. Each interface module
> redundantly specified the same compatible strings for both variants and
> referenced symbols from the variants.
> 
> Now, each variant module has been transformed into a unified driver
> serving both as a platform and an MDIO driver. This modification
> reverses the relationship between the interface and variant modules,
> with the variant module now utilizing symbols from the interface
> modules.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@xxxxxxxxx>
> ---

Reviewed-by: Vladimir Oltean <olteanv@xxxxxxxxx>

Some minor non-functional comments below which you might decide not to
address now, depending on what else is pointed out during review.

> diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c
> index c2572463679f..3433f64fb0d7 100644
> --- a/drivers/net/dsa/realtek/realtek-mdio.c
> +++ b/drivers/net/dsa/realtek/realtek-mdio.c
> @@ -140,7 +141,19 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = {
>  	.disable_locking = true,
>  };
>  
> -static int realtek_mdio_probe(struct mdio_device *mdiodev)
> +/**
> + * realtek_mdio_probe() - Probe a platform device for an MDIO-connected switch
> + * @mdiodev: mdio_device to probe on.
> + *
> + * This function should be used as the .probe in an mdio_driver. It
> + * initializes realtek_priv and read data from the device-tree node. The switch
> + * is hard resetted if a method is provided. It checks the switch chip ID and,

nitpick: participle of 'reset' is 'reset'. Same comment for realtek_smi_probe().

> + * finally, a DSA switch is registered.
> + *
> + * Context: Can sleep. Takes and releases priv->map_lock.
> + * Return: Returns 0 on success, a negative error on failure.
> + */
> +int realtek_mdio_probe(struct mdio_device *mdiodev)
>  {
>  	struct realtek_priv *priv;
>  	struct device *dev = &mdiodev->dev;
> diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c
> index 668336515119..d8a9a6a6b5bc 100644
> --- a/drivers/net/dsa/realtek/realtek-smi.c
> +++ b/drivers/net/dsa/realtek/realtek-smi.c
> @@ -505,8 +518,20 @@ static int realtek_smi_probe(struct platform_device *pdev)
>  	}
>  	return 0;
>  }
> +EXPORT_SYMBOL_NS_GPL(realtek_smi_probe, REALTEK_DSA);
>  
> -static void realtek_smi_remove(struct platform_device *pdev)
> +/**
> + * realtek_smi_remove() - Remove the driver of a SMI-connected switch
> + * @pdev: platform_device to be removed.
> + *
> + * This function should be used as the .remove_new in a platform_driver. First
> + * it unregisters the DSA switch and cleans internal data. If a method is
> + * provided, the hard reset is asserted to avoid traffic leakage.

FWIW, removing the driver unregisters the net devices, which disables
the ports and performs an orderly shutdown of any upper virtual net
devices as well, like bridges. So ports automatically roll back to
standalone operation before this callback is even issued.

Traffic leakage is prevented, and the switch is hard reset, but I don't
think there is any causal relationship between the two.

> + *
> + * Context: Can sleep.
> + * Return: Nothing.
> + */
> +void realtek_smi_remove(struct platform_device *pdev)
>  {
>  	struct realtek_priv *priv = platform_get_drvdata(pdev);
>