Date:   Fri, 3 Feb 2023 08:35:11 +0100
From:   Horatiu Vultur <horatiu.vultur@microchip.com>
To:     Andrew Lunn <andrew@lunn.ch>
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: <20230203073511.5qsj35xgewmcnva2@soft-dev3-1>
References: <20230202094704.175665-1-horatiu.vultur@microchip.com>
 <Y9u+UNHht9OAhKHv@lunn.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <Y9u+UNHht9OAhKHv@lunn.ch>
Precedence: bulk

The 02/02/2023 14:44, Andrew Lunn wrote:

Hi Andrew,

Thanks for the review.

> 
> > @@ -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.

I will try remove all the magic numbers.
The same will apply for all the bellow comments.

> 
> > +     /* 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

-- 
/Horatiu