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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 04.02.2023 11:12, Horatiu Vultur wrote:
> 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.
> 
Looking at the datasheet for LAN7431 (document AN2948, page 76) in register
GPIO_WAKE you can configure the polarity for the PHY interrupt (to be exact,
for all GPIO-triggered interrupts).
Therefore level-low should be fine also with LAN7431.

GPIO Polarity 0-11 (GPIOPOL[11:0])
When clear, the pin functions as a GPIO.
0 = Wakeup/interrupt is triggered when GPIO is driven low.
1 = Wakeup/interrupt is triggered when GPIO is driven high

> 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.
> 
Agree.

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