[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4ltsthrk2oli6ickjiy6uy3pc3kpdddse7lab34qefbadjafhy@oaxoemtrhw3k>
Date: Fri, 8 Dec 2023 14:02:19 +0000
From: Alvin Šipraga <ALSI@...g-olufsen.dk>
To: Luiz Angelo Daros de Luca <luizluca@...il.com>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linus.walleij@...aro.org" <linus.walleij@...aro.org>, "andrew@...n.ch"
<andrew@...n.ch>, "f.fainelli@...il.com" <f.fainelli@...il.com>,
"olteanv@...il.com" <olteanv@...il.com>, "davem@...emloft.net"
<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
"arinc.unal@...nc9.com" <arinc.unal@...nc9.com>
Subject: Re: [PATCH net-next 4/7] net: dsa: realtek: create realtek-common
On Fri, Dec 08, 2023 at 01:41:40AM -0300, Luiz Angelo Daros de Luca wrote:
> +struct realtek_priv *
> +realtek_common_probe_pre(struct device *dev, struct regmap_config rc,
> + struct regmap_config rc_nolock)
> +{
<snip>
> +
> + /* 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 ERR_CAST(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");
> + }
Another thing I would like to suggest is that you do not move the
hardware reset and the /* TODO: regulators */ into the common code. I
actually wanted to add regulator support for rtl8365mb after you are
finished with your series, and I noticed that it will not fit well here,
because the supplies are different for the two switch variants.
If we were to do the hardware reset here in common_probe_pre(), where
should I put my variant-specific regulator_bulk_enable()? I can't put it
before _pre() because I do not have the private data allocated yet. If I
put it afterwards, then the above hardware reset toggle will have had no
effect.
So why not:
realtek_variant_probe() {
realtek_common_probe_pre();
/* enable regulators */
/* hardware reset */
/* other chip specific stuff here */
realtek_common_probe_post();
}
> +
> + return priv;
> +}
> +EXPORT_SYMBOL(realtek_common_probe_pre);
> +
> +int realtek_common_probe_post(struct realtek_priv *priv)
> +{
> + int ret;
> +
> + ret = priv->ops->detect(priv);
> + if (ret) {
> + dev_err(priv->dev, "unable to detect switch\n");
> + return ret;
> + }
This detect() logic can also then be folded into the variant's probe
function between _pre() and _post().
> +
> + priv->ds = devm_kzalloc(priv->dev, sizeof(*priv->ds), GFP_KERNEL);
> + if (!priv->ds)
> + return -ENOMEM;
> +
> + priv->ds->priv = priv;
> + priv->ds->dev = priv->dev;
> + priv->ds->ops = priv->ds_ops;
> + priv->ds->num_ports = priv->num_ports;
> +
> + ret = dsa_register_switch(priv->ds);
> + if (ret) {
> + dev_err_probe(priv->dev, ret, "unable to register switch\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(realtek_common_probe_post);
Powered by blists - more mailing lists