Date:   Wed, 15 Mar 2023 06:57:37 +0100
From:   Michal Swiatkowski <michal.swiatkowski@linux.intel.com>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        Kurt Kanzenbach <kurt@linutronix.de>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>
Subject: Re: [PATCH net-next v6 1/1] net: dsa: hellcreek: Get rid of custom
 led_init_default_state_get()
Message-ID: <ZBFeUazA9X9mmWiJ@localhost.localdomain>
References: <20230314181824.56881-1-andriy.shevchenko@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230314181824.56881-1-andriy.shevchenko@linux.intel.com>
Precedence: bulk

On Tue, Mar 14, 2023 at 08:18:24PM +0200, Andy Shevchenko wrote:
> LED core provides a helper to parse default state from firmware node.
> Use it instead of custom implementation.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
> v6: wrapped long lines (Simon, Jakub)
>  drivers/net/dsa/hirschmann/hellcreek_ptp.c | 45 ++++++++++++----------
>  1 file changed, 24 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/dsa/hirschmann/hellcreek_ptp.c b/drivers/net/dsa/hirschmann/hellcreek_ptp.c
> index b28baab6d56a..3e44ccb7db84 100644
> --- a/drivers/net/dsa/hirschmann/hellcreek_ptp.c
> +++ b/drivers/net/dsa/hirschmann/hellcreek_ptp.c
> @@ -297,7 +297,8 @@ static enum led_brightness hellcreek_led_is_gm_get(struct led_classdev *ldev)
>  static int hellcreek_led_setup(struct hellcreek *hellcreek)
>  {
>  	struct device_node *leds, *led = NULL;
> -	const char *label, *state;
> +	enum led_default_state state;
> +	const char *label;
>  	int ret = -EINVAL;
>  
>  	of_node_get(hellcreek->dev->of_node);
> @@ -318,16 +319,17 @@ static int hellcreek_led_setup(struct hellcreek *hellcreek)
>  	ret = of_property_read_string(led, "label", &label);
>  	hellcreek->led_sync_good.name = ret ? "sync_good" : label;
>  
> -	ret = of_property_read_string(led, "default-state", &state);
> -	if (!ret) {
> -		if (!strcmp(state, "on"))
> -			hellcreek->led_sync_good.brightness = 1;
> -		else if (!strcmp(state, "off"))
> -			hellcreek->led_sync_good.brightness = 0;
> -		else if (!strcmp(state, "keep"))
> -			hellcreek->led_sync_good.brightness =
> -				hellcreek_get_brightness(hellcreek,
> -							 STATUS_OUT_SYNC_GOOD);
> +	state = led_init_default_state_get(of_fwnode_handle(led));
> +	switch (state) {
> +	case LEDS_DEFSTATE_ON:
> +		hellcreek->led_sync_good.brightness = 1;
> +		break;
> +	case LEDS_DEFSTATE_KEEP:
> +		hellcreek->led_sync_good.brightness =
> +			hellcreek_get_brightness(hellcreek, STATUS_OUT_SYNC_GOOD);
> +		break;
> +	default:
> +		hellcreek->led_sync_good.brightness = 0;
>  	}
>  
>  	hellcreek->led_sync_good.max_brightness = 1;
> @@ -344,16 +346,17 @@ static int hellcreek_led_setup(struct hellcreek *hellcreek)
>  	ret = of_property_read_string(led, "label", &label);
>  	hellcreek->led_is_gm.name = ret ? "is_gm" : label;
>  
> -	ret = of_property_read_string(led, "default-state", &state);
> -	if (!ret) {
> -		if (!strcmp(state, "on"))
> -			hellcreek->led_is_gm.brightness = 1;
> -		else if (!strcmp(state, "off"))
> -			hellcreek->led_is_gm.brightness = 0;
> -		else if (!strcmp(state, "keep"))
> -			hellcreek->led_is_gm.brightness =
> -				hellcreek_get_brightness(hellcreek,
> -							 STATUS_OUT_IS_GM);
> +	state = led_init_default_state_get(of_fwnode_handle(led));
> +	switch (state) {
> +	case LEDS_DEFSTATE_ON:
> +		hellcreek->led_is_gm.brightness = 1;
> +		break;
> +	case LEDS_DEFSTATE_KEEP:
> +		hellcreek->led_is_gm.brightness =
> +			hellcreek_get_brightness(hellcreek, STATUS_OUT_IS_GM);
> +		break;
> +	default:
> +		hellcreek->led_is_gm.brightness = 0;

Hi,

You have to fix implict declaration of the led_init_default_state_get().

I wonder if the code duplication here can be avoided:
static void set_led_brightness(hellcreek, led, *brightness, status)
{
	enum led_default_state state =
		led_init_default_state_get(of_fwnode_handle(led));

	switch(state) {
	case LEDS_DEFSTATE_ON:
		*brightness = 1;
		break;
	case LEDS_DEFSTATE_KEEP:
		*brightness = hellcreek_get_brightness(hellcreek, status);
		break;
	default:
		*brightness = 0;
	}
}

and call it like:
set_led_brightness(heelcreek, led, &heelcreek->...,
		   STATUS_OUT_IS_GM/SYNC_GOOD);

Only suggestion, patch looks good.

Thanks,
Michal

>  	}
>  
>  	hellcreek->led_is_gm.max_brightness = 1;
> -- 
> 2.39.2
>