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

[Dan Murphy]

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