Hi Jeffy,
On 2017/7/14 15:03, jeffy wrote:
> 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.
Probably, and I will look into it later.
>
>>>> +
>>>> 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?
yes, I got Brian's point after reading it more times, and I almost
finish converting to per-lane data now..
>
>
>
>
>
