[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230203151059.k5aa6zihibgsedcw@soft-dev3-1>
Date: Fri, 3 Feb 2023 16:10:59 +0100
From: Horatiu Vultur <horatiu.vultur@...rochip.com>
To: Heiner Kallweit <hkallweit1@...il.com>
CC: <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<andrew@...n.ch>, <linux@...linux.org.uk>, <davem@...emloft.net>,
<edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
<michael@...le.cc>
Subject: Re: [PATCH net-next v2] net: micrel: Add support for lan8841 PHY
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.
>
> > + 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
Powered by blists - more mailing lists