On 4/20/20 4:07 PM, Neil Armstrong wrote: [...]
Doesn't checkpatch complain about this extra space before semicolon ?
Nop, even in --strict
Shouldn't it though ? I thought this extra space was forbidden.
No idea, this line is copied from the for loop doing the phy init, itself copied from the Linux code passing all checkpatch checks.
Could you change it to for (i = 0; i < PHY_COUNT; i++) please ?
Sure
Thank you.
( I'm still surprised checkpatch doesn't warn about it, I was quite sure it was one of the checks at some point, hmmmmm. )
> + if (!priv->phys[i].dev) > + continue; > + > + ret = generic_phy_power_on(&priv->phys[i]);
Do we really need to turn on all the PHYs ?
Yes
Then should we have something like clk_bulk_*(), but for phys ?
Eventually, yes, but this goes beyond a fix.
The merge window is open for quite a bit longer, so doing this in a generic way would be appreciated.
btw while you're at it, shouldn't the PHY_COUNT be replaced with some PHY count detection by reading the DT ? ( I don't have the hardware and I don't know the amlogic SoCs though )
Hmm, the PHYs list can have some holes (i.e only the second PHY could be used on 3), and the name of the PHY determines it's type and position (PHY1 is a special OTG phy) this is why we loop over a fixed array and check if the PHY pointer exists.
Um, that sounds like a vaguely familiar problem. Is that an issue with mapping between controller and a USB PHY ? Is that mapping fixed or is it configured from DT ? And then, should all PHYs really be powered on, or should only the PHY matching the currently used controller be powered on ?
This won't match a phy_bulk_code directly, I would still need to map the bulk PHYs to this array for the OTG and USB3 management.
OK