Re: [PATCH v2 3/3] leds: rt5033: Add RT5033 Flash led device driver
From: Jacek Anaszewski
Date: Tue Oct 13 2015 - 04:53:39 EST
Hi Ingi,
On 10/13/2015 04:58 AM, Ingi Kim wrote:
Hi Jacek,
Thanks for your kind comments
I also append reply below
On 2015ë 10ì 13ì 00:10, Jacek Anaszewski wrote:
Hi Ingi,
Thanks for the update. Few comments below.
On 10/12/2015 03:12 PM, Ingi Kim wrote:
This patch adds device driver of Richtek RT5033 PMIC.
The driver supports a current regulated output to drive
white LEDs for camera flash.
Signed-off-by: Ingi Kim <ingi2.kim@xxxxxxxxxxx>
---
drivers/leds/Kconfig | 8 ++
drivers/leds/Makefile | 1 +
drivers/leds/leds-rt5033.c | 223 +++++++++++++++++++++++++++++++++++++
include/linux/mfd/rt5033-private.h | 49 ++++++++
include/linux/mfd/rt5033.h | 22 +++-
5 files changed, 301 insertions(+), 2 deletions(-)
create mode 100644 drivers/leds/leds-rt5033.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 42990f2..29613e3 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -345,6 +345,14 @@ config LEDS_PCA963X
LED driver chip accessed via the I2C bus. Supported
devices include PCA9633 and PCA9634
+config LEDS_RT5033
+ tristate "LED support for RT5033 PMIC"
+ depends on LEDS_CLASS_FLASH && OF
+ depends on MFD_RT5033
+ help
+ This option enables support for on-chip LED driver on
+ RT5033 PMIC.
+
config LEDS_WM831X_STATUS
tristate "LED support for status LEDs on WM831x PMICs"
depends on LEDS_CLASS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index b503f92..bcc4d93 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_LEDS_COBALT_QUBE) += leds-cobalt-qube.o
obj-$(CONFIG_LEDS_COBALT_RAQ) += leds-cobalt-raq.o
obj-$(CONFIG_LEDS_SUNFIRE) += leds-sunfire.o
obj-$(CONFIG_LEDS_PCA9532) += leds-pca9532.o
+obj-$(CONFIG_LEDS_RT5033) += leds-rt5033.o
obj-$(CONFIG_LEDS_GPIO_REGISTER) += leds-gpio-register.o
obj-$(CONFIG_LEDS_GPIO) += leds-gpio.o
obj-$(CONFIG_LEDS_LP3944) += leds-lp3944.o
diff --git a/drivers/leds/leds-rt5033.c b/drivers/leds/leds-rt5033.c
new file mode 100644
index 0000000..b470c94
--- /dev/null
+++ b/drivers/leds/leds-rt5033.c
@@ -0,0 +1,223 @@
+/*
+ * led driver for RT5033
+ *
+ * Copyright (C) 2015 Samsung Electronics, Co., Ltd.
+ * Ingi Kim <ingi2.kim@xxxxxxxxxxx>
+ *
+ * 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/mfd/rt5033.h>
+#include <linux/mfd/rt5033-private.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#define RT5033_LED_FLASH_TIMEOUT_MIN 64000
+#define RT5033_LED_FLASH_TIMEOUT_STEPS 32000
+#define RT5033_LED_TORCH_CURRENT_LEVEL_MAX 16
+
+/* Macro for getting offset of flash timeout */
+#define GET_TIMEOUT_OFFSET(tm, step) ((tm) / (step) - 2)
+
+static struct rt5033_led *flcdev_to_led(
+ struct led_classdev_flash *fled_cdev)
+{
+ return container_of(fled_cdev, struct rt5033_led, fled_cdev);
+}
+
+static int rt5033_led_brightness_set(struct led_classdev *led_cdev,
+ enum led_brightness brightness)
+{
+ struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(led_cdev);
+ struct rt5033_led *led = flcdev_to_led(fled_cdev);
I assume that you don't use mutex here deliberately?
Actually I'm not sure why flash driver uses mutex.
Which flash driver do you have on mind?
In consideration of blocking competition, however, I'd be better to use it
[...]
+
+static int rt5033_led_flash_strobe_set(struct led_classdev_flash *fled_cdev,
+ bool state)
+{
+ struct rt5033_led *led = flcdev_to_led(fled_cdev);
+ u32 flash_tm_reg;
I think that you need a mutex here and in rt5033_led_flash_strobe_set. Otherwise following is possible:
Process 1:
rt5033_led_flash_strobe_set(led_cdev, 1);
regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
RT5033_FLED_FUNC1_MASK, RT5033_FLED_RESET);
fled_cdev->led_cdev.brightness = LED_OFF;
Process 2:
led_set_brightness(led_cdev, 1);
fled_cdev->led_cdev.brightness = 1;
rt5033_led_brightness_set(led_cdev, LED_FULL);
regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
RT5033_FLED_FUNC1_MASK, RT5033_FLED_PINCTRL);
regmap_update_bits(led->regmap, RT5033_REG_FLED_CTRL1,
RT5033_FLED_CTRL1_MASK,
(brightness - 1) << 4);
regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION2,
RT5033_FLED_CTRL2_MASK, RT5033_FLED_ENFLED);
Process 1:
regmap_update_bits(led->regmap, RT5033_REG_FLED_STROBE_CTRL2,
RT5033_FLED_STRBCTRL2_MASK, flash_tm_reg);
regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1,
RT5033_FLED_FUNC1_MASK, RT5033_FLED_STRB_SEL
| RT5033_FLED_PINCTRL);
regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION2,
RT5033_FLED_FUNC2_MASK, RT5033_FLED_ENFLED
In a result LED class device will report brightness value 1,
whereas it would be inconsistent with hardware state, since
flash strobe turns torch mode off.
Thanks for your explanation in detail,
I was worried about cases of using shared resource for flash led.
because I thought flash led would be called just from camera or directly itself.
I should consider using mutex
You don't provide support for v4l2-flash-led-class and you don't
implement external_strobe_set op. Only if the op is implemented a v4l2
device can switch the strobe source from software to external,
which allows strobing the flash directly by camera sensor or ISP,
but without software interaction.
--
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/