* 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 <[email protected]>
---
commit fde7a0afd6f8dca9b498aea689de3ca409c16732
tree c390aefedb2073d8a107ec70efe551993e51c943
parent 329b10bb0feacb7fb9a41389313ff0a51ae56f2a
author Kumar Gala <[email protected]> Tue, 28 Mar 2006 15:07:03 -0600
committer Kumar Gala <[email protected]> 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:
On Tue, 28 Mar 2006 15:07:33 -0600 (CST)
Kumar Gala <[email protected]> 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
On Mar 28, 2006, at 3:40 PM, Alessandro Zummo wrote:
> On Tue, 28 Mar 2006 15:07:33 -0600 (CST)
> Kumar Gala <[email protected]> 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
On Tue, 28 Mar 2006 16:04:20 -0600
Kumar Gala <[email protected]> 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
* 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 <[email protected]>
---
commit 3acc1a4629e70bce7493b5463f3c785379ae8b6b
tree 34463406603ac788697d8d228a2caf2c0ea40b3a
parent 329b10bb0feacb7fb9a41389313ff0a51ae56f2a
author Kumar Gala <[email protected]> Tue, 28 Mar 2006 16:53:43 -0600
committer Kumar Gala <[email protected]> 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:
On Tue, 28 Mar 2006 16:55:01 -0600 (CST)
Kumar Gala <[email protected]> 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
On Mar 28, 2006, at 5:48 PM, Alessandro Zummo wrote:
> On Tue, 28 Mar 2006 16:55:01 -0600 (CST)
> Kumar Gala <[email protected]> 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
On Tue, 28 Mar 2006 17:52:00 -0600
Kumar Gala <[email protected]> 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
On Mar 28, 2006, at 5:57 PM, Alessandro Zummo wrote:
> On Tue, 28 Mar 2006 17:52:00 -0600
> Kumar Gala <[email protected]> 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
On Thu, 30 Mar 2006 14:06:22 -0600
Kumar Gala <[email protected]> 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