On Tue, Jul 08, 2014 at 01:18:17PM -0400, Tejun Heo wrote:

Hello,

On Tue, Jul 08, 2014 at 07:03:53PM +0200, Antoine Ténart wrote:

quoted

quoted

So, hpriv->port is both input and output?  This is messy and can lead
to confusing failures and there now are multiple ways to modify
port_map.  If carrying this information through ahci_host_priv is
necessary, let's remove the direct params and introduce new input
fields to the struct.

We just use hpriv->port_map to check port_map is valid and describes
available ports there.

hpriv->port_map is filed by the generic ahci_platform_get_resources()
function when using the new bindings and not by the drivers. port_map is
the input from the drivers.

So, yeah, it's being used both as input and output and we also have
the arguments which affect port_map, right?  It does seem confusing.

I do see priv->port_map as being automatically set and then restricted
if needed by the port_map input here. I don't see how that's confusing.
The only modification is we restrict the port_map parameter to the set
of available ports. The port_map argument affected priv->port_map before
this patch.

quoted

Well, a had a review a week ago about in the PHY driver saying I should
avoid using fixed sized arrays... And it was in a driver were we know
the maximum number of PHY available.

I think in this case were the number of PHYs depends on the h/w, we should
use a dynamically allocated array.

Well, so does clk.  Let's say clk is more restricted and phy can be
one or more per port and thus needs to be dynamic.  If so, shouldn't
we at least have some correlation between phys and ports?  It bothers
me that now libahci is carrying random number of resources that it has
no idea how to associate with the ports it manages.  What if later we
want to involve phy driver in power managing unoccupied ports?

I see. This is a first (working) attempt to have a one node per port. I
agree that would be nice to have a correlation between ports and PHYs.
This can definitively be added when needed without changing the dt
bindings as only the internal representation changes. This would also
require to get all phys from the port nodes, which is again internal
stuff.

Don't you think we can go by steps, and have a following up series for
this when needed (like in a power managing series for unoccupied ports)?


Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com