diff mbox series

[v7,10/11] net: dsa: qca8k: add LEDs support

Message ID 20221214235438.30271-11-ansuelsmth@gmail.com (mailing list archive)
State Not Applicable
Delegated to: Netdev Maintainers
Headers show
Series Adds support for PHY LEDs with offload triggers | expand

Checks

Context Check Description
netdev/tree_selection success Guessed tree name to be net-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 2 this patch: 8
netdev/cc_maintainers success CCed 9 of 9 maintainers
netdev/build_clang fail Errors and warnings before: 1 this patch: 11
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 2 this patch: 8
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? WARNING: line length of 81 exceeds 80 columns WARNING: line length of 84 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns WARNING: line length of 88 exceeds 80 columns WARNING: line length of 90 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns WARNING: line length of 93 exceeds 80 columns WARNING: please write a help paragraph that fully describes the config symbol
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Christian Marangi Dec. 14, 2022, 11:54 p.m. UTC
Add LEDs support for qca8k Switch Family. This will provide the LEDs
hardware API to permit the PHY LED to support hardware mode.
Each port have at least 3 LEDs and they can HW blink, set on/off or
follow blink modes configured with the LED in hardware mode.
Adds support for leds netdev trigger to support hardware triggers.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca/Kconfig      |   9 +
 drivers/net/dsa/qca/Makefile     |   1 +
 drivers/net/dsa/qca/qca8k-8xxx.c |   4 +
 drivers/net/dsa/qca/qca8k-leds.c | 406 +++++++++++++++++++++++++++++++
 drivers/net/dsa/qca/qca8k.h      |  62 +++++
 5 files changed, 482 insertions(+)
 create mode 100644 drivers/net/dsa/qca/qca8k-leds.c

Comments

kernel test robot Dec. 15, 2022, 3:50 a.m. UTC | #1
Hi Christian,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on pavel-leds/for-next]
[also build test ERROR on robh/for-next net-next/master net/master linus/master v6.1 next-20221214]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Christian-Marangi/Adds-support-for-PHY-LEDs-with-offload-triggers/20221215-080414
base:   git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
patch link:    https://lore.kernel.org/r/20221214235438.30271-11-ansuelsmth%40gmail.com
patch subject: [PATCH v7 10/11] net: dsa: qca8k: add LEDs support
config: sh-allmodconfig
compiler: sh4-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/c3cb4f8cc2edb081c2b31733ed02cb2c022320f1
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Christian-Marangi/Adds-support-for-PHY-LEDs-with-offload-triggers/20221215-080414
        git checkout c3cb4f8cc2edb081c2b31733ed02cb2c022320f1
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sh SHELL=/bin/bash drivers/net/dsa/qca/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/net/dsa/qca/qca8k-leds.c:383:1: error: redefinition of 'qca8k_setup_led_ctrl'
     383 | qca8k_setup_led_ctrl(struct qca8k_priv *priv)
         | ^~~~~~~~~~~~~~~~~~~~
   In file included from drivers/net/dsa/qca/qca8k-leds.c:5:
   drivers/net/dsa/qca/qca8k.h:576:19: note: previous definition of 'qca8k_setup_led_ctrl' with type 'int(struct qca8k_priv *)'
     576 | static inline int qca8k_setup_led_ctrl(struct qca8k_priv *priv)
         |                   ^~~~~~~~~~~~~~~~~~~~


vim +/qca8k_setup_led_ctrl +383 drivers/net/dsa/qca/qca8k-leds.c

   381	
   382	int
 > 383	qca8k_setup_led_ctrl(struct qca8k_priv *priv)
Arun Ramadoss Dec. 15, 2022, 3:54 a.m. UTC | #2
Hi Christian,

On Thu, 2022-12-15 at 00:54 +0100, Christian Marangi wrote:
> Add LEDs support for qca8k Switch Family. This will provide the LEDs
> hardware API to permit the PHY LED to support hardware mode.
> Each port have at least 3 LEDs and they can HW blink, set on/off or
> follow blink modes configured with the LED in hardware mode.
> Adds support for leds netdev trigger to support hardware triggers.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/net/dsa/qca/Kconfig      |   9 +
>  drivers/net/dsa/qca/Makefile     |   1 +
>  drivers/net/dsa/qca/qca8k-8xxx.c |   4 +
>  drivers/net/dsa/qca/qca8k-leds.c | 406
> +++++++++++++++++++++++++++++++
>  drivers/net/dsa/qca/qca8k.h      |  62 +++++
>  5 files changed, 482 insertions(+)
>  create mode 100644 drivers/net/dsa/qca/qca8k-leds.c
> 
> 
> diff --git a/drivers/net/dsa/qca/qca8k-leds.c
> b/drivers/net/dsa/qca/qca8k-leds.c
> new file mode 100644
> index 000000000000..b51cdcae31b2
> --- /dev/null
> +++ b/drivers/net/dsa/qca/qca8k-leds.c
> @@ -0,0 +1,406 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <net/dsa.h>
> +#include <linux/regmap.h>

Alphabatical order

> +
> +#include "qca8k.h"
> +
> +static int
> +qca8k_get_enable_led_reg(int port_num, int led_num, struct
> qca8k_led_pattern_en *reg_info)
> +{
> +	int shift;
> +
> +	switch (port_num) {
> +	case 0:
> +		reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
> +		reg_info->shift = 14;

Any macros other than magic number for 14, 30 

> +		break;
> +	case 1:
> +	case 2:
> +	case 3:
> +		reg_info->reg = QCA8K_LED_CTRL_REG(3);
> +		shift = 2 * led_num + (6 * (port_num - 1));
> +
> +		reg_info->shift = 8 + shift;
> +
> +		break;
> +	case 4:
> +		reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
> +		reg_info->shift = 30;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> 
> +
> +static void
> +qca8k_led_brightness_set(struct qca8k_led *led,
> +			 enum led_brightness b)

variable name other than b to have readability in code

> +{
> +	struct qca8k_led_pattern_en reg_info;
> +	struct qca8k_priv *priv = led->priv;
> +	u32 val = QCA8K_LED_ALWAYS_OFF;
> +
> +	qca8k_get_enable_led_reg(led->port_num, led->led_num,
> &reg_info);
> +
> +	if (b)
> +		val = QCA8K_LED_ALWAYS_ON;
> +
> +	regmap_update_bits(priv->regmap, reg_info.reg,
> +			   GENMASK(1, 0) << reg_info.shift,

GENMASK(1, 0) used in multiple place, replace with macro for
readability

> +			   val << reg_info.shift);
> +}
> +
> 
> +
> +static int
> +qca8k_cled_trigger_offload(struct led_classdev *ldev, bool enable)
> +{
> +	struct qca8k_led *led = container_of(ldev, struct qca8k_led,
> cdev);
> +

Blank line

> +	struct qca8k_led_pattern_en reg_info;
> +	struct qca8k_priv *priv = led->priv;
> +	u32 val = QCA8K_LED_ALWAYS_OFF;
> +
> +	qca8k_get_enable_led_reg(led->port_num, led->led_num,
> &reg_info);
> +
> +	if (enable)
> +		val = QCA8K_LED_RULE_CONTROLLED;
> +
> +	return regmap_update_bits(priv->regmap, reg_info.reg,
> +				  GENMASK(1, 0) << reg_info.shift,
> +				  val << reg_info.shift);
> +}
> +
> 
> +static bool
> +qca8k_cled_hw_control_status(struct led_classdev *ldev)
> +{
> +	struct qca8k_led *led = container_of(ldev, struct qca8k_led,
> cdev);
> +

Blank line

> +	struct qca8k_led_pattern_en reg_info;
> +	struct qca8k_priv *priv = led->priv;
> +	u32 val;
> +
> +	qca8k_get_enable_led_reg(led->port_num, led->led_num,
> &reg_info);
> +
> +	regmap_read(priv->regmap, reg_info.reg, &val);
> +
> +	val >>= reg_info.shift;
> +	val &= GENMASK(1, 0);
> +
> +	return val == QCA8K_LED_RULE_CONTROLLED;
> +}
> +
>
Russell King (Oracle) Dec. 15, 2022, 5:49 p.m. UTC | #3
Hi,

On Thu, Dec 15, 2022 at 12:54:37AM +0100, Christian Marangi wrote:
> +static int
> +qca8k_cled_hw_control_configure(struct led_classdev *ldev, unsigned long rules,
> +				enum blink_mode_cmd cmd)
> +{
> +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> +	struct led_trigger *trigger = ldev->trigger;
> +	struct qca8k_led_pattern_en reg_info;
> +	struct qca8k_priv *priv = led->priv;
> +	u32 offload_trigger = 0, mask, val;
> +	int ret;
> +
> +	/* Check trigger compatibility */
> +	if (strcmp(trigger->name, "netdev"))
> +		return -EOPNOTSUPP;
> +
> +	if (!strcmp(trigger->name, "netdev"))
> +		ret = qca8k_parse_netdev(rules, &offload_trigger, &mask);

I'm not sure how well the compiler will spot that, but as far as
readability goes, that second if() statement appears to be redundant.

> +
> +	if (ret)
> +		return ret;
> +
> +	qca8k_get_control_led_reg(led->port_num, led->led_num, &reg_info);
> +
> +	switch (cmd) {
> +	case BLINK_MODE_SUPPORTED:
> +		/* We reach this point, we are sure the trigger is supported */
> +		return 1;
> +	case BLINK_MODE_ZERO:
> +		/* We set 4hz by default */
> +		u32 default_reg = QCA8K_LED_BLINK_4HZ;
> +
> +		ret = regmap_update_bits(priv->regmap, reg_info.reg,
> +					 QCA8K_LED_RULE_MASK << reg_info.shift,
> +					 default_reg << reg_info.shift);
> +		break;
> +	case BLINK_MODE_ENABLE:
> +		ret = regmap_update_bits(priv->regmap, reg_info.reg,
> +					 mask << reg_info.shift,
> +					 offload_trigger << reg_info.shift);
> +		break;
> +	case BLINK_MODE_DISABLE:
> +		ret = regmap_update_bits(priv->regmap, reg_info.reg,
> +					 mask << reg_info.shift,
> +					 0);
> +		break;

I think it needs to be made more clear in the documentation that if
this is called with ENABLE, then _only_ those modes in flags... (or
is it now called "rules"?) are to be enabled and all other modes
should be disabled. Conversely, DISABLE is used to disable all
modes. However, if that's the case, then ZERO was misdescribed, and
should probably be called DEFAULT. At least that's the impression
that I get from the above code.

> +	case BLINK_MODE_READ:
> +		ret = regmap_read(priv->regmap, reg_info.reg, &val);
> +		if (ret)
> +			return ret;
> +
> +		val >>= reg_info.shift;
> +		val &= offload_trigger;
> +
> +		/* Special handling for LED_BLINK_2HZ */
> +		if (!val && offload_trigger == QCA8K_LED_BLINK_2HZ)
> +			val = 1;

Hmm, so if a number of different modes is in flags or rules, then
as long as one matches, this returns 1? So it's an "any of these
modes is enabled" test.

> +
> +		return val;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return ret;
> +}
> +
> +static void
> +qca8k_led_brightness_set(struct qca8k_led *led,
> +			 enum led_brightness b)
> +{
> +	struct qca8k_led_pattern_en reg_info;
> +	struct qca8k_priv *priv = led->priv;
> +	u32 val = QCA8K_LED_ALWAYS_OFF;
> +
> +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> +
> +	if (b)
> +		val = QCA8K_LED_ALWAYS_ON;
> +
> +	regmap_update_bits(priv->regmap, reg_info.reg,
> +			   GENMASK(1, 0) << reg_info.shift,
> +			   val << reg_info.shift);
> +}
> +
> +static void
> +qca8k_cled_brightness_set(struct led_classdev *ldev,
> +			  enum led_brightness b)
> +{
> +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> +
> +	return qca8k_led_brightness_set(led, b);
> +}
> +
> +static enum led_brightness
> +qca8k_led_brightness_get(struct qca8k_led *led)
> +{
> +	struct qca8k_led_pattern_en reg_info;
> +	struct qca8k_priv *priv = led->priv;
> +	u32 val;
> +	int ret;
> +
> +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> +
> +	ret = regmap_read(priv->regmap, reg_info.reg, &val);
> +	if (ret)
> +		return 0;
> +
> +	val >>= reg_info.shift;
> +	val &= GENMASK(1, 0);
> +
> +	return val > 0 ? 1 : 0;
> +}
> +
> +static enum led_brightness
> +qca8k_cled_brightness_get(struct led_classdev *ldev)
> +{
> +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> +
> +	return qca8k_led_brightness_get(led);
> +}
> +
> +static int
> +qca8k_cled_blink_set(struct led_classdev *ldev,
> +		     unsigned long *delay_on,
> +		     unsigned long *delay_off)
> +{
> +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> +	struct qca8k_led_pattern_en reg_info;
> +	struct qca8k_priv *priv = led->priv;
> +
> +	if (*delay_on == 0 && *delay_off == 0) {
> +		*delay_on = 125;
> +		*delay_off = 125;
> +	}
> +
> +	if (*delay_on != 125 || *delay_off != 125) {
> +		/* The hardware only supports blinking at 4Hz. Fall back
> +		 * to software implementation in other cases.
> +		 */
> +		return -EINVAL;
> +	}
> +
> +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> +
> +	regmap_update_bits(priv->regmap, reg_info.reg,
> +			   GENMASK(1, 0) << reg_info.shift,
> +			   QCA8K_LED_ALWAYS_BLINK_4HZ << reg_info.shift);
> +
> +	return 0;
> +}
> +
> +static int
> +qca8k_cled_trigger_offload(struct led_classdev *ldev, bool enable)
> +{
> +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> +
> +	struct qca8k_led_pattern_en reg_info;
> +	struct qca8k_priv *priv = led->priv;
> +	u32 val = QCA8K_LED_ALWAYS_OFF;
> +
> +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> +
> +	if (enable)
> +		val = QCA8K_LED_RULE_CONTROLLED;
> +
> +	return regmap_update_bits(priv->regmap, reg_info.reg,
> +				  GENMASK(1, 0) << reg_info.shift,
> +				  val << reg_info.shift);

88e151x doesn't have the ability to change in this way - we have
a register with a 4-bit field which selects the LED mode from one
of many, or forces the LED on/off/hi-z/blink.

Not specifically for this patch, but talking generally about this
approach, the other issue I forsee with this is that yes, 88e151x has
three LEDs, but the LED modes are also used to implement control
signals (e.g., on a SFP, LOS can be implemented by programming mode
0 on LED2 (which makes it indicate link or not.) If we expose all the
LEDs we run the risk of the LED subsystem trampling over that
configuration and essentially messing up such modules. So the Marvell
PHY driver would need to know when it is appropriate to expose these
things to the LED subsystem.

I guess doing it dependent on firmware description as you do in
this driver would work - if there's no firmware description, they're
not exposed.
Christian Marangi Dec. 16, 2022, 5:48 p.m. UTC | #4
On Thu, Dec 15, 2022 at 05:49:57PM +0000, Russell King (Oracle) wrote:
> Hi,
> 
> On Thu, Dec 15, 2022 at 12:54:37AM +0100, Christian Marangi wrote:
> > +static int
> > +qca8k_cled_hw_control_configure(struct led_classdev *ldev, unsigned long rules,
> > +				enum blink_mode_cmd cmd)
> > +{
> > +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > +	struct led_trigger *trigger = ldev->trigger;
> > +	struct qca8k_led_pattern_en reg_info;
> > +	struct qca8k_priv *priv = led->priv;
> > +	u32 offload_trigger = 0, mask, val;
> > +	int ret;
> > +
> > +	/* Check trigger compatibility */
> > +	if (strcmp(trigger->name, "netdev"))
> > +		return -EOPNOTSUPP;
> > +
> > +	if (!strcmp(trigger->name, "netdev"))
> > +		ret = qca8k_parse_netdev(rules, &offload_trigger, &mask);
> 
> I'm not sure how well the compiler will spot that, but as far as
> readability goes, that second if() statement appears to be redundant.
>

Leftover when the driver supported 2 trigger. The idea was to provide a
"template" to show the flow of the function.

> > +
> > +	if (ret)
> > +		return ret;
> > +
> > +	qca8k_get_control_led_reg(led->port_num, led->led_num, &reg_info);
> > +
> > +	switch (cmd) {
> > +	case BLINK_MODE_SUPPORTED:
> > +		/* We reach this point, we are sure the trigger is supported */
> > +		return 1;
> > +	case BLINK_MODE_ZERO:
> > +		/* We set 4hz by default */
> > +		u32 default_reg = QCA8K_LED_BLINK_4HZ;
> > +
> > +		ret = regmap_update_bits(priv->regmap, reg_info.reg,
> > +					 QCA8K_LED_RULE_MASK << reg_info.shift,
> > +					 default_reg << reg_info.shift);
> > +		break;
> > +	case BLINK_MODE_ENABLE:
> > +		ret = regmap_update_bits(priv->regmap, reg_info.reg,
> > +					 mask << reg_info.shift,
> > +					 offload_trigger << reg_info.shift);
> > +		break;
> > +	case BLINK_MODE_DISABLE:
> > +		ret = regmap_update_bits(priv->regmap, reg_info.reg,
> > +					 mask << reg_info.shift,
> > +					 0);
> > +		break;
> 
> I think it needs to be made more clear in the documentation that if
> this is called with ENABLE, then _only_ those modes in flags... (or
> is it now called "rules"?) are to be enabled and all other modes
> should be disabled. Conversely, DISABLE is used to disable all
> modes. However, if that's the case, then ZERO was misdescribed, and
> should probably be called DEFAULT. At least that's the impression
> that I get from the above code.
> 

ZERO disable all the rules. Driver can keep some rule enabled (this is
the case for qca8k blink mode at 4hz by default)

ENABLE/DISABLE only act on the provided thing in rules. In the current
imlementation parse rules, generate a mask and update the values
accordingly. Other rules are not touched. This is based on the fact that
the first and the last thing done is calling ZERO to reset all the rules
to a known state.

I will try to improve the Documentation on this aspect.
Hope you know understand better the calling flow.

> > +	case BLINK_MODE_READ:
> > +		ret = regmap_read(priv->regmap, reg_info.reg, &val);
> > +		if (ret)
> > +			return ret;
> > +
> > +		val >>= reg_info.shift;
> > +		val &= offload_trigger;
> > +
> > +		/* Special handling for LED_BLINK_2HZ */
> > +		if (!val && offload_trigger == QCA8K_LED_BLINK_2HZ)
> > +			val = 1;
> 
> Hmm, so if a number of different modes is in flags or rules, then
> as long as one matches, this returns 1? So it's an "any of these
> modes is enabled" test.
> 

Ok this should be changed and you are right. READ should return true or
false if the rule (or rules) are enabled. This work for a single rule
but doesn't if multiple rules are provided.

Will fix that since this is wrong.

> > +
> > +		return val;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static void
> > +qca8k_led_brightness_set(struct qca8k_led *led,
> > +			 enum led_brightness b)
> > +{
> > +	struct qca8k_led_pattern_en reg_info;
> > +	struct qca8k_priv *priv = led->priv;
> > +	u32 val = QCA8K_LED_ALWAYS_OFF;
> > +
> > +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> > +
> > +	if (b)
> > +		val = QCA8K_LED_ALWAYS_ON;
> > +
> > +	regmap_update_bits(priv->regmap, reg_info.reg,
> > +			   GENMASK(1, 0) << reg_info.shift,
> > +			   val << reg_info.shift);
> > +}
> > +
> > +static void
> > +qca8k_cled_brightness_set(struct led_classdev *ldev,
> > +			  enum led_brightness b)
> > +{
> > +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > +
> > +	return qca8k_led_brightness_set(led, b);
> > +}
> > +
> > +static enum led_brightness
> > +qca8k_led_brightness_get(struct qca8k_led *led)
> > +{
> > +	struct qca8k_led_pattern_en reg_info;
> > +	struct qca8k_priv *priv = led->priv;
> > +	u32 val;
> > +	int ret;
> > +
> > +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> > +
> > +	ret = regmap_read(priv->regmap, reg_info.reg, &val);
> > +	if (ret)
> > +		return 0;
> > +
> > +	val >>= reg_info.shift;
> > +	val &= GENMASK(1, 0);
> > +
> > +	return val > 0 ? 1 : 0;
> > +}
> > +
> > +static enum led_brightness
> > +qca8k_cled_brightness_get(struct led_classdev *ldev)
> > +{
> > +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > +
> > +	return qca8k_led_brightness_get(led);
> > +}
> > +
> > +static int
> > +qca8k_cled_blink_set(struct led_classdev *ldev,
> > +		     unsigned long *delay_on,
> > +		     unsigned long *delay_off)
> > +{
> > +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > +	struct qca8k_led_pattern_en reg_info;
> > +	struct qca8k_priv *priv = led->priv;
> > +
> > +	if (*delay_on == 0 && *delay_off == 0) {
> > +		*delay_on = 125;
> > +		*delay_off = 125;
> > +	}
> > +
> > +	if (*delay_on != 125 || *delay_off != 125) {
> > +		/* The hardware only supports blinking at 4Hz. Fall back
> > +		 * to software implementation in other cases.
> > +		 */
> > +		return -EINVAL;
> > +	}
> > +
> > +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> > +
> > +	regmap_update_bits(priv->regmap, reg_info.reg,
> > +			   GENMASK(1, 0) << reg_info.shift,
> > +			   QCA8K_LED_ALWAYS_BLINK_4HZ << reg_info.shift);
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +qca8k_cled_trigger_offload(struct led_classdev *ldev, bool enable)
> > +{
> > +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > +
> > +	struct qca8k_led_pattern_en reg_info;
> > +	struct qca8k_priv *priv = led->priv;
> > +	u32 val = QCA8K_LED_ALWAYS_OFF;
> > +
> > +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> > +
> > +	if (enable)
> > +		val = QCA8K_LED_RULE_CONTROLLED;
> > +
> > +	return regmap_update_bits(priv->regmap, reg_info.reg,
> > +				  GENMASK(1, 0) << reg_info.shift,
> > +				  val << reg_info.shift);
> 
> 88e151x doesn't have the ability to change in this way - we have
> a register with a 4-bit field which selects the LED mode from one
> of many, or forces the LED on/off/hi-z/blink.
> 
> Not specifically for this patch, but talking generally about this
> approach, the other issue I forsee with this is that yes, 88e151x has
> three LEDs, but the LED modes are also used to implement control
> signals (e.g., on a SFP, LOS can be implemented by programming mode
> 0 on LED2 (which makes it indicate link or not.) If we expose all the
> LEDs we run the risk of the LED subsystem trampling over that
> configuration and essentially messing up such modules. So the Marvell
> PHY driver would need to know when it is appropriate to expose these
> things to the LED subsystem.
> 
> I guess doing it dependent on firmware description as you do in
> this driver would work - if there's no firmware description, they're
> not exposed.
> 

The idea is to provide a way to tell the driver what should be done and
tell the trigger that something is not doable and to revert to sw mode.

keeping the thing as simple and direct as possible and leave the rest to
the driver by doing minimal validation on the trigger side.

Do you have suggestion on how this can be improved even more and be more
flexible?
Andrew Lunn Dec. 20, 2022, 11:11 p.m. UTC | #5
> > +qca8k_cled_trigger_offload(struct led_classdev *ldev, bool enable)
> > +{
> > +	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
> > +
> > +	struct qca8k_led_pattern_en reg_info;
> > +	struct qca8k_priv *priv = led->priv;
> > +	u32 val = QCA8K_LED_ALWAYS_OFF;
> > +
> > +	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
> > +
> > +	if (enable)
> > +		val = QCA8K_LED_RULE_CONTROLLED;
> > +
> > +	return regmap_update_bits(priv->regmap, reg_info.reg,
> > +				  GENMASK(1, 0) << reg_info.shift,
> > +				  val << reg_info.shift);
> 
> 88e151x doesn't have the ability to change in this way - we have
> a register with a 4-bit field which selects the LED mode from one
> of many, or forces the LED on/off/hi-z/blink.
> 
> Not specifically for this patch, but talking generally about this
> approach, the other issue I forsee with this is that yes, 88e151x has
> three LEDs, but the LED modes are also used to implement control
> signals (e.g., on a SFP, LOS can be implemented by programming mode
> 0 on LED2 (which makes it indicate link or not.) If we expose all the
> LEDs we run the risk of the LED subsystem trampling over that
> configuration and essentially messing up such modules. So the Marvell
> PHY driver would need to know when it is appropriate to expose these
> things to the LED subsystem.
> 
> I guess doing it dependent on firmware description as you do in
> this driver would work - if there's no firmware description, they're
> not exposed.
> 

I expect there will always be some sort of firmware involved,
describing the hardware. Without that, we have no idea how many LEDs
are actually connected to pins of the PHY, for example.

I've not yet looked at this patchset in detail, but i hope the DT
binding code is reusable, so all PHYs will have the same basic
binding.

    Andrew
diff mbox series

Patch

diff --git a/drivers/net/dsa/qca/Kconfig b/drivers/net/dsa/qca/Kconfig
index ba339747362c..be67164a444e 100644
--- a/drivers/net/dsa/qca/Kconfig
+++ b/drivers/net/dsa/qca/Kconfig
@@ -15,3 +15,12 @@  config NET_DSA_QCA8K
 	help
 	  This enables support for the Qualcomm Atheros QCA8K Ethernet
 	  switch chips.
+
+config NET_DSA_QCA8K_LEDS_SUPPORT
+	tristate "Qualcomm Atheros QCA8K Ethernet switch family LEDs support"
+	depends on NET_DSA_QCA8K
+	select LEDS_OFFLOAD_TRIGGERS
+	help
+	  This enabled support for LEDs present on the Qualcomm Atheros
+	  QCA8K Ethernet switch chips. This require the LEDs offload
+	  triggers support as it can run in offload mode.
diff --git a/drivers/net/dsa/qca/Makefile b/drivers/net/dsa/qca/Makefile
index 701f1d199e93..330ae389e489 100644
--- a/drivers/net/dsa/qca/Makefile
+++ b/drivers/net/dsa/qca/Makefile
@@ -2,3 +2,4 @@ 
 obj-$(CONFIG_NET_DSA_AR9331)	+= ar9331.o
 obj-$(CONFIG_NET_DSA_QCA8K)	+= qca8k.o
 qca8k-y 			+= qca8k-common.o qca8k-8xxx.o
+obj-$(CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT) += qca8k-leds.o
diff --git a/drivers/net/dsa/qca/qca8k-8xxx.c b/drivers/net/dsa/qca/qca8k-8xxx.c
index c5c3b4e92f28..90916d8d8afe 100644
--- a/drivers/net/dsa/qca/qca8k-8xxx.c
+++ b/drivers/net/dsa/qca/qca8k-8xxx.c
@@ -1690,6 +1690,10 @@  qca8k_setup(struct dsa_switch *ds)
 	if (ret)
 		return ret;
 
+	ret = qca8k_setup_led_ctrl(priv);
+	if (ret)
+		return ret;
+
 	qca8k_setup_pcs(priv, &priv->pcs_port_0, 0);
 	qca8k_setup_pcs(priv, &priv->pcs_port_6, 6);
 
diff --git a/drivers/net/dsa/qca/qca8k-leds.c b/drivers/net/dsa/qca/qca8k-leds.c
new file mode 100644
index 000000000000..b51cdcae31b2
--- /dev/null
+++ b/drivers/net/dsa/qca/qca8k-leds.c
@@ -0,0 +1,406 @@ 
+// SPDX-License-Identifier: GPL-2.0
+#include <net/dsa.h>
+#include <linux/regmap.h>
+
+#include "qca8k.h"
+
+static int
+qca8k_get_enable_led_reg(int port_num, int led_num, struct qca8k_led_pattern_en *reg_info)
+{
+	int shift;
+
+	switch (port_num) {
+	case 0:
+		reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
+		reg_info->shift = 14;
+		break;
+	case 1:
+	case 2:
+	case 3:
+		reg_info->reg = QCA8K_LED_CTRL_REG(3);
+		shift = 2 * led_num + (6 * (port_num - 1));
+
+		reg_info->shift = 8 + shift;
+
+		break;
+	case 4:
+		reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
+		reg_info->shift = 30;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int
+qca8k_get_control_led_reg(int port_num, int led_num, struct qca8k_led_pattern_en *reg_info)
+{
+	reg_info->reg = QCA8K_LED_CTRL_REG(led_num);
+
+	/* 6 total control rule:
+	 * 3 control rules for phy0-3 that applies to all their leds
+	 * 3 control rules for phy4
+	 */
+	if (port_num == 4)
+		reg_info->shift = 16;
+	else
+		reg_info->shift = 0;
+
+	return 0;
+}
+
+static int
+qca8k_parse_netdev(unsigned long rules, u32 *offload_trigger, u32 *mask)
+{
+	/* Parsing specific to netdev trigger */
+	if (test_bit(TRIGGER_NETDEV_LINK, &rules))
+		*offload_trigger = QCA8K_LED_LINK_10M_EN_MASK |
+				   QCA8K_LED_LINK_100M_EN_MASK |
+				   QCA8K_LED_LINK_1000M_EN_MASK;
+	if (test_bit(TRIGGER_NETDEV_LINK_10, &rules))
+		*offload_trigger = QCA8K_LED_LINK_10M_EN_MASK;
+	if (test_bit(TRIGGER_NETDEV_LINK_100, &rules))
+		*offload_trigger = QCA8K_LED_LINK_100M_EN_MASK;
+	if (test_bit(TRIGGER_NETDEV_LINK_1000, &rules))
+		*offload_trigger = QCA8K_LED_LINK_1000M_EN_MASK;
+	if (test_bit(TRIGGER_NETDEV_HALF_DUPLEX, &rules))
+		*offload_trigger = QCA8K_LED_HALF_DUPLEX_MASK;
+	if (test_bit(TRIGGER_NETDEV_FULL_DUPLEX, &rules))
+		*offload_trigger = QCA8K_LED_FULL_DUPLEX_MASK;
+	if (test_bit(TRIGGER_NETDEV_TX, &rules))
+		*offload_trigger = QCA8K_LED_TX_BLINK_MASK;
+	if (test_bit(TRIGGER_NETDEV_RX, &rules))
+		*offload_trigger = QCA8K_LED_RX_BLINK_MASK;
+	if (test_bit(TRIGGER_NETDEV_BLINK_2HZ, &rules))
+		*offload_trigger = QCA8K_LED_BLINK_2HZ;
+	if (test_bit(TRIGGER_NETDEV_BLINK_4HZ, &rules))
+		*offload_trigger = QCA8K_LED_BLINK_4HZ;
+	if (test_bit(TRIGGER_NETDEV_BLINK_8HZ, &rules))
+		*offload_trigger = QCA8K_LED_BLINK_8HZ;
+
+	if (!*offload_trigger)
+		return -EOPNOTSUPP;
+
+	if (test_bit(TRIGGER_NETDEV_BLINK_2HZ, &rules) ||
+	    test_bit(TRIGGER_NETDEV_BLINK_4HZ, &rules) ||
+	    test_bit(TRIGGER_NETDEV_BLINK_8HZ, &rules)) {
+		*mask = QCA8K_LED_BLINK_FREQ_MASK;
+	} else {
+		*mask = *offload_trigger;
+	}
+
+	return 0;
+}
+
+static int
+qca8k_cled_hw_control_configure(struct led_classdev *ldev, unsigned long rules,
+				enum blink_mode_cmd cmd)
+{
+	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+	struct led_trigger *trigger = ldev->trigger;
+	struct qca8k_led_pattern_en reg_info;
+	struct qca8k_priv *priv = led->priv;
+	u32 offload_trigger = 0, mask, val;
+	int ret;
+
+	/* Check trigger compatibility */
+	if (strcmp(trigger->name, "netdev"))
+		return -EOPNOTSUPP;
+
+	if (!strcmp(trigger->name, "netdev"))
+		ret = qca8k_parse_netdev(rules, &offload_trigger, &mask);
+
+	if (ret)
+		return ret;
+
+	qca8k_get_control_led_reg(led->port_num, led->led_num, &reg_info);
+
+	switch (cmd) {
+	case BLINK_MODE_SUPPORTED:
+		/* We reach this point, we are sure the trigger is supported */
+		return 1;
+	case BLINK_MODE_ZERO:
+		/* We set 4hz by default */
+		u32 default_reg = QCA8K_LED_BLINK_4HZ;
+
+		ret = regmap_update_bits(priv->regmap, reg_info.reg,
+					 QCA8K_LED_RULE_MASK << reg_info.shift,
+					 default_reg << reg_info.shift);
+		break;
+	case BLINK_MODE_ENABLE:
+		ret = regmap_update_bits(priv->regmap, reg_info.reg,
+					 mask << reg_info.shift,
+					 offload_trigger << reg_info.shift);
+		break;
+	case BLINK_MODE_DISABLE:
+		ret = regmap_update_bits(priv->regmap, reg_info.reg,
+					 mask << reg_info.shift,
+					 0);
+		break;
+	case BLINK_MODE_READ:
+		ret = regmap_read(priv->regmap, reg_info.reg, &val);
+		if (ret)
+			return ret;
+
+		val >>= reg_info.shift;
+		val &= offload_trigger;
+
+		/* Special handling for LED_BLINK_2HZ */
+		if (!val && offload_trigger == QCA8K_LED_BLINK_2HZ)
+			val = 1;
+
+		return val;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return ret;
+}
+
+static void
+qca8k_led_brightness_set(struct qca8k_led *led,
+			 enum led_brightness b)
+{
+	struct qca8k_led_pattern_en reg_info;
+	struct qca8k_priv *priv = led->priv;
+	u32 val = QCA8K_LED_ALWAYS_OFF;
+
+	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
+
+	if (b)
+		val = QCA8K_LED_ALWAYS_ON;
+
+	regmap_update_bits(priv->regmap, reg_info.reg,
+			   GENMASK(1, 0) << reg_info.shift,
+			   val << reg_info.shift);
+}
+
+static void
+qca8k_cled_brightness_set(struct led_classdev *ldev,
+			  enum led_brightness b)
+{
+	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+
+	return qca8k_led_brightness_set(led, b);
+}
+
+static enum led_brightness
+qca8k_led_brightness_get(struct qca8k_led *led)
+{
+	struct qca8k_led_pattern_en reg_info;
+	struct qca8k_priv *priv = led->priv;
+	u32 val;
+	int ret;
+
+	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
+
+	ret = regmap_read(priv->regmap, reg_info.reg, &val);
+	if (ret)
+		return 0;
+
+	val >>= reg_info.shift;
+	val &= GENMASK(1, 0);
+
+	return val > 0 ? 1 : 0;
+}
+
+static enum led_brightness
+qca8k_cled_brightness_get(struct led_classdev *ldev)
+{
+	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+
+	return qca8k_led_brightness_get(led);
+}
+
+static int
+qca8k_cled_blink_set(struct led_classdev *ldev,
+		     unsigned long *delay_on,
+		     unsigned long *delay_off)
+{
+	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+	struct qca8k_led_pattern_en reg_info;
+	struct qca8k_priv *priv = led->priv;
+
+	if (*delay_on == 0 && *delay_off == 0) {
+		*delay_on = 125;
+		*delay_off = 125;
+	}
+
+	if (*delay_on != 125 || *delay_off != 125) {
+		/* The hardware only supports blinking at 4Hz. Fall back
+		 * to software implementation in other cases.
+		 */
+		return -EINVAL;
+	}
+
+	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
+
+	regmap_update_bits(priv->regmap, reg_info.reg,
+			   GENMASK(1, 0) << reg_info.shift,
+			   QCA8K_LED_ALWAYS_BLINK_4HZ << reg_info.shift);
+
+	return 0;
+}
+
+static int
+qca8k_cled_trigger_offload(struct led_classdev *ldev, bool enable)
+{
+	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+
+	struct qca8k_led_pattern_en reg_info;
+	struct qca8k_priv *priv = led->priv;
+	u32 val = QCA8K_LED_ALWAYS_OFF;
+
+	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
+
+	if (enable)
+		val = QCA8K_LED_RULE_CONTROLLED;
+
+	return regmap_update_bits(priv->regmap, reg_info.reg,
+				  GENMASK(1, 0) << reg_info.shift,
+				  val << reg_info.shift);
+}
+
+static int
+qca8k_cled_hw_control_start(struct led_classdev *led_cdev)
+{
+	return qca8k_cled_trigger_offload(led_cdev, true);
+}
+
+static int
+qca8k_cled_hw_control_stop(struct led_classdev *led_cdev)
+{
+	return qca8k_cled_trigger_offload(led_cdev, false);
+}
+
+static bool
+qca8k_cled_hw_control_status(struct led_classdev *ldev)
+{
+	struct qca8k_led *led = container_of(ldev, struct qca8k_led, cdev);
+
+	struct qca8k_led_pattern_en reg_info;
+	struct qca8k_priv *priv = led->priv;
+	u32 val;
+
+	qca8k_get_enable_led_reg(led->port_num, led->led_num, &reg_info);
+
+	regmap_read(priv->regmap, reg_info.reg, &val);
+
+	val >>= reg_info.shift;
+	val &= GENMASK(1, 0);
+
+	return val == QCA8K_LED_RULE_CONTROLLED;
+}
+
+static int
+qca8k_parse_port_leds(struct qca8k_priv *priv, struct fwnode_handle *port, int port_num)
+{
+	struct fwnode_handle *led = NULL, *leds = NULL;
+	struct led_init_data init_data = { };
+	struct qca8k_led *port_led;
+	int led_num, port_index;
+	const char *state;
+	int ret;
+
+	leds = fwnode_get_named_child_node(port, "leds");
+	if (!leds) {
+		dev_dbg(priv->dev, "No Leds node specified in device tree for port %d!\n",
+			port_num);
+		return 0;
+	}
+
+	fwnode_for_each_child_node(leds, led) {
+		/* Reg represent the led number of the port.
+		 * Each port can have at least 3 leds attached
+		 * Commonly:
+		 * 1. is gigabit led
+		 * 2. is mbit led
+		 * 3. additional status led
+		 */
+		if (fwnode_property_read_u32(led, "reg", &led_num))
+			continue;
+
+		if (led_num >= QCA8K_LED_PORT_COUNT) {
+			dev_warn(priv->dev, "Invalid LED reg defined %d", port_num);
+			continue;
+		}
+
+		port_index = 3 * port_num + led_num;
+
+		port_led = &priv->ports_led[port_index];
+		port_led->port_num = port_num;
+		port_led->led_num = led_num;
+		port_led->priv = priv;
+
+		ret = fwnode_property_read_string(led, "default-state", &state);
+		if (!ret) {
+			if (!strcmp(state, "on")) {
+				port_led->cdev.brightness = 1;
+				qca8k_led_brightness_set(port_led, 1);
+			} else if (!strcmp(state, "off")) {
+				port_led->cdev.brightness = 0;
+				qca8k_led_brightness_set(port_led, 0);
+			} else if (!strcmp(state, "keep")) {
+				port_led->cdev.brightness =
+					qca8k_led_brightness_get(port_led);
+			}
+		}
+
+		/* 3 brightness settings can be applied from Documentation:
+		 * 0 always off
+		 * 1 blink at 4Hz
+		 * 2 always on
+		 * 3 rule controlled
+		 * Suppots only 2 mode: (pcb limitation, with always on and blink
+		 * only the last led is set to this mode)
+		 * 0 always off (sets all leds off)
+		 * 3 rule controlled
+		 */
+		port_led->cdev.blink_mode = SOFTWARE_HARDWARE_CONTROLLED;
+		port_led->cdev.max_brightness = 1;
+		port_led->cdev.brightness_set = qca8k_cled_brightness_set;
+		port_led->cdev.brightness_get = qca8k_cled_brightness_get;
+		port_led->cdev.blink_set = qca8k_cled_blink_set;
+		port_led->cdev.hw_control_start = qca8k_cled_hw_control_start;
+		port_led->cdev.hw_control_stop = qca8k_cled_hw_control_stop;
+		port_led->cdev.hw_control_status = qca8k_cled_hw_control_status;
+		port_led->cdev.hw_control_configure = qca8k_cled_hw_control_configure;
+		init_data.default_label = ":port";
+		init_data.devicename = "qca8k";
+		init_data.fwnode = led;
+
+		ret = devm_led_classdev_register_ext(priv->dev, &port_led->cdev, &init_data);
+		if (ret)
+			dev_warn(priv->dev, "Failed to int led");
+	}
+
+	return 0;
+}
+
+int
+qca8k_setup_led_ctrl(struct qca8k_priv *priv)
+{
+	struct fwnode_handle *mdio, *port;
+	int port_num;
+	int ret;
+
+	mdio = device_get_named_child_node(priv->dev, "mdio");
+	if (!mdio) {
+		dev_info(priv->dev, "No MDIO node specified in device tree!\n");
+		return 0;
+	}
+
+	fwnode_for_each_child_node(mdio, port) {
+		if (fwnode_property_read_u32(port, "reg", &port_num))
+			continue;
+
+		/* Each port can have at least 3 different leds attached */
+		ret = qca8k_parse_port_leds(priv, port, port_num);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
diff --git a/drivers/net/dsa/qca/qca8k.h b/drivers/net/dsa/qca/qca8k.h
index 0b7a5cb12321..5c535baa7139 100644
--- a/drivers/net/dsa/qca/qca8k.h
+++ b/drivers/net/dsa/qca/qca8k.h
@@ -11,6 +11,7 @@ 
 #include <linux/delay.h>
 #include <linux/regmap.h>
 #include <linux/gpio.h>
+#include <linux/leds.h>
 #include <linux/dsa/tag_qca.h>
 
 #define QCA8K_ETHERNET_MDIO_PRIORITY			7
@@ -85,6 +86,43 @@ 
 #define   QCA8K_MDIO_MASTER_DATA(x)			FIELD_PREP(QCA8K_MDIO_MASTER_DATA_MASK, x)
 #define   QCA8K_MDIO_MASTER_MAX_PORTS			5
 #define   QCA8K_MDIO_MASTER_MAX_REG			32
+
+/* LED control register */
+#define QCA8K_LED_COUNT					15
+#define QCA8K_LED_PORT_COUNT				3
+#define QCA8K_LED_RULE_COUNT				6
+#define QCA8K_LED_RULE_MAX				11
+#define QCA8K_LED_CTRL_REG(_i)				(0x050 + (_i) * 4)
+#define QCA8K_LED_CTRL0_REG				0x50
+#define QCA8K_LED_CTRL1_REG				0x54
+#define QCA8K_LED_CTRL2_REG				0x58
+#define QCA8K_LED_CTRL3_REG				0x5C
+#define   QCA8K_LED_CTRL_SHIFT(_i)			(((_i) % 2) * 16)
+#define   QCA8K_LED_CTRL_MASK				GENMASK(15, 0)
+#define QCA8K_LED_RULE_MASK				GENMASK(13, 0)
+#define QCA8K_LED_BLINK_FREQ_MASK			GENMASK(1, 0)
+#define QCA8K_LED_BLINK_FREQ_SHITF			0
+#define   QCA8K_LED_BLINK_2HZ				0
+#define   QCA8K_LED_BLINK_4HZ				1
+#define   QCA8K_LED_BLINK_8HZ				2
+#define   QCA8K_LED_BLINK_AUTO				3
+#define QCA8K_LED_LINKUP_OVER_MASK			BIT(2)
+#define QCA8K_LED_TX_BLINK_MASK				BIT(4)
+#define QCA8K_LED_RX_BLINK_MASK				BIT(5)
+#define QCA8K_LED_COL_BLINK_MASK			BIT(7)
+#define QCA8K_LED_LINK_10M_EN_MASK			BIT(8)
+#define QCA8K_LED_LINK_100M_EN_MASK			BIT(9)
+#define QCA8K_LED_LINK_1000M_EN_MASK			BIT(10)
+#define QCA8K_LED_POWER_ON_LIGHT_MASK			BIT(11)
+#define QCA8K_LED_HALF_DUPLEX_MASK			BIT(12)
+#define QCA8K_LED_FULL_DUPLEX_MASK			BIT(13)
+#define QCA8K_LED_PATTERN_EN_MASK			GENMASK(15, 14)
+#define QCA8K_LED_PATTERN_EN_SHIFT			14
+#define   QCA8K_LED_ALWAYS_OFF				0
+#define   QCA8K_LED_ALWAYS_BLINK_4HZ			1
+#define   QCA8K_LED_ALWAYS_ON				2
+#define   QCA8K_LED_RULE_CONTROLLED			3
+
 #define QCA8K_GOL_MAC_ADDR0				0x60
 #define QCA8K_GOL_MAC_ADDR1				0x64
 #define QCA8K_MAX_FRAME_SIZE				0x78
@@ -388,6 +426,19 @@  struct qca8k_pcs {
 	int port;
 };
 
+struct qca8k_led_pattern_en {
+	u32 reg;
+	u8 shift;
+};
+
+struct qca8k_led {
+	u8 port_num;
+	u8 led_num;
+	u16 old_rule;
+	struct qca8k_priv *priv;
+	struct led_classdev cdev;
+};
+
 struct qca8k_priv {
 	u8 switch_id;
 	u8 switch_revision;
@@ -412,6 +463,7 @@  struct qca8k_priv {
 	struct qca8k_pcs pcs_port_0;
 	struct qca8k_pcs pcs_port_6;
 	const struct qca8k_match_data *info;
+	struct qca8k_led ports_led[QCA8K_LED_COUNT];
 };
 
 struct qca8k_mib_desc {
@@ -517,4 +569,14 @@  int qca8k_port_lag_join(struct dsa_switch *ds, int port, struct dsa_lag lag,
 int qca8k_port_lag_leave(struct dsa_switch *ds, int port,
 			 struct dsa_lag lag);
 
+/* Leds Support function */
+#ifdef CONFIG_NET_DSA_QCA8K_LEDS_SUPPORT
+int qca8k_setup_led_ctrl(struct qca8k_priv *priv);
+#else
+static inline int qca8k_setup_led_ctrl(struct qca8k_priv *priv)
+{
+	return 0;
+}
+#endif
+
 #endif /* __QCA8K_H */