From: Vladimir Oltean <olteanv@gmail.com>
To: Daniel Golle <daniel@makrotopia.org>
Cc: netdev@vger.kernel.org, linux-mediatek@lists.infradead.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org,
"Russell King" <linux@armlinux.org.uk>,
"Heiner Kallweit" <hkallweit1@gmail.com>,
"Lorenzo Bianconi" <lorenzo@kernel.org>,
"Mark Lee" <Mark-MC.Lee@mediatek.com>,
"John Crispin" <john@phrozen.org>, "Felix Fietkau" <nbd@nbd.name>,
"AngeloGioacchino Del Regno"
<angelogioacchino.delregno@collabora.com>,
"Matthias Brugger" <matthias.bgg@gmail.com>,
"DENG Qingfang" <dqfext@gmail.com>,
"Landen Chao" <Landen.Chao@mediatek.com>,
"Sean Wang" <sean.wang@mediatek.com>,
"Paolo Abeni" <pabeni@redhat.com>,
"Jakub Kicinski" <kuba@kernel.org>,
"Eric Dumazet" <edumazet@google.com>,
"David S. Miller" <davem@davemloft.net>,
"Florian Fainelli" <f.fainelli@gmail.com>,
"Andrew Lunn" <andrew@lunn.ch>,
"Jianhui Zhao" <zhaojh329@gmail.com>,
"Bjørn Mork" <bjorn@mork.no>
Subject: Re: [PATCH 9/9] net: dsa: mt7530: use external PCS driver
Date: Sat, 4 Feb 2023 00:19:15 +0200 [thread overview]
Message-ID: <20230203221915.tvg4rrjv5cnkshuf@skbuf> (raw)
In-Reply-To: <677a5e37aab97a4f992d35c41329733c5f3082fb.1675407169.git.daniel@makrotopia.org> <677a5e37aab97a4f992d35c41329733c5f3082fb.1675407169.git.daniel@makrotopia.org>
On Fri, Feb 03, 2023 at 07:06:53AM +0000, Daniel Golle wrote:
> @@ -2728,11 +2612,14 @@ mt753x_phylink_mac_select_pcs(struct dsa_switch *ds, int port,
>
> switch (interface) {
> case PHY_INTERFACE_MODE_TRGMII:
> + return &priv->pcs[port].pcs;
> case PHY_INTERFACE_MODE_SGMII:
> case PHY_INTERFACE_MODE_1000BASEX:
> case PHY_INTERFACE_MODE_2500BASEX:
> - return &priv->pcs[port].pcs;
> + if (!mt753x_is_mac_port(port))
> + return ERR_PTR(-EINVAL);
What is the reason for returning ERR_PTR(-EINVAL) to mac_select_pcs()?
>
> + return priv->sgmii_pcs[port - 5];
How about putting the pcs pointer in struct mt7530_port?
> default:
> return NULL;
> }
> @@ -3088,8 +2934,6 @@ mt753x_setup(struct dsa_switch *ds)
> priv->pcs[i].pcs.ops = priv->info->pcs_ops;
> priv->pcs[i].priv = priv;
> priv->pcs[i].port = i;
> - if (mt753x_is_mac_port(i))
> - priv->pcs[i].pcs.poll = 1;
> }
>
> ret = priv->info->sw_setup(ds);
> @@ -3104,6 +2948,15 @@ mt753x_setup(struct dsa_switch *ds)
> if (ret && priv->irq)
> mt7530_free_irq_common(priv);
You need to patch the previous code to "return ret".
>
> + if (priv->id == ID_MT7531)
if the code block below is multi-line (which it is), put braces here too
or can return early if priv->id != ID_MT7531, and this reduces the
indentation by one level.
> + for (i = 0; i < 2; ++i) {
could also iterate over all ports and ignore those which have
!mt753x_is_mac_port(port)
> + regmap = devm_regmap_init(ds->dev,
> + &mt7531_regmap_bus, priv,
> + &mt7531_pcs_config[i]);
can fail
> + priv->sgmii_pcs[i] = mtk_pcs_create(ds->dev, regmap,
> + 0x128, 0);
can fail
Don't forget to do error teardown which isn't leaky
0x128 comes from the old definition of MT7531_PHYA_CTRL_SIGNAL3(port),
so please keep a macro of some sorts to denote the offset of ana_rgc3
for MT7531, rather than just this obscure magic number.
> + }
> +
> return ret;
> }
>
> @@ -3199,7 +3052,7 @@ static const struct mt753x_info mt753x_table[] = {
> },
> [ID_MT7531] = {
> .id = ID_MT7531,
> - .pcs_ops = &mt7531_pcs_ops,
> + .pcs_ops = &mt7530_pcs_ops,
> .sw_setup = mt7531_setup,
> .phy_read_c22 = mt7531_ind_c22_phy_read,
> .phy_write_c22 = mt7531_ind_c22_phy_write,
> @@ -3309,7 +3162,7 @@ static void
> mt7530_remove(struct mdio_device *mdiodev)
> {
> struct mt7530_priv *priv = dev_get_drvdata(&mdiodev->dev);
> - int ret = 0;
> + int ret = 0, i;
>
> if (!priv)
> return;
> @@ -3328,6 +3181,11 @@ mt7530_remove(struct mdio_device *mdiodev)
> mt7530_free_irq(priv);
>
> dsa_unregister_switch(priv->ds);
> +
> + for (i = 0; i < 2; ++i)
There is no ++i in this driver and there are 11 i++, so please be
consistent with what exists.
> + if (priv->sgmii_pcs[i])
> + mtk_pcs_destroy(priv->sgmii_pcs[i]);
> +
> mutex_destroy(&priv->reg_mutex);
> }
>
next prev parent reply other threads:[~2023-02-03 22:19 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-03 6:58 [PATCH 0/9] net: ethernet: mtk_eth_soc: various enhancements Daniel Golle
2023-02-03 7:00 ` [PATCH 1/9] net: ethernet: mtk_eth_soc: add support for MT7981 SoC Daniel Golle
2023-02-03 14:00 ` Andrew Lunn
2023-02-03 14:18 ` Vladimir Oltean
2023-02-03 21:51 ` Vladimir Oltean
2023-02-03 7:01 ` [PATCH 2/9] net: ethernet: mtk_eth_soc: set MDIO bus clock frequency Daniel Golle
2023-02-03 14:06 ` Andrew Lunn
2023-02-03 21:48 ` Vladimir Oltean
2023-02-03 22:15 ` Andrew Lunn
2023-02-03 22:26 ` Vladimir Oltean
2023-02-03 7:01 ` [PATCH 3/9] net: ethernet: mtk_eth_soc: reset PCS state Daniel Golle
2023-02-03 7:02 ` [PATCH 4/9] net: ethernet: mtk_eth_soc: only write values if needed Daniel Golle
2023-02-03 14:08 ` Andrew Lunn
2023-02-03 7:02 ` [PATCH 5/9] net: ethernet: mtk_eth_soc: fix RX data corruption issue Daniel Golle
2023-02-03 14:09 ` Andrew Lunn
2023-02-03 7:05 ` [PATCH 6/9] net: ethernet: mtk_eth_soc: ppe: add support for flow accounting Daniel Golle
2023-02-03 22:55 ` Vladimir Oltean
2023-02-03 7:05 ` [PATCH 7/9] net: pcs: add driver for MediaTek SGMII PCS Daniel Golle
2023-02-03 14:14 ` Andrew Lunn
2023-02-03 15:00 ` Vladimir Oltean
2023-02-03 15:21 ` Andrew Lunn
2023-02-03 7:06 ` [PATCH 8/9] net: ethernet: mtk_eth_soc: switch to external PCS driver Daniel Golle
2023-02-03 9:25 ` Bjørn Mork
2023-02-03 21:56 ` Vladimir Oltean
2023-02-03 7:06 ` [PATCH 9/9] net: dsa: mt7530: use " Daniel Golle
2023-02-03 22:19 ` Vladimir Oltean [this message]
2023-02-04 15:02 ` Daniel Golle
2023-02-04 17:13 ` Andrew Lunn
2023-02-04 23:41 ` Russell King (Oracle)
2023-02-05 12:13 ` Vladimir Oltean
2023-02-04 11:08 ` [PATCH 0/9] net: ethernet: mtk_eth_soc: various enhancements Bjørn Mork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230203221915.tvg4rrjv5cnkshuf@skbuf \
--to=olteanv@gmail.com \
--cc=Landen.Chao@mediatek.com \
--cc=Mark-MC.Lee@mediatek.com \
--cc=andrew@lunn.ch \
--cc=angelogioacchino.delregno@collabora.com \
--cc=bjorn@mork.no \
--cc=daniel@makrotopia.org \
--cc=davem@davemloft.net \
--cc=dqfext@gmail.com \
--cc=edumazet@google.com \
--cc=f.fainelli@gmail.com \
--cc=hkallweit1@gmail.com \
--cc=john@phrozen.org \
--cc=kuba@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux@armlinux.org.uk \
--cc=lorenzo@kernel.org \
--cc=matthias.bgg@gmail.com \
--cc=nbd@nbd.name \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=sean.wang@mediatek.com \
--cc=zhaojh329@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).