Re: RTC: add RTC class interface to m41t00 driver

[Atsushi Nemoto]

On Fri, 4 Aug 2006 16:01:02 +0200, Alexander Bigga <ab@xxxxxxxxxx> wrote:
> like you, I started recently with Mark's m41t00.c driver to add support for 
> the new rtc-subsystem. Mark reviewed it and I added his changes.

Thank you.  Though my patch for m41t00.c intended to keep original
code as is as possible, I like your approach.  I'll work with your new
driver.

> There is still the question, if the code for the interrupt context 
> (workqueues) should stay or not. You bracketed all this with CONFIG_GEN_RTC. 
> I can't say, if this is a good idea. Maybe somebody else has some good 
> comments.

I think read_time and set_time routine of rtc_class never called from
the interrupt context.  It looks true on current RTC class framework
and some RTC class drivers depend on it already.

> +#include <asm/time.h>
> +#include <asm/rtc.h>

The asm/time.h is not exist on some archs.  And while all asm/time.h
are included by asm/rtc.h, this can be removed safely.

> +int m41txx_set_datetime(struct i2c_client *client, struct rtc_time *tm)

static.

> +ulong m41t00_get_rtc_time(void)
> +{
> +	struct rtc_time tm;
> +
> +	m41txx_get_datetime(save_client, &tm);
> +
> +	return mktime(tm.tm_year, tm.tm_mon,
> +		      tm.tm_mday, tm.tm_hour, tm.tm_min, tm.tm_sec);
> +}
> +EXPORT_SYMBOL_GPL(m41t00_get_rtc_time);

Please drop this old interface from new driver.  There are other way
to glue new driver to existing platform, as hctosys.c does.

Then we can remove save_client too.

> +static struct workqueue_struct *m41txx_wq;

As I wrote above, I think this is not needed.  If this is really
needed, it should be done in RTC framework instead of lowlevel driver.

> +st_err:
> +	dev_err(&client->dev, "%s: Can't clear ST bit\n", __FUNCTION__);
> +	goto exit_detach;
> +ht_err:
> +	dev_err(&client->dev, "%s: Can't clear HT bit\n", __FUNCTION__);
> +	goto exit_detach;
> +sqw_err:
> +	dev_err(&client->dev, "%s: Can't set SQW Frequency\n",
> +		__FUNCTION__);
> +
> +exit_detach:
> +	i2c_detach_client(client);

rtc_device_unregister() must be called somewhere in error path.

---
Atsushi Nemoto
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/