Re: [PATCH net-next v2 3/3] net: dsa: realtek: support reset controller
From: Vladimir Oltean <olteanv@gmail.com>
Date: 2023-10-30 20:50:31
On Fri, Oct 27, 2023 at 04:00:57PM -0300, Luiz Angelo Daros de Luca wrote:
quoted hunk ↗ jump to hunk
The 'reset-gpios' will not work when the switch reset is controlled by a reset controller. Although the reset is optional and the driver performs a soft reset during setup, if the initial reset state was asserted, the driver will not detect it. The reset controller will take precedence over the reset GPIO. Signed-off-by: Luiz Angelo Daros de Luca <redacted> --- drivers/net/dsa/realtek/realtek-mdio.c | 51 ++++++++++++++++++++++---- drivers/net/dsa/realtek/realtek-smi.c | 49 ++++++++++++++++++++++--- drivers/net/dsa/realtek/realtek.h | 2 + 3 files changed, 89 insertions(+), 13 deletions(-)diff --git a/drivers/net/dsa/realtek/realtek-mdio.c b/drivers/net/dsa/realtek/realtek-mdio.c index 292e6d087e8b..aad94e49d4c9 100644 --- a/drivers/net/dsa/realtek/realtek-mdio.c +++ b/drivers/net/dsa/realtek/realtek-mdio.c@@ -140,6 +140,40 @@ static const struct regmap_config realtek_mdio_nolock_regmap_config = { .disable_locking = true, }; +static void realtek_mdio_reset_assert(struct realtek_priv *priv) +{ + int ret; + + if (priv->reset_ctl) { + ret = reset_control_assert(priv->reset_ctl); + if (ret) + dev_warn(priv->dev, "Failed to assert the switch reset control. Error: %i", + ret);
Instead of "Error: %i" you can say ".. reset control: %pe\n", ERR_PTR(ret)
which will print the error as a symbolic error name (if CONFIG_SYMBOLIC_ERRNAME=y)
rather than just a numeric value.
Also, I don't know if this is explicit in the coding style, but I
believe it is more consistent if single function calls are enveloped in
curly braces if they span multiple lines, like so:
if (ret) {
dev_warn(priv->dev,
"Failed to assert the switch reset control: %pe",
ERR_PTR(ret));
}
Also, please note that netdev still prefers the 80 character line limit.
+
+ return;
+ }
+
+ if (priv->reset)
+ gpiod_set_value(priv->reset, true);
+}
+
+static void realtek_mdio_reset_deassert(struct realtek_priv *priv)
+{
+ int ret;
+
+ if (priv->reset_ctl) {
+ ret = reset_control_deassert(priv->reset_ctl);
+ if (ret)
+ dev_warn(priv->dev, "Failed to deassert the switch reset control. Error: %i",
+ ret);
+
+ return;Is there a particular reason why this has to ignore a reset GPIO if present, rather than fall through, checking for the latter as well?
quoted hunk ↗ jump to hunk
+ } + + if (priv->reset) + gpiod_set_value(priv->reset, false); +} + static int realtek_mdio_probe(struct mdio_device *mdiodev) { struct realtek_priv *priv;@@ -194,20 +228,24 @@ static int realtek_mdio_probe(struct mdio_device *mdiodev) dev_set_drvdata(dev, priv); - /* TODO: if power is software controlled, set up any regulators here */
As Andrew mentions, this commit does not make power software-controlled, so don't remove this.
quoted hunk ↗ jump to hunk
priv->leds_disabled = of_property_read_bool(np, "realtek,disable-leds"); + priv->reset_ctl = devm_reset_control_get_optional(dev, NULL); + if (IS_ERR(priv->reset_ctl)) { + ret = PTR_ERR(priv->reset_ctl); + return dev_err_probe(dev, ret, "failed to get reset control\n"); + } + 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); + if (priv->reset_ctl || priv->reset) { + realtek_mdio_reset_assert(priv); dev_dbg(dev, "asserted RESET\n"); msleep(REALTEK_HW_STOP_DELAY); - gpiod_set_value(priv->reset, 0); + realtek_mdio_reset_deassert(priv); msleep(REALTEK_HW_START_DELAY); dev_dbg(dev, "deasserted RESET\n"); }@@ -246,8 +284,7 @@ static void realtek_mdio_remove(struct mdio_device *mdiodev) dsa_unregister_switch(priv->ds); /* leave the device reset asserted */ - if (priv->reset) - gpiod_set_value(priv->reset, 1); + realtek_mdio_reset_assert(priv); } static void realtek_mdio_shutdown(struct mdio_device *mdiodev)diff --git a/drivers/net/dsa/realtek/realtek-smi.c b/drivers/net/dsa/realtek/realtek-smi.c index bfd11591faf4..a99e53b5b662 100644 --- a/drivers/net/dsa/realtek/realtek-smi.c +++ b/drivers/net/dsa/realtek/realtek-smi.c@@ -408,6 +408,40 @@ static int realtek_smi_setup_mdio(struct dsa_switch *ds) return ret; } +static void realtek_smi_reset_assert(struct realtek_priv *priv) +{ + int ret; + + if (priv->reset_ctl) { + ret = reset_control_assert(priv->reset_ctl); + if (ret) + dev_warn(priv->dev, "Failed to assert the switch reset control. Error: %i", + ret); + + return; + } + + if (priv->reset) + gpiod_set_value(priv->reset, true); +} + +static void realtek_smi_reset_deassert(struct realtek_priv *priv) +{ + int ret; + + if (priv->reset_ctl) { + ret = reset_control_deassert(priv->reset_ctl); + if (ret) + dev_warn(priv->dev, "Failed to deassert the switch reset control. Error: %i", + ret); + + return; + } + + if (priv->reset) + gpiod_set_value(priv->reset, false); +} +
To respond here, in a single email, to your earlier question (sorry): https://lore.kernel.org/netdev/CAJq09z7miTe7HUzsL4GBSwkrzyy4mVi6z40+ETgvmY=iWGRN-g@mail.gmail.com/ (local) | Both interface modules, realtek-smi and realtek-mdio, do not share | code, except for the realtek.h header file. I don't know if it is | worth it to put the code in a new shared module. What is the best | practice here? Create a realtek_common.c linked to both modules? The answer is: I ran "meld" between realtek-mdio.c and realtek-smi.c, and the probe, remove and shutdown functions are surprisingly similar already, and perhaps might become even more similar in the future. I think it is worth introducing a common kernel module for both interface drivers as a preliminary patch, rather than keeping duplicated probe/remove/shutdown code.
quoted hunk ↗ jump to hunk
static int realtek_smi_probe(struct platform_device *pdev) { const struct realtek_variant *var;@@ -457,18 +491,22 @@ static int realtek_smi_probe(struct platform_device *pdev) dev_set_drvdata(dev, priv); spin_lock_init(&priv->lock); - /* TODO: if power is software controlled, set up any regulators here */ + priv->reset_ctl = devm_reset_control_get_optional(dev, NULL); + if (IS_ERR(priv->reset_ctl)) { + ret = PTR_ERR(priv->reset_ctl); + return dev_err_probe(dev, ret, "failed to get reset control\n"); + } 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); + if (priv->reset_ctl || priv->reset) { + realtek_smi_reset_assert(priv); dev_dbg(dev, "asserted RESET\n"); msleep(REALTEK_HW_STOP_DELAY); - gpiod_set_value(priv->reset, 0); + realtek_smi_reset_deassert(priv); msleep(REALTEK_HW_START_DELAY); dev_dbg(dev, "deasserted RESET\n"); }@@ -518,8 +556,7 @@ static void realtek_smi_remove(struct platform_device *pdev) of_node_put(priv->slave_mii_bus->dev.of_node);
slave_mii_bus was renamed to user_mii_bus, and this prevents the application of the patch currently, so you will need to respin. But I think net-next is going to close soon for 2 weeks, so either you respin as RFC or you wait until it reopens.
/* leave the device reset asserted */ - if (priv->reset) - gpiod_set_value(priv->reset, 1); + realtek_smi_reset_assert(priv); }