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

[Vadim Pasternak <vadimp@mellanox.com>]

> -----Original Message-----
> From: Pavel Machek [mailto:pavel@ucw.cz]
> Sent: Wednesday, August 02, 2017 11:39 AM
> To: Vadim Pasternak <vadimp@mellanox.com>
> Cc: j.anaszewski@samsung.com; rpurdie@rpsys.net; linux-
> leds@vger.kernel.org; lee.jones@linaro.org; robh+dt@kernel.org;
> jiri@resnulli.us; =jacek.anaszewski@gmail.com
> Subject: Re: [patch v3 1/1] leds: add driver for support Mellanox regmap
> LEDs for BMC
> 
> Hi!

Hi Pavel,

Thank you for review.

Would it be possible to review alos [patch v2 1/2] mfd: Add Mellanox regmap core driver , which is after applying you comments for v1?

> 
> > 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@mellanox.com>
> 
> ...
> 
> > +/* 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?


This is about blinking  frequency. Hardware support 6 hz blinking mode and 3hz blinking mode.
If solid orange f.e. has bit mask 1100, setting 1101 or 1110 will blink orange with 3 or 6 KHz.

> 
> > +	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;".

ACK

> 
> > +	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"...)

OK

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