Date:   Thu, 9 Feb 2023 16:37:19 +0000
From:   "Russell King (Oracle)" <linux@armlinux.org.uk>
To:     Horatiu Vultur <horatiu.vultur@microchip.com>
Cc:     netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        andrew@lunn.ch, hkallweit1@gmail.com, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        michael@walle.cc
Subject: Re: [PATCH net-next v2] net: micrel: Add support for lan8841 PHY
Message-ID: <Y+UhP1Ycq8a8RWJr@shell.armlinux.org.uk>
References: <20230203122542.436305-1-horatiu.vultur@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230203122542.436305-1-horatiu.vultur@microchip.com>
Sender: Russell King (Oracle) <linux@armlinux.org.uk>
Precedence: bulk

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!