Messages in this thread | | | From | Tobias Waldekranz <> | Subject | Re: [PATCH net-next 4/5] net/fsl: xgmac_mdio: Support setting the MDC frequency | Date | Wed, 26 Jan 2022 14:30:28 +0100 |
| |
On Wed, Jan 26, 2022 at 14:07, Andrew Lunn <andrew@lunn.ch> wrote: > Hi Tobias > >> struct mdio_fsl_priv { >> struct tgec_mdio_controller __iomem *mdio_base; >> + struct clk *enet_clk; > > It looks like there is a whitespace issue here? >
Ahh, sorry about that!
>> + u32 mdc_freq; >> bool is_little_endian; >> bool has_a009885; >> bool has_a011043; >> @@ -255,6 +258,34 @@ static int xgmac_mdio_read(struct mii_bus *bus, int phy_id, int regnum) >> return ret; >> } >> >> +static void xgmac_mdio_set_mdc_freq(struct mii_bus *bus) >> +{ >> + struct mdio_fsl_priv *priv = (struct mdio_fsl_priv *)bus->priv; >> + struct tgec_mdio_controller __iomem *regs = priv->mdio_base; >> + struct device *dev = bus->parent; >> + u32 mdio_stat, div; >> + >> + if (device_property_read_u32(dev, "clock-frequency", &priv->mdc_freq)) >> + return; >> + >> + priv->enet_clk = devm_clk_get(dev, NULL); >> + if (IS_ERR(priv->enet_clk)) { >> + dev_err(dev, "Input clock unknown, not changing MDC frequency"); > > Is the clock optional or mandatory?
As the code is now, it is mandatory. I could add some default frequency, but I fear that could do more harm than good?
> If mandatory, then i would prefer -ENODEV and fail the probe. > >> + return; >> + } >> + >> + div = ((clk_get_rate(priv->enet_clk) / priv->mdc_freq) - 1) / 2; >> + if (div < 5 || div > 0x1ff) { >> + dev_err(dev, "Requested MDC frequecy is out of range, ignoring"); > > and here return -EINVAL. > > I always think it is best to make it obvious something is broken. One > additional line on the console can be missed for a while. All the > Ethernet devices missing because the PHYs are missing, because the > MDIO bus is missing is going to get noticed very quickly.
Darn, the last thing I did was to change this from a hard to a soft error :)
Ok, no worries about regressions for deployed stuff already out there?
| |