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

[Bjorn]

On Tue 16 Dec 17:15 PST 2014, Stephen Boyd wrote:

> 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

[..]

> > +#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.
> 

Thanks

> > +
> > +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.
> 

Make sense, I'll rework it.

> > +
> > +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.
> 

Agreed

> > +
> > +	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.
> 

Agreed.

> > +
> > +	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?
> 

We could remove the printout about rounding, but the logic turns human-readable
values into register values (indices), so we need something for that.

> > +		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.
> 

Agreed

> > +		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.
> 

Ok

> > +
> > +	return 0;
> > +};
> > +

[..]

> > +
> > +static const struct of_device_id pm8941_wled_match_table[] = {
> > +	{ .compatible = "qcom,pm8941-wled" },
> > +	{}
> > +};
> 
> MODULE_DEVICE_TABLE?
> 

Indeed

Thanks for the review Stephen.

Regards,
Bjorn
--
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/