Re: [PATCH net-next v2] net: micrel: Add support for lan8841 PHY

From: Russell King (Oracle)
Date: Thu Feb 09 2023 - 11:37:36 EST


On Fri, Feb 03, 2023 at 01:25:42PM +0100, Horatiu Vultur wrote:
> +hw_init:
> + /* 100BT Clause 40 improvenent errata */
> + phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
> + LAN8841_ANALOG_CONTROL_1,
> + LAN8841_ANALOG_CONTROL_1_PLL_TRIM(0x2));
> + phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
> + LAN8841_ANALOG_CONTROL_10,
> + LAN8841_ANALOG_CONTROL_10_PLL_DIV(0x1));
> +
> + /* 10M/100M Ethernet Signal Tuning Errata for Shorted-Center Tap
> + * Magnetics
> + */
> + ret = phy_read_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG,
> + LAN8841_OPERATION_MODE_STRAP_OVERRIDE_LOW_REG);

Error handling? If this returns a negative error code, then the if()
statement likely becomes true... although the writes below may also
error out.

> + if (ret & LAN8841_OPERATION_MODE_STRAP_OVERRIDE_LOW_REG_MAGJACK) {
> + phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
> + LAN8841_TX_LOW_I_CH_C_D_POWER_MANAGMENT,
> + LAN8841_TX_LOW_I_CH_C_D_POWER_MANAGMENT_VAL);
> + phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
> + LAN8841_BTRX_POWER_DOWN,
> + LAN8841_BTRX_POWER_DOWN_QBIAS_CH_A |
> + LAN8841_BTRX_POWER_DOWN_BTRX_CH_A |
> + LAN8841_BTRX_POWER_DOWN_QBIAS_CH_B |
> + LAN8841_BTRX_POWER_DOWN_BTRX_CH_B |
> + LAN8841_BTRX_POWER_DOWN_BTRX_CH_C |
> + LAN8841_BTRX_POWER_DOWN_BTRX_CH_D);
> + }
> +
> + /* LDO Adjustment errata */
> + phy_write_mmd(phydev, LAN8841_MMD_ANALOG_REG,
> + LAN8841_ANALOG_CONTROL_11,
> + LAN8841_ANALOG_CONTROL_11_LDO_REF(1));
> +
> + /* 100BT RGMII latency tuning errata */
> + phy_write_mmd(phydev, MDIO_MMD_PMAPMD,
> + LAN8841_ADC_CHANNEL_MASK, 0x0);
> + phy_write_mmd(phydev, LAN8841_MMD_TIMER_REG,
> + LAN8841_MMD0_REGISTER_17,
> + LAN8841_MMD0_REGISTER_17_DROP_OPT(2) |
> + LAN8841_MMD0_REGISTER_17_XMIT_TOG_TX_DIS);
> +
> + return 0;

This function is always succesful, even if the writes fail?

> +}
> +
> +#define LAN8841_OUTPUT_CTRL 25
> +#define LAN8841_OUTPUT_CTRL_INT_BUFFER BIT(14)
> +#define LAN8841_CTRL 31
> +#define LAN8841_CTRL_INTR_POLARITY BIT(14)
> +static int lan8841_config_intr(struct phy_device *phydev)
> +{
> + struct irq_data *irq_data;
> + int temp = 0;
> +
> + irq_data = irq_get_irq_data(phydev->irq);
> + if (!irq_data)
> + return 0;
> +
> + if (irqd_get_trigger_type(irq_data) & IRQ_TYPE_LEVEL_HIGH) {
> + /* Change polarity of the interrupt */
> + phy_modify(phydev, LAN8841_OUTPUT_CTRL,
> + LAN8841_OUTPUT_CTRL_INT_BUFFER,
> + LAN8841_OUTPUT_CTRL_INT_BUFFER);
> + phy_modify(phydev, LAN8841_CTRL,
> + LAN8841_CTRL_INTR_POLARITY,
> + LAN8841_CTRL_INTR_POLARITY);
> + } else {
> + /* It is enough to set INT buffer to open-drain because then
> + * the interrupt will be active low.
> + */
> + phy_modify(phydev, LAN8841_OUTPUT_CTRL,
> + LAN8841_OUTPUT_CTRL_INT_BUFFER, 0);
> + }
> +
> + /* enable / disable interrupts */
> + if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
> + temp = LAN8814_INT_LINK;
> +
> + return phy_write(phydev, LAN8814_INTC, temp);
> +}
> +
> +static irqreturn_t lan8841_handle_interrupt(struct phy_device *phydev)
> +{
> + int irq_status;
> +
> + irq_status = phy_read(phydev, LAN8814_INTS);
> + if (irq_status < 0) {
> + phy_error(phydev);
> + return IRQ_NONE;
> + }
> +
> + if (irq_status & LAN8814_INT_LINK) {
> + phy_trigger_machine(phydev);
> + return IRQ_HANDLED;
> + }
> +
> + return IRQ_NONE;
> +}
> +
> +#define LAN8841_OPERATION_MODE_STRAP_LOW_REGISTER 3
> +#define LAN8841_OPERATION_MODE_STRAP_LOW_REGISTER_STRAP_RGMII_EN BIT(0)
> +static int lan8841_probe(struct phy_device *phydev)
> +{
> + int err;
> +
> + err = kszphy_probe(phydev);
> + if (err)
> + return err;
> +
> + if (phy_read_mmd(phydev, KSZ9131RN_MMD_COMMON_CTRL_REG,
> + LAN8841_OPERATION_MODE_STRAP_LOW_REGISTER) &
> + LAN8841_OPERATION_MODE_STRAP_LOW_REGISTER_STRAP_RGMII_EN)
> + phydev->interface = PHY_INTERFACE_MODE_RGMII_RXID;

I'm not entirely sure what this code is trying to do here, as many
drivers just pass into phy_attach_direct() the interface mode that
was configured in firmware or by the ethernet driver's platform
data, and that will override phydev->interface.

There are a few corner cases in DSA where we do make use of the
phy's ->interface as set at probe() time but that's rather
exceptional.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!