linux-kernel - Re: [PATCH] rtc: Add MOXA ART 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]

Message-ID: <20130710154712.GG24508@sirena.org.uk>
Date:	Wed, 10 Jul 2013 16:47:12 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Jonas Jensen <jonas.jensen@...il.com>
Cc:	rtc-linux@...glegroups.com, a.zummo@...ertech.it, arm@...nel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] rtc: Add MOXA ART RTC driver

On Wed, Jul 10, 2013 at 04:00:48PM +0200, Jonas Jensen wrote:

> +++ b/drivers/rtc/Makefile
> @@ -77,6 +77,7 @@ obj-$(CONFIG_RTC_DRV_MAX6902)	+= rtc-max6902.o
>  obj-$(CONFIG_RTC_DRV_MAX77686)	+= rtc-max77686.o
>  obj-$(CONFIG_RTC_DRV_MC13XXX)	+= rtc-mc13xxx.o
>  obj-$(CONFIG_RTC_DRV_MSM6242)	+= rtc-msm6242.o
> +obj-$(CONFIG_RTC_DRV_MOXART)	+= rtc-moxart.o
>  obj-$(CONFIG_RTC_DRV_MPC5121)	+= rtc-mpc5121.o

It'd be good to keep this sorted.

> +struct rtc_plat_data {

This is a bit confusing - normally platform data is data passed in by
the platform as part of device registration, not runtime data.

> +	struct rtc_device *rtc;
> +};
> +
> +static spinlock_t rtc_lock;

Why is this global not part of the runtime data?  Not that anyone is
likely to have two RTCs in the one system but still...

> +u8 moxart_rtc_read_byte(void)
> +{
> +	int i;
> +	u8 data = 0;
> +
> +	for (i = 0; i < 8; i++) {
> +		gpio_set_value(GPIO_RTC_SCLK, GPIO_EM1240_LOW);
> +		udelay(GPIO_RTC_DELAY_TIME);
> +		gpio_set_value(GPIO_RTC_SCLK, GPIO_EM1240_HIGH);
> +		if (gpio_get_value(GPIO_RTC_DATA))
> +			data |= (1 << i);
> +		udelay(GPIO_RTC_DELAY_TIME);

This looks wrong, although I expect it's probably fine - you're reading
the value with no delay after setting the GPIO high.  I assume the
hardware actually strobes the data out on the the high to low transition
but in that case I'd expect the get to be before the raise.  Either that
or some delay after the raise just to make sure.

It's also very odd seeing the constants for _LOW and _HIGH, these should
just be booleans.

> +static int moxart_rtc_ioctl(struct device *dev, unsigned int cmd,
> +	unsigned long arg)
> +{
> +	switch (cmd) {
> +	default:
> +		return -ENOIOCTLCMD;
> +	}
> +
> +	return 0;
> +}

Why not just remove this function?

> + out:
> +	if (pdata->rtc)
> +		rtc_device_unregister(pdata->rtc);
> +	devm_kfree(&pdev->dev, pdata);

devm_kfree() isn't needed, the main point of devm_ is to avoid having to
explicitly free things.

> +	gpio_free(GPIO_RTC_DATA);
> +	gpio_free(GPIO_RTC_SCLK);
> +	gpio_free(GPIO_RTC_RESET);

Use devm_gpio_request().

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