Message-ID: <CAK+owogkvN+Y28YQ9X28QdLo6VXguR-6tY-10an_F02BMqFtew@mail.gmail.com>
Date: Mon, 19 May 2025 17:16:08 +0200
From: Stefano Radaelli <stefano.radaelli21@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Heiner Kallweit <hkallweit1@...il.com>, Russell King <linux@...linux.org.uk>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Xu Liang <lxu@...linear.com>
Subject: Re: [PATCH net-next v2] net: phy: add driver for MaxLinear MxL86110 PHY

Hi Andrew,

Thank you for your patience and valuable feedback.
I'm learning a lot from your reviews and this process overall.

I'll send v3 shortly with the changes you pointed out.

Best regards,

Stefano


Il giorno lun 19 mag 2025 alle ore 14:35 Andrew Lunn <andrew@...n.ch>
ha scritto:
>
> > +static int mxl86110_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
> > +{
> > +     struct net_device *netdev;
> > +     const u8 *mac;
> > +     int ret = 0;
> > +
> > +     phy_lock_mdio_bus(phydev);
> > +
> > +     if (wol->wolopts & WAKE_MAGIC) {
> > +             netdev = phydev->attached_dev;
> > +             if (!netdev) {
> > +                     ret = -ENODEV;
> > +                     goto error;
> > +             }
>
> ...
>
> > +
> > +     phy_unlock_mdio_bus(phydev);
> > +     return 0;
> > +error:
> > +     phy_unlock_mdio_bus(phydev);
> > +     return ret;
> > +}
>
> You should be able to simplify this. If you have not had an error, ret
> should be 0. So you can also return ret. You have the same pattern in
> other places.
>
> > +/**
> > + * mxl86110_synce_clk_cfg() - applies syncE/clk output configuration
> > + * @phydev: pointer to the phy_device
> > + *
> > + * Custom settings can be defined in custom config section of the driver
> > + * returns 0 or negative errno code
> > + */
>
> Maybe add a comment that the bus is expected to be locked.
>
> > +static int mxl86110_synce_clk_cfg(struct phy_device *phydev)
> > +{
> > +     u16 mask = 0, value = 0;
> > +     int ret = 0;
> > +
> > +     /*
> > +      * Configures the clock output to its default setting as per the datasheet.
> > +      * This results in a 25MHz clock output being selected in the
> > +      * COM_EXT_SYNCE_CFG register for SyncE configuration.
> > +      */
> > +     value = MXL86110_EXT_SYNCE_CFG_EN_SYNC_E |
> > +                     FIELD_PREP(MXL86110_EXT_SYNCE_CFG_CLK_SRC_SEL_MASK,
> > +                                MXL86110_EXT_SYNCE_CFG_CLK_SRC_SEL_25M);
> > +     mask = MXL86110_EXT_SYNCE_CFG_EN_SYNC_E |
> > +            MXL86110_EXT_SYNCE_CFG_CLK_SRC_SEL_MASK |
> > +            MXL86110_EXT_SYNCE_CFG_CLK_FRE_SEL;
> > +
> > +     /* Write clock output configuration */
> > +     ret = mxl86110_modify_extended_reg(phydev, MXL86110_EXT_SYNCE_CFG_REG,
> > +                                        mask, value);
> > +
> > +     return ret;
> > +}
> > +
> > +/**
> > + * mxl86110_broadcast_cfg - Configure MDIO broadcast setting for PHY
> > + * @phydev: Pointer to the PHY device structure
> > + *
> > + * This function configures the MDIO broadcast behavior of the MxL86110 PHY.
> > + * Currently, broadcast mode is explicitly disabled by clearing the EPA0 bit
> > + * in the RGMII_MDIO_CFG extended register.
>
> here as well.
>
> > + *
> > + * Return: 0 on success or a negative errno code on failure.
> > + */
> > +static int mxl86110_broadcast_cfg(struct phy_device *phydev)
> > +{
> > +     int ret = 0;
> > +     u16 val;
> > +
> > +     val = mxl86110_read_extended_reg(phydev, MXL86110_EXT_RGMII_MDIO_CFG);
> > +     if (val < 0)
> > +             return val;
> > +
> > +     val &= ~MXL86110_EXT_RGMII_MDIO_CFG_EPA0_MASK;
> > +     ret = mxl86110_write_extended_reg(phydev, MXL86110_EXT_RGMII_MDIO_CFG, val);
>
> Could _modify_ be used here?
>
> > +
> > +     return ret;
> > +}
> > +
> > +/**
> > + * mxl86110_enable_led_activity_blink - Enable LEDs activity blink on PHY
> > + * @phydev: Pointer to the PHY device structure
> > + *
> > + * Configure all PHY LEDs to blink on traffic activity regardless of their
> > + * ON or OFF state. This behavior allows each LED to serve as a pure activity
> > + * indicator, independently of its use as a link status indicator.
> > + *
> > + * By default, each LED blinks only when it is also in the ON state. This function
> > + * modifies the appropriate registers (LABx fields) to enable blinking even
> > + * when the LEDs are OFF, to allow the LED to be used as a traffic indicator
> > + * without requiring it to also serve as a link status LED.
> > + *
> > + * NOTE: Any further LED customization can be performed via the
> > + * /sys/class/led interface; the functions led_hw_is_supported, led_hw_control_get, and
> > + * led_hw_control_set are used to support this mechanism.
> > + *
> > + * Return: 0 on success or a negative errno code on failure.
> > + */
> > +static int mxl86110_enable_led_activity_blink(struct phy_device *phydev)
> > +{
> > +     int ret, index;
> > +     u16 val = 0;
> > +
> > +     for (index = 0; index < MXL86110_MAX_LEDS; index++) {
> > +             val = mxl86110_read_extended_reg(phydev, MXL86110_LED0_CFG_REG + index);
> > +             if (val < 0)
> > +                     return val;
> > +
> > +             val |= MXL86110_LEDX_CFG_TRAFFIC_ACT_BLINK_IND;
> > +             ret = mxl86110_write_extended_reg(phydev, MXL86110_LED0_CFG_REG + index, val);
> > +             if (ret < 0)
> > +                     return ret;
>
> _modify_ ?
>
> Getting pretty close to finished now. Thanks for keeping working on
> it.
>
>         Andrew

Powered by blists - more mailing lists