> -----Original Message----- > From: Pavel Machek [mailto:pavel@xxxxxx] > Sent: Wednesday, August 02, 2017 11:39 AM > To: Vadim Pasternak <vadimp@xxxxxxxxxxxx> > Cc: j.anaszewski@xxxxxxxxxxx; rpurdie@xxxxxxxxx; linux- > leds@xxxxxxxxxxxxxxx; lee.jones@xxxxxxxxxx; robh+dt@xxxxxxxxxx; > jiri@xxxxxxxxxxx; =jacek.anaszewski@xxxxxxxxx > 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@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? 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, ®val); > > + 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
