LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Bryan Wu <cooloney@gmail.com>
To: Stephen Boyd <sboyd@codeaurora.org>
Cc: Bjorn Andersson <bjorn.andersson@sonymobile.com>,
	Rob Herring <robh+dt@kernel.org>, Pawel Moll <pawel.moll@arm.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Ian Campbell <ijc+devicetree@hellion.org.uk>,
	Kumar Gala <galak@codeaurora.org>,
	Richard Purdie <rpurdie@rpsys.net>,
	Grant Likely <grant.likely@linaro.org>,
	Courtney Cavin <courtney.cavin@sonymobile.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	lkml <linux-kernel@vger.kernel.org>,
	Linux LED Subsystem <linux-leds@vger.kernel.org>,
	linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver
Date: Tue, 17 Feb 2015 14:14:19 -0800	[thread overview]
Message-ID: <CAK5ve-KJK_aT_8sVmhtnTTJRZpdAvSGc45dtH-XH-E59wB=jXQ@mail.gmail.com> (raw)
In-Reply-To: <54DD77BA.5020808@codeaurora.org>

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
>

  reply	other threads:[~2015-02-17 22:14 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-01-24  0:54 [PATCH v2 1/2] leds: add DT binding for Qualcomm PM8941 WLED block Bjorn Andersson
2015-01-24  0:54 ` [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver Bjorn Andersson
2015-01-24  1:22   ` Bjorn Andersson
2015-01-29 12:48   ` Ivan T. Ivanov
2015-01-29 19:07     ` Bjorn Andersson
2015-02-12 19:20       ` Bryan Wu
2015-02-13  4:07     ` Stephen Boyd
2015-02-13  4:28       ` Ivan T. Ivanov
2015-02-13  4:34         ` Stephen Boyd
2015-02-17 23:05       ` Bjorn Andersson
2015-02-13  4:04   ` Stephen Boyd
2015-02-17 22:14     ` Bryan Wu [this message]
2015-02-17 22:30       ` Bjorn Andersson
2015-02-17 22:32         ` Bryan Wu
2015-02-17 23:13     ` Bjorn Andersson
2015-02-13  4:26   ` Stephen Boyd
2015-02-17 23:02     ` Bjorn Andersson
2015-02-18  2:45       ` Stephen Boyd
2015-02-18 14:54         ` Bjorn Andersson
2015-02-12 19:11 ` [PATCH v2 1/2] leds: add DT binding for Qualcomm PM8941 WLED block Bryan Wu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAK5ve-KJK_aT_8sVmhtnTTJRZpdAvSGc45dtH-XH-E59wB=jXQ@mail.gmail.com' \
    --to=cooloney@gmail.com \
    --cc=bjorn.andersson@sonymobile.com \
    --cc=courtney.cavin@sonymobile.com \
    --cc=devicetree@vger.kernel.org \
    --cc=galak@codeaurora.org \
    --cc=grant.likely@linaro.org \
    --cc=ijc+devicetree@hellion.org.uk \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-leds@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=pawel.moll@arm.com \
    --cc=robh+dt@kernel.org \
    --cc=rpurdie@rpsys.net \
    --cc=sboyd@codeaurora.org \
    --subject='Re: [PATCH v2 2/2] leds: add Qualcomm PM8941 WLED driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).