Date:   Thu, 2 Feb 2023 14:44:48 +0100
From:   Andrew Lunn <andrew@lunn.ch>
To:     Horatiu Vultur <horatiu.vultur@microchip.com>
Cc:     netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        hkallweit1@gmail.com, linux@armlinux.org.uk, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        michael@walle.cc
Subject: Re: [PATCH net-next] net: micrel: Add support for lan8841 PHY
Message-ID: <Y9u+UNHht9OAhKHv@lunn.ch>
References: <20230202094704.175665-1-horatiu.vultur@microchip.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230202094704.175665-1-horatiu.vultur@microchip.com>
Precedence: bulk

> @@ -1172,19 +1189,18 @@ static int ksz9131_of_load_skew_values(struct phy_device *phydev,
>  #define KSZ9131RN_MMD_COMMON_CTRL_REG	2
> +	/* 100BT Clause 40 improvenent errata */
> +	phy_write_mmd(phydev, 28, LAN8841_ANALOG_CONTROL_1, 0x40);
> +	phy_write_mmd(phydev, 28, LAN8841_ANALOG_CONTROL_10, 0x1);

Please could you try to avoid magic numbers.

> +	/* 10M/100M Ethernet Signal Tuning Errata for Shorted-Center Tap
> +	 * Magnetics
> +	 */
> +	ret = phy_read_mmd(phydev, 2, 0x2);

KSZ9131RN_MMD_COMMON_CTRL_REG ?

> +	if (ret & BIT(14)) {
> +		phy_write_mmd(phydev, 28,
> +			      LAN8841_TX_LOW_I_CH_C_POWER_MANAGMENT, 0xbffc);
> +		phy_write_mmd(phydev, 28,
> +			      LAN8841_BTRX_POWER_DOWN, 0xaf);
> +	}
> +
> +	/* LDO Adjustment errata */
> +	phy_write_mmd(phydev, 28, LAN8841_ANALOG_CONTROL_11, 0x1000);
> +
> +	/* 100BT RGMII latency tuning errata */
> +	phy_write_mmd(phydev, 1, LAN8841_ADC_CHANNEL_MASK, 0x0);
> +	phy_write_mmd(phydev, 0, LAN8841_MMD0_REGISTER_17, 0xa);

MDIO_MMD_PMAPMD	instead of 1.


> +
> +	return 0;
> +}
> +
> +#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, 2, LAN8841_OPERATION_MODE_STRAP_LOW_REGISTER) &

MDIO_MMD_WIS ?

	Andrew