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

[Horatiu Vultur <horatiu.vultur@microchip.com>]

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