Re: [PATCH net-next v5 05/11] net: dsa: realtek: common rtl83xx module

Vladimir Oltean
2024-02-01T18:23:12Z

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);