linux-kernel - Re: [PATCH v14 13/19] leds: lp55xx: Add multicolor framework support to lp55xx

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 25 Oct 2019 13:18:46 -0500
From:   Dan Murphy <dmurphy@...com>
To:     Jacek Anaszewski <jacek.anaszewski@...il.com>, <pavel@....cz>
CC:     <linux-leds@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v14 13/19] leds: lp55xx: Add multicolor framework support
 to lp55xx

Jacek

On 10/25/19 1:13 PM, Jacek Anaszewski wrote:
> Dan,
>
> On 10/25/19 7:57 PM, Dan Murphy wrote:
>> Jacek
>>
>> On 10/22/19 12:41 PM, Jacek Anaszewski wrote:
>>> Dan,
>>>
>>> On 10/22/19 6:37 PM, Dan Murphy wrote:
>>>> Jacek
>>>>
>>>> On 10/18/19 4:56 PM, Jacek Anaszewski wrote:
>>>>> On 10/18/19 11:48 PM, Jacek Anaszewski wrote:
>>>>>> Dan,
>>>>> +        ret = lp5xx_parse_channel_child(child, cfg, i);
>>>>>> I went into details of this parsing and finally came up with
>>>>>> the code which is a bit greater in size, but IMHO cleaner.
>>>>>> Note changes in variable naming. It is not even compile-tested.
>>>>>>
>>>>>> static int lp55xx_parse_common_child(struct device_node *np,
>>>>>>                                        struct lp55xx_led_config *cfg,
>>>>>>                                        int led_number, int *chan_nr)
>>>>>> {
>>>>>>            int ret;
>>>>>>
>>>>>>            of_property_read_string(np, "chan-name",
>>>>>>                                    &cfg[led_number].name);
>>>>>>            of_property_read_u8(np, "led-cur",
>>>>>>                                &cfg[led_number].led_current);
>>>>>>            of_property_read_u8(np, "max-cur",
>>>>>>                                &cfg[led_number].max_current);
>>>>>>
>>>>>>            ret = of_property_read_u32(np, "reg", chan_nr);
>>>>>>            if (ret)
>>>>>>                    return ret;
>>>>>>
>>>>>>            if (chan_nr < 0 || chan_nr > cfg->max_chan_nr) /* side note:
>>>>>> new
>>>>>> max_chan_nr property needed in cfg */
>>>>>>                    return -EINVAL;
>>>>>>
>>>>>>            return 0;
>>>>>> }
>>>>>>
>>>>>> static int lp55xx_parse_mutli_led_child(struct device_node *np,
>>>>>>                                            struct lp55xx_led_config
>>>>>> *cfg,
>>>>>>                                            int child_number,
>>>>>>                                            int color_number)
>>>>>> {
>>>>>>            int chan_nr, color_id;
>>>>>>
>>>>>>            ret = lp55xx_parse_common_child(child, cfg, child_number,
>>>>>> color_number,
>>>>>>                                            &chan_nr);
>>>>>>            if (ret)
>>>>>>                    return ret;
>>>>>>
>>>>>>            ret = of_property_read_u32(child, "color", &color_id);
>>>>>>            if (ret)
>>>>>>                   return ret;
>>>>>>
>>>>>>            cfg[child_number].color_components[color_number].color_id =
>>>>>> color_id;
>>>>>>           
>>>>>> cfg[child_number].color_components[color_number].output_num =
>>>>>> chan_nr;
>>>>>>            set_bit(color_id, &cfg[child_number].available_colors);
>>>>>>
>>>>>>            return 0;
>>>>>> }
>>>>>>
>>>>>> staitc int lp55xx_parse_mutli_led(struct device_node *np,
>>>>>>                                      struct lp55xx_led_config *cfg,
>>>>>>                                      int child_number)
>>>>>> {
>>>>>>            struct device_node *child;
>>>>>>            int num_colors = 0, i = 0;
>>>>> s/, i = 0//
>>>>>
>>>>>>            for_each_child_of_node(np, child) {
>>>>>>                    ret = lp55xx_parse_mutli_led_child(child, cfg,
>>>>>> num_colors,
>>>>>>                                                       child_number, i))
>>>>> Replace above call with below:
>>>>>
>>>>> ret = lp55xx_parse_mutli_led_child(child, cfg, child_number,
>>>>> num_colors);
>>>>>
>>>> I applied your DT parser patch from the v13 series.  Which eliminates
>>>> this comment correct?
>>> Yes, it contains this fix.
>>>
>> OK I added your patch and it broke a lot of the DT parsing for the LP55xx.
>>
>> I would prefer to stick with the original code without having to
>> re-write this again.
> Let me test that with Qemu first.
>
max_channel is never set so not sure where that is supposed to come from 
since each child device has different number of channels.  And if the 
user has to populate that information from the DT then it does not make 
sense as the user would already be aware of the number of channels.  
This information would have to come from the child device some how and 
the children do not have access to the structure to set it.

In checking the chan_nr to the max_channels you are comparing a pointer 
to an integer.  Easy fix but did not solve the registration issues.

cfg->num_colors is not set anywhere so the registration always goes to 
base led_registration.  Unless we key off a different value to determine 
to register to multicolor class or not.  Or we default this to the 
multi_class registration to figure out how to register based on 
available_colors.

That is what I am seeing so far in my debugging and I still don't have 
it working.

Dan