Re: [PATCH 2/2] leds: add Qualcomm PM8941 WLED driver

[Stephen Boyd]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/