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

[Heiner Kallweit <hkallweit1@gmail.com>]

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