| Message ID | 20230522113331.36872-3-Parthiban.Veerasooran@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | microchip_t1s: Update on Microchip 10BASE-T1S PHY driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 48 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
> > /* Read-Modified Write Pseudocode (from AN1699) > * current_val = read_register(mmd, addr) // Read current register value Hi Parthiban Maybe extend the comment to indicate that although AN1699 says Read, Modify, Write, the write is not required if the register already has the required value. That is what phy_modify_mmd() actually does. > @@ -74,12 +72,11 @@ static int lan867x_config_init(struct phy_device *phydev) > * write_register(mmd, addr, new_val) // Write back updated register value > */ > for (int i = 0; i < ARRAY_SIZE(lan867x_fixup_registers); i++) { > - reg = lan867x_fixup_registers[i]; > - reg_value = phy_read_mmd(phydev, MDIO_MMD_VEND2, reg); > - reg_value &= ~lan867x_fixup_masks[i]; > - reg_value |= lan867x_fixup_values[i]; > - err = phy_write_mmd(phydev, MDIO_MMD_VEND2, reg, reg_value); > - if (err != 0) > + err = phy_modify_mmd(phydev, MDIO_MMD_VEND2, > + lan867x_fixup_registers[i], > + lan867x_fixup_masks[i], > + lan867x_fixup_values[i]); > + if (err) > return err; > } Andrew --- pw-bot: cr
Hi Andrew, On 22/05/23 6:01 pm, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> >> /* Read-Modified Write Pseudocode (from AN1699) >> * current_val = read_register(mmd, addr) // Read current register value > > Hi Parthiban > > Maybe extend the comment to indicate that although AN1699 says Read, > Modify, Write, the write is not required if the register already has > the required value. That is what phy_modify_mmd() actually does. Sure, I will add it in the next version. Best Regards, Parthiban V > >> @@ -74,12 +72,11 @@ static int lan867x_config_init(struct phy_device *phydev) >> * write_register(mmd, addr, new_val) // Write back updated register value >> */ >> for (int i = 0; i < ARRAY_SIZE(lan867x_fixup_registers); i++) { >> - reg = lan867x_fixup_registers[i]; >> - reg_value = phy_read_mmd(phydev, MDIO_MMD_VEND2, reg); >> - reg_value &= ~lan867x_fixup_masks[i]; >> - reg_value |= lan867x_fixup_values[i]; >> - err = phy_write_mmd(phydev, MDIO_MMD_VEND2, reg, reg_value); >> - if (err != 0) >> + err = phy_modify_mmd(phydev, MDIO_MMD_VEND2, >> + lan867x_fixup_registers[i], >> + lan867x_fixup_masks[i], >> + lan867x_fixup_values[i]); >> + if (err) >> return err; >> } > > > Andrew > > --- > pw-bot: cr
diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c index a42a6bb6e3bd..534d9faf8475 100644 --- a/drivers/net/phy/microchip_t1s.c +++ b/drivers/net/phy/microchip_t1s.c @@ -31,19 +31,19 @@ * W 0x1F 0x0099 0x7F80 ------ */ -static const int lan867x_fixup_registers[12] = { +static const u32 lan867x_fixup_registers[12] = { 0x00D0, 0x00D1, 0x0084, 0x0085, 0x008A, 0x0087, 0x0088, 0x008B, 0x0080, 0x00F1, 0x0096, 0x0099, }; -static const int lan867x_fixup_values[12] = { +static const u16 lan867x_fixup_values[12] = { 0x0002, 0x0000, 0x3380, 0x0006, 0xC000, 0x801C, 0x033F, 0x0404, 0x0600, 0x2400, 0x2000, 0x7F80, }; -static const int lan867x_fixup_masks[12] = { +static const u16 lan867x_fixup_masks[12] = { 0x0E03, 0x0300, 0xFFC0, 0x000F, 0xF800, 0x801C, 0x1FFF, 0xFFFF, 0x0600, 0x7F00, 0x2000, 0xFFFF, @@ -63,9 +63,7 @@ static int lan867x_config_init(struct phy_device *phydev) * used, which might then write the same value back as read + modified. */ - int reg_value; int err; - int reg; /* Read-Modified Write Pseudocode (from AN1699) * current_val = read_register(mmd, addr) // Read current register value @@ -74,12 +72,11 @@ static int lan867x_config_init(struct phy_device *phydev) * write_register(mmd, addr, new_val) // Write back updated register value */ for (int i = 0; i < ARRAY_SIZE(lan867x_fixup_registers); i++) { - reg = lan867x_fixup_registers[i]; - reg_value = phy_read_mmd(phydev, MDIO_MMD_VEND2, reg); - reg_value &= ~lan867x_fixup_masks[i]; - reg_value |= lan867x_fixup_values[i]; - err = phy_write_mmd(phydev, MDIO_MMD_VEND2, reg, reg_value); - if (err != 0) + err = phy_modify_mmd(phydev, MDIO_MMD_VEND2, + lan867x_fixup_registers[i], + lan867x_fixup_masks[i], + lan867x_fixup_values[i]); + if (err) return err; }
Replace read-modify-write code in the lan867x_config_init function to avoid handling data type mismatch and to simplify the code. Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com> --- drivers/net/phy/microchip_t1s.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)