Re: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Fri, May 11, 2007 at 11:17:11PM -0700, Andrew Morton wrote:
> On Fri, 11 May 2007 23:55:37 +0200
> Rodolfo Giometti <[EMAIL PROTECTED]> wrote:
>
> > Hello,
> >
> > here my new patch with a lot of fixes.
> >
> > The only issue not still fixed is the one related with:
> >
> > #define NETLINK_PPSAPI 20
> >
> > I need time to resolve it.
> >
> > Follows my comments and then the patch, hope now I can came back into
> > -mm tree again! :)
>
> Well I suppose I could toss it in there for a bit of review-and-test. But
> I'll need to drop it again because we do need to split this patch into the
> series
> of patches, please.
>
> You should do this earlier rather than later because it improves
> reviewability.
>
> > > - This:
> > >
> > > static void pps_class_release(struct class_device *cdev)
> > > {
> > > /* Nop??? */
> > > }
> > >
> > > is a bug and it earns you a nastygram from Greg. These objects must be
> > > dynamically allocated - this is not optional.
> >
> > It could be acceptable defining this function as void?
>
> No, it needs to be a proper release function, like all the other ones
> around the place.
>
> This comes up again and again and again and I recently asked Greg to direct
> me to (or to write) suitable documentation, and I think he did, but I lost
> it. Greg, can you remind us please?
I need to put it in some permanent place, but basically the problem is
that if you need to shut the kernel up by using an empty function for a
release, then you are not understanding why the kernel was trying to
warn you in the first place :(
You need to free your memory for the class_device in the release
function, you can not have a static structure, or rely on some other
reference count to handle the cleanup properly.
Also note that 'struct class_device' is going away and you should use
'struct device' instead. That too needs to be documented better, and is
on my list of things to do...
thanks,
greg k-h
-
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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Fri, 11 May 2007 23:55:37 +0200
Rodolfo Giometti <[EMAIL PROTECTED]> wrote:
> Hello,
>
> here my new patch with a lot of fixes.
>
> The only issue not still fixed is the one related with:
>
> #define NETLINK_PPSAPI 20
>
> I need time to resolve it.
>
> Follows my comments and then the patch, hope now I can came back into
> -mm tree again! :)
Well I suppose I could toss it in there for a bit of review-and-test. But
I'll need to drop it again because we do need to split this patch into the
series
of patches, please.
You should do this earlier rather than later because it improves reviewability.
> > - This:
> >
> > static void pps_class_release(struct class_device *cdev)
> > {
> > /* Nop??? */
> > }
> >
> > is a bug and it earns you a nastygram from Greg. These objects must be
> > dynamically allocated - this is not optional.
>
> It could be acceptable defining this function as void?
No, it needs to be a proper release function, like all the other ones
around the place.
This comes up again and again and again and I recently asked Greg to direct
me to (or to write) suitable documentation, and I think he did, but I lost
it. Greg, can you remind us please?
> > We have a bunch of code in random other drivers which is dependent upon
> > CONFIG_PPS_CLIENT_foo. The problem is that if a kernel was compiled with
> > CONFIG_PPS_CLIENT_foo=n and then the pps driver is later built for that
> > kernel, it won't actually work because lp, serial etc weren't correctly
> > configured when _they_ were built.
> >
> > This sort of cross-module coupling is considered to be a bad thing, but
> > I'm not really sure it's all that important.
> >
> > - Please split the patch up into a series of patches: one for pps core and
> > one for each of the clients (servers?): one for lp, one for serial, etc.
> >
> > Try to arrange for that series of patches to build and run at each stage
> > of application.
> >
> > Please don't lose my changes when you do so ;)
> >
> > Please review the changes I made and a) stick to the same style and b) fix
> > up any sites which I missed.
> >
> > - Please remove all the typedefs:
> >
> > +typedef struct ntp_fp {
> > +typedef union pps_timeu {
> > +typedef struct pps_info {
> > +typedef struct pps_params {
> >
> > and just use `struct ntp_fp' everywhere.
>
> Those typedefs are defined in PPS specifications (please, see RFC 2783).
We don't use typedefs in-kernel. Please convert the code to use `struct
ntp_fp' everywhere.
For RFC compatibility to userspace you can do
#ifndef __KERNEL__
typedef struct ntp_fp ntp_fp_t;
...
#endif
> > - The above four structures are communicated with userspace, yes?
> >
> > I believe that they will not work correctly when 32-bit userspace is
> > communicating with a 64-bit kernel. Alignments change and sizeof(long)
> > changes.
> >
> > You don't want to have to write compat code. I suggest that you redo
> > those structures in terms of __u32, __u64, etc. You probably need to use
> > attribute((packed)) too, not sure.
> >
> > Then let's get that part carefully reviewed (Arnd Bergmann <[EMAIL
> > PROTECTED]>
> > is my go-to guru on this) and please test it carefully.
> >
> > Yeah, you just haven't got a chance that something as huge and as complex
> > as struct pps_netlink_msg will survive the 32->64 transition.
>
> The same as above. These structure are fixed by RFC 2783.
Your answer has no relationship to my question.
The problem here is that under a 64-bit kernel we require that applications
which use this structure definition work correctly when they are compiled
to generate 32-bit code and when they are compiled to generate 64-bit code.
Furthermore we should aim to to have to code work correctly across
different version of the compiler, and when different compiler options are
used, and when altogether different compilers are used.
It is not clear to me that your definition is sufficiently defensive
against _any_ of these things.
> > - Please ensure that `make headers_check' passes OK (you'll hear from me if
> > it doesn't ;))
>
> Done.
>
> > - Can we get rid of the private dbg, err and info macros? Surely there are
> > generic ones somewhere.
>
> They are very useful to LinuxPPS users who can enable/disable them by
> configuration menu.
You misunderstand. I'm not saying "remove the callsites". I'm saying
"remove the definitions".
Because we already have things like pr_debug() and pr_info(), so new code
should use those rather than reinventing them.
Plus, we already have at least 52 different implementations of "dbg" in the
tree and your 53rd one didn't compile because it clashed with someone
else's. This is the compiler sending us a message: "use the exiting
infrastructure". If that infrastructure is insufficient then let's
improve it.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body
Re: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
Hello,
here my new patch with a lot of fixes.
The only issue not still fixed is the one related with:
#define NETLINK_PPSAPI 20
I need time to resolve it.
Follows my comments and then the patch, hope now I can came back into
-mm tree again! :)
On Thu, May 10, 2007 at 12:27:52AM -0700, [EMAIL PROTECTED] wrote:
>
> Review comments:
>
> - Running a timer once per second will make the super-low-power people upset.
The ktimer modules is just for debugging pourpose and it's not needed
into real working system.
> - This uses netlink? Is that interface documented anywhere?
>
> Please check with Dave Miller that this:
>
> #define NETLINK_PPSAPI 20
>
> reservation is OK.
Is not ok. To be fixed.
> - This:
>
> if ((nlpps->tsformat != PPS_TSFMT_TSPEC) != 0 ) {
>
> is weird. I changed it to
>
> if (nlpps->tsformat != PPS_TSFMT_TSPEC) {
Fixed.
> - This:
>
> timeout += nlpps->timeout.tv_nsec/(10/HZ);
>
> probably won't work on i386. We use do_div() for 64/32 divides. I'll
> find out when I compile it.
>
> It's nice to use NSEC_PER_SEC rather than having to count all those
> zeroes.
Fixed.
> - The code uses interruptible_sleep_on_timeout(). That API is deprecated
> and is racy. Please convert to wait_event_interruptible_timeout().
>
> Ditto interruptible_sleep_on()
Fixed.
> - This:
>
> memset(pps_source, 0, sizeof(struct pps_s) * PPS_MAX_SOURCES);
>
> was unneeded. The C startup code already did that.
Fixed.
> - All these separators:
>
> +/* --- Input function --
+*/
>
> aren't typical for kernel code. I left them in, but please consider
> removing them all.
Fixed.
> - This:
>
> static void pps_class_release(struct class_device *cdev)
> {
> /* Nop??? */
> }
>
> is a bug and it earns you a nastygram from Greg. These objects must be
> dynamically allocated - this is not optional.
It could be acceptable defining this function as void?
> - What's this doing in 8250.c?
>
> + if (up->port.flags & UPF_HARDPPS_CD)
> + up->ier |= UART_IER_MSI;/* enable interrupts */
>
> Please fully describe the reasons for this change in the changelog, and in
> a code comment and then get the change reviewed by Russell King
> <[EMAIL PROTECTED]>.
If user specify a serial port as PPS source we enable IRQ on that
port.
> - Please document within the changelog the other changes to the serial code
> and we'll ask Russell to take a look at those as well.
OK. I'll do it.
> - The Kconfig purports to support CONFIG_PPS=m. Does that actually work?
Yes. It works...
> We have a bunch of code in random other drivers which is dependent upon
> CONFIG_PPS_CLIENT_foo. The problem is that if a kernel was compiled with
> CONFIG_PPS_CLIENT_foo=n and then the pps driver is later built for that
> kernel, it won't actually work because lp, serial etc weren't correctly
> configured when _they_ were built.
>
> This sort of cross-module coupling is considered to be a bad thing, but
> I'm not really sure it's all that important.
>
> - Please split the patch up into a series of patches: one for pps core and
> one for each of the clients (servers?): one for lp, one for serial, etc.
>
> Try to arrange for that series of patches to build and run at each stage
> of application.
>
> Please don't lose my changes when you do so ;)
>
> Please review the changes I made and a) stick to the same style and b) fix
> up any sites which I missed.
>
> - Please remove all the typedefs:
>
> +typedef struct ntp_fp {
> +typedef union pps_timeu {
> +typedef struct pps_info {
> +typedef struct pps_params {
>
> and just use `struct ntp_fp' everywhere.
Those typedefs are defined in PPS specifications (please, see RFC 2783).
> - The above four structures are communicated with userspace, yes?
>
> I believe that they will not work correctly when 32-bit userspace is
> communicating with a 64-bit kernel. Alignments change and sizeof(long)
> changes.
>
> You don't want to have to write compat code. I suggest that you redo
> those structures in terms of __u32, __u64, etc. You probably need to use
> attribute((packed)) too, not sure.
>
> Then let's get that part carefully reviewed (Arnd Bergmann <[EMAIL
> PROTECTED]>
> is my go-to guru on this) and please test it carefully.
>
> Yeah, you just haven't got a chance that something as huge and as complex
> as struct pps_netlink_msg will survive the 32->64 transition.
The same as above. These structure are fixed by RFC 2783.
> - Please ensure that `make headers_check' passes OK (you'll hear from me if
> it doesn't ;))
Done.
> - Can we get rid of the private dbg, err and info macros? Surely there are
> generic ones somewhere.
They are very useful to LinuxPPS users who can enable/disable them by
configuration menu.
Also I'm planning
Re: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
Hello,
here my new patch with a lot of fixes.
The only issue not still fixed is the one related with:
#define NETLINK_PPSAPI 20
I need time to resolve it.
Follows my comments and then the patch, hope now I can came back into
-mm tree again! :)
On Thu, May 10, 2007 at 12:27:52AM -0700, [EMAIL PROTECTED] wrote:
Review comments:
- Running a timer once per second will make the super-low-power people upset.
The ktimer modules is just for debugging pourpose and it's not needed
into real working system.
- This uses netlink? Is that interface documented anywhere?
Please check with Dave Miller that this:
#define NETLINK_PPSAPI 20
reservation is OK.
Is not ok. To be fixed.
- This:
if ((nlpps-tsformat != PPS_TSFMT_TSPEC) != 0 ) {
is weird. I changed it to
if (nlpps-tsformat != PPS_TSFMT_TSPEC) {
Fixed.
- This:
timeout += nlpps-timeout.tv_nsec/(10/HZ);
probably won't work on i386. We use do_div() for 64/32 divides. I'll
find out when I compile it.
It's nice to use NSEC_PER_SEC rather than having to count all those
zeroes.
Fixed.
- The code uses interruptible_sleep_on_timeout(). That API is deprecated
and is racy. Please convert to wait_event_interruptible_timeout().
Ditto interruptible_sleep_on()
Fixed.
- This:
memset(pps_source, 0, sizeof(struct pps_s) * PPS_MAX_SOURCES);
was unneeded. The C startup code already did that.
Fixed.
- All these separators:
+/* --- Input function --
+*/
aren't typical for kernel code. I left them in, but please consider
removing them all.
Fixed.
- This:
static void pps_class_release(struct class_device *cdev)
{
/* Nop??? */
}
is a bug and it earns you a nastygram from Greg. These objects must be
dynamically allocated - this is not optional.
It could be acceptable defining this function as void?
- What's this doing in 8250.c?
+ if (up-port.flags UPF_HARDPPS_CD)
+ up-ier |= UART_IER_MSI;/* enable interrupts */
Please fully describe the reasons for this change in the changelog, and in
a code comment and then get the change reviewed by Russell King
[EMAIL PROTECTED].
If user specify a serial port as PPS source we enable IRQ on that
port.
- Please document within the changelog the other changes to the serial code
and we'll ask Russell to take a look at those as well.
OK. I'll do it.
- The Kconfig purports to support CONFIG_PPS=m. Does that actually work?
Yes. It works...
We have a bunch of code in random other drivers which is dependent upon
CONFIG_PPS_CLIENT_foo. The problem is that if a kernel was compiled with
CONFIG_PPS_CLIENT_foo=n and then the pps driver is later built for that
kernel, it won't actually work because lp, serial etc weren't correctly
configured when _they_ were built.
This sort of cross-module coupling is considered to be a bad thing, but
I'm not really sure it's all that important.
- Please split the patch up into a series of patches: one for pps core and
one for each of the clients (servers?): one for lp, one for serial, etc.
Try to arrange for that series of patches to build and run at each stage
of application.
Please don't lose my changes when you do so ;)
Please review the changes I made and a) stick to the same style and b) fix
up any sites which I missed.
- Please remove all the typedefs:
+typedef struct ntp_fp {
+typedef union pps_timeu {
+typedef struct pps_info {
+typedef struct pps_params {
and just use `struct ntp_fp' everywhere.
Those typedefs are defined in PPS specifications (please, see RFC 2783).
- The above four structures are communicated with userspace, yes?
I believe that they will not work correctly when 32-bit userspace is
communicating with a 64-bit kernel. Alignments change and sizeof(long)
changes.
You don't want to have to write compat code. I suggest that you redo
those structures in terms of __u32, __u64, etc. You probably need to use
attribute((packed)) too, not sure.
Then let's get that part carefully reviewed (Arnd Bergmann [EMAIL
PROTECTED]
is my go-to guru on this) and please test it carefully.
Yeah, you just haven't got a chance that something as huge and as complex
as struct pps_netlink_msg will survive the 32-64 transition.
The same as above. These structure are fixed by RFC 2783.
- Please ensure that `make headers_check' passes OK (you'll hear from me if
it doesn't ;))
Done.
- Can we get rid of the private dbg, err and info macros? Surely there are
generic ones somewhere.
They are very useful to LinuxPPS users who can enable/disable them by
configuration menu.
Also I'm planning to add a dinamic enable/disable mechanism...
- struct pps_s has volatiles in it. Please remove them. There's lots of
Re: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Fri, 11 May 2007 23:55:37 +0200
Rodolfo Giometti [EMAIL PROTECTED] wrote:
Hello,
here my new patch with a lot of fixes.
The only issue not still fixed is the one related with:
#define NETLINK_PPSAPI 20
I need time to resolve it.
Follows my comments and then the patch, hope now I can came back into
-mm tree again! :)
Well I suppose I could toss it in there for a bit of review-and-test. But
I'll need to drop it again because we do need to split this patch into the
series
of patches, please.
You should do this earlier rather than later because it improves reviewability.
- This:
static void pps_class_release(struct class_device *cdev)
{
/* Nop??? */
}
is a bug and it earns you a nastygram from Greg. These objects must be
dynamically allocated - this is not optional.
It could be acceptable defining this function as void?
No, it needs to be a proper release function, like all the other ones
around the place.
This comes up again and again and again and I recently asked Greg to direct
me to (or to write) suitable documentation, and I think he did, but I lost
it. Greg, can you remind us please?
We have a bunch of code in random other drivers which is dependent upon
CONFIG_PPS_CLIENT_foo. The problem is that if a kernel was compiled with
CONFIG_PPS_CLIENT_foo=n and then the pps driver is later built for that
kernel, it won't actually work because lp, serial etc weren't correctly
configured when _they_ were built.
This sort of cross-module coupling is considered to be a bad thing, but
I'm not really sure it's all that important.
- Please split the patch up into a series of patches: one for pps core and
one for each of the clients (servers?): one for lp, one for serial, etc.
Try to arrange for that series of patches to build and run at each stage
of application.
Please don't lose my changes when you do so ;)
Please review the changes I made and a) stick to the same style and b) fix
up any sites which I missed.
- Please remove all the typedefs:
+typedef struct ntp_fp {
+typedef union pps_timeu {
+typedef struct pps_info {
+typedef struct pps_params {
and just use `struct ntp_fp' everywhere.
Those typedefs are defined in PPS specifications (please, see RFC 2783).
We don't use typedefs in-kernel. Please convert the code to use `struct
ntp_fp' everywhere.
For RFC compatibility to userspace you can do
#ifndef __KERNEL__
typedef struct ntp_fp ntp_fp_t;
...
#endif
- The above four structures are communicated with userspace, yes?
I believe that they will not work correctly when 32-bit userspace is
communicating with a 64-bit kernel. Alignments change and sizeof(long)
changes.
You don't want to have to write compat code. I suggest that you redo
those structures in terms of __u32, __u64, etc. You probably need to use
attribute((packed)) too, not sure.
Then let's get that part carefully reviewed (Arnd Bergmann [EMAIL
PROTECTED]
is my go-to guru on this) and please test it carefully.
Yeah, you just haven't got a chance that something as huge and as complex
as struct pps_netlink_msg will survive the 32-64 transition.
The same as above. These structure are fixed by RFC 2783.
Your answer has no relationship to my question.
The problem here is that under a 64-bit kernel we require that applications
which use this structure definition work correctly when they are compiled
to generate 32-bit code and when they are compiled to generate 64-bit code.
Furthermore we should aim to to have to code work correctly across
different version of the compiler, and when different compiler options are
used, and when altogether different compilers are used.
It is not clear to me that your definition is sufficiently defensive
against _any_ of these things.
- Please ensure that `make headers_check' passes OK (you'll hear from me if
it doesn't ;))
Done.
- Can we get rid of the private dbg, err and info macros? Surely there are
generic ones somewhere.
They are very useful to LinuxPPS users who can enable/disable them by
configuration menu.
You misunderstand. I'm not saying remove the callsites. I'm saying
remove the definitions.
Because we already have things like pr_debug() and pr_info(), so new code
should use those rather than reinventing them.
Plus, we already have at least 52 different implementations of dbg in the
tree and your 53rd one didn't compile because it clashed with someone
else's. This is the compiler sending us a message: use the exiting
infrastructure. If that infrastructure is insufficient then let's
improve 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Fri, May 11, 2007 at 11:17:11PM -0700, Andrew Morton wrote:
On Fri, 11 May 2007 23:55:37 +0200
Rodolfo Giometti [EMAIL PROTECTED] wrote:
Hello,
here my new patch with a lot of fixes.
The only issue not still fixed is the one related with:
#define NETLINK_PPSAPI 20
I need time to resolve it.
Follows my comments and then the patch, hope now I can came back into
-mm tree again! :)
Well I suppose I could toss it in there for a bit of review-and-test. But
I'll need to drop it again because we do need to split this patch into the
series
of patches, please.
You should do this earlier rather than later because it improves
reviewability.
- This:
static void pps_class_release(struct class_device *cdev)
{
/* Nop??? */
}
is a bug and it earns you a nastygram from Greg. These objects must be
dynamically allocated - this is not optional.
It could be acceptable defining this function as void?
No, it needs to be a proper release function, like all the other ones
around the place.
This comes up again and again and again and I recently asked Greg to direct
me to (or to write) suitable documentation, and I think he did, but I lost
it. Greg, can you remind us please?
I need to put it in some permanent place, but basically the problem is
that if you need to shut the kernel up by using an empty function for a
release, then you are not understanding why the kernel was trying to
warn you in the first place :(
You need to free your memory for the class_device in the release
function, you can not have a static structure, or rely on some other
reference count to handle the cleanup properly.
Also note that 'struct class_device' is going away and you should use
'struct device' instead. That too needs to be documented better, and is
on my list of things to do...
thanks,
greg k-h
-
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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
BTW, please remove the linuxpps list from the CC: for future postings in this thread, it bounces every one of my emails back because it only allows postings from subscribers. Thanks a lot. - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
From: Rodolfo Giometti <[EMAIL PROTECTED]> Date: Thu, 10 May 2007 13:45:03 +0200 > On Thu, May 10, 2007 at 04:01:52AM -0700, David Miller wrote: > > > > It's not OK, please use the generic netlink interface and as > > such you will not need to allocate any numbers at all. > > > > Documentation/networking/generic_netlink.txt gives a link > > to some infomration on this topic. > > If I well understand doing like this means that I have to modify also > userland API, is that true? Yes. > I know that you are forcing in using this new interface for new kernel > projects, but if I have to change my code I need also change NTPD > related code and this is frustrating. I have to interact with both > kernel developers and NTPD ones... :) > > It could be acceptable let me use id "20" (or just another number) to > allow Andrew Morton and other LinuxPPS users to test this new support? > Please, consider that this is not a new project, it was developed > since 2005 when this new interface was not available. Being a 2005 project means only that you've been out of tree for nearly 2 years. Sorry, we are not allocating a netlink IDs for folks, and we're doing it exactly because generic netlink avoids all the fixed numbering API issues. - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Thu, May 10, 2007 at 04:01:52AM -0700, David Miller wrote: > > It's not OK, please use the generic netlink interface and as > such you will not need to allocate any numbers at all. > > Documentation/networking/generic_netlink.txt gives a link > to some infomration on this topic. If I well understand doing like this means that I have to modify also userland API, is that true? I know that you are forcing in using this new interface for new kernel projects, but if I have to change my code I need also change NTPD related code and this is frustrating. I have to interact with both kernel developers and NTPD ones... :) It could be acceptable let me use id "20" (or just another number) to allow Andrew Morton and other LinuxPPS users to test this new support? Please, consider that this is not a new project, it was developed since 2005 when this new interface was not available. Thanks for your attention, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
From: Rodolfo Giometti <[EMAIL PROTECTED]> Date: Thu, 10 May 2007 12:58:37 +0200 > On Thu, May 10, 2007 at 12:27:40AM -0700, Andrew Morton wrote: > > > > Please check with Dave Miller that this: > > > > #define NETLINK_PPSAPI 20 > > > > reservation is OK. > > Hello, as you can see here Andrew Morton asked to me to check with you > about NETLINK_PPSAPI reservation. > > Please, let me know if it's ok. It's not OK, please use the generic netlink interface and as such you will not need to allocate any numbers at all. Documentation/networking/generic_netlink.txt gives a link to some infomration on this topic. - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Thu, May 10, 2007 at 12:27:40AM -0700, Andrew Morton wrote: > > Please check with Dave Miller that this: > > #define NETLINK_PPSAPI 20 > > reservation is OK. Hello, as you can see here Andrew Morton asked to me to check with you about NETLINK_PPSAPI reservation. Please, let me know if it's ok. Thanks, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Thu, 10 May 2007 00:27:40 -0700 Andrew Morton <[EMAIL PROTECTED]> wrote: > - Can we get rid of the private dbg, err and info macros? Surely there are > generic ones somewhere. i386 allmodconfig: In file included from drivers/usb/misc/uss720.c:48: include/linux/usb.h:1505:1: warning: "dbg" redefined In file included from include/linux/parport.h:108, from drivers/usb/misc/uss720.c:46: include/linux/pps.h:139:1: warning: this is the location of the previous definition In file included from drivers/usb/misc/uss720.c:48: include/linux/usb.h:1511:1: warning: "err" redefined In file included from include/linux/parport.h:108, from drivers/usb/misc/uss720.c:46: include/linux/pps.h:145:1: warning: this is the location of the previous definition In file included from drivers/usb/misc/uss720.c:48: include/linux/usb.h:1513:1: warning: "info" redefined In file included from include/linux/parport.h:108, from drivers/usb/misc/uss720.c:46: include/linux/pps.h:147:1: warning: this is the location of the previous definition - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, 2 May 2007 21:33:15 +0200 Rodolfo Giometti <[EMAIL PROTECTED]> wrote:
> Pulse per Second (PPS) support for Linux.
Have a patch:
From: Andrew Morton <[EMAIL PROTECTED]>
Review comments:
- Running a timer once per second will make the super-low-power people upset.
- This uses netlink? Is that interface documented anywhere?
Please check with Dave Miller that this:
#define NETLINK_PPSAPI 20
reservation is OK.
- This:
if ((nlpps->tsformat != PPS_TSFMT_TSPEC) != 0 ) {
is weird. I changed it to
if (nlpps->tsformat != PPS_TSFMT_TSPEC) {
- This:
timeout += nlpps->timeout.tv_nsec/(10/HZ);
probably won't work on i386. We use do_div() for 64/32 divides. I'll
find out when I compile it.
It's nice to use NSEC_PER_SEC rather than having to count all those
zeroes.
- The code uses interruptible_sleep_on_timeout(). That API is deprecated
and is racy. Please convert to wait_event_interruptible_timeout().
Ditto interruptible_sleep_on()
- This:
memset(pps_source, 0, sizeof(struct pps_s) * PPS_MAX_SOURCES);
was unneeded. The C startup code already did that.
- All these separators:
+/* --- Input function -- */
aren't typical for kernel code. I left them in, but please consider
removing them all.
- This:
static void pps_class_release(struct class_device *cdev)
{
/* Nop??? */
}
is a bug and it earns you a nastygram from Greg. These objects must be
dynamically allocated - this is not optional.
- What's this doing in 8250.c?
+ if (up->port.flags & UPF_HARDPPS_CD)
+ up->ier |= UART_IER_MSI;/* enable interrupts */
Please fully describe the reasons for this change in the changelog, and in
a code comment and then get the change reviewed by Russell King
<[EMAIL PROTECTED]>.
- Please document within the changelog the other changes to the serial code
and we'll ask Russell to take a look at those as well.
- The Kconfig purports to support CONFIG_PPS=m. Does that actually work?
We have a bunch of code in random other drivers which is dependent upon
CONFIG_PPS_CLIENT_foo. The problem is that if a kernel was compiled with
CONFIG_PPS_CLIENT_foo=n and then the pps driver is later built for that
kernel, it won't actually work because lp, serial etc weren't correctly
configured when _they_ were built.
This sort of cross-module coupling is considered to be a bad thing, but
I'm not really sure it's all that important.
- Please split the patch up into a series of patches: one for pps core and
one for each of the clients (servers?): one for lp, one for serial, etc.
Try to arrange for that series of patches to build and run at each stage
of application.
Please don't lose my changes when you do so ;)
Please review the changes I made and a) stick to the same style and b) fix
up any sites which I missed.
- Please remove all the typedefs:
+typedef struct ntp_fp {
+typedef union pps_timeu {
+typedef struct pps_info {
+typedef struct pps_params {
and just use `struct ntp_fp' everywhere.
- The above four structures are communicated with userspace, yes?
I believe that they will not work correctly when 32-bit userspace is
communicating with a 64-bit kernel. Alignments change and sizeof(long)
changes.
You don't want to have to write compat code. I suggest that you redo
those structures in terms of __u32, __u64, etc. You probably need to use
attribute((packed)) too, not sure.
Then let's get that part carefully reviewed (Arnd Bergmann <[EMAIL PROTECTED]>
is my go-to guru on this) and please test it carefully.
Yeah, you just haven't got a chance that something as huge and as complex
as struct pps_netlink_msg will survive the 32->64 transition.
- Please ensure that `make headers_check' passes OK (you'll hear from me if
it doesn't ;))
- Can we get rid of the private dbg, err and info macros? Surely there are
generic ones somewhere.
- struct pps_s has volatiles in it. Please remove them. There's lots of
discussion on this topic on linux-kernel just today.
- Why did the
port->icount.dcd++;
get moved in uart_handle_dcd_change()?
- In lots of places you do:
+#ifdef CONFIG_PPS_CLIENT_UART
+#include
+#endif
Please remove the ifdefs at all these sites and make the header file
handle it.
- It no longer compiles, because netlink_kernel_create now requires a
`struct mutex *'. I stuck a NULL in there, which is permitted. But I don't
know if that was a good thing - please check this.
Please also chase the net guys with a pointy stick for failing to document
their damned APIs.
- Generally: code looks OK and is probably useful. Please keep going ;)
Code changes:
- fix docs a bit
- uninline lp_pps_echo(): we take its address.
- remove unneeded and undesirable casts of void*'s
-
Re: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, 2 May 2007 21:33:15 +0200 Rodolfo Giometti [EMAIL PROTECTED] wrote:
Pulse per Second (PPS) support for Linux.
Have a patch:
From: Andrew Morton [EMAIL PROTECTED]
Review comments:
- Running a timer once per second will make the super-low-power people upset.
- This uses netlink? Is that interface documented anywhere?
Please check with Dave Miller that this:
#define NETLINK_PPSAPI 20
reservation is OK.
- This:
if ((nlpps-tsformat != PPS_TSFMT_TSPEC) != 0 ) {
is weird. I changed it to
if (nlpps-tsformat != PPS_TSFMT_TSPEC) {
- This:
timeout += nlpps-timeout.tv_nsec/(10/HZ);
probably won't work on i386. We use do_div() for 64/32 divides. I'll
find out when I compile it.
It's nice to use NSEC_PER_SEC rather than having to count all those
zeroes.
- The code uses interruptible_sleep_on_timeout(). That API is deprecated
and is racy. Please convert to wait_event_interruptible_timeout().
Ditto interruptible_sleep_on()
- This:
memset(pps_source, 0, sizeof(struct pps_s) * PPS_MAX_SOURCES);
was unneeded. The C startup code already did that.
- All these separators:
+/* --- Input function -- */
aren't typical for kernel code. I left them in, but please consider
removing them all.
- This:
static void pps_class_release(struct class_device *cdev)
{
/* Nop??? */
}
is a bug and it earns you a nastygram from Greg. These objects must be
dynamically allocated - this is not optional.
- What's this doing in 8250.c?
+ if (up-port.flags UPF_HARDPPS_CD)
+ up-ier |= UART_IER_MSI;/* enable interrupts */
Please fully describe the reasons for this change in the changelog, and in
a code comment and then get the change reviewed by Russell King
[EMAIL PROTECTED].
- Please document within the changelog the other changes to the serial code
and we'll ask Russell to take a look at those as well.
- The Kconfig purports to support CONFIG_PPS=m. Does that actually work?
We have a bunch of code in random other drivers which is dependent upon
CONFIG_PPS_CLIENT_foo. The problem is that if a kernel was compiled with
CONFIG_PPS_CLIENT_foo=n and then the pps driver is later built for that
kernel, it won't actually work because lp, serial etc weren't correctly
configured when _they_ were built.
This sort of cross-module coupling is considered to be a bad thing, but
I'm not really sure it's all that important.
- Please split the patch up into a series of patches: one for pps core and
one for each of the clients (servers?): one for lp, one for serial, etc.
Try to arrange for that series of patches to build and run at each stage
of application.
Please don't lose my changes when you do so ;)
Please review the changes I made and a) stick to the same style and b) fix
up any sites which I missed.
- Please remove all the typedefs:
+typedef struct ntp_fp {
+typedef union pps_timeu {
+typedef struct pps_info {
+typedef struct pps_params {
and just use `struct ntp_fp' everywhere.
- The above four structures are communicated with userspace, yes?
I believe that they will not work correctly when 32-bit userspace is
communicating with a 64-bit kernel. Alignments change and sizeof(long)
changes.
You don't want to have to write compat code. I suggest that you redo
those structures in terms of __u32, __u64, etc. You probably need to use
attribute((packed)) too, not sure.
Then let's get that part carefully reviewed (Arnd Bergmann [EMAIL PROTECTED]
is my go-to guru on this) and please test it carefully.
Yeah, you just haven't got a chance that something as huge and as complex
as struct pps_netlink_msg will survive the 32-64 transition.
- Please ensure that `make headers_check' passes OK (you'll hear from me if
it doesn't ;))
- Can we get rid of the private dbg, err and info macros? Surely there are
generic ones somewhere.
- struct pps_s has volatiles in it. Please remove them. There's lots of
discussion on this topic on linux-kernel just today.
- Why did the
port-icount.dcd++;
get moved in uart_handle_dcd_change()?
- In lots of places you do:
+#ifdef CONFIG_PPS_CLIENT_UART
+#include linux/pps.h
+#endif
Please remove the ifdefs at all these sites and make the header file
handle it.
- It no longer compiles, because netlink_kernel_create now requires a
`struct mutex *'. I stuck a NULL in there, which is permitted. But I don't
know if that was a good thing - please check this.
Please also chase the net guys with a pointy stick for failing to document
their damned APIs.
- Generally: code looks OK and is probably useful. Please keep going ;)
Code changes:
- fix docs a bit
- uninline lp_pps_echo(): we take its address.
- remove unneeded and undesirable casts of void*'s
- coding-style
Re: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Thu, 10 May 2007 00:27:40 -0700 Andrew Morton [EMAIL PROTECTED] wrote: - Can we get rid of the private dbg, err and info macros? Surely there are generic ones somewhere. i386 allmodconfig: In file included from drivers/usb/misc/uss720.c:48: include/linux/usb.h:1505:1: warning: dbg redefined In file included from include/linux/parport.h:108, from drivers/usb/misc/uss720.c:46: include/linux/pps.h:139:1: warning: this is the location of the previous definition In file included from drivers/usb/misc/uss720.c:48: include/linux/usb.h:1511:1: warning: err redefined In file included from include/linux/parport.h:108, from drivers/usb/misc/uss720.c:46: include/linux/pps.h:145:1: warning: this is the location of the previous definition In file included from drivers/usb/misc/uss720.c:48: include/linux/usb.h:1513:1: warning: info redefined In file included from include/linux/parport.h:108, from drivers/usb/misc/uss720.c:46: include/linux/pps.h:147:1: warning: this is the location of the previous definition - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Thu, May 10, 2007 at 12:27:40AM -0700, Andrew Morton wrote: Please check with Dave Miller that this: #define NETLINK_PPSAPI 20 reservation is OK. Hello, as you can see here Andrew Morton asked to me to check with you about NETLINK_PPSAPI reservation. Please, let me know if it's ok. Thanks, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
From: Rodolfo Giometti [EMAIL PROTECTED] Date: Thu, 10 May 2007 12:58:37 +0200 On Thu, May 10, 2007 at 12:27:40AM -0700, Andrew Morton wrote: Please check with Dave Miller that this: #define NETLINK_PPSAPI 20 reservation is OK. Hello, as you can see here Andrew Morton asked to me to check with you about NETLINK_PPSAPI reservation. Please, let me know if it's ok. It's not OK, please use the generic netlink interface and as such you will not need to allocate any numbers at all. Documentation/networking/generic_netlink.txt gives a link to some infomration on this topic. - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Thu, May 10, 2007 at 04:01:52AM -0700, David Miller wrote: It's not OK, please use the generic netlink interface and as such you will not need to allocate any numbers at all. Documentation/networking/generic_netlink.txt gives a link to some infomration on this topic. If I well understand doing like this means that I have to modify also userland API, is that true? I know that you are forcing in using this new interface for new kernel projects, but if I have to change my code I need also change NTPD related code and this is frustrating. I have to interact with both kernel developers and NTPD ones... :) It could be acceptable let me use id 20 (or just another number) to allow Andrew Morton and other LinuxPPS users to test this new support? Please, consider that this is not a new project, it was developed since 2005 when this new interface was not available. Thanks for your attention, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
From: Rodolfo Giometti [EMAIL PROTECTED] Date: Thu, 10 May 2007 13:45:03 +0200 On Thu, May 10, 2007 at 04:01:52AM -0700, David Miller wrote: It's not OK, please use the generic netlink interface and as such you will not need to allocate any numbers at all. Documentation/networking/generic_netlink.txt gives a link to some infomration on this topic. If I well understand doing like this means that I have to modify also userland API, is that true? Yes. I know that you are forcing in using this new interface for new kernel projects, but if I have to change my code I need also change NTPD related code and this is frustrating. I have to interact with both kernel developers and NTPD ones... :) It could be acceptable let me use id 20 (or just another number) to allow Andrew Morton and other LinuxPPS users to test this new support? Please, consider that this is not a new project, it was developed since 2005 when this new interface was not available. Being a 2005 project means only that you've been out of tree for nearly 2 years. Sorry, we are not allocating a netlink IDs for folks, and we're doing it exactly because generic netlink avoids all the fixed numbering API issues. - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
BTW, please remove the linuxpps list from the CC: for future postings in this thread, it bounces every one of my emails back because it only allows postings from subscribers. Thanks a lot. - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, May 02, 2007 at 02:06:53PM -0700, john stultz wrote: > > Please inline your patch, rather then attaching them. It makes it very > difficult to discuss when it is attached. Ok. > > +++ b/drivers/pps/clients/ktimer.c > > @@ -0,0 +1,106 @@ > > +/* > > + * ktimer.c -- kernel timer test client > > + * > > Could you use a better name, like pps_ktimer_test.c, so it is less generic? Ok. But consider that this is just a testing program. > Same goes for your kabi.c and sysfs.c files. Why? These files don't generate .ko files. > > @@ -2004,6 +2004,8 @@ serial8250_set_termios(struct uart_port *port, struct > > ktermios *termios, > > up->ier |= UART_IER_MSI; > > if (up->capabilities & UART_CAP_UUE) > > up->ier |= UART_IER_UUE | UART_IER_RTOIE; > > + if (up->port.flags & UPF_HARDPPS_CD) > > + up->ier |= UART_IER_MSI;/* enable interrupts */ > > > > This isn't covered by a #ifdef, so is this always safe? Should it be in > a separate patch? No, this part is regarding the serial driver itself. Maybe it can be placed in a separate patch but it is still about the PPS support for Linux... > Unfortunately I don't have any hardware to play with this, but I'd > suggest you send this to Andrew Morton for inclusion into his tree for > testing. I'll do it. Thanks, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, May 02, 2007 at 02:06:53PM -0700, john stultz wrote: Please inline your patch, rather then attaching them. It makes it very difficult to discuss when it is attached. Ok. +++ b/drivers/pps/clients/ktimer.c @@ -0,0 +1,106 @@ +/* + * ktimer.c -- kernel timer test client + * Could you use a better name, like pps_ktimer_test.c, so it is less generic? Ok. But consider that this is just a testing program. Same goes for your kabi.c and sysfs.c files. Why? These files don't generate .ko files. @@ -2004,6 +2004,8 @@ serial8250_set_termios(struct uart_port *port, struct ktermios *termios, up-ier |= UART_IER_MSI; if (up-capabilities UART_CAP_UUE) up-ier |= UART_IER_UUE | UART_IER_RTOIE; + if (up-port.flags UPF_HARDPPS_CD) + up-ier |= UART_IER_MSI;/* enable interrupts */ This isn't covered by a #ifdef, so is this always safe? Should it be in a separate patch? No, this part is regarding the serial driver itself. Maybe it can be placed in a separate patch but it is still about the PPS support for Linux... Unfortunately I don't have any hardware to play with this, but I'd suggest you send this to Andrew Morton for inclusion into his tree for testing. I'll do it. Thanks, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, 2007-05-02 at 21:33 +0200, Rodolfo Giometti wrote: > Pulse per Second (PPS) support for Linux. > > Signed-off-by: Rodolfo Giometti <[EMAIL PROTECTED]> > > --- > > Here my last release of PPS support for Linux. > > The difference against my last patch is about all userland specific > code (timepps.h) which has been removed, I hope now you can consider > adding it into kernel source tree! > > Please, let me know if I still should fix something else. Please inline your patch, rather then attaching them. It makes it very difficult to discuss when it is attached. > diff --git a/drivers/pps/clients/ktimer.c b/drivers/pps/clients/ktimer.c > new file mode 100644 > index 000..7514389 > --- /dev/null > +++ b/drivers/pps/clients/ktimer.c > @@ -0,0 +1,106 @@ > +/* > + * ktimer.c -- kernel timer test client > + * Could you use a better name, like pps_ktimer_test.c, so it is less generic? Same goes for your kabi.c and sysfs.c files. > diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c > index 98ec861..543c7cb 100644 > --- a/drivers/serial/8250.c > +++ b/drivers/serial/8250.c > @@ -2004,6 +2004,8 @@ serial8250_set_termios(struct uart_port *port, struct > ktermios *termios, > up->ier |= UART_IER_MSI; > if (up->capabilities & UART_CAP_UUE) > up->ier |= UART_IER_UUE | UART_IER_RTOIE; > + if (up->port.flags & UPF_HARDPPS_CD) > + up->ier |= UART_IER_MSI;/* enable interrupts */ > This isn't covered by a #ifdef, so is this always safe? Should it be in a separate patch? Unfortunately I don't have any hardware to play with this, but I'd suggest you send this to Andrew Morton for inclusion into his tree for testing. thanks -john - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, 2007-05-02 at 21:33 +0200, Rodolfo Giometti wrote: Pulse per Second (PPS) support for Linux. Signed-off-by: Rodolfo Giometti [EMAIL PROTECTED] --- Here my last release of PPS support for Linux. The difference against my last patch is about all userland specific code (timepps.h) which has been removed, I hope now you can consider adding it into kernel source tree! Please, let me know if I still should fix something else. Please inline your patch, rather then attaching them. It makes it very difficult to discuss when it is attached. diff --git a/drivers/pps/clients/ktimer.c b/drivers/pps/clients/ktimer.c new file mode 100644 index 000..7514389 --- /dev/null +++ b/drivers/pps/clients/ktimer.c @@ -0,0 +1,106 @@ +/* + * ktimer.c -- kernel timer test client + * Could you use a better name, like pps_ktimer_test.c, so it is less generic? Same goes for your kabi.c and sysfs.c files. diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c index 98ec861..543c7cb 100644 --- a/drivers/serial/8250.c +++ b/drivers/serial/8250.c @@ -2004,6 +2004,8 @@ serial8250_set_termios(struct uart_port *port, struct ktermios *termios, up-ier |= UART_IER_MSI; if (up-capabilities UART_CAP_UUE) up-ier |= UART_IER_UUE | UART_IER_RTOIE; + if (up-port.flags UPF_HARDPPS_CD) + up-ier |= UART_IER_MSI;/* enable interrupts */ This isn't covered by a #ifdef, so is this always safe? Should it be in a separate patch? Unfortunately I don't have any hardware to play with this, but I'd suggest you send this to Andrew Morton for inclusion into his tree for testing. thanks -john - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Mar 21, 2007 at 09:05:34AM +0100, Jon K Hellan wrote: > Have you received any comments on this from the NTP community? From > Ulrich Windl? Well as just a user, this patch seems to work, and is actively being worked on and maintained for modern kernels (which unforunately Ulrich Windl's patch hardly appears to be). This one also seems much less intrusive, doesn't break serial console systems (I sent a patch to Ulrich Windl to fix that, due to one of many places where pointers are dereferenced without being checked for !NULL first). Is LinuxPPS better? I have no idea, and I don't think I am qualified to say one way or another. I do know that I could apply it to 2.6.18 with minimal work, while I couldn't get Ulrich Windl's patch to do so (too much has changed since 2.6.15, while I did manage to get it to apply to 2.6.16 with quite a bit of work). Having PPS support actually part of the kernel would make it much simpler to deal with in the future, and perhaps ntp would start being compiled with such support regularly. -- Len Sorensen - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Mar 21, 2007 at 09:05:34AM +0100, Jon K Hellan wrote: > Have you received any comments on this from the NTP community? From > Ulrich Windl? Not yet... that's why I reposted my patch. Thanks, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
Rodolfo Giometti wrote: Pulse per Second (PPS) support for Linux. Signed-off-by: Rodolfo Giometti <[EMAIL PROTECTED]> --- Please, note that this PPS implementation is not RFC 2783 fully compatible since, IMHO, the RFC simply doesn't consider PPS devices connected with special GPIOs or other ports different from serial ports and parallele ports. Have you received any comments on this from the NTP community? From Ulrich Windl? Jon Kåre Hellan, UNINETT, Trondheim, Norway - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
Rodolfo Giometti wrote: Pulse per Second (PPS) support for Linux. Signed-off-by: Rodolfo Giometti [EMAIL PROTECTED] --- Please, note that this PPS implementation is not RFC 2783 fully compatible since, IMHO, the RFC simply doesn't consider PPS devices connected with special GPIOs or other ports different from serial ports and parallele ports. Have you received any comments on this from the NTP community? From Ulrich Windl? Jon Kåre Hellan, UNINETT, Trondheim, Norway - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Mar 21, 2007 at 09:05:34AM +0100, Jon K Hellan wrote: Have you received any comments on this from the NTP community? From Ulrich Windl? Not yet... that's why I reposted my patch. Thanks, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Mar 21, 2007 at 09:05:34AM +0100, Jon K Hellan wrote: Have you received any comments on this from the NTP community? From Ulrich Windl? Well as just a user, this patch seems to work, and is actively being worked on and maintained for modern kernels (which unforunately Ulrich Windl's patch hardly appears to be). This one also seems much less intrusive, doesn't break serial console systems (I sent a patch to Ulrich Windl to fix that, due to one of many places where pointers are dereferenced without being checked for !NULL first). Is LinuxPPS better? I have no idea, and I don't think I am qualified to say one way or another. I do know that I could apply it to 2.6.18 with minimal work, while I couldn't get Ulrich Windl's patch to do so (too much has changed since 2.6.15, while I did manage to get it to apply to 2.6.16 with quite a bit of work). Having PPS support actually part of the kernel would make it much simpler to deal with in the future, and perhaps ntp would start being compiled with such support regularly. -- Len Sorensen - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Thu, Mar 15, 2007 at 11:18:55AM -0400, Lennart Sorensen wrote: > How come none of the .patch files in > http://ftp.enneenne.com/pub/misc/linuxpps/refclocks/nmea/ can be > accessed? Does your web server not like serving up .patch files? Sorry. I set wrong file permissions. :) Try now. Ciao, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Thu, Mar 15, 2007 at 11:29:12AM +0100, Rodolfo Giometti wrote: > Can you please provide a little help about it? A patch against current > wiki wuold be great! ;) Well all I actually did was simply stick #define HAVE_PPSAPI at the tome of the refclock_nmea.c file. The configure script in ntp is explcitly checking for PPS API version 1 so version 2 doesn't match and it claims you don't have a timepps.h file it can use. Unfortunately I really have trouble understanding autoconf scripts and can't figure out where the code is that tells it how to do the check for timepps.h and the API version inside it. > I modify your code add inquiry functionality. Can you please test it? > > See http://ftp.enneenne.com/pub/misc/linuxpps/test/ and the wiki at > http://wiki.enneenne.com/index.php/LinuxPPS_support#Compiling_the_code. I will try out the ppstest tool. How come none of the .patch files in http://ftp.enneenne.com/pub/misc/linuxpps/refclocks/nmea/ can be accessed? Does your web server not like serving up .patch files? -- Len Sorensen - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Mar 14, 2007 at 04:57:32PM -0400, Lennart Sorensen wrote:
>
> Well it does work for our GPS receiver at least. Of course I have to
> change the baud rate in the driver since our unit doens't use the NNEA
> standard 4800. And the configure script for ntp doesn't recognize the
> v2 PPSAPI, so I have to manually explain to it that I have the PPS API
> and it should actually include it.
Can you please provide a little help about it? A patch against current
wiki wuold be great! ;)
> Here is my utility for enabling PPS on my serial port now. Seems less
> of a pain that patching setserial and including that (with 256MB flash
> space, setserial seems wasteful).
I modify your code add inquiry functionality. Can you please test it?
See http://ftp.enneenne.com/pub/misc/linuxpps/test/ and the wiki at
http://wiki.enneenne.com/index.php/LinuxPPS_support#Compiling_the_code.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail:[EMAIL PROTECTED]
Linux Device Driver [EMAIL PROTECTED]
Embedded Systems[EMAIL PROTECTED]
UNIX programming phone: +39 349 2432127
#include
#include
#include
#include
#include
#include
#include
#include
#include
#include
void usage(char *name)
{
fprintf(stderr, "usage: %s [enable|disable]\n", name);
exit(EXIT_FAILURE);
}
int main(int argc, char *argv[])
{
int fd, ret;
struct serial_struct ss;
if (argc < 2)
usage(argv[0]);
fd = open(argv[1], O_RDWR);
if (fd < 0) {
perror("open");
exit(EXIT_FAILURE);
}
ret = ioctl(fd, TIOCGSERIAL, );
if (ret < 0 ) {
perror("ioctl(TIOCGSERIAL)");
exit(EXIT_FAILURE);
}
if (argc < 3) { /* just read PPS status */
printf("PPS is %sabled\n",
ss.flags & ASYNC_HARDPPS_CD ? "en" : "dis");
exit(EXIT_SUCCESS);
}
if (argv[2][0] == 'e' || argv[2][0] == '1')
ss.flags |= ASYNC_HARDPPS_CD;
else if (argv[2][0] == 'd' || argv[2][0] == '0')
ss.flags &= ~ASYNC_HARDPPS_CD;
else {
fprintf(stderr, "invalid state argument \"%s\"\n", argv[2]);
exit(EXIT_FAILURE);
}
ret = ioctl(fd, TIOCSSERIAL, );
if (ret < 0) {
perror("ioctl(TIOCSSERIAL)");
exit(EXIT_FAILURE);
}
return 0;
}
Re: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Mar 14, 2007 at 04:57:32PM -0400, Lennart Sorensen wrote:
Well it does work for our GPS receiver at least. Of course I have to
change the baud rate in the driver since our unit doens't use the NNEA
standard 4800. And the configure script for ntp doesn't recognize the
v2 PPSAPI, so I have to manually explain to it that I have the PPS API
and it should actually include it.
Can you please provide a little help about it? A patch against current
wiki wuold be great! ;)
Here is my utility for enabling PPS on my serial port now. Seems less
of a pain that patching setserial and including that (with 256MB flash
space, setserial seems wasteful).
I modify your code add inquiry functionality. Can you please test it?
See http://ftp.enneenne.com/pub/misc/linuxpps/test/ and the wiki at
http://wiki.enneenne.com/index.php/LinuxPPS_support#Compiling_the_code.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail:[EMAIL PROTECTED]
Linux Device Driver [EMAIL PROTECTED]
Embedded Systems[EMAIL PROTECTED]
UNIX programming phone: +39 349 2432127
#include stdio.h
#include unistd.h
#include stdlib.h
#include errno.h
#include sys/ioctl.h
#include sys/types.h
#include sys/stat.h
#include fcntl.h
#include string.h
#include linux/serial.h
void usage(char *name)
{
fprintf(stderr, usage: %s ttyS [enable|disable]\n, name);
exit(EXIT_FAILURE);
}
int main(int argc, char *argv[])
{
int fd, ret;
struct serial_struct ss;
if (argc 2)
usage(argv[0]);
fd = open(argv[1], O_RDWR);
if (fd 0) {
perror(open);
exit(EXIT_FAILURE);
}
ret = ioctl(fd, TIOCGSERIAL, ss);
if (ret 0 ) {
perror(ioctl(TIOCGSERIAL));
exit(EXIT_FAILURE);
}
if (argc 3) { /* just read PPS status */
printf(PPS is %sabled\n,
ss.flags ASYNC_HARDPPS_CD ? en : dis);
exit(EXIT_SUCCESS);
}
if (argv[2][0] == 'e' || argv[2][0] == '1')
ss.flags |= ASYNC_HARDPPS_CD;
else if (argv[2][0] == 'd' || argv[2][0] == '0')
ss.flags = ~ASYNC_HARDPPS_CD;
else {
fprintf(stderr, invalid state argument \%s\\n, argv[2]);
exit(EXIT_FAILURE);
}
ret = ioctl(fd, TIOCSSERIAL, ss);
if (ret 0) {
perror(ioctl(TIOCSSERIAL));
exit(EXIT_FAILURE);
}
return 0;
}
Re: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Thu, Mar 15, 2007 at 11:29:12AM +0100, Rodolfo Giometti wrote: Can you please provide a little help about it? A patch against current wiki wuold be great! ;) Well all I actually did was simply stick #define HAVE_PPSAPI at the tome of the refclock_nmea.c file. The configure script in ntp is explcitly checking for PPS API version 1 so version 2 doesn't match and it claims you don't have a timepps.h file it can use. Unfortunately I really have trouble understanding autoconf scripts and can't figure out where the code is that tells it how to do the check for timepps.h and the API version inside it. I modify your code add inquiry functionality. Can you please test it? See http://ftp.enneenne.com/pub/misc/linuxpps/test/ and the wiki at http://wiki.enneenne.com/index.php/LinuxPPS_support#Compiling_the_code. I will try out the ppstest tool. How come none of the .patch files in http://ftp.enneenne.com/pub/misc/linuxpps/refclocks/nmea/ can be accessed? Does your web server not like serving up .patch files? -- Len Sorensen - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Thu, Mar 15, 2007 at 11:18:55AM -0400, Lennart Sorensen wrote: How come none of the .patch files in http://ftp.enneenne.com/pub/misc/linuxpps/refclocks/nmea/ can be accessed? Does your web server not like serving up .patch files? Sorry. I set wrong file permissions. :) Try now. Ciao, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Mar 14, 2007 at 04:47:32PM +0100, Rodolfo Giometti wrote:
> On Wed, Mar 14, 2007 at 11:37:05AM -0400, Lennart Sorensen wrote:
> > Well here is my current version of the refclock_nmea.c.patch for
> > LinuxPPS. It now uses /dev/gps# for the nmea messages and /dev/pps# for
> > the PPS device (which in my case is of course the same device). I am
> > running some more tests on it, but I think it is OK.
>
> Thanks, published. :)
Well it does work for our GPS receiver at least. Of course I have to
change the baud rate in the driver since our unit doens't use the NNEA
standard 4800. And the configure script for ntp doesn't recognize the
v2 PPSAPI, so I have to manually explain to it that I have the PPS API
and it should actually include it.
Here is my utility for enabling PPS on my serial port now. Seems less
of a pain that patching setserial and including that (with 256MB flash
space, setserial seems wasteful).
ppsctl.c
#include
#include
#include
#include
#include
#include
#include
void usage(char *name) {
fprintf(stderr,"Usage: %s /dev/port [0|1]\n",name);
fprintf(stderr,"Where 0 means disable PPS and 1 means enable PPS\n");
}
int main(int argc, char *argv[]) {
int fd;
char *state;
if(argc<3) {
usage(argv[0]);
return 1;
}
fd = open(argv[1],O_RDWR);
if (fd>=0) {
struct serial_struct ss;
if (ioctl(fd, TIOCGSERIAL, ) < 0 ) {
perror("TIOCGSERIAL");
return 1;
} else {
if(strcmp(argv[2],"1")==0) {
ss.flags |= ASYNC_HARDPPS_CD;
state="enabled";
} else if(strcmp(argv[2],"0")==0) {
ss.flags &= ~ASYNC_HARDPPS_CD;
state="disabled";
} else {
fprintf(stderr,"Invalid state argument
\"%s\"\n",argv[2]);
return 1;
}
if (ioctl(fd, TIOCSSERIAL, ) < 0) {
perror("TIOCSSERIAL");
} else {
fprintf(stderr,"PPS on %s is now
%s\n",argv[1],state);
}
}
} else {
fprintf(stderr,"Can't open \"%s\":",argv[1]);
perror("");
}
return 0;
}
/* vim:ts=4:shiftwidth=4:
* */
--
Len Sorensen
-
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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Mar 14, 2007 at 11:37:05AM -0400, Lennart Sorensen wrote: > Well here is my current version of the refclock_nmea.c.patch for > LinuxPPS. It now uses /dev/gps# for the nmea messages and /dev/pps# for > the PPS device (which in my case is of course the same device). I am > running some more tests on it, but I think it is OK. Thanks, published. :) Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
Well here is my current version of the refclock_nmea.c.patch for
LinuxPPS. It now uses /dev/gps# for the nmea messages and /dev/pps# for
the PPS device (which in my case is of course the same device). I am
running some more tests on it, but I think it is OK.
--- refclock_nmea.c.ori 2007-03-14 11:24:14.0 -0400
+++ refclock_nmea.c 2007-03-14 11:31:04.0 -0400
@@ -69,6 +69,7 @@
# define DEVICE "COM%d:" /* COM 1 - 3 supported */
#else
# define DEVICE"/dev/gps%d"/* name of radio device */
+# define PPSDEVICE "/dev/pps%d"/* name of pps device */
#endif
#defineSPEED232B4800 /* uart speed (4800 bps) */
#definePRECISION (-9)/* precision assumed (about 2 ms) */
@@ -79,6 +80,7 @@
#define RANGEGATE 50 /* range gate (ns) */
#define LENNMEA75 /* min timecode length */
+#define LENPPS PPS_MAX_NAME_LEN
/*
* Tables to compute the ddd of year form icky dd/mm timecode. Viva la
@@ -99,6 +101,7 @@
pps_params_t pps_params; /* pps parameters */
pps_info_t pps_info;/* last pps data */
pps_handle_t handle;/* pps handlebars */
+ int handle_created; /* pps handle created flag */
#endif /* HAVE_PPSAPI */
};
@@ -147,6 +150,11 @@
register struct nmeaunit *up;
struct refclockproc *pp;
int fd;
+#ifdef PPS_HAVE_FINDPATH
+ char id[LENPPS] = "",
+path[LENPPS],
+ppsdevice[LENPPS] = "";/* just a default device */
+#endif /* PPS_HAVE_FINDPATH */
char device[20];
/*
@@ -238,12 +246,26 @@
* Start the PPSAPI interface if it is there. Default to use
* the assert edge and do not enable the kernel hardpps.
*/
+#ifdef PPS_HAVE_FINDPATH
+ /* Get the PPS source's real name */
+ (void)sprintf(ppsdevice, PPSDEVICE, unit);
+ time_pps_readlink(ppsdevice, LENPPS, path, LENPPS);
+
+ /* Try to find the source */
+ fd = time_pps_findpath(path, LENPPS, id, LENPPS);
+ if (fd < 0) {
+ msyslog(LOG_ERR, "refclock_nmea: cannot find PPS path \"%s\" in
the system", path);
+ return (0);
+ }
+ msyslog(LOG_INFO, "refclock_nmea: found PPS source \"%s\" at id #%d on
\"%s\"", path, fd, id);
+#endif /* PPS_HAVE_FINDPATH */
if (time_pps_create(fd, >handle) < 0) {
- up->handle = 0;
+ up->handle_created = 0;
msyslog(LOG_ERR,
"refclock_nmea: time_pps_create failed: %m");
return (1);
}
+ up->handle_created = ~0;
return(nmea_ppsapi(peer, 0, 0));
#else
return (1);
@@ -265,8 +287,10 @@
pp = peer->procptr;
up = (struct nmeaunit *)pp->unitptr;
#ifdef HAVE_PPSAPI
- if (up->handle != 0)
+ if (up->handle_created) {
time_pps_destroy(up->handle);
+ up->handle_created = 0;
+ }
#endif /* HAVE_PPSAPI */
io_closeclock(>io);
free(up);
@@ -374,7 +398,7 @@
/*
* Convert the timespec nanoseconds field to ntp l_fp units.
*/
- if (up->handle == 0)
+ if (!up->handle_created)
return (0);
timeout.tv_sec = 0;
timeout.tv_nsec = 0;
--
Len Sorensen
-
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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Mar 14, 2007 at 10:42:51AM -0400, Lennart Sorensen wrote: > > Now you said to check the return value of time_pps_readlink. Well it I refere to readlink(), not to time_pps_readlink(). I'm sorry for mistake. > If the call was to readlink directly it needs to be done, while your > time_pps_readlink is just a handy wrapper that does all that for you, > right? Right. Ciao, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Mar 14, 2007 at 03:27:12PM +0100, Rodolfo Giometti wrote: > Ok! Thanks. > > Yes, this could be a good solution while waiting for a new setserial. Now you said to check the return value of time_pps_readlink. Well it returns void so that isn't much good, and the stuff the wiki says to do, appears to be done internally by the function. So I guess that means I should undo that change so that I can actually get ntpd to compile again. If the call was to readlink directly it needs to be done, while your time_pps_readlink is just a handy wrapper that does all that for you, right? -- Len Sorensen - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Mar 14, 2007 at 10:12:53AM -0400, Lennart Sorensen wrote: > > I looked at those, and they didn't sound important. I will grab it > anyhow just for completeness. Ok! Thanks. > Well I think I may just write a small tool to do the ioctl to enable it > so I don't need the full setserial around. Yes, this could be a good solution while waiting for a new setserial. Ciao, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Mar 14, 2007 at 03:06:23PM +0100, Rodolfo Giometti wrote: > Some fixes up... see the git log for further info. I looked at those, and they didn't sound important. I will grab it anyhow just for completeness. > Unluckely you need a patched version of setserial (see the patch on my > site). On the same site you can find a precompiled version which I use > for my tests, maybe it works for you... OK, that explains why it wasn't in setserial's code. > This is a specific problem of NTPD not of LinuxPPS itself. I wrote > some letters about this problem into NTP list but with no results. > > The sysadm shoulkd use setserial to enable a serial port to become a > PPS source and then NTPD should verify if such PPS source exists > (using time_pps_findpath() & Co.). Well I think I may just write a small tool to do the ioctl to enable it so I don't need the full setserial around. > Did you read this example on the wiki? > >[EMAIL PROTECTED]:~/linuxpps$ cat /sys/class/pps/01/name >serial1 >[EMAIL PROTECTED]:~/linuxpps$ cat /sys/class/pps/01/path >/dev/ttyS1 >giometti at jeeg:~/linuxpps/test$ sudo ln -sf /dev/ttyS1 /dev/gps0 >giometti at jeeg:~/linuxpps/test$ sudo ./ppstest /dev/gps0 >found PPS source #2 "serial1" on "/dev/ttyS1" >giometti at jeeg:~/linuxpps/test$ sudo ln -sf ktimer /dev/gps0 >giometti at jeeg:~/linuxpps/test$ sudo ./ppstest /dev/gps0 >found PPS source #0 "ktimer" on "" > > it doesn't work for you? I will have to try that again to be sure. I may have got myself confused when I first tried it. One symlink I can deal with (that's ntpd sillyness after all). -- Len Sorensen - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Mar 14, 2007 at 09:19:34AM -0400, Lennart Sorensen wrote: > > I will grab the last couple of commits and try although they didn't > sound like they really make much difference. Some fixes up... see the git log for further info. > I couldn't find any way to do that with setserial (at least not the > version I have), and I would rather not have to install setserial just > to do that. Which version of setserial is needed and what arguments > does it need to do it? Unluckely you need a patched version of setserial (see the patch on my site). On the same site you can find a precompiled version which I use for my tests, maybe it works for you... > If it is NOT connected to the same device, then how would you specify > it? The ntp configuration is rather sparse when it comes to specifying > anything and seems to rely in symlinks to hardcoded device names for > finding everything. I suppose one could have gps# for the nmea messages > and pps# for the associated pps device name symlink (which may point > to something that doesn't even exist if there is an internal source of > that name with no associated device). Does that seem reasonable? I can > certainly change it to do that. Certainly refclock_atom already uses > /dev/pps# as it's device, so using that again may be reasonable. This is a specific problem of NTPD not of LinuxPPS itself. I wrote some letters about this problem into NTP list but with no results. The sysadm shoulkd use setserial to enable a serial port to become a PPS source and then NTPD should verify if such PPS source exists (using time_pps_findpath() & Co.). > I actually find the way it determines the pps device a bit annoying. > Right now I have to do this: > > cd /dev > ln -s ttyn0 jsm0 > ln -s jsm0 gps0 > > This way gps0 is the symlink the ntp refclock looks for when asked for > device 0, and readlink turns that into jsm0 (since the internal driver > name for ttyn0 is jsm, that is what the pps code insists it must be > named), which then is another symlink to the real device name. Same for > ttyS3 <- serial3 <- gps0. Now it would be nice if the internal driver > name matched the device name, but apparently that really never seems to > happen. Is all this symlink spagheti really necesary? Did you read this example on the wiki? [EMAIL PROTECTED]:~/linuxpps$ cat /sys/class/pps/01/name serial1 [EMAIL PROTECTED]:~/linuxpps$ cat /sys/class/pps/01/path /dev/ttyS1 giometti at jeeg:~/linuxpps/test$ sudo ln -sf /dev/ttyS1 /dev/gps0 giometti at jeeg:~/linuxpps/test$ sudo ./ppstest /dev/gps0 found PPS source #2 "serial1" on "/dev/ttyS1" giometti at jeeg:~/linuxpps/test$ sudo ln -sf ktimer /dev/gps0 giometti at jeeg:~/linuxpps/test$ sudo ./ppstest /dev/gps0 found PPS source #0 "ktimer" on "" it doesn't work for you? > Will do. Thanks a lot, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Mar 14, 2007 at 10:31:46AM +0100, Rodolfo Giometti wrote:
> On Tue, Mar 13, 2007 at 06:48:17PM -0400, Lennart Sorensen wrote:
> >
> > I have tried out 3.0.0-rc2 which seems to work pretty well so far (when
>
> Thanks. I just posted to the linux kernel ML the last release
> 3.0.0. Maybe you can do a "git pull" and try it out. :)
I will grab the last couple of commits and try although they didn't
sound like they really make much difference.
> > combined with the patches to the jsm driver I just posted). It took soe
> > work to get ntp's refclock_nmea to work though, since the patch that is
> > linked to from the linuxpps page seems out of date. Here is the patch
> > that seems to be working for me, although I am still testing it. Given
> > you know the linuxpps code better perhaps you can see if it looks sane
> > to you.
> >
> > --- ntpd/refclock_nmea.c.ori2007-03-13 18:38:01.0 -0400
> > +++ ntpd/refclock_nmea.c2007-03-13 18:44:47.0 -0400
> > @@ -79,6 +79,7 @@
> > #define RANGEGATE 50 /* range gate (ns) */
> >
> > #define LENNMEA75 /* min timecode length */
> > +#define LENPPS PPS_MAX_NAME_LEN
> >
> > /*
> > * Tables to compute the ddd of year form icky dd/mm timecode. Viva la
> > @@ -99,6 +100,7 @@
> > pps_params_t pps_params; /* pps parameters */
> > pps_info_t pps_info;/* last pps data */
> > pps_handle_t handle;/* pps handlebars */
> > + int handle_created; /* pps handle created flag */
> > #endif /* HAVE_PPSAPI */
> > };
> >
> > @@ -147,6 +149,11 @@
> > register struct nmeaunit *up;
> > struct refclockproc *pp;
> > int fd;
> > +#ifdef PPS_HAVE_FINDPATH
> > + char id[LENPPS] = "",
> > +path[LENPPS],
> > +mylink[LENPPS] = "";/* just a default device */
> > +#endif /* PPS_HAVE_FINDPATH */
> > char device[20];
> >
> > /*
> > @@ -201,7 +208,20 @@
> > #else
> > return (0);
> > #endif
> > -}
> > +} else {
> > +struct serial_struct ss;
> > +if (ioctl(fd, TIOCGSERIAL, ) < 0 ||
> > +(
> > +ss.flags |= ASYNC_HARDPPS_CD,
> > + ioctl(fd, TIOCSSERIAL, )) < 0) {
> > + msyslog(LOG_NOTICE, "refclock_nmea: TIOCSSERIAL fd %d,
> > %m", fd);
> > + msyslog(LOG_NOTICE,
> > + "refclock_nmea: optional PPS processing not
> > available");
> > +} else {
> > +msyslog(LOG_INFO,
> > +"refclock_nmea: PPS detection on");
> > +}
> > + }
>
> You should use "setserial" here. Keep in mind that the PPS source
> could be _not_ connected with the serial line at all.
I couldn't find any way to do that with setserial (at least not the
version I have), and I would rather not have to install setserial just
to do that. Which version of setserial is needed and what arguments
does it need to do it?
If it is NOT connected to the same device, then how would you specify
it? The ntp configuration is rather sparse when it comes to specifying
anything and seems to rely in symlinks to hardcoded device names for
finding everything. I suppose one could have gps# for the nmea messages
and pps# for the associated pps device name symlink (which may point
to something that doesn't even exist if there is an internal source of
that name with no associated device). Does that seem reasonable? I can
certainly change it to do that. Certainly refclock_atom already uses
/dev/pps# as it's device, so using that again may be reasonable.
> > /*
> > * Allocate and initialize unit structure
> > @@ -238,12 +258,26 @@
> > * Start the PPSAPI interface if it is there. Default to use
> > * the assert edge and do not enable the kernel hardpps.
> > */
> > +#ifdef PPS_HAVE_FINDPATH
> > + /* Get the PPS source's real name */
> > + //time_pps_readlink(mylink, LENPPS, path, LENPPS);
>
> Remove unneeded code.
Oops. Missed that one. I was changing it to the below line to reuse
the same serial device already specified for the NMEA messages.
I actually find the way it determines the pps device a bit annoying.
Right now I have to do this:
cd /dev
ln -s ttyn0 jsm0
ln -s jsm0 gps0
This way gps0 is the symlink the ntp refclock looks for when asked for
device 0, and readlink turns that into jsm0 (since the internal driver
name for ttyn0 is jsm, that is what the pps code insists it must be
named), which then is another symlink to the real device name. Same for
ttyS3 <- serial3 <- gps0. Now it would be nice if the internal driver
name matched the device name, but apparently that really never seems to
happen. Is all this symlink spagheti really necesary?
> > + time_pps_readlink(device, LENPPS, path, LENPPS);
>
> Test the return value (see the wiki at
> http://wiki.enneenne.com/index.php/LinuxPPS_support#How_to_modify_a_refclock_to_work_with_LinuxPPS).
OK, I will
Re: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Tue, Mar 13, 2007 at 06:48:17PM -0400, Lennart Sorensen wrote:
>
> I have tried out 3.0.0-rc2 which seems to work pretty well so far (when
Thanks. I just posted to the linux kernel ML the last release
3.0.0. Maybe you can do a "git pull" and try it out. :)
> combined with the patches to the jsm driver I just posted). It took soe
> work to get ntp's refclock_nmea to work though, since the patch that is
> linked to from the linuxpps page seems out of date. Here is the patch
> that seems to be working for me, although I am still testing it. Given
> you know the linuxpps code better perhaps you can see if it looks sane
> to you.
>
> --- ntpd/refclock_nmea.c.ori 2007-03-13 18:38:01.0 -0400
> +++ ntpd/refclock_nmea.c 2007-03-13 18:44:47.0 -0400
> @@ -79,6 +79,7 @@
> #define RANGEGATE50 /* range gate (ns) */
>
> #define LENNMEA 75 /* min timecode length */
> +#define LENPPS PPS_MAX_NAME_LEN
>
> /*
> * Tables to compute the ddd of year form icky dd/mm timecode. Viva la
> @@ -99,6 +100,7 @@
> pps_params_t pps_params; /* pps parameters */
> pps_info_t pps_info;/* last pps data */
> pps_handle_t handle;/* pps handlebars */
> + int handle_created; /* pps handle created flag */
> #endif /* HAVE_PPSAPI */
> };
>
> @@ -147,6 +149,11 @@
> register struct nmeaunit *up;
> struct refclockproc *pp;
> int fd;
> +#ifdef PPS_HAVE_FINDPATH
> + char id[LENPPS] = "",
> + path[LENPPS],
> + mylink[LENPPS] = "";/* just a default device */
> +#endif /* PPS_HAVE_FINDPATH */
> char device[20];
>
> /*
> @@ -201,7 +208,20 @@
> #else
> return (0);
> #endif
> -}
> +} else {
> +struct serial_struct ss;
> +if (ioctl(fd, TIOCGSERIAL, ) < 0 ||
> +(
> + ss.flags |= ASYNC_HARDPPS_CD,
> + ioctl(fd, TIOCSSERIAL, )) < 0) {
> + msyslog(LOG_NOTICE, "refclock_nmea: TIOCSSERIAL fd %d, %m",
> fd);
> + msyslog(LOG_NOTICE,
> + "refclock_nmea: optional PPS processing not
> available");
> +} else {
> +msyslog(LOG_INFO,
> +"refclock_nmea: PPS detection on");
> +}
> + }
You should use "setserial" here. Keep in mind that the PPS source
could be _not_ connected with the serial line at all.
> /*
>* Allocate and initialize unit structure
> @@ -238,12 +258,26 @@
>* Start the PPSAPI interface if it is there. Default to use
>* the assert edge and do not enable the kernel hardpps.
>*/
> +#ifdef PPS_HAVE_FINDPATH
> + /* Get the PPS source's real name */
> + //time_pps_readlink(mylink, LENPPS, path, LENPPS);
Remove unneeded code.
> + time_pps_readlink(device, LENPPS, path, LENPPS);
Test the return value (see the wiki at
http://wiki.enneenne.com/index.php/LinuxPPS_support#How_to_modify_a_refclock_to_work_with_LinuxPPS).
> + /* Try to find the source */
> + fd = time_pps_findpath(path, LENPPS, id, LENPPS);
> + if (fd < 0) {
> + msyslog(LOG_ERR, "refclock_nmea: cannot find PPS path \"%s\" in
> the system", path);
> + return (0);
> + }
> + msyslog(LOG_INFO, "refclock_nmea: found PPS source \"%s\" at id #%d on
> \"%s\"", path, fd, id);
> +#endif /* PPS_HAVE_FINDPATH */
> if (time_pps_create(fd, >handle) < 0) {
> - up->handle = 0;
> + up->handle_created = 0;
> msyslog(LOG_ERR,
> "refclock_nmea: time_pps_create failed: %m");
> return (1);
> }
> + up->handle_created = ~0;
> return(nmea_ppsapi(peer, 0, 0));
> #else
> return (1);
> @@ -265,8 +299,10 @@
> pp = peer->procptr;
> up = (struct nmeaunit *)pp->unitptr;
> #ifdef HAVE_PPSAPI
> - if (up->handle != 0)
> + if (up->handle_created) {
> time_pps_destroy(up->handle);
> + up->handle_created = 0;
> + }
> #endif /* HAVE_PPSAPI */
> io_closeclock(>io);
> free(up);
> @@ -374,7 +410,7 @@
> /*
>* Convert the timespec nanoseconds field to ntp l_fp units.
>*/
> - if (up->handle == 0)
> + if (!up->handle_created)
> return (0);
> timeout.tv_sec = 0;
> timeout.tv_nsec = 0;
Please, rewiev and test the code and I'll publish it.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail:[EMAIL PROTECTED]
Linux Device Driver [EMAIL PROTECTED]
Embedded Systems[EMAIL PROTECTED]
UNIX programming phone: +39 349 2432127
-
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
Re: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Tue, Mar 13, 2007 at 06:48:17PM -0400, Lennart Sorensen wrote:
I have tried out 3.0.0-rc2 which seems to work pretty well so far (when
Thanks. I just posted to the linux kernel ML the last release
3.0.0. Maybe you can do a git pull and try it out. :)
combined with the patches to the jsm driver I just posted). It took soe
work to get ntp's refclock_nmea to work though, since the patch that is
linked to from the linuxpps page seems out of date. Here is the patch
that seems to be working for me, although I am still testing it. Given
you know the linuxpps code better perhaps you can see if it looks sane
to you.
--- ntpd/refclock_nmea.c.ori 2007-03-13 18:38:01.0 -0400
+++ ntpd/refclock_nmea.c 2007-03-13 18:44:47.0 -0400
@@ -79,6 +79,7 @@
#define RANGEGATE50 /* range gate (ns) */
#define LENNMEA 75 /* min timecode length */
+#define LENPPS PPS_MAX_NAME_LEN
/*
* Tables to compute the ddd of year form icky dd/mm timecode. Viva la
@@ -99,6 +100,7 @@
pps_params_t pps_params; /* pps parameters */
pps_info_t pps_info;/* last pps data */
pps_handle_t handle;/* pps handlebars */
+ int handle_created; /* pps handle created flag */
#endif /* HAVE_PPSAPI */
};
@@ -147,6 +149,11 @@
register struct nmeaunit *up;
struct refclockproc *pp;
int fd;
+#ifdef PPS_HAVE_FINDPATH
+ char id[LENPPS] = ,
+ path[LENPPS],
+ mylink[LENPPS] = ;/* just a default device */
+#endif /* PPS_HAVE_FINDPATH */
char device[20];
/*
@@ -201,7 +208,20 @@
#else
return (0);
#endif
-}
+} else {
+struct serial_struct ss;
+if (ioctl(fd, TIOCGSERIAL, ss) 0 ||
+(
+ ss.flags |= ASYNC_HARDPPS_CD,
+ ioctl(fd, TIOCSSERIAL, ss)) 0) {
+ msyslog(LOG_NOTICE, refclock_nmea: TIOCSSERIAL fd %d, %m,
fd);
+ msyslog(LOG_NOTICE,
+ refclock_nmea: optional PPS processing not
available);
+} else {
+msyslog(LOG_INFO,
+refclock_nmea: PPS detection on);
+}
+ }
You should use setserial here. Keep in mind that the PPS source
could be _not_ connected with the serial line at all.
/*
* Allocate and initialize unit structure
@@ -238,12 +258,26 @@
* Start the PPSAPI interface if it is there. Default to use
* the assert edge and do not enable the kernel hardpps.
*/
+#ifdef PPS_HAVE_FINDPATH
+ /* Get the PPS source's real name */
+ //time_pps_readlink(mylink, LENPPS, path, LENPPS);
Remove unneeded code.
+ time_pps_readlink(device, LENPPS, path, LENPPS);
Test the return value (see the wiki at
http://wiki.enneenne.com/index.php/LinuxPPS_support#How_to_modify_a_refclock_to_work_with_LinuxPPS).
+ /* Try to find the source */
+ fd = time_pps_findpath(path, LENPPS, id, LENPPS);
+ if (fd 0) {
+ msyslog(LOG_ERR, refclock_nmea: cannot find PPS path \%s\ in
the system, path);
+ return (0);
+ }
+ msyslog(LOG_INFO, refclock_nmea: found PPS source \%s\ at id #%d on
\%s\, path, fd, id);
+#endif /* PPS_HAVE_FINDPATH */
if (time_pps_create(fd, up-handle) 0) {
- up-handle = 0;
+ up-handle_created = 0;
msyslog(LOG_ERR,
refclock_nmea: time_pps_create failed: %m);
return (1);
}
+ up-handle_created = ~0;
return(nmea_ppsapi(peer, 0, 0));
#else
return (1);
@@ -265,8 +299,10 @@
pp = peer-procptr;
up = (struct nmeaunit *)pp-unitptr;
#ifdef HAVE_PPSAPI
- if (up-handle != 0)
+ if (up-handle_created) {
time_pps_destroy(up-handle);
+ up-handle_created = 0;
+ }
#endif /* HAVE_PPSAPI */
io_closeclock(pp-io);
free(up);
@@ -374,7 +410,7 @@
/*
* Convert the timespec nanoseconds field to ntp l_fp units.
*/
- if (up-handle == 0)
+ if (!up-handle_created)
return (0);
timeout.tv_sec = 0;
timeout.tv_nsec = 0;
Please, rewiev and test the code and I'll publish it.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail:[EMAIL PROTECTED]
Linux Device Driver [EMAIL PROTECTED]
Embedded Systems[EMAIL PROTECTED]
UNIX programming phone: +39 349 2432127
-
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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Mar 14, 2007 at 10:31:46AM +0100, Rodolfo Giometti wrote:
On Tue, Mar 13, 2007 at 06:48:17PM -0400, Lennart Sorensen wrote:
I have tried out 3.0.0-rc2 which seems to work pretty well so far (when
Thanks. I just posted to the linux kernel ML the last release
3.0.0. Maybe you can do a git pull and try it out. :)
I will grab the last couple of commits and try although they didn't
sound like they really make much difference.
combined with the patches to the jsm driver I just posted). It took soe
work to get ntp's refclock_nmea to work though, since the patch that is
linked to from the linuxpps page seems out of date. Here is the patch
that seems to be working for me, although I am still testing it. Given
you know the linuxpps code better perhaps you can see if it looks sane
to you.
--- ntpd/refclock_nmea.c.ori2007-03-13 18:38:01.0 -0400
+++ ntpd/refclock_nmea.c2007-03-13 18:44:47.0 -0400
@@ -79,6 +79,7 @@
#define RANGEGATE 50 /* range gate (ns) */
#define LENNMEA75 /* min timecode length */
+#define LENPPS PPS_MAX_NAME_LEN
/*
* Tables to compute the ddd of year form icky dd/mm timecode. Viva la
@@ -99,6 +100,7 @@
pps_params_t pps_params; /* pps parameters */
pps_info_t pps_info;/* last pps data */
pps_handle_t handle;/* pps handlebars */
+ int handle_created; /* pps handle created flag */
#endif /* HAVE_PPSAPI */
};
@@ -147,6 +149,11 @@
register struct nmeaunit *up;
struct refclockproc *pp;
int fd;
+#ifdef PPS_HAVE_FINDPATH
+ char id[LENPPS] = ,
+path[LENPPS],
+mylink[LENPPS] = ;/* just a default device */
+#endif /* PPS_HAVE_FINDPATH */
char device[20];
/*
@@ -201,7 +208,20 @@
#else
return (0);
#endif
-}
+} else {
+struct serial_struct ss;
+if (ioctl(fd, TIOCGSERIAL, ss) 0 ||
+(
+ss.flags |= ASYNC_HARDPPS_CD,
+ ioctl(fd, TIOCSSERIAL, ss)) 0) {
+ msyslog(LOG_NOTICE, refclock_nmea: TIOCSSERIAL fd %d,
%m, fd);
+ msyslog(LOG_NOTICE,
+ refclock_nmea: optional PPS processing not
available);
+} else {
+msyslog(LOG_INFO,
+refclock_nmea: PPS detection on);
+}
+ }
You should use setserial here. Keep in mind that the PPS source
could be _not_ connected with the serial line at all.
I couldn't find any way to do that with setserial (at least not the
version I have), and I would rather not have to install setserial just
to do that. Which version of setserial is needed and what arguments
does it need to do it?
If it is NOT connected to the same device, then how would you specify
it? The ntp configuration is rather sparse when it comes to specifying
anything and seems to rely in symlinks to hardcoded device names for
finding everything. I suppose one could have gps# for the nmea messages
and pps# for the associated pps device name symlink (which may point
to something that doesn't even exist if there is an internal source of
that name with no associated device). Does that seem reasonable? I can
certainly change it to do that. Certainly refclock_atom already uses
/dev/pps# as it's device, so using that again may be reasonable.
/*
* Allocate and initialize unit structure
@@ -238,12 +258,26 @@
* Start the PPSAPI interface if it is there. Default to use
* the assert edge and do not enable the kernel hardpps.
*/
+#ifdef PPS_HAVE_FINDPATH
+ /* Get the PPS source's real name */
+ //time_pps_readlink(mylink, LENPPS, path, LENPPS);
Remove unneeded code.
Oops. Missed that one. I was changing it to the below line to reuse
the same serial device already specified for the NMEA messages.
I actually find the way it determines the pps device a bit annoying.
Right now I have to do this:
cd /dev
ln -s ttyn0 jsm0
ln -s jsm0 gps0
This way gps0 is the symlink the ntp refclock looks for when asked for
device 0, and readlink turns that into jsm0 (since the internal driver
name for ttyn0 is jsm, that is what the pps code insists it must be
named), which then is another symlink to the real device name. Same for
ttyS3 - serial3 - gps0. Now it would be nice if the internal driver
name matched the device name, but apparently that really never seems to
happen. Is all this symlink spagheti really necesary?
+ time_pps_readlink(device, LENPPS, path, LENPPS);
Test the return value (see the wiki at
http://wiki.enneenne.com/index.php/LinuxPPS_support#How_to_modify_a_refclock_to_work_with_LinuxPPS).
OK, I will do that too. Apparently the current refclock_nmea patch
fails that too then.
+ /* Try to find the source */
+ fd = time_pps_findpath(path, LENPPS, id,
Re: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Mar 14, 2007 at 09:19:34AM -0400, Lennart Sorensen wrote: I will grab the last couple of commits and try although they didn't sound like they really make much difference. Some fixes up... see the git log for further info. I couldn't find any way to do that with setserial (at least not the version I have), and I would rather not have to install setserial just to do that. Which version of setserial is needed and what arguments does it need to do it? Unluckely you need a patched version of setserial (see the patch on my site). On the same site you can find a precompiled version which I use for my tests, maybe it works for you... If it is NOT connected to the same device, then how would you specify it? The ntp configuration is rather sparse when it comes to specifying anything and seems to rely in symlinks to hardcoded device names for finding everything. I suppose one could have gps# for the nmea messages and pps# for the associated pps device name symlink (which may point to something that doesn't even exist if there is an internal source of that name with no associated device). Does that seem reasonable? I can certainly change it to do that. Certainly refclock_atom already uses /dev/pps# as it's device, so using that again may be reasonable. This is a specific problem of NTPD not of LinuxPPS itself. I wrote some letters about this problem into NTP list but with no results. The sysadm shoulkd use setserial to enable a serial port to become a PPS source and then NTPD should verify if such PPS source exists (using time_pps_findpath() Co.). I actually find the way it determines the pps device a bit annoying. Right now I have to do this: cd /dev ln -s ttyn0 jsm0 ln -s jsm0 gps0 This way gps0 is the symlink the ntp refclock looks for when asked for device 0, and readlink turns that into jsm0 (since the internal driver name for ttyn0 is jsm, that is what the pps code insists it must be named), which then is another symlink to the real device name. Same for ttyS3 - serial3 - gps0. Now it would be nice if the internal driver name matched the device name, but apparently that really never seems to happen. Is all this symlink spagheti really necesary? Did you read this example on the wiki? [EMAIL PROTECTED]:~/linuxpps$ cat /sys/class/pps/01/name serial1 [EMAIL PROTECTED]:~/linuxpps$ cat /sys/class/pps/01/path /dev/ttyS1 giometti at jeeg:~/linuxpps/test$ sudo ln -sf /dev/ttyS1 /dev/gps0 giometti at jeeg:~/linuxpps/test$ sudo ./ppstest /dev/gps0 found PPS source #2 serial1 on /dev/ttyS1 giometti at jeeg:~/linuxpps/test$ sudo ln -sf ktimer /dev/gps0 giometti at jeeg:~/linuxpps/test$ sudo ./ppstest /dev/gps0 found PPS source #0 ktimer on it doesn't work for you? Will do. Thanks a lot, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Mar 14, 2007 at 03:06:23PM +0100, Rodolfo Giometti wrote: Some fixes up... see the git log for further info. I looked at those, and they didn't sound important. I will grab it anyhow just for completeness. Unluckely you need a patched version of setserial (see the patch on my site). On the same site you can find a precompiled version which I use for my tests, maybe it works for you... OK, that explains why it wasn't in setserial's code. This is a specific problem of NTPD not of LinuxPPS itself. I wrote some letters about this problem into NTP list but with no results. The sysadm shoulkd use setserial to enable a serial port to become a PPS source and then NTPD should verify if such PPS source exists (using time_pps_findpath() Co.). Well I think I may just write a small tool to do the ioctl to enable it so I don't need the full setserial around. Did you read this example on the wiki? [EMAIL PROTECTED]:~/linuxpps$ cat /sys/class/pps/01/name serial1 [EMAIL PROTECTED]:~/linuxpps$ cat /sys/class/pps/01/path /dev/ttyS1 giometti at jeeg:~/linuxpps/test$ sudo ln -sf /dev/ttyS1 /dev/gps0 giometti at jeeg:~/linuxpps/test$ sudo ./ppstest /dev/gps0 found PPS source #2 serial1 on /dev/ttyS1 giometti at jeeg:~/linuxpps/test$ sudo ln -sf ktimer /dev/gps0 giometti at jeeg:~/linuxpps/test$ sudo ./ppstest /dev/gps0 found PPS source #0 ktimer on it doesn't work for you? I will have to try that again to be sure. I may have got myself confused when I first tried it. One symlink I can deal with (that's ntpd sillyness after all). -- Len Sorensen - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Mar 14, 2007 at 10:12:53AM -0400, Lennart Sorensen wrote: I looked at those, and they didn't sound important. I will grab it anyhow just for completeness. Ok! Thanks. Well I think I may just write a small tool to do the ioctl to enable it so I don't need the full setserial around. Yes, this could be a good solution while waiting for a new setserial. Ciao, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Mar 14, 2007 at 03:27:12PM +0100, Rodolfo Giometti wrote: Ok! Thanks. Yes, this could be a good solution while waiting for a new setserial. Now you said to check the return value of time_pps_readlink. Well it returns void so that isn't much good, and the stuff the wiki says to do, appears to be done internally by the function. So I guess that means I should undo that change so that I can actually get ntpd to compile again. If the call was to readlink directly it needs to be done, while your time_pps_readlink is just a handy wrapper that does all that for you, right? -- Len Sorensen - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Mar 14, 2007 at 10:42:51AM -0400, Lennart Sorensen wrote: Now you said to check the return value of time_pps_readlink. Well it I refere to readlink(), not to time_pps_readlink(). I'm sorry for mistake. If the call was to readlink directly it needs to be done, while your time_pps_readlink is just a handy wrapper that does all that for you, right? Right. Ciao, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
Well here is my current version of the refclock_nmea.c.patch for
LinuxPPS. It now uses /dev/gps# for the nmea messages and /dev/pps# for
the PPS device (which in my case is of course the same device). I am
running some more tests on it, but I think it is OK.
--- refclock_nmea.c.ori 2007-03-14 11:24:14.0 -0400
+++ refclock_nmea.c 2007-03-14 11:31:04.0 -0400
@@ -69,6 +69,7 @@
# define DEVICE COM%d: /* COM 1 - 3 supported */
#else
# define DEVICE/dev/gps%d/* name of radio device */
+# define PPSDEVICE /dev/pps%d/* name of pps device */
#endif
#defineSPEED232B4800 /* uart speed (4800 bps) */
#definePRECISION (-9)/* precision assumed (about 2 ms) */
@@ -79,6 +80,7 @@
#define RANGEGATE 50 /* range gate (ns) */
#define LENNMEA75 /* min timecode length */
+#define LENPPS PPS_MAX_NAME_LEN
/*
* Tables to compute the ddd of year form icky dd/mm timecode. Viva la
@@ -99,6 +101,7 @@
pps_params_t pps_params; /* pps parameters */
pps_info_t pps_info;/* last pps data */
pps_handle_t handle;/* pps handlebars */
+ int handle_created; /* pps handle created flag */
#endif /* HAVE_PPSAPI */
};
@@ -147,6 +150,11 @@
register struct nmeaunit *up;
struct refclockproc *pp;
int fd;
+#ifdef PPS_HAVE_FINDPATH
+ char id[LENPPS] = ,
+path[LENPPS],
+ppsdevice[LENPPS] = ;/* just a default device */
+#endif /* PPS_HAVE_FINDPATH */
char device[20];
/*
@@ -238,12 +246,26 @@
* Start the PPSAPI interface if it is there. Default to use
* the assert edge and do not enable the kernel hardpps.
*/
+#ifdef PPS_HAVE_FINDPATH
+ /* Get the PPS source's real name */
+ (void)sprintf(ppsdevice, PPSDEVICE, unit);
+ time_pps_readlink(ppsdevice, LENPPS, path, LENPPS);
+
+ /* Try to find the source */
+ fd = time_pps_findpath(path, LENPPS, id, LENPPS);
+ if (fd 0) {
+ msyslog(LOG_ERR, refclock_nmea: cannot find PPS path \%s\ in
the system, path);
+ return (0);
+ }
+ msyslog(LOG_INFO, refclock_nmea: found PPS source \%s\ at id #%d on
\%s\, path, fd, id);
+#endif /* PPS_HAVE_FINDPATH */
if (time_pps_create(fd, up-handle) 0) {
- up-handle = 0;
+ up-handle_created = 0;
msyslog(LOG_ERR,
refclock_nmea: time_pps_create failed: %m);
return (1);
}
+ up-handle_created = ~0;
return(nmea_ppsapi(peer, 0, 0));
#else
return (1);
@@ -265,8 +287,10 @@
pp = peer-procptr;
up = (struct nmeaunit *)pp-unitptr;
#ifdef HAVE_PPSAPI
- if (up-handle != 0)
+ if (up-handle_created) {
time_pps_destroy(up-handle);
+ up-handle_created = 0;
+ }
#endif /* HAVE_PPSAPI */
io_closeclock(pp-io);
free(up);
@@ -374,7 +398,7 @@
/*
* Convert the timespec nanoseconds field to ntp l_fp units.
*/
- if (up-handle == 0)
+ if (!up-handle_created)
return (0);
timeout.tv_sec = 0;
timeout.tv_nsec = 0;
--
Len Sorensen
-
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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Mar 14, 2007 at 11:37:05AM -0400, Lennart Sorensen wrote: Well here is my current version of the refclock_nmea.c.patch for LinuxPPS. It now uses /dev/gps# for the nmea messages and /dev/pps# for the PPS device (which in my case is of course the same device). I am running some more tests on it, but I think it is OK. Thanks, published. :) Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Mar 14, 2007 at 04:47:32PM +0100, Rodolfo Giometti wrote:
On Wed, Mar 14, 2007 at 11:37:05AM -0400, Lennart Sorensen wrote:
Well here is my current version of the refclock_nmea.c.patch for
LinuxPPS. It now uses /dev/gps# for the nmea messages and /dev/pps# for
the PPS device (which in my case is of course the same device). I am
running some more tests on it, but I think it is OK.
Thanks, published. :)
Well it does work for our GPS receiver at least. Of course I have to
change the baud rate in the driver since our unit doens't use the NNEA
standard 4800. And the configure script for ntp doesn't recognize the
v2 PPSAPI, so I have to manually explain to it that I have the PPS API
and it should actually include it.
Here is my utility for enabling PPS on my serial port now. Seems less
of a pain that patching setserial and including that (with 256MB flash
space, setserial seems wasteful).
ppsctl.c
#include sys/ioctl.h
#include sys/types.h
#include sys/stat.h
#include fcntl.h
#include stdio.h
#include string.h
#include linux/serial.h
void usage(char *name) {
fprintf(stderr,Usage: %s /dev/port [0|1]\n,name);
fprintf(stderr,Where 0 means disable PPS and 1 means enable PPS\n);
}
int main(int argc, char *argv[]) {
int fd;
char *state;
if(argc3) {
usage(argv[0]);
return 1;
}
fd = open(argv[1],O_RDWR);
if (fd=0) {
struct serial_struct ss;
if (ioctl(fd, TIOCGSERIAL, ss) 0 ) {
perror(TIOCGSERIAL);
return 1;
} else {
if(strcmp(argv[2],1)==0) {
ss.flags |= ASYNC_HARDPPS_CD;
state=enabled;
} else if(strcmp(argv[2],0)==0) {
ss.flags = ~ASYNC_HARDPPS_CD;
state=disabled;
} else {
fprintf(stderr,Invalid state argument
\%s\\n,argv[2]);
return 1;
}
if (ioctl(fd, TIOCSSERIAL, ss) 0) {
perror(TIOCSSERIAL);
} else {
fprintf(stderr,PPS on %s is now
%s\n,argv[1],state);
}
}
} else {
fprintf(stderr,Can't open \%s\:,argv[1]);
perror();
}
return 0;
}
/* vim:ts=4:shiftwidth=4:
* */
--
Len Sorensen
-
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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Tue, Mar 13, 2007 at 10:38:43PM +0100, Rodolfo Giometti wrote:
> here my new patch for PPS support in Linux.
>
> I tried to follow your suggestions as much possible! Please let me
> know if this new version could be more acceptable.
I have tried out 3.0.0-rc2 which seems to work pretty well so far (when
combined with the patches to the jsm driver I just posted). It took soe
work to get ntp's refclock_nmea to work though, since the patch that is
linked to from the linuxpps page seems out of date. Here is the patch
that seems to be working for me, although I am still testing it. Given
you know the linuxpps code better perhaps you can see if it looks sane
to you.
--- ntpd/refclock_nmea.c.ori2007-03-13 18:38:01.0 -0400
+++ ntpd/refclock_nmea.c2007-03-13 18:44:47.0 -0400
@@ -79,6 +79,7 @@
#define RANGEGATE 50 /* range gate (ns) */
#define LENNMEA75 /* min timecode length */
+#define LENPPS PPS_MAX_NAME_LEN
/*
* Tables to compute the ddd of year form icky dd/mm timecode. Viva la
@@ -99,6 +100,7 @@
pps_params_t pps_params; /* pps parameters */
pps_info_t pps_info;/* last pps data */
pps_handle_t handle;/* pps handlebars */
+ int handle_created; /* pps handle created flag */
#endif /* HAVE_PPSAPI */
};
@@ -147,6 +149,11 @@
register struct nmeaunit *up;
struct refclockproc *pp;
int fd;
+#ifdef PPS_HAVE_FINDPATH
+ char id[LENPPS] = "",
+path[LENPPS],
+mylink[LENPPS] = "";/* just a default device */
+#endif /* PPS_HAVE_FINDPATH */
char device[20];
/*
@@ -201,7 +208,20 @@
#else
return (0);
#endif
-}
+} else {
+struct serial_struct ss;
+if (ioctl(fd, TIOCGSERIAL, ) < 0 ||
+(
+ss.flags |= ASYNC_HARDPPS_CD,
+ ioctl(fd, TIOCSSERIAL, )) < 0) {
+ msyslog(LOG_NOTICE, "refclock_nmea: TIOCSSERIAL fd %d, %m",
fd);
+ msyslog(LOG_NOTICE,
+ "refclock_nmea: optional PPS processing not
available");
+} else {
+msyslog(LOG_INFO,
+"refclock_nmea: PPS detection on");
+}
+ }
/*
* Allocate and initialize unit structure
@@ -238,12 +258,26 @@
* Start the PPSAPI interface if it is there. Default to use
* the assert edge and do not enable the kernel hardpps.
*/
+#ifdef PPS_HAVE_FINDPATH
+ /* Get the PPS source's real name */
+ //time_pps_readlink(mylink, LENPPS, path, LENPPS);
+ time_pps_readlink(device, LENPPS, path, LENPPS);
+
+ /* Try to find the source */
+ fd = time_pps_findpath(path, LENPPS, id, LENPPS);
+ if (fd < 0) {
+ msyslog(LOG_ERR, "refclock_nmea: cannot find PPS path \"%s\" in
the system", path);
+ return (0);
+ }
+ msyslog(LOG_INFO, "refclock_nmea: found PPS source \"%s\" at id #%d on
\"%s\"", path, fd, id);
+#endif /* PPS_HAVE_FINDPATH */
if (time_pps_create(fd, >handle) < 0) {
- up->handle = 0;
+ up->handle_created = 0;
msyslog(LOG_ERR,
"refclock_nmea: time_pps_create failed: %m");
return (1);
}
+ up->handle_created = ~0;
return(nmea_ppsapi(peer, 0, 0));
#else
return (1);
@@ -265,8 +299,10 @@
pp = peer->procptr;
up = (struct nmeaunit *)pp->unitptr;
#ifdef HAVE_PPSAPI
- if (up->handle != 0)
+ if (up->handle_created) {
time_pps_destroy(up->handle);
+ up->handle_created = 0;
+ }
#endif /* HAVE_PPSAPI */
io_closeclock(>io);
free(up);
@@ -374,7 +410,7 @@
/*
* Convert the timespec nanoseconds field to ntp l_fp units.
*/
- if (up->handle == 0)
+ if (!up->handle_created)
return (0);
timeout.tv_sec = 0;
timeout.tv_nsec = 0;
--
Len Sorensen
-
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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Tue, Mar 13, 2007 at 10:38:43PM +0100, Rodolfo Giometti wrote:
here my new patch for PPS support in Linux.
I tried to follow your suggestions as much possible! Please let me
know if this new version could be more acceptable.
I have tried out 3.0.0-rc2 which seems to work pretty well so far (when
combined with the patches to the jsm driver I just posted). It took soe
work to get ntp's refclock_nmea to work though, since the patch that is
linked to from the linuxpps page seems out of date. Here is the patch
that seems to be working for me, although I am still testing it. Given
you know the linuxpps code better perhaps you can see if it looks sane
to you.
--- ntpd/refclock_nmea.c.ori2007-03-13 18:38:01.0 -0400
+++ ntpd/refclock_nmea.c2007-03-13 18:44:47.0 -0400
@@ -79,6 +79,7 @@
#define RANGEGATE 50 /* range gate (ns) */
#define LENNMEA75 /* min timecode length */
+#define LENPPS PPS_MAX_NAME_LEN
/*
* Tables to compute the ddd of year form icky dd/mm timecode. Viva la
@@ -99,6 +100,7 @@
pps_params_t pps_params; /* pps parameters */
pps_info_t pps_info;/* last pps data */
pps_handle_t handle;/* pps handlebars */
+ int handle_created; /* pps handle created flag */
#endif /* HAVE_PPSAPI */
};
@@ -147,6 +149,11 @@
register struct nmeaunit *up;
struct refclockproc *pp;
int fd;
+#ifdef PPS_HAVE_FINDPATH
+ char id[LENPPS] = ,
+path[LENPPS],
+mylink[LENPPS] = ;/* just a default device */
+#endif /* PPS_HAVE_FINDPATH */
char device[20];
/*
@@ -201,7 +208,20 @@
#else
return (0);
#endif
-}
+} else {
+struct serial_struct ss;
+if (ioctl(fd, TIOCGSERIAL, ss) 0 ||
+(
+ss.flags |= ASYNC_HARDPPS_CD,
+ ioctl(fd, TIOCSSERIAL, ss)) 0) {
+ msyslog(LOG_NOTICE, refclock_nmea: TIOCSSERIAL fd %d, %m,
fd);
+ msyslog(LOG_NOTICE,
+ refclock_nmea: optional PPS processing not
available);
+} else {
+msyslog(LOG_INFO,
+refclock_nmea: PPS detection on);
+}
+ }
/*
* Allocate and initialize unit structure
@@ -238,12 +258,26 @@
* Start the PPSAPI interface if it is there. Default to use
* the assert edge and do not enable the kernel hardpps.
*/
+#ifdef PPS_HAVE_FINDPATH
+ /* Get the PPS source's real name */
+ //time_pps_readlink(mylink, LENPPS, path, LENPPS);
+ time_pps_readlink(device, LENPPS, path, LENPPS);
+
+ /* Try to find the source */
+ fd = time_pps_findpath(path, LENPPS, id, LENPPS);
+ if (fd 0) {
+ msyslog(LOG_ERR, refclock_nmea: cannot find PPS path \%s\ in
the system, path);
+ return (0);
+ }
+ msyslog(LOG_INFO, refclock_nmea: found PPS source \%s\ at id #%d on
\%s\, path, fd, id);
+#endif /* PPS_HAVE_FINDPATH */
if (time_pps_create(fd, up-handle) 0) {
- up-handle = 0;
+ up-handle_created = 0;
msyslog(LOG_ERR,
refclock_nmea: time_pps_create failed: %m);
return (1);
}
+ up-handle_created = ~0;
return(nmea_ppsapi(peer, 0, 0));
#else
return (1);
@@ -265,8 +299,10 @@
pp = peer-procptr;
up = (struct nmeaunit *)pp-unitptr;
#ifdef HAVE_PPSAPI
- if (up-handle != 0)
+ if (up-handle_created) {
time_pps_destroy(up-handle);
+ up-handle_created = 0;
+ }
#endif /* HAVE_PPSAPI */
io_closeclock(pp-io);
free(up);
@@ -374,7 +410,7 @@
/*
* Convert the timespec nanoseconds field to ntp l_fp units.
*/
- if (up-handle == 0)
+ if (!up-handle_created)
return (0);
timeout.tv_sec = 0;
timeout.tv_nsec = 0;
--
Len Sorensen
-
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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Feb 21, 2007 at 10:16:45AM +, Pavel Machek wrote: > Hi! > > > @@ -0,0 +1,206 @@ > > + > > + PPS - Pulse Per Second > > + -- > > + > > +(C) Copyright 2007 Rodolfo Giometti <[EMAIL PROTECTED]> > > If you add copyright, add license, too. Ok. > No new /proc files, please. Already removed. As soon as possible I'll repost a new patch. Thanks! Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
Hi! > @@ -0,0 +1,206 @@ > + > + PPS - Pulse Per Second > + -- > + > +(C) Copyright 2007 Rodolfo Giometti <[EMAIL PROTECTED]> If you add copyright, add license, too. > +PROCFS support > +-- > + > +If PROCFS support is enabled a new directory "/proc/pps" is created: > + > +$ ls /proc/pps/ > +00 01 sources > + > +The file "sources" holds a brief description of all PPS sources > +defined in the system: > + > +$ cat /proc/pps/sources > +idmode echonamepath > + -- > +001133 no serial0 /dev/ttyS0 > +011133 no serial1 /dev/ttyS1 > + > +The other entries are directories that hold one or two files according > +to the ability of the associated source to provide "assert" and > +"clear" timestamps: > + > +$ ls /proc/pps/00 > +assert clear > + > +Inside each "assert" and "clear" file you can find the timestamp and a > +sequence number: > + > +$ cat /proc/pps/00/assert > +1170026870.983207967 #8 No new /proc files, please. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Thu, Feb 22, 2007 at 12:51:48AM +0100, Roman Zippel wrote: > Hi, > > On Wednesday 21 February 2007 13:04, Rodolfo Giometti wrote: > > > RFC simply doesn't consider the fact that you can have a PPS source > > __without__ a filedes connected with, and a single filedes is > > considered __always__ connected with a single PPS source. > > That's not entirely true. It doesn't say that pps_handle_t must be a file > descriptor and it leaves the option for the argument to time_pps_create() not > to be a file descriptor as well. Yes. In fact that's my solution! The problem is that "pps_handle_t" is forced by the RFC to be a scalar and _not_ a generic (and opaque) type. I suppose that since right now such handler was simply the filedes of the serial/parallel port connected with the GPS antenna. But this is not always the case. As already told the GPS antenna can be connected with the serial line while the PPS signal is not, so NTPD should always open the serial port to read GPS data but it must not use such filedes for the time_pps_create(). My support try to resolve this problem with minor changes in both RFC and NTPD code. Ciao, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Feb 21, 2007 at 08:14:14AM -0800, H. Peter Anvin wrote: > > If you have a kernel driver at all, then it makes perfect sense. If you > don't have a kernel driver at all, then it's irrelevant to the > linux-kernel discussion. ??? So you are told me that if my PPS source is connected with a single CPU GPIO I have to add a new driver to the kernel? I have to choose a proper major/minor numbers for just one PIN? =:-o > >RFC simply doesn't consider the fact that you can have a PPS source > >__without__ a filedes connected with, and a single filedes is > >considered __always__ connected with a single PPS source. > > That's the Unix way. But not PPS one. In several embedded systems the GPS antenna is connected with the serial port and the PPS source, which cames from the __same__ GPS antenna, is connected with a GPIO. How I should manage such case? Which fildes I should use? Also consider that NTPD currently manage only one filedes which should support both GPS antenna's data and PPS source. My patch can solve this problem. Ciao, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Feb 21, 2007 at 08:14:14AM -0800, H. Peter Anvin wrote: If you have a kernel driver at all, then it makes perfect sense. If you don't have a kernel driver at all, then it's irrelevant to the linux-kernel discussion. ??? So you are told me that if my PPS source is connected with a single CPU GPIO I have to add a new driver to the kernel? I have to choose a proper major/minor numbers for just one PIN? =:-o RFC simply doesn't consider the fact that you can have a PPS source __without__ a filedes connected with, and a single filedes is considered __always__ connected with a single PPS source. That's the Unix way. But not PPS one. In several embedded systems the GPS antenna is connected with the serial port and the PPS source, which cames from the __same__ GPS antenna, is connected with a GPIO. How I should manage such case? Which fildes I should use? Also consider that NTPD currently manage only one filedes which should support both GPS antenna's data and PPS source. My patch can solve this problem. Ciao, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Thu, Feb 22, 2007 at 12:51:48AM +0100, Roman Zippel wrote: Hi, On Wednesday 21 February 2007 13:04, Rodolfo Giometti wrote: RFC simply doesn't consider the fact that you can have a PPS source __without__ a filedes connected with, and a single filedes is considered __always__ connected with a single PPS source. That's not entirely true. It doesn't say that pps_handle_t must be a file descriptor and it leaves the option for the argument to time_pps_create() not to be a file descriptor as well. Yes. In fact that's my solution! The problem is that pps_handle_t is forced by the RFC to be a scalar and _not_ a generic (and opaque) type. I suppose that since right now such handler was simply the filedes of the serial/parallel port connected with the GPS antenna. But this is not always the case. As already told the GPS antenna can be connected with the serial line while the PPS signal is not, so NTPD should always open the serial port to read GPS data but it must not use such filedes for the time_pps_create(). My support try to resolve this problem with minor changes in both RFC and NTPD code. Ciao, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
Hi! @@ -0,0 +1,206 @@ + + PPS - Pulse Per Second + -- + +(C) Copyright 2007 Rodolfo Giometti [EMAIL PROTECTED] If you add copyright, add license, too. +PROCFS support +-- + +If PROCFS support is enabled a new directory /proc/pps is created: + +$ ls /proc/pps/ +00 01 sources + +The file sources holds a brief description of all PPS sources +defined in the system: + +$ cat /proc/pps/sources +idmode echonamepath + -- +001133 no serial0 /dev/ttyS0 +011133 no serial1 /dev/ttyS1 + +The other entries are directories that hold one or two files according +to the ability of the associated source to provide assert and +clear timestamps: + +$ ls /proc/pps/00 +assert clear + +Inside each assert and clear file you can find the timestamp and a +sequence number: + +$ cat /proc/pps/00/assert +1170026870.983207967 #8 No new /proc files, please. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Wed, Feb 21, 2007 at 10:16:45AM +, Pavel Machek wrote: Hi! @@ -0,0 +1,206 @@ + + PPS - Pulse Per Second + -- + +(C) Copyright 2007 Rodolfo Giometti [EMAIL PROTECTED] If you add copyright, add license, too. Ok. No new /proc files, please. Already removed. As soon as possible I'll repost a new patch. Thanks! Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
Hi, On Wednesday 21 February 2007 13:04, Rodolfo Giometti wrote: > RFC simply doesn't consider the fact that you can have a PPS source > __without__ a filedes connected with, and a single filedes is > considered __always__ connected with a single PPS source. That's not entirely true. It doesn't say that pps_handle_t must be a file descriptor and it leaves the option for the argument to time_pps_create() not to be a file descriptor as well. bye, Roman - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
Rodolfo Giometti wrote: The problem is that sometimes you cannot have a filedescriptor at all. Think about a PPS source connected with a CPU's GPIO pin. You have no filedes to use and defining one just for a PPS source or for a class of PPS sources, I think, is a non sense. If you have a kernel driver at all, then it makes perfect sense. If you don't have a kernel driver at all, then it's irrelevant to the linux-kernel discussion. RFC simply doesn't consider the fact that you can have a PPS source __without__ a filedes connected with, and a single filedes is considered __always__ connected with a single PPS source. That's the Unix way. -hpa - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Mon, Feb 19, 2007 at 06:56:20PM -0800, H. Peter Anvin wrote: > It's not a precondition for a file descriptor, either. There are plenty > of ioctl-only device drivers in existence. > > Furthermore, a file descriptor doesn't imply a device entry. Consider > pipe(2), for example. > > As far as the kernel is concerned, a file handle is a nice, uniform > system for providing communication between the kernel and user space. > It doesn't matter if one can read() or write() on it; it's perfectly > normal to support only a subset of the normal operations. The problem is that sometimes you cannot have a filedescriptor at all. Think about a PPS source connected with a CPU's GPIO pin. You have no filedes to use and defining one just for a PPS source or for a class of PPS sources, I think, is a non sense. RFC simply doesn't consider the fact that you can have a PPS source __without__ a filedes connected with, and a single filedes is considered __always__ connected with a single PPS source. Ciao, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Mon, Feb 19, 2007 at 06:56:20PM -0800, H. Peter Anvin wrote: It's not a precondition for a file descriptor, either. There are plenty of ioctl-only device drivers in existence. Furthermore, a file descriptor doesn't imply a device entry. Consider pipe(2), for example. As far as the kernel is concerned, a file handle is a nice, uniform system for providing communication between the kernel and user space. It doesn't matter if one can read() or write() on it; it's perfectly normal to support only a subset of the normal operations. The problem is that sometimes you cannot have a filedescriptor at all. Think about a PPS source connected with a CPU's GPIO pin. You have no filedes to use and defining one just for a PPS source or for a class of PPS sources, I think, is a non sense. RFC simply doesn't consider the fact that you can have a PPS source __without__ a filedes connected with, and a single filedes is considered __always__ connected with a single PPS source. Ciao, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
Rodolfo Giometti wrote: The problem is that sometimes you cannot have a filedescriptor at all. Think about a PPS source connected with a CPU's GPIO pin. You have no filedes to use and defining one just for a PPS source or for a class of PPS sources, I think, is a non sense. If you have a kernel driver at all, then it makes perfect sense. If you don't have a kernel driver at all, then it's irrelevant to the linux-kernel discussion. RFC simply doesn't consider the fact that you can have a PPS source __without__ a filedes connected with, and a single filedes is considered __always__ connected with a single PPS source. That's the Unix way. -hpa - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
Hi, On Wednesday 21 February 2007 13:04, Rodolfo Giometti wrote: RFC simply doesn't consider the fact that you can have a PPS source __without__ a filedes connected with, and a single filedes is considered __always__ connected with a single PPS source. That's not entirely true. It doesn't say that pps_handle_t must be a file descriptor and it leaves the option for the argument to time_pps_create() not to be a file descriptor as well. bye, Roman - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
Rodolfo Giometti wrote: Please read the following consideratios before sending to /dev/null! :) RFC considerations -- While implementing a PPS API as RFC 2783 defines and using an embedded CPU GPIO-Pin as physical link to the signal, I encountered a deeper problem: At startup it needs a file descriptor as argument for the function time_pps_create(). This implies that the source has a /dev/... entry. This assumption is ok for the serial and parallel port, where you can do something usefull beside(!) the gathering of timestamps as it is the central task for a PPS-API. But this assumption does not work for a single purpose GPIO line. In this case even basic file-related functionality (like read() and write()) makes no sense at all and should not be a precondition for the use of a PPS-API. It's not a precondition for a file descriptor, either. There are plenty of ioctl-only device drivers in existence. Furthermore, a file descriptor doesn't imply a device entry. Consider pipe(2), for example. As far as the kernel is concerned, a file handle is a nice, uniform system for providing communication between the kernel and user space. It doesn't matter if one can read() or write() on it; it's perfectly normal to support only a subset of the normal operations. -hpa - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
Rodolfo Giometti wrote: Please read the following consideratios before sending to /dev/null! :) RFC considerations -- While implementing a PPS API as RFC 2783 defines and using an embedded CPU GPIO-Pin as physical link to the signal, I encountered a deeper problem: At startup it needs a file descriptor as argument for the function time_pps_create(). This implies that the source has a /dev/... entry. This assumption is ok for the serial and parallel port, where you can do something usefull beside(!) the gathering of timestamps as it is the central task for a PPS-API. But this assumption does not work for a single purpose GPIO line. In this case even basic file-related functionality (like read() and write()) makes no sense at all and should not be a precondition for the use of a PPS-API. It's not a precondition for a file descriptor, either. There are plenty of ioctl-only device drivers in existence. Furthermore, a file descriptor doesn't imply a device entry. Consider pipe(2), for example. As far as the kernel is concerned, a file handle is a nice, uniform system for providing communication between the kernel and user space. It doesn't matter if one can read() or write() on it; it's perfectly normal to support only a subset of the normal operations. -hpa - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
Rodolfo Giometti wrote: >>> +PROCFS support >>> +-- >> New features shouldn't introduce new /proc stuff. > > It's a must? I can leave procfs for backward compatibility with old > utilities? Hmm, as this is a new feature with regard to the mainline kernel, old utilities don't count (if you can install a new kernel you can also be expected to install new user-space tools for the new feature). >> You read the comment above your line? > > No, sorry. I'm going to choose another id number... or can I keep 17? I don't know, ask whoever is responsible for the file. >>> +#define to_class_dev(obj) container_of((obj), struct class_device, kobj) >> pretty generic name. > > I should change it? If it's of general use put it in the appropriate header file. If it's just for the pps subsystem name it as such. >> Have you thought about 32/64bit issues? > > No. I have no 64 bits machine to test the code... Hmm, think about x86_64 with 64-bit kernel and 32-bit userspace, probably having got different padding in the struct. Read LDD3, chapter 11, especially 11.4 . Jan - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Fri, Feb 16, 2007 at 08:51:35PM +, Russell King wrote: > > You can't because it doesn't go through the interfaces you're hooking > into. Existing interfaces are "changed" to point at the UARTs using > setserial, which does its work via an ioctl. I see. > Not specifically only userland - if it happens to be set when the port > is registered then enable PPS support then as well. > > So: > > 1. uart_configure_port - if UPF_HARDPPS_CD is set, register the port >for PPS support. > 2. uart_remove_one_port - if UPF_HARDPPS_CD is set, unregister the port >for PPS support. > 3. uart_set_info - if changing UPF_HARDPPS_CD, appropriately register or >unregister the port for PPS support. Ok. I'm going to study how to modify the code. > PS, [EMAIL PROTECTED] dropped from the cc: since it rejects my > postings. I just fixed my spam filter (at least I hope so :). Thanks, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Fri, Feb 16, 2007 at 08:56:18PM +0100, Jan Dittmer wrote: > Drop the linux prefix. It's in the linux kernel after all. Ok. > > +PROCFS support > > +-- > > New features shouldn't introduce new /proc stuff. It's a must? I can leave procfs for backward compatibility with old utilities? > Add to MAINTAINERS Ok. > Your way to hook into lp and 8250 is pretty gross. It should at least be > possible to deactivate it via the kernel command line, but it would be > a lot nicer to have pps_lp and pps_8250 modules which you can load. Also I think it's not possible... however the Russell's suggestions should go in that direction. > what happens if you've multiple lp ports? How do you control which to > grab? No way... I can add a specific flag as for uart lines or a kernel module parameter. > - don't implement your own dbg() stuff, use dprintk and friends > - drop the inlines, gcc will do the right thing. Ok. Ok. > Perhaps just implement empty defines for the none pps cases and get > rid of the ifdefs? But this should really be controllabe via > sysfs or such. Mmm... let me think about howto implement that... > help text Ok. > help text and difference to CLIENT_LP? Ok. > Why no dynamically allocated array? It's easier! :P Also it's very difficult having more that 3 or 4 PPS sources in a system. > I wouldn't bet on that. Why not? =:-o Also locking instructions may add extra code and delay the timestamp recording... > Doesn't match filename I'm going to fix it. > > +++ b/drivers/pps/procfs.c > > I'd drop that completely. :'( > You read the comment above your line? No, sorry. I'm going to choose another id number... or can I keep 17? > These should use dprintk and friends Ok. > Isn't something like 4 more reasonable (lp + 8250 + ktimer?) It should be enought... > I think you can drop the volatiles, there was a discussion some time ago > that they mostly waste of words. I see... > This one looks pretty fishy. After the check you normally want > to use it, don't you? And then you already lost the guarantee. You are right... > > +#define to_class_dev(obj) container_of((obj), struct class_device, kobj) > > pretty generic name. I should change it? > Have you thought about 32/64bit issues? No. I have no 64 bits machine to test the code... > Function in .h? I'm going to check it. Thanks for your suggestions, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Fri, Feb 16, 2007 at 09:43:36PM +0100, Rodolfo Giometti wrote: > On Fri, Feb 16, 2007 at 07:12:08PM +, Russell King wrote: > > > > Yuck. Please. No. Doing it this way means you have to modify every > > single serial driver out there which is a mamouth task. > > > > > uart_handle_dcd_change(>port, status & > > > UART_MSR_DCD); > > > > Did you not look to see what's in this helper? You'll find within here > > the following code: > > > > #ifdef CONFIG_HARD_PPS > > if ((port->flags & UPF_HARDPPS_CD) && status) > > hardpps(); > > #endif > > > > which should've been a big sign lit up in bright lights in Times Square > > pointing you towards the right place to put your code. > > Ok. > > > Why not continue to leave it as a decision of the administrator - if > > you want ports to default to having PPS support enabled, change all > > the registration to set UPF_HARDPPS_CD. But leave the admin with > > the ability to disable it. > > Ok. > > > This means that PPS support is not available for any port which wasn't > > autoprobed at device discovery time. That seems quite restrictive. > > How I can force probing for a specified uart port? You can't because it doesn't go through the interfaces you're hooking into. Existing interfaces are "changed" to point at the UARTs using setserial, which does its work via an ioctl. > > Maybe it needs to be coupled with the setting/clearing of UPF_HARDPPS_CD ? > > What do you think about? I should enable the PPS support only if the > userland sets the UPF_HARDPPS_CD flag? Not specifically only userland - if it happens to be set when the port is registered then enable PPS support then as well. So: 1. uart_configure_port - if UPF_HARDPPS_CD is set, register the port for PPS support. 2. uart_remove_one_port - if UPF_HARDPPS_CD is set, unregister the port for PPS support. 3. uart_set_info - if changing UPF_HARDPPS_CD, appropriately register or unregister the port for PPS support. PS, [EMAIL PROTECTED] dropped from the cc: since it rejects my postings. -- Russell King Linux kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Fri, Feb 16, 2007 at 07:12:08PM +, Russell King wrote: > > Yuck. Please. No. Doing it this way means you have to modify every > single serial driver out there which is a mamouth task. > > > uart_handle_dcd_change(>port, status & > > UART_MSR_DCD); > > Did you not look to see what's in this helper? You'll find within here > the following code: > > #ifdef CONFIG_HARD_PPS > if ((port->flags & UPF_HARDPPS_CD) && status) > hardpps(); > #endif > > which should've been a big sign lit up in bright lights in Times Square > pointing you towards the right place to put your code. Ok. > Why not continue to leave it as a decision of the administrator - if > you want ports to default to having PPS support enabled, change all > the registration to set UPF_HARDPPS_CD. But leave the admin with > the ability to disable it. Ok. > This means that PPS support is not available for any port which wasn't > autoprobed at device discovery time. That seems quite restrictive. How I can force probing for a specified uart port? > Maybe it needs to be coupled with the setting/clearing of UPF_HARDPPS_CD ? What do you think about? I should enable the PPS support only if the userland sets the UPF_HARDPPS_CD flag? Thanks for your comments, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
Some non political comments
Rodolfo Giometti wrote:
> +Coding example
> +--
> +
> +To register a PPS source into the kernel you should define a struct
> +linuxpps_source_info_s as follow:
> +
> +static struct linuxpps_source_info_s linuxpps_ktimer_info = {
Drop the linux prefix. It's in the linux kernel after all.
> +PROCFS support
> +--
New features shouldn't introduce new /proc stuff.
> +Resources
> +-
> +
> +Wiki: http://wiki.enneenne.com/index.php/LinuxPPS_support and the LinuxPPS
> +ML: http://ml.enneenne.com/cgi-bin/mailman/listinfo/linuxpps.
Add to MAINTAINERS
Your way to hook into lp and 8250 is pretty gross. It should at least be
possible to deactivate it via the kernel command line, but it would be
a lot nicer to have pps_lp and pps_8250 modules which you can load. Also
what happens if you've multiple lp ports? How do you control which to
grab?
- don't implement your own dbg() stuff, use dprintk and friends
- drop the inlines, gcc will do the right thing.
> --- a/drivers/char/lp.c
> +++ b/drivers/char/lp.c
> static struct parport_driver lp_driver = {
> diff --git a/drivers/parport/parport_pc.c b/drivers/parport/parport_pc.c
> index b61c17b..426b0ac 100644
> --- a/drivers/parport/parport_pc.c
> +++ b/drivers/parport/parport_pc.c
> @@ -272,6 +272,14 @@ static int clear_epp_timeout(struct parport *pb)
>
> static irqreturn_t parport_pc_interrupt(int irq, void *dev_id)
> {
> +#ifdef CONFIG_PPS_CLIENT_LP_PARPORT_PC
> + struct parport *p = (struct parport *) dev_id;
> +
> + linuxpps_event(p->pps_source, PPS_CAPTUREASSERT, p);
Perhaps just implement empty defines for the none pps cases and get
rid of the ifdefs? But this should really be controllabe via
sysfs or such.
> --- /dev/null
> +++ b/drivers/pps/clients/Kconfig
> @@ -0,0 +1,56 @@
> +#
> +# LinuxPPS clients configuration
> +#
> +
> +if PPS
> +
> +comment "PPS clients support"
> +
> +config PPS_CLIENT_KTIMER
> + tristate "Kernel timer client (Testing client, use for debug)"
> + help
> + If you say yes here you get support for a PPS debugging client
> + which uses a kernel timer to generate the PPS signal.
> +
> + This driver can also be built as a module. If so, the module
> + will be called ktimer.o.
> +
> +comment "UART serial support (forced off)"
> + depends on SERIAL_CORE && ( PPS = m && SERIAL_CORE = y )
> +
> +config PPS_CLIENT_UART
> + bool "UART serial support"
> + depends on SERIAL_CORE && ! ( PPS = m && SERIAL_CORE = y )
help text
> +
> +comment "8250 serial support (forced off)"
> + depends on PPS_CLIENT_UART && SERIAL_8250 && \
> + ( PPS = m && SERIAL_8250 = y )
> +
> +config PPS_CLIENT_UART_8250
> + bool "8250 serial support"
> + depends on PPS_CLIENT_UART && SERIAL_8250 && \
> + ! ( PPS = m && SERIAL_8250 = y )
> + help
> + If you say yes here you get support for a PPS source connected
> + with the CD (Carrier Detect) pin of your 8250 serial line chip.
> +
> +comment "Parallel printer support (forced off)"
> + depends on PRINTER && ( PPS = m && PRINTER = y )
> +
> +config PPS_CLIENT_LP
> + bool "Parallel printer support"
> + depends on PRINTER && ! ( PPS = m && PRINTER = y )
help text
> +comment "Parport PC support (forced off)"
> + depends on PPS_CLIENT_LP && PARPORT_PC && \
> + ( PPS = m && PARPORT_PC = y )
> +
> +config PPS_CLIENT_LP_PARPORT_PC
> + bool "Parport PC support"
> + depends on PPS_CLIENT_LP && PARPORT_PC && \
> + ! ( PPS = m && PARPORT_PC = y )
> + help
> + If you say yes here you get support for a PPS source connected
> + with the interrupt pin of your PC parallel port.
help text and difference to CLIENT_LP?
> +++ b/drivers/pps/kapi.c
> +/* --- Local functions -
> */
> +
> +#ifndef NSEC_PER_SEC
> +#define NSEC_PER_SEC10
> +#endif
What's that for? Why is(n't) it defined?
> + for (i = 0; i < LINUXPPS_MAX_SOURCES; i++)
> + if (!__linuxpps_is_allocated(i))
> + break;
> + if (i >= LINUXPPS_MAX_SOURCES) {
> + err("no free source ids");
> + return -ENOMEM;
> + }
Why no dynamically allocated array?
> +void linuxpps_event(int source, int event, void *data)
> +{
> + struct timespec ts;
> +
> + /* In this function we shouldn't need locking at all since each PPS
> + * source arrives once per second and due the per-PPS source data
> + * array... */
I wouldn't bet on that.
> +++ b/drivers/pps/pps.c
> @@ -0,0 +1,377 @@
> +/*
> + * main.c -- Main driver file
Doesn't match filename
> +++ b/drivers/pps/procfs.c
I'd drop that completely.
> +++ b/include/linux/netlink.h
> @@ -21,7 +21,7 @@
> #define NETLINK_DNRTMSG 14 /* DECnet
Fwd: Re: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
For anyone elses benefit when replying to Rodolfo... - Forwarded message from [EMAIL PROTECTED] - From: [EMAIL PROTECTED] To: [EMAIL PROTECTED] Date: Fri, 16 Feb 2007 20:11:37 +0100 Subject: Re: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux You are not allowed to post to this mailing list, and your message has been automatically rejected. If you think that your messages are being rejected in error, contact the mailing list owner at [EMAIL PROTECTED] - End forwarded message - -- Russell King Linux kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Fri, Feb 16, 2007 at 07:52:30PM +0100, Rodolfo Giometti wrote:
> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> index 98ec861..cd9a003 100644
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -1315,8 +1315,25 @@ static unsigned int check_modem_status(struct
> uart_8250_port *up)
> up->port.icount.rng++;
> if (status & UART_MSR_DDSR)
> up->port.icount.dsr++;
> - if (status & UART_MSR_DDCD)
> + if (status & UART_MSR_DDCD) {
> +#ifdef CONFIG_PPS_CLIENT_UART_8250
> + if (status & UART_MSR_DCD) {
> + linuxpps_event(up->port.pps_source,
> + PPS_CAPTUREASSERT, up);
> + dbg("serial8250: PPS assert event at %lu "
> + "on source #%d",
> + jiffies, up->port.pps_source);
> + }
> + else {
> + linuxpps_event(up->port.pps_source,
> + PPS_CAPTURECLEAR, up);
> + dbg("serial8250: PPS clear event at %lu "
> + "on source #%d",
> + jiffies, up->port.pps_source);
> + }
> +#endif
Yuck. Please. No. Doing it this way means you have to modify every
single serial driver out there which is a mamouth task.
> uart_handle_dcd_change(>port, status &
> UART_MSR_DCD);
Did you not look to see what's in this helper? You'll find within here
the following code:
#ifdef CONFIG_HARD_PPS
if ((port->flags & UPF_HARDPPS_CD) && status)
hardpps();
#endif
which should've been a big sign lit up in bright lights in Times Square
pointing you towards the right place to put your code.
> + }
> if (status & UART_MSR_DCTS)
> uart_handle_cts_change(>port, status &
> UART_MSR_CTS);
>
> @@ -2004,6 +2021,9 @@ serial8250_set_termios(struct uart_port *port, struct
> ktermios *termios,
> up->ier |= UART_IER_MSI;
> if (up->capabilities & UART_CAP_UUE)
> up->ier |= UART_IER_UUE | UART_IER_RTOIE;
> +#ifdef CONFIG_PPS_CLIENT_UART_8250
> + up->ier |= UART_IER_MSI;/* enable interrupts */
> +#endif
Why not continue to leave it as a decision of the administrator - if
you want ports to default to having PPS support enabled, change all
the registration to set UPF_HARDPPS_CD. But leave the admin with
the ability to disable it.
> @@ -2124,6 +2130,33 @@ uart_configure_port(struct uart_driver *drv, struct
> uart_state *state,
>*/
> if (!uart_console(port))
> uart_change_pm(state, 3);
> +
> +#ifdef CONFIG_PPS_CLIENT_UART
> + /*
> + * Add the PPS support for the probed port.
> + */
> +snprintf(state->pps_info.name, LINUXPPS_MAX_NAME_LEN,
> + "%s%d", drv->driver_name, port->line);
> + snprintf(state->pps_info.path, LINUXPPS_MAX_NAME_LEN,
> + "/dev/%s%d", drv->dev_name, port->line);
> +
> + state->pps_info.mode = PPS_CAPTUREBOTH | \
> + PPS_OFFSETASSERT | PPS_OFFSETCLEAR | \
> + PPS_CANWAIT | PPS_TSFMT_TSPEC;
> +
> + retval = linuxpps_register_source(>pps_info,
> + PPS_CAPTUREBOTH | \
> + PPS_OFFSETASSERT | PPS_OFFSETCLEAR,
> + -1 /* PPS ID is up to the system */);
> + if (retval < 0) {
> + err("cannot register PPS source \"%s\"",
> + state->pps_info.path);
> + return;
> + }
> + port->pps_source = retval;
> + info("PPS source #%d \"%s\" added to the system ",
> + port->pps_source, state->pps_info.path);
> +#endif
This means that PPS support is not available for any port which wasn't
autoprobed at device discovery time. That seems quite restrictive.
Maybe it needs to be coupled with the setting/clearing of UPF_HARDPPS_CD ?
--
Russell King
Linux kernel2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
-
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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Fri, Feb 16, 2007 at 07:52:30PM +0100, Rodolfo Giometti wrote:
diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index 98ec861..cd9a003 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -1315,8 +1315,25 @@ static unsigned int check_modem_status(struct
uart_8250_port *up)
up-port.icount.rng++;
if (status UART_MSR_DDSR)
up-port.icount.dsr++;
- if (status UART_MSR_DDCD)
+ if (status UART_MSR_DDCD) {
+#ifdef CONFIG_PPS_CLIENT_UART_8250
+ if (status UART_MSR_DCD) {
+ linuxpps_event(up-port.pps_source,
+ PPS_CAPTUREASSERT, up);
+ dbg(serial8250: PPS assert event at %lu
+ on source #%d,
+ jiffies, up-port.pps_source);
+ }
+ else {
+ linuxpps_event(up-port.pps_source,
+ PPS_CAPTURECLEAR, up);
+ dbg(serial8250: PPS clear event at %lu
+ on source #%d,
+ jiffies, up-port.pps_source);
+ }
+#endif
Yuck. Please. No. Doing it this way means you have to modify every
single serial driver out there which is a mamouth task.
uart_handle_dcd_change(up-port, status
UART_MSR_DCD);
Did you not look to see what's in this helper? You'll find within here
the following code:
#ifdef CONFIG_HARD_PPS
if ((port-flags UPF_HARDPPS_CD) status)
hardpps();
#endif
which should've been a big sign lit up in bright lights in Times Square
pointing you towards the right place to put your code.
+ }
if (status UART_MSR_DCTS)
uart_handle_cts_change(up-port, status
UART_MSR_CTS);
@@ -2004,6 +2021,9 @@ serial8250_set_termios(struct uart_port *port, struct
ktermios *termios,
up-ier |= UART_IER_MSI;
if (up-capabilities UART_CAP_UUE)
up-ier |= UART_IER_UUE | UART_IER_RTOIE;
+#ifdef CONFIG_PPS_CLIENT_UART_8250
+ up-ier |= UART_IER_MSI;/* enable interrupts */
+#endif
Why not continue to leave it as a decision of the administrator - if
you want ports to default to having PPS support enabled, change all
the registration to set UPF_HARDPPS_CD. But leave the admin with
the ability to disable it.
@@ -2124,6 +2130,33 @@ uart_configure_port(struct uart_driver *drv, struct
uart_state *state,
*/
if (!uart_console(port))
uart_change_pm(state, 3);
+
+#ifdef CONFIG_PPS_CLIENT_UART
+ /*
+ * Add the PPS support for the probed port.
+ */
+snprintf(state-pps_info.name, LINUXPPS_MAX_NAME_LEN,
+ %s%d, drv-driver_name, port-line);
+ snprintf(state-pps_info.path, LINUXPPS_MAX_NAME_LEN,
+ /dev/%s%d, drv-dev_name, port-line);
+
+ state-pps_info.mode = PPS_CAPTUREBOTH | \
+ PPS_OFFSETASSERT | PPS_OFFSETCLEAR | \
+ PPS_CANWAIT | PPS_TSFMT_TSPEC;
+
+ retval = linuxpps_register_source(state-pps_info,
+ PPS_CAPTUREBOTH | \
+ PPS_OFFSETASSERT | PPS_OFFSETCLEAR,
+ -1 /* PPS ID is up to the system */);
+ if (retval 0) {
+ err(cannot register PPS source \%s\,
+ state-pps_info.path);
+ return;
+ }
+ port-pps_source = retval;
+ info(PPS source #%d \%s\ added to the system ,
+ port-pps_source, state-pps_info.path);
+#endif
This means that PPS support is not available for any port which wasn't
autoprobed at device discovery time. That seems quite restrictive.
Maybe it needs to be coupled with the setting/clearing of UPF_HARDPPS_CD ?
--
Russell King
Linux kernel2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
-
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/
Fwd: Re: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
For anyone elses benefit when replying to Rodolfo... - Forwarded message from [EMAIL PROTECTED] - From: [EMAIL PROTECTED] To: [EMAIL PROTECTED] Date: Fri, 16 Feb 2007 20:11:37 +0100 Subject: Re: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux You are not allowed to post to this mailing list, and your message has been automatically rejected. If you think that your messages are being rejected in error, contact the mailing list owner at [EMAIL PROTECTED] message removed - End forwarded message - -- Russell King Linux kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
Some non political comments
Rodolfo Giometti wrote:
+Coding example
+--
+
+To register a PPS source into the kernel you should define a struct
+linuxpps_source_info_s as follow:
+
+static struct linuxpps_source_info_s linuxpps_ktimer_info = {
Drop the linux prefix. It's in the linux kernel after all.
+PROCFS support
+--
New features shouldn't introduce new /proc stuff.
+Resources
+-
+
+Wiki: http://wiki.enneenne.com/index.php/LinuxPPS_support and the LinuxPPS
+ML: http://ml.enneenne.com/cgi-bin/mailman/listinfo/linuxpps.
Add to MAINTAINERS
Your way to hook into lp and 8250 is pretty gross. It should at least be
possible to deactivate it via the kernel command line, but it would be
a lot nicer to have pps_lp and pps_8250 modules which you can load. Also
what happens if you've multiple lp ports? How do you control which to
grab?
- don't implement your own dbg() stuff, use dprintk and friends
- drop the inlines, gcc will do the right thing.
--- a/drivers/char/lp.c
+++ b/drivers/char/lp.c
static struct parport_driver lp_driver = {
diff --git a/drivers/parport/parport_pc.c b/drivers/parport/parport_pc.c
index b61c17b..426b0ac 100644
--- a/drivers/parport/parport_pc.c
+++ b/drivers/parport/parport_pc.c
@@ -272,6 +272,14 @@ static int clear_epp_timeout(struct parport *pb)
static irqreturn_t parport_pc_interrupt(int irq, void *dev_id)
{
+#ifdef CONFIG_PPS_CLIENT_LP_PARPORT_PC
+ struct parport *p = (struct parport *) dev_id;
+
+ linuxpps_event(p-pps_source, PPS_CAPTUREASSERT, p);
Perhaps just implement empty defines for the none pps cases and get
rid of the ifdefs? But this should really be controllabe via
sysfs or such.
--- /dev/null
+++ b/drivers/pps/clients/Kconfig
@@ -0,0 +1,56 @@
+#
+# LinuxPPS clients configuration
+#
+
+if PPS
+
+comment PPS clients support
+
+config PPS_CLIENT_KTIMER
+ tristate Kernel timer client (Testing client, use for debug)
+ help
+ If you say yes here you get support for a PPS debugging client
+ which uses a kernel timer to generate the PPS signal.
+
+ This driver can also be built as a module. If so, the module
+ will be called ktimer.o.
+
+comment UART serial support (forced off)
+ depends on SERIAL_CORE ( PPS = m SERIAL_CORE = y )
+
+config PPS_CLIENT_UART
+ bool UART serial support
+ depends on SERIAL_CORE ! ( PPS = m SERIAL_CORE = y )
help text
+
+comment 8250 serial support (forced off)
+ depends on PPS_CLIENT_UART SERIAL_8250 \
+ ( PPS = m SERIAL_8250 = y )
+
+config PPS_CLIENT_UART_8250
+ bool 8250 serial support
+ depends on PPS_CLIENT_UART SERIAL_8250 \
+ ! ( PPS = m SERIAL_8250 = y )
+ help
+ If you say yes here you get support for a PPS source connected
+ with the CD (Carrier Detect) pin of your 8250 serial line chip.
+
+comment Parallel printer support (forced off)
+ depends on PRINTER ( PPS = m PRINTER = y )
+
+config PPS_CLIENT_LP
+ bool Parallel printer support
+ depends on PRINTER ! ( PPS = m PRINTER = y )
help text
+comment Parport PC support (forced off)
+ depends on PPS_CLIENT_LP PARPORT_PC \
+ ( PPS = m PARPORT_PC = y )
+
+config PPS_CLIENT_LP_PARPORT_PC
+ bool Parport PC support
+ depends on PPS_CLIENT_LP PARPORT_PC \
+ ! ( PPS = m PARPORT_PC = y )
+ help
+ If you say yes here you get support for a PPS source connected
+ with the interrupt pin of your PC parallel port.
help text and difference to CLIENT_LP?
+++ b/drivers/pps/kapi.c
+/* --- Local functions -
*/
+
+#ifndef NSEC_PER_SEC
+#define NSEC_PER_SEC10
+#endif
What's that for? Why is(n't) it defined?
+ for (i = 0; i LINUXPPS_MAX_SOURCES; i++)
+ if (!__linuxpps_is_allocated(i))
+ break;
+ if (i = LINUXPPS_MAX_SOURCES) {
+ err(no free source ids);
+ return -ENOMEM;
+ }
Why no dynamically allocated array?
+void linuxpps_event(int source, int event, void *data)
+{
+ struct timespec ts;
+
+ /* In this function we shouldn't need locking at all since each PPS
+ * source arrives once per second and due the per-PPS source data
+ * array... */
I wouldn't bet on that.
+++ b/drivers/pps/pps.c
@@ -0,0 +1,377 @@
+/*
+ * main.c -- Main driver file
Doesn't match filename
+++ b/drivers/pps/procfs.c
I'd drop that completely.
+++ b/include/linux/netlink.h
@@ -21,7 +21,7 @@
#define NETLINK_DNRTMSG 14 /* DECnet routing messages */
#define NETLINK_KOBJECT_UEVENT 15 /* Kernel messages to userspace
*/
#define NETLINK_GENERIC 16
-/* leave room for NETLINK_DM (DM
Re: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Fri, Feb 16, 2007 at 07:12:08PM +, Russell King wrote: Yuck. Please. No. Doing it this way means you have to modify every single serial driver out there which is a mamouth task. uart_handle_dcd_change(up-port, status UART_MSR_DCD); Did you not look to see what's in this helper? You'll find within here the following code: #ifdef CONFIG_HARD_PPS if ((port-flags UPF_HARDPPS_CD) status) hardpps(); #endif which should've been a big sign lit up in bright lights in Times Square pointing you towards the right place to put your code. Ok. Why not continue to leave it as a decision of the administrator - if you want ports to default to having PPS support enabled, change all the registration to set UPF_HARDPPS_CD. But leave the admin with the ability to disable it. Ok. This means that PPS support is not available for any port which wasn't autoprobed at device discovery time. That seems quite restrictive. How I can force probing for a specified uart port? Maybe it needs to be coupled with the setting/clearing of UPF_HARDPPS_CD ? What do you think about? I should enable the PPS support only if the userland sets the UPF_HARDPPS_CD flag? Thanks for your comments, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Fri, Feb 16, 2007 at 09:43:36PM +0100, Rodolfo Giometti wrote: On Fri, Feb 16, 2007 at 07:12:08PM +, Russell King wrote: Yuck. Please. No. Doing it this way means you have to modify every single serial driver out there which is a mamouth task. uart_handle_dcd_change(up-port, status UART_MSR_DCD); Did you not look to see what's in this helper? You'll find within here the following code: #ifdef CONFIG_HARD_PPS if ((port-flags UPF_HARDPPS_CD) status) hardpps(); #endif which should've been a big sign lit up in bright lights in Times Square pointing you towards the right place to put your code. Ok. Why not continue to leave it as a decision of the administrator - if you want ports to default to having PPS support enabled, change all the registration to set UPF_HARDPPS_CD. But leave the admin with the ability to disable it. Ok. This means that PPS support is not available for any port which wasn't autoprobed at device discovery time. That seems quite restrictive. How I can force probing for a specified uart port? You can't because it doesn't go through the interfaces you're hooking into. Existing interfaces are changed to point at the UARTs using setserial, which does its work via an ioctl. Maybe it needs to be coupled with the setting/clearing of UPF_HARDPPS_CD ? What do you think about? I should enable the PPS support only if the userland sets the UPF_HARDPPS_CD flag? Not specifically only userland - if it happens to be set when the port is registered then enable PPS support then as well. So: 1. uart_configure_port - if UPF_HARDPPS_CD is set, register the port for PPS support. 2. uart_remove_one_port - if UPF_HARDPPS_CD is set, unregister the port for PPS support. 3. uart_set_info - if changing UPF_HARDPPS_CD, appropriately register or unregister the port for PPS support. PS, [EMAIL PROTECTED] dropped from the cc: since it rejects my postings. -- Russell King Linux kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Fri, Feb 16, 2007 at 08:56:18PM +0100, Jan Dittmer wrote: Drop the linux prefix. It's in the linux kernel after all. Ok. +PROCFS support +-- New features shouldn't introduce new /proc stuff. It's a must? I can leave procfs for backward compatibility with old utilities? Add to MAINTAINERS Ok. Your way to hook into lp and 8250 is pretty gross. It should at least be possible to deactivate it via the kernel command line, but it would be a lot nicer to have pps_lp and pps_8250 modules which you can load. Also I think it's not possible... however the Russell's suggestions should go in that direction. what happens if you've multiple lp ports? How do you control which to grab? No way... I can add a specific flag as for uart lines or a kernel module parameter. - don't implement your own dbg() stuff, use dprintk and friends - drop the inlines, gcc will do the right thing. Ok. Ok. Perhaps just implement empty defines for the none pps cases and get rid of the ifdefs? But this should really be controllabe via sysfs or such. Mmm... let me think about howto implement that... help text Ok. help text and difference to CLIENT_LP? Ok. Why no dynamically allocated array? It's easier! :P Also it's very difficult having more that 3 or 4 PPS sources in a system. I wouldn't bet on that. Why not? =:-o Also locking instructions may add extra code and delay the timestamp recording... Doesn't match filename I'm going to fix it. +++ b/drivers/pps/procfs.c I'd drop that completely. :'( You read the comment above your line? No, sorry. I'm going to choose another id number... or can I keep 17? These should use dprintk and friends Ok. Isn't something like 4 more reasonable (lp + 8250 + ktimer?) It should be enought... I think you can drop the volatiles, there was a discussion some time ago that they mostly waste of words. I see... This one looks pretty fishy. After the check you normally want to use it, don't you? And then you already lost the guarantee. You are right... +#define to_class_dev(obj) container_of((obj), struct class_device, kobj) pretty generic name. I should change it? Have you thought about 32/64bit issues? No. I have no 64 bits machine to test the code... Function in .h? I'm going to check it. Thanks for your suggestions, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
On Fri, Feb 16, 2007 at 08:51:35PM +, Russell King wrote: You can't because it doesn't go through the interfaces you're hooking into. Existing interfaces are changed to point at the UARTs using setserial, which does its work via an ioctl. I see. Not specifically only userland - if it happens to be set when the port is registered then enable PPS support then as well. So: 1. uart_configure_port - if UPF_HARDPPS_CD is set, register the port for PPS support. 2. uart_remove_one_port - if UPF_HARDPPS_CD is set, unregister the port for PPS support. 3. uart_set_info - if changing UPF_HARDPPS_CD, appropriately register or unregister the port for PPS support. Ok. I'm going to study how to modify the code. PS, [EMAIL PROTECTED] dropped from the cc: since it rejects my postings. I just fixed my spam filter (at least I hope so :). Thanks, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - 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: [PATCH 1/1] LinuxPPS: Pulse per Second support for Linux
Rodolfo Giometti wrote: +PROCFS support +-- New features shouldn't introduce new /proc stuff. It's a must? I can leave procfs for backward compatibility with old utilities? Hmm, as this is a new feature with regard to the mainline kernel, old utilities don't count (if you can install a new kernel you can also be expected to install new user-space tools for the new feature). You read the comment above your line? No, sorry. I'm going to choose another id number... or can I keep 17? I don't know, ask whoever is responsible for the file. +#define to_class_dev(obj) container_of((obj), struct class_device, kobj) pretty generic name. I should change it? If it's of general use put it in the appropriate header file. If it's just for the pps subsystem name it as such. Have you thought about 32/64bit issues? No. I have no 64 bits machine to test the code... Hmm, think about x86_64 with 64-bit kernel and 32-bit userspace, probably having got different padding in the struct. Read LDD3, chapter 11, especially 11.4 . Jan - 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/
