* [PATCH 1/2 v3] leds: pm8058: add device tree bindings @ 2016-08-08 18:11 Linus Walleij 2016-08-08 18:11 ` [PATCH 2/2 v3] leds: add PM8058 LEDs driver Linus Walleij ` (2 more replies) 0 siblings, 3 replies; 6+ messages in thread From: Linus Walleij @ 2016-08-08 18:11 UTC (permalink / raw) To: Jacek Anaszewski, Richard Purdie Cc: linux-leds, Linus Walleij, devicetree, linux-arm-msm, Andy Gross, Stephen Boyd, Bjorn Andersson This adds the device tree bindings for the PM8058 LEDs. Cc: devicetree@vger.kernel.org Cc: linux-arm-msm@vger.kernel.org Cc: Andy Gross <andy.gross@linaro.org> Cc: Stephen Boyd <sboyd@codeaurora.org> Cc: Bjorn Andersson <bjorn.andersson@linaro.org> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- ChangeLog v2->v3: - Rename the keypad LED in the example from "keypad" to "pm8050:white:keypad" ChangeLog v1->v2: - Fix some spelling mistakes - Reference common bindings directly without any local descriptions --- .../devicetree/bindings/leds/leds-pm8058.txt | 67 ++++++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/leds-pm8058.txt diff --git a/Documentation/devicetree/bindings/leds/leds-pm8058.txt b/Documentation/devicetree/bindings/leds/leds-pm8058.txt new file mode 100644 index 000000000000..89584c49aab2 --- /dev/null +++ b/Documentation/devicetree/bindings/leds/leds-pm8058.txt @@ -0,0 +1,67 @@ +Qualcomm PM8058 LED driver + +The Qualcomm PM8058 is a multi-functional device which contains +an LED driver block for up to six LEDs: three normal LEDs, two +"flash" LEDs and one "keypad backlight" LED. The names are +quoted because sometimes these LED drivers are used for wildly +different things than flash or keypad backlight: their names +are more of a suggestion than a hard-wired usecase. + +Hardware-wise the different LEDs support slightly different +output currents. The "flash" LEDs do not need to charge nor +do they support external triggers. They are just powerful LED +drivers. + +The LEDs appear as children to the PM8058 device, with the +proper compatible string. For the PM8058 bindings see: +mfd/qcom-pm8xxx.txt. + +Each LED is represented as a sub-node of the syscon device. Each +node's name represents the name of the corresponding LED. + +LED sub-node properties: + +Required properties: +- compatible: one of + "qcom,pm8058-led" (for the normal LEDs at 0x131, 0x132 and 0x133) + "qcom,pm8058-keypad-led" (for the "keypad" LED at 0x48) + "qcom,pm8058-flash-led" (for the "flash" LEDs at 0x49 and 0xFB) + +Optional properties: +- label: see Documentation/devicetree/bindings/leds/common.txt +- default-state: see Documentation/devicetree/bindings/leds/common.txt +- linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt + +Example: + +qcom,ssbi@500000 { + pmicintc: pmic@0 { + compatible = "qcom,pm8058"; + led@48 { + compatible = "qcom,pm8058-keypad-led"; + reg = <0x48>; + label = "pm8050:white:keypad"; + default-state = "off"; + }; + led@131 { + compatible = "qcom,pm8058-led"; + reg = <0x131>; + label = "pm8058:red"; + default-state = "off"; + }; + led@132 { + compatible = "qcom,pm8058-led"; + reg = <0x132>; + label = "pm8058:yellow"; + default-state = "off"; + linux,default-trigger = "mmc0"; + }; + led@133 { + compatible = "qcom,pm8058-led"; + reg = <0x133>; + label = "pm8058:green"; + default-state = "on"; + linux,default-trigger = "heartbeat"; + }; + }; +}; -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2 v3] leds: add PM8058 LEDs driver 2016-08-08 18:11 [PATCH 1/2 v3] leds: pm8058: add device tree bindings Linus Walleij @ 2016-08-08 18:11 ` Linus Walleij 2016-08-10 5:15 ` Bjorn Andersson 2016-08-10 4:39 ` [PATCH 1/2 v3] leds: pm8058: add device tree bindings Bjorn Andersson 2016-08-10 20:23 ` Rob Herring 2 siblings, 1 reply; 6+ messages in thread From: Linus Walleij @ 2016-08-08 18:11 UTC (permalink / raw) To: Jacek Anaszewski, Richard Purdie Cc: linux-leds, Linus Walleij, linux-arm-msm, Andy Gross, Stephen Boyd, Bjorn Andersson This adds a driver for the six PM8058 LEDs, three ordinary LEDs, two "flash" LEDs and one "keypad" LED. The "keypad" and "flash" LEDs are not really hard-wired to these usecases: for example on the APQ8060 Dragonboard, the "keypad" LED is instead used to drive an IR LED used for the proximity sensor. The "flash" LEDs are just ordinary high-current LED drivers. Cc: linux-arm-msm@vger.kernel.org Cc: Andy Gross <andy.gross@linaro.org> Cc: Stephen Boyd <sboyd@codeaurora.org> Cc: Bjorn Andersson <bjorn.andersson@linaro.org> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- ChangeLog v2->v3: - No changes, just resending. ChangeLog v1->v2: - Arrange #includes in alphabetical order - Use #define'd mask and shift and abstain from magic values - Drop surplus break; statements in switch clause - Put in the missed terminator {} in the OF match table - Use the devm_led_classdev_register() call to get managed resources - As a result drop the whole redundant .remove() clause --- drivers/leds/Kconfig | 8 ++ drivers/leds/Makefile | 1 + drivers/leds/leds-pm8058.c | 188 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 197 insertions(+) create mode 100644 drivers/leds/leds-pm8058.c diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 9dcc9b13d495..f499db5a8552 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -631,6 +631,14 @@ config LEDS_VERSATILE This option enabled support for the LEDs on the ARM Versatile and RealView boards. Say Y to enabled these. +config LEDS_PM8058 + tristate "LED Support for the Qualcomm PM8058 PMIC" + depends on MFD_PM8921_CORE + depends on LEDS_CLASS + help + Choose this option if you want to use the LED drivers in + the Qualcomm PM8058 PMIC. + comment "LED Triggers" source "drivers/leds/trigger/Kconfig" diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index 0684c865a1c0..220860c970ca 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -68,6 +68,7 @@ obj-$(CONFIG_LEDS_KTD2692) += leds-ktd2692.o obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o obj-$(CONFIG_LEDS_SEAD3) += leds-sead3.o obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o +obj-$(CONFIG_LEDS_PM8058) += leds-pm8058.o # LED SPI Drivers obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o diff --git a/drivers/leds/leds-pm8058.c b/drivers/leds/leds-pm8058.c new file mode 100644 index 000000000000..70a6a48496cc --- /dev/null +++ b/drivers/leds/leds-pm8058.c @@ -0,0 +1,188 @@ +/* Copyright (c) 2010, 2011, 2016 The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ +#include <linux/init.h> +#include <linux/leds.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm.h> +#include <linux/regmap.h> +#include <linux/slab.h> +#include <linux/spinlock.h> + +#define PM8058_LED_TYPE_COMMON 0x00 +#define PM8058_LED_TYPE_KEYPAD 0x01 +#define PM8058_LED_TYPE_FLASH 0x02 + +#define PM8058_LED_TYPE_COMMON_MASK 0xf8 +#define PM8058_LED_TYPE_KEYPAD_MASK 0xf0 +#define PM8058_LED_TYPE_COMMON_SHIFT 3 +#define PM8058_LED_TYPE_KEYPAD_SHIFT 4 + +struct pm8058_led { + struct regmap *map; + unsigned int reg; + u32 ledtype; + struct led_classdev cdev; +}; + +static void pm8058_led_set(struct led_classdev *cled, + enum led_brightness value) +{ + struct pm8058_led *led; + int ret = 0; + + led = container_of(cled, struct pm8058_led, cdev); + switch (led->ledtype) { + case PM8058_LED_TYPE_COMMON: + ret = regmap_update_bits(led->map, led->reg, + PM8058_LED_TYPE_COMMON_MASK, + value << PM8058_LED_TYPE_COMMON_SHIFT); + break; + case PM8058_LED_TYPE_KEYPAD: + case PM8058_LED_TYPE_FLASH: + ret = regmap_update_bits(led->map, led->reg, + PM8058_LED_TYPE_KEYPAD_MASK, + value << PM8058_LED_TYPE_KEYPAD_SHIFT); + break; + default: + break; + } + if (ret) + pr_err("Failed to set LED brightness\n"); +} + +static enum led_brightness pm8058_led_get(struct led_classdev *cled) +{ + struct pm8058_led *led; + int ret; + u32 val; + + led = container_of(cled, struct pm8058_led, cdev); + switch (led->ledtype) { + case PM8058_LED_TYPE_COMMON: + ret = regmap_read(led->map, led->reg, &val); + if (ret) { + pr_err("Failed to get LED brightness\n"); + return LED_OFF; + } + return ((val & PM8058_LED_TYPE_COMMON_MASK) >> + PM8058_LED_TYPE_COMMON_SHIFT); + case PM8058_LED_TYPE_KEYPAD: + case PM8058_LED_TYPE_FLASH: + ret = regmap_read(led->map, led->reg, &val); + if (ret) { + pr_err("Failed to get LED brightness\n"); + return LED_OFF; + } + return ((val & PM8058_LED_TYPE_KEYPAD_MASK) >> + PM8058_LED_TYPE_KEYPAD_SHIFT); + default: + break; + } + + return LED_OFF; +} + +static const struct of_device_id pm8058_leds_id_table[] = { + { + .compatible = "qcom,pm8058-led", + .data = (void *)PM8058_LED_TYPE_COMMON + }, + { + .compatible = "qcom,pm8058-keypad-led", + .data = (void *)PM8058_LED_TYPE_KEYPAD + }, + { + .compatible = "qcom,pm8058-flash-led", + .data = (void *)PM8058_LED_TYPE_FLASH + }, + { }, +}; +MODULE_DEVICE_TABLE(of, pm8058_leds_id_table); + +static int pm8058_led_probe(struct platform_device *pdev) +{ + struct pm8058_led *led; + struct device_node *np = pdev->dev.of_node; + int ret; + struct regmap *map; + const struct of_device_id *match; + const char *state; + + led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL); + if (led == NULL) + return -ENOMEM; + + match = of_match_node(pm8058_leds_id_table, np); + if (!match) + return -ENXIO; + led->ledtype = (u32)match->data; + + map = dev_get_regmap(pdev->dev.parent, NULL); + if (!map) { + dev_err(&pdev->dev, "Parent regmap unavailable.\n"); + return -ENXIO; + } + led->map = map; + + ret = of_property_read_u32(np, "reg", &led->reg); + if (ret) { + dev_err(&pdev->dev, "no register offset specified\n"); + return -EINVAL; + } + + /* Use label else node name */ + led->cdev.name = of_get_property(np, "label", NULL) ? : np->name; + led->cdev.default_trigger = + of_get_property(np, "linux,default-trigger", NULL); + led->cdev.brightness_set = pm8058_led_set; + led->cdev.brightness_get = pm8058_led_get; + led->cdev.max_brightness = 15; + + state = of_get_property(np, "default-state", NULL); + if (state) { + if (!strcmp(state, "keep")) { + led->cdev.brightness = pm8058_led_get(&led->cdev); + } else if (!strcmp(state, "on")) { + led->cdev.brightness = LED_FULL; + pm8058_led_set(&led->cdev, LED_FULL); + } else { + led->cdev.brightness = LED_OFF; + pm8058_led_set(&led->cdev, LED_OFF); + } + } + led->cdev.flags = LED_CORE_SUSPENDRESUME; + + ret = devm_led_classdev_register(&pdev->dev, &led->cdev); + if (ret) { + dev_err(&pdev->dev, "unable to register led \"%s\"\n", + led->cdev.name); + return ret; + } + platform_set_drvdata(pdev, led); + + return 0; +} + +static struct platform_driver pm8058_led_driver = { + .probe = pm8058_led_probe, + .driver = { + .name = "pm8058-leds", + .of_match_table = pm8058_leds_id_table, + }, +}; +module_platform_driver(pm8058_led_driver); + +MODULE_DESCRIPTION("PM8058 LEDs driver"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:pm8058-leds"); -- 2.7.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2 v3] leds: add PM8058 LEDs driver 2016-08-08 18:11 ` [PATCH 2/2 v3] leds: add PM8058 LEDs driver Linus Walleij @ 2016-08-10 5:15 ` Bjorn Andersson 2016-08-15 12:19 ` Jacek Anaszewski 0 siblings, 1 reply; 6+ messages in thread From: Bjorn Andersson @ 2016-08-10 5:15 UTC (permalink / raw) To: Linus Walleij Cc: Jacek Anaszewski, Richard Purdie, linux-leds, linux-arm-msm, Andy Gross, Stephen Boyd On Mon 08 Aug 11:11 PDT 2016, Linus Walleij wrote: > This adds a driver for the six PM8058 LEDs, three ordinary LEDs, > two "flash" LEDs and one "keypad" LED. > > The "keypad" and "flash" LEDs are not really hard-wired to these > usecases: for example on the APQ8060 Dragonboard, the "keypad" > LED is instead used to drive an IR LED used for the proximity > sensor. The "flash" LEDs are just ordinary high-current LED > drivers. > > Cc: linux-arm-msm@vger.kernel.org > Cc: Andy Gross <andy.gross@linaro.org> > Cc: Stephen Boyd <sboyd@codeaurora.org> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- [..] > diff --git a/drivers/leds/leds-pm8058.c b/drivers/leds/leds-pm8058.c [..] > +#include <linux/init.h> I don't see any uses of this. > +#include <linux/leds.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/pm.h> > +#include <linux/regmap.h> > +#include <linux/slab.h> Dito > +#include <linux/spinlock.h> Dito > + > +#define PM8058_LED_TYPE_COMMON 0x00 > +#define PM8058_LED_TYPE_KEYPAD 0x01 > +#define PM8058_LED_TYPE_FLASH 0x02 > + > +#define PM8058_LED_TYPE_COMMON_MASK 0xf8 > +#define PM8058_LED_TYPE_KEYPAD_MASK 0xf0 > +#define PM8058_LED_TYPE_COMMON_SHIFT 3 > +#define PM8058_LED_TYPE_KEYPAD_SHIFT 4 > + > +struct pm8058_led { > + struct regmap *map; > + unsigned int reg; reg should be u32, as you pass it to of_property_read_u32() > + u32 ledtype; > + struct led_classdev cdev; > +}; > + > +static void pm8058_led_set(struct led_classdev *cled, > + enum led_brightness value) > +{ > + struct pm8058_led *led; > + int ret = 0; > + > + led = container_of(cled, struct pm8058_led, cdev); > + switch (led->ledtype) { > + case PM8058_LED_TYPE_COMMON: > + ret = regmap_update_bits(led->map, led->reg, > + PM8058_LED_TYPE_COMMON_MASK, > + value << PM8058_LED_TYPE_COMMON_SHIFT); > + break; > + case PM8058_LED_TYPE_KEYPAD: > + case PM8058_LED_TYPE_FLASH: > + ret = regmap_update_bits(led->map, led->reg, > + PM8058_LED_TYPE_KEYPAD_MASK, > + value << PM8058_LED_TYPE_KEYPAD_SHIFT); > + break; > + default: "default" shouldn't ever happen, so I think you should have the switch massage value and pick a mask and then move the regmap_update_bits() after the switch. > + break; > + } > + if (ret) > + pr_err("Failed to set LED brightness\n"); > +} > + > +static enum led_brightness pm8058_led_get(struct led_classdev *cled) > +{ > + struct pm8058_led *led; > + int ret; > + u32 val; regmap_read takes an unsigned int. > + > + led = container_of(cled, struct pm8058_led, cdev); > + switch (led->ledtype) { > + case PM8058_LED_TYPE_COMMON: > + ret = regmap_read(led->map, led->reg, &val); > + if (ret) { > + pr_err("Failed to get LED brightness\n"); > + return LED_OFF; > + } > + return ((val & PM8058_LED_TYPE_COMMON_MASK) >> > + PM8058_LED_TYPE_COMMON_SHIFT); > + case PM8058_LED_TYPE_KEYPAD: > + case PM8058_LED_TYPE_FLASH: > + ret = regmap_read(led->map, led->reg, &val); > + if (ret) { > + pr_err("Failed to get LED brightness\n"); > + return LED_OFF; > + } > + return ((val & PM8058_LED_TYPE_KEYPAD_MASK) >> > + PM8058_LED_TYPE_KEYPAD_SHIFT); > + default: "default" should never happen. > + break; > + } I think you should move the read above the switch statement, then have the cases massage the value and have a common return here. regmap_read(&val) if (error) ... switch (type) case ...: val &= MASK; val >>= SHIFT; break; case ...; ... return val; Would look cleaner. > + > + return LED_OFF; > +} > + > +static const struct of_device_id pm8058_leds_id_table[] = { > + { > + .compatible = "qcom,pm8058-led", > + .data = (void *)PM8058_LED_TYPE_COMMON > + }, > + { > + .compatible = "qcom,pm8058-keypad-led", > + .data = (void *)PM8058_LED_TYPE_KEYPAD > + }, > + { > + .compatible = "qcom,pm8058-flash-led", > + .data = (void *)PM8058_LED_TYPE_FLASH > + }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, pm8058_leds_id_table); > + > +static int pm8058_led_probe(struct platform_device *pdev) > +{ > + struct pm8058_led *led; > + struct device_node *np = pdev->dev.of_node; > + int ret; > + struct regmap *map; > + const struct of_device_id *match; > + const char *state; > + > + led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL); > + if (led == NULL) if (!led) > + return -ENOMEM; > + > + match = of_match_node(pm8058_leds_id_table, np); > + if (!match) > + return -ENXIO; > + led->ledtype = (u32)match->data; Use of_device_get_match_data() instead, then you can keep the pm8058_leds_id_table at the bottom of the file. > + > + map = dev_get_regmap(pdev->dev.parent, NULL); > + if (!map) { > + dev_err(&pdev->dev, "Parent regmap unavailable.\n"); > + return -ENXIO; > + } > + led->map = map; > + > + ret = of_property_read_u32(np, "reg", &led->reg); > + if (ret) { > + dev_err(&pdev->dev, "no register offset specified\n"); > + return -EINVAL; > + } > + > + /* Use label else node name */ > + led->cdev.name = of_get_property(np, "label", NULL) ? : np->name; > + led->cdev.default_trigger = > + of_get_property(np, "linux,default-trigger", NULL); > + led->cdev.brightness_set = pm8058_led_set; > + led->cdev.brightness_get = pm8058_led_get; > + led->cdev.max_brightness = 15; For the "common" LED you have 5 bits of precision. > + > + state = of_get_property(np, "default-state", NULL); > + if (state) { > + if (!strcmp(state, "keep")) { > + led->cdev.brightness = pm8058_led_get(&led->cdev); > + } else if (!strcmp(state, "on")) { > + led->cdev.brightness = LED_FULL; > + pm8058_led_set(&led->cdev, LED_FULL); It's kinda ugly to let regmap_update_bits() shave off the upper 3 or 4 bits of LED_FULL. Perhaps pass max_brightness here instead? There's no need to limit the paramter inside pm8058_led_set(), as subsequent calls to set_brightness() will pass through a min(value, max). > + } else { > + led->cdev.brightness = LED_OFF; > + pm8058_led_set(&led->cdev, LED_OFF); > + } > + } > + led->cdev.flags = LED_CORE_SUSPENDRESUME; I can see this being a feature for the keyboard and flash, but for the "common" LEDs I don't think this is the right thing to do. > + > + ret = devm_led_classdev_register(&pdev->dev, &led->cdev); > + if (ret) { > + dev_err(&pdev->dev, "unable to register led \"%s\"\n", > + led->cdev.name); > + return ret; > + } > + platform_set_drvdata(pdev, led); You don't retrieve the drvdata, so no need to set it. > + > + return 0; > +} > + > +static struct platform_driver pm8058_led_driver = { > + .probe = pm8058_led_probe, > + .driver = { > + .name = "pm8058-leds", > + .of_match_table = pm8058_leds_id_table, > + }, > +}; > +module_platform_driver(pm8058_led_driver); > + > +MODULE_DESCRIPTION("PM8058 LEDs driver"); > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:pm8058-leds"); Regards, Bjorn ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2 v3] leds: add PM8058 LEDs driver 2016-08-10 5:15 ` Bjorn Andersson @ 2016-08-15 12:19 ` Jacek Anaszewski 0 siblings, 0 replies; 6+ messages in thread From: Jacek Anaszewski @ 2016-08-15 12:19 UTC (permalink / raw) To: Bjorn Andersson, Linus Walleij Cc: Jacek Anaszewski, Richard Purdie, linux-leds, linux-arm-msm, Andy Gross, Stephen Boyd Hi Bjorn, Thanks for the valuable review. Linus, please address the issues spotted below. Thanks, Jacek Anaszewski On 08/10/2016 07:15 AM, Bjorn Andersson wrote: > On Mon 08 Aug 11:11 PDT 2016, Linus Walleij wrote: > >> This adds a driver for the six PM8058 LEDs, three ordinary LEDs, >> two "flash" LEDs and one "keypad" LED. >> >> The "keypad" and "flash" LEDs are not really hard-wired to these >> usecases: for example on the APQ8060 Dragonboard, the "keypad" >> LED is instead used to drive an IR LED used for the proximity >> sensor. The "flash" LEDs are just ordinary high-current LED >> drivers. >> >> Cc: linux-arm-msm@vger.kernel.org >> Cc: Andy Gross <andy.gross@linaro.org> >> Cc: Stephen Boyd <sboyd@codeaurora.org> >> Cc: Bjorn Andersson <bjorn.andersson@linaro.org> >> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> >> --- > [..] >> diff --git a/drivers/leds/leds-pm8058.c b/drivers/leds/leds-pm8058.c > [..] >> +#include <linux/init.h> > > I don't see any uses of this. > >> +#include <linux/leds.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/pm.h> >> +#include <linux/regmap.h> >> +#include <linux/slab.h> > > Dito > >> +#include <linux/spinlock.h> > > Dito > >> + >> +#define PM8058_LED_TYPE_COMMON 0x00 >> +#define PM8058_LED_TYPE_KEYPAD 0x01 >> +#define PM8058_LED_TYPE_FLASH 0x02 >> + >> +#define PM8058_LED_TYPE_COMMON_MASK 0xf8 >> +#define PM8058_LED_TYPE_KEYPAD_MASK 0xf0 >> +#define PM8058_LED_TYPE_COMMON_SHIFT 3 >> +#define PM8058_LED_TYPE_KEYPAD_SHIFT 4 >> + >> +struct pm8058_led { >> + struct regmap *map; >> + unsigned int reg; > > reg should be u32, as you pass it to of_property_read_u32() > >> + u32 ledtype; >> + struct led_classdev cdev; >> +}; >> + >> +static void pm8058_led_set(struct led_classdev *cled, >> + enum led_brightness value) >> +{ >> + struct pm8058_led *led; >> + int ret = 0; >> + >> + led = container_of(cled, struct pm8058_led, cdev); >> + switch (led->ledtype) { >> + case PM8058_LED_TYPE_COMMON: >> + ret = regmap_update_bits(led->map, led->reg, >> + PM8058_LED_TYPE_COMMON_MASK, >> + value << PM8058_LED_TYPE_COMMON_SHIFT); >> + break; >> + case PM8058_LED_TYPE_KEYPAD: >> + case PM8058_LED_TYPE_FLASH: >> + ret = regmap_update_bits(led->map, led->reg, >> + PM8058_LED_TYPE_KEYPAD_MASK, >> + value << PM8058_LED_TYPE_KEYPAD_SHIFT); >> + break; >> + default: > > "default" shouldn't ever happen, so I think you should have the switch > massage value and pick a mask and then move the regmap_update_bits() > after the switch. > >> + break; >> + } >> + if (ret) >> + pr_err("Failed to set LED brightness\n"); >> +} >> + >> +static enum led_brightness pm8058_led_get(struct led_classdev *cled) >> +{ >> + struct pm8058_led *led; >> + int ret; >> + u32 val; > > regmap_read takes an unsigned int. > >> + >> + led = container_of(cled, struct pm8058_led, cdev); >> + switch (led->ledtype) { >> + case PM8058_LED_TYPE_COMMON: >> + ret = regmap_read(led->map, led->reg, &val); >> + if (ret) { >> + pr_err("Failed to get LED brightness\n"); >> + return LED_OFF; >> + } >> + return ((val & PM8058_LED_TYPE_COMMON_MASK) >> >> + PM8058_LED_TYPE_COMMON_SHIFT); >> + case PM8058_LED_TYPE_KEYPAD: >> + case PM8058_LED_TYPE_FLASH: >> + ret = regmap_read(led->map, led->reg, &val); >> + if (ret) { >> + pr_err("Failed to get LED brightness\n"); >> + return LED_OFF; >> + } >> + return ((val & PM8058_LED_TYPE_KEYPAD_MASK) >> >> + PM8058_LED_TYPE_KEYPAD_SHIFT); >> + default: > > "default" should never happen. > >> + break; >> + } > > I think you should move the read above the switch statement, then have > the cases massage the value and have a common return here. > > regmap_read(&val) > if (error) > ... > > switch (type) > case ...: > val &= MASK; > val >>= SHIFT; > break; > case ...; > ... > > return val; > > Would look cleaner. > >> + >> + return LED_OFF; >> +} >> + >> +static const struct of_device_id pm8058_leds_id_table[] = { >> + { >> + .compatible = "qcom,pm8058-led", >> + .data = (void *)PM8058_LED_TYPE_COMMON >> + }, >> + { >> + .compatible = "qcom,pm8058-keypad-led", >> + .data = (void *)PM8058_LED_TYPE_KEYPAD >> + }, >> + { >> + .compatible = "qcom,pm8058-flash-led", >> + .data = (void *)PM8058_LED_TYPE_FLASH >> + }, >> + { }, >> +}; >> +MODULE_DEVICE_TABLE(of, pm8058_leds_id_table); >> + >> +static int pm8058_led_probe(struct platform_device *pdev) >> +{ >> + struct pm8058_led *led; >> + struct device_node *np = pdev->dev.of_node; >> + int ret; >> + struct regmap *map; >> + const struct of_device_id *match; >> + const char *state; >> + >> + led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL); >> + if (led == NULL) > > if (!led) > >> + return -ENOMEM; >> + >> + match = of_match_node(pm8058_leds_id_table, np); >> + if (!match) >> + return -ENXIO; >> + led->ledtype = (u32)match->data; > > Use of_device_get_match_data() instead, then you can keep the > pm8058_leds_id_table at the bottom of the file. > >> + >> + map = dev_get_regmap(pdev->dev.parent, NULL); >> + if (!map) { >> + dev_err(&pdev->dev, "Parent regmap unavailable.\n"); >> + return -ENXIO; >> + } >> + led->map = map; >> + >> + ret = of_property_read_u32(np, "reg", &led->reg); >> + if (ret) { >> + dev_err(&pdev->dev, "no register offset specified\n"); >> + return -EINVAL; >> + } >> + >> + /* Use label else node name */ >> + led->cdev.name = of_get_property(np, "label", NULL) ? : np->name; >> + led->cdev.default_trigger = >> + of_get_property(np, "linux,default-trigger", NULL); >> + led->cdev.brightness_set = pm8058_led_set; >> + led->cdev.brightness_get = pm8058_led_get; >> + led->cdev.max_brightness = 15; > > For the "common" LED you have 5 bits of precision. > >> + >> + state = of_get_property(np, "default-state", NULL); >> + if (state) { >> + if (!strcmp(state, "keep")) { >> + led->cdev.brightness = pm8058_led_get(&led->cdev); >> + } else if (!strcmp(state, "on")) { >> + led->cdev.brightness = LED_FULL; >> + pm8058_led_set(&led->cdev, LED_FULL); > > It's kinda ugly to let regmap_update_bits() shave off the upper 3 or 4 > bits of LED_FULL. Perhaps pass max_brightness here instead? > > There's no need to limit the paramter inside pm8058_led_set(), as > subsequent calls to set_brightness() will pass through a min(value, > max). > >> + } else { >> + led->cdev.brightness = LED_OFF; >> + pm8058_led_set(&led->cdev, LED_OFF); >> + } >> + } >> + led->cdev.flags = LED_CORE_SUSPENDRESUME; > > I can see this being a feature for the keyboard and flash, but for the > "common" LEDs I don't think this is the right thing to do. > >> + >> + ret = devm_led_classdev_register(&pdev->dev, &led->cdev); >> + if (ret) { >> + dev_err(&pdev->dev, "unable to register led \"%s\"\n", >> + led->cdev.name); >> + return ret; >> + } >> + platform_set_drvdata(pdev, led); > > You don't retrieve the drvdata, so no need to set it. > >> + >> + return 0; >> +} >> + >> +static struct platform_driver pm8058_led_driver = { >> + .probe = pm8058_led_probe, >> + .driver = { >> + .name = "pm8058-leds", >> + .of_match_table = pm8058_leds_id_table, >> + }, >> +}; >> +module_platform_driver(pm8058_led_driver); >> + >> +MODULE_DESCRIPTION("PM8058 LEDs driver"); >> +MODULE_LICENSE("GPL v2"); >> +MODULE_ALIAS("platform:pm8058-leds"); > > Regards, > Bjorn > -- > To unsubscribe from this list: send the line "unsubscribe linux-leds" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2 v3] leds: pm8058: add device tree bindings 2016-08-08 18:11 [PATCH 1/2 v3] leds: pm8058: add device tree bindings Linus Walleij 2016-08-08 18:11 ` [PATCH 2/2 v3] leds: add PM8058 LEDs driver Linus Walleij @ 2016-08-10 4:39 ` Bjorn Andersson 2016-08-10 20:23 ` Rob Herring 2 siblings, 0 replies; 6+ messages in thread From: Bjorn Andersson @ 2016-08-10 4:39 UTC (permalink / raw) To: Linus Walleij Cc: Jacek Anaszewski, Richard Purdie, linux-leds, devicetree, linux-arm-msm, Andy Gross, Stephen Boyd On Mon 08 Aug 11:11 PDT 2016, Linus Walleij wrote: > This adds the device tree bindings for the PM8058 LEDs. > > Cc: devicetree@vger.kernel.org > Cc: linux-arm-msm@vger.kernel.org > Cc: Andy Gross <andy.gross@linaro.org> > Cc: Stephen Boyd <sboyd@codeaurora.org> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> Looks reasonable Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org> Regards, Bjorn > --- > ChangeLog v2->v3: > - Rename the keypad LED in the example from "keypad" to > "pm8050:white:keypad" > ChangeLog v1->v2: > - Fix some spelling mistakes > - Reference common bindings directly without any local descriptions > --- > .../devicetree/bindings/leds/leds-pm8058.txt | 67 ++++++++++++++++++++++ > 1 file changed, 67 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/leds-pm8058.txt > > diff --git a/Documentation/devicetree/bindings/leds/leds-pm8058.txt b/Documentation/devicetree/bindings/leds/leds-pm8058.txt > new file mode 100644 > index 000000000000..89584c49aab2 > --- /dev/null > +++ b/Documentation/devicetree/bindings/leds/leds-pm8058.txt > @@ -0,0 +1,67 @@ > +Qualcomm PM8058 LED driver > + > +The Qualcomm PM8058 is a multi-functional device which contains > +an LED driver block for up to six LEDs: three normal LEDs, two > +"flash" LEDs and one "keypad backlight" LED. The names are > +quoted because sometimes these LED drivers are used for wildly > +different things than flash or keypad backlight: their names > +are more of a suggestion than a hard-wired usecase. > + > +Hardware-wise the different LEDs support slightly different > +output currents. The "flash" LEDs do not need to charge nor > +do they support external triggers. They are just powerful LED > +drivers. > + > +The LEDs appear as children to the PM8058 device, with the > +proper compatible string. For the PM8058 bindings see: > +mfd/qcom-pm8xxx.txt. > + > +Each LED is represented as a sub-node of the syscon device. Each > +node's name represents the name of the corresponding LED. > + > +LED sub-node properties: > + > +Required properties: > +- compatible: one of > + "qcom,pm8058-led" (for the normal LEDs at 0x131, 0x132 and 0x133) > + "qcom,pm8058-keypad-led" (for the "keypad" LED at 0x48) > + "qcom,pm8058-flash-led" (for the "flash" LEDs at 0x49 and 0xFB) > + > +Optional properties: > +- label: see Documentation/devicetree/bindings/leds/common.txt > +- default-state: see Documentation/devicetree/bindings/leds/common.txt > +- linux,default-trigger: see Documentation/devicetree/bindings/leds/common.txt > + > +Example: > + > +qcom,ssbi@500000 { > + pmicintc: pmic@0 { > + compatible = "qcom,pm8058"; > + led@48 { > + compatible = "qcom,pm8058-keypad-led"; > + reg = <0x48>; > + label = "pm8050:white:keypad"; > + default-state = "off"; > + }; > + led@131 { > + compatible = "qcom,pm8058-led"; > + reg = <0x131>; > + label = "pm8058:red"; > + default-state = "off"; > + }; > + led@132 { > + compatible = "qcom,pm8058-led"; > + reg = <0x132>; > + label = "pm8058:yellow"; > + default-state = "off"; > + linux,default-trigger = "mmc0"; > + }; > + led@133 { > + compatible = "qcom,pm8058-led"; > + reg = <0x133>; > + label = "pm8058:green"; > + default-state = "on"; > + linux,default-trigger = "heartbeat"; > + }; > + }; > +}; > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2 v3] leds: pm8058: add device tree bindings 2016-08-08 18:11 [PATCH 1/2 v3] leds: pm8058: add device tree bindings Linus Walleij 2016-08-08 18:11 ` [PATCH 2/2 v3] leds: add PM8058 LEDs driver Linus Walleij 2016-08-10 4:39 ` [PATCH 1/2 v3] leds: pm8058: add device tree bindings Bjorn Andersson @ 2016-08-10 20:23 ` Rob Herring 2 siblings, 0 replies; 6+ messages in thread From: Rob Herring @ 2016-08-10 20:23 UTC (permalink / raw) To: Linus Walleij Cc: Jacek Anaszewski, Richard Purdie, linux-leds, devicetree, linux-arm-msm, Andy Gross, Stephen Boyd, Bjorn Andersson On Mon, Aug 08, 2016 at 08:11:50PM +0200, Linus Walleij wrote: > This adds the device tree bindings for the PM8058 LEDs. > > Cc: devicetree@vger.kernel.org > Cc: linux-arm-msm@vger.kernel.org > Cc: Andy Gross <andy.gross@linaro.org> > Cc: Stephen Boyd <sboyd@codeaurora.org> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > ChangeLog v2->v3: > - Rename the keypad LED in the example from "keypad" to > "pm8050:white:keypad" > ChangeLog v1->v2: > - Fix some spelling mistakes > - Reference common bindings directly without any local descriptions > --- > .../devicetree/bindings/leds/leds-pm8058.txt | 67 ++++++++++++++++++++++ > 1 file changed, 67 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/leds-pm8058.txt Acked-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-08-15 12:19 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-08 18:11 [PATCH 1/2 v3] leds: pm8058: add device tree bindings Linus Walleij 2016-08-08 18:11 ` [PATCH 2/2 v3] leds: add PM8058 LEDs driver Linus Walleij 2016-08-10 5:15 ` Bjorn Andersson 2016-08-15 12:19 ` Jacek Anaszewski 2016-08-10 4:39 ` [PATCH 1/2 v3] leds: pm8058: add device tree bindings Bjorn Andersson 2016-08-10 20:23 ` Rob Herring
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.