Messages in this thread | | | From | Bjorn <> | Date | Thu, 18 Dec 2014 15:43:53 -0800 | Subject | Re: [PATCH 2/2] leds: add Qualcomm PM8941 WLED driver |
| |
On Tue 16 Dec 16:54 PST 2014, Bryan Wu wrote:
> On Mon, Dec 8, 2014 at 4:22 PM, Bjorn Andersson
[..]
> > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
[..]
> > +config LEDS_PM8941_WLED > > + tristate "LED support for the Qualcomm PM8941 WLED block" > > + depends on LEDS_CLASS > > + depends on REGMAP > Here should be "select REGMAP" >
Thanks > > + help > > + This option enables support for the 'White' LED block > > + on Qualcomm PM8941 PMICs. > > +
[..]
> > diff --git a/drivers/leds/leds-pm8941-wled.c b/drivers/leds/leds-pm8941-wled.c
[..]
> > + > > +struct pm8941_wled_context { > > + struct pm8941_wled wled; > > + struct regmap *regmap; > > +}; > > + > > I think you can just add *regmap into pm8941_wled struct and > pm8941_wled_context is redundant to me. >
Looks like a development leftover, I will fold it in.
[..]
> > + > > +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; > > + } > > + > > Is this regmap mmio registers or i2c registers? Will the regmap > operation sleep? >
It's a spmi regmap, in it's current configuration it's marked as "fast" i.e. it will not sleep.
I discussed this with Courtney before sending the patch out and as this is used for backlight we don't see a reason calling set_brightness() in atomic context. If this changes I would send out a follow up patch.
> > + ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL); > > + if (ctx == NULL) > > + 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); > > + > > + return 0; > > +}; > > +
[..]
> > +MODULE_DESCRIPTION("pm8941 wled driver"); > > +MODULE_LICENSE("GPL v2"); > > +MODULE_ALIAS("platform: pm8941-wled"); > No space after ":" >
Oops.
Regards, Bjorn
| |