* Re: [PATCH] gdth update
[not found] <200409281401.i8SE1dXL006887@hera.kernel.org>
@ 2004-09-29 11:42 ` Arjan van de Ven
0 siblings, 0 replies; 6+ messages in thread
From: Arjan van de Ven @ 2004-09-29 11:42 UTC (permalink / raw)
To: Linux Kernel Mailing List; +Cc: Achim_Leubner
[-- Attachment #1: Type: text/plain, Size: 1208 bytes --]
On Tue, 2004-09-28 at 13:12, Linux Kernel Mailing List wrote:
> * IO-mapping with virt_to_bus(), gdth_readb(), gdth_writeb(), ...
> - * register_reboot_notifier() to get a notify on shutdown used
> + * register_reboot_notifier() to get a notify on shutown used
why this change ?
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,0)
> +static irqreturn_t gdth_interrupt(int irq, void *dev_id, struct pt_regs *regs);
> #else
> -static void gdth_interrupt(int irq,struct pt_regs *regs);
> +static void gdth_interrupt(int irq, void *dev_id, struct pt_regs *regs);
> #endif
this really is the wrong way to do such irq prototype compatibility in
drivers. *really*
> +static struct file_operations gdth_fops = {
> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,0)
> + .ioctl = gdth_ioctl,
> + .open = gdth_open,
> + .release = gdth_close,
> +#else
> + ioctl:gdth_ioctl,
> + open:gdth_open,
> + release:gdth_close,
> +#endif
C99 initializers work in all kernel versions since it's a property of
the C compiler not of the kernel. I wonder why you are putting this
ifdef here....
the rest of your ifdefs are generally quite fishy too unfortionately...
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] gdth update
@ 2004-09-29 14:28 Leubner, Achim
0 siblings, 0 replies; 6+ messages in thread
From: Leubner, Achim @ 2004-09-29 14:28 UTC (permalink / raw)
To: Christoph Hellwig, Jörn Engel; +Cc: arjanv, Linux Kernel Mailing List
Ok, thanks to all. I will make the proposed changes in the next version.
> -----Original Message-----
> From: Christoph Hellwig [mailto:hch@infradead.org]
> Sent: Mittwoch, 29. September 2004 16:21
> To: Jörn Engel
> Cc: Leubner, Achim; arjanv@redhat.com; Linux Kernel Mailing List
> Subject: Re: [PATCH] gdth update
>
> On Wed, Sep 29, 2004 at 03:43:01PM +0200, Jörn Engel wrote:
> > On Wed, 29 September 2004 14:15:57 +0200, Leubner, Achim wrote:
> > >
> > > > > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,0)
> > > > > +static irqreturn_t gdth_interrupt(int irq, void *dev_id, struct
> > > pt_regs *regs);
> > > > > #else
> > > > > -static void gdth_interrupt(int irq,struct pt_regs *regs);
> > > > > +static void gdth_interrupt(int irq, void *dev_id, struct pt_regs
> > > *regs);
> > > > > #endif
> > > >
> > > > this really is the wrong way to do such irq prototype compatibility in
> > > > drivers. *really*
> > > >
> > > So please tell me what the right way should be. It works without any
> > > problem.
> >
> > #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,0)
> > #define irqreturn_t void
> > #define IRQ_NONE
> > #define IRQ_HANDLED
> > #endif
>
> Actually all these are in recent 2.4.x release. So better check for
> #ifndef IRQ_HANDLED.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdth update
2004-09-29 13:43 ` Jörn Engel
@ 2004-09-29 14:21 ` Christoph Hellwig
0 siblings, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2004-09-29 14:21 UTC (permalink / raw)
To: Jörn Engel; +Cc: Leubner, Achim, arjanv, Linux Kernel Mailing List
On Wed, Sep 29, 2004 at 03:43:01PM +0200, Jörn Engel wrote:
> On Wed, 29 September 2004 14:15:57 +0200, Leubner, Achim wrote:
> >
> > > > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,0)
> > > > +static irqreturn_t gdth_interrupt(int irq, void *dev_id, struct
> > pt_regs *regs);
> > > > #else
> > > > -static void gdth_interrupt(int irq,struct pt_regs *regs);
> > > > +static void gdth_interrupt(int irq, void *dev_id, struct pt_regs
> > *regs);
> > > > #endif
> > >
> > > this really is the wrong way to do such irq prototype compatibility in
> > > drivers. *really*
> > >
> > So please tell me what the right way should be. It works without any
> > problem.
>
> #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,0)
> #define irqreturn_t void
> #define IRQ_NONE
> #define IRQ_HANDLED
> #endif
Actually all these are in recent 2.4.x release. So better check for
#ifndef IRQ_HANDLED.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdth update
2004-09-29 12:15 Leubner, Achim
2004-09-29 12:41 ` Christoph Hellwig
@ 2004-09-29 13:43 ` Jörn Engel
2004-09-29 14:21 ` Christoph Hellwig
1 sibling, 1 reply; 6+ messages in thread
From: Jörn Engel @ 2004-09-29 13:43 UTC (permalink / raw)
To: Leubner, Achim; +Cc: arjanv, Linux Kernel Mailing List
On Wed, 29 September 2004 14:15:57 +0200, Leubner, Achim wrote:
>
> > > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,0)
> > > +static irqreturn_t gdth_interrupt(int irq, void *dev_id, struct
> pt_regs *regs);
> > > #else
> > > -static void gdth_interrupt(int irq,struct pt_regs *regs);
> > > +static void gdth_interrupt(int irq, void *dev_id, struct pt_regs
> *regs);
> > > #endif
> >
> > this really is the wrong way to do such irq prototype compatibility in
> > drivers. *really*
> >
> So please tell me what the right way should be. It works without any
> problem.
#if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,0)
#define irqreturn_t void
#define IRQ_NONE
#define IRQ_HANDLED
#endif
static irqreturn_t gdth_interrupt(int irq, void *_dev, struct pt_regs *regs)
{
if (/*not for me*/)
return IRQ_NONE;
/* some work */
return IRQ_HANDLED;
}
Magically get's converted to old driver code by the macros above.
Point is that all ugly parts are confined to some header and don't
pollute the driver proper.
Jörn
--
Do not stop an army on its way home.
-- Sun Tzu
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] gdth update
2004-09-29 12:15 Leubner, Achim
@ 2004-09-29 12:41 ` Christoph Hellwig
2004-09-29 13:43 ` Jörn Engel
1 sibling, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2004-09-29 12:41 UTC (permalink / raw)
To: Leubner, Achim; +Cc: arjanv, Linux Kernel Mailing List
On Wed, Sep 29, 2004 at 02:15:57PM +0200, Leubner, Achim wrote:
> > C99 initializers work in all kernel versions since it's a property of
> > the C compiler not of the kernel. I wonder why you are putting this
> > ifdef here....
> >
> Agree. If the initializers works also fine with compiler versions in
> older distributions with the 2.4.x and 2.2.x kernels, the ifdef is
> really useless.
C99 initializes (.foo) are supported at least down to gcc 2.7
> > the rest of your ifdefs are generally quite fishy too
> unfortionately...
> >
> Could you please explain it exactly? I really want to learn what the
> problems are to correct it in the next version.
e.g. you have
#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,4,11)
MODULE_LICENSE("GPL");
#endif
much better would be to put a
#ifndef MODULE_LICENSE
#define MODULE_LICENSE(name)
#endif
into some header and use it unconditionally later on.
or you have
#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,0)
b = virt_ctr ? NUMDATA(scp->device->host)->busnum : scp->device->channel;
t = scp->device->id;
#else
b = virt_ctr ? NUMDATA(scp->host)->busnum : scp->channel;
t = scp->target;
#endif
where the 2.6 branch just works for 2.4 and 2.2 kernels aswell, so you
could get rid of the old branch completely.
In genereal always try to write to the current API and emulate it on
older releases.
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] gdth update
@ 2004-09-29 12:15 Leubner, Achim
2004-09-29 12:41 ` Christoph Hellwig
2004-09-29 13:43 ` Jörn Engel
0 siblings, 2 replies; 6+ messages in thread
From: Leubner, Achim @ 2004-09-29 12:15 UTC (permalink / raw)
To: arjanv, Linux Kernel Mailing List
> On Tue, 2004-09-28 at 13:12, Linux Kernel Mailing List wrote:
> > * IO-mapping with virt_to_bus(), gdth_readb(), gdth_writeb(), ...
> > - * register_reboot_notifier() to get a notify on shutdown used
> > + * register_reboot_notifier() to get a notify on shutown used
>
> why this change ?
>
OK, my fault.
> > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,0)
> > +static irqreturn_t gdth_interrupt(int irq, void *dev_id, struct
pt_regs *regs);
> > #else
> > -static void gdth_interrupt(int irq,struct pt_regs *regs);
> > +static void gdth_interrupt(int irq, void *dev_id, struct pt_regs
*regs);
> > #endif
>
> this really is the wrong way to do such irq prototype compatibility in
> drivers. *really*
>
So please tell me what the right way should be. It works without any
problem.
> > +static struct file_operations gdth_fops = {
> > +#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,0)
> > + .ioctl = gdth_ioctl,
> > + .open = gdth_open,
> > + .release = gdth_close,
> > +#else
> > + ioctl:gdth_ioctl,
> > + open:gdth_open,
> > + release:gdth_close,
> > +#endif
>
> C99 initializers work in all kernel versions since it's a property of
> the C compiler not of the kernel. I wonder why you are putting this
> ifdef here....
>
Agree. If the initializers works also fine with compiler versions in
older distributions with the 2.4.x and 2.2.x kernels, the ifdef is
really useless.
> the rest of your ifdefs are generally quite fishy too
unfortionately...
>
Could you please explain it exactly? I really want to learn what the
problems are to correct it in the next version.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2004-09-29 14:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <200409281401.i8SE1dXL006887@hera.kernel.org>
2004-09-29 11:42 ` [PATCH] gdth update Arjan van de Ven
2004-09-29 12:15 Leubner, Achim
2004-09-29 12:41 ` Christoph Hellwig
2004-09-29 13:43 ` Jörn Engel
2004-09-29 14:21 ` Christoph Hellwig
2004-09-29 14:28 Leubner, Achim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®