Re: [PATCH net v3] net: phy: dp83640: fix per-bus clock lifetime

Re: [PATCH net v3] net: phy: dp83640: fix per-bus clock lifetime

[Andrew Lunn]

> +static void dp83640_clock_init(struct dp83640_clock *clock)
>  {
> -	INIT_LIST_HEAD(&clock->list);
> -	clock->bus = bus;
>  	mutex_init(&clock->extreg_lock);
>  	mutex_init(&clock->clock_lock);
>  	INIT_LIST_HEAD(&clock->phylist);
> +	clock->caps.pin_config = clock->pin_config;

Can this patch be split up a bit? It is not clear to my why this is
needed. If you have lots of small patches, each with good commit
messages, you can explain "Why?" this is needed.

>  	clock->caps.owner = THIS_MODULE;
>  	sprintf(clock->caps.name, "dp83640 timer");
>  	clock->caps.max_adj	= 1953124;
> @@ -977,14 +971,8 @@ static void dp83640_clock_init(struct dp83640_clock *clock, struct mii_bus *bus)
>  	clock->caps.settime64	= ptp_dp83640_settime;
>  	clock->caps.enable	= ptp_dp83640_enable;
>  	clock->caps.verify	= ptp_dp83640_verify;
> -	/*
> -	 * Convert the module param defaults into a dynamic pin configuration.
> -	 */
> +	/* Convert the module param defaults into the pin configuration. */
>  	dp83640_gpio_defaults(clock->caps.pin_config);

Why is this no longer dynamic?

> +	clock = phy_package_get_priv(phydev);
> +	/* Ensure other PHY probes wait for shared clock initialization. */
> +	mutex_lock(&phydev->mdio.bus->shared_lock);

Please add phy_package_lock() and phy_package_unlock().


    Andrew

---
pw-bot: cr

Re: [PATCH net v3] net: phy: dp83640: fix per-bus clock lifetime

[luoxuanqiang]

Hi Andrew,

在 2026/8/5 20:11, Andrew Lunn 写道:
>> +static void dp83640_clock_init(struct dp83640_clock *clock)
>>   {
>> -	INIT_LIST_HEAD(&clock->list);
>> -	clock->bus = bus;
>>   	mutex_init(&clock->extreg_lock);
>>   	mutex_init(&clock->clock_lock);
>>   	INIT_LIST_HEAD(&clock->phylist);
>> +	clock->caps.pin_config = clock->pin_config;
> Can this patch be split up a bit? It is not clear to my why this is
> needed. If you have lots of small patches, each with good commit
> messages, you can explain "Why?" this is needed.

You're right. I'll split the changes into small, independently reviewable
patches.

>>   	clock->caps.owner = THIS_MODULE;
>>   	sprintf(clock->caps.name, "dp83640 timer");
>>   	clock->caps.max_adj	= 1953124;
>> @@ -977,14 +971,8 @@ static void dp83640_clock_init(struct dp83640_clock *clock, struct mii_bus *bus)
>>   	clock->caps.settime64	= ptp_dp83640_settime;
>>   	clock->caps.enable	= ptp_dp83640_enable;
>>   	clock->caps.verify	= ptp_dp83640_verify;
>> -	/*
>> -	 * Convert the module param defaults into a dynamic pin configuration.
>> -	 */
>> +	/* Convert the module param defaults into the pin configuration. */
>>   	dp83640_gpio_defaults(clock->caps.pin_config);
> Why is this no longer dynamic?

Sorry, the updated comment is misleading.

The pin configuration is no longer dynamically allocated, but it can
still be changed at runtime via ptp_set_pinfunc().

I'll fix the comment and move this change into a separate patch,
since it can be reviewed independently.

>> +	clock = phy_package_get_priv(phydev);
>> +	/* Ensure other PHY probes wait for shared clock initialization. */
>> +	mutex_lock(&phydev->mdio.bus->shared_lock);
> Please add phy_package_lock() and phy_package_unlock().
>
I'll add phy_package_lock() and phy_package_unlock().

Thanks,
Xuanqiang