Re: [PATCH v2 3/6] net: phy: DP83640: Add LED handling

Bastien Curutchet
2024-02-29T07:28:54Z

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