Le 10/07/2024 à 20:48, Joseph Strauss a écrit :
Add multicolor support to the BlinkM driver, making it easier to control
from userspace. The BlinkM LED is a programmable RGB LED. The driver
currently supports only the regular LED sysfs class, resulting in the
creation of three distinct classes, one for red, green, and blue. The
user then has to input three values into the three seperate brightness
files within those classes. The multicolor LED framework makes the
device easier to control with the multi_intensity file: the user can
input three values at once to form a color, while still controlling the
lightness with the brightness file.
The main struct blinkm_led has changed slightly. The struct led_classdev
for the regular sysfs classes remain. The blinkm_probe function checks
CONFIG_LEDS_BLINKM_MULTICOLOR to decide whether to load the seperate
sysfs classes or the single multicolor one, but never both. The
blinkm_set_mc_brightness() function had to be added to calculate the
three color components and then set the fields of the blinkm_data
structure accordingly.
Signed-off-by: Joseph Strauss <jstrauss@xxxxxxxxxxx>
---
Hi,
this patch has already been applied, but I have 2 questions/remarks below:
...
-static int blinkm_probe(struct i2c_client *client)
+static int register_separate_colors(struct i2c_client *client, struct blinkm_data *data)
{
- struct blinkm_data *data;
- struct blinkm_led *led[3];
- int err, i;
+ /* 3 separate classes for red, green, and blue respectively */
+ struct blinkm_led *leds[NUM_LEDS];
+ int err;
char blinkm_led_name[28];
-
- data = devm_kzalloc(&client->dev,
- sizeof(struct blinkm_data), GFP_KERNEL);
- if (!data) {
- err = -ENOMEM;
- goto exit;
- }
-
- data->i2c_addr = 0x08;
- /* i2c addr - use fake addr of 0x08 initially (real is 0x09) */
- data->fw_ver = 0xfe;
- /* firmware version - use fake until we read real value
- * (currently broken - BlinkM confused!) */
- data->script_id = 0x01;
- data->i2c_client = client;
-
- i2c_set_clientdata(client, data);
- mutex_init(&data->update_lock);
-
- /* Register sysfs hooks */
- err = sysfs_create_group(&client->dev.kobj, &blinkm_group);
- if (err < 0) {
- dev_err(&client->dev, "couldn't register sysfs group\n");
- goto exit;
- }
-
- for (i = 0; i < 3; i++) {
+ /* Register red, green, and blue sysfs classes */
+ for (int i = 0; i < NUM_LEDS; i++) {
/* RED = 0, GREEN = 1, BLUE = 2 */
- led[i] = &data->blinkm_leds[i];
- led[i]->i2c_client = client;
- led[i]->id = i;
- led[i]->led_cdev.max_brightness = 255;
- led[i]->led_cdev.flags = LED_CORE_SUSPENDRESUME;
+ leds[i] = &data->blinkm_leds[i];
+ leds[i]->i2c_client = client;
+ leds[i]->id = i;
+ leds[i]->cdev.led_cdev.max_brightness = 255;
+ leds[i]->cdev.led_cdev.flags = LED_CORE_SUSPENDRESUME;
switch (i) {
case RED:
- snprintf(blinkm_led_name, sizeof(blinkm_led_name),
+ scnprintf(blinkm_led_name, sizeof(blinkm_led_name),
"blinkm-%d-%d-red",
client->adapter->nr,
client->addr);
- led[i]->led_cdev.name = blinkm_led_name;
- led[i]->led_cdev.brightness_set_blocking =
+ leds[i]->cdev.led_cdev.name = blinkm_led_name;
+ leds[i]->cdev.led_cdev.brightness_set_blocking =
blinkm_led_red_set;
err = led_classdev_register(&client->dev,
- &led[i]->led_cdev);
+ &leds[i]->cdev.led_cdev);
if (err < 0) {
dev_err(&client->dev,
"couldn't register LED %s\n",
- led[i]->led_cdev.name);
+ leds[i]->cdev.led_cdev.name);
goto failred;
}
break;
case GREEN:
- snprintf(blinkm_led_name, sizeof(blinkm_led_name),
+ scnprintf(blinkm_led_name, sizeof(blinkm_led_name),
"blinkm-%d-%d-green",
client->adapter->nr,
client->addr);
- led[i]->led_cdev.name = blinkm_led_name;
- led[i]->led_cdev.brightness_set_blocking =
+ leds[i]->cdev.led_cdev.name = blinkm_led_name;
+ leds[i]->cdev.led_cdev.brightness_set_blocking =
blinkm_led_green_set;
err = led_classdev_register(&client->dev,
- &led[i]->led_cdev);
+ &leds[i]->cdev.led_cdev);
if (err < 0) {
dev_err(&client->dev,
"couldn't register LED %s\n",
- led[i]->led_cdev.name);
+ leds[i]->cdev.led_cdev.name);
goto failgreen;
}
break;
case BLUE:
- snprintf(blinkm_led_name, sizeof(blinkm_led_name),
+ scnprintf(blinkm_led_name, sizeof(blinkm_led_name),
"blinkm-%d-%d-blue",
client->adapter->nr,
client->addr);
- led[i]->led_cdev.name = blinkm_led_name;
- led[i]->led_cdev.brightness_set_blocking =
+ leds[i]->cdev.led_cdev.name = blinkm_led_name;
+ leds[i]->cdev.led_cdev.brightness_set_blocking =
blinkm_led_blue_set;
err = led_classdev_register(&client->dev,
- &led[i]->led_cdev);
+ &leds[i]->cdev.led_cdev);
if (err < 0) {
dev_err(&client->dev,
"couldn't register LED %s\n",
- led[i]->led_cdev.name);
+ leds[i]->cdev.led_cdev.name);
goto failblue;
}
break;
+ default:
+ break;
} /* end switch */
} /* end for */
-
- /* Initialize the blinkm */
- blinkm_init_hw(client);
-
return 0;
failblue:
- led_classdev_unregister(&led[GREEN]->led_cdev);
-
+ led_classdev_unregister(&leds[GREEN]->cdev.led_cdev);
failgreen:
- led_classdev_unregister(&led[RED]->led_cdev);
-
+ led_classdev_unregister(&leds[RED]->cdev.led_cdev);
failred:
sysfs_remove_group(&client->dev.kobj, &blinkm_group);
I think that this would be more logical to have it in an error handling
path in blinkm_probe(), as it was before.
This is strange to un-do sysfs_create_group() here in
register_separate_colors() and...
-exit:
+
return err;
}
+static int register_multicolor(struct i2c_client *client, struct blinkm_data *data)
+{
+ struct blinkm_led *mc_led;
+ struct mc_subled *mc_led_info;
+ char blinkm_led_name[28];
+ int err;
+
+ /* Register multicolor sysfs class */
+ /* The first element of leds is used for multicolor facilities */
+ mc_led = &data->blinkm_leds[RED];
+ mc_led->i2c_client = client;
+
+ mc_led_info = devm_kcalloc(&client->dev, NUM_LEDS, sizeof(*mc_led_info),
+ GFP_KERNEL);
+ if (!mc_led_info)
+ return -ENOMEM;
+
+ mc_led_info[RED].color_index = LED_COLOR_ID_RED;
+ mc_led_info[GREEN].color_index = LED_COLOR_ID_GREEN;
+ mc_led_info[BLUE].color_index = LED_COLOR_ID_BLUE;
+
+ mc_led->cdev.mcled_cdev.subled_info = mc_led_info;
+ mc_led->cdev.mcled_cdev.num_colors = NUM_LEDS;
+ mc_led->cdev.mcled_cdev.led_cdev.brightness = 255;
+ mc_led->cdev.mcled_cdev.led_cdev.max_brightness = 255;
+ mc_led->cdev.mcled_cdev.led_cdev.flags = LED_CORE_SUSPENDRESUME;
+
+ scnprintf(blinkm_led_name, sizeof(blinkm_led_name),
+ "blinkm-%d-%d:rgb:indicator",
+ client->adapter->nr,
+ client->addr);
+ mc_led->cdev.mcled_cdev.led_cdev.name = blinkm_led_name;
+ mc_led->cdev.mcled_cdev.led_cdev.brightness_set_blocking = blinkm_set_mc_brightness;
+
+ err = led_classdev_multicolor_register(&client->dev, &mc_led->cdev.mcled_cdev);
+ if (err < 0) {
+ dev_err(&client->dev, "couldn't register LED %s\n",
+ mc_led->cdev.led_cdev.name);
+ sysfs_remove_group(&client->dev.kobj, &blinkm_group);
... here in register_multicolor().
Also, is it on pourpose that err is *not* propagated?
If led_classdev_multicolor_register() fails, we still return 0.
+ }
+ return 0;
+}
+
+static int blinkm_probe(struct i2c_client *client)
+{
+ struct blinkm_data *data;
+ int err;
+
+ data = devm_kzalloc(&client->dev,
+ sizeof(struct blinkm_data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->i2c_addr = 0x08;
+ /* i2c addr - use fake addr of 0x08 initially (real is 0x09) */
+ data->fw_ver = 0xfe;
+ /* firmware version - use fake until we read real value
+ * (currently broken - BlinkM confused!)
+ */
+ data->script_id = 0x01;
+ data->i2c_client = client;
+
+ i2c_set_clientdata(client, data);
+ mutex_init(&data->update_lock);
+
+ /* Register sysfs hooks */
+ err = sysfs_create_group(&client->dev.kobj, &blinkm_group);
+ if (err < 0) {
+ dev_err(&client->dev, "couldn't register sysfs group\n");
+ return err;
+ }
+
+ if (!IS_ENABLED(CONFIG_LEDS_BLINKM_MULTICOLOR)) {
+ err = register_separate_colors(client, data);
+ if (err < 0)
+ return err;
+ } else {
+ err = register_multicolor(client, data);
+ if (err < 0)
+ return err;
+ }
+
+ blinkm_init_hw(client);
+
+ return 0;
+}
...
CJ
