linux-kernel - Re: [PATCH V5 3/3] drivers/rtc/rtc-as3722: add RTC 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]

Date:	Thu, 10 Oct 2013 15:24:46 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Laxman Dewangan <ldewangan@...dia.com>
Cc:	<lee.jones@...aro.org>, <sameo@...ux.intel.com>,
	<broonie@...nel.org>, <linus.walleij@...aro.org>,
	<devicetree@...r.kernel.org>, <linux-doc@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-gpio@...r.kernel.org>,
	<rtc-linux@...glegroups.com>, <rob.herring@...xeda.com>,
	<mark.rutland@....com>, <pawel.moll@....com>,
	<swarren@...dotorg.org>, <rob@...dley.net>,
	<ijc+devicetree@...lion.org.uk>, <grant.likely@...aro.org>,
	Florian Lobmaier <florian.lobmaier@....com>
Subject: Re: [PATCH V5 3/3] drivers/rtc/rtc-as3722: add RTC driver

On Wed, 9 Oct 2013 17:59:21 +0530 Laxman Dewangan <ldewangan@...dia.com> wrote:

> The ams AS3722 is a compact system PMU suitable for mobile phones,
> tablets etc.
> 
> Add a driver to support accessing the RTC found on the ams AS3722
> PMIC using RTC framework.

I hate these patchsets, because ...

> index 9654aa3..d8785d7 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -153,6 +153,16 @@ config RTC_DRV_88PM80X
>  	  This driver can also be built as a module. If so, the module
>  	  will be called rtc-88pm80x.
>  
> +config RTC_DRV_AS3722
> +	tristate "ams AS3722 RTC driver"
> +	depends on MFD_AS3722

... the rtc patch depends on an mfd patch, so everyone ends up sitting
around looking at everyone else, wondering who will merge it all.

> +	help
> +	  If you say yes here you get support for the RTC of ams AS3722 PMIC
> +	  chips.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called rtc-as3722.
> +
>
> ...
>
> +static int as3722_rtc_probe(struct platform_device *pdev)
> +{
> +	struct as3722 *as3722 = dev_get_drvdata(pdev->dev.parent);
> +	struct as3722_rtc *as3722_rtc;
> +	int val;
> +	int ret;
> +
> +	as3722_rtc = devm_kzalloc(&pdev->dev, sizeof(*as3722_rtc), GFP_KERNEL);
> +	if (!as3722_rtc)
> +		return -ENOMEM;
> +
> +	as3722_rtc->as3722 = as3722;
> +	as3722_rtc->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, as3722_rtc);
> +
> +	/* Enable the RTC to make sure it is running. */
> +	ret = as3722_update_bits(as3722, AS3722_RTC_CONTROL_REG,
> +				AS3722_RTC_ON, AS3722_RTC_ON);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "RTC_CONTROL reg update failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	val = AS3722_RTC_ON | AS3722_RTC_ALARM_WAKEUP_EN;
> +	ret = as3722_write(as3722, AS3722_RTC_CONTROL_REG, val);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "RTC_CONTROL reg write failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	device_init_wakeup(&pdev->dev, 1);
> +
> +	as3722_rtc->rtc = rtc_device_register("as3722", &pdev->dev,
> +				&as3722_rtc_ops, THIS_MODULE);

devm_rtc_device_register()?

> +	if (IS_ERR(as3722_rtc->rtc)) {
> +		ret = PTR_ERR(as3722_rtc->rtc);
> +		dev_err(&pdev->dev, "RTC register failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	as3722_rtc->alarm_irq = platform_get_irq(pdev, 0);
> +	dev_info(&pdev->dev, "RTC interrupt %d\n", as3722_rtc->alarm_irq);
> +
> +	ret = request_threaded_irq(as3722_rtc->alarm_irq, NULL,
> +			as3722_alarm_irq, IRQF_ONESHOT | IRQF_EARLY_RESUME,
> +			"rtc-alarm", as3722_rtc);

devm_request_threaded_irq()?

> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to request alarm IRQ %d: %d\n",
> +				as3722_rtc->alarm_irq, ret);
> +		goto scrub;
> +	}
> +	disable_irq(as3722_rtc->alarm_irq);

It seems strange to disable your IRQ here.

> +	return 0;
> +scrub:
> +	rtc_device_unregister(as3722_rtc->rtc);
> +	return ret;
> +}

The function is a bit confused.  It uses a mix of the "returns
sprinkled all over the place" approach and the "goto out" approach. 
The latter is preferred.

> +static int as3722_rtc_remove(struct platform_device *pdev)
> +{
> +	struct as3722_rtc *as3722_rtc = platform_get_drvdata(pdev);
> +
> +	free_irq(as3722_rtc->alarm_irq, as3722_rtc);
> +	rtc_device_unregister(as3722_rtc->rtc);
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM

Most (but not all!) rtc drivers use CONFIG_PM_SLEEP.  People have madly
mucked with CONFIG_PM* and I don't know the difference and I don't know
what's going on and I don't know of a convenient place to go to find
out.  If you work it out, please be sure to tell me!  But as soon as I
figure it out I'm sure they'll go and madly muck with it again.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/