Re: [PATCH net-next 3/3] net: mdio: realtek-rtl9300: Support non-default clock frequency
From: sashiko-bot@kernel.org
Date: 2026-09-04 12:22:03
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] Using an uninitialized `bus->dev` pointer for logging before the MDIO bus is registered, resulting in a '(null)' prefix. - [Low] The `global_freq` variable is overloaded as both an initialization flag and frequency storage, failing if `clock-frequency` is explicitly set to 0. -- commit 9b57074cdf8cafdf9765568d20f29655308463cf Author: Markus Stockhausen [off-list ref] net: mdio: realtek-rtl9300: Support non-default clock frequency This commit enhances the realtek-rtl9300 MDIO driver to allow setting the MDC clock frequency via device tree. For the RTL839x series, which only supports global frequency selection, the driver configures the global speed based on the first probed bus and issues warnings for conflicts.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c --- a/drivers/net/mdio/mdio-realtek-rtl9300.c +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -749,6 +766,34 @@ static int otto_emdio_8380_setup_controller(struct otto_emdio_priv *priv) return regmap_set_bits(priv->regmap, RTL8380_SMI_GLB_CTRL, RTL8380_SMI_PHY_PATCH_DONE); } +static int otto_emdio_8390_set_bus_frequency(struct mii_bus *bus, u32 freq) +{ + u32 val = FIELD_PREP(RTL8390_SMI_FREQ_SEL, min_t(u32, freq / 2500000, 2U)); + struct otto_emdio_priv *priv = otto_emdio_bus_to_priv(bus); + + if (priv->global_freq) {
[Severity: Low] Does this logic correctly handle a scenario where the device tree explicitly sets the clock-frequency to 0? Since priv->global_freq is used as both an initialization flag and for storing the frequency, a frequency of 0 would evaluate to false here. This could bypass the conflict detection for subsequent buses and silently overwrite the global hardware frequency. Would a dedicated initialization flag or sentinel value (like -1) be more robust to handle all integer inputs?
+ /* With only one global frequency available skip setup for second bus. */ + if (freq != priv->global_freq) + dev_warn(&bus->dev, "Only one frequency for all buses supported");
[Severity: Medium] Will this dev_warn() print a (null) device name prefix? Looking at otto_emdio_probe_one(), set_bus_frequency() is called before the bus is registered via devm_of_mdiobus_register(): err = priv->info->set_bus_frequency(bus, freq); if (err) return dev_err_probe(...); err = devm_of_mdiobus_register(dev, bus, node); Because dev_set_name() hasn't been called on bus->dev yet, the logger won't have a valid name to display. Could this be resolved by logging with bus->parent instead?
+ return 0; + } + priv->global_freq = freq; + + return regmap_update_bits(priv->regmap, RTL8390_SMI_GLB_CTRL, RTL8390_SMI_FREQ_SEL, val); +}
-- Sashiko AI review · https://sashiko.dev/#/patchset/20260903122119.2638354-1-markus.stockhausen@gmx.de?part=3