lkml.org 
[lkml]   [2013]   [Oct]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] Input: add regulator haptic driver
On Fri, Oct 11, 2013 at 11:22:00AM +0900, hyunhee.kim wrote:

> + if (enable && !haptic->enabled) {
> + haptic->enabled = true;
> + ret = regulator_enable(haptic->regulator);
> + if (ret)
> + pr_err("haptic: %s failed to enable regulator\n",
> + __func__);

These should probably set the flag after the regulator API call has
succeeded, otherwise if the call fails the driver will incorrectly
remember that the regulator is enabled and never disable it (or vice
versa on disable).

> +static void regulator_haptic_work(struct work_struct *work)
> +{
> + struct regulator_haptic *haptic = container_of(work,
> + struct
> regulator_haptic,
> + work);
> + if (haptic->level)
> + regulator_haptic_enable(haptic, true);
> + else
> + regulator_haptic_enable(haptic, false);
> +
> +}

Should the mutex for the level be at this level rather than in the
subfunctions? Though it'd probably be just as well to inline the true
and false cases since there's basically two equivalent forks in the
enable function anyway.

> +
> + haptic = kzalloc(sizeof(*haptic), GFP_KERNEL);
> + if (!haptic) {
> + dev_err(&pdev->dev, "unable to allocate memory for
> haptic\n");
> + return -ENOMEM;
> + }

Might be better to use devm_ functions throughout here, saves error
handling and cleanup code...

> +static int regulator_haptic_remove(struct platform_device *pdev)
> +{
> + struct regulator_haptic *haptic = platform_get_drvdata(pdev);
> +
> + input_unregister_device(haptic->input_dev);
> +
> + return 0;

...which is currently missing.
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2013-10-24 16:01    [W:0.097 / U:0.980 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site