Message-ID: <0280ecbc-06e4-72ce-95f8-17217833c19f@gmail.com>
Date:   Fri, 3 Feb 2023 22:57:22 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
To:     Horatiu Vultur <horatiu.vultur@microchip.com>
Cc:     netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        andrew@lunn.ch, linux@armlinux.org.uk, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        michael@walle.cc
References: <20230203122542.436305-1-horatiu.vultur@microchip.com>
 <0f81d14d-50cb-b807-b103-8fa066d0769c@gmail.com>
 <20230203151059.k5aa6zihibgsedcw@soft-dev3-1>
Content-Language: en-US
From:   Heiner Kallweit <hkallweit1@gmail.com>
Subject: Re: [PATCH net-next v2] net: micrel: Add support for lan8841 PHY
In-Reply-To: <20230203151059.k5aa6zihibgsedcw@soft-dev3-1>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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