Re: [PATCH] Input: add regulator haptic driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 1607 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]