On Tue, Jul 26, 2022 at 8:18 PM Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: ... > > Just for saving memory space. > > Because these led_classdevs do not be used at the same time. > > Or do you think it would be better to rewrite it as follows? > > ------------------------------------------------------------------------------------- > > struct mt6370_led { > > struct led_classdev isink; > > struct led_classdev_mc mc; > > struct mt6370_priv *priv; > > u32 default_state; > > u32 index; > > }; > > ------------------------------------------------------------------------------------- > > You obviously didn't get what I'm talking about... > Each union to work properly should have an associated variable that > holds the information of which field of the union is in use. Do you > have such a variable? If not, how does your code know which one to > use? If yes, add a proper comment there. > Ummm... from my understanding, if the colors of these four LEDs are set to 'LED_COLOR_ID_RGB' or 'LED_COLOR_ID_MULTI' in DT, their 'led->index' will be set to 'MT6370_VIRTUAL_MULTICOLOR' in 'mt6370_leds_probe()'. If so, these led devices will be set as 'struct led_classdev_mc' and use related ops functions in 'mt6370_init_led_properties()'. Instead, they whose 'led->index' is not 'MT6370_VIRTUAL_MULTICOLOR' will be set as 'struct led_classdev'. So, maybe the member 'index' of the 'struct mt6370_led' is what you describe the information of which field of the union is in use? I will add the proper comment here to describe this thing. I'm so sorry for misunderstanding your mean last time. Thanks again for your review. -- Best Regards, ChiaEn Wu