Re: + blackfin-on-chip-rtc-controller-driver.patch added to -mm tree
On 3/1/07, David Brownell <[EMAIL PROTECTED]> wrote:
Look again, but at 2.6.21-rc2 instead ... ISTR that fix went into RC1,
but that one's been fixed in various trees (like handhelds.org) for
some time.
this driver was originally written against 2.6.19.x as we havent
gotten the Blackfin arch into mainline yet so it's hard to track the
latest git
i'll pull the latest git and look into the pointers you've posted ...
and send out a sep patch for updating the documentation ;)
> > It looks to me like you're handling the time in set_alarm()
> > incorrectly. Do the conversion and test in set_alarm, not
> > AIE_ON ... since set_alarm() needs to be able to include AIE_ON
> > capability.
>
> the reason for this is so that i do not have to worry about keeping
> two structures in sync ... there's the common RTC representation and
> then there's the funky Blackfin representation, so by delaying the
> conversion until RTC_AIE_ON, i didnt have to worry about keeping these
> fields in sync
Yes, Blackfin has an unusually funky representation, but that
doesn't mean you can't do the conversion when the alarm time
comes into the driver. It's not like the alarm is based on
offset-from-current-time, so that it'd need to be adjusted when
the current time gets updated (like some RTCs)... it's just a
strange way to encode an absolute time.
fair enough ... my other concern was that now i'll also have to
translate it back out in rtc_read_alarm so by maintaining everything
in common RTC terms, there would be no overhead
> > The fields tm_{wday,yday,isdst} are never used,
> > but you're testing tm_yday. If you meant to test tm_mday in
> > order to distinguish the WKALM_SET and ALM_SET cases, then you
> > are mishandling a wraparound case; see how rtc-omap handles
> > it. (If it's 23:00 now, an 01:00 alarm means TOMORROW. I
> > think other RTCs may goof this case too.)
>
> this is exactly why i'm checking tm_yday ... the rtc-dev interface
> sets those fields to -1 when using the WKALM ioctls and there's no
> other way to distinguish this
Thing is, "man 4 rtc" reports those fields as unused, which means
they can come from userspace with arbitrary values. You should
stick to testing tm_mday, which *is* used.
i dont rely on the manpages for kernel interfaces as they get stale
too fast ... i read the files found in Documentation, rtc.txt in this
case
i guess rtc.txt needs more stuff added to it
-mike
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: + blackfin-on-chip-rtc-controller-driver.patch added to -mm tree
On Thursday 01 March 2007 11:06 am, Mike Frysinger wrote:
> > The set_alarm() method needs to enable the alarm irq if the
> > "enabled" flag is set, and the read_alarm() method needs to
> > report whether the alarm is enabled.
>
> from reading other drivers and the documentation,
That particular call came from the RTC interface exposed by EFI,
which is pretty clear on things. The only notable glitch in
translating it to Linux is that Linux splits "alarm" and "wakeup"
capabilities ... but then, EFI lets you poll that RTC and gives
effectively that same consequence: alarm not coupled to wakeup,
at least if the system isn't asleep.
> > It's unclear why you
> > chose to report "pending" (irq issued but not yet acked) since
> > that's uselessly transient state on non-polled hardwre. (That
> > flag definition came from EFI, a polled firmware RTC.)
>
> because other drivers do ? there is no documentation here that covers
> what exactly the read_alarm function in the rtc_class_ops is supposed
> to do which leaves it up to people looking at other drivers
>
> rtc-sa1100.c for example:
> static int sa1100_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> {
> memcpy(>time, _alarm, sizeof(struct rtc_time));
> alrm->pending = RTSR & RTSR_AL ? 1 : 0;
> return 0;
> }
Look again, but at 2.6.21-rc2 instead ... ISTR that fix went into RC1,
but that one's been fixed in various trees (like handhelds.org) for
some time.
> if someone decides what the behavior is supposed to be here, i'll
> happily implement it
>
> > Correct handling of the "enabled" flag in read_alarm() will let
> > you remove a redundant seq_printf() in your proc() callback...
>
> i dont understand this ... rtc-sh.c and rtc-sa1100.c both do a
> seq_printf() for the alarm irq
Look again at current kernel sources ... bugfixes have been merged
since the codebase you seem to have used as a reference. I did an
audit of driver handling of those flags; most drivers are now fixed.
> > It looks to me like you're handling the time in set_alarm()
> > incorrectly. Do the conversion and test in set_alarm, not
> > AIE_ON ... since set_alarm() needs to be able to include AIE_ON
> > capability.
>
> the reason for this is so that i do not have to worry about keeping
> two structures in sync ... there's the common RTC representation and
> then there's the funky Blackfin representation, so by delaying the
> conversion until RTC_AIE_ON, i didnt have to worry about keeping these
> fields in sync
Yes, Blackfin has an unusually funky representation, but that
doesn't mean you can't do the conversion when the alarm time
comes into the driver. It's not like the alarm is based on
offset-from-current-time, so that it'd need to be adjusted when
the current time gets updated (like some RTCs)... it's just a
strange way to encode an absolute time.
> > The fields tm_{wday,yday,isdst} are never used,
> > but you're testing tm_yday. If you meant to test tm_mday in
> > order to distinguish the WKALM_SET and ALM_SET cases, then you
> > are mishandling a wraparound case; see how rtc-omap handles
> > it. (If it's 23:00 now, an 01:00 alarm means TOMORROW. I
> > think other RTCs may goof this case too.)
>
> this is exactly why i'm checking tm_yday ... the rtc-dev interface
> sets those fields to -1 when using the WKALM ioctls and there's no
> other way to distinguish this
Thing is, "man 4 rtc" reports those fields as unused, which means
they can come from userspace with arbitrary values. You should
stick to testing tm_mday, which *is* used.
> if the wraparound case is so common, then perhaps it should be a
> helper function in the generic rtc code rather than reimplementing in
> every rtc driver ...
You have a point there. Someone (other than me) should come
up with a patch to handle that.
- Dave
> > The use of class_device conflicts with the patch series I
> > recently resubmitted, see it on [EMAIL PROTECTED] ...
> > if you submit a separate patch, then maybe your Blackfin RTC
> > can be merged first.
>
> i'll take a look
> -mike
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: + blackfin-on-chip-rtc-controller-driver.patch added to -mm tree
On Thu, 1 Mar 2007 14:06:49 -0500 "Mike Frysinger" <[EMAIL PROTECTED]> wrote: > > It's unclear why you > > chose to report "pending" (irq issued but not yet acked) since > > that's uselessly transient state on non-polled hardwre. (That > > flag definition came from EFI, a polled firmware RTC.) > > because other drivers do ? there is no documentation here that covers > what exactly the read_alarm function in the rtc_class_ops is supposed > to do which leaves it up to people looking at other drivers there has been a bit of confusion on this topic. I think it's now the time to exactly define what should happen with alarms and how they should be reported back to the class. I'm usually inclined to agree with David on that particular topic :) other than that, I would like to remove the /proc interface in the future, so users are advised not to count on it and device authors to not add properties. -- Best regards, Alessandro Zummo, Tower Technologies - Torino, Italy http://www.towertech.it - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: + blackfin-on-chip-rtc-controller-driver.patch added to -mm tree
On 3/1/07, David Brownell <[EMAIL PROTECTED]> wrote:
Bryan, it'd be nice to see a followup patch addressing those
comments from Paul Mundt, especially about that code which
will spin-forever-under-spinlock. That spin should probably
drop the lock then msleep(1) then restore it before retest...
i've been chatting with Paul on irc about it
The set_alarm() method needs to enable the alarm irq if the
"enabled" flag is set, and the read_alarm() method needs to
report whether the alarm is enabled.
from reading other drivers and the documentation, i couldnt determine
whether this was the standard behavior or whether set_alarm simply set
the alarm time but you still needed to call the ioctl RTC_AIE_ON in
order to actual enable it
It's unclear why you
chose to report "pending" (irq issued but not yet acked) since
that's uselessly transient state on non-polled hardwre. (That
flag definition came from EFI, a polled firmware RTC.)
because other drivers do ? there is no documentation here that covers
what exactly the read_alarm function in the rtc_class_ops is supposed
to do which leaves it up to people looking at other drivers
rtc-sa1100.c for example:
static int sa1100_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
{
memcpy(>time, _alarm, sizeof(struct rtc_time));
alrm->pending = RTSR & RTSR_AL ? 1 : 0;
return 0;
}
if someone decides what the behavior is supposed to be here, i'll
happily implement it
Correct handling of the "enabled" flag in read_alarm() will let
you remove a redundant seq_printf() in your proc() callback...
i dont understand this ... rtc-sh.c and rtc-sa1100.c both do a
seq_printf() for the alarm irq
It looks to me like you're handling the time in set_alarm()
incorrectly. Do the conversion and test in set_alarm, not
AIE_ON ... since set_alarm() needs to be able to include AIE_ON
capability.
the reason for this is so that i do not have to worry about keeping
two structures in sync ... there's the common RTC representation and
then there's the funky Blackfin representation, so by delaying the
conversion until RTC_AIE_ON, i didnt have to worry about keeping these
fields in sync
The fields tm_{wday,yday,isdst} are never used,
but you're testing tm_yday. If you meant to test tm_mday in
order to distinguish the WKALM_SET and ALM_SET cases, then you
are mishandling a wraparound case; see how rtc-omap handles
it. (If it's 23:00 now, an 01:00 alarm means TOMORROW. I
think other RTCs may goof this case too.)
this is exactly why i'm checking tm_yday ... the rtc-dev interface
sets those fields to -1 when using the WKALM ioctls and there's no
other way to distinguish this
if the wraparound case is so common, then perhaps it should be a
helper function in the generic rtc code rather than reimplementing in
every rtc driver ...
The use of class_device conflicts with the patch series I
recently resubmitted, see it on [EMAIL PROTECTED] ...
if you submit a separate patch, then maybe your Blackfin RTC
can be merged first.
i'll take a look
-mike
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: + blackfin-on-chip-rtc-controller-driver.patch added to -mm tree
On 3/1/07, David Brownell [EMAIL PROTECTED] wrote:
Bryan, it'd be nice to see a followup patch addressing those
comments from Paul Mundt, especially about that code which
will spin-forever-under-spinlock. That spin should probably
drop the lock then msleep(1) then restore it before retest...
i've been chatting with Paul on irc about it
The set_alarm() method needs to enable the alarm irq if the
enabled flag is set, and the read_alarm() method needs to
report whether the alarm is enabled.
from reading other drivers and the documentation, i couldnt determine
whether this was the standard behavior or whether set_alarm simply set
the alarm time but you still needed to call the ioctl RTC_AIE_ON in
order to actual enable it
It's unclear why you
chose to report pending (irq issued but not yet acked) since
that's uselessly transient state on non-polled hardwre. (That
flag definition came from EFI, a polled firmware RTC.)
because other drivers do ? there is no documentation here that covers
what exactly the read_alarm function in the rtc_class_ops is supposed
to do which leaves it up to people looking at other drivers
rtc-sa1100.c for example:
static int sa1100_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
{
memcpy(alrm-time, rtc_alarm, sizeof(struct rtc_time));
alrm-pending = RTSR RTSR_AL ? 1 : 0;
return 0;
}
if someone decides what the behavior is supposed to be here, i'll
happily implement it
Correct handling of the enabled flag in read_alarm() will let
you remove a redundant seq_printf() in your proc() callback...
i dont understand this ... rtc-sh.c and rtc-sa1100.c both do a
seq_printf() for the alarm irq
It looks to me like you're handling the time in set_alarm()
incorrectly. Do the conversion and test in set_alarm, not
AIE_ON ... since set_alarm() needs to be able to include AIE_ON
capability.
the reason for this is so that i do not have to worry about keeping
two structures in sync ... there's the common RTC representation and
then there's the funky Blackfin representation, so by delaying the
conversion until RTC_AIE_ON, i didnt have to worry about keeping these
fields in sync
The fields tm_{wday,yday,isdst} are never used,
but you're testing tm_yday. If you meant to test tm_mday in
order to distinguish the WKALM_SET and ALM_SET cases, then you
are mishandling a wraparound case; see how rtc-omap handles
it. (If it's 23:00 now, an 01:00 alarm means TOMORROW. I
think other RTCs may goof this case too.)
this is exactly why i'm checking tm_yday ... the rtc-dev interface
sets those fields to -1 when using the WKALM ioctls and there's no
other way to distinguish this
if the wraparound case is so common, then perhaps it should be a
helper function in the generic rtc code rather than reimplementing in
every rtc driver ...
The use of class_device conflicts with the patch series I
recently resubmitted, see it on [EMAIL PROTECTED] ...
if you submit a separate patch, then maybe your Blackfin RTC
can be merged first.
i'll take a look
-mike
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: + blackfin-on-chip-rtc-controller-driver.patch added to -mm tree
On Thu, 1 Mar 2007 14:06:49 -0500 Mike Frysinger [EMAIL PROTECTED] wrote: It's unclear why you chose to report pending (irq issued but not yet acked) since that's uselessly transient state on non-polled hardwre. (That flag definition came from EFI, a polled firmware RTC.) because other drivers do ? there is no documentation here that covers what exactly the read_alarm function in the rtc_class_ops is supposed to do which leaves it up to people looking at other drivers there has been a bit of confusion on this topic. I think it's now the time to exactly define what should happen with alarms and how they should be reported back to the class. I'm usually inclined to agree with David on that particular topic :) other than that, I would like to remove the /proc interface in the future, so users are advised not to count on it and device authors to not add properties. -- Best regards, Alessandro Zummo, Tower Technologies - Torino, Italy http://www.towertech.it - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: + blackfin-on-chip-rtc-controller-driver.patch added to -mm tree
On Thursday 01 March 2007 11:06 am, Mike Frysinger wrote:
The set_alarm() method needs to enable the alarm irq if the
enabled flag is set, and the read_alarm() method needs to
report whether the alarm is enabled.
from reading other drivers and the documentation,
That particular call came from the RTC interface exposed by EFI,
which is pretty clear on things. The only notable glitch in
translating it to Linux is that Linux splits alarm and wakeup
capabilities ... but then, EFI lets you poll that RTC and gives
effectively that same consequence: alarm not coupled to wakeup,
at least if the system isn't asleep.
It's unclear why you
chose to report pending (irq issued but not yet acked) since
that's uselessly transient state on non-polled hardwre. (That
flag definition came from EFI, a polled firmware RTC.)
because other drivers do ? there is no documentation here that covers
what exactly the read_alarm function in the rtc_class_ops is supposed
to do which leaves it up to people looking at other drivers
rtc-sa1100.c for example:
static int sa1100_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
{
memcpy(alrm-time, rtc_alarm, sizeof(struct rtc_time));
alrm-pending = RTSR RTSR_AL ? 1 : 0;
return 0;
}
Look again, but at 2.6.21-rc2 instead ... ISTR that fix went into RC1,
but that one's been fixed in various trees (like handhelds.org) for
some time.
if someone decides what the behavior is supposed to be here, i'll
happily implement it
Correct handling of the enabled flag in read_alarm() will let
you remove a redundant seq_printf() in your proc() callback...
i dont understand this ... rtc-sh.c and rtc-sa1100.c both do a
seq_printf() for the alarm irq
Look again at current kernel sources ... bugfixes have been merged
since the codebase you seem to have used as a reference. I did an
audit of driver handling of those flags; most drivers are now fixed.
It looks to me like you're handling the time in set_alarm()
incorrectly. Do the conversion and test in set_alarm, not
AIE_ON ... since set_alarm() needs to be able to include AIE_ON
capability.
the reason for this is so that i do not have to worry about keeping
two structures in sync ... there's the common RTC representation and
then there's the funky Blackfin representation, so by delaying the
conversion until RTC_AIE_ON, i didnt have to worry about keeping these
fields in sync
Yes, Blackfin has an unusually funky representation, but that
doesn't mean you can't do the conversion when the alarm time
comes into the driver. It's not like the alarm is based on
offset-from-current-time, so that it'd need to be adjusted when
the current time gets updated (like some RTCs)... it's just a
strange way to encode an absolute time.
The fields tm_{wday,yday,isdst} are never used,
but you're testing tm_yday. If you meant to test tm_mday in
order to distinguish the WKALM_SET and ALM_SET cases, then you
are mishandling a wraparound case; see how rtc-omap handles
it. (If it's 23:00 now, an 01:00 alarm means TOMORROW. I
think other RTCs may goof this case too.)
this is exactly why i'm checking tm_yday ... the rtc-dev interface
sets those fields to -1 when using the WKALM ioctls and there's no
other way to distinguish this
Thing is, man 4 rtc reports those fields as unused, which means
they can come from userspace with arbitrary values. You should
stick to testing tm_mday, which *is* used.
if the wraparound case is so common, then perhaps it should be a
helper function in the generic rtc code rather than reimplementing in
every rtc driver ...
You have a point there. Someone (other than me) should come
up with a patch to handle that.
- Dave
The use of class_device conflicts with the patch series I
recently resubmitted, see it on [EMAIL PROTECTED] ...
if you submit a separate patch, then maybe your Blackfin RTC
can be merged first.
i'll take a look
-mike
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: + blackfin-on-chip-rtc-controller-driver.patch added to -mm tree
On 3/1/07, David Brownell [EMAIL PROTECTED] wrote:
Look again, but at 2.6.21-rc2 instead ... ISTR that fix went into RC1,
but that one's been fixed in various trees (like handhelds.org) for
some time.
this driver was originally written against 2.6.19.x as we havent
gotten the Blackfin arch into mainline yet so it's hard to track the
latest git
i'll pull the latest git and look into the pointers you've posted ...
and send out a sep patch for updating the documentation ;)
It looks to me like you're handling the time in set_alarm()
incorrectly. Do the conversion and test in set_alarm, not
AIE_ON ... since set_alarm() needs to be able to include AIE_ON
capability.
the reason for this is so that i do not have to worry about keeping
two structures in sync ... there's the common RTC representation and
then there's the funky Blackfin representation, so by delaying the
conversion until RTC_AIE_ON, i didnt have to worry about keeping these
fields in sync
Yes, Blackfin has an unusually funky representation, but that
doesn't mean you can't do the conversion when the alarm time
comes into the driver. It's not like the alarm is based on
offset-from-current-time, so that it'd need to be adjusted when
the current time gets updated (like some RTCs)... it's just a
strange way to encode an absolute time.
fair enough ... my other concern was that now i'll also have to
translate it back out in rtc_read_alarm so by maintaining everything
in common RTC terms, there would be no overhead
The fields tm_{wday,yday,isdst} are never used,
but you're testing tm_yday. If you meant to test tm_mday in
order to distinguish the WKALM_SET and ALM_SET cases, then you
are mishandling a wraparound case; see how rtc-omap handles
it. (If it's 23:00 now, an 01:00 alarm means TOMORROW. I
think other RTCs may goof this case too.)
this is exactly why i'm checking tm_yday ... the rtc-dev interface
sets those fields to -1 when using the WKALM ioctls and there's no
other way to distinguish this
Thing is, man 4 rtc reports those fields as unused, which means
they can come from userspace with arbitrary values. You should
stick to testing tm_mday, which *is* used.
i dont rely on the manpages for kernel interfaces as they get stale
too fast ... i read the files found in Documentation, rtc.txt in this
case
i guess rtc.txt needs more stuff added to it
-mike
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
