Re: [PATCH 2/2] phy: add a driver for the Rockchip SoC internal eMMC PHY

[Shawn Lin <shawn.lin@rock-chips.com>]

Hi Kishon,
On 2016/1/4 15:45, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Tuesday 29 December 2015 07:22 AM, Shawn Lin wrote:
>> This patch to add a generic PHY driver for ROCKCHIP eMMC PHY.

[...]

>> +
>> +struct rockchip_emmc_phy {
>> +	unsigned int	reg_offset;
>> +	struct regmap	*reg_base;
>> +	struct phy	*phy;
>
> The phy looks unnecessary.

right, got it.

>> +	bool		state;
>
> hmm.. I want to have some sort of state machine in phy core so that individual
> PHY drivers don't have to maintain the state. However I'm not sure if all the
> PHY's will require such mechanism.
>

In general, phy core is the best place to maintain it.
Presumably, phy driver maintainer is much likely to
maintain the corresponding caller driver at the same time, so he/she
should be in charge of maintaining the on/off, init/exit pairs. From
this point, it doesn't need the state machine. But, I'm not sure if the
caller driver will always keep the on/off pairs correct.


Anyway, I will remove it from this driver. And may we request a RFC
for all sub-phy drivers to discuss this issue if we want to add the
state machine into phy core. How about?


>> +};
>> +
>> +static int rockchip_emmc_phy_power(struct rockchip_emmc_phy *rk_phy,

[...]

>> +}
>> +
>> +static int rockchip_emmc_phy_init(struct phy *phy)
>> +{
>> +	rockchip_emmc_phy_power_on(phy);
>
> do only phy initialization here, power on can be done later.

yep.

>> +	return 0;
>> +}
>> +
>> +static int rockchip_emmc_phy_exit(struct phy *phy)
>> +{
>> +	rockchip_emmc_phy_power_off(phy);
>
> same here.
>
> Thanks
> Kishon
>
>
>


-- 
Best Regards
Shawn Lin