* Re: [PATCH] phy: rockchip: usbdp: Check these parameters only when the corresponding set flags are set
@ 2025-03-10 7:26 ` Vinod Koul
0 siblings, 0 replies; 15+ messages in thread
From: Vinod Koul @ 2025-03-10 7:26 UTC (permalink / raw)
To: Andy Yan
Cc: heiko, kishon, sebastian.reichel, yubing.zhang, dmitry.baryshkov,
frank.wang, linux-arm-kernel, linux-kernel, linux-phy,
linux-rockchip, Andy Yan
On 06-03-25, 14:59, Andy Yan wrote:
> From: Andy Yan <andy.yan@rock-chips.com>
>
> According documentation of phy_configure_opts_dp, at the configure
> stage, we should only verify/configure the link_rate when set_rate
> flag is set, the same applies to lanes and voltage.
>
> So we do it as the documentation says, also record the link rate
> and lanes in phy internal for set_voltate stage.
Whenever you say also, that is a sign that it should be another patch!
>
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> ---
>
> drivers/phy/rockchip/phy-rockchip-usbdp.c | 63 +++++++++++------------
> 1 file changed, 31 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index c04cf64f8a35..d1bbdf382aa2 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -187,6 +187,8 @@ struct rk_udphy {
> u32 dp_aux_din_sel;
> bool dp_sink_hpd_sel;
> bool dp_sink_hpd_cfg;
> + unsigned int link_rate;
> + unsigned int lanes;
> u8 bw;
> int id;
>
> @@ -1102,42 +1104,39 @@ static int rk_udphy_dp_phy_power_off(struct phy *phy)
> return 0;
> }
>
> -static int rk_udphy_dp_phy_verify_link_rate(unsigned int link_rate)
> -{
> - switch (link_rate) {
> - case 1620:
> - case 2700:
> - case 5400:
> - case 8100:
> - break;
> -
> - default:
> - return -EINVAL;
> - }
> -
> - return 0;
> -}
> -
> static int rk_udphy_dp_phy_verify_config(struct rk_udphy *udphy,
> struct phy_configure_opts_dp *dp)
> {
> - int i, ret;
> + int i;
>
> - /* If changing link rate was required, verify it's supported. */
> - ret = rk_udphy_dp_phy_verify_link_rate(dp->link_rate);
> - if (ret)
> - return ret;
> + /* Verify link rate. */
> + if (dp->set_rate) {
> + switch (dp->link_rate) {
> + case 1620:
> + case 2700:
> + case 5400:
> + case 8100:
> + udphy->link_rate = dp->link_rate;
> + break;
> +
> + default:
> + return -EINVAL;
> + }
why drop helper? Why not set the rate on success?
> + }
>
> /* Verify lane count. */
> - switch (dp->lanes) {
> - case 1:
> - case 2:
> - case 4:
> - /* valid lane count. */
> - break;
> + if (dp->set_lanes) {
> + switch (dp->lanes) {
> + case 1:
> + case 2:
> + case 4:
> + /* valid lane count. */
> + udphy->lanes = dp->lanes;
> + break;
>
> - default:
> - return -EINVAL;
> + default:
> + return -EINVAL;
> + }
another change where helper would have made this look better
> }
>
> /*
> @@ -1146,7 +1145,7 @@ static int rk_udphy_dp_phy_verify_config(struct rk_udphy *udphy,
> */
> if (dp->set_voltages) {
> /* Lane count verified previously. */
> - for (i = 0; i < dp->lanes; i++) {
> + for (i = 0; i < udphy->lanes; i++) {
> if (dp->voltage[i] > 3 || dp->pre[i] > 3)
> return -EINVAL;
>
> @@ -1243,9 +1242,9 @@ static int rk_udphy_dp_phy_configure(struct phy *phy,
> }
>
> if (dp->set_voltages) {
> - for (i = 0; i < dp->lanes; i++) {
> + for (i = 0; i < udphy->lanes; i++) {
> lane = udphy->dp_lane_sel[i];
> - switch (dp->link_rate) {
> + switch (udphy->link_rate) {
> case 1620:
> case 2700:
> regmap_update_bits(udphy->pma_regmap,
> --
> 2.34.1
--
~Vinod
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] phy: rockchip: usbdp: Check these parameters only when the corresponding set flags are set
@ 2025-03-10 7:26 ` Vinod Koul
0 siblings, 0 replies; 15+ messages in thread
From: Vinod Koul @ 2025-03-10 7:26 UTC (permalink / raw)
To: Andy Yan
Cc: heiko, kishon, sebastian.reichel, yubing.zhang, dmitry.baryshkov,
frank.wang, linux-arm-kernel, linux-kernel, linux-phy,
linux-rockchip, Andy Yan
On 06-03-25, 14:59, Andy Yan wrote:
> From: Andy Yan <andy.yan@rock-chips.com>
>
> According documentation of phy_configure_opts_dp, at the configure
> stage, we should only verify/configure the link_rate when set_rate
> flag is set, the same applies to lanes and voltage.
>
> So we do it as the documentation says, also record the link rate
> and lanes in phy internal for set_voltate stage.
Whenever you say also, that is a sign that it should be another patch!
>
> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
> ---
>
> drivers/phy/rockchip/phy-rockchip-usbdp.c | 63 +++++++++++------------
> 1 file changed, 31 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index c04cf64f8a35..d1bbdf382aa2 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -187,6 +187,8 @@ struct rk_udphy {
> u32 dp_aux_din_sel;
> bool dp_sink_hpd_sel;
> bool dp_sink_hpd_cfg;
> + unsigned int link_rate;
> + unsigned int lanes;
> u8 bw;
> int id;
>
> @@ -1102,42 +1104,39 @@ static int rk_udphy_dp_phy_power_off(struct phy *phy)
> return 0;
> }
>
> -static int rk_udphy_dp_phy_verify_link_rate(unsigned int link_rate)
> -{
> - switch (link_rate) {
> - case 1620:
> - case 2700:
> - case 5400:
> - case 8100:
> - break;
> -
> - default:
> - return -EINVAL;
> - }
> -
> - return 0;
> -}
> -
> static int rk_udphy_dp_phy_verify_config(struct rk_udphy *udphy,
> struct phy_configure_opts_dp *dp)
> {
> - int i, ret;
> + int i;
>
> - /* If changing link rate was required, verify it's supported. */
> - ret = rk_udphy_dp_phy_verify_link_rate(dp->link_rate);
> - if (ret)
> - return ret;
> + /* Verify link rate. */
> + if (dp->set_rate) {
> + switch (dp->link_rate) {
> + case 1620:
> + case 2700:
> + case 5400:
> + case 8100:
> + udphy->link_rate = dp->link_rate;
> + break;
> +
> + default:
> + return -EINVAL;
> + }
why drop helper? Why not set the rate on success?
> + }
>
> /* Verify lane count. */
> - switch (dp->lanes) {
> - case 1:
> - case 2:
> - case 4:
> - /* valid lane count. */
> - break;
> + if (dp->set_lanes) {
> + switch (dp->lanes) {
> + case 1:
> + case 2:
> + case 4:
> + /* valid lane count. */
> + udphy->lanes = dp->lanes;
> + break;
>
> - default:
> - return -EINVAL;
> + default:
> + return -EINVAL;
> + }
another change where helper would have made this look better
> }
>
> /*
> @@ -1146,7 +1145,7 @@ static int rk_udphy_dp_phy_verify_config(struct rk_udphy *udphy,
> */
> if (dp->set_voltages) {
> /* Lane count verified previously. */
> - for (i = 0; i < dp->lanes; i++) {
> + for (i = 0; i < udphy->lanes; i++) {
> if (dp->voltage[i] > 3 || dp->pre[i] > 3)
> return -EINVAL;
>
> @@ -1243,9 +1242,9 @@ static int rk_udphy_dp_phy_configure(struct phy *phy,
> }
>
> if (dp->set_voltages) {
> - for (i = 0; i < dp->lanes; i++) {
> + for (i = 0; i < udphy->lanes; i++) {
> lane = udphy->dp_lane_sel[i];
> - switch (dp->link_rate) {
> + switch (udphy->link_rate) {
> case 1620:
> case 2700:
> regmap_update_bits(udphy->pma_regmap,
> --
> 2.34.1
--
~Vinod
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 15+ messages in thread* Re:Re: [PATCH] phy: rockchip: usbdp: Check these parameters only when the corresponding set flags are set
2025-03-10 7:26 ` Vinod Koul
(?)
@ 2025-03-11 0:52 ` Andy Yan
-1 siblings, 0 replies; 15+ messages in thread
From: Andy Yan @ 2025-03-11 0:52 UTC (permalink / raw)
To: Vinod Koul
Cc: heiko, kishon, sebastian.reichel, yubing.zhang, dmitry.baryshkov,
frank.wang, linux-arm-kernel, linux-kernel, linux-phy,
linux-rockchip, Andy Yan
Hi Vinod,
At 2025-03-10 15:26:50, "Vinod Koul" <vkoul@kernel.org> wrote:
>On 06-03-25, 14:59, Andy Yan wrote:
>> From: Andy Yan <andy.yan@rock-chips.com>
>>
>> According documentation of phy_configure_opts_dp, at the configure
>> stage, we should only verify/configure the link_rate when set_rate
>> flag is set, the same applies to lanes and voltage.
>>
>> So we do it as the documentation says, also record the link rate
>> and lanes in phy internal for set_voltate stage.
>
>Whenever you say also, that is a sign that it should be another patch!
This patch is inspired by Dmitry's suggestion for DP/eDP driver which calls phy_configure[0][1]:
the phy driver should skip the values for which the .set_foo isn't set. So we should not set rates and
lanes when we set_voltate, but the rates and lanes are needed for voltage set, this is why I record
rates and lanes in this patch.
I can split the record to another patch, but then this patch will not work correctly itself,
I'm not sure if this is suitable.
[0]https://patchwork.freedesktop.org/patch/629780/?series=141829&rev=3
[1]https://patchwork.freedesktop.org/patch/638961/?series=145286&rev=1
>
>>
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>> ---
>>
>> drivers/phy/rockchip/phy-rockchip-usbdp.c | 63 +++++++++++------------
>> 1 file changed, 31 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
>> index c04cf64f8a35..d1bbdf382aa2 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
>> @@ -187,6 +187,8 @@ struct rk_udphy {
>> u32 dp_aux_din_sel;
>> bool dp_sink_hpd_sel;
>> bool dp_sink_hpd_cfg;
>> + unsigned int link_rate;
>> + unsigned int lanes;
>> u8 bw;
>> int id;
>>
>> @@ -1102,42 +1104,39 @@ static int rk_udphy_dp_phy_power_off(struct phy *phy)
>> return 0;
>> }
>>
>> -static int rk_udphy_dp_phy_verify_link_rate(unsigned int link_rate)
>> -{
>> - switch (link_rate) {
>> - case 1620:
>> - case 2700:
>> - case 5400:
>> - case 8100:
>> - break;
>> -
>> - default:
>> - return -EINVAL;
>> - }
>> -
>> - return 0;
>> -}
>> -
>> static int rk_udphy_dp_phy_verify_config(struct rk_udphy *udphy,
>> struct phy_configure_opts_dp *dp)
>> {
>> - int i, ret;
>> + int i;
>>
>> - /* If changing link rate was required, verify it's supported. */
>> - ret = rk_udphy_dp_phy_verify_link_rate(dp->link_rate);
>> - if (ret)
>> - return ret;
>> + /* Verify link rate. */
>> + if (dp->set_rate) {
>> + switch (dp->link_rate) {
>> + case 1620:
>> + case 2700:
>> + case 5400:
>> + case 8100:
>> + udphy->link_rate = dp->link_rate;
>> + break;
>> +
>> + default:
>> + return -EINVAL;
>> + }
>
>why drop helper? Why not set the rate on success?
I drop helper just because the flowing check for lanes and votages don't
have helper.
The rate is set in rk_udphy_dp_phy_configure.
>
>> + }
>>
>> /* Verify lane count. */
>> - switch (dp->lanes) {
>> - case 1:
>> - case 2:
>> - case 4:
>> - /* valid lane count. */
>> - break;
>> + if (dp->set_lanes) {
>> + switch (dp->lanes) {
>> + case 1:
>> + case 2:
>> + case 4:
>> + /* valid lane count. */
>> + udphy->lanes = dp->lanes;
>> + break;
>>
>> - default:
>> - return -EINVAL;
>> + default:
>> + return -EINVAL;
>> + }
>
>another change where helper would have made this look better
Do you mean we should do it like this:
static int rk_udphy_dp_phy_configure(struct phy *phy,
union phy_configure_opts *opts)
{
struct rk_udphy *udphy = phy_get_drvdata(phy);
struct phy_configure_opts_dp *dp = &opts->dp;
u32 i, val, lane;
int ret;
.............
if (dp->set_rate)
ret = rk_udphy_dp_phy_verify_link_rate(dp->link_rate);
if (dp->set_lanes)
ret = rk_udphy_dp_phy_verify_link_lanes(dp->lanes);
if (dp->set_voltates)
ret = rk_udphy_dp_phy_verify_link_voltate(dp->voltate);
Add helper for each of them ?
>
>> }
>>
>> /*
>> @@ -1146,7 +1145,7 @@ static int rk_udphy_dp_phy_verify_config(struct rk_udphy *udphy,
>> */
>> if (dp->set_voltages) {
>> /* Lane count verified previously. */
>> - for (i = 0; i < dp->lanes; i++) {
>> + for (i = 0; i < udphy->lanes; i++) {
>> if (dp->voltage[i] > 3 || dp->pre[i] > 3)
>> return -EINVAL;
>>
>> @@ -1243,9 +1242,9 @@ static int rk_udphy_dp_phy_configure(struct phy *phy,
>> }
>>
>> if (dp->set_voltages) {
>> - for (i = 0; i < dp->lanes; i++) {
>> + for (i = 0; i < udphy->lanes; i++) {
>> lane = udphy->dp_lane_sel[i];
>> - switch (dp->link_rate) {
>> + switch (udphy->link_rate) {
>> case 1620:
>> case 2700:
>> regmap_update_bits(udphy->pma_regmap,
>> --
>> 2.34.1
>
>--
>~Vinod
>
>_______________________________________________
>Linux-rockchip mailing list
>Linux-rockchip@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 15+ messages in thread* Re:Re: [PATCH] phy: rockchip: usbdp: Check these parameters only when the corresponding set flags are set
@ 2025-03-11 0:52 ` Andy Yan
0 siblings, 0 replies; 15+ messages in thread
From: Andy Yan @ 2025-03-11 0:52 UTC (permalink / raw)
To: Vinod Koul
Cc: heiko, kishon, sebastian.reichel, yubing.zhang, dmitry.baryshkov,
frank.wang, linux-arm-kernel, linux-kernel, linux-phy,
linux-rockchip, Andy Yan
Hi Vinod,
At 2025-03-10 15:26:50, "Vinod Koul" <vkoul@kernel.org> wrote:
>On 06-03-25, 14:59, Andy Yan wrote:
>> From: Andy Yan <andy.yan@rock-chips.com>
>>
>> According documentation of phy_configure_opts_dp, at the configure
>> stage, we should only verify/configure the link_rate when set_rate
>> flag is set, the same applies to lanes and voltage.
>>
>> So we do it as the documentation says, also record the link rate
>> and lanes in phy internal for set_voltate stage.
>
>Whenever you say also, that is a sign that it should be another patch!
This patch is inspired by Dmitry's suggestion for DP/eDP driver which calls phy_configure[0][1]:
the phy driver should skip the values for which the .set_foo isn't set. So we should not set rates and
lanes when we set_voltate, but the rates and lanes are needed for voltage set, this is why I record
rates and lanes in this patch.
I can split the record to another patch, but then this patch will not work correctly itself,
I'm not sure if this is suitable.
[0]https://patchwork.freedesktop.org/patch/629780/?series=141829&rev=3
[1]https://patchwork.freedesktop.org/patch/638961/?series=145286&rev=1
>
>>
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>> ---
>>
>> drivers/phy/rockchip/phy-rockchip-usbdp.c | 63 +++++++++++------------
>> 1 file changed, 31 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
>> index c04cf64f8a35..d1bbdf382aa2 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
>> @@ -187,6 +187,8 @@ struct rk_udphy {
>> u32 dp_aux_din_sel;
>> bool dp_sink_hpd_sel;
>> bool dp_sink_hpd_cfg;
>> + unsigned int link_rate;
>> + unsigned int lanes;
>> u8 bw;
>> int id;
>>
>> @@ -1102,42 +1104,39 @@ static int rk_udphy_dp_phy_power_off(struct phy *phy)
>> return 0;
>> }
>>
>> -static int rk_udphy_dp_phy_verify_link_rate(unsigned int link_rate)
>> -{
>> - switch (link_rate) {
>> - case 1620:
>> - case 2700:
>> - case 5400:
>> - case 8100:
>> - break;
>> -
>> - default:
>> - return -EINVAL;
>> - }
>> -
>> - return 0;
>> -}
>> -
>> static int rk_udphy_dp_phy_verify_config(struct rk_udphy *udphy,
>> struct phy_configure_opts_dp *dp)
>> {
>> - int i, ret;
>> + int i;
>>
>> - /* If changing link rate was required, verify it's supported. */
>> - ret = rk_udphy_dp_phy_verify_link_rate(dp->link_rate);
>> - if (ret)
>> - return ret;
>> + /* Verify link rate. */
>> + if (dp->set_rate) {
>> + switch (dp->link_rate) {
>> + case 1620:
>> + case 2700:
>> + case 5400:
>> + case 8100:
>> + udphy->link_rate = dp->link_rate;
>> + break;
>> +
>> + default:
>> + return -EINVAL;
>> + }
>
>why drop helper? Why not set the rate on success?
I drop helper just because the flowing check for lanes and votages don't
have helper.
The rate is set in rk_udphy_dp_phy_configure.
>
>> + }
>>
>> /* Verify lane count. */
>> - switch (dp->lanes) {
>> - case 1:
>> - case 2:
>> - case 4:
>> - /* valid lane count. */
>> - break;
>> + if (dp->set_lanes) {
>> + switch (dp->lanes) {
>> + case 1:
>> + case 2:
>> + case 4:
>> + /* valid lane count. */
>> + udphy->lanes = dp->lanes;
>> + break;
>>
>> - default:
>> - return -EINVAL;
>> + default:
>> + return -EINVAL;
>> + }
>
>another change where helper would have made this look better
Do you mean we should do it like this:
static int rk_udphy_dp_phy_configure(struct phy *phy,
union phy_configure_opts *opts)
{
struct rk_udphy *udphy = phy_get_drvdata(phy);
struct phy_configure_opts_dp *dp = &opts->dp;
u32 i, val, lane;
int ret;
.............
if (dp->set_rate)
ret = rk_udphy_dp_phy_verify_link_rate(dp->link_rate);
if (dp->set_lanes)
ret = rk_udphy_dp_phy_verify_link_lanes(dp->lanes);
if (dp->set_voltates)
ret = rk_udphy_dp_phy_verify_link_voltate(dp->voltate);
Add helper for each of them ?
>
>> }
>>
>> /*
>> @@ -1146,7 +1145,7 @@ static int rk_udphy_dp_phy_verify_config(struct rk_udphy *udphy,
>> */
>> if (dp->set_voltages) {
>> /* Lane count verified previously. */
>> - for (i = 0; i < dp->lanes; i++) {
>> + for (i = 0; i < udphy->lanes; i++) {
>> if (dp->voltage[i] > 3 || dp->pre[i] > 3)
>> return -EINVAL;
>>
>> @@ -1243,9 +1242,9 @@ static int rk_udphy_dp_phy_configure(struct phy *phy,
>> }
>>
>> if (dp->set_voltages) {
>> - for (i = 0; i < dp->lanes; i++) {
>> + for (i = 0; i < udphy->lanes; i++) {
>> lane = udphy->dp_lane_sel[i];
>> - switch (dp->link_rate) {
>> + switch (udphy->link_rate) {
>> case 1620:
>> case 2700:
>> regmap_update_bits(udphy->pma_regmap,
>> --
>> 2.34.1
>
>--
>~Vinod
>
>_______________________________________________
>Linux-rockchip mailing list
>Linux-rockchip@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-rockchip
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 15+ messages in thread* Re:Re: [PATCH] phy: rockchip: usbdp: Check these parameters only when the corresponding set flags are set
@ 2025-03-11 0:52 ` Andy Yan
0 siblings, 0 replies; 15+ messages in thread
From: Andy Yan @ 2025-03-11 0:52 UTC (permalink / raw)
To: Vinod Koul
Cc: heiko, kishon, sebastian.reichel, yubing.zhang, dmitry.baryshkov,
frank.wang, linux-arm-kernel, linux-kernel, linux-phy,
linux-rockchip, Andy Yan
Hi Vinod,
At 2025-03-10 15:26:50, "Vinod Koul" <vkoul@kernel.org> wrote:
>On 06-03-25, 14:59, Andy Yan wrote:
>> From: Andy Yan <andy.yan@rock-chips.com>
>>
>> According documentation of phy_configure_opts_dp, at the configure
>> stage, we should only verify/configure the link_rate when set_rate
>> flag is set, the same applies to lanes and voltage.
>>
>> So we do it as the documentation says, also record the link rate
>> and lanes in phy internal for set_voltate stage.
>
>Whenever you say also, that is a sign that it should be another patch!
This patch is inspired by Dmitry's suggestion for DP/eDP driver which calls phy_configure[0][1]:
the phy driver should skip the values for which the .set_foo isn't set. So we should not set rates and
lanes when we set_voltate, but the rates and lanes are needed for voltage set, this is why I record
rates and lanes in this patch.
I can split the record to another patch, but then this patch will not work correctly itself,
I'm not sure if this is suitable.
[0]https://patchwork.freedesktop.org/patch/629780/?series=141829&rev=3
[1]https://patchwork.freedesktop.org/patch/638961/?series=145286&rev=1
>
>>
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>> ---
>>
>> drivers/phy/rockchip/phy-rockchip-usbdp.c | 63 +++++++++++------------
>> 1 file changed, 31 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
>> index c04cf64f8a35..d1bbdf382aa2 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
>> @@ -187,6 +187,8 @@ struct rk_udphy {
>> u32 dp_aux_din_sel;
>> bool dp_sink_hpd_sel;
>> bool dp_sink_hpd_cfg;
>> + unsigned int link_rate;
>> + unsigned int lanes;
>> u8 bw;
>> int id;
>>
>> @@ -1102,42 +1104,39 @@ static int rk_udphy_dp_phy_power_off(struct phy *phy)
>> return 0;
>> }
>>
>> -static int rk_udphy_dp_phy_verify_link_rate(unsigned int link_rate)
>> -{
>> - switch (link_rate) {
>> - case 1620:
>> - case 2700:
>> - case 5400:
>> - case 8100:
>> - break;
>> -
>> - default:
>> - return -EINVAL;
>> - }
>> -
>> - return 0;
>> -}
>> -
>> static int rk_udphy_dp_phy_verify_config(struct rk_udphy *udphy,
>> struct phy_configure_opts_dp *dp)
>> {
>> - int i, ret;
>> + int i;
>>
>> - /* If changing link rate was required, verify it's supported. */
>> - ret = rk_udphy_dp_phy_verify_link_rate(dp->link_rate);
>> - if (ret)
>> - return ret;
>> + /* Verify link rate. */
>> + if (dp->set_rate) {
>> + switch (dp->link_rate) {
>> + case 1620:
>> + case 2700:
>> + case 5400:
>> + case 8100:
>> + udphy->link_rate = dp->link_rate;
>> + break;
>> +
>> + default:
>> + return -EINVAL;
>> + }
>
>why drop helper? Why not set the rate on success?
I drop helper just because the flowing check for lanes and votages don't
have helper.
The rate is set in rk_udphy_dp_phy_configure.
>
>> + }
>>
>> /* Verify lane count. */
>> - switch (dp->lanes) {
>> - case 1:
>> - case 2:
>> - case 4:
>> - /* valid lane count. */
>> - break;
>> + if (dp->set_lanes) {
>> + switch (dp->lanes) {
>> + case 1:
>> + case 2:
>> + case 4:
>> + /* valid lane count. */
>> + udphy->lanes = dp->lanes;
>> + break;
>>
>> - default:
>> - return -EINVAL;
>> + default:
>> + return -EINVAL;
>> + }
>
>another change where helper would have made this look better
Do you mean we should do it like this:
static int rk_udphy_dp_phy_configure(struct phy *phy,
union phy_configure_opts *opts)
{
struct rk_udphy *udphy = phy_get_drvdata(phy);
struct phy_configure_opts_dp *dp = &opts->dp;
u32 i, val, lane;
int ret;
.............
if (dp->set_rate)
ret = rk_udphy_dp_phy_verify_link_rate(dp->link_rate);
if (dp->set_lanes)
ret = rk_udphy_dp_phy_verify_link_lanes(dp->lanes);
if (dp->set_voltates)
ret = rk_udphy_dp_phy_verify_link_voltate(dp->voltate);
Add helper for each of them ?
>
>> }
>>
>> /*
>> @@ -1146,7 +1145,7 @@ static int rk_udphy_dp_phy_verify_config(struct rk_udphy *udphy,
>> */
>> if (dp->set_voltages) {
>> /* Lane count verified previously. */
>> - for (i = 0; i < dp->lanes; i++) {
>> + for (i = 0; i < udphy->lanes; i++) {
>> if (dp->voltage[i] > 3 || dp->pre[i] > 3)
>> return -EINVAL;
>>
>> @@ -1243,9 +1242,9 @@ static int rk_udphy_dp_phy_configure(struct phy *phy,
>> }
>>
>> if (dp->set_voltages) {
>> - for (i = 0; i < dp->lanes; i++) {
>> + for (i = 0; i < udphy->lanes; i++) {
>> lane = udphy->dp_lane_sel[i];
>> - switch (dp->link_rate) {
>> + switch (udphy->link_rate) {
>> case 1620:
>> case 2700:
>> regmap_update_bits(udphy->pma_regmap,
>> --
>> 2.34.1
>
>--
>~Vinod
>
>_______________________________________________
>Linux-rockchip mailing list
>Linux-rockchip@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/linux-rockchip
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: Re: [PATCH] phy: rockchip: usbdp: Check these parameters only when the corresponding set flags are set
2025-03-11 0:52 ` Andy Yan
(?)
@ 2025-03-11 11:13 ` Vinod Koul
-1 siblings, 0 replies; 15+ messages in thread
From: Vinod Koul @ 2025-03-11 11:13 UTC (permalink / raw)
To: Andy Yan
Cc: heiko, kishon, sebastian.reichel, yubing.zhang, dmitry.baryshkov,
frank.wang, linux-arm-kernel, linux-kernel, linux-phy,
linux-rockchip, Andy Yan
On 11-03-25, 08:52, Andy Yan wrote:
> Do you mean we should do it like this:
>
> static int rk_udphy_dp_phy_configure(struct phy *phy,
> union phy_configure_opts *opts)
> {
> struct rk_udphy *udphy = phy_get_drvdata(phy);
> struct phy_configure_opts_dp *dp = &opts->dp;
> u32 i, val, lane;
> int ret;
>
> .............
> if (dp->set_rate)
> ret = rk_udphy_dp_phy_verify_link_rate(dp->link_rate);
> if (dp->set_lanes)
> ret = rk_udphy_dp_phy_verify_link_lanes(dp->lanes);
> if (dp->set_voltates)
> ret = rk_udphy_dp_phy_verify_link_voltate(dp->voltate);
>
>
> Add helper for each of them ?
That would look better for sure
--
~Vinod
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: Re: [PATCH] phy: rockchip: usbdp: Check these parameters only when the corresponding set flags are set
@ 2025-03-11 11:13 ` Vinod Koul
0 siblings, 0 replies; 15+ messages in thread
From: Vinod Koul @ 2025-03-11 11:13 UTC (permalink / raw)
To: Andy Yan
Cc: heiko, kishon, sebastian.reichel, yubing.zhang, dmitry.baryshkov,
frank.wang, linux-arm-kernel, linux-kernel, linux-phy,
linux-rockchip, Andy Yan
On 11-03-25, 08:52, Andy Yan wrote:
> Do you mean we should do it like this:
>
> static int rk_udphy_dp_phy_configure(struct phy *phy,
> union phy_configure_opts *opts)
> {
> struct rk_udphy *udphy = phy_get_drvdata(phy);
> struct phy_configure_opts_dp *dp = &opts->dp;
> u32 i, val, lane;
> int ret;
>
> .............
> if (dp->set_rate)
> ret = rk_udphy_dp_phy_verify_link_rate(dp->link_rate);
> if (dp->set_lanes)
> ret = rk_udphy_dp_phy_verify_link_lanes(dp->lanes);
> if (dp->set_voltates)
> ret = rk_udphy_dp_phy_verify_link_voltate(dp->voltate);
>
>
> Add helper for each of them ?
That would look better for sure
--
~Vinod
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: Re: [PATCH] phy: rockchip: usbdp: Check these parameters only when the corresponding set flags are set
@ 2025-03-11 11:13 ` Vinod Koul
0 siblings, 0 replies; 15+ messages in thread
From: Vinod Koul @ 2025-03-11 11:13 UTC (permalink / raw)
To: Andy Yan
Cc: heiko, kishon, sebastian.reichel, yubing.zhang, dmitry.baryshkov,
frank.wang, linux-arm-kernel, linux-kernel, linux-phy,
linux-rockchip, Andy Yan
On 11-03-25, 08:52, Andy Yan wrote:
> Do you mean we should do it like this:
>
> static int rk_udphy_dp_phy_configure(struct phy *phy,
> union phy_configure_opts *opts)
> {
> struct rk_udphy *udphy = phy_get_drvdata(phy);
> struct phy_configure_opts_dp *dp = &opts->dp;
> u32 i, val, lane;
> int ret;
>
> .............
> if (dp->set_rate)
> ret = rk_udphy_dp_phy_verify_link_rate(dp->link_rate);
> if (dp->set_lanes)
> ret = rk_udphy_dp_phy_verify_link_lanes(dp->lanes);
> if (dp->set_voltates)
> ret = rk_udphy_dp_phy_verify_link_voltate(dp->voltate);
>
>
> Add helper for each of them ?
That would look better for sure
--
~Vinod
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 15+ messages in thread* Re:Re: Re: [PATCH] phy: rockchip: usbdp: Check these parameters only when the corresponding set flags are set
2025-03-11 11:13 ` Vinod Koul
(?)
@ 2025-03-11 12:22 ` Andy Yan
-1 siblings, 0 replies; 15+ messages in thread
From: Andy Yan @ 2025-03-11 12:22 UTC (permalink / raw)
To: Vinod Koul
Cc: heiko, kishon, sebastian.reichel, yubing.zhang, dmitry.baryshkov,
frank.wang, linux-arm-kernel, linux-kernel, linux-phy,
linux-rockchip, Andy Yan
Hi Vinod,
At 2025-03-11 19:13:46, "Vinod Koul" <vkoul@kernel.org> wrote:
>On 11-03-25, 08:52, Andy Yan wrote:
>
>> Do you mean we should do it like this:
>>
>> static int rk_udphy_dp_phy_configure(struct phy *phy,
>> union phy_configure_opts *opts)
>> {
>> struct rk_udphy *udphy = phy_get_drvdata(phy);
>> struct phy_configure_opts_dp *dp = &opts->dp;
>> u32 i, val, lane;
>> int ret;
>>
>> .............
>> if (dp->set_rate)
>> ret = rk_udphy_dp_phy_verify_link_rate(dp->link_rate);
>> if (dp->set_lanes)
>> ret = rk_udphy_dp_phy_verify_link_lanes(dp->lanes);
>> if (dp->set_voltates)
>> ret = rk_udphy_dp_phy_verify_link_voltate(dp->voltate);
>>
>>
>> Add helper for each of them ?
>
>That would look better for sure
I will do as that in V2.
Thanks
>
>--
>~Vinod
^ permalink raw reply [flat|nested] 15+ messages in thread* Re:Re: Re: [PATCH] phy: rockchip: usbdp: Check these parameters only when the corresponding set flags are set
@ 2025-03-11 12:22 ` Andy Yan
0 siblings, 0 replies; 15+ messages in thread
From: Andy Yan @ 2025-03-11 12:22 UTC (permalink / raw)
To: Vinod Koul
Cc: heiko, kishon, sebastian.reichel, yubing.zhang, dmitry.baryshkov,
frank.wang, linux-arm-kernel, linux-kernel, linux-phy,
linux-rockchip, Andy Yan
Hi Vinod,
At 2025-03-11 19:13:46, "Vinod Koul" <vkoul@kernel.org> wrote:
>On 11-03-25, 08:52, Andy Yan wrote:
>
>> Do you mean we should do it like this:
>>
>> static int rk_udphy_dp_phy_configure(struct phy *phy,
>> union phy_configure_opts *opts)
>> {
>> struct rk_udphy *udphy = phy_get_drvdata(phy);
>> struct phy_configure_opts_dp *dp = &opts->dp;
>> u32 i, val, lane;
>> int ret;
>>
>> .............
>> if (dp->set_rate)
>> ret = rk_udphy_dp_phy_verify_link_rate(dp->link_rate);
>> if (dp->set_lanes)
>> ret = rk_udphy_dp_phy_verify_link_lanes(dp->lanes);
>> if (dp->set_voltates)
>> ret = rk_udphy_dp_phy_verify_link_voltate(dp->voltate);
>>
>>
>> Add helper for each of them ?
>
>That would look better for sure
I will do as that in V2.
Thanks
>
>--
>~Vinod
_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip
^ permalink raw reply [flat|nested] 15+ messages in thread* Re:Re: Re: [PATCH] phy: rockchip: usbdp: Check these parameters only when the corresponding set flags are set
@ 2025-03-11 12:22 ` Andy Yan
0 siblings, 0 replies; 15+ messages in thread
From: Andy Yan @ 2025-03-11 12:22 UTC (permalink / raw)
To: Vinod Koul
Cc: heiko, kishon, sebastian.reichel, yubing.zhang, dmitry.baryshkov,
frank.wang, linux-arm-kernel, linux-kernel, linux-phy,
linux-rockchip, Andy Yan
Hi Vinod,
At 2025-03-11 19:13:46, "Vinod Koul" <vkoul@kernel.org> wrote:
>On 11-03-25, 08:52, Andy Yan wrote:
>
>> Do you mean we should do it like this:
>>
>> static int rk_udphy_dp_phy_configure(struct phy *phy,
>> union phy_configure_opts *opts)
>> {
>> struct rk_udphy *udphy = phy_get_drvdata(phy);
>> struct phy_configure_opts_dp *dp = &opts->dp;
>> u32 i, val, lane;
>> int ret;
>>
>> .............
>> if (dp->set_rate)
>> ret = rk_udphy_dp_phy_verify_link_rate(dp->link_rate);
>> if (dp->set_lanes)
>> ret = rk_udphy_dp_phy_verify_link_lanes(dp->lanes);
>> if (dp->set_voltates)
>> ret = rk_udphy_dp_phy_verify_link_voltate(dp->voltate);
>>
>>
>> Add helper for each of them ?
>
>That would look better for sure
I will do as that in V2.
Thanks
>
>--
>~Vinod
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply [flat|nested] 15+ messages in thread