Messages in this thread | | | Date | Thu, 29 Feb 2024 08:28:51 +0100 | Subject | Re: [PATCH v2 3/6] net: phy: DP83640: Add LED handling | From | Bastien Curutchet <> |
| |
Hi
On 2/28/24 16:04, Andrew Lunn wrote: >> +static int dp83640_led_brightness_set(struct phy_device *phydev, u8 index, >> + enum led_brightness brightness) >> +{ >> + int val; >> + >> + if (index > DP83640_SPDLED_IDX) >> + return -EINVAL; >> + >> + phy_write(phydev, PAGESEL, 0); >> + >> + val = phy_read(phydev, LEDCR) & ~DP83640_LED_DIS(index); >> + val |= DP83640_LED_DRV(index); >> + if (brightness) >> + val |= DP83640_LED_VAL(index); >> + else >> + val &= ~DP83640_LED_VAL(index); >> + phy_write(phydev, LEDCR, val); > I don't understand this driver too well, but should this be using > ext_read() and ext_write(). > > I'm also woundering if it would be good to implement the .read_page > and .write_page members in phy_driver and then use phy_write_paged() > and phy_write_paged() and phy_modify_paged(), which should do better > locking. I don't feel comfortable implementing .read_page and write_page members as I have only one PHY on my board so I'll not be able to test all the broadcast thing.
If that's OK with you, I'll use the ext_read() and ext_write()
Best regards,
Bastien
| |