| Message ID | 20230314181824.56881-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d565263b7d83371e64ef315bdc3428909808b46f |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v6,1/1] net: dsa: hellcreek: Get rid of custom led_init_default_state_get() | expand |
Hi Andy,
I love your patch! Yet something to improve:
[auto build test ERROR on net-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/net-dsa-hellcreek-Get-rid-of-custom-led_init_default_state_get/20230315-021931
patch link: https://lore.kernel.org/r/20230314181824.56881-1-andriy.shevchenko%40linux.intel.com
patch subject: [PATCH net-next v6 1/1] net: dsa: hellcreek: Get rid of custom led_init_default_state_get()
config: i386-randconfig-a015-20230313 (https://download.01.org/0day-ci/archive/20230315/202303150831.vgyKe8FD-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/fdd54417a75386e7ad47065c21403835b7fda94a
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Andy-Shevchenko/net-dsa-hellcreek-Get-rid-of-custom-led_init_default_state_get/20230315-021931
git checkout fdd54417a75386e7ad47065c21403835b7fda94a
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/net/dsa/hirschmann/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202303150831.vgyKe8FD-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/net/dsa/hirschmann/hellcreek_ptp.c:322:10: error: implicit declaration of function 'led_init_default_state_get' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
state = led_init_default_state_get(of_fwnode_handle(led));
^
1 error generated.
vim +/led_init_default_state_get +322 drivers/net/dsa/hirschmann/hellcreek_ptp.c
292
293 /* There two available LEDs internally called sync_good and is_gm. However, the
294 * user might want to use a different label and specify the default state. Take
295 * those properties from device tree.
296 */
297 static int hellcreek_led_setup(struct hellcreek *hellcreek)
298 {
299 struct device_node *leds, *led = NULL;
300 enum led_default_state state;
301 const char *label;
302 int ret = -EINVAL;
303
304 of_node_get(hellcreek->dev->of_node);
305 leds = of_find_node_by_name(hellcreek->dev->of_node, "leds");
306 if (!leds) {
307 dev_err(hellcreek->dev, "No LEDs specified in device tree!\n");
308 return ret;
309 }
310
311 hellcreek->status_out = 0;
312
313 led = of_get_next_available_child(leds, led);
314 if (!led) {
315 dev_err(hellcreek->dev, "First LED not specified!\n");
316 goto out;
317 }
318
319 ret = of_property_read_string(led, "label", &label);
320 hellcreek->led_sync_good.name = ret ? "sync_good" : label;
321
> 322 state = led_init_default_state_get(of_fwnode_handle(led));
323 switch (state) {
324 case LEDS_DEFSTATE_ON:
325 hellcreek->led_sync_good.brightness = 1;
326 break;
327 case LEDS_DEFSTATE_KEEP:
328 hellcreek->led_sync_good.brightness =
329 hellcreek_get_brightness(hellcreek, STATUS_OUT_SYNC_GOOD);
330 break;
331 default:
332 hellcreek->led_sync_good.brightness = 0;
333 }
334
335 hellcreek->led_sync_good.max_brightness = 1;
336 hellcreek->led_sync_good.brightness_set = hellcreek_led_sync_good_set;
337 hellcreek->led_sync_good.brightness_get = hellcreek_led_sync_good_get;
338
339 led = of_get_next_available_child(leds, led);
340 if (!led) {
341 dev_err(hellcreek->dev, "Second LED not specified!\n");
342 ret = -EINVAL;
343 goto out;
344 }
345
346 ret = of_property_read_string(led, "label", &label);
347 hellcreek->led_is_gm.name = ret ? "is_gm" : label;
348
349 state = led_init_default_state_get(of_fwnode_handle(led));
350 switch (state) {
351 case LEDS_DEFSTATE_ON:
352 hellcreek->led_is_gm.brightness = 1;
353 break;
354 case LEDS_DEFSTATE_KEEP:
355 hellcreek->led_is_gm.brightness =
356 hellcreek_get_brightness(hellcreek, STATUS_OUT_IS_GM);
357 break;
358 default:
359 hellcreek->led_is_gm.brightness = 0;
360 }
361
362 hellcreek->led_is_gm.max_brightness = 1;
363 hellcreek->led_is_gm.brightness_set = hellcreek_led_is_gm_set;
364 hellcreek->led_is_gm.brightness_get = hellcreek_led_is_gm_get;
365
366 /* Set initial state */
367 if (hellcreek->led_sync_good.brightness == 1)
368 hellcreek_set_brightness(hellcreek, STATUS_OUT_SYNC_GOOD, 1);
369 if (hellcreek->led_is_gm.brightness == 1)
370 hellcreek_set_brightness(hellcreek, STATUS_OUT_IS_GM, 1);
371
372 /* Register both leds */
373 led_classdev_register(hellcreek->dev, &hellcreek->led_sync_good);
374 led_classdev_register(hellcreek->dev, &hellcreek->led_is_gm);
375
376 ret = 0;
377
378 out:
379 of_node_put(leds);
380
381 return ret;
382 }
383
On Tue, Mar 14, 2023 at 08:18:24PM +0200, Andy Shevchenko wrote: > LED core provides a helper to parse default state from firmware node. > Use it instead of custom implementation. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de> > --- > v6: wrapped long lines (Simon, Jakub) > drivers/net/dsa/hirschmann/hellcreek_ptp.c | 45 ++++++++++++---------- > 1 file changed, 24 insertions(+), 21 deletions(-) > > diff --git a/drivers/net/dsa/hirschmann/hellcreek_ptp.c b/drivers/net/dsa/hirschmann/hellcreek_ptp.c > index b28baab6d56a..3e44ccb7db84 100644 > --- a/drivers/net/dsa/hirschmann/hellcreek_ptp.c > +++ b/drivers/net/dsa/hirschmann/hellcreek_ptp.c > @@ -297,7 +297,8 @@ static enum led_brightness hellcreek_led_is_gm_get(struct led_classdev *ldev) > static int hellcreek_led_setup(struct hellcreek *hellcreek) > { > struct device_node *leds, *led = NULL; > - const char *label, *state; > + enum led_default_state state; > + const char *label; > int ret = -EINVAL; > > of_node_get(hellcreek->dev->of_node); > @@ -318,16 +319,17 @@ static int hellcreek_led_setup(struct hellcreek *hellcreek) > ret = of_property_read_string(led, "label", &label); > hellcreek->led_sync_good.name = ret ? "sync_good" : label; > > - ret = of_property_read_string(led, "default-state", &state); > - if (!ret) { > - if (!strcmp(state, "on")) > - hellcreek->led_sync_good.brightness = 1; > - else if (!strcmp(state, "off")) > - hellcreek->led_sync_good.brightness = 0; > - else if (!strcmp(state, "keep")) > - hellcreek->led_sync_good.brightness = > - hellcreek_get_brightness(hellcreek, > - STATUS_OUT_SYNC_GOOD); > + state = led_init_default_state_get(of_fwnode_handle(led)); > + switch (state) { > + case LEDS_DEFSTATE_ON: > + hellcreek->led_sync_good.brightness = 1; > + break; > + case LEDS_DEFSTATE_KEEP: > + hellcreek->led_sync_good.brightness = > + hellcreek_get_brightness(hellcreek, STATUS_OUT_SYNC_GOOD); > + break; > + default: > + hellcreek->led_sync_good.brightness = 0; > } > > hellcreek->led_sync_good.max_brightness = 1; > @@ -344,16 +346,17 @@ static int hellcreek_led_setup(struct hellcreek *hellcreek) > ret = of_property_read_string(led, "label", &label); > hellcreek->led_is_gm.name = ret ? "is_gm" : label; > > - ret = of_property_read_string(led, "default-state", &state); > - if (!ret) { > - if (!strcmp(state, "on")) > - hellcreek->led_is_gm.brightness = 1; > - else if (!strcmp(state, "off")) > - hellcreek->led_is_gm.brightness = 0; > - else if (!strcmp(state, "keep")) > - hellcreek->led_is_gm.brightness = > - hellcreek_get_brightness(hellcreek, > - STATUS_OUT_IS_GM); > + state = led_init_default_state_get(of_fwnode_handle(led)); > + switch (state) { > + case LEDS_DEFSTATE_ON: > + hellcreek->led_is_gm.brightness = 1; > + break; > + case LEDS_DEFSTATE_KEEP: > + hellcreek->led_is_gm.brightness = > + hellcreek_get_brightness(hellcreek, STATUS_OUT_IS_GM); > + break; > + default: > + hellcreek->led_is_gm.brightness = 0; Hi, You have to fix implict declaration of the led_init_default_state_get(). I wonder if the code duplication here can be avoided: static void set_led_brightness(hellcreek, led, *brightness, status) { enum led_default_state state = led_init_default_state_get(of_fwnode_handle(led)); switch(state) { case LEDS_DEFSTATE_ON: *brightness = 1; break; case LEDS_DEFSTATE_KEEP: *brightness = hellcreek_get_brightness(hellcreek, status); break; default: *brightness = 0; } } and call it like: set_led_brightness(heelcreek, led, &heelcreek->..., STATUS_OUT_IS_GM/SYNC_GOOD); Only suggestion, patch looks good. Thanks, Michal > } > > hellcreek->led_is_gm.max_brightness = 1; > -- > 2.39.2 >
On Wed, Mar 15, 2023 at 06:57:37AM +0100, Michal Swiatkowski wrote: > On Tue, Mar 14, 2023 at 08:18:24PM +0200, Andy Shevchenko wrote: > > LED core provides a helper to parse default state from firmware node. > > Use it instead of custom implementation. ... > You have to fix implict declaration of the led_init_default_state_get(). Seems like users have to choose between 'select NEW_LEDS' and 'depends on NEW_LEDS' in the Kconfig. > I wonder if the code duplication here can be avoided: Whether or not this is out of the scope of this patch. Feel free to submit one :-) ... > Only suggestion, patch looks good. Thank you!
On Wed, Mar 15, 2023 at 09:05:25AM +0800, kernel test robot wrote: > Hi Andy, > > I love your patch! Yet something to improve: > > [auto build test ERROR on net-next/master] > > url: https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/net-dsa-hellcreek-Get-rid-of-custom-led_init_default_state_get/20230315-021931 > patch link: https://lore.kernel.org/r/20230314181824.56881-1-andriy.shevchenko%40linux.intel.com > patch subject: [PATCH net-next v6 1/1] net: dsa: hellcreek: Get rid of custom led_init_default_state_get() > config: i386-randconfig-a015-20230313 (https://download.01.org/0day-ci/archive/20230315/202303150831.vgyKe8FD-lkp@intel.com/config) > compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1) > reproduce (this is a W=1 build): > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # https://github.com/intel-lab-lkp/linux/commit/fdd54417a75386e7ad47065c21403835b7fda94a > git remote add linux-review https://github.com/intel-lab-lkp/linux > git fetch --no-tags linux-review Andy-Shevchenko/net-dsa-hellcreek-Get-rid-of-custom-led_init_default_state_get/20230315-021931 > git checkout fdd54417a75386e7ad47065c21403835b7fda94a > # save the config file > mkdir build_dir && cp config build_dir/.config > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/net/dsa/hirschmann/ > > If you fix the issue, kindly add following tag where applicable > | Reported-by: kernel test robot <lkp@intel.com> > | Link: https://lore.kernel.org/oe-kbuild-all/202303150831.vgyKe8FD-lkp@intel.com/ > > All errors (new ones prefixed by >>): > > >> drivers/net/dsa/hirschmann/hellcreek_ptp.c:322:10: error: implicit declaration of function 'led_init_default_state_get' is invalid in C99 [-Werror,-Wimplicit-function-declaration] > state = led_init_default_state_get(of_fwnode_handle(led)); > ^ > 1 error generated. I can not reproduce it. I have downloaded net-next and applied the only patch on top. I have downloaded the above mentioned kernel configuration and repeated the steps with `make ... oldconfig; make W=1 ...` Can you shed a light on what's going on here? Note, the bug is impossibly related to my patch because the new API is in the same header as already used from the LEDS framework. If it's reproducible, it should be also without my patch.
On Wed, Mar 15, 2023 at 06:57:37AM +0100, Michal Swiatkowski wrote: > On Tue, Mar 14, 2023 at 08:18:24PM +0200, Andy Shevchenko wrote: ... > You have to fix implict declaration of the led_init_default_state_get(). FWIW, I have spent more than a couple of hours on this and have no idea how it's even possible. If there is a bug it must have been reproduced without my patch.
On Wed, Mar 15, 2023 at 07:06:35PM +0200, Andy Shevchenko wrote: > On Wed, Mar 15, 2023 at 09:05:25AM +0800, kernel test robot wrote: > > Hi Andy, > > > > I love your patch! Yet something to improve: > > > > [auto build test ERROR on net-next/master] > > > > url: https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/net-dsa-hellcreek-Get-rid-of-custom-led_init_default_state_get/20230315-021931 > > patch link: https://lore.kernel.org/r/20230314181824.56881-1-andriy.shevchenko%40linux.intel.com > > patch subject: [PATCH net-next v6 1/1] net: dsa: hellcreek: Get rid of custom led_init_default_state_get() > > config: i386-randconfig-a015-20230313 (https://download.01.org/0day-ci/archive/20230315/202303150831.vgyKe8FD-lkp@intel.com/config) > > compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1) > > reproduce (this is a W=1 build): > > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > > chmod +x ~/bin/make.cross > > # https://github.com/intel-lab-lkp/linux/commit/fdd54417a75386e7ad47065c21403835b7fda94a > > git remote add linux-review https://github.com/intel-lab-lkp/linux > > git fetch --no-tags linux-review Andy-Shevchenko/net-dsa-hellcreek-Get-rid-of-custom-led_init_default_state_get/20230315-021931 > > git checkout fdd54417a75386e7ad47065c21403835b7fda94a > > # save the config file > > mkdir build_dir && cp config build_dir/.config > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 olddefconfig > > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/net/dsa/hirschmann/ > > > > If you fix the issue, kindly add following tag where applicable > > | Reported-by: kernel test robot <lkp@intel.com> > > | Link: https://lore.kernel.org/oe-kbuild-all/202303150831.vgyKe8FD-lkp@intel.com/ > > > > All errors (new ones prefixed by >>): > > > > >> drivers/net/dsa/hirschmann/hellcreek_ptp.c:322:10: error: implicit declaration of function 'led_init_default_state_get' is invalid in C99 [-Werror,-Wimplicit-function-declaration] > > state = led_init_default_state_get(of_fwnode_handle(led)); > > ^ > > 1 error generated. > > I can not reproduce it. > > I have downloaded net-next and applied the only patch on top. > I have downloaded the above mentioned kernel configuration and > repeated the steps with `make ... oldconfig; make W=1 ...` > > Can you shed a light on what's going on here? > > Note, the bug is impossibly related to my patch because the new API is in the > same header as already used from the LEDS framework. If it's reproducible, it > should be also without my patch. If you modify the GitHub link above the 'git remote' command above from 'commit' to 'commits', you can see that your patch was applied on top of mainline commit 5b7c4cabbb65 ("Merge tag 'net-next-6.3' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next"), which was before the pull that moved led_init_default_state_get() into include/linux/leds.h, commit e4bc15889506 ("Merge tag 'leds-next-6.3' of git://git.kernel.org/pub/scm/linux/kernel/git/lee/leds"). Not sure why that was the base that was chosen but it explains the error. Cheers, Nathan
On Wed, 15 Mar 2023 12:51:54 -0700 Nathan Chancellor wrote: > If you modify the GitHub link above the 'git remote' command above from > 'commit' to 'commits', you can see that your patch was applied on top of > mainline commit 5b7c4cabbb65 ("Merge tag 'net-next-6.3' of > git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next"), which > was before the pull that moved led_init_default_state_get() into > include/linux/leds.h, commit e4bc15889506 ("Merge tag 'leds-next-6.3' of > git://git.kernel.org/pub/scm/linux/kernel/git/lee/leds"). Not sure why > that was the base that was chosen but it explains the error. Because they still haven't moved to using the main branch of netdev trees, they try to pull master :| I'll email them directly, I think they don't see the in-reply messages.
On Wed, Mar 15, 2023 at 02:15:38PM -0700, Jakub Kicinski wrote: > On Wed, 15 Mar 2023 12:51:54 -0700 Nathan Chancellor wrote: > > If you modify the GitHub link above the 'git remote' command above from > > 'commit' to 'commits', you can see that your patch was applied on top of > > mainline commit 5b7c4cabbb65 ("Merge tag 'net-next-6.3' of > > git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next"), which > > was before the pull that moved led_init_default_state_get() into > > include/linux/leds.h, commit e4bc15889506 ("Merge tag 'leds-next-6.3' of > > git://git.kernel.org/pub/scm/linux/kernel/git/lee/leds"). Not sure why > > that was the base that was chosen but it explains the error. > > Because they still haven't moved to using the main branch of netdev > trees, they try to pull master :| I'll email them directly, I think > they don't see the in-reply messages. Sorry for missing the early information, we didn't notice it and still apply to the wrong branch. We will resolve this asap to use main branch instead. Thanks >
On Wed, Mar 15, 2023 at 03:16:42PM +0200, Andy Shevchenko wrote: > On Wed, Mar 15, 2023 at 06:57:37AM +0100, Michal Swiatkowski wrote: > > On Tue, Mar 14, 2023 at 08:18:24PM +0200, Andy Shevchenko wrote: > > > LED core provides a helper to parse default state from firmware node. > > > Use it instead of custom implementation. > > ... > > > You have to fix implict declaration of the led_init_default_state_get(). > > Seems like users have to choose between 'select NEW_LEDS' and > 'depends on NEW_LEDS' in the Kconfig. > > > I wonder if the code duplication here can be avoided: > > Whether or not this is out of the scope of this patch. > Feel free to submit one :-) > > ... > Reasonable ;) Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > > Only suggestion, patch looks good. > > Thank you! > > -- > With Best Regards, > Andy Shevchenko > >
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Tue, 14 Mar 2023 20:18:24 +0200 you wrote: > LED core provides a helper to parse default state from firmware node. > Use it instead of custom implementation. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de> > --- > v6: wrapped long lines (Simon, Jakub) > drivers/net/dsa/hirschmann/hellcreek_ptp.c | 45 ++++++++++++---------- > 1 file changed, 24 insertions(+), 21 deletions(-) Here is the summary with links: - [net-next,v6,1/1] net: dsa: hellcreek: Get rid of custom led_init_default_state_get() https://git.kernel.org/netdev/net-next/c/d565263b7d83 You are awesome, thank you!
diff --git a/drivers/net/dsa/hirschmann/hellcreek_ptp.c b/drivers/net/dsa/hirschmann/hellcreek_ptp.c index b28baab6d56a..3e44ccb7db84 100644 --- a/drivers/net/dsa/hirschmann/hellcreek_ptp.c +++ b/drivers/net/dsa/hirschmann/hellcreek_ptp.c @@ -297,7 +297,8 @@ static enum led_brightness hellcreek_led_is_gm_get(struct led_classdev *ldev) static int hellcreek_led_setup(struct hellcreek *hellcreek) { struct device_node *leds, *led = NULL; - const char *label, *state; + enum led_default_state state; + const char *label; int ret = -EINVAL; of_node_get(hellcreek->dev->of_node); @@ -318,16 +319,17 @@ static int hellcreek_led_setup(struct hellcreek *hellcreek) ret = of_property_read_string(led, "label", &label); hellcreek->led_sync_good.name = ret ? "sync_good" : label; - ret = of_property_read_string(led, "default-state", &state); - if (!ret) { - if (!strcmp(state, "on")) - hellcreek->led_sync_good.brightness = 1; - else if (!strcmp(state, "off")) - hellcreek->led_sync_good.brightness = 0; - else if (!strcmp(state, "keep")) - hellcreek->led_sync_good.brightness = - hellcreek_get_brightness(hellcreek, - STATUS_OUT_SYNC_GOOD); + state = led_init_default_state_get(of_fwnode_handle(led)); + switch (state) { + case LEDS_DEFSTATE_ON: + hellcreek->led_sync_good.brightness = 1; + break; + case LEDS_DEFSTATE_KEEP: + hellcreek->led_sync_good.brightness = + hellcreek_get_brightness(hellcreek, STATUS_OUT_SYNC_GOOD); + break; + default: + hellcreek->led_sync_good.brightness = 0; } hellcreek->led_sync_good.max_brightness = 1; @@ -344,16 +346,17 @@ static int hellcreek_led_setup(struct hellcreek *hellcreek) ret = of_property_read_string(led, "label", &label); hellcreek->led_is_gm.name = ret ? "is_gm" : label; - ret = of_property_read_string(led, "default-state", &state); - if (!ret) { - if (!strcmp(state, "on")) - hellcreek->led_is_gm.brightness = 1; - else if (!strcmp(state, "off")) - hellcreek->led_is_gm.brightness = 0; - else if (!strcmp(state, "keep")) - hellcreek->led_is_gm.brightness = - hellcreek_get_brightness(hellcreek, - STATUS_OUT_IS_GM); + state = led_init_default_state_get(of_fwnode_handle(led)); + switch (state) { + case LEDS_DEFSTATE_ON: + hellcreek->led_is_gm.brightness = 1; + break; + case LEDS_DEFSTATE_KEEP: + hellcreek->led_is_gm.brightness = + hellcreek_get_brightness(hellcreek, STATUS_OUT_IS_GM); + break; + default: + hellcreek->led_is_gm.brightness = 0; } hellcreek->led_is_gm.max_brightness = 1;