Messages in this thread | | | Subject | Re: [PATCH v31 03/12] leds: lp50xx: Add the LP50XX family of the RGB LED driver | From | Dan Murphy <> | Date | Tue, 21 Jul 2020 19:04:54 -0500 |
| |
Pavel
On 7/21/20 4:05 PM, Pavel Machek wrote: > Hi! > >> The device has the ability to group LED output into control banks >> so that multiple LED banks can be controlled with the same mixing and >> brightness. Inversely the LEDs can also be controlled independently. > Inversely? I will revise it. > >> --- /dev/null >> +++ b/drivers/leds/leds-lp50xx.c >> @@ -0,0 +1,784 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// TI LP50XX LED chip family driver >> +// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/ >> + > Can we get https here and in the binding document? > > Please run this through checkpatch -- I believe it will have some > comments. OK. > >> + >> + device_for_each_child_node(priv->dev, child) { >> + led = &priv->leds[i]; >> + ret = fwnode_property_count_u32(child, "reg"); >> + if (ret < 0) { >> + dev_err(&priv->client->dev, >> + "reg property is invalid\n"); >> + return -EINVAL; > is handle_put(child) needed here?
It will be after I refactor the label
>> + } >> + if (ret > 1) { >> + priv->num_of_banked_leds = ret; >> + if (priv->num_of_banked_leds > >> + priv->chip_info->max_modules) { >> + dev_err(&priv->client->dev, >> + "reg property is invalid\n"); >> + ret = -EINVAL; >> + fwnode_handle_put(child); >> + goto child_out; >> + } >> + >> + ret = fwnode_property_read_u32_array(child, >> + "reg", >> + led_banks, >> + ret); > Move this to subfunction to reduce the indentation? (Or, just refactor > it somehow).
Actually I can just put it all on the same line since the 80 character requirement is relaxed.
>> + if (ret) { >> + dev_err(&priv->client->dev, >> + "reg property is missing\n"); >> + fwnode_handle_put(child); >> + goto child_out; >> + } > Create label that does the handle_put so you don't need to repeat it > quite so often? I will rework it for all > >> + fwnode_for_each_child_node(child, led_node) { >> + ret = fwnode_property_read_u32(led_node, "color", >> + &color_id); >> + if (ret) >> + dev_err(priv->dev, "Cannot read color\n"); >> + >> + mc_led_info[num_colors].color_index = color_id; > This uses undefined value. OK needs to goto to out. > >> + ret = lp50xx_reset(led); > Does the GPIO need to be disabled before enabling it for reset?
You mean toggle the GPIO? Yes it should be toggled I will update it.
Dan
> Best regards, > Pavel >
| |