[RFC] rtc: rtc-pl030 fixes

- remove empty ioctl routine
- use __devinit/__devexit

Signed-off-by: Alessandro Zummo <a.zummo@towertech.it>
Cc: Russell King <rmk@arm.linux.org.uk>
---

 drivers/rtc/rtc-pl030.c |   21 +++++++--------------
 1 files changed, 7 insertions(+), 14 deletions(-)



--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
-~----------~----~----~----~------~----~------~--~---

On Mon, Nov 17, 2008 at 10:48:08PM +0100, Alessandro Zummo wrote:
> - remove empty ioctl routine
> - use __devinit/__devexit
> 
> Signed-off-by: Alessandro Zummo <a.zummo@towertech.it>
> Cc: Russell King <rmk@arm.linux.org.uk>
> ---
> 
>  drivers/rtc/rtc-pl030.c |   21 +++++++--------------
>  1 files changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-pl030.c b/drivers/rtc/rtc-pl030.c
> index 8261535..9c0c930 100644
> --- a/drivers/rtc/rtc-pl030.c
> +++ b/drivers/rtc/rtc-pl030.c
> @@ -1,12 +1,13 @@
>  /*
> - *  linux/drivers/rtc/rtc-pl030.c
> + * rtc-pl030.c
>   *
> - *  Copyright (C) 2000-2001 Deep Blue Solutions Ltd.
> + * Copyright (C) 2000-2001 Deep Blue Solutions Ltd.
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
>   */
> +

This is just random formatting changes for no real benefit.

>  #include <linux/module.h>
>  #include <linux/rtc.h>
>  #include <linux/init.h>
> @@ -34,11 +35,6 @@ static irqreturn_t pl030_interrupt(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> -static int pl030_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
> -{
> -	return -ENOIOCTLCMD;
> -}
> -

Ok.

>  static int pl030_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>  {
>  	struct pl030_rtc *rtc = dev_get_drvdata(dev);
> @@ -69,7 +65,6 @@ static int pl030_read_time(struct device *dev, struct rtc_time *tm)
>  	struct pl030_rtc *rtc = dev_get_drvdata(dev);
>  
>  	rtc_time_to_tm(readl(rtc->base + RTC_DR), tm);
> -

More pointless changes.

>  	return 0;
>  }
>  
> @@ -95,14 +90,13 @@ static int pl030_set_time(struct device *dev, struct rtc_time *tm)
>  }
>  
>  static const struct rtc_class_ops pl030_ops = {
> -	.ioctl		= pl030_ioctl,

Ok.

>  	.read_time	= pl030_read_time,
>  	.set_time	= pl030_set_time,
>  	.read_alarm	= pl030_read_alarm,
>  	.set_alarm	= pl030_set_alarm,
>  };
>  
> -static int pl030_probe(struct amba_device *dev, void *id)
> +static int __devinit pl030_probe(struct amba_device *dev, void *id)

Ok.

>  {
>  	struct pl030_rtc *rtc;
>  	int ret;
> @@ -154,16 +148,15 @@ static int pl030_probe(struct amba_device *dev, void *id)
>  	return ret;
>  }
>  
> -static int pl030_remove(struct amba_device *dev)
> +static int __devexit pl030_remove(struct amba_device *dev)

Ok.

>  {
>  	struct pl030_rtc *rtc = amba_get_drvdata(dev);
>  
> -	amba_set_drvdata(dev, NULL);
> -
>  	writel(0, rtc->base + RTC_CR);
>  
>  	free_irq(dev->irq[0], rtc);
>  	rtc_device_unregister(rtc->rtc);
> +	amba_set_drvdata(dev, NULL);

You haven't explained why this is necessary.

>  	iounmap(rtc->base);
>  	kfree(rtc);
>  	amba_release_regions(dev);
> @@ -184,7 +177,7 @@ static struct amba_driver pl030_driver = {
>  		.name	= "rtc-pl030",
>  	},
>  	.probe		= pl030_probe,
> -	.remove		= pl030_remove,
> +	.remove		= __devexit_p(pl030_remove),

Ok.

On Mon, 17 Nov 2008 22:05:01 +0000
Russell King <rmk@arm.linux.org.uk> wrote:

>  -	amba_set_drvdata(dev, NULL);
> > -
> >  	writel(0, rtc->base + RTC_CR);
> >  
> >  	free_irq(dev->irq[0], rtc);
> >  	rtc_device_unregister(rtc->rtc);
> > +	amba_set_drvdata(dev, NULL);  
> 
> You haven't explained why this is necessary.

 I did it a while ago, I guess my intention was to remove 
 the call. Nobody else should be using the drvdata pointer, right?

On Mon, Nov 17, 2008 at 11:17:39PM +0100, Alessandro Zummo wrote:
> On Mon, 17 Nov 2008 22:05:01 +0000
> Russell King <rmk@arm.linux.org.uk> wrote:
> 
> >  -	amba_set_drvdata(dev, NULL);
> > > -
> > >  	writel(0, rtc->base + RTC_CR);
> > >  
> > >  	free_irq(dev->irq[0], rtc);
> > >  	rtc_device_unregister(rtc->rtc);
> > > +	amba_set_drvdata(dev, NULL);  
> > 
> > You haven't explained why this is necessary.
> 
>  I did it a while ago, I guess my intention was to remove 
>  the call. Nobody else should be using the drvdata pointer, right? 

There is a valid viewpoint that long lived pointers to data which
become no longer valid should not be left dangling for someone to
access.

The important part of your statement is "should".  What if someone
does - consider the possible effects with and without that line being
present.

On Mon, 17 Nov 2008 22:25:27 +0000
Russell King <rmk@arm.linux.org.uk> wrote:

> > >  -	amba_set_drvdata(dev, NULL);
> > > > -
> > > >  	writel(0, rtc->base + RTC_CR);
> > > >  
> > > >  	free_irq(dev->irq[0], rtc);
> > > >  	rtc_device_unregister(rtc->rtc);
> > > > +	amba_set_drvdata(dev, NULL);  
> > > 
> > > You haven't explained why this is necessary.
> > 
> >  I did it a while ago, I guess my intention was to remove 
> >  the call. Nobody else should be using the drvdata pointer, right? 
> 
> There is a valid viewpoint that long lived pointers to data which
> become no longer valid should not be left dangling for someone to
> access.
> 
> The important part of your statement is "should".  What if someone
> does - consider the possible effects with and without that line being
> present.

 There might be effects, but given that the device model is known
 and predictable, I'm sure nobody will touch that pointer. 

 Neither the driver nor the kernel are using it, so it's safe.
 But you're the author, so I'll leave it there.