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

[Florian Fainelli <f.fainelli@gmail.com>]

On 12/28/2021 2:09 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.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@milecki.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