| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 16 Dec 2014 17:15:22 -0800
From: Stephen Boyd <sboyd@...eaurora.org>
To: Bjorn Andersson <bjorn.andersson@...ymobile.com>,
Rob Herring <robh+dt@...nel.org>,
Pawel Moll <pawel.moll@....com>,
Mark Rutland <mark.rutland@....com>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>,
Bryan Wu <cooloney@...il.com>,
Richard Purdie <rpurdie@...ys.net>,
Grant Likely <grant.likely@...aro.org>
CC: Courtney Cavin <courtney.cavin@...ymobile.com>,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-leds@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH 2/2] leds: add Qualcomm PM8941 WLED driver
On 12/08/2014 04:22 PM, Bjorn Andersson wrote:
> diff --git a/drivers/leds/leds-pm8941-wled.c b/drivers/leds/leds-pm8941-wled.c
> new file mode 100644
> index 0000000..aa06d0b
> --- /dev/null
> +++ b/drivers/leds/leds-pm8941-wled.c
> @@ -0,0 +1,466 @@
> +/* Copyright (c) 2013, Sony Mobile Communications, AB.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
Missing <linux/kernel.h> for container_of() macro.
> +
> +enum pm8941_wled_reg {
> + PM8941_WLED_REG_VAL_BASE = 0x40,
> +#define PM8941_WLED_REG_VAL_MAX 0xFFF
> +
> + PM8941_WLED_REG_MOD_EN = 0x46,
> +#define PM8941_WLED_REG_MOD_EN_BIT BIT(7)
> +#define PM8941_WLED_REG_MOD_EN_MASK BIT(7)
> +
> + PM8941_WLED_REG_SYNC = 0x47,
> +#define PM8941_WLED_REG_SYNC_MASK 0x07
> +#define PM8941_WLED_REG_SYNC_LED1 BIT(0)
> +#define PM8941_WLED_REG_SYNC_LED2 BIT(1)
> +#define PM8941_WLED_REG_SYNC_LED3 BIT(2)
> +#define PM8941_WLED_REG_SYNC_ALL 0x07
> +#define PM8941_WLED_REG_SYNC_CLEAR 0x00
> +
> + PM8941_WLED_REG_FREQ = 0x4c,
> +#define PM8941_WLED_REG_FREQ_MASK 0x0f
> +
> + PM8941_WLED_REG_OVP = 0x4d,
> +#define PM8941_WLED_REG_OVP_MASK 0x03
> +
> + PM8941_WLED_REG_BOOST = 0x4e,
> +#define PM8941_WLED_REG_BOOST_MASK 0x07
> +
> + PM8941_WLED_REG_SINK = 0x4f,
> +#define PM8941_WLED_REG_SINK_MASK 0xe0
> +#define PM8941_WLED_REG_SINK_SHFT 0x05
> +
> +/* Per-'string' registers below */
> +#define PM8941_WLED_REG_STR_OFFSET 0x10
> +
> + PM8941_WLED_REG_STR_MOD_EN_BASE = 0x60,
> +#define PM8941_WLED_REG_STR_MOD_EN BIT(7)
> +#define PM8941_WLED_REG_STR_MOD_MASK BIT(7)
> +
> + PM8941_WLED_REG_STR_SCALE_BASE = 0x62,
> +#define PM8941_WLED_REG_STR_SCALE_MASK 0x1f
> +
> + PM8941_WLED_REG_STR_MOD_SRC_BASE = 0x63,
> +#define PM8941_WLED_REG_STR_MOD_SRC_MASK 0x01
> +#define PM8941_WLED_REG_STR_MOD_SRC_INT 0x00
> +#define PM8941_WLED_REG_STR_MOD_SRC_EXT 0x01
> +
> + PM8941_WLED_REG_STR_CABC_BASE = 0x66,
> +#define PM8941_WLED_REG_STR_CABC_MASK BIT(7)
> +#define PM8941_WLED_REG_STR_CABC_EN BIT(7)
> +};
I don't see any value in this enum. It's never used in a switch
statement so why not just have #defines for the register offsets. It
would make reading this a lot easier too.
> +
> +static int pm8941_wled_set(struct led_classdev *cdev,
> + enum led_brightness value)
> +{
> + struct pm8941_wled_context *ctx;
> + struct pm8941_wled *wled;
> + u8 ctrl = 0;
> + u16 val;
> + int rc;
> + int i;
> +
> + wled = container_of(cdev, struct pm8941_wled, cdev);
> + ctx = container_of(wled, struct pm8941_wled_context, wled);
> +
> + if (value != 0)
> + ctrl = PM8941_WLED_REG_MOD_EN_BIT;
> +
> + val = (value * PM8941_WLED_REG_VAL_MAX) / LED_FULL;
Unnecessary parens here.
> +
> + rc = regmap_update_bits(ctx->regmap,
>
> +
> +static int pm8941_wled_configure(struct pm8941_wled *wled, struct device *dev)
> +{
[...]
> +
> + if (dev->of_node == NULL) {
> + dev_err(dev, "no OF node!\n");
> + return -EINVAL;
> + }
Seems like an unnecessary check. Everything's DT so far.
> +
> + 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;
> + }
> + if (sel != val) {
> + dev_warn(dev, "rounding '%s' down to %u\n",
> + u32_opts[i].name, sel);
> + }
Do we really want to do all this complicated rounding and checking?
Can't we just assume the DT is correct?
> + dev_dbg(dev, "'%s' = %u\n", u32_opts[i].name, sel);
> + *u32_opts[i].val_ptr = rj;
> + };
> +
> + for (i = 0; i < ARRAY_SIZE(bool_opts); ++i) {
> + if (of_property_read_bool(dev->of_node, bool_opts[i].name))
> + *bool_opts[i].val_ptr = true;
> + }
> +
> + cfg->num_strings = cfg->num_strings + 1;
> +
> + return 0;
> +}
> +
> +static int pm8941_wled_probe(struct platform_device *pdev)
> +{
> + struct pm8941_wled_context *ctx;
> + struct regmap *regmap;
> + int rc;
> +
> + regmap = dev_get_regmap(pdev->dev.parent, NULL);
> + if (!regmap) {
> + dev_err(&pdev->dev, "Unable to get regmap\n");
> + return -EINVAL;
> + }
> +
> + ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
> + if (ctx == NULL)
Nitpick only because it doesn't match the !regmap style above. Please
use the !ctx style here as well.
> + return -ENOMEM;
> +
> + ctx->regmap = regmap;
> +
> + rc = pm8941_wled_configure(&ctx->wled, &pdev->dev);
> + if (rc)
> + return rc;
> +
> + rc = pm8941_wled_setup(&ctx->wled);
> + if (rc)
> + return rc;
> +
> + ctx->wled.cdev.brightness_set = pm8941_wled_set_brightness;
> +
> + rc = led_classdev_register(&pdev->dev, &ctx->wled.cdev);
> + if (rc)
> + return rc;
> +
> + platform_set_drvdata(pdev, ctx);
> + dev_info(&pdev->dev, "registered led \"%s\"\n", ctx->wled.cdev.name);
Please don't spam the log with "I'm alive!" messages.
> +
> + return 0;
> +};
> +
> +static int pm8941_wled_remove(struct platform_device *pdev)
> +{
> + struct pm8941_wled_context *ctx;
> +
> + ctx = platform_get_drvdata(pdev);
> + led_classdev_unregister(&ctx->wled.cdev);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id pm8941_wled_match_table[] = {
> + { .compatible = "qcom,pm8941-wled" },
> + {}
> +};
MODULE_DEVICE_TABLE?
> +
> +static struct platform_driver pm8941_wled_driver = {
> + .probe = pm8941_wled_probe,
> + .remove = pm8941_wled_remove,
> + .driver = {
> + .name = "pm8941-wled",
> + .owner = THIS_MODULE,
> + .of_match_table = pm8941_wled_match_table,
> + },
> +};
> +
> +module_platform_driver(pm8941_wled_driver);
> +
> +MODULE_DESCRIPTION("pm8941 wled driver");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform: pm8941-wled");
--
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 majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists