Re: [PATCH v31 03/12] leds: lp50xx: Add the LP50XX family of the RGB LED driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux