Messages in this thread | | | Date | Fri, 3 Feb 2023 22:57:22 +0100 | From | Heiner Kallweit <> | Subject | Re: [PATCH net-next v2] net: micrel: Add support for lan8841 PHY |
| |
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. And if you use level-low and open-drain, why would you want to fiddle with the polarity? 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?
>> >>> + 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; >>> +} >>> + >
| |