[i2c] [PATCH 1/5] rtc: RTC class driver for the ds1374

[David Brownell]

Hi Scott,

Glad to see this.  :)

I suggest you have a look at the rtc-rs5c372.c driver for a slightly
less complete (no irq handler) example of an I2C based RTC with alarm
functionality.  In particular, how it handles the case of boards that
don't happen to have the IRQ wired up.

Comments below focus on things that haven't been raised already.

- Dave



On Thursday 17 May 2007, Scott Wood wrote:
> --- /dev/null
> +++ b/drivers/rtc/rtc-ds1374.c

> +struct ds1374 {
> +	struct i2c_client *client;
> +	struct rtc_device *rtc;
> +	struct work_struct work;
> +	struct mutex mutex;
> +	spinlock_t lock;

Both spinlock and mutex?  That looks wrong.  Previously
I've written I2C drivers that accept IRQs and only need
the mutex (well, it was a semaphore back then).


> +	int have_irq;

"have_irq" isn't needed.  If client->irq is nonnegative,
it's an error if you can't request_irq().  Else you've
got an IRQ. 

> +static u8 read_reg(struct i2c_client *client, int reg, int *err)

> +static void write_reg(struct i2c_client *client, int reg, u8 val, int *err)

The usual convention is to return negative errno ... for the read()
case you can use that too, since positive values can only mean "u8"
values.  (Same notion as ssize_t...)

Returning status indirectly ("int *err") is both confusing and
error prone ... avoid that idiom!!  In this case, I notice those
routines don't zero "*err" on success paths, so one confusion is
how to tell whether a nonzero "err" was from that call or from an
older one.  The confusion is eliminated by using the standard call
convention, returning a status code so that normal compiler checks
can report a variety of misuses.


> +static int ds1374_read_time(struct device *dev, struct rtc_time *time)
> +{
> +       u32 itime;
> +
> +       int ret = ds1374_read_rtc_retry(dev, &itime,
> +                                       DS1374_REG_TOD0,
> +                                       DS1374_REG_TOD3);
> +
> +       if (!ret)
> +               rtc_time_to_tm(itime, time);
> +
> +       return ret;
> +}

You might consider adding a base to that time ... e.g. January 1 2005,
before which these chips didn't exist.  (Or 2004; whatever.)  Or not;
the point being that using the POSIX epoch that way, with a 32 bit
counter, means overflow before all that long; and in this case you
could get 30+ extra years.


> +static int ds1374_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
> +{
> +	...
> +
> +	rtc_time_to_tm(now + cur_alarm, &alarm->time);
> +	alarm->enabled = !!(cr & DS1374_REG_CR_WACE);
> +	alarm->pending = !!(sr & DS1374_REG_SR_AF);

Wow -- you actually set both flags correctly!!


> +
> +	return 0;
> +}
> +
> +static int ds1374_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct ds1374 *ds1374 = i2c_get_clientdata(client);
> +	struct rtc_time now;
> +	unsigned long new_alarm, itime;
> +	u8 cr;
> +	int ret = 0;
> +
> +	if (!ds1374->have_irq)
> +		return -EINVAL;
> +
> +	cr = read_reg(client, DS1374_REG_CR, &ret);
> +	if (ret)
> +		return ret;
> +
> +	ret = ds1374_read_time(dev, &now);
> +	if (ret)
> +		return ret;
> +
> +	rtc_tm_to_time(&alarm->time, &new_alarm);
> +	rtc_tm_to_time(&now, &itime);
> +
> +	new_alarm -= itime;
> +
> +	/* This should only happen due to races, or if the full date
> +	 * up to the year was specified (and is in the past).  Partial
> +	 * dates in the past are interpreted into the future by
> +	 * rtc_merge_alarm().

rtc_merge_alarm() is gone though ... treat it as an error if
somehow a time-in-the-past gets to here.


> +	 */
> +	if (new_alarm <= 0)
> +		new_alarm = 1;
> +
> +	mutex_lock(&ds1374->mutex);
> +
> +	ret = ds1374_write_rtc_retry(dev, new_alarm,
> +	                             DS1374_REG_WDALM0,
> +	                             DS1374_REG_WDALM2);
> +
> +	if (alarm->enabled) {

You should have disabled it earlier though; its not
correct to let an "old" alarm-is-enabled state linger
if the caller said not to enable the alarm.


> +		cr |= DS1374_REG_CR_WACE | DS1374_REG_CR_AIE;
> +		cr &= ~DS1374_REG_CR_WDALM;
> +
> +		write_reg(client, DS1374_REG_CR, cr, &ret);
> +	}
> +
> +	mutex_unlock(&ds1374->mutex);
> +	return ret;
> +}
> +
> +static irqreturn_t ds1374_irq(int irq, void *dev_id)
> +{
> +	struct i2c_client *client = dev_id;
> +	struct ds1374 *ds1374 = i2c_get_clientdata(client);
> +	irqreturn_t ret = IRQ_NONE;
> +
> +	spin_lock_irq(&ds1374->lock);
> +
> +	if (ds1374->have_irq) {

It should not be possible to get here if this driver has
no IRQ!!  Test not needed.


> +		disable_irq_nosync(irq);

... and it seems here you're assuming this IRQ is level
triggered?  It'd make a lot more sense to trigger on the
falling (?) edge; if you can do that, it'd even be possible
to put this on a shared IRQ line.


> +		schedule_work(&ds1374->work);
> +		ret = IRQ_HANDLED;
> +	}
> +
> +	spin_unlock_irq(&ds1374->lock);
> +	return ret;
> +}
> +
> +static void ds1374_work(struct work_struct *work)
> +{
> +	struct ds1374 *ds1374 = container_of(work, struct ds1374, work);
> +	struct i2c_client *client = ds1374->client;
> +	u8 stat, control;
> +
> +	mutex_lock(&ds1374->mutex);
> +
> +	stat = read_reg(client, DS1374_REG_SR, NULL);
> +
> +	if (stat & DS1374_REG_SR_AF) {
> +		stat &= ~DS1374_REG_SR_AF;
> +		write_reg(client, DS1374_REG_SR, stat, NULL);
> +
> +		control = read_reg(client, DS1374_REG_CR, NULL);
> +		control &= ~(DS1374_REG_CR_WACE | DS1374_REG_CR_AIE);
> +		write_reg(client, DS1374_REG_CR, control, NULL);
> +
> +		rtc_update_irq(ds1374->rtc, 1, RTC_AF | RTC_IRQF);

But rtc_update_irq() must be called with IRQs disabled.
Lockdep will complain otherwise ... perhaps it doesn't
yet work on your platform?


> +	}
> +
> +	mutex_unlock(&ds1374->mutex);
> +
> +	enable_irq(client->irq);

Again, relevant only for level triggered IRQs, which would
be the least desirable model here.


> +}
> +
> +static const struct rtc_class_ops ds1374_rtc_ops = {
> +	.read_time = ds1374_read_time,
> +	.set_time = ds1374_set_time,
> +	.read_alarm = ds1374_read_alarm,
> +	.set_alarm = ds1374_set_alarm,

I think you should have an ioctl() handling RTC_AIE_{ON,OFF}
too.  There's a fair amount of code expecting the /dev/rtcN
interfaces to support the old-style RTC_ALM_SET + RTC_AIE_ON
idiom, not just new-style RTC_WKALM_SET ...


> +};
> +
> +static int ds1374_probe(struct i2c_client *client)
> +{
> +	struct ds1374 *ds1374;
> +	int ret;
> +
> +	ds1374 = kzalloc(sizeof(struct ds1374), GFP_KERNEL);
> +	if (!ds1374)
> +		return -ENOMEM;
> +
> +	ds1374->client = client;
> +	i2c_set_clientdata(client, ds1374);
> +
> +	INIT_WORK(&ds1374->work, ds1374_work);
> +	spin_lock_init(&ds1374->lock);
> +	mutex_init(&ds1374->mutex);
> +
> +	ret = ds1374_check_rtc_status(client);
> +	if (ret)
> +		return ret;

I was trying to figure out how check_rtc_status() could
fail, and then I saw it ... please fix those calling
conventions so that they *all* return status codes the
normal way, rather than indirecting through pointers!


> +
> +	if (client->irq != -1 &&
> +	    !request_irq(client->irq, ds1374_irq, 0, DS1374_DRV_NAME, client))
> +		ds1374->have_irq = 1;

As noted above, "have_irq" is not needed.  And you should
probably try to use an edge trigger mode here ... it's
unfortunate that while genirq lets you pass the trigger
mode in the flags, it won't report errors if that mode
wasn't set, and it won't report the trigger mode later.

So I think the cleanest solution is to not auto-enable the
IRQ as you register, then set_irq_type(), then manually
enable it.  The flag you'd need would be whether you need
to disable the level-triggered IRQ until you can ack it.

Unfortunately I suspect handling multiple trigger types
here may be required, long term, because of hardware
variability on the GPIO lines normally used to hook up
external chips like this one.


> +
> +	ds1374->rtc = rtc_device_register(client->name, &client->dev,
> +	                                  &ds1374_rtc_ops, THIS_MODULE);
> +	if (IS_ERR(ds1374->rtc)) {
> +		ret = PTR_ERR(ds1374->rtc);
> +		dev_err(&client->dev, "unable to register the class device\n");

... and unregister the IRQ handler ...

> +		kfree(ds1374);
> +		return ret;
> +	}
> +
> +	return 0;
> +}