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
