Re: [rtc-linux] [PATCH] Add the support of ST M48T59 RTC chip in rtc-class driver subsystem

On Mon, 11 Jun 2007 15:56:40 +0800
Mark Zhan [off-list ref] wrote:

Add the support of ST M48T59 RTC chip driver in RTC class subsystem for
Wind River SBC PowerQUICCII 82xx board

 Hello Mark, thanks for you contribution. Code looks
 good, I only have some minor comments:

 
+config RTC_DRV_M48T59
+	tristate "ST M48T59"
+	depends on RTC_CLASS
+	help
+	  If you say Y here you will get support for the
+	  ST M48T59 RTC chip.
+
+	  This driver can also be built as a module, if so, the module
+	  will be called "rtc-m48t59".

 Please add some infos about the Wind River board in the help.

+
+#ifdef DEBUG_M48T59
+#define DPRINTK(fmt, args...)	printk(KERN_DEBUG "%s: "fmt,
__FUNCTION__, ##args)
+#else
+#define DPRINTK(fmt, args...)
+#endif

 please use standard device debugging/reporting functions

+
+static unsigned char * m48t59_vbase = NULL;
+static unsigned int m48t59_irq = -1;

 those shouldn't be globals. please see other drivers for suggestions
 on encapsulation.

+static int m48t59_rtc_proc(struct device *dev, struct seq_file *seq)
+{
+	unsigned char val;
+
+	val = M48T59_READ(M48T59_FLAGS);
+	seq_printf(seq, "battery\t\t: %s\n",
+		 (val & M48T59_FLAGS_BF) ? "low" : "normal");
+
+	return 0;
+}

 this is going to be deprecated, you can use it but it will fade away
 sooner or later. you might want to add a sysfs attribute
 with the battery info.

 thanks.

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it