Hi! > >>+cat /sys/class/leds/multicolor:status/multi_led_index > >>+green blue red > >Hmm. We should really make sure LEDs are ordered as "red green > >blue". Yes, userspace should support any order, but... > > Ordering is not guaranteed since it is based on the DT ordering. I don't > think we can mandate that these LEDs be put in order in the DT. > > Besides the framework and the device driver do not care what color is where > only the user space needs to care. The FW and device driver only care about > the brightness, intensity and channel.
Ok, lets keep it like this.
> >>+ offset += nrchars;
> >>+ }
> >This checks for "not enough" intensities. Do we need check for "too
> >many" intensities?
>
> We ignore anything greater then mcled_cdev->num_colors. So if this is set
> to 3 we only read the first 3 values.
>
> So we cannot read more then what is set by the DT.
Please make it return an error if extra values are passed in.
> >>+static ssize_t multi_led_intensity_show(struct device *dev,
> >>+ struct device_attribute *intensity_attr,
> >>+ char *buf)
> >>+{
> >>+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
> >>+ struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev);
> >>+ int len = 0;
> >>+ int i;
> >>+
> >>+ for (i = 0; i < mcled_cdev->num_colors; i++)
> >>+ len += sprintf(buf + len, "%d ",
> >>+ mcled_cdev->multicolor_info[i].color_led_intensity);
> >>+
> >>+ len += sprintf(buf + len, "%s", "\n");
> >This will result in extra " " before end of line.
> >
> >Please don't use "%s", "\n" to add single character. "\n" would be enough.
> Ack changed to just sprintf(buf + len, "\n");
Also note the extra space before end of line.
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
