Re: [patch v3 1/1] leds: add driver for support Mellanox regmap LEDs for BMC

[Pavel Machek <pavel@xxxxxx>]

Hi!

> Driver obtains LED devices according to system configuration and creates
> devices in form: "devicename:color:function", like
> The full path is to be:
> /sys/class/leds/mlxreg\:status\:amber/brightness
> After timer trigger activation:
> echo timer > /sys/class/leds/mlxreg\:status\:amber/trigger
> Attributes for LED blinking will appaer in sysfs infrastructure:
> /sys/class/leds/mlxreg\:status\:amber/delay_off
> /sys/class/leds/mlxreg\:status\:amber/delay_on
> 
> LED setting is controlled through the on-board programmable devices,
> which exports its register map. This device could be attached to any
> bus type, for which register mapping is supported.
> 
> Signed-off-by: Vadim Pasternak <vadimp@xxxxxxxxxxxx>

...

> +/* Codes for LEDs. */
> +#define MLXREG_LED_OFFSET_HALF	0x01 /* Offset from solid: 3Hz blink */
> +#define MLXREG_LED_OFFSET_FULL	0x02 /* Offset from solid: 6Hz

Can it do half brightness?

> +	ret = regmap_read(pdata->regmap, led_pdata->led->reg, &regval);
> +	if (ret < 0) {
> +		dev_warn(led_pdata->led_cdev.dev, "Failed to get current brightness, error: %d\n",
> +			 ret);
> +		/* Assume the LED is OFF */
> +		ret = LED_OFF;
> +		goto access_error;

Just "return  LED_OFF;".

> +	if (regval >= led_pdata->base_color &&
> +	    regval <= (led_pdata->base_color + MLXREG_LED_OFFSET_FULL))
> +		ret = LED_FULL;
> +	else
> +		ret = LED_OFF;

Because it does not seem so here.

> +	for (i = 0; i < pdata->counter; i++, data++) {
> +		led_data = &pdata->data[i];
> +		led_cdev = &led_data->led_cdev;
> +		led_data->data_parent = priv;
> +		if (strstr(data->label, "red")) {
> +			brightness = LED_OFF;
> +			led_data->base_color = MLXREG_LED_RED_SOLID;
> +		} else if (strstr(data->label, "amber")) {
> +			brightness = LED_OFF;
> +			led_data->base_color = MLXREG_LED_AMBER_SOLID;
> +		} else {
> +			brightness = LED_OFF;
> +			led_data->base_color = MLXREG_LED_GREEN_SOLID;
> +		}

i don't know about the future, but you may check for "green"
substring, too? (Besides the fact that substring search is
"interesting"...)

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachment: signature.asc
Description: Digital signature