Received-SPF: pass (google.com: domain of linux-kernel+bounces-48691-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 1 Feb 2024 20:23:07 +0200
From: Vladimir Oltean <olteanv@gmail.com>
To: Luiz Angelo Daros de Luca <luizluca@gmail.com>
Cc: Linus Walleij <linus.walleij@linaro.org>,
	Alvin =?utf-8?Q?=C5=A0ipraga?= <alsi@bang-olufsen.dk>,
	Andrew Lunn <andrew@lunn.ch>,
	Florian Fainelli <f.fainelli@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Russell King <linux@armlinux.org.uk>, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v5 05/11] net: dsa: realtek: common rtl83xx
 module
Message-ID: <20240201182307.mls7qnrpu2zhh5xk@skbuf>
References: <20240130-realtek_reverse-v5-0-ecafd9283a07@gmail.com>
 <20240130-realtek_reverse-v5-0-ecafd9283a07@gmail.com>
 <20240130-realtek_reverse-v5-5-ecafd9283a07@gmail.com>
 <20240130-realtek_reverse-v5-5-ecafd9283a07@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240130-realtek_reverse-v5-5-ecafd9283a07@gmail.com>
 <20240130-realtek_reverse-v5-5-ecafd9283a07@gmail.com>

On Tue, Jan 30, 2024 at 08:13:24PM -0300, Luiz Angelo Daros de Luca wrote:
>  /**
>   * realtek_smi_probe() - Probe a platform device for an SMI-connected switch
>   * @pdev: platform_device to probe on.
>   *
> - * This function should be used as the .probe in a platform_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,
> - * finally, a DSA switch is registered.
> + * This function should be used as the .probe in a platform_driver. After
> + * calling the common probe function for both interfaces, it initializes the
> + * values specific for SMI-connected devices. Finally, it calls a common
> + * function to register the DSA switch.
>   *
>   * Context: Can sleep. Takes and releases priv->map_lock.
>   * Return: Returns 0 on success, a negative error on failure.
>   */
>  int realtek_smi_probe(struct platform_device *pdev)
>  {
> -	const struct realtek_variant *var;
>  	struct device *dev = &pdev->dev;
>  	struct realtek_priv *priv;
> -	struct regmap_config rc;
> -	struct device_node *np;
>  	int ret;
>  
> -	var = of_device_get_match_data(dev);
> -	np = dev->of_node;
> -
> -	priv = devm_kzalloc(dev, sizeof(*priv) + var->chip_data_sz, GFP_KERNEL);
> -	if (!priv)
> -		return -ENOMEM;
> -	priv->chip_data = (void *)priv + sizeof(*priv);
> -
> -	mutex_init(&priv->map_lock);
> -
> -	rc = realtek_smi_regmap_config;
> -	rc.lock_arg = priv;
> -	priv->map = devm_regmap_init(dev, NULL, priv, &rc);
> -	if (IS_ERR(priv->map)) {
> -		ret = PTR_ERR(priv->map);
> -		dev_err(dev, "regmap init failed: %d\n", ret);
> -		return ret;
> -	}
> -
> -	rc = realtek_smi_nolock_regmap_config;
> -	priv->map_nolock = devm_regmap_init(dev, NULL, priv, &rc);
> -	if (IS_ERR(priv->map_nolock)) {
> -		ret = PTR_ERR(priv->map_nolock);
> -		dev_err(dev, "regmap init failed: %d\n", ret);
> -		return ret;
> -	}
> -
> -	/* Link forward and backward */
> -	priv->dev = dev;
> -	priv->variant = var;
> -	priv->ops = var->ops;
> -
> -	priv->setup_interface = realtek_smi_setup_mdio;
> -	priv->write_reg_noack = realtek_smi_write_reg_noack;
> -
> -	dev_set_drvdata(dev, priv);
> -	spin_lock_init(&priv->lock);
> -
> -	/* TODO: if power is software controlled, set up any regulators here */
> -
> -	priv->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW);
> -	if (IS_ERR(priv->reset)) {
> -		dev_err(dev, "failed to get RESET GPIO\n");
> -		return PTR_ERR(priv->reset);
> -	}
> -	if (priv->reset) {
> -		gpiod_set_value(priv->reset, 1);
> -		dev_dbg(dev, "asserted RESET\n");
> -		msleep(REALTEK_HW_STOP_DELAY);
> -		gpiod_set_value(priv->reset, 0);
> -		msleep(REALTEK_HW_START_DELAY);
> -		dev_dbg(dev, "deasserted RESET\n");
> -	}
> +	priv = rtl83xx_probe(dev, &realtek_smi_info);
> +	if (IS_ERR(priv))
> +		return PTR_ERR(priv);
>  
>  	/* Fetch MDIO pins */
>  	priv->mdc = devm_gpiod_get_optional(dev, "mdc", GPIOD_OUT_LOW);
>  	if (IS_ERR(priv->mdc))
>  		return PTR_ERR(priv->mdc);

Be consistent in the usage of the API you create. Every time rtl83xx_probe()
succeeds and something else down the line fails, you must call
rtl83xx_remove(). Otherwise, it is a shaky base to build upon.

The rtl83xx_remove() function can even be left empty if you feel that
the existing hard reset is of no value (which I would agree with) - see
mv88e6xxx_hwtstamp_free() as an example.

> +
>  	priv->mdio = devm_gpiod_get_optional(dev, "mdio", GPIOD_OUT_LOW);
>  	if (IS_ERR(priv->mdio))
>  		return PTR_ERR(priv->mdio);
>  
> -	priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds");
> +	priv->write_reg_noack = realtek_smi_write_reg_noack;
> +	priv->setup_interface = realtek_smi_setup_mdio;
> +	priv->ds_ops = priv->variant->ds_ops_smi;
>  
> -	ret = priv->ops->detect(priv);
> -	if (ret) {
> -		dev_err(dev, "unable to detect switch\n");
> +	ret = rtl83xx_register_switch(priv);
> +	if (ret)
>  		return ret;
> -	}
> -
> -	priv->ds = devm_kzalloc(dev, sizeof(*priv->ds), GFP_KERNEL);
> -	if (!priv->ds)
> -		return -ENOMEM;
>  
> -	priv->ds->dev = dev;
> -	priv->ds->num_ports = priv->num_ports;
> -	priv->ds->priv = priv;
> -
> -	priv->ds->ops = var->ds_ops_smi;
> -	ret = dsa_register_switch(priv->ds);
> -	if (ret) {
> -		dev_err_probe(dev, ret, "unable to register switch\n");
> -		return ret;
> -	}
>  	return 0;
>  }
>  EXPORT_SYMBOL_NS_GPL(realtek_smi_probe, REALTEK_DSA);