Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
On Tue, Feb 17, 2015 at 6:45 PM, Stephen Boyd wrote: > On 02/17/15 15:02, Bjorn Andersson wrote: >> On Thu, Feb 12, 2015 at 8:26 PM, Stephen Boyd 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 linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
On Tue, Feb 17, 2015 at 6:45 PM, Stephen Boyd sb...@codeaurora.org wrote: On 02/17/15 15:02, Bjorn Andersson wrote: On Thu, Feb 12, 2015 at 8:26 PM, Stephen Boyd sb...@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 linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
On 02/17/15 15:02, Bjorn Andersson wrote:
> On Thu, Feb 12, 2015 at 8:26 PM, Stephen Boyd 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.
--
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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
On Thu, Feb 12, 2015 at 8:04 PM, Stephen Boyd wrote:
[..]
>> +
>> +static int pm8941_wled_remove(struct platform_device *pdev)
>> +{
>> + struct pm8941_wled *wled;
>> +
>> + wled = platform_get_drvdata(pdev);
>> + led_classdev_unregister(>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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
On Thu, Feb 12, 2015 at 8:07 PM, 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. > 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 linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
On Thu, Feb 12, 2015 at 8:26 PM, Stephen Boyd 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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
On Tue, Feb 17, 2015 at 2:30 PM, Bjorn Andersson wrote: > On Tue, Feb 17, 2015 at 2:14 PM, Bryan Wu wrote: >> On Thu, Feb 12, 2015 at 8:04 PM, Stephen Boyd 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 linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
On Tue, Feb 17, 2015 at 2:14 PM, Bryan Wu wrote: > On Thu, Feb 12, 2015 at 8:04 PM, Stephen Boyd 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 linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
On Thu, Feb 12, 2015 at 8:04 PM, Stephen Boyd 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 = >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",
>> + >i_boost_limit,
>> + .cfg = _wled_i_boost_limit_cfg,
>> + },
>> + {
>> + "qcom,current-limit",
>> + >i_limit,
>> + .cfg = _wled_i_limit_cfg,
>> + },
>> + {
>> + "qcom,ovp",
>> + >ovp,
>> + .cfg = _wled_ovp_cfg,
>> + },
>> + {
>> + "qcom,switching-freq",
>> + >switch_freq,
>> + .cfg = _wled_switch_freq_cfg,
>> + },
>> + {
>> + "qcom,num-strings",
>> + >num_strings,
>> + .cfg = _wled_num_strings_cfg,
>> + },
>> + };
>> + const struct {
>> + const char *name;
>> + bool *val_ptr;
>> + } bool_opts[] = {
>> + { "qcom,cs-out", >cs_out_en, },
>> + { "qcom,ext-gen", >ext_gen, },
>> + { "qcom,cabc", >cabc_en, },
>> + };
>> +
>> + rc = of_property_read_u32(dev->of_node, "reg", );
>> + if (rc || val > 0x) {
>> + dev_err(dev, "invalid IO resources\n");
>> + return rc ? rc : -EINVAL;
>> + }
>> + wled->addr = val;
>> +
>> + rc = of_property_read_string(dev->of_node, "label", >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,
>> );
>> + 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(>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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
On Thu, Feb 12, 2015 at 8:07 PM, Stephen Boyd sb...@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 linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
On 02/17/15 15:02, Bjorn Andersson wrote:
On Thu, Feb 12, 2015 at 8:26 PM, Stephen Boyd sb...@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.
--
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 linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
On Thu, Feb 12, 2015 at 8:26 PM, Stephen Boyd sb...@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 linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
On Thu, Feb 12, 2015 at 8:04 PM, Stephen Boyd sb...@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 linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
On Thu, Feb 12, 2015 at 8:04 PM, Stephen Boyd sb...@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 0x) {
+ 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 linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
On Tue, Feb 17, 2015 at 2:14 PM, Bryan Wu coolo...@gmail.com wrote: On Thu, Feb 12, 2015 at 8:04 PM, Stephen Boyd sb...@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 linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
On Tue, Feb 17, 2015 at 2:30 PM, Bjorn Andersson bj...@kryo.se wrote: On Tue, Feb 17, 2015 at 2:14 PM, Bryan Wu coolo...@gmail.com wrote: On Thu, Feb 12, 2015 at 8:04 PM, Stephen Boyd sb...@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 linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
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. -- 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 linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
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 linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
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
--
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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
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. -- 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 linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
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 = >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",
> + >i_boost_limit,
> + .cfg = _wled_i_boost_limit_cfg,
> + },
> + {
> + "qcom,current-limit",
> + >i_limit,
> + .cfg = _wled_i_limit_cfg,
> + },
> + {
> + "qcom,ovp",
> + >ovp,
> + .cfg = _wled_ovp_cfg,
> + },
> + {
> + "qcom,switching-freq",
> + >switch_freq,
> + .cfg = _wled_switch_freq_cfg,
> + },
> + {
> + "qcom,num-strings",
> + >num_strings,
> + .cfg = _wled_num_strings_cfg,
> + },
> + };
> + const struct {
> + const char *name;
> + bool *val_ptr;
> + } bool_opts[] = {
> + { "qcom,cs-out", >cs_out_en, },
> + { "qcom,ext-gen", >ext_gen, },
> + { "qcom,cabc", >cabc_en, },
> + };
> +
> + rc = of_property_read_u32(dev->of_node, "reg", );
> + if (rc || val > 0x) {
> + dev_err(dev, "invalid IO resources\n");
> + return rc ? rc : -EINVAL;
> + }
> + wled->addr = val;
> +
> + rc = of_property_read_string(dev->of_node, "label", >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, );
> + 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(>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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
On Thu, Jan 29, 2015 at 11:07 AM, Bjorn Andersson
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 ca...@sonymobile.com>
>> >
>> > This adds support for the WLED ('White' LED) block on Qualcomm's
>> > PM8941 PMICs.
>> >
>> > Signed-off-by: Courtney Cavin ca...@sonymobile.com>
>> > Signed-off-by: Bjorn Andersson anders...@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
>> >
>>
>>
>>
>> > +
>> > +#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_BITBIT(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;
>> > +};
>> > +
>> >
>>
>>
>>
>> > +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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
On Thu, Jan 29, 2015 at 11:07 AM, Bjorn Andersson
bjorn.anders...@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 ca...@sonymobile.com
This adds support for the WLED ('White' LED) block on Qualcomm's
PM8941 PMICs.
Signed-off-by: Courtney Cavin ca...@sonymobile.com
Signed-off-by: Bjorn Andersson anders...@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_BITBIT(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 linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
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
--
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 linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
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. -- 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 linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
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. -- 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 linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
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 linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
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 0x) {
+ 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 linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
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 ca...@sonymobile.com>
> >
> > This adds support for the WLED ('White' LED) block on Qualcomm's
> > PM8941 PMICs.
> >
> > Signed-off-by: Courtney Cavin ca...@sonymobile.com>
> > Signed-off-by: Bjorn Andersson anders...@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
> >
>
>
>
> > +
> > +#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_BITBIT(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;
> > +};
> > +
> >
>
>
>
> > +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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
Hi Bjorn,
Just few nitpick comments.
On Fri, 2015-01-23 at 16:54 -0800, Bjorn Andersson wrote:
> From: Courtney Cavin ca...@sonymobile.com>
>
> This adds support for the WLED ('White' LED) block on Qualcomm's
> PM8941 PMICs.
>
> Signed-off-by: Courtney Cavin ca...@sonymobile.com>
> Signed-off-by: Bjorn Andersson anders...@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
>
> +
> +#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_BITBIT(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;
> +};
> +
>
> +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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
Hi Bjorn,
Just few nitpick comments.
On Fri, 2015-01-23 at 16:54 -0800, Bjorn Andersson wrote:
From: Courtney Cavin ca...@sonymobile.com
This adds support for the WLED ('White' LED) block on Qualcomm's
PM8941 PMICs.
Signed-off-by: Courtney Cavin ca...@sonymobile.com
Signed-off-by: Bjorn Andersson anders...@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_BITBIT(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 linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
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 ca...@sonymobile.com
This adds support for the WLED ('White' LED) block on Qualcomm's
PM8941 PMICs.
Signed-off-by: Courtney Cavin ca...@sonymobile.com
Signed-off-by: Bjorn Andersson anders...@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_BITBIT(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 linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
On Fri, Jan 23, 2015 at 4:54 PM, Bjorn Andersson
wrote:
> From: Courtney Cavin
>
> This adds support for the WLED ('White' LED) block on Qualcomm's
> PM8941 PMICs.
>
> Signed-off-by: Courtney Cavin
> Signed-off-by: Bjorn Andersson
> ---
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 linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
On Fri, Jan 23, 2015 at 4:54 PM, Bjorn Andersson
bjorn.anders...@sonymobile.com wrote:
From: Courtney Cavin courtney.ca...@sonymobile.com
This adds support for the WLED ('White' LED) block on Qualcomm's
PM8941 PMICs.
Signed-off-by: Courtney Cavin courtney.ca...@sonymobile.com
Signed-off-by: Bjorn Andersson bjorn.anders...@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 linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
