Messages in this thread | | | Date | Sat, 4 Feb 2023 11:12:35 +0100 | From | Horatiu Vultur <> | Subject | Re: [PATCH net-next v2] net: micrel: Add support for lan8841 PHY |
| |
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
| |