lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <d1771420-b45e-3ce1-f469-bc5fd9ab6dfa@traphandler.com> Date: Tue, 28 Mar 2023 17:31:36 +0200 From: Jean-Jacques Hiblot <jjhiblot@...phandler.com> To: Lee Jones <lee@...nel.org> CC: <lee.jones@...aro.org>, <pavel@....cz>, <linux-leds@...r.kernel.org>, <linux-kernel@...r.kernel.org> Subject: Re: [PATCH v7 6/6] leds: Add a multicolor LED driver to group monochromatic LEDs On 15/03/2023 16:52, Lee Jones wrote: >> + for (;;) { >> + struct led_classdev *led_cdev; >> + >> + led_cdev = devm_of_led_get_optional(dev, count); >> + if (IS_ERR(led_cdev)) > > Doesn't devm_of_led_get_optional() return NULL on failure? Hi Lee, Thanks for you review. I'll send an updated version shortly. devm_of_led_get_optional() return an error-pointer when it cannot get the LED (like EPROBE_DEFER or ENOMEM). When the LED is not defined, it returns NULL. > >> + return dev_err_probe(dev, PTR_ERR(led_cdev), >> + "Unable to get led #%d", count); >> + /* Reached the end of the list ?*/ > > Besides the incorrect formatting, that '?' isn't filling me with > confidence. The comment just meant that NULL indicates the end of the list of the LEDs. JJ > >> + if (!led_cdev) >> + break; >> + >> + priv->monochromatics = devm_krealloc_array(dev, priv->monochromatics, >> + count + 1, sizeof(*priv->monochromatics), >> + GFP_KERNEL); >> + if (!priv->monochromatics) >> + return -ENOMEM; >> + >> + priv->monochromatics[count] = led_cdev; >> + >> + max_brightness = max(max_brightness, led_cdev->max_brightness); >> + count++; >> + } >> + >> + subled = devm_kcalloc(dev, count, sizeof(*subled), GFP_KERNEL); >> + if (!subled) >> + return -ENOMEM; >> + priv->mc_cdev.subled_info = subled; >> + >> + for (i = 0; i < count; i++) { >> + struct led_classdev *led_cdev = priv->monochromatics[i]; >> + >> + subled[i].color_index = led_cdev->color; > > '\n' > >> + /* configure the LED intensity to its maximum */ > > Use correct grammar in comments please. > > Start with an uppercase char. > >> + subled[i].intensity = max_brightness; >> + } >> + >> + /* init the multicolor's LED class device */ > > As above and please be fully forthcoming in comments: "Initialise". > >> + cdev = &priv->mc_cdev.led_cdev; >> + cdev->flags = LED_CORE_SUSPENDRESUME; >> + cdev->brightness_set_blocking = led_mcg_set; >> + cdev->max_brightness = max_brightness; >> + cdev->color = LED_COLOR_ID_MULTI; >> + priv->mc_cdev.num_colors = count; >> + >> + init_data.fwnode = dev_fwnode(dev); >> + ret = devm_led_classdev_multicolor_register_ext(dev, &priv->mc_cdev, >> + &init_data); > > Use the full 100-char limit everywhere please. > >> + if (ret) >> + return dev_err_probe(dev, ret, >> + "failed to register multicolor led for %s.\n", > > "LED" > >> + cdev->name); >> + >> + ret = led_mcg_set(cdev, cdev->brightness); >> + if (ret) >> + return dev_err_probe(dev, ret, >> + "failed to set led value for %s.", >> + cdev->name); >> + >> + for (i = 0; i < count; i++) { >> + struct led_classdev *led_cdev = priv->monochromatics[i]; >> + >> + /* Make the sysfs of the monochromatic LED read-only */ >> + mutex_lock(&led_cdev->led_access); >> + led_sysfs_disable(led_cdev); >> + mutex_unlock(&led_cdev->led_access); >> + >> + /* Restore sysfs access when the multicolor LED is released */ >> + devm_add_action_or_reset(dev, restore_sysfs_access, led_cdev); >> + } >> + >> + return 0; >> +} >> + >> +static const struct of_device_id of_led_mcg_match[] = { >> + { .compatible = "leds-group-multicolor" }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, of_led_mcg_match); >> + >> +static struct platform_driver led_mcg_driver = { >> + .probe = led_mcg_probe, >> + .driver = { >> + .name = "leds_group_multicolor", >> + .of_match_table = of_led_mcg_match, >> + } >> +}; >> +module_platform_driver(led_mcg_driver); >> + >> +MODULE_AUTHOR("Jean-Jacques Hiblot <jjhiblot@...phandler.com>"); >> +MODULE_DESCRIPTION("multi-color LED group driver"); >> +MODULE_LICENSE("GPL"); >> +MODULE_ALIAS("platform:leds-group-multicolor"); >> -- >> 2.25.1 >> > > -- > Lee Jones [李琼斯]
Powered by blists - more mailing lists