Re: [PATCH net-next v5 1/1] net: dsa: hellcreek: Get rid of custom led_init_default_state_get()

[Simon Horman]

On Fri, Mar 10, 2023 at 06:38:55PM +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@xxxxxxxxxxxxxxx>
> Reviewed-by: Kurt Kanzenbach <kurt@xxxxxxxxxxxxx>
> ---
> v5: resent after v6.3-rc1 with proper net-next prefix
>  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..793b2c296314 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);

nit: I think < 80 columns wide is still preferred for network code

> +		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;
>  	}

This seems to duplicate the logic in the earlier hunk of this patch.
Could it be moved into a helper?

>  
>  	hellcreek->led_is_gm.max_brightness = 1;
> -- 
> 2.39.1
>