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.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On Thu, Feb 12, 2015 at 8:04 PM, Stephen Boyd <[email protected]> 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
>
On Tue, Feb 17, 2015 at 2:14 PM, Bryan Wu <[email protected]> wrote:
> On Thu, Feb 12, 2015 at 8:04 PM, Stephen Boyd <[email protected]> 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
On Tue, Feb 17, 2015 at 2:30 PM, Bjorn Andersson <[email protected]> wrote:
> On Tue, Feb 17, 2015 at 2:14 PM, Bryan Wu <[email protected]> wrote:
>> On Thu, Feb 12, 2015 at 8:04 PM, Stephen Boyd <[email protected]> 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
On Thu, Feb 12, 2015 at 8:04 PM, Stephen Boyd <[email protected]> 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