diff mbox series

[RFC,v2,1/9] leds: add TI LMU backlight driver

Message ID 20180928182954.25446-2-dmurphy@ti.com (mailing list archive)
State New, archived
Headers show
Series TI LMU and Dedicated Drivers | expand

Commit Message

Dan Murphy Sept. 28, 2018, 6:29 p.m. UTC
From: Pavel Machek <pavel@ucw.cz>

This adds backlight support for the following TI LMU
chips: LM3532, LM3631, LM3632, LM3633, LM3695 and LM3697.

It controls LEDs on Droid 4
smartphone, including keyboard and screen backlights.

Signed-off-by: Milo Kim <milo.kim@ti.com>
[add LED subsystem support for keyboard backlight and rework DT
binding according to Rob Herrings feedback]
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
[remove backlight subsystem support for now]
Signed-off-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/leds/Kconfig             |   8 ++
 drivers/leds/Makefile            |   1 +
 drivers/leds/ti-lmu-led-common.c | 138 +++++++++++++++++++++++++++++++
 drivers/leds/ti-lmu-led-common.h |  54 ++++++++++++
 4 files changed, 201 insertions(+)
 create mode 100644 drivers/leds/ti-lmu-led-common.c
 create mode 100644 drivers/leds/ti-lmu-led-common.h

Comments

Pavel Machek Oct. 2, 2018, 7:56 a.m. UTC | #1
On Fri 2018-09-28 13:29:46, Dan Murphy wrote:
> From: Pavel Machek <pavel@ucw.cz>
> 
> This adds backlight support for the following TI LMU
> chips: LM3532, LM3631, LM3632, LM3633, LM3695 and LM3697.
> 
> It controls LEDs on Droid 4
> smartphone, including keyboard and screen backlights.
> 
> Signed-off-by: Milo Kim <milo.kim@ti.com>
> [add LED subsystem support for keyboard backlight and rework DT
> binding according to Rob Herrings feedback]
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> [remove backlight subsystem support for now]
> Signed-off-by: Pavel Machek <pavel@ucw.cz>

So... this driver adds support for LM3532, LM3631, LM3632, LM3633,
LM3695 and LM3697 (or it did when I signed it off).

The rest of the series does not really bring any advantages (you claim
it may add advantages in future). It takes code out of common driver
and duplicates it.

Could we take this patch, get the basic support for LM3532, LM3631,
LM3632, LM3633, LM3695 and LM3697, and then split out the drivers when
we actually gain some advantage doing so (and also when the costs are
clear)?

Thanks,

								Pavel

>  drivers/leds/Kconfig             |   8 ++
>  drivers/leds/Makefile            |   1 +
>  drivers/leds/ti-lmu-led-common.c | 138 +++++++++++++++++++++++++++++++
>  drivers/leds/ti-lmu-led-common.h |  54 ++++++++++++
>  4 files changed, 201 insertions(+)
>  create mode 100644 drivers/leds/ti-lmu-led-common.c
>  create mode 100644 drivers/leds/ti-lmu-led-common.h
Dan Murphy Oct. 2, 2018, 12:32 p.m. UTC | #2
Pavel

On 10/02/2018 02:56 AM, Pavel Machek wrote:
> On Fri 2018-09-28 13:29:46, Dan Murphy wrote:
>> From: Pavel Machek <pavel@ucw.cz>
>>
>> This adds backlight support for the following TI LMU
>> chips: LM3532, LM3631, LM3632, LM3633, LM3695 and LM3697.
>>
>> It controls LEDs on Droid 4
>> smartphone, including keyboard and screen backlights.
>>
>> Signed-off-by: Milo Kim <milo.kim@ti.com>
>> [add LED subsystem support for keyboard backlight and rework DT
>> binding according to Rob Herrings feedback]
>> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
>> [remove backlight subsystem support for now]
>> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> So... this driver adds support for LM3532, LM3631, LM3632, LM3633,
> LM3695 and LM3697 (or it did when I signed it off).

Yes I have to pull these out of the patch.

> 
> The rest of the series does not really bring any advantages (you claim
> it may add advantages in future). It takes code out of common driver
> and duplicates it.


I disagree.  Honestly using that ideallogy all LED drivers should use the common code
as it is a wrapper around regmap and a few if statements.

The 3632 adds the proper LED flash class support coupled with proper backlight support.
The 3633 adds the proper support for LV and HV LED support.

Duplicate code that I could find is put in the common file.
This patch set adds the LED devices as all other LED devices are added in the LED directory.

> 
> Could we take this patch, get the basic support for LM3532, LM3631,
> LM3632, LM3633, LM3695 and LM3697, and then split out the drivers when
> we actually gain some advantage doing so (and also when the costs are
> clear)?

We have debated this over and over and now we have 3 different implementations
available we need to collude on which one we want to support.

Jacek I defer to you and Pavel since you are both LED maintainers.

I can support the dedicated LED drivers but I cannot support the TI LMU only implementation.

Dan

> 
> Thanks,
> 
> 								Pavel
> 
>>  drivers/leds/Kconfig             |   8 ++
>>  drivers/leds/Makefile            |   1 +
>>  drivers/leds/ti-lmu-led-common.c | 138 +++++++++++++++++++++++++++++++
>>  drivers/leds/ti-lmu-led-common.h |  54 ++++++++++++
>>  4 files changed, 201 insertions(+)
>>  create mode 100644 drivers/leds/ti-lmu-led-common.c
>>  create mode 100644 drivers/leds/ti-lmu-led-common.h
>
Jacek Anaszewski Oct. 2, 2018, 6:52 p.m. UTC | #3
On 10/02/2018 02:32 PM, Dan Murphy wrote:
> Pavel
> 
> On 10/02/2018 02:56 AM, Pavel Machek wrote:
>> On Fri 2018-09-28 13:29:46, Dan Murphy wrote:
>>> From: Pavel Machek <pavel@ucw.cz>
>>>
>>> This adds backlight support for the following TI LMU
>>> chips: LM3532, LM3631, LM3632, LM3633, LM3695 and LM3697.
>>>
>>> It controls LEDs on Droid 4
>>> smartphone, including keyboard and screen backlights.
>>>
>>> Signed-off-by: Milo Kim <milo.kim@ti.com>
>>> [add LED subsystem support for keyboard backlight and rework DT
>>> binding according to Rob Herrings feedback]
>>> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
>>> [remove backlight subsystem support for now]
>>> Signed-off-by: Pavel Machek <pavel@ucw.cz>
>>
>> So... this driver adds support for LM3532, LM3631, LM3632, LM3633,
>> LM3695 and LM3697 (or it did when I signed it off).
> 
> Yes I have to pull these out of the patch.
> 
>>
>> The rest of the series does not really bring any advantages (you claim
>> it may add advantages in future). It takes code out of common driver
>> and duplicates it.
> 
> 
> I disagree.  Honestly using that ideallogy all LED drivers should use the common code
> as it is a wrapper around regmap and a few if statements.
> 
> The 3632 adds the proper LED flash class support coupled with proper backlight support.
> The 3633 adds the proper support for LV and HV LED support.
> 
> Duplicate code that I could find is put in the common file.
> This patch set adds the LED devices as all other LED devices are added in the LED directory.
> 
>>
>> Could we take this patch, get the basic support for LM3532, LM3631,
>> LM3632, LM3633, LM3695 and LM3697, and then split out the drivers when
>> we actually gain some advantage doing so (and also when the costs are
>> clear)?
> 
> We have debated this over and over and now we have 3 different implementations
> available we need to collude on which one we want to support.
> 
> Jacek I defer to you and Pavel since you are both LED maintainers.
> 
> I can support the dedicated LED drivers but I cannot support the TI LMU only implementation.

I uphold my previous opinion - please go ahead with moving the support
for non-MFD devices from MFD subsystem to the LED subsystem. And yes -
along with the bindings. This is semantically correct, and yet we don't
have mainline users.

Pavel - you will have to engage more people for your crusade to prevail.
For now, to speed up the things, I am forced to ignore your NAK.
So NAK to your NAK. Sorry.
Pavel Machek Oct. 2, 2018, 10:07 p.m. UTC | #4
Hi!

> > We have debated this over and over and now we have 3 different implementations
> > available we need to collude on which one we want to support.
> > 
> > Jacek I defer to you and Pavel since you are both LED maintainers.
> > 
> > I can support the dedicated LED drivers but I cannot support the TI LMU only implementation.
> 
> I uphold my previous opinion - please go ahead with moving the support
> for non-MFD devices from MFD subsystem to the LED subsystem. And yes -
> along with the bindings. This is semantically correct, and yet we don't
> have mainline users.
> 
> Pavel - you will have to engage more people for your crusade to prevail.
> For now, to speed up the things, I am forced to ignore your NAK.
> So NAK to your NAK. Sorry.

No need to be sorry :-).

Lets ignore the code for now, as code can be changed easily.

Bindings are not something you or I maintain, so we don't have final
word there, and I have feeling this is not going to go past device
tree maintainers. [AFAICT: you can move binding in Documentation/ to
different place, that's not a problem; but creating a new binding when
old one exists is.]

If you and Dan feel that is okay, you are welcome to try to get the
patches past Rob, just please retain the NAK so that he remembers the
discussion, and so that it is clear that I don't like the changes.

I believe smart thing to do is to try to do that before working
further on the code, but of course, its all up to you :-).

Friends,
									Pavel
Dan Murphy Oct. 3, 2018, noon UTC | #5
Hello

On 10/02/2018 05:07 PM, Pavel Machek wrote:
> Hi!
> 
>>> We have debated this over and over and now we have 3 different implementations
>>> available we need to collude on which one we want to support.
>>>
>>> Jacek I defer to you and Pavel since you are both LED maintainers.
>>>
>>> I can support the dedicated LED drivers but I cannot support the TI LMU only implementation.
>>
>> I uphold my previous opinion - please go ahead with moving the support
>> for non-MFD devices from MFD subsystem to the LED subsystem. And yes -
>> along with the bindings. This is semantically correct, and yet we don't
>> have mainline users.
>>
>> Pavel - you will have to engage more people for your crusade to prevail.
>> For now, to speed up the things, I am forced to ignore your NAK.
>> So NAK to your NAK. Sorry.
> 
> No need to be sorry :-).
> 
> Lets ignore the code for now, as code can be changed easily.
> 
> Bindings are not something you or I maintain, so we don't have final
> word there, and I have feeling this is not going to go past device
> tree maintainers. [AFAICT: you can move binding in Documentation/ to
> different place, that's not a problem; but creating a new binding when
> old one exists is.]
> 
> If you and Dan feel that is okay, you are welcome to try to get the
> patches past Rob, just please retain the NAK so that he remembers the
> discussion, and so that it is clear that I don't like the changes.
> 
> I believe smart thing to do is to try to do that before working
> further on the code, but of course, its all up to you :-).

I was looking for the review for the ti-lmu.txt binding on patchworks to see what
the comments were on the review or any explanation from reviewers to
why this implementation was done in the MFD.

Maybe there is some historical context that needs to be learned from here from
1.5 years ago.

I cannot seem to find any review posted in the patchworks archive.
I see reviews posted for the ti-lmu-backlight binding but none from
the ti-lmu.txt base binding.

Does anyone have a reference to the review?

If there is no review then I am wondering how this binding was accepted.
If there is no review and no current users then I would think that binding
 modification should be allowed.  But thats just my opinion.

Dan


> 
> Friends,
> 									Pavel
>
Dan Murphy Oct. 3, 2018, 12:10 p.m. UTC | #6
Hello

On 10/03/2018 07:00 AM, Dan Murphy wrote:
> Hello
> 
> On 10/02/2018 05:07 PM, Pavel Machek wrote:
>> Hi!
>>
>>>> We have debated this over and over and now we have 3 different implementations
>>>> available we need to collude on which one we want to support.
>>>>
>>>> Jacek I defer to you and Pavel since you are both LED maintainers.
>>>>
>>>> I can support the dedicated LED drivers but I cannot support the TI LMU only implementation.
>>>
>>> I uphold my previous opinion - please go ahead with moving the support
>>> for non-MFD devices from MFD subsystem to the LED subsystem. And yes -
>>> along with the bindings. This is semantically correct, and yet we don't
>>> have mainline users.
>>>
>>> Pavel - you will have to engage more people for your crusade to prevail.
>>> For now, to speed up the things, I am forced to ignore your NAK.
>>> So NAK to your NAK. Sorry.
>>
>> No need to be sorry :-).
>>
>> Lets ignore the code for now, as code can be changed easily.
>>
>> Bindings are not something you or I maintain, so we don't have final
>> word there, and I have feeling this is not going to go past device
>> tree maintainers. [AFAICT: you can move binding in Documentation/ to
>> different place, that's not a problem; but creating a new binding when
>> old one exists is.]
>>
>> If you and Dan feel that is okay, you are welcome to try to get the
>> patches past Rob, just please retain the NAK so that he remembers the
>> discussion, and so that it is clear that I don't like the changes.
>>
>> I believe smart thing to do is to try to do that before working
>> further on the code, but of course, its all up to you :-).
> 
> I was looking for the review for the ti-lmu.txt binding on patchworks to see what
> the comments were on the review or any explanation from reviewers to
> why this implementation was done in the MFD.
> 
> Maybe there is some historical context that needs to be learned from here from
> 1.5 years ago.
> 
> I cannot seem to find any review posted in the patchworks archive.
> I see reviews posted for the ti-lmu-backlight binding but none from
> the ti-lmu.txt base binding.
> 
> Does anyone have a reference to the review?

I found the review or at least the reference for the ti-lmu.txt binding.

https://lore.kernel.org/patchwork/patch/764180/

Does not appear that the binding was sent to the device tree mail list.
(Maybe that email list did not exist in Feb 2017).
Especially with the amount of change that is being submitted in the
newer patchsets.

Dan

> 
> If there is no review then I am wondering how this binding was accepted.
> If there is no review and no current users then I would think that binding
>  modification should be allowed.  But thats just my opinion.
> 
> Dan
> 
> 
>>
>> Friends,
>> 									Pavel
>>
> 
>
diff mbox series

Patch

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 44097a3e0fcc..dc717b30d9d3 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -756,6 +756,14 @@  config LEDS_NIC78BX
 	  To compile this driver as a module, choose M here: the module
 	  will be called leds-nic78bx.
 
+config LEDS_TI_LMU_COMMON
+	tristate "LED driver for TI LMU"
+	depends on REGMAP
+	help
+          Say Y to enable the LED driver for TI LMU devices.
+          This supports common features between the TI LM3532, LM3631, LM3632,
+	  LM3633, LM3695 and LM3697.
+
 comment "LED Triggers"
 source "drivers/leds/trigger/Kconfig"
 
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 420b5d2cfa62..e09bb27bc7ea 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -78,6 +78,7 @@  obj-$(CONFIG_LEDS_MT6323)		+= leds-mt6323.o
 obj-$(CONFIG_LEDS_LM3692X)		+= leds-lm3692x.o
 obj-$(CONFIG_LEDS_SC27XX_BLTC)		+= leds-sc27xx-bltc.o
 obj-$(CONFIG_LEDS_LM3601X)		+= leds-lm3601x.o
+obj-$(CONFIG_LEDS_TI_LMU_COMMON)	+= ti-lmu-led-common.o
 
 # LED SPI Drivers
 obj-$(CONFIG_LEDS_CR0014114)		+= leds-cr0014114.o
diff --git a/drivers/leds/ti-lmu-led-common.c b/drivers/leds/ti-lmu-led-common.c
new file mode 100644
index 000000000000..2d4f22f480d1
--- /dev/null
+++ b/drivers/leds/ti-lmu-led-common.c
@@ -0,0 +1,138 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2015 Texas Instruments
+ * Copyright 2018 Sebastian Reichel
+ * Copyright 2018 Pavel Machek <pavel@ucw.cz>
+ *
+ * TI LMU Led driver, based on previous work from
+ * Milo Kim <milo.kim@ti.com>
+ */
+
+#include <linux/bitops.h>
+#include <linux/err.h>
+#include <linux/notifier.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#include "ti-lmu-led-common.h"
+
+const static int ramp_table[16] = { 2, 262, 524, 1049, 2090, 4194, 8389,
+				16780, 33550, 41940, 50330, 58720,
+				67110, 83880, 100660, 117440};
+
+static int ti_lmu_common_update_brightness_register(struct ti_lmu_bank *lmu_bank,
+						       int brightness)
+{
+	struct regmap *regmap = lmu_bank->regmap;
+	u8 reg, val;
+	int ret;
+
+	/*
+	 * Brightness register update
+	 *
+	 * 11 bit dimming: update LSB bits and write MSB byte.
+	 *		   MSB brightness should be shifted.
+	 *  8 bit dimming: write MSB byte.
+	 */
+	if (lmu_bank->max_brightness == MAX_BRIGHTNESS_11BIT) {
+		reg = lmu_bank->lsb_brightness_reg;
+		ret = regmap_update_bits(regmap, reg,
+					 LMU_11BIT_LSB_MASK,
+					 brightness);
+		if (ret)
+			return ret;
+
+		val = brightness >> LMU_11BIT_MSB_SHIFT;
+	} else {
+		val = brightness;
+	}
+
+	reg = lmu_bank->msb_brightness_reg;
+
+	return regmap_write(regmap, reg, val);
+}
+
+int ti_lmu_common_set_brightness(struct ti_lmu_bank *lmu_bank,
+				    int brightness)
+{
+	lmu_bank->current_brightness = brightness;
+
+	return ti_lmu_common_update_brightness_register(lmu_bank, brightness);
+}
+EXPORT_SYMBOL(ti_lmu_common_set_brightness);
+
+static int ti_lmu_common_convert_ramp_to_index(unsigned int msec)
+{
+	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 */
+		if (msec > ramp_table[i - 1] && msec < ramp_table[i]) {
+			if (msec - ramp_table[i - 1] < ramp_table[i] - msec)
+				return i - 1;
+			else
+				return i;
+		}
+	}
+
+	return -EINVAL;
+}
+
+int ti_lmu_common_set_ramp(struct ti_lmu_bank *lmu_bank)
+{
+	struct regmap *regmap = lmu_bank->regmap;
+	u8 ramp, ramp_up, ramp_down;
+
+	if (lmu_bank->ramp_up_msec == 0 && lmu_bank->ramp_down_msec == 0) {
+		ramp_up = 0;
+		ramp_down = 0;
+	} else {
+		ramp_up = ti_lmu_common_convert_ramp_to_index(lmu_bank->ramp_up_msec);
+		ramp_down = ti_lmu_common_convert_ramp_to_index(lmu_bank->ramp_down_msec);
+	}
+
+	if (ramp_up < 0 || ramp_down < 0)
+		return -EINVAL;
+
+	ramp = (ramp_up << 4) | ramp_down;
+
+	return regmap_write(regmap, lmu_bank->runtime_ramp_reg, ramp);
+
+}
+EXPORT_SYMBOL(ti_lmu_common_set_ramp);
+
+int ti_lmu_common_get_ramp_params(struct device *dev,
+				  struct fwnode_handle *child,
+				  struct ti_lmu_bank *lmu_data)
+{
+	int ret;
+
+	ret = fwnode_property_read_u32(child, "ramp-up-ms",
+				 &lmu_data->ramp_up_msec);
+	if (ret)
+		dev_warn(dev, "ramp-up-ms property missing\n");
+
+
+	ret = fwnode_property_read_u32(child, "ramp-down-ms",
+				 &lmu_data->ramp_down_msec);
+	if (ret)
+		dev_warn(dev, "ramp-down-ms property missing\n");
+
+	return 0;
+}
+EXPORT_SYMBOL(ti_lmu_common_get_ramp_params);
+
+MODULE_DESCRIPTION("TI LMU LED Driver");
+MODULE_AUTHOR("Sebastian Reichel");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:ti-lmu-led");
diff --git a/drivers/leds/ti-lmu-led-common.h b/drivers/leds/ti-lmu-led-common.h
new file mode 100644
index 000000000000..511768dd54b6
--- /dev/null
+++ b/drivers/leds/ti-lmu-led-common.h
@@ -0,0 +1,54 @@ 
+// SPDX-License-Identifier: GPL-2.0
+// TI LMU Common Core
+// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <uapi/linux/uleds.h>
+
+#define LMU_DUAL_CHANNEL_USED	(BIT(0) | BIT(1))
+#define LMU_11BIT_LSB_MASK	(BIT(0) | BIT(1) | BIT(2))
+#define LMU_11BIT_MSB_SHIFT	3
+
+#define MAX_BRIGHTNESS_8BIT	255
+#define MAX_BRIGHTNESS_11BIT	2047
+
+#define NUM_DUAL_CHANNEL	2
+
+struct ti_lmu_bank {
+	struct regmap *regmap;
+
+	int bank_id;
+	int fault_monitor_used;
+
+	u8 enable_reg;
+	unsigned long enable_usec;
+
+	int current_brightness;
+	u32 default_brightness;
+	int max_brightness;
+
+	u8 lsb_brightness_reg;
+	u8 msb_brightness_reg;
+
+	u8 runtime_ramp_reg;
+	u32 ramp_up_msec;
+	u32 ramp_down_msec;
+};
+
+
+int ti_lmu_common_set_brightness(struct ti_lmu_bank *lmu_bank,
+				    int brightness);
+
+int ti_lmu_common_set_ramp(struct ti_lmu_bank *lmu_bank);
+
+int ti_lmu_common_get_ramp_params(struct device *dev,
+				  struct fwnode_handle *child,
+				  struct ti_lmu_bank *lmu_data);