[PATCH] rtc: Added support for ds1672 control

[PATCH] rtc: Added support for ds1672 control

[Kumar Gala]

* Always enable the oscillator when we set the time
* If the oscillator is disable when we probe the RTC report back a warning
  to the user
* Added sysfs attribute to represent the state of the oscillator

Signed-off-by: Kumar Gala <galak@kernel.crashing.org>

---
commit fde7a0afd6f8dca9b498aea689de3ca409c16732
tree c390aefedb2073d8a107ec70efe551993e51c943
parent 329b10bb0feacb7fb9a41389313ff0a51ae56f2a
author Kumar Gala <galak@kernel.crashing.org> Tue, 28 Mar 2006 15:07:03 -0600
committer Kumar Gala <galak@kernel.crashing.org> Tue, 28 Mar 2006 15:07:03 -0600

 drivers/rtc/rtc-ds1672.c |   51 +++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/rtc-ds1672.c b/drivers/rtc/rtc-ds1672.c
index 358695a..283d19f 100644
--- a/drivers/rtc/rtc-ds1672.c
+++ b/drivers/rtc/rtc-ds1672.c
@@ -72,16 +72,17 @@ static int ds1672_get_datetime(struct i2
 static int ds1672_set_mmss(struct i2c_client *client, unsigned long secs)
 {
 	int xfer;
-	unsigned char buf[5];
+	unsigned char buf[6];
 
 	buf[0] = DS1672_REG_CNT_BASE;
 	buf[1] = secs & 0x000000FF;
 	buf[2] = (secs & 0x0000FF00) >> 8;
 	buf[3] = (secs & 0x00FF0000) >> 16;
 	buf[4] = (secs & 0xFF000000) >> 24;
+	buf[5] = 0;
 
-	xfer = i2c_master_send(client, buf, 5);
-	if (xfer != 5) {
+	xfer = i2c_master_send(client, buf, 6);
+	if (xfer != 6) {
 		dev_err(&client->dev, "%s: send: %d\n", __FUNCTION__, xfer);
 		return -EIO;
 	}
@@ -120,6 +121,37 @@ static int ds1672_rtc_set_mmss(struct de
 	return ds1672_set_mmss(to_i2c_client(dev), secs);
 }
 
+static int ds1672_get_control(struct i2c_client *client)
+{
+	unsigned char addr = DS1672_REG_CONTROL;
+	unsigned char val = 0;
+
+	struct i2c_msg msgs[] = {
+		{ client->addr, 0, 1, &addr },		/* setup read ptr */
+		{ client->addr, I2C_M_RD, 1, &val },	/* read control */
+	};
+
+	/* read control register */
+	if ((i2c_transfer(client->adapter, &msgs[0], 2)) != 2) {
+		dev_err(&client->dev, "%s: read error\n", __FUNCTION__);
+		return -EIO;
+	} else
+		return val;
+}
+
+/* following are the sysfs callback functions */
+static ssize_t show_control(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	char *state = "enabled";
+
+	if (ds1672_get_control(client))
+		state = "disabled";
+	return sprintf(buf, "%s\n", state);
+}
+
+static DEVICE_ATTR(control, S_IRUGO, show_control, NULL);
+
 static struct rtc_class_ops ds1672_rtc_ops = {
 	.read_time	= ds1672_rtc_read_time,
 	.set_time	= ds1672_rtc_set_time,
@@ -202,6 +234,19 @@ static int ds1672_probe(struct i2c_adapt
 
 	i2c_set_clientdata(client, rtc);
 
+	/* read control register */
+	err = ds1672_get_control(client);
+	if (err < 0) {
+		dev_err(&client->dev, "%s: read error\n", __FUNCTION__);
+		goto exit_detach;
+	}
+	if (err > 0)
+		dev_warn(&client->dev, "Oscillator not enabled. "
+					"Set time to enable\n");
+
+	/* Register sysfs hooks */
+	device_create_file(&client->dev, &dev_attr_control);
+
 	return 0;
 
 exit_detach:

Re: [PATCH] rtc: Added support for ds1672 control

[Alessandro Zummo]

On Tue, 28 Mar 2006 15:07:33 -0600 (CST)
Kumar Gala <galak@kernel.crashing.org> wrote:

 is almost ok, please check my comments below:

>  
>  	buf[0] = DS1672_REG_CNT_BASE;
>  	buf[1] = secs & 0x000000FF;
>  	buf[2] = (secs & 0x0000FF00) >> 8;
>  	buf[3] = (secs & 0x00FF0000) >> 16;
>  	buf[4] = (secs & 0xFF000000) >> 24;
> +	buf[5] = 0;

 I'd add a comment to say that 0 enables the osc.

> +static int ds1672_get_control(struct i2c_client *client)
> +{

[..]

> +	} else
> +		return val;
> +}

 I think it would be cleaner to define the routine as follow:
.. ds1672_get_control(...., unsigned char *status)

 and to usa the space provided by the caller to store the result.

> +	if (ds1672_get_control(client))
> +		state = "disabled";
> +	return sprintf(buf, "%s\n", state);
> +}

please #define DS1672_REG_CONTROL_EOSC 0x80
and check the single bit.


> +	err = ds1672_get_control(client);

 ditto.


 thanks for your work!

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Turin, Italy

  http://www.towertech.it

Re: [PATCH] rtc: Added support for ds1672 control

[Kumar Gala]

On Mar 28, 2006, at 3:40 PM, Alessandro Zummo wrote:

> On Tue, 28 Mar 2006 15:07:33 -0600 (CST)
> Kumar Gala <galak@kernel.crashing.org> wrote:
>
>  is almost ok, please check my comments below:
>
>>
>>  	buf[0] = DS1672_REG_CNT_BASE;
>>  	buf[1] = secs & 0x000000FF;
>>  	buf[2] = (secs & 0x0000FF00) >> 8;
>>  	buf[3] = (secs & 0x00FF0000) >> 16;
>>  	buf[4] = (secs & 0xFF000000) >> 24;
>> +	buf[5] = 0;
>
>  I'd add a comment to say that 0 enables the osc.

ack.

>> +static int ds1672_get_control(struct i2c_client *client)
>> +{
>
> [..]
>
>> +	} else
>> +		return val;
>> +}
>
>  I think it would be cleaner to define the routine as follow:
> .. ds1672_get_control(...., unsigned char *status)
>
>  and to usa the space provided by the caller to store the result.
>
>> +	if (ds1672_get_control(client))
>> +		state = "disabled";
>> +	return sprintf(buf, "%s\n", state);
>> +}

Not sure if I follow you here.  I think you are suggesting for  
ds1672_get_control to look like:

static int ds1672_get_control(struct i2c_client *client, u8 *status)
{
         unsigned char addr = DS1672_REG_CONTROL;

         struct i2c_msg msgs[] = {
                 { client->addr, 0, 1, &addr },          /* setup  
read ptr */
                 { client->addr, I2C_M_RD, 1, status },  /* read  
control */
         };
...
}

which is fine.  Any suggestions on what to do on an error in the  
sysfs attrib function?

> please #define DS1672_REG_CONTROL_EOSC 0x80
> and check the single bit.

ack

>> +	err = ds1672_get_control(client);
>
>  ditto.
>
>
>  thanks for your work!

no problem.

- k

Re: [PATCH] rtc: Added support for ds1672 control

[Alessandro Zummo]

On Tue, 28 Mar 2006 16:04:20 -0600
Kumar Gala <galak@kernel.crashing.org> wrote:

> Not sure if I follow you here.  I think you are suggesting for  
> ds1672_get_control to look like:
> 
> static int ds1672_get_control(struct i2c_client *client, u8 *status)
> {
>          unsigned char addr = DS1672_REG_CONTROL;
> 
>          struct i2c_msg msgs[] = {
>                  { client->addr, 0, 1, &addr },          /* setup  
> read ptr */
>                  { client->addr, I2C_M_RD, 1, status },  /* read  
> control */
>          };
> ...
> }

 yes.


> which is fine.  Any suggestions on what to do on an error in the  
> sysfs attrib function?

 i2c_transfer will give the errno to return back.
 This remembers me that I forgot to do that in other drivers :(

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Turin, Italy

  http://www.towertech.it

[PATCH][UPDATE] rtc: Added support for ds1672 control

[Kumar Gala]

* Always enable the oscillator when we set the time
* If the oscillator is disable when we probe the RTC report back a warning
  to the user
* Added sysfs attribute to represent the state of the oscillator

Signed-off-by: Kumar Gala <galak@kernel.crashing.org>

---
commit 3acc1a4629e70bce7493b5463f3c785379ae8b6b
tree 34463406603ac788697d8d228a2caf2c0ea40b3a
parent 329b10bb0feacb7fb9a41389313ff0a51ae56f2a
author Kumar Gala <galak@kernel.crashing.org> Tue, 28 Mar 2006 16:53:43 -0600
committer Kumar Gala <galak@kernel.crashing.org> Tue, 28 Mar 2006 16:53:43 -0600

 drivers/rtc/rtc-ds1672.c |   61 ++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 58 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/rtc-ds1672.c b/drivers/rtc/rtc-ds1672.c
index 358695a..6d2dc70 100644
--- a/drivers/rtc/rtc-ds1672.c
+++ b/drivers/rtc/rtc-ds1672.c
@@ -23,6 +23,7 @@ I2C_CLIENT_INSMOD;
 
 #define DS1672_REG_CNT_BASE	0
 #define DS1672_REG_CONTROL	4
+#define DS1672_REG_CONTROL_EOSC	0x80
 #define DS1672_REG_TRICKLE	5
 
 
@@ -72,16 +73,17 @@ static int ds1672_get_datetime(struct i2
 static int ds1672_set_mmss(struct i2c_client *client, unsigned long secs)
 {
 	int xfer;
-	unsigned char buf[5];
+	unsigned char buf[6];
 
 	buf[0] = DS1672_REG_CNT_BASE;
 	buf[1] = secs & 0x000000FF;
 	buf[2] = (secs & 0x0000FF00) >> 8;
 	buf[3] = (secs & 0x00FF0000) >> 16;
 	buf[4] = (secs & 0xFF000000) >> 24;
+	buf[5] = 0;	/* set control reg to enable counting */
 
-	xfer = i2c_master_send(client, buf, 5);
-	if (xfer != 5) {
+	xfer = i2c_master_send(client, buf, 6);
+	if (xfer != 6) {
 		dev_err(&client->dev, "%s: send: %d\n", __FUNCTION__, xfer);
 		return -EIO;
 	}
@@ -120,6 +122,44 @@ static int ds1672_rtc_set_mmss(struct de
 	return ds1672_set_mmss(to_i2c_client(dev), secs);
 }
 
+static int ds1672_get_control(struct i2c_client *client, u8 *status)
+{
+	unsigned char addr = DS1672_REG_CONTROL;
+
+	struct i2c_msg msgs[] = {
+		{ client->addr, 0, 1, &addr },		/* setup read ptr */
+		{ client->addr, I2C_M_RD, 1, status },	/* read control */
+	};
+
+	/* read control register */
+	if ((i2c_transfer(client->adapter, &msgs[0], 2)) != 2) {
+		dev_err(&client->dev, "%s: read error\n", __FUNCTION__);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+/* following are the sysfs callback functions */
+static ssize_t show_control(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	char *state = "enabled";
+	u8 control;
+	int err;
+
+	err = ds1672_get_control(client, &control);
+	if (err)
+		return err;
+
+	if (control & DS1672_REG_CONTROL_EOSC)
+		state = "disabled";
+
+	return sprintf(buf, "%s\n", state);
+}
+
+static DEVICE_ATTR(control, S_IRUGO, show_control, NULL);
+
 static struct rtc_class_ops ds1672_rtc_ops = {
 	.read_time	= ds1672_rtc_read_time,
 	.set_time	= ds1672_rtc_set_time,
@@ -162,6 +202,7 @@ static struct i2c_driver ds1672_driver =
 static int ds1672_probe(struct i2c_adapter *adapter, int address, int kind)
 {
 	int err = 0;
+	u8 control;
 	struct i2c_client *client;
 	struct rtc_device *rtc;
 
@@ -202,6 +243,20 @@ static int ds1672_probe(struct i2c_adapt
 
 	i2c_set_clientdata(client, rtc);
 
+	/* read control register */
+	err = ds1672_get_control(client, &control);
+	if (err) {
+		dev_err(&client->dev, "%s: read error\n", __FUNCTION__);
+		goto exit_detach;
+	}
+
+	if (control & DS1672_REG_CONTROL_EOSC)
+		dev_warn(&client->dev, "Oscillator not enabled. "
+					"Set time to enable.\n");
+
+	/* Register sysfs hooks */
+	device_create_file(&client->dev, &dev_attr_control);
+
 	return 0;
 
 exit_detach:

Re: [PATCH][UPDATE] rtc: Added support for ds1672 control

[Alessandro Zummo]

On Tue, 28 Mar 2006 16:55:01 -0600 (CST)
Kumar Gala <galak@kernel.crashing.org> wrote:

> +/* following are the sysfs callback functions */
> +static ssize_t show_control(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	char *state = "enabled";
> +	u8 control;
> +	int err;
> +
> +	err = ds1672_get_control(client, &control);
> +	if (err)
> +		return err;

 shouldn't this be
 if (err < 0)
	return err;

 ?

> +	/* read control register */
> +	err = ds1672_get_control(client, &control);
> +	if (err) {
> +		dev_err(&client->dev, "%s: read error\n", __FUNCTION__);
> +		goto exit_detach;
> +	}

 ditto.

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Turin, Italy

  http://www.towertech.it

Re: [PATCH][UPDATE] rtc: Added support for ds1672 control

[Kumar Gala]

On Mar 28, 2006, at 5:48 PM, Alessandro Zummo wrote:

> On Tue, 28 Mar 2006 16:55:01 -0600 (CST)
> Kumar Gala <galak@kernel.crashing.org> wrote:
>
>> +/* following are the sysfs callback functions */
>> +static ssize_t show_control(struct device *dev, struct  
>> device_attribute *attr, char *buf)
>> +{
>> +	struct i2c_client *client = to_i2c_client(dev);
>> +	char *state = "enabled";
>> +	u8 control;
>> +	int err;
>> +
>> +	err = ds1672_get_control(client, &control);
>> +	if (err)
>> +		return err;
>
>  shouldn't this be
>  if (err < 0)
> 	return err;

It could be, but doesn't need to.  ds1672_get_control either returns  
0 (success) or non-zero (-EIO) for failure.

>> +	/* read control register */
>> +	err = ds1672_get_control(client, &control);
>> +	if (err) {
>> +		dev_err(&client->dev, "%s: read error\n", __FUNCTION__);
>> +		goto exit_detach;
>> +	}
>
>  ditto.

ditto.

- kumar

Re: [PATCH][UPDATE] rtc: Added support for ds1672 control

[Alessandro Zummo]

On Tue, 28 Mar 2006 17:52:00 -0600
Kumar Gala <galak@kernel.crashing.org> wrote:

> >
> >  shouldn't this be
> >  if (err < 0)
> > 	return err;
> 
> It could be, but doesn't need to.  ds1672_get_control either returns  
> 0 (success) or non-zero (-EIO) for failure.
> 
> >> +	/* read control register */
> >> +	err = ds1672_get_control(client, &control);
> >> +	if (err) {
> >> +		dev_err(&client->dev, "%s: read error\n", __FUNCTION__);
> >> +		goto exit_detach;
> >> +	}
> >
> >  ditto.
> 
> ditto.

 ok. will apply, thanks.


-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Turin, Italy

  http://www.towertech.it

Re: [PATCH][UPDATE] rtc: Added support for ds1672 control

[Kumar Gala]

On Mar 28, 2006, at 5:57 PM, Alessandro Zummo wrote:

> On Tue, 28 Mar 2006 17:52:00 -0600
> Kumar Gala <galak@kernel.crashing.org> wrote:
>
>>>
>>>  shouldn't this be
>>>  if (err < 0)
>>> 	return err;
>>
>> It could be, but doesn't need to.  ds1672_get_control either returns
>> 0 (success) or non-zero (-EIO) for failure.
>>
>>>> +	/* read control register */
>>>> +	err = ds1672_get_control(client, &control);
>>>> +	if (err) {
>>>> +		dev_err(&client->dev, "%s: read error\n", __FUNCTION__);
>>>> +		goto exit_detach;
>>>> +	}
>>>
>>>  ditto.
>>
>> ditto.
>
>  ok. will apply, thanks.

Do you have a queue of patches for 2.6.17 or should I send this to  
Andrew to get into 2.6.17?

- kumar

Re: [PATCH][UPDATE] rtc: Added support for ds1672 control

[Alessandro Zummo]

On Thu, 30 Mar 2006 14:06:22 -0600
Kumar Gala <galak@kernel.crashing.org> wrote:

> >>
> >> ditto.
> >
> >  ok. will apply, thanks.
> 
> Do you have a queue of patches for 2.6.17 or should I send this to  
> Andrew to get into 2.6.17?

 I have a queue. Will send it to Andrew tomorrow.

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Turin, Italy

  http://www.towertech.it