| Message ID | 743a1fd7-e1b2-d548-1c22-7c1a2e3b268e@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] r8169: add new helper rtl8168g_enable_gphy_10m | expand |
On Sat, 2020-03-21 at 19:08 +0100, Heiner Kallweit wrote: > Factor out setting GPHY 10M to new helper rtl8168g_enable_gphy_10m. [] > diff --git a/drivers/net/ethernet/realtek/r8169_phy_config.c b/drivers/net/ethernet/realtek/r8169_phy_config.c [] > @@ -796,6 +796,11 @@ static void rtl8168g_disable_aldps(struct phy_device *phydev) > phy_modify_paged(phydev, 0x0a43, 0x10, BIT(2), 0); > } > > +static void rtl8168g_enable_gphy_10m(struct phy_device *phydev) > +{ > + phy_modify_paged(phydev, 0x0a44, 0x11, 0, BIT(11)); > +} Perhaps this should be some generic to set characteristics like: enum rtl8168g_char { ... } static void rtl8168g_enable_char(struct phy_device *phydev, enum rtl8168g_char type) { switch (type) { case FOO: etc... }
On 21.03.2020 19:55, Joe Perches wrote: > On Sat, 2020-03-21 at 19:08 +0100, Heiner Kallweit wrote: >> Factor out setting GPHY 10M to new helper rtl8168g_enable_gphy_10m. > [] >> diff --git a/drivers/net/ethernet/realtek/r8169_phy_config.c b/drivers/net/ethernet/realtek/r8169_phy_config.c > [] >> @@ -796,6 +796,11 @@ static void rtl8168g_disable_aldps(struct phy_device *phydev) >> phy_modify_paged(phydev, 0x0a43, 0x10, BIT(2), 0); >> } >> >> +static void rtl8168g_enable_gphy_10m(struct phy_device *phydev) >> +{ >> + phy_modify_paged(phydev, 0x0a44, 0x11, 0, BIT(11)); >> +} > > Perhaps this should be some generic to set characteristics like: > > enum rtl8168g_char { > ... > } > > static void rtl8168g_enable_char(struct phy_device *phydev, > enum rtl8168g_char type) > { > switch (type) { > case FOO: > etc... > } > > Then we'd need one more such generic to disable characteristics like in rtl8168g_disable_aldps(). Also we have existing functions like rtl8168g_phy_adjust_10m_aldps() that change a characteristic, and the function name is used to describe what the function does. So yes, it would be an option, but I don't see that we would gain anything. More the opposite, it would add overhead and we'd have a mix of fct_what_it_does() and fct_generic(WHAT_IT_DOES).
From: Heiner Kallweit <hkallweit1@gmail.com> Date: Sat, 21 Mar 2020 19:08:09 +0100 > Factor out setting GPHY 10M to new helper rtl8168g_enable_gphy_10m. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Applied.
diff --git a/drivers/net/ethernet/realtek/r8169_phy_config.c b/drivers/net/ethernet/realtek/r8169_phy_config.c index e367e77c7..b73f7d023 100644 --- a/drivers/net/ethernet/realtek/r8169_phy_config.c +++ b/drivers/net/ethernet/realtek/r8169_phy_config.c @@ -796,6 +796,11 @@ static void rtl8168g_disable_aldps(struct phy_device *phydev) phy_modify_paged(phydev, 0x0a43, 0x10, BIT(2), 0); } +static void rtl8168g_enable_gphy_10m(struct phy_device *phydev) +{ + phy_modify_paged(phydev, 0x0a44, 0x11, 0, BIT(11)); +} + static void rtl8168g_phy_adjust_10m_aldps(struct phy_device *phydev) { phy_modify_paged(phydev, 0x0bcc, 0x14, BIT(8), 0); @@ -904,8 +909,7 @@ static void rtl8168h_1_hw_phy_config(struct rtl8169_private *tp, r8168g_phy_param(phydev, 0x0811, 0x0000, 0x0800); phy_modify_paged(phydev, 0x0a42, 0x16, 0x0000, 0x0002); - /* enable GPHY 10M */ - phy_modify_paged(phydev, 0x0a44, 0x11, 0, BIT(11)); + rtl8168g_enable_gphy_10m(phydev); /* SAR ADC performance */ phy_modify_paged(phydev, 0x0bca, 0x17, BIT(12) | BIT(13), BIT(14)); @@ -940,8 +944,7 @@ static void rtl8168h_2_hw_phy_config(struct rtl8169_private *tp, r8168g_phy_param(phydev, 0x0811, 0x0000, 0x0800); phy_modify_paged(phydev, 0x0a42, 0x16, 0x0000, 0x0002); - /* enable GPHY 10M */ - phy_modify_paged(phydev, 0x0a44, 0x11, 0, BIT(11)); + rtl8168g_enable_gphy_10m(phydev); ioffset = rtl8168h_2_get_adc_bias_ioffset(tp); if (ioffset != 0xffff) @@ -1063,8 +1066,7 @@ static void rtl8117_hw_phy_config(struct rtl8169_private *tp, r8168g_phy_param(phydev, 0x8011, 0x0000, 0x0800); - /* enable GPHY 10M */ - phy_modify_paged(phydev, 0x0a44, 0x11, 0, BIT(11)); + rtl8168g_enable_gphy_10m(phydev); r8168g_phy_param(phydev, 0x8016, 0x0000, 0x0400); @@ -1171,7 +1173,7 @@ static void rtl8125_1_hw_phy_config(struct rtl8169_private *tp, phy_write_paged(phydev, 0xbc3, 0x12, 0x5555); phy_modify_paged(phydev, 0xbf0, 0x15, 0x0e00, 0x0a00); phy_modify_paged(phydev, 0xa5c, 0x10, 0x0400, 0x0000); - phy_modify_paged(phydev, 0xa44, 0x11, 0x0000, 0x0800); + rtl8168g_enable_gphy_10m(phydev); rtl8125_config_eee_phy(phydev); } @@ -1236,7 +1238,7 @@ static void rtl8125_2_hw_phy_config(struct rtl8169_private *tp, phy_modify_paged(phydev, 0xa5d, 0x12, 0x0000, 0x0020); phy_modify_paged(phydev, 0xad4, 0x17, 0x0010, 0x0000); phy_modify_paged(phydev, 0xa86, 0x15, 0x0001, 0x0000); - phy_modify_paged(phydev, 0xa44, 0x11, 0x0000, 0x0800); + rtl8168g_enable_gphy_10m(phydev); rtl8125_config_eee_phy(phydev); }
Factor out setting GPHY 10M to new helper rtl8168g_enable_gphy_10m. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- .../net/ethernet/realtek/r8169_phy_config.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)