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?
