rtc: pcf85063Add support for different loads
PATCH v2 4/4 rtc: pcf85063Add support for different loads
--- drivers/rtc/rtc-pcf85063.c | 37 +++++++++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 6 deletions(-)
drivers/rtc/rtc-pcf85063.c +31 -6
@@ -16,10 +16,12 @@#include <linux/rtc.h> #include <linux/module.h> -#define DRV_VERSION "0.0.1" +#define DRV_VERSION "0.0.2"#define PCF85063_REG_CTRL1 0x00/* status */ +#define PCF85063_REG_CTRL1_CAP_SEL (1 << 0)#define PCF85063_REG_CTRL2 0x01 +#define PCF85063_REG_OFFSET 0x02#define PCF85063_REG_SC 0x04/* datetime */ #define PCF85063_REG_MN 0x05
@@ -94,10 +96,6 @@ static int pcf85063_set_datetime(struct i2c_client *client, struct rtc_time *tm)int i =0 , err =0 ;unsigned char buf[11 ]; -/* Control & status */ - buf[PCF85063_REG_CTRL1 ] =0 ; - buf[PCF85063_REG_CTRL2 ] =5 ; -/* hours, minutes and seconds */ buf[PCF85063_REG_SC ] =bin2bcd (tm->tm_sec) &0x7F ;
@@ -117,7 +115,7 @@ static int pcf85063_set_datetime(struct i2c_client *client, struct rtc_time *tm) buf[PCF85063_REG_YR ] =bin2bcd (tm->tm_year %100 );/* write register's data */ -for (i =0 ; i <sizeof (buf); i++) { +for (i =PCF85063_REG_SC ; i <sizeof (buf); i++) {unsigned char data[2 ] = { i, buf[i] }; err =i2c_master_send (client, data,sizeof (data));
@@ -149,7 +147,14 @@ static const struct rtc_class_ops pcf85063_rtc_ops = {static int pcf85063_probe (struct i2c_client *client,const struct i2c_device_id *id) { +int i =0 , err =0 ;struct pcf85063 *pcf85063; +unsigned char buf[3 ]; + +/* Control & status */ + buf[PCF85063_REG_CTRL1 ] =0 ; + buf[PCF85063_REG_CTRL2 ] =7 ; + buf[PCF85063_REG_OFFSET ] =0x00 ;dev_dbg (&client->dev,"%s\n" , __func__);
@@ -169,6 +174,26 @@ static int pcf85063_probe(struct i2c_client *client, pcf85063_driver.driver.name, &pcf85063_rtc_ops, THIS_MODULE); +if (of_property_match_string (client->dev.of_node, +"quartz_load" ,"12.5pF" ) ==0 ) + buf[PCF85063_REG_CTRL1 ] |=PCF85063_REG_CTRL1_CAP_SEL ; + +if (of_property_match_string (client->dev.of_node, +"quartz_load" ,"7pF" ) ==0 ) + buf[PCF85063_REG_CTRL1 ] &= ~PCF85063_REG_CTRL1_CAP_SEL ; + +/* write register's data */ +for (i =0 ; i <sizeof (buf); i++) { +unsigned char data[3 ] = { i, buf[i] }; + + err =i2c_master_send (client, data,sizeof (data)); +if (err !=sizeof (data)) { +dev_err (&client->dev,"%s: err=%d addr=%02x, data=%02x\n" , + __func__, err, data[0 ], data[1 ]); +return -EIO ; + } + } +return PTR_ERR_OR_ZERO (pcf85063->rtc); }
Hi,
Please always include a commit log even if it is short.
Quoted 10 lines · Søren Andersen
Signed-off-by Soeren Andersen <san at rosetechnology.dk>--- drivers/rtc/rtc-pcf85063.c | 37 +++++++++++++++++++++++++++++++------ 1 file changed, 31 insertions(+), 6 deletions(-)drivers/rtc/rtc-pcf85063.c +0
-0
drivers/rtc/rtc-pcf85063.c line 20
Quoted 6 lines
drivers/rtc/rtc-pcf85063.c +1
-1@@ -16,10 +16,12 @@#include <linux/rtc.h> #include <linux/module.h> -#define DRV_VERSION "0.0.1" +#define DRV_VERSION "0.0.2"
Do we really care about a driver version that is separate from the kernel version?
drivers/rtc/rtc-pcf85063.c line 22
Quoted 2 lines
drivers/rtc/rtc-pcf85063.c +1
-0#define PCF85063_REG_CTRL1 0x00/* status */ +#define PCF85063_REG_CTRL1_CAP_SEL (1 << 0)
Please use the BIT() macro.
Quoted 26 lines
drivers/rtc/rtc-pcf85063.c +2
-5#define PCF85063_REG_CTRL2 0x01 +#define PCF85063_REG_OFFSET 0x02#define PCF85063_REG_SC 0x04/* datetime */ #define PCF85063_REG_MN 0x05@@ -94,10 +96,6 @@ static int pcf85063_set_datetime(struct i2c_client *client, struct rtc_time *tm)int i =0 , err =0 ;unsigned char buf[11 ]; -/* Control & status */ - buf[PCF85063_REG_CTRL1 ] =0 ; - buf[PCF85063_REG_CTRL2 ] =5 ; -/* hours, minutes and seconds */ buf[PCF85063_REG_SC ] =bin2bcd (tm->tm_sec) &0x7F ;@@ -117,7 +115,7 @@ static int pcf85063_set_datetime(struct i2c_client *client, struct rtc_time *tm) buf[PCF85063_REG_YR ] =bin2bcd (tm->tm_year %100 );/* write register's data */ -for (i =0 ; i <sizeof (buf); i++) { +for (i =PCF85063_REG_SC ; i <sizeof (buf); i++) {unsigned char data[2 ] = { i, buf[i] }; err =i2c_master_send (client, data,sizeof (data));
drivers/rtc/rtc-pcf85063.c static const struct rtc_class_ops pcf85063_rtc_ops = { · line 156
Quoted 10 lines
drivers/rtc/rtc-pcf85063.c +6
-0@@ -149,7 +147,14 @@ static const struct rtc_class_ops pcf85063_rtc_ops = {static int pcf85063_probe (struct i2c_client *client,const struct i2c_device_id *id) { +int i =0 , err =0 ;struct pcf85063 *pcf85063; +unsigned char buf[3 ]; + +/* Control & status */ + buf[PCF85063_REG_CTRL1 ] =0 ; + buf[PCF85063_REG_CTRL2 ] =7 ;
Bonus points if you get rid of that magic value and use defines ;)
Quoted 6 lines
drivers/rtc/rtc-pcf85063.c +1
-0+ buf[PCF85063_REG_OFFSET ] =0x00 ;dev_dbg (&client->dev,"%s\n" , __func__);@@ -169,6 +174,26 @@ static int pcf85063_probe(struct i2c_client *client, pcf85063_driver.driver.name,
drivers/rtc/rtc-pcf85063.c static int pcf85063_probe(struct i2c_client *client, · line 184
Quoted 10 lines
drivers/rtc/rtc-pcf85063.c +8
-0&pcf85063_rtc_ops, THIS_MODULE); +if (of_property_match_string (client->dev.of_node, +"quartz_load" ,"12.5pF" ) ==0 ) + buf[PCF85063_REG_CTRL1 ] |=PCF85063_REG_CTRL1_CAP_SEL ; + +if (of_property_match_string (client->dev.of_node, +"quartz_load" ,"7pF" ) ==0 ) + buf[PCF85063_REG_CTRL1 ] &= ~PCF85063_REG_CTRL1_CAP_SEL ; +
As this is not a generic property from the core, I would prefer that is it prefixed with the vendor, like nxp,quartz_load.
drivers/rtc/rtc-pcf85063.c static int pcf85063_probe(struct i2c_client *client, · line 196
Quoted 12 lines
drivers/rtc/rtc-pcf85063.c +12
-0+/* write register's data */ +for (i =0 ; i <sizeof (buf); i++) { +unsigned char data[3 ] = { i, buf[i] }; + + err =i2c_master_send (client, data,sizeof (data)); +if (err !=sizeof (data)) { +dev_err (&client->dev,"%s: err=%d addr=%02x, data=%02x\n" , + __func__, err, data[0 ], data[1 ]); +return -EIO ; + } + } +
It would be preferable if you could use the smbus api to read and set registers as it removes the need for that for loop and will handle retries and arbitration issues automatically. If you do that, please switch pcf85063_get_datetime and pcf85063_set_datetime in a preliminary patch.
Can you also squash patch 2 and 3?
Thanks!
Signature · 3 lines
Alexandre Belloni, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com