linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Brian Masney <masneyb@onstation.org>
Cc: andy.gross@linaro.org, david.brown@linaro.org,
	robh+dt@kernel.org, mark.rutland@arm.com,
	linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-msm@vger.kernel.org, linux-soc@vger.kernel.org,
	devicetree@vger.kernel.org, jonathan@marek.ca
Subject: Re: [PATCH v3 2/3] Input: add new vibrator driver for various MSM SOCs
Date: Tue, 5 Feb 2019 11:42:46 -0800	[thread overview]
Message-ID: <20190205194246.GA19151@dtor-ws> (raw)
In-Reply-To: <20181025012937.2154-3-masneyb@onstation.org>

Hi Brian,

On Wed, Oct 24, 2018 at 09:29:36PM -0400, Brian Masney wrote:
> This patch adds a new vibrator driver that supports various Qualcomm
> MSM SOCs. Driver was tested on a LG Nexus 5 (hammerhead) phone.
> 

...

> +
> +#define msm_vibrator_write(msm_vibrator, offset, value) \
> +	writel((value), (void __iomem *)((msm_vibrator)->base + (offset)))

Make in a function? It will be inlined anyways...

> +
> +	vibrator->vcc = devm_regulator_get(&pdev->dev, "vcc");
> +	if (IS_ERR(vibrator->vcc)) {
> +		if (PTR_ERR(vibrator->vcc) != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "Failed to get regulator: %ld\n",
> +				PTR_ERR(vibrator->vcc));
> +		return PTR_ERR(vibrator->vcc);
> +	}
> +
> +	vibrator->enable_gpio = devm_gpiod_get(&pdev->dev, "enable",
> +					       GPIOD_OUT_LOW);
> +	if (IS_ERR(vibrator->enable_gpio)) {

You have explicit deferral handling for the regulator, but not gpio. I'd
prefer if we did (or not) it consistently.

> +		dev_err(&pdev->dev, "Failed to get enable gpio: %ld\n",
> +			PTR_ERR(vibrator->enable_gpio));
> +		return PTR_ERR(vibrator->enable_gpio);
> +	}
> +
> +	vibrator->clk = devm_clk_get(&pdev->dev, "pwm");
> +	if (IS_ERR(vibrator->clk)) {

Same here.

> +		dev_err(&pdev->dev, "Failed to lookup pwm clock: %ld\n",
> +			PTR_ERR(vibrator->clk));
> +		return PTR_ERR(vibrator->clk);
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Failed to get platform resource\n");
> +		return -ENODEV;
> +	}
> +
> +	vibrator->base = devm_ioremap(&pdev->dev, res->start,
> +				     resource_size(res));
> +	if (IS_ERR(vibrator->base)) {

devm_ioremap() returns NULL on error. You either need to check for NULL
or see of you can use devm_ioremap_resource().

> +		dev_err(&pdev->dev, "Failed to iomap resource: %ld\n",
> +			PTR_ERR(vibrator->base));
> +		return PTR_ERR(vibrator->base);
> +	}
> +
> +	vibrator->enabled = false;
> +	mutex_init(&vibrator->mutex);
> +	INIT_WORK(&vibrator->worker, msm_vibrator_worker);
> +
> +	vibrator->input->name = "msm-vibrator";
> +	vibrator->input->id.bustype = BUS_HOST;
> +	vibrator->input->dev.parent = &pdev->dev;

You allocated input device with devm so there is no need to set parent
by hand.

> +	vibrator->input->close = msm_vibrator_close;
> +
> +	input_set_drvdata(vibrator->input, vibrator);
> +	input_set_capability(vibrator->input, EV_FF, FF_RUMBLE);
> +
> +	ret = input_ff_create_memless(vibrator->input, NULL,
> +				      msm_vibrator_play_effect);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to create ff memless: %d", ret);
> +		return ret;
> +	}
> +
> +	ret = input_register_device(vibrator->input);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to register input device: %d", ret);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, vibrator);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused msm_vibrator_suspend(struct device *dev)
> +{
> +	struct msm_vibrator *vibrator = dev_get_drvdata(dev);

Prefer explicit platform_get_drvdata() since we are working with
platform device (even though it resolves to the same thing).

> +
> +	cancel_work_sync(&vibrator->worker);
> +
> +	if (vibrator->enabled)
> +		msm_vibrator_stop(vibrator);
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused msm_vibrator_resume(struct device *dev)
> +{
> +	struct msm_vibrator *vibrator = dev_get_drvdata(dev);

Same here.

Thanks.

-- 
Dmitry

  parent reply	other threads:[~2019-02-05 19:42 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-25  1:29 [PATCH v3 0/3] ARM: qcom: add vibrator support " Brian Masney
2018-10-25  1:29 ` [PATCH v3 1/3] dt-bindings: Input: new bindings for MSM vibrator Brian Masney
2018-10-25 21:44   ` Rob Herring
2018-10-25  1:29 ` [PATCH v3 2/3] Input: add new vibrator driver for various MSM SOCs Brian Masney
2019-02-05  9:26   ` Brian Masney
2019-02-05 19:42   ` Dmitry Torokhov [this message]
2019-02-06  0:52     ` Brian Masney
2018-10-25  1:29 ` [PATCH v3 3/3] ARM: dts: qcom: msm8974-hammerhead: add device tree bindings for vibrator Brian Masney

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190205194246.GA19151@dtor-ws \
    --to=dmitry.torokhov@gmail.com \
    --cc=andy.gross@linaro.org \
    --cc=david.brown@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=jonathan@marek.ca \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-soc@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=masneyb@onstation.org \
    --cc=robh+dt@kernel.org \
    --subject='Re: [PATCH v3 2/3] Input: add new vibrator driver for various MSM SOCs' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).