Re: [RFC PATCH 3/6] phy: rockcip-pcie: reconstruct driver to support per-lane PHYs

[jeffy <jeffy.chen@rock-chips.com>]

Hi Shawn,

On 07/14/2017 02:33 PM, Shawn Lin wrote:
>>
>>> +        return rk_phy->phys[0];
>>> +    }
>>> +}
>>> +
>>>   static inline void phy_wr_cfg(struct rockchip_pcie_phy *rk_phy,
>>>                     u32 addr, u32 data)
>>>   {
>>> @@ -114,20 +139,55 @@ static inline u32 phy_rd_cfg(struct
>>> rockchip_pcie_phy *rk_phy,
>>>       return val;
>>>   }
>>> -static int rockchip_pcie_phy_power_off(struct phy *phy)
>>> +static int rockchip_pcie_phy_common_power_off(struct phy *phy)
>>>   {
>>>       struct rockchip_pcie_phy *rk_phy = phy_get_drvdata(phy);
>>>       int err = 0;
>>> +    if (WARN_ON(!rk_phy->pwr_cnt))
>>> +        return -EINVAL;
>>> +
>>> +    if (rk_phy->pwr_cnt > 0)
>>
>> This should be:
>>
>>     if (--rk_phy->pwr_cnt)
>>
>> Also, you technically might need locking, now that multiple phys (which
>> each only have their own independent mutex) are accessing the same
>> refcount. Or maybe just make this an atomic variable.
>
> Good catch!
Sounds like we need something similar to phy-core.c's power_count and 
init_count.

>>> +
>>>       return 0;
>>>   }
>>> +#define DECLARE_PHY_POWER_OFF_PER_LANE(id) \
>>> +static int rockchip_pcie_lane##id##_phy_power_off(struct phy *phy) \
>>
>> What? All this macro magic (and duplicate generated functions) should
>> not be necessary. You just need some per-phy data that keeps the index.
>
> I can't quite follow yours here. The only argument passing on to
> the PHY APIs is 'struct phy *phy', and how could you trace the index
> from it? The caller should save phy instead of 'rockchip_pcie_phy', in
> which the per-phy data should be.
>
> Or could you kindly show me some example here:)
>
Maybe add a struct rockchip_pcie_phy_data for each phy, contains their 
index and a pointer to the common struct rockchip_pcie_phy?