| Message ID | 1422060853-1067-1-git-send-email-bjorn.andersson@sonymobile.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, Jan 23, 2015 at 4:54 PM, Bjorn Andersson <bjorn.andersson@sonymobile.com> wrote: > From: Courtney Cavin <courtney.cavin@sonymobile.com> > > This adds support for the WLED ('White' LED) block on Qualcomm's > PM8941 PMICs. > > Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com> > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> > --- Sorry, I missed the change log. Changed since v1: * Replace enum blob with defines * Merged wled_context and wled structs * Some style updates Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, Just few nitpick comments. On Fri, 2015-01-23 at 16:54 -0800, Bjorn Andersson wrote: > From: Courtney Cavin cavin@sonymobile.com> > > This adds support for the WLED ('White' LED) block on Qualcomm's > PM8941 PMICs. > > Signed-off-by: Courtney Cavin cavin@sonymobile.com> > Signed-off-by: Bjorn Andersson andersson@sonymobile.com> > --- > drivers/leds/Kconfig | 8 + > drivers/leds/Makefile | 1 + > drivers/leds/leds-pm8941-wled.c | 459 ++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 468 insertions(+) > create mode 100644 drivers/leds/leds-pm8941-wled.c > <snip> > + > +#define PM8941_WLED_REG_VAL_BASE 0x40 > +#define PM8941_WLED_REG_VAL_MAX 0xFFF > + > +#define PM8941_WLED_REG_MOD_EN 0x46 > +#define PM8941_WLED_REG_MOD_EN_BIT BIT(7) > +#define PM8941_WLED_REG_MOD_EN_MASK BIT(7) Is it possible bit definitions to have same indentation like registers offsets? > > +struct pm8941_wled_config { > + u32 i_boost_limit; > + u32 ovp; > + u32 switch_freq; > + u32 num_strings; > + u32 i_limit; > + bool cs_out_en; > + bool ext_gen; > + bool cabc_en; > +}; > + Could this be further squashed to bellow structure? > +struct pm8941_wled { > + struct regmap *regmap; > + u16 addr; > + > + struct led_classdev cdev; > + > + struct pm8941_wled_config cfg; > +}; > + > <snip> > +static void pm8941_wled_set_brightness(struct led_classdev *cdev, > + enum > led_brightness value) > +{ > + if (pm8941_wled_set(cdev, value)) { pm8941_wled_set() is used only here, could it be merged into this function? > + dev_err(cdev->dev, "Unable to set brightness\n"); > + return; > + } > + cdev->brightness = value; > +} > + > Otherwise it looks good. Driver is loaded and device is detected properly (i have added readings for type and subtype registers). Do you know where I can measure result from changing brightness sysfs entry. I am using 8074 dragonboard? Thank you, Ivan -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 29 Jan 04:48 PST 2015, Ivan T. Ivanov wrote: > > Hi Bjorn, > > Just few nitpick comments. > Thanks. > On Fri, 2015-01-23 at 16:54 -0800, Bjorn Andersson wrote: > > From: Courtney Cavin cavin@sonymobile.com> > > > > This adds support for the WLED ('White' LED) block on Qualcomm's > > PM8941 PMICs. > > > > Signed-off-by: Courtney Cavin cavin@sonymobile.com> > > Signed-off-by: Bjorn Andersson andersson@sonymobile.com> > > --- > > drivers/leds/Kconfig | 8 + > > drivers/leds/Makefile | 1 + > > drivers/leds/leds-pm8941-wled.c | 459 ++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 468 insertions(+) > > create mode 100644 drivers/leds/leds-pm8941-wled.c > > > > <snip> > > > + > > +#define PM8941_WLED_REG_VAL_BASE 0x40 > > +#define PM8941_WLED_REG_VAL_MAX 0xFFF > > + > > +#define PM8941_WLED_REG_MOD_EN 0x46 > > +#define PM8941_WLED_REG_MOD_EN_BIT BIT(7) > > +#define PM8941_WLED_REG_MOD_EN_MASK BIT(7) > > Is it possible bit definitions to have same indentation like registers offsets? > Well, yes ;) But I like the separation, so unless Bryan would like me to change it I prefer to leave it as is. > > > > +struct pm8941_wled_config { > > + u32 i_boost_limit; > > + u32 ovp; > > + u32 switch_freq; > > + u32 num_strings; > > + u32 i_limit; > > + bool cs_out_en; > > + bool ext_gen; > > + bool cabc_en; > > +}; > > + > > Could this be further squashed to bellow structure? > It could, but I see that it would simplify things. > > +struct pm8941_wled { > > + struct regmap *regmap; > > + u16 addr; > > + > > + struct led_classdev cdev; > > + > > + struct pm8941_wled_config cfg; > > +}; > > + > > > > <snip> > > > +static void pm8941_wled_set_brightness(struct led_classdev *cdev, > > + enum > > led_brightness value) > > +{ > > + if (pm8941_wled_set(cdev, value)) { > > pm8941_wled_set() is used only here, could it be merged into this function? > It could, but that would just mean that we move these lines into the tail of pm8941_wled_set and replace all the returns with a bunch of gotos. So I don't think it's cleaner. > > + dev_err(cdev->dev, "Unable to set brightness\n"); > > + return; > > + } > > + cdev->brightness = value; > > +} > > + > > > > Otherwise it looks good. Driver is loaded and device is detected > properly (i have added readings for type and subtype registers). > Do you know where I can measure result from changing brightness > sysfs entry. I am using 8074 dragonboard? > I'm not sure how this is exposed on the dragonboard. I've tested it on Xperia Z1 (honami) and the display lights up nicely. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 23, 2015 at 4:54 PM, Bjorn Andersson <bjorn.andersson@sonymobile.com> wrote: > From: Courtney Cavin <courtney.cavin@sonymobile.com> > > This adds device tree binding documentation for the WLED ('White' LED) > block on Qualcomm's PM8941 PMICs. > > Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com> > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> I will merge this into my branch for next 3.20 release. Thanks, -Bryan > --- > .../devicetree/bindings/leds/leds-pm8941-wled.txt | 43 ++++++++++++++++++++++ > 1 file changed, 43 insertions(+) > create mode 100644 Documentation/devicetree/bindings/leds/leds-pm8941-wled.txt > > diff --git a/Documentation/devicetree/bindings/leds/leds-pm8941-wled.txt b/Documentation/devicetree/bindings/leds/leds-pm8941-wled.txt > new file mode 100644 > index 0000000..a85a964 > --- /dev/null > +++ b/Documentation/devicetree/bindings/leds/leds-pm8941-wled.txt > @@ -0,0 +1,43 @@ > +Binding for Qualcomm PM8941 WLED driver > + > +Required properties: > +- compatible: should be "qcom,pm8941-wled" > +- reg: slave address > + > +Optional properties: > +- label: The label for this led > + See Documentation/devicetree/bindings/leds/common.txt > +- linux,default-trigger: Default trigger assigned to the LED > + See Documentation/devicetree/bindings/leds/common.txt > +- qcom,cs-out: bool; enable current sink output > +- qcom,cabc: bool; enable content adaptive backlight control > +- qcom,ext-gen: bool; use externally generated modulator signal to dim > +- qcom,current-limit: mA; per-string current limit; value from 0 to 25 > + default: 20mA > +- qcom,current-boost-limit: mA; boost current limit; one of: > + 105, 385, 525, 805, 980, 1260, 1400, 1680 > + default: 805mA > +- qcom,switching-freq: kHz; switching frequency; one of: > + 600, 640, 685, 738, 800, 872, 960, 1066, 1200, 1371, > + 1600, 1920, 2400, 3200, 4800, 9600, > + default: 1600kHz > +- qcom,ovp: V; Over-voltage protection limit; one of: > + 27, 29, 32, 35 > + default: 29V > +- qcom,num-strings: #; number of led strings attached; value from 1 to 3 > + default: 2 > + > +Example: > + > +pm8941-wled@d800 { > + compatible = "qcom,pm8941-wled"; > + reg = <0xd800>; > + label = "backlight"; > + > + qcom,cs-out; > + qcom,current-limit = <20>; > + qcom,current-boost-limit = <805>; > + qcom,switching-freq = <1600>; > + qcom,ovp = <29>; > + qcom,num-strings = <2>; > +}; > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 29, 2015 at 11:07 AM, Bjorn Andersson <bjorn.andersson@sonymobile.com> wrote: > On Thu 29 Jan 04:48 PST 2015, Ivan T. Ivanov wrote: > >> >> Hi Bjorn, >> >> Just few nitpick comments. >> > > Thanks. > >> On Fri, 2015-01-23 at 16:54 -0800, Bjorn Andersson wrote: >> > From: Courtney Cavin cavin@sonymobile.com> >> > >> > This adds support for the WLED ('White' LED) block on Qualcomm's >> > PM8941 PMICs. >> > >> > Signed-off-by: Courtney Cavin cavin@sonymobile.com> >> > Signed-off-by: Bjorn Andersson andersson@sonymobile.com> I'm good with this patch and will merge it into my tree. Thanks, -Bryan >> > --- >> > drivers/leds/Kconfig | 8 + >> > drivers/leds/Makefile | 1 + >> > drivers/leds/leds-pm8941-wled.c | 459 ++++++++++++++++++++++++++++++++++++++++ >> > 3 files changed, 468 insertions(+) >> > create mode 100644 drivers/leds/leds-pm8941-wled.c >> > >> >> <snip> >> >> > + >> > +#define PM8941_WLED_REG_VAL_BASE 0x40 >> > +#define PM8941_WLED_REG_VAL_MAX 0xFFF >> > + >> > +#define PM8941_WLED_REG_MOD_EN 0x46 >> > +#define PM8941_WLED_REG_MOD_EN_BIT BIT(7) >> > +#define PM8941_WLED_REG_MOD_EN_MASK BIT(7) >> >> Is it possible bit definitions to have same indentation like registers offsets? >> > > Well, yes ;) > > But I like the separation, so unless Bryan would like me to change it I prefer > to leave it as is. > >> > >> > +struct pm8941_wled_config { >> > + u32 i_boost_limit; >> > + u32 ovp; >> > + u32 switch_freq; >> > + u32 num_strings; >> > + u32 i_limit; >> > + bool cs_out_en; >> > + bool ext_gen; >> > + bool cabc_en; >> > +}; >> > + >> >> Could this be further squashed to bellow structure? >> > > It could, but I see that it would simplify things. > >> > +struct pm8941_wled { >> > + struct regmap *regmap; >> > + u16 addr; >> > + >> > + struct led_classdev cdev; >> > + >> > + struct pm8941_wled_config cfg; >> > +}; >> > + >> > >> >> <snip> >> >> > +static void pm8941_wled_set_brightness(struct led_classdev *cdev, >> > + enum >> > led_brightness value) >> > +{ >> > + if (pm8941_wled_set(cdev, value)) { >> >> pm8941_wled_set() is used only here, could it be merged into this function? >> > > It could, but that would just mean that we move these lines into the tail of > pm8941_wled_set and replace all the returns with a bunch of gotos. So I don't > think it's cleaner. > >> > + dev_err(cdev->dev, "Unable to set brightness\n"); >> > + return; >> > + } >> > + cdev->brightness = value; >> > +} >> > + >> > >> >> Otherwise it looks good. Driver is loaded and device is detected >> properly (i have added readings for type and subtype registers). >> Do you know where I can measure result from changing brightness >> sysfs entry. I am using 8074 dragonboard? >> > > I'm not sure how this is exposed on the dragonboard. I've tested it on Xperia > Z1 (honami) and the display lights up nicely. > > Regards, > Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/23/15 16:54, Bjorn Andersson wrote: > + > +static int pm8941_wled_configure(struct pm8941_wled *wled, struct device *dev) > +{ > + struct pm8941_wled_config *cfg = &wled->cfg; > + u32 val; > + int rc; > + int i; > + > + const struct { > + const char *name; > + u32 *val_ptr; > + const struct pm8941_wled_var_cfg *cfg; > + } u32_opts[] = { > + { > + "qcom,current-boost-limit", > + &cfg->i_boost_limit, > + .cfg = &pm8941_wled_i_boost_limit_cfg, > + }, > + { > + "qcom,current-limit", > + &cfg->i_limit, > + .cfg = &pm8941_wled_i_limit_cfg, > + }, > + { > + "qcom,ovp", > + &cfg->ovp, > + .cfg = &pm8941_wled_ovp_cfg, > + }, > + { > + "qcom,switching-freq", > + &cfg->switch_freq, > + .cfg = &pm8941_wled_switch_freq_cfg, > + }, > + { > + "qcom,num-strings", > + &cfg->num_strings, > + .cfg = &pm8941_wled_num_strings_cfg, > + }, > + }; > + const struct { > + const char *name; > + bool *val_ptr; > + } bool_opts[] = { > + { "qcom,cs-out", &cfg->cs_out_en, }, > + { "qcom,ext-gen", &cfg->ext_gen, }, > + { "qcom,cabc", &cfg->cabc_en, }, > + }; > + > + rc = of_property_read_u32(dev->of_node, "reg", &val); > + if (rc || val > 0xffff) { > + dev_err(dev, "invalid IO resources\n"); > + return rc ? rc : -EINVAL; > + } > + wled->addr = val; > + > + rc = of_property_read_string(dev->of_node, "label", &wled->cdev.name); > + if (rc) > + wled->cdev.name = dev->of_node->name; > + > + wled->cdev.default_trigger = of_get_property(dev->of_node, > + "linux,default-trigger", NULL); > + > + *cfg = pm8941_wled_config_defaults; > + for (i = 0; i < ARRAY_SIZE(u32_opts); ++i) { > + u32 sel, c; > + int j, rj; > + > + rc = of_property_read_u32(dev->of_node, u32_opts[i].name, &val); > + if (rc) { > + if (rc != -EINVAL) { > + dev_err(dev, "error reading '%s'\n", > + u32_opts[i].name); > + return rc; > + } > + continue; > + } > + > + sel = UINT_MAX; > + rj = -1; > + c = pm8941_wled_values(u32_opts[i].cfg, 0); > + for (j = 0; c != UINT_MAX; ++j) { > + if (c <= val && (sel == UINT_MAX || c >= sel)) { > + sel = c; > + rj = j; > + } > + c = pm8941_wled_values(u32_opts[i].cfg, j + 1); > + } > + if (sel == UINT_MAX) { > + dev_err(dev, "invalid value for '%s'\n", > + u32_opts[i].name); > + return rc; Isn't rc always 0 here? Don't we want to return an error? Also, I find this code very convoluted given that we loop through a table and match based on nodes and call function pointers, etc. Why can't we just have a handful of if statements with of_property_read_u32 in them? That way we don't have to jump through so many hoops, bouncing all around this file to figure out what's going on. If we did I imagine we wouldn't have missed out on rc being 0 here. > + > +static int pm8941_wled_remove(struct platform_device *pdev) > +{ > + struct pm8941_wled *wled; > + > + wled = platform_get_drvdata(pdev); > + led_classdev_unregister(&wled->cdev); Would be nice to have a devm for this one too. > + > + return 0; > +} > + > +static const struct of_device_id pm8941_wled_match_table[] = { > + { .compatible = "qcom,pm8941-wled" }, > + {} > +}; > +MODULE_DEVICE_TABLE(of, pm8941_wled_match_table); > + > +static struct platform_driver pm8941_wled_driver = { > + .probe = pm8941_wled_probe, > + .remove = pm8941_wled_remove, > + .driver = { > + .name = "pm8941-wled", > + .owner = THIS_MODULE, THIS_MODULE should be removed.
On 01/29/15 04:48, Ivan T. Ivanov wrote: > > Otherwise it looks good. Driver is loaded and device is detected > properly (i have added readings for type and subtype registers). > Do you know where I can measure result from changing brightness > sysfs entry. I am using 8074 dragonboard? Does the backlight turn on? From what I can tell it controls the backlight, but it may be that nothings getting displayed so it won't be noticeable.
On 01/23/15 16:54, Bjorn Andersson wrote: > + > +static int pm8941_wled_set(struct led_classdev *cdev, > + enum led_brightness value) > +{ > + struct pm8941_wled *wled; > + u8 ctrl = 0; > + u16 val; > + int rc; > + int i; > + > + wled = container_of(cdev, struct pm8941_wled, cdev); > + > + if (value != 0) > + ctrl = PM8941_WLED_REG_MOD_EN_BIT; > + > + val = value * PM8941_WLED_REG_VAL_MAX / LED_FULL; > + > + rc = regmap_update_bits(wled->regmap, > + wled->addr + PM8941_WLED_REG_MOD_EN, > + PM8941_WLED_REG_MOD_EN_MASK, ctrl); > + if (rc) > + return rc; > + > + for (i = 0; i < wled->cfg.num_strings; ++i) { > + u8 v[2] = { val & 0xff, (val >> 8) & 0xf }; > + > + rc = regmap_bulk_write(wled->regmap, > + wled->addr + PM8941_WLED_REG_VAL_BASE + 2 * i, > + v, 2); > + if (rc) > + return rc; > + } > + > + rc = regmap_update_bits(wled->regmap, > + wled->addr + PM8941_WLED_REG_SYNC, > + PM8941_WLED_REG_SYNC_MASK, PM8941_WLED_REG_SYNC_ALL); > + if (rc) > + return rc; > + > + rc = regmap_update_bits(wled->regmap, > + wled->addr + PM8941_WLED_REG_SYNC, > + PM8941_WLED_REG_SYNC_MASK, PM8941_WLED_REG_SYNC_CLEAR); > + return rc; > +} This doesn't seem to do anything for the OVP spike mentioned in this patch[1]. Do you see that problem on your device? I imagine the PMIC is the same. [1] https://www.codeaurora.org/cgit/quic/la/kernel/msm/commit/?id=fef9e15072562f0f28dc7066dcdd69388df81ed3
On Thu, 2015-02-12 at 20:07 -0800, Stephen Boyd wrote: > On 01/29/15 04:48, Ivan T. Ivanov wrote: > > Otherwise it looks good. Driver is loaded and device is detected > > properly (i have added readings for type and subtype registers). > > Do you know where I can measure result from changing brightness > > sysfs entry. I am using 8074 dragonboard? > > Does the backlight turn on? From what I can tell it controls the > backlight, but it may be that nothings getting displayed so it won't be > noticeable. > Yes, I can not see visual changes. That is why I have asked where I could hook the probe and measure current change or wherever. BL_WLEDx signals go out from J2, but somehow I was unable to locate it on the board, will try to look harder :-) Regards, Ivan -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/12/15 20:28, Ivan T. Ivanov wrote: > On Thu, 2015-02-12 at 20:07 -0800, Stephen Boyd wrote: >> On 01/29/15 04:48, Ivan T. Ivanov wrote: >>> Otherwise it looks good. Driver is loaded and device is detected >>> properly (i have added readings for type and subtype registers). >>> Do you know where I can measure result from changing brightness >>> sysfs entry. I am using 8074 dragonboard? >> Does the backlight turn on? From what I can tell it controls the >> backlight, but it may be that nothings getting displayed so it won't be >> noticeable. >> > Yes, I can not see visual changes. That is why I have asked where > I could hook the probe and measure current change or wherever. > BL_WLEDx signals go out from J2, but somehow I was unable to > locate it on the board, will try to look harder :-) > > When the screen is "off" in android I can still turn on the brightness to max in sysfs and see the screen glow. Hopefully we don't need something else to make that work.
On Thu, Feb 12, 2015 at 8:04 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 01/23/15 16:54, Bjorn Andersson wrote: Thanks for the review, Stephen. Bjorn, could you please update your patch according to Stephen's review. -Bryan >> + >> +static int pm8941_wled_configure(struct pm8941_wled *wled, struct device *dev) >> +{ >> + struct pm8941_wled_config *cfg = &wled->cfg; >> + u32 val; >> + int rc; >> + int i; >> + >> + const struct { >> + const char *name; >> + u32 *val_ptr; >> + const struct pm8941_wled_var_cfg *cfg; >> + } u32_opts[] = { >> + { >> + "qcom,current-boost-limit", >> + &cfg->i_boost_limit, >> + .cfg = &pm8941_wled_i_boost_limit_cfg, >> + }, >> + { >> + "qcom,current-limit", >> + &cfg->i_limit, >> + .cfg = &pm8941_wled_i_limit_cfg, >> + }, >> + { >> + "qcom,ovp", >> + &cfg->ovp, >> + .cfg = &pm8941_wled_ovp_cfg, >> + }, >> + { >> + "qcom,switching-freq", >> + &cfg->switch_freq, >> + .cfg = &pm8941_wled_switch_freq_cfg, >> + }, >> + { >> + "qcom,num-strings", >> + &cfg->num_strings, >> + .cfg = &pm8941_wled_num_strings_cfg, >> + }, >> + }; >> + const struct { >> + const char *name; >> + bool *val_ptr; >> + } bool_opts[] = { >> + { "qcom,cs-out", &cfg->cs_out_en, }, >> + { "qcom,ext-gen", &cfg->ext_gen, }, >> + { "qcom,cabc", &cfg->cabc_en, }, >> + }; >> + >> + rc = of_property_read_u32(dev->of_node, "reg", &val); >> + if (rc || val > 0xffff) { >> + dev_err(dev, "invalid IO resources\n"); >> + return rc ? rc : -EINVAL; >> + } >> + wled->addr = val; >> + >> + rc = of_property_read_string(dev->of_node, "label", &wled->cdev.name); >> + if (rc) >> + wled->cdev.name = dev->of_node->name; >> + >> + wled->cdev.default_trigger = of_get_property(dev->of_node, >> + "linux,default-trigger", NULL); >> + >> + *cfg = pm8941_wled_config_defaults; >> + for (i = 0; i < ARRAY_SIZE(u32_opts); ++i) { >> + u32 sel, c; >> + int j, rj; >> + >> + rc = of_property_read_u32(dev->of_node, u32_opts[i].name, &val); >> + if (rc) { >> + if (rc != -EINVAL) { >> + dev_err(dev, "error reading '%s'\n", >> + u32_opts[i].name); >> + return rc; >> + } >> + continue; >> + } >> + >> + sel = UINT_MAX; >> + rj = -1; >> + c = pm8941_wled_values(u32_opts[i].cfg, 0); >> + for (j = 0; c != UINT_MAX; ++j) { >> + if (c <= val && (sel == UINT_MAX || c >= sel)) { >> + sel = c; >> + rj = j; >> + } >> + c = pm8941_wled_values(u32_opts[i].cfg, j + 1); >> + } >> + if (sel == UINT_MAX) { >> + dev_err(dev, "invalid value for '%s'\n", >> + u32_opts[i].name); >> + return rc; > > Isn't rc always 0 here? Don't we want to return an error? > > Also, I find this code very convoluted given that we loop through a > table and match based on nodes and call function pointers, etc. Why > can't we just have a handful of if statements with of_property_read_u32 > in them? That way we don't have to jump through so many hoops, bouncing > all around this file to figure out what's going on. If we did I imagine > we wouldn't have missed out on rc being 0 here. > >> + >> +static int pm8941_wled_remove(struct platform_device *pdev) >> +{ >> + struct pm8941_wled *wled; >> + >> + wled = platform_get_drvdata(pdev); >> + led_classdev_unregister(&wled->cdev); > > Would be nice to have a devm for this one too. > >> + >> + return 0; >> +} >> + >> +static const struct of_device_id pm8941_wled_match_table[] = { >> + { .compatible = "qcom,pm8941-wled" }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, pm8941_wled_match_table); >> + >> +static struct platform_driver pm8941_wled_driver = { >> + .probe = pm8941_wled_probe, >> + .remove = pm8941_wled_remove, >> + .driver = { >> + .name = "pm8941-wled", >> + .owner = THIS_MODULE, > > THIS_MODULE should be removed. > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 17, 2015 at 2:14 PM, Bryan Wu <cooloney@gmail.com> wrote: > On Thu, Feb 12, 2015 at 8:04 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >> On 01/23/15 16:54, Bjorn Andersson wrote: > > Thanks for the review, Stephen. > Bjorn, could you please update your patch according to Stephen's review. > I will do so, do you want me to send a new patch or an incremental patch with the changes? Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 17, 2015 at 2:30 PM, Bjorn Andersson <bjorn@kryo.se> wrote: > On Tue, Feb 17, 2015 at 2:14 PM, Bryan Wu <cooloney@gmail.com> wrote: >> On Thu, Feb 12, 2015 at 8:04 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >>> On 01/23/15 16:54, Bjorn Andersson wrote: >> >> Thanks for the review, Stephen. >> Bjorn, could you please update your patch according to Stephen's review. >> > > I will do so, do you want me to send a new patch or an incremental > patch with the changes? > Please send me a new one. -Bryan -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 12, 2015 at 8:26 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 01/23/15 16:54, Bjorn Andersson wrote: >> + >> +static int pm8941_wled_set(struct led_classdev *cdev, >> + enum led_brightness value) >> +{ >> + struct pm8941_wled *wled; >> + u8 ctrl = 0; >> + u16 val; >> + int rc; >> + int i; >> + >> + wled = container_of(cdev, struct pm8941_wled, cdev); >> + >> + if (value != 0) >> + ctrl = PM8941_WLED_REG_MOD_EN_BIT; >> + >> + val = value * PM8941_WLED_REG_VAL_MAX / LED_FULL; >> + >> + rc = regmap_update_bits(wled->regmap, >> + wled->addr + PM8941_WLED_REG_MOD_EN, >> + PM8941_WLED_REG_MOD_EN_MASK, ctrl); >> + if (rc) >> + return rc; >> + >> + for (i = 0; i < wled->cfg.num_strings; ++i) { >> + u8 v[2] = { val & 0xff, (val >> 8) & 0xf }; >> + >> + rc = regmap_bulk_write(wled->regmap, >> + wled->addr + PM8941_WLED_REG_VAL_BASE + 2 * i, >> + v, 2); >> + if (rc) >> + return rc; >> + } >> + >> + rc = regmap_update_bits(wled->regmap, >> + wled->addr + PM8941_WLED_REG_SYNC, >> + PM8941_WLED_REG_SYNC_MASK, PM8941_WLED_REG_SYNC_ALL); >> + if (rc) >> + return rc; >> + >> + rc = regmap_update_bits(wled->regmap, >> + wled->addr + PM8941_WLED_REG_SYNC, >> + PM8941_WLED_REG_SYNC_MASK, PM8941_WLED_REG_SYNC_CLEAR); >> + return rc; >> +} > > This doesn't seem to do anything for the OVP spike mentioned in this > patch[1]. Do you see that problem on your device? I imagine the PMIC is > the same. > > [1] > https://www.codeaurora.org/cgit/quic/la/kernel/msm/commit/?id=fef9e15072562f0f28dc7066dcdd69388df81ed3 > That's odd, I can't find that fix in any officially supported releases for 8974 - which Courtney used as reference for this driver. Just to make sure I understand the solution; when disabling the sinks the over-voltage-protection sometimes triggers, so we should detect that and reset the ovp configuration? I presume the side effect would be that the sinks would not give any output until this is done? Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 12, 2015 at 8:07 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 01/29/15 04:48, Ivan T. Ivanov wrote: >> >> Otherwise it looks good. Driver is loaded and device is detected >> properly (i have added readings for type and subtype registers). >> Do you know where I can measure result from changing brightness >> sysfs entry. I am using 8074 dragonboard? > > Does the backlight turn on? From what I can tell it controls the > backlight, but it may be that nothings getting displayed so it won't be > noticeable. > I have not measured the voltage/current, nor have had something on the display. But on the various devices I've tested (Xperia Z1 & Xperia Z Ultra) the backlight is clearly on. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 12, 2015 at 8:04 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: [..] >> + >> +static int pm8941_wled_remove(struct platform_device *pdev) >> +{ >> + struct pm8941_wled *wled; >> + >> + wled = platform_get_drvdata(pdev); >> + led_classdev_unregister(&wled->cdev); > > Would be nice to have a devm for this one too. > I agree, will hack one up. >> + >> + return 0; >> +} >> + >> +static const struct of_device_id pm8941_wled_match_table[] = { >> + { .compatible = "qcom,pm8941-wled" }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, pm8941_wled_match_table); >> + >> +static struct platform_driver pm8941_wled_driver = { >> + .probe = pm8941_wled_probe, >> + .remove = pm8941_wled_remove, >> + .driver = { >> + .name = "pm8941-wled", >> + .owner = THIS_MODULE, > > THIS_MODULE should be removed. > Right, we've had this patch in purgatory for too long time... Will update. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/17/15 15:02, Bjorn Andersson wrote: > On Thu, Feb 12, 2015 at 8:26 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >> On 01/23/15 16:54, Bjorn Andersson wrote: >>> + >>> +static int pm8941_wled_set(struct led_classdev *cdev, >>> + enum led_brightness value) >>> +{ >>> + struct pm8941_wled *wled; >>> + u8 ctrl = 0; >>> + u16 val; >>> + int rc; >>> + int i; >>> + >>> + wled = container_of(cdev, struct pm8941_wled, cdev); >>> + >>> + if (value != 0) >>> + ctrl = PM8941_WLED_REG_MOD_EN_BIT; >>> + >>> + val = value * PM8941_WLED_REG_VAL_MAX / LED_FULL; >>> + >>> + rc = regmap_update_bits(wled->regmap, >>> + wled->addr + PM8941_WLED_REG_MOD_EN, >>> + PM8941_WLED_REG_MOD_EN_MASK, ctrl); >>> + if (rc) >>> + return rc; >>> + >>> + for (i = 0; i < wled->cfg.num_strings; ++i) { >>> + u8 v[2] = { val & 0xff, (val >> 8) & 0xf }; >>> + >>> + rc = regmap_bulk_write(wled->regmap, >>> + wled->addr + PM8941_WLED_REG_VAL_BASE + 2 * i, >>> + v, 2); >>> + if (rc) >>> + return rc; >>> + } >>> + >>> + rc = regmap_update_bits(wled->regmap, >>> + wled->addr + PM8941_WLED_REG_SYNC, >>> + PM8941_WLED_REG_SYNC_MASK, PM8941_WLED_REG_SYNC_ALL); >>> + if (rc) >>> + return rc; >>> + >>> + rc = regmap_update_bits(wled->regmap, >>> + wled->addr + PM8941_WLED_REG_SYNC, >>> + PM8941_WLED_REG_SYNC_MASK, PM8941_WLED_REG_SYNC_CLEAR); >>> + return rc; >>> +} >> This doesn't seem to do anything for the OVP spike mentioned in this >> patch[1]. Do you see that problem on your device? I imagine the PMIC is >> the same. >> >> [1] >> https://www.codeaurora.org/cgit/quic/la/kernel/msm/commit/?id=fef9e15072562f0f28dc7066dcdd69388df81ed3 >> > That's odd, I can't find that fix in any officially supported releases > for 8974 - which Courtney used as reference for this driver. > > Just to make sure I understand the solution; when disabling the sinks > the over-voltage-protection sometimes triggers, so we should detect > that and reset the ovp configuration? > > I presume the side effect would be that the sinks would not give any > output until this is done? > It seems that commit is not very good at describing the problem. From what I can tell, the inductor current may spike when the WLED is turned off and the switch is stuck on (the circuit is a boost convertor). To make sure this doesn't happen, we force an OVP so that the switch is known to be open (i.e. off) and thus can't cause the current spike when we disable.
On Tue, Feb 17, 2015 at 6:45 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 02/17/15 15:02, Bjorn Andersson wrote: >> On Thu, Feb 12, 2015 at 8:26 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: [..] >>> This doesn't seem to do anything for the OVP spike mentioned in this >>> patch[1]. Do you see that problem on your device? I imagine the PMIC is >>> the same. >>> >>> [1] >>> https://www.codeaurora.org/cgit/quic/la/kernel/msm/commit/?id=fef9e15072562f0f28dc7066dcdd69388df81ed3 >>> >> That's odd, I can't find that fix in any officially supported releases >> for 8974 - which Courtney used as reference for this driver. >> >> Just to make sure I understand the solution; when disabling the sinks >> the over-voltage-protection sometimes triggers, so we should detect >> that and reset the ovp configuration? >> >> I presume the side effect would be that the sinks would not give any >> output until this is done? >> > > It seems that commit is not very good at describing the problem. From > what I can tell, the inductor current may spike when the WLED is turned > off and the switch is stuck on (the circuit is a boost convertor). To > make sure this doesn't happen, we force an OVP so that the switch is > known to be open (i.e. off) and thus can't cause the current spike when > we disable. > Apparently that commit is only half of the solution, there's another commit earlier in history with the same commit message [2]. With both [1] and [2] in place there are a bunch of different operations going on, so I'm not able to fully map these to your description - which does make sense. [2] https://www.codeaurora.org/cgit/quic/la/kernel/msm/commit/?id=b1a2f61113c69d2aba68c583101ac40a05cb2c5b Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/leds/leds-pm8941-wled.txt b/Documentation/devicetree/bindings/leds/leds-pm8941-wled.txt new file mode 100644 index 0000000..a85a964 --- /dev/null +++ b/Documentation/devicetree/bindings/leds/leds-pm8941-wled.txt @@ -0,0 +1,43 @@ +Binding for Qualcomm PM8941 WLED driver + +Required properties: +- compatible: should be "qcom,pm8941-wled" +- reg: slave address + +Optional properties: +- label: The label for this led + See Documentation/devicetree/bindings/leds/common.txt +- linux,default-trigger: Default trigger assigned to the LED + See Documentation/devicetree/bindings/leds/common.txt +- qcom,cs-out: bool; enable current sink output +- qcom,cabc: bool; enable content adaptive backlight control +- qcom,ext-gen: bool; use externally generated modulator signal to dim +- qcom,current-limit: mA; per-string current limit; value from 0 to 25 + default: 20mA +- qcom,current-boost-limit: mA; boost current limit; one of: + 105, 385, 525, 805, 980, 1260, 1400, 1680 + default: 805mA +- qcom,switching-freq: kHz; switching frequency; one of: + 600, 640, 685, 738, 800, 872, 960, 1066, 1200, 1371, + 1600, 1920, 2400, 3200, 4800, 9600, + default: 1600kHz +- qcom,ovp: V; Over-voltage protection limit; one of: + 27, 29, 32, 35 + default: 29V +- qcom,num-strings: #; number of led strings attached; value from 1 to 3 + default: 2 + +Example: + +pm8941-wled@d800 { + compatible = "qcom,pm8941-wled"; + reg = <0xd800>; + label = "backlight"; + + qcom,cs-out; + qcom,current-limit = <20>; + qcom,current-boost-limit = <805>; + qcom,switching-freq = <1600>; + qcom,ovp = <29>; + qcom,num-strings = <2>; +};