First off, bear with me here, I only recently started upstreaming
patches to kernel. It still feels like navigating an extremely busy
shipping lane... Either way, your feedback is highly valued.
On 2/2/23 14:15, Krzysztof Kozlowski wrote:
diff --git a/include/dt-bindings/leds/leds-lp55xx.h b/include/dt-bindings/leds/leds-lp55xx.h
new file mode 100644
index 000000000000..8f59c1c12dee
--- /dev/null
+++ b/include/dt-bindings/leds/leds-lp55xx.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _DT_BINDINGS_LEDS_LP55XX_H
+#define _DT_BINDINGS_LEDS_LP55XX_H
+
+#define LP55XX_CP_OFF 0
+#define LP55XX_CP_BYPASS 1
+#define LP55XX_CP_BOOST 2
+#define LP55XX_CP_AUTO 3
Additionally, these are not used, so it's a dead binding. Drop. Sorry,
this is not the approach you should take.
Best regards,
Krzysztof
These definitions are intended to be used in the DTS's, so it seems
normal to me that most of them go unused in the code? What am I missing?
As for the changes v2 > v3, this was based on input I got on v2 from Lee
Jones, maintainer for leds, on the implementation of the parsing of this
option:
+ pdata->charge_pump_mode = LP55XX_CP_AUTO;
+ ret = of_property_read_string(np, "ti,charge-pump-mode", &pm);
+ if (!ret) {
+ for (cp_mode = LP55XX_CP_OFF;
+ cp_mode < ARRAY_SIZE(charge_pump_modes);
+ cp_mode++) {
+ if (!strcasecmp(pm, charge_pump_modes[cp_mode])) {
+ pdata->charge_pump_mode = cp_mode;
+ break;
+ }
+ }
+ }
A little over-engineered, no?
Why not make the property a numerical value, then simply:
ret = of_property_read_u32(np, "ti,charge-pump-mode", &pdata->charge_pump_mode);
if (ret)
data->charge_pump_mode = LP55XX_CP_AUTO;
I found examples of similar configuration options of both types with
other drivers in the kernel tree (ie string & uint8). I can appreciate
both viewpoints but unfortunately cannot comply with both.
Best regards,
Maarten
