Re: [PATCH v14 2/2] leds: Add driver for Qualcomm LPG

[Marijn Suijten <marijn.suijten@somainline.org>]

On 2022-04-30 14:35:04, Marijn Suijten wrote:
[..]
> > +static int lpg_add_led(struct lpg *lpg, struct device_node *np)
> > +{
> > +	struct led_init_data init_data = {};
> > +	struct led_classdev *cdev;
> > +	struct device_node *child;
> > +	struct mc_subled *info;
> > +	struct lpg_led *led;
> > +	const char *state;
> > +	int num_channels;
> > +	u32 color = 0;
> > +	int ret;
> > +	int i;
> > +
> > +	ret = of_property_read_u32(np, "color", &color);
> > +	if (ret < 0 && ret != -EINVAL) {
> > +		dev_err(lpg->dev, "failed to parse \"color\" of %pOF\n", np);
> > +		return ret;
> > +	}
> > +
> > +	if (color == LED_COLOR_ID_RGB)
> > +		num_channels = of_get_available_child_count(np);
> > +	else
> > +		num_channels = 1;
> > +
> > +	led = devm_kzalloc(lpg->dev, struct_size(led, channels, num_channels), GFP_KERNEL);
> > +	if (!led)
> > +		return -ENOMEM;
> > +
> > +	led->lpg = lpg;
> > +	led->num_channels = num_channels;
> > +
> > +	if (color == LED_COLOR_ID_RGB) {
> > +		info = devm_kcalloc(lpg->dev, num_channels, sizeof(*info), GFP_KERNEL);
> > +		if (!info)
> > +			return -ENOMEM;
> > +		i = 0;
> > +		for_each_available_child_of_node(np, child) {
> > +			ret = lpg_parse_channel(lpg, child, &led->channels[i]);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			info[i].color_index = led->channels[i]->color;
> > +			info[i].intensity = 0;
> 
> This struct also has a "channel" field that doesn't seem to be used
> anywhere.  Should we still initialize it to something sensible, or is it
> better reported upstream for removal?

Never mind the second bit: lp552[13] uses the `channel` field internally
to store a custom index.  This LPG driver simply iterates over all the
led channels (stored in the LPG struct, with all necessary info) in the
same order as the subleds in the multicolor led device, so it's most
likely of no use for us.

- Marijn