From: Florian Fainelli <f.fainelli@gmail.com>
To: Luiz Angelo Daros de Luca <luizluca@gmail.com>, netdev@vger.kernel.org
Cc: linus.walleij@linaro.org, andrew@lunn.ch,
vivien.didelot@gmail.com, olteanv@gmail.com,
alsi@bang-olufsen.dk, arinc.unal@arinc9.com,
frank-w@public-files.de
Subject: Re: [PATCH net-next v4 06/11] net: dsa: realtek: add new mdio interface for drivers
Date: Sun, 16 Jan 2022 20:22:55 -0800 [thread overview]
Message-ID: <feaf03db-6fb9-d79f-0f51-bbedca739785@gmail.com> (raw)
In-Reply-To: <20220105031515.29276-7-luizluca@gmail.com>
On 1/4/2022 7:15 PM, Luiz Angelo Daros de Luca wrote:
> This driver is a mdio_driver instead of a platform driver (like
> realtek-smi).
>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> Tested-by: Arınç ÜNAL <arinc.unal@arinc9.com>
> ---
> drivers/net/dsa/realtek/Kconfig | 11 +-
> drivers/net/dsa/realtek/Makefile | 1 +
> drivers/net/dsa/realtek/realtek-mdio.c | 221 +++++++++++++++++++++++++
> drivers/net/dsa/realtek/realtek.h | 2 +
> 4 files changed, 233 insertions(+), 2 deletions(-)
> create mode 100644 drivers/net/dsa/realtek/realtek-mdio.c
>
> diff --git a/drivers/net/dsa/realtek/Kconfig b/drivers/net/dsa/realtek/Kconfig
> index cd1aa95b7bf0..73b26171fade 100644
> --- a/drivers/net/dsa/realtek/Kconfig
> +++ b/drivers/net/dsa/realtek/Kconfig
> @@ -9,6 +9,13 @@ menuconfig NET_DSA_REALTEK
> help
> Select to enable support for Realtek Ethernet switch chips.
>
> +config NET_DSA_REALTEK_MDIO
> + tristate "Realtek MDIO connected switch driver"
> + depends on NET_DSA_REALTEK
> + default y
I suppose this is fine since we depend on NET_DSA_REALTEK.
[snip]
> +static int realtek_mdio_read_reg(struct realtek_priv *priv, u32 addr, u32 *data)
> +{
> + u32 phy_id = priv->phy_id;
> + struct mii_bus *bus = priv->bus;
> +
> + mutex_lock(&bus->mdio_lock);
> +
> + bus->write(bus, phy_id, REALTEK_MDIO_CTRL0_REG, REALTEK_MDIO_ADDR_OP);
> + bus->write(bus, phy_id, REALTEK_MDIO_START_REG, REALTEK_MDIO_START_OP);
> + bus->write(bus, phy_id, REALTEK_MDIO_ADDRESS_REG, addr);
> + bus->write(bus, phy_id, REALTEK_MDIO_START_REG, REALTEK_MDIO_START_OP);
> + bus->write(bus, phy_id, REALTEK_MDIO_CTRL1_REG, REALTEK_MDIO_READ_OP);
> + bus->write(bus, phy_id, REALTEK_MDIO_START_REG, REALTEK_MDIO_START_OP);
> + *data = bus->read(bus, phy_id, REALTEK_MDIO_DATA_READ_REG);
Do you have no way to return an error for instance, if you read from a
non-existent PHY device on the MDIO bus, -EIO would be expected for
instance. If the data returned is 0xffff that ought to be enough.
> +
> + mutex_unlock(&bus->mdio_lock);
> +
> + return 0;
> +}
> +
> +static int realtek_mdio_write_reg(struct realtek_priv *priv, u32 addr, u32 data)
> +{
> + u32 phy_id = priv->phy_id;
> + struct mii_bus *bus = priv->bus;
> +
> + mutex_lock(&bus->mdio_lock);
> +
> + bus->write(bus, phy_id, REALTEK_MDIO_CTRL0_REG, REALTEK_MDIO_ADDR_OP);
> + bus->write(bus, phy_id, REALTEK_MDIO_START_REG, REALTEK_MDIO_START_OP);
> + bus->write(bus, phy_id, REALTEK_MDIO_ADDRESS_REG, addr);
This repeats between read and writes, might be worth a helper function.
> + bus->write(bus, phy_id, REALTEK_MDIO_START_REG, REALTEK_MDIO_START_OP);
> + bus->write(bus, phy_id, REALTEK_MDIO_DATA_WRITE_REG, data);
> + bus->write(bus, phy_id, REALTEK_MDIO_START_REG, REALTEK_MDIO_START_OP);
> + bus->write(bus, phy_id, REALTEK_MDIO_CTRL1_REG, REALTEK_MDIO_WRITE_OP);
> +
> + mutex_unlock(&bus->mdio_lock);
> +
> + return 0;
> +}
> +
> +/* Regmap accessors */
> +
> +static int realtek_mdio_write(void *ctx, u32 reg, u32 val)
> +{
> + struct realtek_priv *priv = ctx;
> +
> + return realtek_mdio_write_reg(priv, reg, val);
> +}
> +
> +static int realtek_mdio_read(void *ctx, u32 reg, u32 *val)
> +{
> + struct realtek_priv *priv = ctx;
> +
> + return realtek_mdio_read_reg(priv, reg, val);
> +}
Do you see a value for this function as oppposed to inlining the bodies
of realtek_mdio_read_reg and realtek_mdio_write_reg directly into these
two functions?
> +
> +static const struct regmap_config realtek_mdio_regmap_config = {
> + .reg_bits = 10, /* A4..A0 R4..R0 */
> + .val_bits = 16,
> + .reg_stride = 1,
> + /* PHY regs are at 0x8000 */
> + .max_register = 0xffff,
> + .reg_format_endian = REGMAP_ENDIAN_BIG,
> + .reg_read = realtek_mdio_read,
> + .reg_write = realtek_mdio_write,
> + .cache_type = REGCACHE_NONE,
> +};
> +
> +static int realtek_mdio_probe(struct mdio_device *mdiodev)
> +{
> + struct realtek_priv *priv;
> + struct device *dev = &mdiodev->dev;
> + const struct realtek_variant *var;
> + int ret;
> + struct device_node *np;
> +
> + var = of_device_get_match_data(dev);
Don't you have to check that var is non-NULL just in case?
> + priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->map = devm_regmap_init(dev, NULL, priv, &realtek_mdio_regmap_config);
> + if (IS_ERR(priv->map)) {
> + ret = PTR_ERR(priv->map);
> + dev_err(dev, "regmap init failed: %d\n", ret);
> + return ret;
> + }
> +
> + priv->phy_id = mdiodev->addr;
Please use a more descriptive variable name such as mdio_addr or
something like that. I know that phy_id is typically used but it could
also mean a 32-bit PHY unique identifier, which a MDIO device does not
have typically.
Looks fine otherwise.
--
Florian
next prev parent reply other threads:[~2022-01-17 4:25 UTC|newest]
Thread overview: 88+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-05 3:15 [PATCH net-next v4 00/11] net: dsa: realtek: MDIO interface and RTL8367S Luiz Angelo Daros de Luca
2022-01-05 3:15 ` [PATCH net-next v4 01/11] net: dsa: realtek-smi: move to subdirectory Luiz Angelo Daros de Luca
2022-01-05 3:15 ` [PATCH net-next v4 02/11] net: dsa: realtek: rename realtek_smi to realtek_priv Luiz Angelo Daros de Luca
2022-01-07 3:42 ` Jakub Kicinski
2022-01-10 12:33 ` Alvin Šipraga
2022-01-16 0:04 ` Linus Walleij
2022-01-20 14:37 ` Vladimir Oltean
2022-01-05 3:15 ` [PATCH net-next v4 03/11] net: dsa: realtek: remove direct calls to realtek-smi Luiz Angelo Daros de Luca
2022-01-10 12:38 ` Alvin Šipraga
2022-01-16 0:05 ` Linus Walleij
2022-01-17 3:46 ` Florian Fainelli
2022-01-05 3:15 ` [PATCH net-next v4 04/11] net: dsa: realtek: convert subdrivers into modules Luiz Angelo Daros de Luca
2022-01-10 12:43 ` Alvin Šipraga
2022-01-17 4:02 ` Florian Fainelli
2022-01-05 3:15 ` [PATCH net-next v4 05/11] net: dsa: realtek: use phy_read in ds->ops Luiz Angelo Daros de Luca
2022-01-10 13:09 ` Alvin Šipraga
2022-01-17 4:15 ` Florian Fainelli
2022-01-18 2:55 ` Luiz Angelo Daros de Luca
2022-01-18 13:16 ` Andrew Lunn
2022-01-21 22:13 ` Luiz Angelo Daros de Luca
2022-01-21 23:48 ` Andrew Lunn
2022-01-05 3:15 ` [PATCH net-next v4 06/11] net: dsa: realtek: add new mdio interface for drivers Luiz Angelo Daros de Luca
2022-01-10 13:09 ` Alvin Šipraga
2022-01-17 4:22 ` Florian Fainelli [this message]
2022-01-18 4:38 ` Luiz Angelo Daros de Luca
2022-01-05 3:15 ` [PATCH net-next v4 07/11] net: dsa: realtek: rtl8365mb: rename extport to extint, add "realtek,ext-int" Luiz Angelo Daros de Luca
2022-01-10 13:15 ` Alvin Šipraga
2022-01-17 4:25 ` Florian Fainelli
2022-01-05 3:15 ` [PATCH net-next v4 08/11] net: dsa: realtek: rtl8365mb: use GENMASK(n-1,0) instead of BIT(n)-1 Luiz Angelo Daros de Luca
2022-01-10 13:18 ` Alvin Šipraga
2022-01-17 4:25 ` Florian Fainelli
2022-01-05 3:15 ` [PATCH net-next v4 09/11] net: dsa: realtek: rtl8365mb: use DSA CPU port Luiz Angelo Daros de Luca
2022-01-07 3:37 ` Jakub Kicinski
2022-01-10 13:22 ` Alvin Šipraga
2022-01-17 4:26 ` Florian Fainelli
2022-01-05 3:15 ` [PATCH net-next v4 10/11] net: dsa: realtek: rtl8365mb: add RTL8367S support Luiz Angelo Daros de Luca
2022-01-10 13:26 ` Alvin Šipraga
2022-01-17 4:26 ` Florian Fainelli
2022-01-05 3:15 ` [PATCH net-next v4 11/11] net: dsa: realtek: rtl8365mb: multiple cpu ports, non cpu extint Luiz Angelo Daros de Luca
2022-01-10 13:39 ` Alvin Šipraga
2022-01-10 13:53 ` Aw: " Frank Wunderlich
2022-01-11 18:17 ` Alvin Šipraga
2022-01-11 18:45 ` Aw: " Frank Wunderlich
2022-01-13 12:37 ` Alvin Šipraga
2022-01-13 15:56 ` Aw: " Frank Wunderlich
2022-01-18 4:58 ` Luiz Angelo Daros de Luca
2022-01-18 10:13 ` Alvin Šipraga
2022-01-18 13:20 ` Re: Re: " Andrew Lunn
2022-01-20 15:12 ` Vladimir Oltean
2022-01-20 23:35 ` Luiz Angelo Daros de Luca
2022-01-21 2:06 ` Vladimir Oltean
2022-01-21 3:13 ` Luiz Angelo Daros de Luca
2022-01-21 3:22 ` Florian Fainelli
2022-01-21 3:42 ` Luiz Angelo Daros de Luca
2022-01-21 3:50 ` Florian Fainelli
2022-01-21 4:37 ` Luiz Angelo Daros de Luca
2022-01-21 9:07 ` Arınç ÜNAL
2022-01-21 18:50 ` Vladimir Oltean
2022-01-21 21:51 ` Luiz Angelo Daros de Luca
2022-01-21 22:49 ` Vladimir Oltean
2022-01-22 20:12 ` Luiz Angelo Daros de Luca
2022-01-24 15:31 ` Vladimir Oltean
2022-01-24 16:46 ` Jakub Kicinski
2022-01-24 16:55 ` Vladimir Oltean
2022-01-24 17:01 ` Florian Fainelli
2022-01-24 17:21 ` Vladimir Oltean
2022-01-24 17:30 ` Florian Fainelli
2022-01-24 17:35 ` Jakub Kicinski
2022-01-24 18:20 ` Jakub Kicinski
2022-01-24 19:08 ` Vladimir Oltean
2022-01-24 19:38 ` Jakub Kicinski
2022-01-24 20:56 ` Vladimir Oltean
2022-01-24 21:42 ` Jakub Kicinski
2022-01-24 22:30 ` Vladimir Oltean
2022-01-25 7:15 ` Luiz Angelo Daros de Luca
2022-01-25 9:47 ` Vladimir Oltean
2022-01-25 22:29 ` Luiz Angelo Daros de Luca
2022-01-25 23:56 ` Florian Fainelli
2022-01-26 22:49 ` Luiz Angelo Daros de Luca
2022-01-25 9:44 ` Arınç ÜNAL
2022-01-22 15:51 ` Andrew Lunn
2022-01-30 1:54 ` Re: Re: " Luiz Angelo Daros de Luca
2022-01-30 4:42 ` Luiz Angelo Daros de Luca
2022-01-30 17:24 ` Florian Fainelli
2022-01-31 17:26 ` Luiz Angelo Daros de Luca
2022-02-01 14:46 ` Vladimir Oltean
2022-01-20 14:36 ` [PATCH net-next v4 00/11] net: dsa: realtek: MDIO interface and RTL8367S Vladimir Oltean
2022-01-20 17:46 ` Luiz Angelo Daros de Luca
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=feaf03db-6fb9-d79f-0f51-bbedca739785@gmail.com \
--to=f.fainelli@gmail.com \
--cc=alsi@bang-olufsen.dk \
--cc=andrew@lunn.ch \
--cc=arinc.unal@arinc9.com \
--cc=frank-w@public-files.de \
--cc=linus.walleij@linaro.org \
--cc=luizluca@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=olteanv@gmail.com \
--cc=vivien.didelot@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).