diff mbox series

[net-next,v8,3/5] net: phy: add support for PHY LEDs polarity modes

Message ID 20240104110114.2020-4-ansuelsmth@gmail.com
State New
Headers
Series None |

Commit Message

Christian Marangi Jan. 4, 2024, 11:01 a.m. UTC 
  Add support for PHY LEDs polarity modes. Some PHY require LED to be set
to active low to be turned ON. Adds support for this by declaring
active-low property in DT.

PHY driver needs to declare .led_polarity_set() to configure LED
polarity modes. Function will pass the index with the LED index and a
bitmap with all the required modes to set.

Current supported modes are:
- active-low with the flag PHY_LED_ACTIVE_LOW. LED is set to active-low
  to turn it ON.
- inactive-high-impedance with the flag PHY_LED_INACTIVE_HIGH_IMPEDANCE.
  LED is set to high impedance to turn it OFF.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
Changes v5:
- Rework to LED modes bitmap
Changes v4:
- Drop for global active-low
- Rework to polarity option (for marvell10g series support)
Changes v3:
- Out of RFC
Changes v2:
- Add this patch

 drivers/net/phy/phy_device.c | 12 ++++++++++++
 include/linux/phy.h          | 22 ++++++++++++++++++++++
 2 files changed, 34 insertions(+)

Comments

Andrew Lunn Jan. 4, 2024, 1:20 p.m. UTC | #1 
> +	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
Christian Marangi Jan. 4, 2024, 9:42 p.m. UTC | #2 
On Thu, Jan 04, 2024 at 02:20:45PM +0100, Andrew Lunn wrote:
> > +	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.
>

If you are referring to the special way of setting the LED globally,
that is not a thing anymore. Rob pointed out that having the double
reference in DT is problematic and PHY driver should handle that
internally.

> > +	 *
> > +	 * Returns 0, or an error code.
> > +	 */
> > +	int (*led_polarity_set)(struct phy_device *dev, int index,
> > +				unsigned long modes);
> 
> 
>     Andrew
> 
> ---
> pw-bot: cr
Andrew Lunn Jan. 5, 2024, 2:25 a.m. UTC | #3 
> > > +	 * @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.
> >
> 
> If you are referring to the special way of setting the LED globally,
> that is not a thing anymore. Rob pointed out that having the double
> reference in DT is problematic and PHY driver should handle that
> internally.

So it sounds like you need to change

> > > +	 * @index: Which LED of the PHY device or -1

to remove the 'or -1'

   Andrew
diff mbox series

Patch

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1e595762afea..437a1c6046ab 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3152,6 +3152,7 @@  static int of_phy_led(struct phy_device *phydev,
 	struct device *dev = &phydev->mdio.dev;
 	struct led_init_data init_data = {};
 	struct led_classdev *cdev;
+	unsigned long modes = 0;
 	struct phy_led *phyled;
 	u32 index;
 	int err;
@@ -3169,6 +3170,17 @@  static int of_phy_led(struct phy_device *phydev,
 	if (index > U8_MAX)
 		return -EINVAL;
 
+	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;
+	}
+
 	phyled->index = index;
 	if (phydev->drv->led_brightness_set)
 		cdev->brightness_set_blocking = phy_led_set_brightness;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index ac22b8e28a85..da0a15a6d638 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -860,6 +860,15 @@  struct phy_plca_status {
 	bool pst;
 };
 
+/* Modes for PHY LED configuration */
+enum phy_led_modes {
+	PHY_LED_ACTIVE_LOW = 0,
+	PHY_LED_INACTIVE_HIGH_IMPEDANCE = 1,
+
+	/* keep it last */
+	__PHY_LED_MODES_NUM,
+};
+
 /**
  * struct phy_led: An LED driven by the PHY
  *
@@ -1153,6 +1162,19 @@  struct phy_driver {
 	int (*led_hw_control_get)(struct phy_device *dev, u8 index,
 				  unsigned long *rules);
 
+	/**
+	 * @led_polarity_set: Set the LED polarity if active low
+	 * @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.
+	 *
+	 * Returns 0, or an error code.
+	 */
+	int (*led_polarity_set)(struct phy_device *dev, int index,
+				unsigned long modes);
 };
 #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
 				      struct phy_driver, mdiodrv)