netdev - Re: [PATCH] net: dsa: bcm_sf2: refactor LED regs access

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <097bb80d-a5c5-3884-4ce4-64c9fec1b26a@gmail.com>
Date:   Wed, 29 Dec 2021 08:40:08 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Rafał Miłecki <zajec5@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>
Cc:     netdev@...r.kernel.org, bcm-kernel-feedback-list@...adcom.com,
        Rafał Miłecki <rafal@...ecki.pl>
Subject: Re: [PATCH] net: dsa: bcm_sf2: refactor LED regs access



On 12/28/2021 2:09 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@...ecki.pl>
> 
> 1. Define more regs. Some switches (e.g. BCM4908) have up to 6 regs.
> 2. Add helper for handling non-lineral port <-> reg mappings.
> 3. Add support for 12 B LED reg blocks on BCM4908 (different layout)
> 
> Complete support for LEDs setup will be implemented once Linux receives
> a proper design & implementation for "hardware" LEDs.
> 
> Signed-off-by: Rafał Miłecki <rafal@...ecki.pl>

This looks good for the most part, just one nit below:

> ---
>   drivers/net/dsa/bcm_sf2.c      | 60 +++++++++++++++++++++++++++----
>   drivers/net/dsa/bcm_sf2.h      | 10 ++++++
>   drivers/net/dsa/bcm_sf2_regs.h | 65 +++++++++++++++++++++++++++++++---
>   3 files changed, 125 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
> index 13aa43b5cffd..c2447de9d441 100644
> --- a/drivers/net/dsa/bcm_sf2.c
> +++ b/drivers/net/dsa/bcm_sf2.c
> @@ -62,6 +62,44 @@ static u16 bcm_sf2_reg_rgmii_cntrl(struct bcm_sf2_priv *priv, int port)
>   	return REG_SWITCH_STATUS;
>   }
>   
> +static u16 bcm_sf2_reg_led_base(struct bcm_sf2_priv *priv, int port)
> +{
> +	switch (priv->type) {
> +	case BCM4908_DEVICE_ID:
> +		switch (port) {
> +		case 0:
> +			return REG_LED_0_CNTRL;
> +		case 1:
> +			return REG_LED_1_CNTRL;
> +		case 2:
> +			return REG_LED_2_CNTRL;

Up until that port count, we have a common path, it is only after port > 
2 that we stop having a common path. Only BCM7445 and BCM7278 have two 
external ports, 63138 (and 63148 when that gets added eventually) as 
well as a 4908 have more ports, so I would do something like this:

switch (port) {
case 0:
	return REG_LED_0_CNTRL;
case 1:
	return REG_LED_1_CNTRL;
case 2:
	return REG_LED_2_CNTRL;
default:
	break;
}

if (priv->type == BCM7445_DEVICE_ID || priv->type == BCM7278_DEVICE_ID)
	goto out;

Also, this LED controller is also present in the GENETv5 adapters, so we 
may have some value in writing a common LED framework driver for it at 
some point.

Thanks!
-- 
Florian