Re: [PATCH v2 8/9] leds: add LM3633 driver

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

 




Hi Jacek,

Thanks for your detailed feedback. Please refer to my comments below.

On 11/27/2015 8:19 PM, Jacek Anaszewski wrote:
Hi Milo,

Thanks for the update. I have few comments below.

diff --git a/drivers/leds/leds-lm3633.c b/drivers/leds/leds-lm3633.c
new file mode 100644
index 0000000..9c391ca
--- /dev/null
+++ b/drivers/leds/leds-lm3633.c
@@ -0,0 +1,840 @@
+/*
+ * TI LM3633 LED driver
+ *
+ * Copyright 2015 Texas Instruments
+ *
+ * Author: Milo Kim <milo.kim@xxxxxx>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/bitops.h>
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/mfd/ti-lmu.h>
+#include <linux/mfd/ti-lmu-register.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define LM3633_MAX_PWM				255
+#define LM3633_MIN_CURRENT			5000
+#define LM3633_MAX_CURRENT			30000
+#define LM3633_MAX_PERIOD			9700
+#define LM3633_SHORT_TIMESTEP			16
+#define LM3633_LONG_TIMESTEP			131
+#define LM3633_TIME_OFFSET			61
+#define LM3633_PATTERN_REG_OFFSET		16
+#define LM3633_IMAX_OFFSET			6
+
+enum lm3633_led_bank_id {
+	LM3633_LED_BANK_C,
+	LM3633_LED_BANK_D,
+	LM3633_LED_BANK_E,
+	LM3633_LED_BANK_F,
+	LM3633_LED_BANK_G,
+	LM3633_LED_BANK_H,
+	LM3633_MAX_LEDS,
+};
+
+/**
+ * struct ti_lmu_led_chip
+ *
+ * @dev:		Parent device pointer
+ * @lmu:		LMU structure. Used for register R/W access.
+ * @lock:		Secure handling for multiple user interface access
+ * @lmu_led:		Multiple LED strings

Could you clarify what "string" means here?

+ * @num_leds:		Number of LED strings

and here?

Oops! I forgot to replace this term with 'output channel'. Thanks for catching this.

+static u8 lm3633_convert_time_to_index(unsigned int msec)
+{
+	u8 idx, offset;
+
+	/*
+	 * Find an approximate index

I think that we shouldn't approximate but clamp the msec
to the nearest possible device setting.

+	 *
+	 *      0 <= time <= 1000 : 16ms step
+	 *   1000 <  time <= 9700 : 131ms step, base index is 61
+	 */
+
+	msec = min_t(int, msec, LM3633_MAX_PERIOD);
+
+	if (msec <= 1000) {
+		idx = msec / LM3633_SHORT_TIMESTEP;
+		if (idx > 1)
+			idx--;
+		offset = 0;
+	} else {
+		idx = (msec - 1000) / LM3633_LONG_TIMESTEP;
+		offset = LM3633_TIME_OFFSET;
+	}
+
+	return idx + offset;
+}
+
+static u8 lm3633_convert_ramp_to_index(unsigned int msec)
+{
+	const int ramp_table[] = { 2, 250, 500, 1000, 2000, 4000, 8000, 16000 };
+	int size = ARRAY_SIZE(ramp_table);
+	int i;
+
+	if (msec <= ramp_table[0])
+		return 0;
+
+	if (msec > ramp_table[size - 1])
+		return size - 1;
+
+	for (i = 1; i < size; i++) {
+		if (msec == ramp_table[i])
+			return i;
+
+		/* Find an approximate index by looking up the table */

Similarly here. So you should have an array of the possible msec
values and iterate through it to look for the nearest one.

Driver uses 'ramp_table[]' for this purpose. Could you describe it in more details?

+
+static int lm3633_led_brightness_set(struct led_classdev *cdev,
+				     enum led_brightness brightness)
+{
+	struct ti_lmu_led *lmu_led = container_of(cdev, struct ti_lmu_led,
+						  cdev);
+	struct ti_lmu_led_chip *chip = lmu_led->chip;
+	struct regmap *regmap = chip->lmu->regmap;
+	u8 reg = LM3633_REG_BRT_LVLED_BASE + lmu_led->bank_id;
+	int ret;
+
+	mutex_lock(&chip->lock);
+
+	ret = regmap_write(regmap, reg, brightness);
+	if (ret) {
+		mutex_unlock(&chip->lock);
+		return ret;
+	}
+
+	if (brightness == 0)
+		lm3633_led_disable_bank(lmu_led);

This should be checked at first, as I suppose that disabling
a bank suffices to turn the LED off.

Well, it's not a big deal when turn off the LED. However, our silicon designer recommended brightness update should be done prior to enabling LED bank. Let me share some block diagram.

[I2C logic] - [brightness control] - [control bank] - [output channel]

If control bank is enabled first, then previous brightness value in register can be used instead of new updated brightness. So brightness update should be done first.

+static u8 lm3633_led_scale_max_brightness(struct ti_lmu_led *lmu_led, u32 imax)
+{
+	u8 max_current = lm3633_led_convert_current_to_index(imax);
+	const u8 max_brightness_table[] = {
+		[LMU_IMAX_5mA]  = 191,
+		[LMU_IMAX_6mA]  = 197,
+		[LMU_IMAX_7mA]  = 203,
+		[LMU_IMAX_8mA]  = 208,
+		[LMU_IMAX_9mA]  = 212,
+		[LMU_IMAX_10mA] = 216,
+		[LMU_IMAX_11mA] = 219,
+		[LMU_IMAX_12mA] = 222,
+		[LMU_IMAX_13mA] = 225,
+		[LMU_IMAX_14mA] = 228,
+		[LMU_IMAX_15mA] = 230,
+		[LMU_IMAX_16mA] = 233,
+		[LMU_IMAX_17mA] = 235,
+		[LMU_IMAX_18mA] = 237,
+		[LMU_IMAX_19mA] = 239,
+		[LMU_IMAX_20mA] = 241,
+		[LMU_IMAX_21mA] = 242,
+		[LMU_IMAX_22mA] = 244,
+		[LMU_IMAX_23mA] = 246,
+		[LMU_IMAX_24mA] = 247,
+		[LMU_IMAX_25mA] = 249,
+		[LMU_IMAX_26mA] = 250,
+		[LMU_IMAX_27mA] = 251,
+		[LMU_IMAX_28mA] = 253,
+		[LMU_IMAX_29mA] = 254,
+		[LMU_IMAX_30mA] = 255,
+	};

After analyzing the subject one more time I think that we need to
change the approach regarding max brightness issue.

At first - we shouldn't fix max current to max possible register value.
Instead we should take led-max-microamp property and write its value
to the [0x22 + bank offset] registers.

With this approach whole 0-255 range of brightness levels will be
valid for the driver.

In effect all LMU_IMAX* enums seem to be not needed.

Good to hear that, it's the most simplest work for the device ;)
Let me update the driver. Thanks!

Best regards,
Milo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[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