> + if (phydev->drv->led_polarity_set) { > + if (of_property_read_bool(led, "active-low")) > + set_bit(PHY_LED_ACTIVE_LOW, &modes); > + if (of_property_read_bool(led, "inactive-high-impedance")) > + set_bit(PHY_LED_INACTIVE_HIGH_IMPEDANCE, &modes); > + > + err = phydev->drv->led_polarity_set(phydev, index, modes); > + if (err) > + return err; > + } I think we should return an error if asked to set the mode, but its not implemented by the driver. Something like: if (of_property_read_bool(led, "active-low")) set_bit(PHY_LED_ACTIVE_LOW, &modes); if (of_property_read_bool(led, "inactive-high-impedance")) set_bit(PHY_LED_INACTIVE_HIGH_IMPEDANCE, &modes); if (mode) if (phydev->drv->led_polarity_set) { return -EINVAL; } else { err = phydev->drv->led_polarity_set(phydev, index, modes); if (err) return err; } } > + /** > + * @led_polarity_set: Set the LED polarity if active low The 'if active low' is not ouw of date, since it is used for more than that. > + * @dev: PHY device which has the LED > + * @index: Which LED of the PHY device or -1 > + * @modes: bitmap of LED polarity modes > + * > + * Configure LED with all the required polarity modes in @modes > + * to make it correctly turn ON or OFF. index == -1 should be explained. > + * > + * Returns 0, or an error code. > + */ > + int (*led_polarity_set)(struct phy_device *dev, int index, > + unsigned long modes); Andrew --- pw-bot: cr