On 17/02/14 17:48, Florian Fainelli wrote:
2014-02-17 9:42 GMT-08:00 Ben Dooks <ben.dooks@xxxxxxxxxxxxxxx>:
On 17/02/14 17:26, Florian Fainelli wrote:
Hi Ben,
2014-02-17 8:29 GMT-08:00 Ben Dooks <ben.dooks@xxxxxxxxxxxxxxx>:
The of_mdiobus_register_phy() is not setting phy->irq this causing
some drivers to incorrectly assume that the PHY does not have an
IRQ associated with it or install an interrupt handler for the
PHY.
Simplify the code setting irq and set the phy->irq at the same
time so that the case if mdio->irq is not NULL is easier to read.
The real bug fix, which is not properly explained here, is that
irq_of_parse_and_map() should return values > 0 when the interrupt is
valid, so this makes me wonder why we are not propagating the return
value from irq_of_parse_and_map() in case the call to
of_irq_parse_one() does return something non-zero?
No, the first issue is phy->dev never gets set, which causes the
issue. The cleanup was added as it seemed easier to put it in with
this.
Ok, that really needs to be mentioned in the commit message, even
being quite familiar (and possibly dumb too) with the code, I could
not figure this out by reading your patch.
I think phy->irq is already initialised to PHY_POLL and thus there
is no need to set phy->irq if the irq_of_parse_and_map() fails.
That is correct, the reason why I introduced 7d97637 ("net: of_mdio:
do not overwrite PHY interrupt configuration") was that you are also
allowed to change the irq type before calling into
of_mdiobus_register(), so we want to preserve other irq values being
set here, such as PHY_IGNORE_INTERRUPT. Your patch does take care of
that since it only overrides the irq in case we could parse it.
Ok, the first sentence has a spell of thus, but does refer to phy->irq
being the problem we are looking to fix in this patch.
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
