* [PATCH v2 0/3] Add RT5033 Flash LED driver @ 2015-10-12 13:12 Ingi Kim 2015-10-12 13:12 ` [PATCH v2 1/3] leds: rt5033: Add DT binding for RT5033 Ingi Kim ` (2 more replies) 0 siblings, 3 replies; 11+ messages in thread From: Ingi Kim @ 2015-10-12 13:12 UTC (permalink / raw) To: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak, sameo, lee.jones, rpurdie, j.anaszewski Cc: inki.dae, sw0312.kim, beomho.seo, devicetree, linux-kernel, linux-leds, Ingi Kim This patch supports flash led of RT5033 PMIC. Changes since v2: - Split MFC code from rt5033 flash led patch - Fix typo error - Change naming of mfd register back again - Fix compile error Ingi Kim (3): leds: rt5033: Add DT binding for RT5033 mfd: rt5033: Add RT5033 Flash led sub device leds: rt5033: Add RT5033 Flash led device driver .../devicetree/bindings/leds/leds-rt5033.txt | 38 ++++ drivers/leds/Kconfig | 8 + drivers/leds/Makefile | 1 + drivers/leds/leds-rt5033.c | 223 +++++++++++++++++++++ drivers/mfd/rt5033.c | 6 +- include/linux/mfd/rt5033-private.h | 49 +++++ include/linux/mfd/rt5033.h | 22 +- 7 files changed, 344 insertions(+), 3 deletions(-) create mode 100644 Documentation/devicetree/bindings/leds/leds-rt5033.txt create mode 100644 drivers/leds/leds-rt5033.c -- 2.0.5 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/3] leds: rt5033: Add DT binding for RT5033 2015-10-12 13:12 [PATCH v2 0/3] Add RT5033 Flash LED driver Ingi Kim @ 2015-10-12 13:12 ` Ingi Kim 2015-10-12 13:12 ` [PATCH v2 2/3] mfd: rt5033: Add RT5033 Flash led sub device Ingi Kim 2015-10-12 13:12 ` [PATCH v2 3/3] leds: rt5033: Add RT5033 Flash led device driver Ingi Kim 2 siblings, 0 replies; 11+ messages in thread From: Ingi Kim @ 2015-10-12 13:12 UTC (permalink / raw) To: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak, sameo, lee.jones, rpurdie, j.anaszewski Cc: inki.dae, sw0312.kim, beomho.seo, devicetree, linux-kernel, linux-leds, Ingi Kim This patch adds the device tree bindings for RT5033 flash LEDs. Signed-off-by: Ingi Kim <ingi2.kim@samsung.com> Acked-by: Rob Herring <robh@kernel.org> --- .../devicetree/bindings/leds/leds-rt5033.txt | 38 ++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-rt5033.txt diff --git a/Documentation/devicetree/bindings/leds/leds-rt5033.txt b/Documentation/devicetree/bindings/leds/leds-rt5033.txt new file mode 100644 index 0000000..2ef7bdc --- /dev/null +++ b/Documentation/devicetree/bindings/leds/leds-rt5033.txt @@ -0,0 +1,38 @@ +* Richtek Technology Corporation - RT5033 Flash LED Driver + +The RT5033 Flash LED Circuit is designed for one or two LEDs driving +for torch and strobe applications, it provides an I2C software command +to trigger the torch and strobe operation. + +Required properties: +- compatible : Must be "richtek,rt5033-led". + +A discrete LED element connected to the device must be represented by a child +node - see Documentation/devicetree/bindings/leds/common.txt. + +Required properties of the LED child node: + See Documentation/devicetree/bindings/leds/common.txt +- led-max-microamp : Minimum Threshold for Timer protection + is defined internally (Maximum 200mA). +- flash-max-microamp : Flash LED maximum current +- flash-max-timeout-us : Flash LED maximum timeout + +Optional properties of the LED child node: +- label : see Documentation/devicetree/bindings/leds/common.txt + +Example: + +rt5033 { + compatible = "richtek,rt5033"; + + led { + compatible = "richtek,rt5033-led"; + + flash-led { + label = "rt5033-flash"; + led-max-microamp = <200000>; + flash-max-microamp = <800000>; + flash-max-timeout-us = <1216000>; + }; + }; +} -- 2.0.5 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/3] mfd: rt5033: Add RT5033 Flash led sub device 2015-10-12 13:12 [PATCH v2 0/3] Add RT5033 Flash LED driver Ingi Kim 2015-10-12 13:12 ` [PATCH v2 1/3] leds: rt5033: Add DT binding for RT5033 Ingi Kim @ 2015-10-12 13:12 ` Ingi Kim [not found] ` <1444655578-19806-3-git-send-email-ingi2.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> 2015-10-12 13:12 ` [PATCH v2 3/3] leds: rt5033: Add RT5033 Flash led device driver Ingi Kim 2 siblings, 1 reply; 11+ messages in thread From: Ingi Kim @ 2015-10-12 13:12 UTC (permalink / raw) To: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak, sameo, lee.jones, rpurdie, j.anaszewski Cc: inki.dae, sw0312.kim, beomho.seo, devicetree, linux-kernel, linux-leds, Ingi Kim This patch adds rt5033-led sub device to support it. Signed-off-by: Ingi Kim <ingi2.kim@samsung.com> --- drivers/mfd/rt5033.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/mfd/rt5033.c b/drivers/mfd/rt5033.c index d60f916..b7f374b 100644 --- a/drivers/mfd/rt5033.c +++ b/drivers/mfd/rt5033.c @@ -40,13 +40,17 @@ static const struct regmap_irq_chip rt5033_irq_chip = { }; static const struct mfd_cell rt5033_devs[] = { - { .name = "rt5033-regulator", }, { + .name = "rt5033-regulator", + }, { .name = "rt5033-charger", .of_compatible = "richtek,rt5033-charger", }, { .name = "rt5033-battery", .of_compatible = "richtek,rt5033-battery", + }, { + .name = "rt5033-led", + .of_compatible = "richtek,rt5033-led", }, }; -- 2.0.5 ^ permalink raw reply related [flat|nested] 11+ messages in thread
[parent not found: <1444655578-19806-3-git-send-email-ingi2.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>]
* Re: [PATCH v2 2/3] mfd: rt5033: Add RT5033 Flash led sub device 2015-10-12 13:12 ` [PATCH v2 2/3] mfd: rt5033: Add RT5033 Flash led sub device Ingi Kim @ 2015-10-13 7:06 ` Lee Jones 0 siblings, 0 replies; 11+ messages in thread From: Lee Jones @ 2015-10-13 7:06 UTC (permalink / raw) To: Ingi Kim Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, pawel.moll-5wv7dgnIgG8, mark.rutland-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg, galak-sgV2jX0FEOL9JmXXK+q4OQ, sameo-VuQAYsv1563Yd54FQh9/CA, rpurdie-Fm38FmjxZ/leoWH0uzbU5w, j.anaszewski-Sze3O3UU22JBDgjK7y7TUQ, inki.dae-Sze3O3UU22JBDgjK7y7TUQ, sw0312.kim-Sze3O3UU22JBDgjK7y7TUQ, beomho.seo-Sze3O3UU22JBDgjK7y7TUQ, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-leds-u79uwXL29TY76Z2rM5mHXA On Mon, 12 Oct 2015, Ingi Kim wrote: > This patch adds rt5033-led sub device to support it. > > Signed-off-by: Ingi Kim <ingi2.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> > --- > drivers/mfd/rt5033.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/mfd/rt5033.c b/drivers/mfd/rt5033.c > index d60f916..b7f374b 100644 > --- a/drivers/mfd/rt5033.c > +++ b/drivers/mfd/rt5033.c > @@ -40,13 +40,17 @@ static const struct regmap_irq_chip rt5033_irq_chip = { > }; > > static const struct mfd_cell rt5033_devs[] = { > - { .name = "rt5033-regulator", }, > { > + .name = "rt5033-regulator", > + }, { There's no need for this change. Please remove it. > .name = "rt5033-charger", > .of_compatible = "richtek,rt5033-charger", > }, { > .name = "rt5033-battery", > .of_compatible = "richtek,rt5033-battery", > + }, { > + .name = "rt5033-led", > + .of_compatible = "richtek,rt5033-led", For when you resubmit with the hunk above removed: Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> > }, > }; > -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] mfd: rt5033: Add RT5033 Flash led sub device @ 2015-10-13 7:06 ` Lee Jones 0 siblings, 0 replies; 11+ messages in thread From: Lee Jones @ 2015-10-13 7:06 UTC (permalink / raw) To: Ingi Kim Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak, sameo, rpurdie, j.anaszewski, inki.dae, sw0312.kim, beomho.seo, devicetree, linux-kernel, linux-leds On Mon, 12 Oct 2015, Ingi Kim wrote: > This patch adds rt5033-led sub device to support it. > > Signed-off-by: Ingi Kim <ingi2.kim@samsung.com> > --- > drivers/mfd/rt5033.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/mfd/rt5033.c b/drivers/mfd/rt5033.c > index d60f916..b7f374b 100644 > --- a/drivers/mfd/rt5033.c > +++ b/drivers/mfd/rt5033.c > @@ -40,13 +40,17 @@ static const struct regmap_irq_chip rt5033_irq_chip = { > }; > > static const struct mfd_cell rt5033_devs[] = { > - { .name = "rt5033-regulator", }, > { > + .name = "rt5033-regulator", > + }, { There's no need for this change. Please remove it. > .name = "rt5033-charger", > .of_compatible = "richtek,rt5033-charger", > }, { > .name = "rt5033-battery", > .of_compatible = "richtek,rt5033-battery", > + }, { > + .name = "rt5033-led", > + .of_compatible = "richtek,rt5033-led", For when you resubmit with the hunk above removed: Acked-by: Lee Jones <lee.jones@linaro.org> > }, > }; > -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/3] mfd: rt5033: Add RT5033 Flash led sub device 2015-10-13 7:06 ` Lee Jones (?) @ 2015-10-13 8:03 ` Ingi Kim -1 siblings, 0 replies; 11+ messages in thread From: Ingi Kim @ 2015-10-13 8:03 UTC (permalink / raw) To: Lee Jones Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak, sameo, rpurdie, j.anaszewski, inki.dae, sw0312.kim, beomho.seo, devicetree, linux-kernel, linux-leds Hi Lee Jones, Okay, I'm try to remove needless part and resend it Thanks for the review Thank you On 2015년 10월 13일 16:06, Lee Jones wrote: > On Mon, 12 Oct 2015, Ingi Kim wrote: > >> This patch adds rt5033-led sub device to support it. >> >> Signed-off-by: Ingi Kim <ingi2.kim@samsung.com> >> --- >> drivers/mfd/rt5033.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mfd/rt5033.c b/drivers/mfd/rt5033.c >> index d60f916..b7f374b 100644 >> --- a/drivers/mfd/rt5033.c >> +++ b/drivers/mfd/rt5033.c >> @@ -40,13 +40,17 @@ static const struct regmap_irq_chip rt5033_irq_chip = { >> }; >> >> static const struct mfd_cell rt5033_devs[] = { >> - { .name = "rt5033-regulator", }, >> { >> + .name = "rt5033-regulator", >> + }, { > > There's no need for this change. Please remove it. > >> .name = "rt5033-charger", >> .of_compatible = "richtek,rt5033-charger", >> }, { >> .name = "rt5033-battery", >> .of_compatible = "richtek,rt5033-battery", >> + }, { >> + .name = "rt5033-led", >> + .of_compatible = "richtek,rt5033-led", > > For when you resubmit with the hunk above removed: > > Acked-by: Lee Jones <lee.jones@linaro.org> > >> }, >> }; >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 3/3] leds: rt5033: Add RT5033 Flash led device driver 2015-10-12 13:12 [PATCH v2 0/3] Add RT5033 Flash LED driver Ingi Kim 2015-10-12 13:12 ` [PATCH v2 1/3] leds: rt5033: Add DT binding for RT5033 Ingi Kim 2015-10-12 13:12 ` [PATCH v2 2/3] mfd: rt5033: Add RT5033 Flash led sub device Ingi Kim @ 2015-10-12 13:12 ` Ingi Kim 2015-10-12 15:10 ` Jacek Anaszewski 2 siblings, 1 reply; 11+ messages in thread From: Ingi Kim @ 2015-10-12 13:12 UTC (permalink / raw) To: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak, sameo, lee.jones, rpurdie, j.anaszewski Cc: inki.dae, sw0312.kim, beomho.seo, devicetree, linux-kernel, linux-leds, Ingi Kim 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@samsung.com> --- 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@samsung.com> + * + * 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); + + if (!brightness) { + regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION2, + RT5033_FLED_CTRL2_MASK, 0x0); + } else { + 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); + } + + return 0; +} + +static void rt5033_init_flash_timeout(struct rt5033_led *led) +{ + struct led_flash_setting *setting; + + setting = &led->fled_cdev.timeout; + setting->min = RT5033_LED_FLASH_TIMEOUT_MIN; + setting->max = led->data->flash_max_timeout; + setting->step = RT5033_LED_FLASH_TIMEOUT_STEPS; + setting->val = led->data->flash_max_timeout; +} + +static int rt5033_led_parse_dt(struct rt5033_led *led, struct device *dev) +{ + struct device_node *np = dev->of_node; + struct device_node *child_node; + struct rt5033_led_config_data *data; + int ret = 0; + + if (!np) + return -ENXIO; + + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + child_node = of_get_next_available_child(np, NULL); + if (!child_node) { + dev_err(dev, "DT child node isn't found\n"); + return -EINVAL; + } + + led->fled_cdev.led_cdev.name = + of_get_property(child_node, "label", NULL) ? : child_node->name; + + ret = of_property_read_u32(child_node, "led-max-microamp", + &data->torch_max_microamp); + if (ret) { + dev_err(dev, "failed to parse led-max-microamp\n"); + return ret; + } + + ret = of_property_read_u32(child_node, "flash-max-microamp", + &data->flash_max_microamp); + if (ret) { + dev_err(dev, "failed to parse flash-max-microamp\n"); + return ret; + } + + ret = of_property_read_u32(child_node, "flash-max-timeout-us", + &data->flash_max_timeout); + if (ret) + dev_err(dev, "failed to parse flash-max-timeout-us\n"); + + of_node_put(child_node); + led->data = data; + + return ret; +} + +static int rt5033_led_flash_timeout_set(struct led_classdev_flash *fled_cdev, + u32 timeout) +{ + return 0; +} + +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; + + regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1, + RT5033_FLED_FUNC1_MASK, RT5033_FLED_RESET); + fled_cdev->led_cdev.brightness = LED_OFF; + + if (state) { + flash_tm_reg = GET_TIMEOUT_OFFSET(fled_cdev->timeout.val, + fled_cdev->timeout.step); + 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 + | RT5033_FLED_SREG_STRB); + } + + return 0; +} + +static const struct led_flash_ops flash_ops = { + .strobe_set = rt5033_led_flash_strobe_set, + .timeout_set = rt5033_led_flash_timeout_set, +}; + +static int rt5033_led_probe(struct platform_device *pdev) +{ + struct rt5033_dev *rt5033 = dev_get_drvdata(pdev->dev.parent); + struct rt5033_led *led; + struct led_classdev *led_cdev; + int ret; + + led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL); + if (!led) + return -ENOMEM; + + platform_set_drvdata(pdev, led); + led->dev = &pdev->dev; + led->regmap = rt5033->regmap; + + ret = rt5033_led_parse_dt(led, &pdev->dev); + if (ret) + return ret; + + rt5033_init_flash_timeout(led); + led->fled_cdev.ops = &flash_ops; + led_cdev = &led->fled_cdev.led_cdev; + + led_cdev->max_brightness = RT5033_LED_TORCH_CURRENT_LEVEL_MAX; + led_cdev->brightness_set_sync = rt5033_led_brightness_set; + led_cdev->flags |= LED_CORE_SUSPENDRESUME | LED_DEV_CAP_FLASH; + + ret = led_classdev_flash_register(&pdev->dev, &led->fled_cdev); + if (ret < 0) { + dev_err(&pdev->dev, "can't register LED %s\n", led_cdev->name); + return ret; + } + + regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1, + RT5033_FLED_FUNC1_MASK, RT5033_FLED_RESET); + + return 0; +} + +static int rt5033_led_remove(struct platform_device *pdev) +{ + struct rt5033_led *led = platform_get_drvdata(pdev); + + led_classdev_flash_unregister(&led->fled_cdev); + + return 0; +} + +static const struct platform_device_id rt5033_led_id[] = { + { "rt5033-led", }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(platform, rt5033_led_id); + +static const struct of_device_id rt5033_led_match[] = { + { .compatible = "richtek,rt5033-led", }, + { /* sentinel */ }, +}; +MODULE_DEVICE_TABLE(of, rt5033_led_match); + +static struct platform_driver rt5033_led_driver = { + .driver = { + .name = "rt5033-led", + .of_match_table = rt5033_led_match, + }, + .probe = rt5033_led_probe, + .id_table = rt5033_led_id, + .remove = rt5033_led_remove, +}; +module_platform_driver(rt5033_led_driver); + +MODULE_AUTHOR("Ingi Kim <ingi2.kim@samsung.com>"); +MODULE_DESCRIPTION("Richtek RT5033 LED driver"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:rt5033-led"); diff --git a/include/linux/mfd/rt5033-private.h b/include/linux/mfd/rt5033-private.h index 1b63fc2..c8e99e4 100644 --- a/include/linux/mfd/rt5033-private.h +++ b/include/linux/mfd/rt5033-private.h @@ -93,6 +93,55 @@ enum rt5033_reg { #define RT5033_RT_CTRL1_UUG_MASK 0x02 #define RT5033_RT_HZ_MASK 0x01 +/* RT5033 FLED Function1 register */ +#define RT5033_FLED_FUNC1_MASK 0x94 +#define RT5033_FLED_STRB_SEL 0x04 +#define RT5033_FLED_PINCTRL 0x10 +#define RT5033_FLED_RESET 0x80 + +/* RT5033 FLED Function2 register */ +#define RT5033_FLED_FUNC2_MASK 0x81 +#define RT5033_FLED_ENFLED 0x01 +#define RT5033_FLED_SREG_STRB 0x80 + +/* RT5033 FLED Strobe Control1 */ +#define RT5033_FLED_STRBCTRL1_MASK 0xFF +#define RT5033_FLED_STRBCTRL1_TM_CUR_MAX 0xE0 +#define RT5033_FLED_STRBCTRL1_FL_CUR_DEF 0x0D +#define RT5033_FLED_STRBCTRL1_FL_CUR_MAX 0x1F + +/* RT5033 FLED Strobe Control2 */ +#define RT5033_FLED_STRBCTRL2_MASK 0x3F +#define RT5033_FLED_STRBCTRL2_TM_DEF 0x0F +#define RT5033_FLED_STRBCTRL2_TM_MAX 0x3F + +/* RT5033 FLED Control1 */ +#define RT5033_FLED_CTRL1_MASK 0xF7 +#define RT5033_FLED_CTRL1_TORCH_CUR_DEF 0x20 +#define RT5033_FLED_CTRL1_TORCH_CUR_MAX 0xF0 +#define RT5033_FLED_CTRL1_LBP_DEF 0x02 +#define RT5033_FLED_CTRL1_LBP_MAX 0x07 + +/* RT5033 FLED Control2 */ +#define RT5033_FLED_CTRL2_MASK 0xFF +#define RT5033_FLED_CTRL2_VMID_DEF 0x37 +#define RT5033_FLED_CTRL2_VMID_MAX 0x3F +#define RT5033_FLED_CTRL2_TRACK_ALIVE 0x40 +#define RT5033_FLED_CTRL2_MID_TRACK 0x80 + +/* RT5033 FLED Control4 */ +#define RT5033_FLED_CTRL4_MASK 0xE0 +#define RT5033_FLED_CTRL4_RESV 0x14 +#define RT5033_FLED_CTRL4_VTRREG_DEF 0x40 +#define RT5033_FLED_CTRL4_VTRREG_MAX 0x60 +#define RT5033_FLED_CTRL4_TRACK_CLK 0x80 + +/* RT5033 FLED Control5 */ +#define RT5033_FLED_CTRL5_MASK 0xC0 +#define RT5033_FLED_CTRL5_RESV 0x10 +#define RT5033_FLED_CTRL5_TA_GOOD 0x40 +#define RT5033_FLED_CTRL5_TA_EXIST 0x80 + /* RT5033 control register */ #define RT5033_CTRL_FCCM_BUCK_MASK 0x00 #define RT5033_CTRL_BUCKOMS_MASK 0x01 diff --git a/include/linux/mfd/rt5033.h b/include/linux/mfd/rt5033.h index 6cff5cf..6374d84 100644 --- a/include/linux/mfd/rt5033.h +++ b/include/linux/mfd/rt5033.h @@ -12,10 +12,11 @@ #ifndef __RT5033_H__ #define __RT5033_H__ -#include <linux/regulator/consumer.h> #include <linux/i2c.h> -#include <linux/regmap.h> +#include <linux/led-class-flash.h> #include <linux/power_supply.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> /* RT5033 regulator IDs */ enum rt5033_regulators { @@ -59,4 +60,21 @@ struct rt5033_charger { struct rt5033_charger_data *chg; }; +/* RT5033 Flash led platform data */ +struct rt5033_led_config_data { + u32 torch_max_microamp; + u32 flash_max_microamp; + u32 flash_max_timeout; + enum led_brightness max_brightness; +}; + +struct rt5033_led { + struct device *dev; + struct rt5033_dev *rt5033; + struct regmap *regmap; + + struct led_classdev_flash fled_cdev; + struct rt5033_led_config_data *data; +}; + #endif /* __RT5033_H__ */ -- 2.0.5 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] leds: rt5033: Add RT5033 Flash led device driver 2015-10-12 13:12 ` [PATCH v2 3/3] leds: rt5033: Add RT5033 Flash led device driver Ingi Kim @ 2015-10-12 15:10 ` Jacek Anaszewski 2015-10-13 2:58 ` Ingi Kim 0 siblings, 1 reply; 11+ messages in thread From: Jacek Anaszewski @ 2015-10-12 15:10 UTC (permalink / raw) To: Ingi Kim Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak, sameo, lee.jones, rpurdie, inki.dae, sw0312.kim, beomho.seo, devicetree, linux-kernel, linux-leds 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@samsung.com> > --- > 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@samsung.com> > + * > + * 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? > + if (!brightness) { > + regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION2, > + RT5033_FLED_CTRL2_MASK, 0x0); > + } else { > + 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); > + } > + > + return 0; > +} > + > +static void rt5033_init_flash_timeout(struct rt5033_led *led) > +{ > + struct led_flash_setting *setting; > + > + setting = &led->fled_cdev.timeout; > + setting->min = RT5033_LED_FLASH_TIMEOUT_MIN; > + setting->max = led->data->flash_max_timeout; > + setting->step = RT5033_LED_FLASH_TIMEOUT_STEPS; > + setting->val = led->data->flash_max_timeout; > +} > + > +static int rt5033_led_parse_dt(struct rt5033_led *led, struct device *dev) > +{ > + struct device_node *np = dev->of_node; > + struct device_node *child_node; > + struct rt5033_led_config_data *data; > + int ret = 0; > + > + if (!np) > + return -ENXIO; > + > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; This way data will remain allocated until driver removal, whereas you are using it only in rt5033_init_flash_timeout during driver probing AFAICS. Please define local variable in rt5033_led_probe, pass its pointer to this function and than pass the pointer to rt5033_init_flash_timeout. > + child_node = of_get_next_available_child(np, NULL); > + if (!child_node) { > + dev_err(dev, "DT child node isn't found\n"); > + return -EINVAL; > + } > + > + led->fled_cdev.led_cdev.name = > + of_get_property(child_node, "label", NULL) ? : child_node->name; > + > + ret = of_property_read_u32(child_node, "led-max-microamp", > + &data->torch_max_microamp); > + if (ret) { > + dev_err(dev, "failed to parse led-max-microamp\n"); > + return ret; > + } data->torch_max_microamp isn't used anywhere, while it should be used for initializing led_cdev->max_brightness. > + > + ret = of_property_read_u32(child_node, "flash-max-microamp", > + &data->flash_max_microamp); > + if (ret) { > + dev_err(dev, "failed to parse flash-max-microamp\n"); > + return ret; > + } data->flash_max_microamp also isn't used anywhere. Does your device support flash brightness setting? If yes then you should implement also flash_brightness_set op. Otherwise this property is redundant. > + ret = of_property_read_u32(child_node, "flash-max-timeout-us", > + &data->flash_max_timeout); > + if (ret) > + dev_err(dev, "failed to parse flash-max-timeout-us\n"); > + > + of_node_put(child_node); > + led->data = data; > + > + return ret; > +} > + > +static int rt5033_led_flash_timeout_set(struct led_classdev_flash *fled_cdev, > + u32 timeout) > +{ > + return 0; > +} > + > +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. > + > + regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1, > + RT5033_FLED_FUNC1_MASK, RT5033_FLED_RESET); > + fled_cdev->led_cdev.brightness = LED_OFF; > + > + if (state) { > + flash_tm_reg = GET_TIMEOUT_OFFSET(fled_cdev->timeout.val, > + fled_cdev->timeout.step); > + 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 > + | RT5033_FLED_SREG_STRB); > + } > + > + return 0; > +} > + > +static const struct led_flash_ops flash_ops = { > + .strobe_set = rt5033_led_flash_strobe_set, > + .timeout_set = rt5033_led_flash_timeout_set, > +}; > + > +static int rt5033_led_probe(struct platform_device *pdev) > +{ > + struct rt5033_dev *rt5033 = dev_get_drvdata(pdev->dev.parent); > + struct rt5033_led *led; > + struct led_classdev *led_cdev; > + int ret; > + > + led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL); > + if (!led) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, led); > + led->dev = &pdev->dev; > + led->regmap = rt5033->regmap; > + > + ret = rt5033_led_parse_dt(led, &pdev->dev); > + if (ret) > + return ret; > + > + rt5033_init_flash_timeout(led); > + led->fled_cdev.ops = &flash_ops; > + led_cdev = &led->fled_cdev.led_cdev; > + > + led_cdev->max_brightness = RT5033_LED_TORCH_CURRENT_LEVEL_MAX; This should depend on led-max-microamp. > + led_cdev->brightness_set_sync = rt5033_led_brightness_set; > + led_cdev->flags |= LED_CORE_SUSPENDRESUME | LED_DEV_CAP_FLASH; You need also a work queue since your ops can sleep. > + > + ret = led_classdev_flash_register(&pdev->dev, &led->fled_cdev); > + if (ret < 0) { > + dev_err(&pdev->dev, "can't register LED %s\n", led_cdev->name); > + return ret; > + } > + > + regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1, > + RT5033_FLED_FUNC1_MASK, RT5033_FLED_RESET); > + > + return 0; > +} > + > +static int rt5033_led_remove(struct platform_device *pdev) > +{ > + struct rt5033_led *led = platform_get_drvdata(pdev); > + > + led_classdev_flash_unregister(&led->fled_cdev); > + > + return 0; > +} > + > +static const struct platform_device_id rt5033_led_id[] = { > + { "rt5033-led", }, > + { /* sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(platform, rt5033_led_id); > + > +static const struct of_device_id rt5033_led_match[] = { > + { .compatible = "richtek,rt5033-led", }, > + { /* sentinel */ }, > +}; > +MODULE_DEVICE_TABLE(of, rt5033_led_match); > + > +static struct platform_driver rt5033_led_driver = { > + .driver = { > + .name = "rt5033-led", > + .of_match_table = rt5033_led_match, > + }, > + .probe = rt5033_led_probe, > + .id_table = rt5033_led_id, > + .remove = rt5033_led_remove, > +}; > +module_platform_driver(rt5033_led_driver); > + > +MODULE_AUTHOR("Ingi Kim <ingi2.kim@samsung.com>"); > +MODULE_DESCRIPTION("Richtek RT5033 LED driver"); > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:rt5033-led"); > diff --git a/include/linux/mfd/rt5033-private.h b/include/linux/mfd/rt5033-private.h > index 1b63fc2..c8e99e4 100644 > --- a/include/linux/mfd/rt5033-private.h > +++ b/include/linux/mfd/rt5033-private.h > @@ -93,6 +93,55 @@ enum rt5033_reg { > #define RT5033_RT_CTRL1_UUG_MASK 0x02 > #define RT5033_RT_HZ_MASK 0x01 > > +/* RT5033 FLED Function1 register */ > +#define RT5033_FLED_FUNC1_MASK 0x94 > +#define RT5033_FLED_STRB_SEL 0x04 > +#define RT5033_FLED_PINCTRL 0x10 > +#define RT5033_FLED_RESET 0x80 > + > +/* RT5033 FLED Function2 register */ > +#define RT5033_FLED_FUNC2_MASK 0x81 > +#define RT5033_FLED_ENFLED 0x01 > +#define RT5033_FLED_SREG_STRB 0x80 > + > +/* RT5033 FLED Strobe Control1 */ > +#define RT5033_FLED_STRBCTRL1_MASK 0xFF > +#define RT5033_FLED_STRBCTRL1_TM_CUR_MAX 0xE0 > +#define RT5033_FLED_STRBCTRL1_FL_CUR_DEF 0x0D > +#define RT5033_FLED_STRBCTRL1_FL_CUR_MAX 0x1F > + > +/* RT5033 FLED Strobe Control2 */ > +#define RT5033_FLED_STRBCTRL2_MASK 0x3F > +#define RT5033_FLED_STRBCTRL2_TM_DEF 0x0F > +#define RT5033_FLED_STRBCTRL2_TM_MAX 0x3F > + > +/* RT5033 FLED Control1 */ > +#define RT5033_FLED_CTRL1_MASK 0xF7 > +#define RT5033_FLED_CTRL1_TORCH_CUR_DEF 0x20 > +#define RT5033_FLED_CTRL1_TORCH_CUR_MAX 0xF0 > +#define RT5033_FLED_CTRL1_LBP_DEF 0x02 > +#define RT5033_FLED_CTRL1_LBP_MAX 0x07 > + > +/* RT5033 FLED Control2 */ > +#define RT5033_FLED_CTRL2_MASK 0xFF > +#define RT5033_FLED_CTRL2_VMID_DEF 0x37 > +#define RT5033_FLED_CTRL2_VMID_MAX 0x3F > +#define RT5033_FLED_CTRL2_TRACK_ALIVE 0x40 > +#define RT5033_FLED_CTRL2_MID_TRACK 0x80 > + > +/* RT5033 FLED Control4 */ > +#define RT5033_FLED_CTRL4_MASK 0xE0 > +#define RT5033_FLED_CTRL4_RESV 0x14 > +#define RT5033_FLED_CTRL4_VTRREG_DEF 0x40 > +#define RT5033_FLED_CTRL4_VTRREG_MAX 0x60 > +#define RT5033_FLED_CTRL4_TRACK_CLK 0x80 > + > +/* RT5033 FLED Control5 */ > +#define RT5033_FLED_CTRL5_MASK 0xC0 > +#define RT5033_FLED_CTRL5_RESV 0x10 > +#define RT5033_FLED_CTRL5_TA_GOOD 0x40 > +#define RT5033_FLED_CTRL5_TA_EXIST 0x80 > + > /* RT5033 control register */ > #define RT5033_CTRL_FCCM_BUCK_MASK 0x00 > #define RT5033_CTRL_BUCKOMS_MASK 0x01 > diff --git a/include/linux/mfd/rt5033.h b/include/linux/mfd/rt5033.h > index 6cff5cf..6374d84 100644 > --- a/include/linux/mfd/rt5033.h > +++ b/include/linux/mfd/rt5033.h > @@ -12,10 +12,11 @@ > #ifndef __RT5033_H__ > #define __RT5033_H__ > > -#include <linux/regulator/consumer.h> > #include <linux/i2c.h> > -#include <linux/regmap.h> > +#include <linux/led-class-flash.h> > #include <linux/power_supply.h> > +#include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > > /* RT5033 regulator IDs */ > enum rt5033_regulators { > @@ -59,4 +60,21 @@ struct rt5033_charger { > struct rt5033_charger_data *chg; > }; > > +/* RT5033 Flash led platform data */ > +struct rt5033_led_config_data { > + u32 torch_max_microamp; > + u32 flash_max_microamp; > + u32 flash_max_timeout; > + enum led_brightness max_brightness; > +}; > + > +struct rt5033_led { > + struct device *dev; > + struct rt5033_dev *rt5033; > + struct regmap *regmap; > + > + struct led_classdev_flash fled_cdev; > + struct rt5033_led_config_data *data; It is not needed to have config data here. > +}; > + > #endif /* __RT5033_H__ */ > -- Best Regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] leds: rt5033: Add RT5033 Flash led device driver 2015-10-12 15:10 ` Jacek Anaszewski @ 2015-10-13 2:58 ` Ingi Kim 2015-10-13 8:53 ` Jacek Anaszewski 0 siblings, 1 reply; 11+ messages in thread From: Ingi Kim @ 2015-10-13 2:58 UTC (permalink / raw) To: Jacek Anaszewski Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak, sameo, lee.jones, rpurdie, inki.dae, sw0312.kim, beomho.seo, devicetree, linux-kernel, linux-leds 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@samsung.com> >> --- >> 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@samsung.com> >> + * >> + * 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. In consideration of blocking competition, however, I'd be better to use it >> + if (!brightness) { >> + regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION2, >> + RT5033_FLED_CTRL2_MASK, 0x0); >> + } else { >> + 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); >> + } >> + >> + return 0; >> +} >> + >> +static void rt5033_init_flash_timeout(struct rt5033_led *led) >> +{ >> + struct led_flash_setting *setting; >> + >> + setting = &led->fled_cdev.timeout; >> + setting->min = RT5033_LED_FLASH_TIMEOUT_MIN; >> + setting->max = led->data->flash_max_timeout; >> + setting->step = RT5033_LED_FLASH_TIMEOUT_STEPS; >> + setting->val = led->data->flash_max_timeout; >> +} >> + >> +static int rt5033_led_parse_dt(struct rt5033_led *led, struct device *dev) >> +{ >> + struct device_node *np = dev->of_node; >> + struct device_node *child_node; >> + struct rt5033_led_config_data *data; >> + int ret = 0; >> + >> + if (!np) >> + return -ENXIO; >> + >> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); >> + if (!data) >> + return -ENOMEM; > > This way data will remain allocated until driver removal, whereas > you are using it only in rt5033_init_flash_timeout during driver > probing AFAICS. Please define local variable in rt5033_led_probe, > pass its pointer to this function and than pass the pointer to > rt5033_init_flash_timeout. > You're right, I should have moved it to local, I'll try to fix it. >> + child_node = of_get_next_available_child(np, NULL); >> + if (!child_node) { >> + dev_err(dev, "DT child node isn't found\n"); >> + return -EINVAL; >> + } >> + >> + led->fled_cdev.led_cdev.name = >> + of_get_property(child_node, "label", NULL) ? : child_node->name; >> + >> + ret = of_property_read_u32(child_node, "led-max-microamp", >> + &data->torch_max_microamp); >> + if (ret) { >> + dev_err(dev, "failed to parse led-max-microamp\n"); >> + return ret; >> + } > > data->torch_max_microamp isn't used anywhere, while it should be used > for initializing led_cdev->max_brightness. > Oh, I see. I'll fix it >> + >> + ret = of_property_read_u32(child_node, "flash-max-microamp", >> + &data->flash_max_microamp); >> + if (ret) { >> + dev_err(dev, "failed to parse flash-max-microamp\n"); >> + return ret; >> + } > > data->flash_max_microamp also isn't used anywhere. Does your device > support flash brightness setting? If yes then you should implement > also flash_brightness_set op. Otherwise this property is redundant. > Yes, It support to set flash brightness. I should implement it. >> + ret = of_property_read_u32(child_node, "flash-max-timeout-us", >> + &data->flash_max_timeout); >> + if (ret) >> + dev_err(dev, "failed to parse flash-max-timeout-us\n"); >> + >> + of_node_put(child_node); >> + led->data = data; >> + >> + return ret; >> +} >> + >> +static int rt5033_led_flash_timeout_set(struct led_classdev_flash *fled_cdev, >> + u32 timeout) >> +{ >> + return 0; >> +} >> + >> +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 >> + >> + regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1, >> + RT5033_FLED_FUNC1_MASK, RT5033_FLED_RESET); >> + fled_cdev->led_cdev.brightness = LED_OFF; >> + >> + if (state) { >> + flash_tm_reg = GET_TIMEOUT_OFFSET(fled_cdev->timeout.val, >> + fled_cdev->timeout.step); >> + 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 >> + | RT5033_FLED_SREG_STRB); >> + } >> + >> + return 0; >> +} >> + >> +static const struct led_flash_ops flash_ops = { >> + .strobe_set = rt5033_led_flash_strobe_set, >> + .timeout_set = rt5033_led_flash_timeout_set, >> +}; >> + >> +static int rt5033_led_probe(struct platform_device *pdev) >> +{ >> + struct rt5033_dev *rt5033 = dev_get_drvdata(pdev->dev.parent); >> + struct rt5033_led *led; >> + struct led_classdev *led_cdev; >> + int ret; >> + >> + led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL); >> + if (!led) >> + return -ENOMEM; >> + >> + platform_set_drvdata(pdev, led); >> + led->dev = &pdev->dev; >> + led->regmap = rt5033->regmap; >> + >> + ret = rt5033_led_parse_dt(led, &pdev->dev); >> + if (ret) >> + return ret; >> + >> + rt5033_init_flash_timeout(led); >> + led->fled_cdev.ops = &flash_ops; >> + led_cdev = &led->fled_cdev.led_cdev; >> + >> + led_cdev->max_brightness = RT5033_LED_TORCH_CURRENT_LEVEL_MAX; > > This should depend on led-max-microamp. > Right, I'll fix it >> + led_cdev->brightness_set_sync = rt5033_led_brightness_set; >> + led_cdev->flags |= LED_CORE_SUSPENDRESUME | LED_DEV_CAP_FLASH; > > You need also a work queue since your ops can sleep. > Ok, I'll try to implement it >> + >> + ret = led_classdev_flash_register(&pdev->dev, &led->fled_cdev); >> + if (ret < 0) { >> + dev_err(&pdev->dev, "can't register LED %s\n", led_cdev->name); >> + return ret; >> + } >> + >> + regmap_update_bits(led->regmap, RT5033_REG_FLED_FUNCTION1, >> + RT5033_FLED_FUNC1_MASK, RT5033_FLED_RESET); >> + >> + return 0; >> +} >> + >> +static int rt5033_led_remove(struct platform_device *pdev) >> +{ >> + struct rt5033_led *led = platform_get_drvdata(pdev); >> + >> + led_classdev_flash_unregister(&led->fled_cdev); >> + >> + return 0; >> +} >> + >> +static const struct platform_device_id rt5033_led_id[] = { >> + { "rt5033-led", }, >> + { /* sentinel */ }, >> +}; >> +MODULE_DEVICE_TABLE(platform, rt5033_led_id); >> + >> +static const struct of_device_id rt5033_led_match[] = { >> + { .compatible = "richtek,rt5033-led", }, >> + { /* sentinel */ }, >> +}; >> +MODULE_DEVICE_TABLE(of, rt5033_led_match); >> + >> +static struct platform_driver rt5033_led_driver = { >> + .driver = { >> + .name = "rt5033-led", >> + .of_match_table = rt5033_led_match, >> + }, >> + .probe = rt5033_led_probe, >> + .id_table = rt5033_led_id, >> + .remove = rt5033_led_remove, >> +}; >> +module_platform_driver(rt5033_led_driver); >> + >> +MODULE_AUTHOR("Ingi Kim <ingi2.kim@samsung.com>"); >> +MODULE_DESCRIPTION("Richtek RT5033 LED driver"); >> +MODULE_LICENSE("GPL v2"); >> +MODULE_ALIAS("platform:rt5033-led"); >> diff --git a/include/linux/mfd/rt5033-private.h b/include/linux/mfd/rt5033-private.h >> index 1b63fc2..c8e99e4 100644 >> --- a/include/linux/mfd/rt5033-private.h >> +++ b/include/linux/mfd/rt5033-private.h >> @@ -93,6 +93,55 @@ enum rt5033_reg { >> #define RT5033_RT_CTRL1_UUG_MASK 0x02 >> #define RT5033_RT_HZ_MASK 0x01 >> [..] >> >> +/* RT5033 Flash led platform data */ >> +struct rt5033_led_config_data { >> + u32 torch_max_microamp; >> + u32 flash_max_microamp; >> + u32 flash_max_timeout; >> + enum led_brightness max_brightness; >> +}; >> + >> +struct rt5033_led { >> + struct device *dev; >> + struct rt5033_dev *rt5033; >> + struct regmap *regmap; >> + >> + struct led_classdev_flash fled_cdev; >> + struct rt5033_led_config_data *data; > > It is not needed to have config data here. > Okay, I try to move config data to local. >> +}; >> + >> #endif /* __RT5033_H__ */ >> > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] leds: rt5033: Add RT5033 Flash led device driver 2015-10-13 2:58 ` Ingi Kim @ 2015-10-13 8:53 ` Jacek Anaszewski 2015-10-16 6:09 ` Ingi Kim 0 siblings, 1 reply; 11+ messages in thread From: Jacek Anaszewski @ 2015-10-13 8:53 UTC (permalink / raw) To: Ingi Kim Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak, sameo, lee.jones, rpurdie, inki.dae, sw0312.kim, beomho.seo, devicetree, linux-kernel, linux-leds 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@samsung.com> >>> --- >>> 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@samsung.com> >>> + * >>> + * 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 3/3] leds: rt5033: Add RT5033 Flash led device driver 2015-10-13 8:53 ` Jacek Anaszewski @ 2015-10-16 6:09 ` Ingi Kim 0 siblings, 0 replies; 11+ messages in thread From: Ingi Kim @ 2015-10-16 6:09 UTC (permalink / raw) To: Jacek Anaszewski Cc: robh+dt, pawel.moll, mark.rutland, ijc+devicetree, galak, sameo, lee.jones, rpurdie, inki.dae, sw0312.kim, beomho.seo, devicetree, linux-kernel, linux-leds Hi Jacek, Sorry I'm late :( I'll send version3 patch soon On 2015년 10월 13일 17:53, Jacek Anaszewski wrote: > 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@samsung.com> >>>> --- >>>> 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@samsung.com> >>>> + * >>>> + * 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? > Oh, nothing on my mind, I can understand it's usage on flash led driver through your description >> 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. > Okay, thanks :) ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2015-10-16 6:09 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-10-12 13:12 [PATCH v2 0/3] Add RT5033 Flash LED driver Ingi Kim 2015-10-12 13:12 ` [PATCH v2 1/3] leds: rt5033: Add DT binding for RT5033 Ingi Kim 2015-10-12 13:12 ` [PATCH v2 2/3] mfd: rt5033: Add RT5033 Flash led sub device Ingi Kim [not found] ` <1444655578-19806-3-git-send-email-ingi2.kim-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> 2015-10-13 7:06 ` Lee Jones 2015-10-13 7:06 ` Lee Jones 2015-10-13 8:03 ` Ingi Kim 2015-10-12 13:12 ` [PATCH v2 3/3] leds: rt5033: Add RT5033 Flash led device driver Ingi Kim 2015-10-12 15:10 ` Jacek Anaszewski 2015-10-13 2:58 ` Ingi Kim 2015-10-13 8:53 ` Jacek Anaszewski 2015-10-16 6:09 ` Ingi Kim
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.