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

Andrew Lunn
2024-02-28T15:04:16Z

> +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.

Andrew