Hi! ting/sysfs-class-led-multicolor > new file mode 100644 > index 000000000000..ada0dbecfeab > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-class-led-multicolor > @@ -0,0 +1,42 @@ > +What: /sys/class/leds/<led>/multi_led_index > +Date: March 2020 > +KernelVersion: 5.8 > +Contact: Dan Murphy <dmurphy@xxxxxx> > +Description: read > + The multi_led_index array, when read, will output the LED colors > + by name as they are indexed in the multi_led_intensity file. Can we make it multi_index? We are already in leds directory, and it is a bit shorter. > +What: /sys/class/leds/<led>/num_multi_leds > +Date: March 2020 > +KernelVersion: 5.8 > +Contact: Dan Murphy <dmurphy@xxxxxx> > +Description: read > + The num_multi_leds indicates the number of LEDs defined in the > + multi_led_intensity and multi_led_index files. Please drop this one. > +What: /sys/class/leds/<led>/multi_led_intensity > +Date: March 2020 > +KernelVersion: 5.8 > +Contact: Dan Murphy <dmurphy@xxxxxx> > +Description: read/write > + Intensity level for the LED color within the array. > + The intensities for each color must be entered based on the > + multi_led_index array. And let this one be multi_intensity. > +For more details on hue and lightness notions please refer to > +https://en.wikipedia.org/wiki/CIECAM02. I'd drop this reference. multi_intensity file controls both hue and saturation AFAICT. > +Example: > +A user first writes the multi_led_intensity file with the brightness levels > +for each LED that are necessary to achieve a blueish violet output from a > +multicolor LED group. I don't believe we can guarantee that. 255/255/255 will produce different colors on different hardware (not white), and 43/226/138 will also produce different colors.... > +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... > +The user can control the brightness of that multicolor LED group by writing the > +parent 'brightness' control. Assuming a parent max_brightness of 255 the user delete "parent", twice? > + for (i = 0; i < mcled_cdev->num_colors; i++) > + mcled_cdev->multicolor_info[i].color_brightness = brightness * > + mcled_cdev->multicolor_info[i].color_led_intensity / > + led_cdev->max_brightness; It would be good to get this under ~80 characters. Perhaps shorter identifiers would help... shortening multicolor_ to mc_? > +static ssize_t multi_led_intensity_store(struct device *dev, > + struct device_attribute *intensity_attr, > + const char *buf, size_t size) > +{ > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev); > + int nrchars, offset = 0; > + int intensity_value[LED_COLOR_ID_MAX]; > + int i; > + ssize_t ret; > + > + mutex_lock(&led_cdev->led_access); > + > + for (i = 0; i < mcled_cdev->num_colors; i++) { > + ret = sscanf(buf + offset, "%i%n", > + &intensity_value[i], &nrchars); > + if (ret != 1) { > + dev_err(led_cdev->dev, dev_dbg, at most. It is user-triggerable. > + "Incorrect number of LEDs expected %i values intensity was not applied\n", > + mcled_cdev->num_colors); > + goto err_out; Should not we return -ERRNO to userspace on error? > + } > + offset += nrchars; > + } This checks for "not enough" intensities. Do we need check for "too many" intensities? > +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. > + struct led_classdev *led_cdev = dev_get_drvdata(dev); > + struct led_classdev_mc *mcled_cdev = lcdev_to_mccdev(led_cdev); > + int len = 0; > + int index; > + int i; > + > + for (i = 0; i < mcled_cdev->num_colors; i++) { > + index = mcled_cdev->multicolor_info[i].color_index; > + len += sprintf(buf + len, "%s ", led_colors[index]); > + } > + > + len += sprintf(buf + len, "%s", "\n"); Same here. > +int led_classdev_multicolor_register_ext(struct device *parent, > + struct led_classdev_mc *mcled_cdev, > + struct led_init_data *init_data) > +{ > + struct led_classdev *led_cdev; > + > + if (!mcled_cdev) > + return -EINVAL; > + > + if (!mcled_cdev->num_colors) > + return -EINVAL; if (num_colors > max)... ? > +#ifndef __LINUX_MULTICOLOR_LEDS_H_INCLUDED > +#define __LINUX_MULTICOLOR_LEDS_H_INCLUDED Usual style is "_LINUX_MULTICOLOR_LEDS_H". > +#else > + > +static inline int led_classdev_multicolor_register_ext(struct device *parent, double space after "inline". > + struct led_classdev_mc *mcled_cdev, > + struct led_init_data *init_data) > +{ > + return -EINVAL; > +} Do we need to include these stubs? I guess it is okay to have them, OTOH I'd expect drivers to depend on MULTICOLOR being available... Best regards, 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
