Date:   Sat, 4 Feb 2023 11:12:35 +0100
From:   Horatiu Vultur <horatiu.vultur@microchip.com>
To:     Heiner Kallweit <hkallweit1@gmail.com>
CC:     <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <andrew@lunn.ch>, <linux@armlinux.org.uk>, <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: <20230204101235.7fk4jqditdjrqegp@soft-dev3-1>
References: <20230203122542.436305-1-horatiu.vultur@microchip.com>
 <0f81d14d-50cb-b807-b103-8fa066d0769c@gmail.com>
 <20230203151059.k5aa6zihibgsedcw@soft-dev3-1>
 <0280ecbc-06e4-72ce-95f8-17217833c19f@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <0280ecbc-06e4-72ce-95f8-17217833c19f@gmail.com>
Precedence: bulk

The 02/03/2023 22:57, Heiner Kallweit wrote:
> 
> On 03.02.2023 16:10, Horatiu Vultur wrote:
> > The 02/03/2023 14:55, Heiner Kallweit wrote:
> >
> > Hi Heiner,
> >
> >>
> >> On 03.02.2023 13:25, Horatiu Vultur wrote:
> >
> > ...
> >
> >>> +
> >>> +#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 */
> >>
> >> Why this a little bit esoteric logic? Can't you set the interrupt
> >> to level-low in the chip (like most other ones), and then define
> >> the polarity the usual way e.g. in DT?
> >
> > To set the interrupt to level-low it needs to be set to open-drain and
> > in that case I can't use the polarity register, because doesn't have any
> > effect on the interrupt. So I can't set the interrupt to level low and
> > then use the polarity to select if it is high or low.
> > That is the reason why I have these checks.
> >
> To me this still doesn't look right. After checking the datasheet I'd say:
> At first open-drain should be preferred because only in this mode the
> interrupt line can be shared.

Agree.

> And if you use level-low and open-drain, why would you want to fiddle
> with the polarity?

In this case, I don't fiddle with the polarity. That case is on the else
branch of this if condition. I play with the polarity only when using
push-pull.

> Level-low and open-drain is the only mode supported by
> most PHY's and it's totally fine.
>
> Or do you have a special use case where
> you want to connect the interrupt pin to an interrupt controller that
> only supports level-high and has no programmable inverter in its path?

I have two cases:
1. When lan966x is connected to this lan8841. In this case the interrupt
controller supports both level-low and level-high. But in this case I
can test only the level-low.

2. When lan7431 is connected to this lan8841 and using x86. If I
remember correctly (I don't have the setup to test it anymore and will
take a some time to get it again) this worked only with level-high
interrupts. To get this working I had some changes in the lan7431 driver
to enable interrupts from the external PHY.

Maybe a better approach would be for now, just to set the interrupt to
open-drain in the lan8841. And only when I add the changes to lan7431
also add the changes to lan8841 to support level-high interrupts if it
is still needed.

> 
> >>
> >>> +             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;
> >>> +}
> >>> +
> >
> 

-- 
/Horatiu