linux-kernel - Re: [PATCH] Input: add regulator haptic driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20131024133836.GG18506@sirena.org.uk>
Date:	Thu, 24 Oct 2013 14:38:36 +0100
From:	Mark Brown <broonie@...nel.org>
To:	"hyunhee.kim" <hyunhee.kim@...sung.com>
Cc:	dmitry.torokhov@...il.com, peter.ujfalusi@...com,
	wfp5p@...ginia.edu, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org
Subject: Re: [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.

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)