Patchwork [2/6] rtc: rtc-m48t86: add hooks to support driver side memory mapping

login
register
mail settings
Submitter Alexander Clouter
Date April 1, 2013, 11:22 p.m.
Message ID <1364858565-17192-3-git-send-email-alex@digriz.org.uk>
Download mbox | patch
Permalink /patch/232839/
State New
Headers show

Comments

Alexander Clouter - April 1, 2013, 11:22 p.m.
If platform_data is not defined (as before), then named memory io
ranges need to be defined ("rtc_index" and "rtc_data").  The driver
then maps those regions and uses them as the RTC index and data
addresses.

Does compile with the following warnings, I cannot see the codepath
affected myself:
----
drivers/rtc/rtc-m48t86.c: In function ‘m48t86_rtc_probe’:
drivers/rtc/rtc-m48t86.c:180: warning: ‘res_index’ may be used uninitialized in this function
drivers/rtc/rtc-m48t86.c:180: warning: ‘res_data’ may be used uninitialized in this function
----

Signed-off-by: Alexander Clouter <alex@digriz.org.uk>
---
 drivers/rtc/rtc-m48t86.c |  230 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 173 insertions(+), 57 deletions(-)
Ryan Mallon - April 1, 2013, 11:36 p.m.
On 02/04/13 10:22, Alexander Clouter wrote:
> If platform_data is not defined (as before), then named memory io
> ranges need to be defined ("rtc_index" and "rtc_data").  The driver
> then maps those regions and uses them as the RTC index and data
> addresses.
> 
> Does compile with the following warnings, I cannot see the codepath
> affected myself:
> ----
> drivers/rtc/rtc-m48t86.c: In function ‘m48t86_rtc_probe’:
> drivers/rtc/rtc-m48t86.c:180: warning: ‘res_index’ may be used uninitialized in this function
> drivers/rtc/rtc-m48t86.c:180: warning: ‘res_data’ may be used uninitialized in this function

It is caused by the exit paths. If pdev->dev.platform_data is set, the
res_index and res_data are never initialised, but in the error case you
still for rtc_device_register you jump to out_io_data, which will then
dereference res_index/res_data. You need to make the exit paths
conditional on pdev->dev.platform_data (or init res_index/data to NULL
and make the release_mem_regions conditional on that).

~Ryan

> ----
> 
> Signed-off-by: Alexander Clouter <alex@digriz.org.uk>
> ---
>  drivers/rtc/rtc-m48t86.c |  230 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 173 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-m48t86.c b/drivers/rtc/rtc-m48t86.c
> index 25e0116..6f99e64 100644
> --- a/drivers/rtc/rtc-m48t86.c
> +++ b/drivers/rtc/rtc-m48t86.c
> @@ -18,6 +18,8 @@
>  #include <linux/platform_device.h>
>  #include <linux/platform_data/rtc-m48t86.h>
>  #include <linux/bcd.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
>  
>  #define M48T86_REG_SEC		0x00
>  #define M48T86_REG_SECALRM	0x01
> @@ -41,40 +43,71 @@
>  
>  #define DRV_VERSION "0.1"
>  
> +struct m48t86_rtc_private_data {
> +	void __iomem		*io_index;
> +	void __iomem		*io_data;
> +
> +	struct rtc_device	*rtc;
> +	struct m48t86_ops	*ops;
> +};
> +
> +static void m48t86_rtc_writebyte(struct device *dev,
> +			unsigned char value, unsigned long addr)
> +{
> +	struct m48t86_rtc_private_data *priv = dev_get_drvdata(dev);
> +
> +	if (priv->ops) {
> +		priv->ops->writebyte(value, addr);
> +		return;
> +	}
> +
> +	writeb(addr, priv->io_index);
> +	writeb(value, priv->io_data);
> +}
> +
> +static unsigned char m48t86_rtc_readbyte(struct device *dev,
> +			unsigned long addr)
> +{
> +	struct m48t86_rtc_private_data *priv = dev_get_drvdata(dev);
> +
> +	if (priv->ops)
> +		return priv->ops->readbyte(addr);
> +
> +	writeb(addr, priv->io_index);
> +	return readb(priv->io_data);
> +}
>  
>  static int m48t86_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  {
>  	unsigned char reg;
> -	struct platform_device *pdev = to_platform_device(dev);
> -	struct m48t86_ops *ops = pdev->dev.platform_data;
>  
> -	reg = ops->readbyte(M48T86_REG_B);
> +	reg = m48t86_rtc_readbyte(dev, M48T86_REG_B);
>  
>  	if (reg & M48T86_REG_B_DM) {
>  		/* data (binary) mode */
> -		tm->tm_sec	= ops->readbyte(M48T86_REG_SEC);
> -		tm->tm_min	= ops->readbyte(M48T86_REG_MIN);
> -		tm->tm_hour	= ops->readbyte(M48T86_REG_HOUR) & 0x3F;
> -		tm->tm_mday	= ops->readbyte(M48T86_REG_DOM);
> +		tm->tm_sec	= m48t86_rtc_readbyte(dev, M48T86_REG_SEC);
> +		tm->tm_min	= m48t86_rtc_readbyte(dev, M48T86_REG_MIN);
> +		tm->tm_hour	= m48t86_rtc_readbyte(dev, M48T86_REG_HOUR) & 0x3F;
> +		tm->tm_mday	= m48t86_rtc_readbyte(dev, M48T86_REG_DOM);
>  		/* tm_mon is 0-11 */
> -		tm->tm_mon	= ops->readbyte(M48T86_REG_MONTH) - 1;
> -		tm->tm_year	= ops->readbyte(M48T86_REG_YEAR) + 100;
> -		tm->tm_wday	= ops->readbyte(M48T86_REG_DOW);
> +		tm->tm_mon	= m48t86_rtc_readbyte(dev, M48T86_REG_MONTH) - 1;
> +		tm->tm_year	= m48t86_rtc_readbyte(dev, M48T86_REG_YEAR) + 100;
> +		tm->tm_wday	= m48t86_rtc_readbyte(dev, M48T86_REG_DOW);
>  	} else {
>  		/* bcd mode */
> -		tm->tm_sec	= bcd2bin(ops->readbyte(M48T86_REG_SEC));
> -		tm->tm_min	= bcd2bin(ops->readbyte(M48T86_REG_MIN));
> -		tm->tm_hour	= bcd2bin(ops->readbyte(M48T86_REG_HOUR) & 0x3F);
> -		tm->tm_mday	= bcd2bin(ops->readbyte(M48T86_REG_DOM));
> +		tm->tm_sec	= bcd2bin(m48t86_rtc_readbyte(dev, M48T86_REG_SEC));
> +		tm->tm_min	= bcd2bin(m48t86_rtc_readbyte(dev, M48T86_REG_MIN));
> +		tm->tm_hour	= bcd2bin(m48t86_rtc_readbyte(dev, M48T86_REG_HOUR) & 0x3F);
> +		tm->tm_mday	= bcd2bin(m48t86_rtc_readbyte(dev, M48T86_REG_DOM));
>  		/* tm_mon is 0-11 */
> -		tm->tm_mon	= bcd2bin(ops->readbyte(M48T86_REG_MONTH)) - 1;
> -		tm->tm_year	= bcd2bin(ops->readbyte(M48T86_REG_YEAR)) + 100;
> -		tm->tm_wday	= bcd2bin(ops->readbyte(M48T86_REG_DOW));
> +		tm->tm_mon	= bcd2bin(m48t86_rtc_readbyte(dev, M48T86_REG_MONTH)) - 1;
> +		tm->tm_year	= bcd2bin(m48t86_rtc_readbyte(dev, M48T86_REG_YEAR)) + 100;
> +		tm->tm_wday	= bcd2bin(m48t86_rtc_readbyte(dev, M48T86_REG_DOW));
>  	}
>  
>  	/* correct the hour if the clock is in 12h mode */
>  	if (!(reg & M48T86_REG_B_H24))
> -		if (ops->readbyte(M48T86_REG_HOUR) & 0x80)
> +		if (m48t86_rtc_readbyte(dev, M48T86_REG_HOUR) & 0x80)
>  			tm->tm_hour += 12;
>  
>  	return rtc_valid_tm(tm);
> @@ -83,38 +116,36 @@ static int m48t86_rtc_read_time(struct device *dev, struct rtc_time *tm)
>  static int m48t86_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  {
>  	unsigned char reg;
> -	struct platform_device *pdev = to_platform_device(dev);
> -	struct m48t86_ops *ops = pdev->dev.platform_data;
>  
> -	reg = ops->readbyte(M48T86_REG_B);
> +	reg = m48t86_rtc_readbyte(dev, M48T86_REG_B);
>  
>  	/* update flag and 24h mode */
>  	reg |= M48T86_REG_B_SET | M48T86_REG_B_H24;
> -	ops->writebyte(reg, M48T86_REG_B);
> +	m48t86_rtc_writebyte(dev, reg, M48T86_REG_B);
>  
>  	if (reg & M48T86_REG_B_DM) {
>  		/* data (binary) mode */
> -		ops->writebyte(tm->tm_sec, M48T86_REG_SEC);
> -		ops->writebyte(tm->tm_min, M48T86_REG_MIN);
> -		ops->writebyte(tm->tm_hour, M48T86_REG_HOUR);
> -		ops->writebyte(tm->tm_mday, M48T86_REG_DOM);
> -		ops->writebyte(tm->tm_mon + 1, M48T86_REG_MONTH);
> -		ops->writebyte(tm->tm_year % 100, M48T86_REG_YEAR);
> -		ops->writebyte(tm->tm_wday, M48T86_REG_DOW);
> +		m48t86_rtc_writebyte(dev, tm->tm_sec, M48T86_REG_SEC);
> +		m48t86_rtc_writebyte(dev, tm->tm_min, M48T86_REG_MIN);
> +		m48t86_rtc_writebyte(dev, tm->tm_hour, M48T86_REG_HOUR);
> +		m48t86_rtc_writebyte(dev, tm->tm_mday, M48T86_REG_DOM);
> +		m48t86_rtc_writebyte(dev, tm->tm_mon + 1, M48T86_REG_MONTH);
> +		m48t86_rtc_writebyte(dev, tm->tm_year % 100, M48T86_REG_YEAR);
> +		m48t86_rtc_writebyte(dev, tm->tm_wday, M48T86_REG_DOW);
>  	} else {
>  		/* bcd mode */
> -		ops->writebyte(bin2bcd(tm->tm_sec), M48T86_REG_SEC);
> -		ops->writebyte(bin2bcd(tm->tm_min), M48T86_REG_MIN);
> -		ops->writebyte(bin2bcd(tm->tm_hour), M48T86_REG_HOUR);
> -		ops->writebyte(bin2bcd(tm->tm_mday), M48T86_REG_DOM);
> -		ops->writebyte(bin2bcd(tm->tm_mon + 1), M48T86_REG_MONTH);
> -		ops->writebyte(bin2bcd(tm->tm_year % 100), M48T86_REG_YEAR);
> -		ops->writebyte(bin2bcd(tm->tm_wday), M48T86_REG_DOW);
> +		m48t86_rtc_writebyte(dev, bin2bcd(tm->tm_sec), M48T86_REG_SEC);
> +		m48t86_rtc_writebyte(dev, bin2bcd(tm->tm_min), M48T86_REG_MIN);
> +		m48t86_rtc_writebyte(dev, bin2bcd(tm->tm_hour), M48T86_REG_HOUR);
> +		m48t86_rtc_writebyte(dev, bin2bcd(tm->tm_mday), M48T86_REG_DOM);
> +		m48t86_rtc_writebyte(dev, bin2bcd(tm->tm_mon + 1), M48T86_REG_MONTH);
> +		m48t86_rtc_writebyte(dev, bin2bcd(tm->tm_year % 100), M48T86_REG_YEAR);
> +		m48t86_rtc_writebyte(dev, bin2bcd(tm->tm_wday), M48T86_REG_DOW);
>  	}
>  
>  	/* update ended */
>  	reg &= ~M48T86_REG_B_SET;
> -	ops->writebyte(reg, M48T86_REG_B);
> +	m48t86_rtc_writebyte(dev, reg, M48T86_REG_B);
>  
>  	return 0;
>  }
> @@ -122,15 +153,13 @@ static int m48t86_rtc_set_time(struct device *dev, struct rtc_time *tm)
>  static int m48t86_rtc_proc(struct device *dev, struct seq_file *seq)
>  {
>  	unsigned char reg;
> -	struct platform_device *pdev = to_platform_device(dev);
> -	struct m48t86_ops *ops = pdev->dev.platform_data;
>  
> -	reg = ops->readbyte(M48T86_REG_B);
> +	reg = m48t86_rtc_readbyte(dev, M48T86_REG_B);
>  
>  	seq_printf(seq, "mode\t\t: %s\n",
>  		 (reg & M48T86_REG_B_DM) ? "binary" : "bcd");
>  
> -	reg = ops->readbyte(M48T86_REG_D);
> +	reg = m48t86_rtc_readbyte(dev, M48T86_REG_D);
>  
>  	seq_printf(seq, "battery\t\t: %s\n",
>  		 (reg & M48T86_REG_D_VRT) ? "ok" : "exhausted");
> @@ -144,34 +173,121 @@ static const struct rtc_class_ops m48t86_rtc_ops = {
>  	.proc		= m48t86_rtc_proc,
>  };
>  
> -static int m48t86_rtc_probe(struct platform_device *dev)
> +static int m48t86_rtc_probe(struct platform_device *pdev)
>  {
>  	unsigned char reg;
> -	struct m48t86_ops *ops = dev->dev.platform_data;
> -	struct rtc_device *rtc = rtc_device_register("m48t86",
> -				&dev->dev, &m48t86_rtc_ops, THIS_MODULE);
> -
> -	if (IS_ERR(rtc))
> -		return PTR_ERR(rtc);
> -
> -	platform_set_drvdata(dev, rtc);
> +	int err;
> +	struct resource *res_index, *res_data;
> +	struct m48t86_rtc_private_data *priv;
> +
> +	/* Allocate memory for the device structure (and zero it) */
> +	priv = kzalloc(sizeof(struct m48t86_rtc_private_data), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	if (!pdev->dev.platform_data) {
> +		res_index = platform_get_resource_byname(
> +					pdev, IORESOURCE_MEM, "rtc_index");
> +		if (!res_index) {
> +			err = -ENXIO;
> +			goto out_free;
> +		}
> +
> +		res_data = platform_get_resource_byname(
> +					pdev, IORESOURCE_MEM, "rtc_data");
> +		if (!res_data) {
> +			err = -ENXIO;
> +			goto out_free;
> +		}
> +
> +		if (!request_mem_region(res_index->start,
> +					resource_size(res_index),
> +					dev_name(&pdev->dev))) {
> +			err = -EBUSY;
> +			goto out_free;
> +		}
> +
> +		if (!request_mem_region(res_data->start,
> +					resource_size(res_data),
> +					dev_name(&pdev->dev))) {
> +			err = -EBUSY;
> +			goto out_release_index;
> +		}
> +
> +		priv->io_index = ioremap(res_index->start,
> +					resource_size(res_index));
> +		if (!priv->io_index) {
> +			err = -EIO;
> +			goto out_release_data;
> +		}
> +
> +		priv->io_data = ioremap(res_data->start,
> +					resource_size(res_data));
> +		if (!priv->io_data) {
> +			err = -EIO;
> +			goto out_io_index;
> +		}
> +	} else
> +		priv->ops = pdev->dev.platform_data;
> +
> +	priv->rtc = rtc_device_register("m48t86",
> +				&pdev->dev, &m48t86_rtc_ops, THIS_MODULE);
> +	if (IS_ERR(priv->rtc)) {
> +		err = PTR_ERR(priv->rtc);
> +		if (!pdev->dev.platform_data)
> +			goto out_io_data;
> +		else
> +			goto out_free;
> +	}
>  
>  	/* read battery status */
> -	reg = ops->readbyte(M48T86_REG_D);
> -	dev_info(&dev->dev, "battery %s\n",
> +	reg = m48t86_rtc_readbyte(&pdev->dev, M48T86_REG_D);
> +	dev_info(&pdev->dev, "battery %s\n",
>  		(reg & M48T86_REG_D_VRT) ? "ok" : "exhausted");
>  
>  	return 0;
> +
> +out_io_data:
> +	iounmap(priv->io_data);
> +out_io_index:
> +	iounmap(priv->io_index);
> +out_release_data:
> +	release_mem_region(res_data->start, resource_size(res_data));
> +out_release_index:
> +	release_mem_region(res_index->start, resource_size(res_index));
> +out_free:
> +	platform_set_drvdata(pdev, NULL);
> +	kfree(priv);
> +	return err;
>  }
>  
> -static int m48t86_rtc_remove(struct platform_device *dev)
> +static int m48t86_rtc_remove(struct platform_device *pdev)
>  {
> -	struct rtc_device *rtc = platform_get_drvdata(dev);
> +	struct resource *res;
> +	struct m48t86_rtc_private_data *priv = platform_get_drvdata(pdev);
> +
> +	if (priv->rtc)
> +		rtc_device_unregister(priv->rtc);
> +
> +	if (priv->io_data) {
> +		iounmap(priv->io_data);
> +		res = platform_get_resource_byname(
> +					pdev, IORESOURCE_MEM, "rtc_data");
> +		release_mem_region(res->start, resource_size(res));
> +	}
> +
> +	if (priv->io_index) {
> +		iounmap(priv->io_index);
> +		res = platform_get_resource_byname(
> +					pdev, IORESOURCE_MEM, "rtc_index");
> +		release_mem_region(res->start, resource_size(res));
> +	}
>  
> - 	if (rtc)
> -		rtc_device_unregister(rtc);
> +	platform_set_drvdata(pdev, NULL);
>  
> -	platform_set_drvdata(dev, NULL);
> +	kfree(priv);
>  
>  	return 0;
>  }
>
Alexander Clouter - April 1, 2013, 11:42 p.m.
On Tue, Apr 02, 2013 at 10:36:43AM +1100, Ryan Mallon wrote:
>On 02/04/13 10:22, Alexander Clouter wrote:
>> If platform_data is not defined (as before), then named memory io
>> ranges need to be defined ("rtc_index" and "rtc_data").  The driver
>> then maps those regions and uses them as the RTC index and data
>> addresses.
>>
>> Does compile with the following warnings, I cannot see the codepath
>> affected myself:
>> ----
>> drivers/rtc/rtc-m48t86.c: In function ‘m48t86_rtc_probe’:
>> drivers/rtc/rtc-m48t86.c:180: warning: ‘res_index’ may be used uninitialized in this function
>> drivers/rtc/rtc-m48t86.c:180: warning: ‘res_data’ may be used uninitialized in this function
>
>It is caused by the exit paths. If pdev->dev.platform_data is set, the
>res_index and res_data are never initialised, but in the error case you
>still for rtc_device_register you jump to out_io_data, which will then
>dereference res_index/res_data. You need to make the exit paths
>conditional on pdev->dev.platform_data (or init res_index/data to NULL
>and make the release_mem_regions conditional on that).

However, the 'goto out_io_data' in the 'IS_ERR(priv->rtc)' is wrapped in a 'if 
(!pdev->dev.platform_data)', else we jump to out_free.

I suspect I am probably missing something *too* obvious here for it to click?

Cheers

>> +	priv->rtc = rtc_device_register("m48t86",
>> +				&pdev->dev, &m48t86_rtc_ops, THIS_MODULE);
>> +	if (IS_ERR(priv->rtc)) {                <--------------
>> +		err = PTR_ERR(priv->rtc);
>> +		if (!pdev->dev.platform_data)   <--------------
>> +			goto out_io_data;
>> +		else
>> +			goto out_free;
>> +	}
>>
>>  	/* read battery status */
>> -	reg = ops->readbyte(M48T86_REG_D);
>> -	dev_info(&dev->dev, "battery %s\n",
>> +	reg = m48t86_rtc_readbyte(&pdev->dev, M48T86_REG_D);
>> +	dev_info(&pdev->dev, "battery %s\n",
>>  		(reg & M48T86_REG_D_VRT) ? "ok" : "exhausted");
>>
>>  	return 0;
>> +
>> +out_io_data:
>> +	iounmap(priv->io_data);
>> +out_io_index:
>> +	iounmap(priv->io_index);
>> +out_release_data:
>> +	release_mem_region(res_data->start, resource_size(res_data));
>> +out_release_index:
>> +	release_mem_region(res_index->start, resource_size(res_index));
>> +out_free:
>> +	platform_set_drvdata(pdev, NULL);
>> +	kfree(priv);
>> +	return err;
>>  }
Ryan Mallon - April 2, 2013, 5:34 a.m.
On 02/04/13 10:22, Alexander Clouter wrote:
> If platform_data is not defined (as before), then named memory io
> ranges need to be defined ("rtc_index" and "rtc_data").  The driver
> then maps those regions and uses them as the RTC index and data
> addresses.
> 
> Does compile with the following warnings, I cannot see the codepath
> affected myself:
> ----
> drivers/rtc/rtc-m48t86.c: In function ‘m48t86_rtc_probe’:
> drivers/rtc/rtc-m48t86.c:180: warning: ‘res_index’ may be used uninitialized in this function
> drivers/rtc/rtc-m48t86.c:180: warning: ‘res_data’ may be used uninitialized in this function
> ----
> 
> Signed-off-by: Alexander Clouter <alex@digriz.org.uk>
> ---
>  drivers/rtc/rtc-m48t86.c |  230 ++++++++++++++++++++++++++++++++++------------
>  1 file changed, 173 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-m48t86.c b/drivers/rtc/rtc-m48t86.c
> index 25e0116..6f99e64 100644
> --- a/drivers/rtc/rtc-m48t86.c
> +++ b/drivers/rtc/rtc-m48t86.c
> @@ -18,6 +18,8 @@
>  #include <linux/platform_device.h>
>  #include <linux/platform_data/rtc-m48t86.h>
>  #include <linux/bcd.h>
> +#include <linux/slab.h>
> +#include <linux/io.h>
>  
>  #define M48T86_REG_SEC		0x00
>  #define M48T86_REG_SECALRM	0x01
> @@ -41,40 +43,71 @@
>  
>  #define DRV_VERSION "0.1"
>  
> +struct m48t86_rtc_private_data {
> +	void __iomem		*io_index;
> +	void __iomem		*io_data;
> +
> +	struct rtc_device	*rtc;
> +	struct m48t86_ops	*ops;
> +};
> +
> +static void m48t86_rtc_writebyte(struct device *dev,
> +			unsigned char value, unsigned long addr)
> +{
> +	struct m48t86_rtc_private_data *priv = dev_get_drvdata(dev);
> +
> +	if (priv->ops) {
> +		priv->ops->writebyte(value, addr);
> +		return;
> +	}
> +
> +	writeb(addr, priv->io_index);
> +	writeb(value, priv->io_data);
> +}
> +
> +static unsigned char m48t86_rtc_readbyte(struct device *dev,
> +			unsigned long addr)

The read/writebyte functions should return a u8 and take a u8 for the
address (since you are using readb/writeb). For the temporary step which
still has the ops structure, you can explicitly cast addr to unsigned
long if you want to make it obvious that the old api was silly.

~Ryan
Ryan Mallon - April 2, 2013, 5:37 a.m.
On 02/04/13 10:42, Alexander Clouter wrote:
> On Tue, Apr 02, 2013 at 10:36:43AM +1100, Ryan Mallon wrote:
>> On 02/04/13 10:22, Alexander Clouter wrote:
>>> If platform_data is not defined (as before), then named memory io
>>> ranges need to be defined ("rtc_index" and "rtc_data").  The driver
>>> then maps those regions and uses them as the RTC index and data
>>> addresses.
>>>
>>> Does compile with the following warnings, I cannot see the codepath
>>> affected myself:
>>> ----
>>> drivers/rtc/rtc-m48t86.c: In function ‘m48t86_rtc_probe’:
>>> drivers/rtc/rtc-m48t86.c:180: warning: ‘res_index’ may be used
>>> uninitialized in this function
>>> drivers/rtc/rtc-m48t86.c:180: warning: ‘res_data’ may be used
>>> uninitialized in this function
>>
>> It is caused by the exit paths. If pdev->dev.platform_data is set, the
>> res_index and res_data are never initialised, but in the error case you
>> still for rtc_device_register you jump to out_io_data, which will then
>> dereference res_index/res_data. You need to make the exit paths
>> conditional on pdev->dev.platform_data (or init res_index/data to NULL
>> and make the release_mem_regions conditional on that).
> 
> However, the 'goto out_io_data' in the 'IS_ERR(priv->rtc)' is wrapped in
> a 'if (!pdev->dev.platform_data)', else we jump to out_free.

Ah right, I missed that. In that case, I can't see the problem either :-/.

> 
> I suspect I am probably missing something *too* obvious here for it to
> click?

It could be gcc being dumb, though this does seem a straight-forward
enough case for gcc to get it correct. It would be nice to get rid of
the warning though. Doing:

  struct resource *res_index = NULL; /* Avoid GCC warning */

Isn't too costly.

~Ryan
Andrew Lunn - April 4, 2013, 7:25 a.m.
> -static int m48t86_rtc_probe(struct platform_device *dev)
> +static int m48t86_rtc_probe(struct platform_device *pdev)
>  {
>  	unsigned char reg;
> -	struct m48t86_ops *ops = dev->dev.platform_data;
> -	struct rtc_device *rtc = rtc_device_register("m48t86",
> -				&dev->dev, &m48t86_rtc_ops, THIS_MODULE);
> -
> -	if (IS_ERR(rtc))
> -		return PTR_ERR(rtc);
> -
> -	platform_set_drvdata(dev, rtc);
> +	int err;
> +	struct resource *res_index, *res_data;
> +	struct m48t86_rtc_private_data *priv;
> +
> +	/* Allocate memory for the device structure (and zero it) */
> +	priv = kzalloc(sizeof(struct m48t86_rtc_private_data), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	if (!pdev->dev.platform_data) {
> +		res_index = platform_get_resource_byname(
> +					pdev, IORESOURCE_MEM, "rtc_index");
> +		if (!res_index) {
> +			err = -ENXIO;
> +			goto out_free;
> +		}
> +
> +		res_data = platform_get_resource_byname(
> +					pdev, IORESOURCE_MEM, "rtc_data");
> +		if (!res_data) {
> +			err = -ENXIO;
> +			goto out_free;
> +		}
> +
> +		if (!request_mem_region(res_index->start,
> +					resource_size(res_index),
> +					dev_name(&pdev->dev))) {
> +			err = -EBUSY;
> +			goto out_free;
> +		}
> +
> +		if (!request_mem_region(res_data->start,
> +					resource_size(res_data),
> +					dev_name(&pdev->dev))) {
> +			err = -EBUSY;
> +			goto out_release_index;
> +		}
> +
> +		priv->io_index = ioremap(res_index->start,
> +					resource_size(res_index));
> +		if (!priv->io_index) {
> +			err = -EIO;
> +			goto out_release_data;
> +		}
> +
> +		priv->io_data = ioremap(res_data->start,
> +					resource_size(res_data));
> +		if (!priv->io_data) {
> +			err = -EIO;
> +			goto out_io_index;
> +		}
> +	} else

Hi Alexander

It would be good to use the devm_ functions here. It will make the
cleanup on error much simpler, solve your warnings problem, and simply
the remove function.


> +		priv->ops = pdev->dev.platform_data;
> +
> +	priv->rtc = rtc_device_register("m48t86",
> +				&pdev->dev, &m48t86_rtc_ops, THIS_MODULE);
> +	if (IS_ERR(priv->rtc)) {
> +		err = PTR_ERR(priv->rtc);
> +		if (!pdev->dev.platform_data)
> +			goto out_io_data;
> +		else
> +			goto out_free;
> +	}
>  
>  	/* read battery status */
> -	reg = ops->readbyte(M48T86_REG_D);
> -	dev_info(&dev->dev, "battery %s\n",
> +	reg = m48t86_rtc_readbyte(&pdev->dev, M48T86_REG_D);
> +	dev_info(&pdev->dev, "battery %s\n",
>  		(reg & M48T86_REG_D_VRT) ? "ok" : "exhausted");
>  
>  	return 0;
> +
> +out_io_data:
> +	iounmap(priv->io_data);
> +out_io_index:
> +	iounmap(priv->io_index);
> +out_release_data:
> +	release_mem_region(res_data->start, resource_size(res_data));
> +out_release_index:
> +	release_mem_region(res_index->start, resource_size(res_index));
> +out_free:
> +	platform_set_drvdata(pdev, NULL);
> +	kfree(priv);
> +	return err;
>  }
>  
> -static int m48t86_rtc_remove(struct platform_device *dev)
> +static int m48t86_rtc_remove(struct platform_device *pdev)
>  {
> -	struct rtc_device *rtc = platform_get_drvdata(dev);
> +	struct resource *res;
> +	struct m48t86_rtc_private_data *priv = platform_get_drvdata(pdev);
> +
> +	if (priv->rtc)
> +		rtc_device_unregister(priv->rtc);
> +
> +	if (priv->io_data) {
> +		iounmap(priv->io_data);
> +		res = platform_get_resource_byname(
> +					pdev, IORESOURCE_MEM, "rtc_data");
> +		release_mem_region(res->start, resource_size(res));
> +	}
> +
> +	if (priv->io_index) {
> +		iounmap(priv->io_index);
> +		res = platform_get_resource_byname(
> +					pdev, IORESOURCE_MEM, "rtc_index");
> +		release_mem_region(res->start, resource_size(res));
> +	}
>  
> - 	if (rtc)
> -		rtc_device_unregister(rtc);
> +	platform_set_drvdata(pdev, NULL);
>  
> -	platform_set_drvdata(dev, NULL);
> +	kfree(priv);
>  
>  	return 0;
>  }
> -- 
> 1.7.10.4
>

Patch

diff --git a/drivers/rtc/rtc-m48t86.c b/drivers/rtc/rtc-m48t86.c
index 25e0116..6f99e64 100644
--- a/drivers/rtc/rtc-m48t86.c
+++ b/drivers/rtc/rtc-m48t86.c
@@ -18,6 +18,8 @@ 
 #include <linux/platform_device.h>
 #include <linux/platform_data/rtc-m48t86.h>
 #include <linux/bcd.h>
+#include <linux/slab.h>
+#include <linux/io.h>
 
 #define M48T86_REG_SEC		0x00
 #define M48T86_REG_SECALRM	0x01
@@ -41,40 +43,71 @@ 
 
 #define DRV_VERSION "0.1"
 
+struct m48t86_rtc_private_data {
+	void __iomem		*io_index;
+	void __iomem		*io_data;
+
+	struct rtc_device	*rtc;
+	struct m48t86_ops	*ops;
+};
+
+static void m48t86_rtc_writebyte(struct device *dev,
+			unsigned char value, unsigned long addr)
+{
+	struct m48t86_rtc_private_data *priv = dev_get_drvdata(dev);
+
+	if (priv->ops) {
+		priv->ops->writebyte(value, addr);
+		return;
+	}
+
+	writeb(addr, priv->io_index);
+	writeb(value, priv->io_data);
+}
+
+static unsigned char m48t86_rtc_readbyte(struct device *dev,
+			unsigned long addr)
+{
+	struct m48t86_rtc_private_data *priv = dev_get_drvdata(dev);
+
+	if (priv->ops)
+		return priv->ops->readbyte(addr);
+
+	writeb(addr, priv->io_index);
+	return readb(priv->io_data);
+}
 
 static int m48t86_rtc_read_time(struct device *dev, struct rtc_time *tm)
 {
 	unsigned char reg;
-	struct platform_device *pdev = to_platform_device(dev);
-	struct m48t86_ops *ops = pdev->dev.platform_data;
 
-	reg = ops->readbyte(M48T86_REG_B);
+	reg = m48t86_rtc_readbyte(dev, M48T86_REG_B);
 
 	if (reg & M48T86_REG_B_DM) {
 		/* data (binary) mode */
-		tm->tm_sec	= ops->readbyte(M48T86_REG_SEC);
-		tm->tm_min	= ops->readbyte(M48T86_REG_MIN);
-		tm->tm_hour	= ops->readbyte(M48T86_REG_HOUR) & 0x3F;
-		tm->tm_mday	= ops->readbyte(M48T86_REG_DOM);
+		tm->tm_sec	= m48t86_rtc_readbyte(dev, M48T86_REG_SEC);
+		tm->tm_min	= m48t86_rtc_readbyte(dev, M48T86_REG_MIN);
+		tm->tm_hour	= m48t86_rtc_readbyte(dev, M48T86_REG_HOUR) & 0x3F;
+		tm->tm_mday	= m48t86_rtc_readbyte(dev, M48T86_REG_DOM);
 		/* tm_mon is 0-11 */
-		tm->tm_mon	= ops->readbyte(M48T86_REG_MONTH) - 1;
-		tm->tm_year	= ops->readbyte(M48T86_REG_YEAR) + 100;
-		tm->tm_wday	= ops->readbyte(M48T86_REG_DOW);
+		tm->tm_mon	= m48t86_rtc_readbyte(dev, M48T86_REG_MONTH) - 1;
+		tm->tm_year	= m48t86_rtc_readbyte(dev, M48T86_REG_YEAR) + 100;
+		tm->tm_wday	= m48t86_rtc_readbyte(dev, M48T86_REG_DOW);
 	} else {
 		/* bcd mode */
-		tm->tm_sec	= bcd2bin(ops->readbyte(M48T86_REG_SEC));
-		tm->tm_min	= bcd2bin(ops->readbyte(M48T86_REG_MIN));
-		tm->tm_hour	= bcd2bin(ops->readbyte(M48T86_REG_HOUR) & 0x3F);
-		tm->tm_mday	= bcd2bin(ops->readbyte(M48T86_REG_DOM));
+		tm->tm_sec	= bcd2bin(m48t86_rtc_readbyte(dev, M48T86_REG_SEC));
+		tm->tm_min	= bcd2bin(m48t86_rtc_readbyte(dev, M48T86_REG_MIN));
+		tm->tm_hour	= bcd2bin(m48t86_rtc_readbyte(dev, M48T86_REG_HOUR) & 0x3F);
+		tm->tm_mday	= bcd2bin(m48t86_rtc_readbyte(dev, M48T86_REG_DOM));
 		/* tm_mon is 0-11 */
-		tm->tm_mon	= bcd2bin(ops->readbyte(M48T86_REG_MONTH)) - 1;
-		tm->tm_year	= bcd2bin(ops->readbyte(M48T86_REG_YEAR)) + 100;
-		tm->tm_wday	= bcd2bin(ops->readbyte(M48T86_REG_DOW));
+		tm->tm_mon	= bcd2bin(m48t86_rtc_readbyte(dev, M48T86_REG_MONTH)) - 1;
+		tm->tm_year	= bcd2bin(m48t86_rtc_readbyte(dev, M48T86_REG_YEAR)) + 100;
+		tm->tm_wday	= bcd2bin(m48t86_rtc_readbyte(dev, M48T86_REG_DOW));
 	}
 
 	/* correct the hour if the clock is in 12h mode */
 	if (!(reg & M48T86_REG_B_H24))
-		if (ops->readbyte(M48T86_REG_HOUR) & 0x80)
+		if (m48t86_rtc_readbyte(dev, M48T86_REG_HOUR) & 0x80)
 			tm->tm_hour += 12;
 
 	return rtc_valid_tm(tm);
@@ -83,38 +116,36 @@  static int m48t86_rtc_read_time(struct device *dev, struct rtc_time *tm)
 static int m48t86_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
 	unsigned char reg;
-	struct platform_device *pdev = to_platform_device(dev);
-	struct m48t86_ops *ops = pdev->dev.platform_data;
 
-	reg = ops->readbyte(M48T86_REG_B);
+	reg = m48t86_rtc_readbyte(dev, M48T86_REG_B);
 
 	/* update flag and 24h mode */
 	reg |= M48T86_REG_B_SET | M48T86_REG_B_H24;
-	ops->writebyte(reg, M48T86_REG_B);
+	m48t86_rtc_writebyte(dev, reg, M48T86_REG_B);
 
 	if (reg & M48T86_REG_B_DM) {
 		/* data (binary) mode */
-		ops->writebyte(tm->tm_sec, M48T86_REG_SEC);
-		ops->writebyte(tm->tm_min, M48T86_REG_MIN);
-		ops->writebyte(tm->tm_hour, M48T86_REG_HOUR);
-		ops->writebyte(tm->tm_mday, M48T86_REG_DOM);
-		ops->writebyte(tm->tm_mon + 1, M48T86_REG_MONTH);
-		ops->writebyte(tm->tm_year % 100, M48T86_REG_YEAR);
-		ops->writebyte(tm->tm_wday, M48T86_REG_DOW);
+		m48t86_rtc_writebyte(dev, tm->tm_sec, M48T86_REG_SEC);
+		m48t86_rtc_writebyte(dev, tm->tm_min, M48T86_REG_MIN);
+		m48t86_rtc_writebyte(dev, tm->tm_hour, M48T86_REG_HOUR);
+		m48t86_rtc_writebyte(dev, tm->tm_mday, M48T86_REG_DOM);
+		m48t86_rtc_writebyte(dev, tm->tm_mon + 1, M48T86_REG_MONTH);
+		m48t86_rtc_writebyte(dev, tm->tm_year % 100, M48T86_REG_YEAR);
+		m48t86_rtc_writebyte(dev, tm->tm_wday, M48T86_REG_DOW);
 	} else {
 		/* bcd mode */
-		ops->writebyte(bin2bcd(tm->tm_sec), M48T86_REG_SEC);
-		ops->writebyte(bin2bcd(tm->tm_min), M48T86_REG_MIN);
-		ops->writebyte(bin2bcd(tm->tm_hour), M48T86_REG_HOUR);
-		ops->writebyte(bin2bcd(tm->tm_mday), M48T86_REG_DOM);
-		ops->writebyte(bin2bcd(tm->tm_mon + 1), M48T86_REG_MONTH);
-		ops->writebyte(bin2bcd(tm->tm_year % 100), M48T86_REG_YEAR);
-		ops->writebyte(bin2bcd(tm->tm_wday), M48T86_REG_DOW);
+		m48t86_rtc_writebyte(dev, bin2bcd(tm->tm_sec), M48T86_REG_SEC);
+		m48t86_rtc_writebyte(dev, bin2bcd(tm->tm_min), M48T86_REG_MIN);
+		m48t86_rtc_writebyte(dev, bin2bcd(tm->tm_hour), M48T86_REG_HOUR);
+		m48t86_rtc_writebyte(dev, bin2bcd(tm->tm_mday), M48T86_REG_DOM);
+		m48t86_rtc_writebyte(dev, bin2bcd(tm->tm_mon + 1), M48T86_REG_MONTH);
+		m48t86_rtc_writebyte(dev, bin2bcd(tm->tm_year % 100), M48T86_REG_YEAR);
+		m48t86_rtc_writebyte(dev, bin2bcd(tm->tm_wday), M48T86_REG_DOW);
 	}
 
 	/* update ended */
 	reg &= ~M48T86_REG_B_SET;
-	ops->writebyte(reg, M48T86_REG_B);
+	m48t86_rtc_writebyte(dev, reg, M48T86_REG_B);
 
 	return 0;
 }
@@ -122,15 +153,13 @@  static int m48t86_rtc_set_time(struct device *dev, struct rtc_time *tm)
 static int m48t86_rtc_proc(struct device *dev, struct seq_file *seq)
 {
 	unsigned char reg;
-	struct platform_device *pdev = to_platform_device(dev);
-	struct m48t86_ops *ops = pdev->dev.platform_data;
 
-	reg = ops->readbyte(M48T86_REG_B);
+	reg = m48t86_rtc_readbyte(dev, M48T86_REG_B);
 
 	seq_printf(seq, "mode\t\t: %s\n",
 		 (reg & M48T86_REG_B_DM) ? "binary" : "bcd");
 
-	reg = ops->readbyte(M48T86_REG_D);
+	reg = m48t86_rtc_readbyte(dev, M48T86_REG_D);
 
 	seq_printf(seq, "battery\t\t: %s\n",
 		 (reg & M48T86_REG_D_VRT) ? "ok" : "exhausted");
@@ -144,34 +173,121 @@  static const struct rtc_class_ops m48t86_rtc_ops = {
 	.proc		= m48t86_rtc_proc,
 };
 
-static int m48t86_rtc_probe(struct platform_device *dev)
+static int m48t86_rtc_probe(struct platform_device *pdev)
 {
 	unsigned char reg;
-	struct m48t86_ops *ops = dev->dev.platform_data;
-	struct rtc_device *rtc = rtc_device_register("m48t86",
-				&dev->dev, &m48t86_rtc_ops, THIS_MODULE);
-
-	if (IS_ERR(rtc))
-		return PTR_ERR(rtc);
-
-	platform_set_drvdata(dev, rtc);
+	int err;
+	struct resource *res_index, *res_data;
+	struct m48t86_rtc_private_data *priv;
+
+	/* Allocate memory for the device structure (and zero it) */
+	priv = kzalloc(sizeof(struct m48t86_rtc_private_data), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, priv);
+
+	if (!pdev->dev.platform_data) {
+		res_index = platform_get_resource_byname(
+					pdev, IORESOURCE_MEM, "rtc_index");
+		if (!res_index) {
+			err = -ENXIO;
+			goto out_free;
+		}
+
+		res_data = platform_get_resource_byname(
+					pdev, IORESOURCE_MEM, "rtc_data");
+		if (!res_data) {
+			err = -ENXIO;
+			goto out_free;
+		}
+
+		if (!request_mem_region(res_index->start,
+					resource_size(res_index),
+					dev_name(&pdev->dev))) {
+			err = -EBUSY;
+			goto out_free;
+		}
+
+		if (!request_mem_region(res_data->start,
+					resource_size(res_data),
+					dev_name(&pdev->dev))) {
+			err = -EBUSY;
+			goto out_release_index;
+		}
+
+		priv->io_index = ioremap(res_index->start,
+					resource_size(res_index));
+		if (!priv->io_index) {
+			err = -EIO;
+			goto out_release_data;
+		}
+
+		priv->io_data = ioremap(res_data->start,
+					resource_size(res_data));
+		if (!priv->io_data) {
+			err = -EIO;
+			goto out_io_index;
+		}
+	} else
+		priv->ops = pdev->dev.platform_data;
+
+	priv->rtc = rtc_device_register("m48t86",
+				&pdev->dev, &m48t86_rtc_ops, THIS_MODULE);
+	if (IS_ERR(priv->rtc)) {
+		err = PTR_ERR(priv->rtc);
+		if (!pdev->dev.platform_data)
+			goto out_io_data;
+		else
+			goto out_free;
+	}
 
 	/* read battery status */
-	reg = ops->readbyte(M48T86_REG_D);
-	dev_info(&dev->dev, "battery %s\n",
+	reg = m48t86_rtc_readbyte(&pdev->dev, M48T86_REG_D);
+	dev_info(&pdev->dev, "battery %s\n",
 		(reg & M48T86_REG_D_VRT) ? "ok" : "exhausted");
 
 	return 0;
+
+out_io_data:
+	iounmap(priv->io_data);
+out_io_index:
+	iounmap(priv->io_index);
+out_release_data:
+	release_mem_region(res_data->start, resource_size(res_data));
+out_release_index:
+	release_mem_region(res_index->start, resource_size(res_index));
+out_free:
+	platform_set_drvdata(pdev, NULL);
+	kfree(priv);
+	return err;
 }
 
-static int m48t86_rtc_remove(struct platform_device *dev)
+static int m48t86_rtc_remove(struct platform_device *pdev)
 {
-	struct rtc_device *rtc = platform_get_drvdata(dev);
+	struct resource *res;
+	struct m48t86_rtc_private_data *priv = platform_get_drvdata(pdev);
+
+	if (priv->rtc)
+		rtc_device_unregister(priv->rtc);
+
+	if (priv->io_data) {
+		iounmap(priv->io_data);
+		res = platform_get_resource_byname(
+					pdev, IORESOURCE_MEM, "rtc_data");
+		release_mem_region(res->start, resource_size(res));
+	}
+
+	if (priv->io_index) {
+		iounmap(priv->io_index);
+		res = platform_get_resource_byname(
+					pdev, IORESOURCE_MEM, "rtc_index");
+		release_mem_region(res->start, resource_size(res));
+	}
 
- 	if (rtc)
-		rtc_device_unregister(rtc);
+	platform_set_drvdata(pdev, NULL);
 
-	platform_set_drvdata(dev, NULL);
+	kfree(priv);
 
 	return 0;
 }