Re: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
On Fri, Jun 12, 2009 at 10:01 PM, Felipe Balbi wrote:
> On Fri, Jun 12, 2009 at 04:50:09PM +0200, ext Pandita, Vikram wrote:
>>
>>
>> >-Original Message-
>> >From: Menon, Nishanth
>> >Sent: Friday, June 12, 2009 9:46 AM
>> >To: felipe.ba...@nokia.com
>> >Cc: Pandita, Vikram; linux-omap@vger.kernel.org
>> >Subject: RE: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
>> >
>> >> -Original Message-
>> >> From: Felipe Balbi [mailto:felipe.ba...@nokia.com]
>> >> Sent: Friday, June 12, 2009 1:38 AM
>> >> To: Menon, Nishanth
>> >> Cc: Pandita, Vikram; linux-omap@vger.kernel.org
>> >> Subject: Re: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
>> >>
>> >> On Fri, Jun 12, 2009 at 03:46:37AM +0200, ext Menon, Nishanth wrote:
>> >> >
>> >> > > -Original Message-
>> >> > > From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
>> >> > > ow...@vger.kernel.org] On Behalf Of Pandita, Vikram
>> >> > > Sent: Thursday, June 11, 2009 7:50 PM
>> >> > > To: linux-omap@vger.kernel.org
>> >> > > Cc: Pandita, Vikram
>> >> > > Subject: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
>> >> > >
>> >> > >
>> >> > > + /* Get IRQ Trigger Flag */
>> >> > > + if (up->port.flags & UPF_IRQ_TRIG_RISING)
>> >> > > + irq_flags |= IRQF_TRIGGER_RISING;
>> >> > > + else if (up->port.flags & UPF_IRQ_TRIG_FALLING)
>> >> > > + irq_flags |= IRQF_TRIGGER_FALLING;
>> >> > > + else if (up->port.flags & UPF_IRQ_TRIG_HIGH)
>> >> > > + irq_flags |= IRQF_TRIGGER_HIGH;
>> >> > > + else if (up->port.flags & UPF_IRQ_TRIG_LOW)
>> >> > > + irq_flags |= IRQF_TRIGGER_LOW;
>> >> > > +
>> >> > Blame it on my dislike for nested if elseif, but...
>> >> > irq_flags |=
>> >> > (up->port.flags & UPF_IRQ_TRIG_RISING)? IRQF_TRIGGER_RISING :
>> >> > (up->port.flags & UPF_IRQ_TRIG_FALLING)? IRQF_TRIGGER_FALLING :
>> >> > (up->port.flags & IRQF_TRIGGER_HIGH)? IRQF_TRIGGER_HIGH :
>> >> > (up->port.flags & UPF_IRQ_TRIG_LOW)? IRQF_TRIGGER_LOW : 0;
>> >> >
>> >> > Makes sense?
>> >>
>> >> switch (up->port.flags & UPF_IRQ_FLAGS_MASK) {
>> >> case UPF_IRQ_TRIG_RISING:
>> >> irq_flags |= IRQF_TRIGGER_RISING;
>> >> break;
>> >> case UPF_IRQ_TRIG_FALLING:
>> >> irq_flags |= IRQF_TRIGGER_FALLING;
>> >> break;
>> >> case UPF_IRQ_TRIG_HIGH:
>> >> irq_flags |= IRQF_TRIGGER_HIGH;
>> >> break;
>> >> case UPF_IRQ_TRIG_LOW:
>> >> irq_flags |= IRQF_TRIGGER_LOW;
>> >> break;
>> >> default:
>> >> printk(KERN_ERR "Unsupported flag\n");
>> >> return -EINVAL;
>> >> }
>> >>
>> >Better :) infact, if the port.flags = UPF_IRQ_TRIG_LOW | UPF_IRQ_TRIG_HIGH
>> >it might be better to
>> >return -EINVAL, which Felipe's change does :).
>>
>> Needs more investigation as to how current boards using 8250 driver do not
>> pass any flag (maybe just SHARED flag) and they still work. Maybe some
>> default taken by request_irq()
>>
>> In short NO_ need to return failure as no flag is a valid use case.
>
> remove the default part:
>
> switch (up->port.flags & UPF_IRQ_FLAGS_MASK) {
> case UPF_IRQ_TRIG_RISING:
> irq_flags |= IRQF_TRIGGER_RISING;
> break;
How about?
irq_flags |= IRQF_TRIGGER_RISING; break;
As far as I can tell Linux code-style tries to use as less lines of
code as possible while retaining sane readability.
> case UPF_IRQ_TRIG_FALLING:
> irq_flags |= IRQF_TRIGGER_FALLING;
> break;
> case UPF_IRQ_TRIG_HIGH:
> irq_flags |= IRQF_TRIGGER_HIGH;
> break;
> case UPF_IRQ_TRIG_LOW:
> irq_flags |= IRQF_TRIGGER_LOW;
> break;
> default:
> break;
There's no need for the final break, is it?
> }
Cheers.
--
Felipe Contreras
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
>-Original Message-
>From: Felipe Balbi [mailto:felipe.ba...@nokia.com]
>Sent: Friday, June 12, 2009 2:02 PM
>To: Pandita, Vikram
>Cc: Menon, Nishanth; Balbi Felipe (Nokia-D/Helsinki);
>linux-omap@vger.kernel.org
>Subject: Re: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
>
>On Fri, Jun 12, 2009 at 04:50:09PM +0200, ext Pandita, Vikram wrote:
>>
>>
>
>remove the default part:
>
>switch (up->port.flags & UPF_IRQ_FLAGS_MASK) {
>case UPF_IRQ_TRIG_RISING:
> irq_flags |= IRQF_TRIGGER_RISING;
> break;
>case UPF_IRQ_TRIG_FALLING:
> irq_flags |= IRQF_TRIGGER_FALLING;
> break;
>case UPF_IRQ_TRIG_HIGH:
> irq_flags |= IRQF_TRIGGER_HIGH;
> break;
>case UPF_IRQ_TRIG_LOW:
> irq_flags |= IRQF_TRIGGER_LOW;
> break;
>default:
> break;
>}
Have posted the original patch to linux-serial already.
I can see your change is a replacement for if:else with switch:case
Thanks for the review. Lets see what's the response on the patch, and see when
we can put these changes in.
>
>--
>balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
On Fri, Jun 12, 2009 at 04:50:09PM +0200, ext Pandita, Vikram wrote:
>
>
> >-Original Message-
> >From: Menon, Nishanth
> >Sent: Friday, June 12, 2009 9:46 AM
> >To: felipe.ba...@nokia.com
> >Cc: Pandita, Vikram; linux-omap@vger.kernel.org
> >Subject: RE: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
> >
> >> -Original Message-
> >> From: Felipe Balbi [mailto:felipe.ba...@nokia.com]
> >> Sent: Friday, June 12, 2009 1:38 AM
> >> To: Menon, Nishanth
> >> Cc: Pandita, Vikram; linux-omap@vger.kernel.org
> >> Subject: Re: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
> >>
> >> On Fri, Jun 12, 2009 at 03:46:37AM +0200, ext Menon, Nishanth wrote:
> >> >
> >> > > -Original Message-
> >> > > From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
> >> > > ow...@vger.kernel.org] On Behalf Of Pandita, Vikram
> >> > > Sent: Thursday, June 11, 2009 7:50 PM
> >> > > To: linux-omap@vger.kernel.org
> >> > > Cc: Pandita, Vikram
> >> > > Subject: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
> >> > >
> >> > >
> >> > > + /* Get IRQ Trigger Flag */
> >> > > + if (up->port.flags & UPF_IRQ_TRIG_RISING)
> >> > > + irq_flags |= IRQF_TRIGGER_RISING;
> >> > > + else if (up->port.flags & UPF_IRQ_TRIG_FALLING)
> >> > > + irq_flags |= IRQF_TRIGGER_FALLING;
> >> > > + else if (up->port.flags & UPF_IRQ_TRIG_HIGH)
> >> > > + irq_flags |= IRQF_TRIGGER_HIGH;
> >> > > + else if (up->port.flags & UPF_IRQ_TRIG_LOW)
> >> > > + irq_flags |= IRQF_TRIGGER_LOW;
> >> > > +
> >> > Blame it on my dislike for nested if elseif, but...
> >> > irq_flags |=
> >> > (up->port.flags & UPF_IRQ_TRIG_RISING)? IRQF_TRIGGER_RISING :
> >> > (up->port.flags & UPF_IRQ_TRIG_FALLING)? IRQF_TRIGGER_FALLING :
> >> > (up->port.flags & IRQF_TRIGGER_HIGH)? IRQF_TRIGGER_HIGH :
> >> > (up->port.flags & UPF_IRQ_TRIG_LOW)? IRQF_TRIGGER_LOW : 0;
> >> >
> >> > Makes sense?
> >>
> >> switch (up->port.flags & UPF_IRQ_FLAGS_MASK) {
> >> case UPF_IRQ_TRIG_RISING:
> >>irq_flags |= IRQF_TRIGGER_RISING;
> >>break;
> >> case UPF_IRQ_TRIG_FALLING:
> >>irq_flags |= IRQF_TRIGGER_FALLING;
> >>break;
> >> case UPF_IRQ_TRIG_HIGH:
> >>irq_flags |= IRQF_TRIGGER_HIGH;
> >>break;
> >> case UPF_IRQ_TRIG_LOW:
> >>irq_flags |= IRQF_TRIGGER_LOW;
> >>break;
> >> default:
> >>printk(KERN_ERR "Unsupported flag\n");
> >>return -EINVAL;
> >> }
> >>
> >Better :) infact, if the port.flags = UPF_IRQ_TRIG_LOW | UPF_IRQ_TRIG_HIGH
> >it might be better to
> >return -EINVAL, which Felipe's change does :).
>
> Needs more investigation as to how current boards using 8250 driver do not
> pass any flag (maybe just SHARED flag) and they still work. Maybe some
> default taken by request_irq()
>
> In short NO_ need to return failure as no flag is a valid use case.
remove the default part:
switch (up->port.flags & UPF_IRQ_FLAGS_MASK) {
case UPF_IRQ_TRIG_RISING:
irq_flags |= IRQF_TRIGGER_RISING;
break;
case UPF_IRQ_TRIG_FALLING:
irq_flags |= IRQF_TRIGGER_FALLING;
break;
case UPF_IRQ_TRIG_HIGH:
irq_flags |= IRQF_TRIGGER_HIGH;
break;
case UPF_IRQ_TRIG_LOW:
irq_flags |= IRQF_TRIGGER_LOW;
break;
default:
break;
}
--
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
On Thu, 11 Jun 2009 19:53:50 -0500 Vikram Pandita wrote: > Boards with serial irq High/Low/Rising/Falling IRQ requirement > do not work today > > 8250 serial driver does not have provision to pass on IRQ flags > from platform_device > > This is requred for OMAP Zoom2 board for which Serial IRQ trigger > is IRQF_TRIGGER_HIGH > > Signed-off-by: Vikram Pandita Arcom Viper (pxa255 based) has the same kind of requirement (triggers on rising edge), and currently uses a couple of "set_irq_type" that I'd love to get rid of. So a patch like this one should definitely be considered. Best regards, M. -- We don't mess around. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
Vikram Pandita writes:
> Boards with serial irq High/Low/Rising/Falling IRQ requirement
> do not work today
>
> 8250 serial driver does not have provision to pass on IRQ flags
> from platform_device
>
> This is requred for OMAP Zoom2 board for which Serial IRQ trigger
> is IRQF_TRIGGER_HIGH
>
> Signed-off-by: Vikram Pandita
Acked-by: Kevin Hilman
Tested and verified on zoom2. This should to go linux-serial, and CC
LKML, linux-omap.
Before sending, I would update the subject/description slightly:
Subject: serial: 8250: add IRQ trigger support
Description:
There is currently no provision for passing IRQ trigger flags for
serial IRQs with triggering requirements (such as GPIO IRQs.)
This patch adds UPF_IRQ_TRIG_* flags which map on to IRQF_TRIGGER_*
flags.
> ---
> drivers/serial/8250.c | 10 ++
> include/linux/serial_core.h |4
> 2 files changed, 14 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
> index bab115e..8235ef5 100644
> --- a/drivers/serial/8250.c
> +++ b/drivers/serial/8250.c
> @@ -1641,6 +1641,16 @@ static int serial_link_irq_chain(struct uart_8250_port
> *up)
> struct irq_info *i;
> int ret, irq_flags = up->port.flags & UPF_SHARE_IRQ ? IRQF_SHARED : 0;
>
> + /* Get IRQ Trigger Flag */
> + if (up->port.flags & UPF_IRQ_TRIG_RISING)
> + irq_flags |= IRQF_TRIGGER_RISING;
> + else if (up->port.flags & UPF_IRQ_TRIG_FALLING)
> + irq_flags |= IRQF_TRIGGER_FALLING;
> + else if (up->port.flags & UPF_IRQ_TRIG_HIGH)
> + irq_flags |= IRQF_TRIGGER_HIGH;
> + else if (up->port.flags & UPF_IRQ_TRIG_LOW)
> + irq_flags |= IRQF_TRIGGER_LOW;
> +
> mutex_lock(&hash_mutex);
>
> h = &irq_lists[up->port.irq % NR_IRQ_HASH];
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 57a97e5..07591d5 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -296,7 +296,11 @@ struct uart_port {
> #define UPF_SPD_WARP ((__force upf_t) (0x1010))
> #define UPF_SKIP_TEST((__force upf_t) (1 << 6))
> #define UPF_AUTO_IRQ ((__force upf_t) (1 << 7))
> +#define UPF_IRQ_TRIG_RISING ((__force upf_t) (1 << 8))
> +#define UPF_IRQ_TRIG_FALLING ((__force upf_t) (1 << 9))
> +#define UPF_IRQ_TRIG_HIGH((__force upf_t) (1 << 10))
> #define UPF_HARDPPS_CD ((__force upf_t) (1 << 11))
> +#define UPF_IRQ_TRIG_LOW ((__force upf_t) (1 << 12))
> #define UPF_LOW_LATENCY ((__force upf_t) (1 << 13))
> #define UPF_BUGGY_UART ((__force upf_t) (1 << 14))
> #define UPF_NO_TXEN_TEST ((__force upf_t) (1 << 15))
> --
> 1.6.0.3.613.g9f8f13
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
Regards,
Nishanth Menon
> -Original Message-
> From: Pandita, Vikram
> Sent: Friday, June 12, 2009 9:50 AM
> To: Menon, Nishanth; felipe.ba...@nokia.com
> Cc: linux-omap@vger.kernel.org
> Subject: RE: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
>
>
>
> >-Original Message-
> >From: Menon, Nishanth
> >Sent: Friday, June 12, 2009 9:46 AM
> >To: felipe.ba...@nokia.com
> >Cc: Pandita, Vikram; linux-omap@vger.kernel.org
> >Subject: RE: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
> >
> >> -Original Message-
> >> From: Felipe Balbi [mailto:felipe.ba...@nokia.com]
> >> Sent: Friday, June 12, 2009 1:38 AM
> >> To: Menon, Nishanth
> >> Cc: Pandita, Vikram; linux-omap@vger.kernel.org
> >> Subject: Re: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
> >>
> >> On Fri, Jun 12, 2009 at 03:46:37AM +0200, ext Menon, Nishanth wrote:
> >> >
> >> > > -Original Message-
> >> > > From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
> >> > > ow...@vger.kernel.org] On Behalf Of Pandita, Vikram
> >> > > Sent: Thursday, June 11, 2009 7:50 PM
> >> > > To: linux-omap@vger.kernel.org
> >> > > Cc: Pandita, Vikram
> >> > > Subject: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
> >> > >
> >> > >
> >> > > + /* Get IRQ Trigger Flag */
> >> > > + if (up->port.flags & UPF_IRQ_TRIG_RISING)
> >> > > + irq_flags |= IRQF_TRIGGER_RISING;
> >> > > + else if (up->port.flags & UPF_IRQ_TRIG_FALLING)
> >> > > + irq_flags |= IRQF_TRIGGER_FALLING;
> >> > > + else if (up->port.flags & UPF_IRQ_TRIG_HIGH)
> >> > > + irq_flags |= IRQF_TRIGGER_HIGH;
> >> > > + else if (up->port.flags & UPF_IRQ_TRIG_LOW)
> >> > > + irq_flags |= IRQF_TRIGGER_LOW;
> >> > > +
> >> > Blame it on my dislike for nested if elseif, but...
> >> > irq_flags |=
> >> > (up->port.flags & UPF_IRQ_TRIG_RISING)? IRQF_TRIGGER_RISING :
> >> > (up->port.flags & UPF_IRQ_TRIG_FALLING)? IRQF_TRIGGER_FALLING :
> >> > (up->port.flags & IRQF_TRIGGER_HIGH)? IRQF_TRIGGER_HIGH :
> >> > (up->port.flags & UPF_IRQ_TRIG_LOW)? IRQF_TRIGGER_LOW : 0;
> >> >
> >> > Makes sense?
> >>
> >> switch (up->port.flags & UPF_IRQ_FLAGS_MASK) {
> >> case UPF_IRQ_TRIG_RISING:
> >>irq_flags |= IRQF_TRIGGER_RISING;
> >>break;
> >> case UPF_IRQ_TRIG_FALLING:
> >>irq_flags |= IRQF_TRIGGER_FALLING;
> >>break;
> >> case UPF_IRQ_TRIG_HIGH:
> >>irq_flags |= IRQF_TRIGGER_HIGH;
> >>break;
> >> case UPF_IRQ_TRIG_LOW:
> >>irq_flags |= IRQF_TRIGGER_LOW;
> >>break;
> >> default:
> >>printk(KERN_ERR "Unsupported flag\n");
> >>return -EINVAL;
> >> }
> >>
> >Better :) infact, if the port.flags = UPF_IRQ_TRIG_LOW |
> UPF_IRQ_TRIG_HIGH it might be better to
> >return -EINVAL, which Felipe's change does :).
>
> Needs more investigation as to how current boards using 8250 driver do not
> pass any flag (maybe just SHARED flag) and they still work. Maybe some
> default taken by request_irq()
>
> In short NO_ need to return failure as no flag is a valid use case.
Replacing switch (up->port.flags & UPF_IRQ_FLAGS_MASK)
With
#define UPF_IRQ_LEVEL_FLAGS_MASK \
(UPF_IRQ_TRIG_RISING | UPF_IRQ_TRIG_FALLING |\
UPF_IRQ_TRIG_HIGH | UPF_IRQ_TRIG_LOW)
switch (up->port.flags & UPF_IRQ_LEVEL_FLAGS_MASK) {
will allow for valid -EINVAL return.
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
>-Original Message-
>From: Menon, Nishanth
>Sent: Friday, June 12, 2009 9:46 AM
>To: felipe.ba...@nokia.com
>Cc: Pandita, Vikram; linux-omap@vger.kernel.org
>Subject: RE: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
>
>> -Original Message-
>> From: Felipe Balbi [mailto:felipe.ba...@nokia.com]
>> Sent: Friday, June 12, 2009 1:38 AM
>> To: Menon, Nishanth
>> Cc: Pandita, Vikram; linux-omap@vger.kernel.org
>> Subject: Re: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
>>
>> On Fri, Jun 12, 2009 at 03:46:37AM +0200, ext Menon, Nishanth wrote:
>> >
>> > > -Original Message-
>> > > From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
>> > > ow...@vger.kernel.org] On Behalf Of Pandita, Vikram
>> > > Sent: Thursday, June 11, 2009 7:50 PM
>> > > To: linux-omap@vger.kernel.org
>> > > Cc: Pandita, Vikram
>> > > Subject: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
>> > >
>> > >
>> > > +/* Get IRQ Trigger Flag */
>> > > +if (up->port.flags & UPF_IRQ_TRIG_RISING)
>> > > +irq_flags |= IRQF_TRIGGER_RISING;
>> > > +else if (up->port.flags & UPF_IRQ_TRIG_FALLING)
>> > > +irq_flags |= IRQF_TRIGGER_FALLING;
>> > > +else if (up->port.flags & UPF_IRQ_TRIG_HIGH)
>> > > +irq_flags |= IRQF_TRIGGER_HIGH;
>> > > +else if (up->port.flags & UPF_IRQ_TRIG_LOW)
>> > > +irq_flags |= IRQF_TRIGGER_LOW;
>> > > +
>> > Blame it on my dislike for nested if elseif, but...
>> > irq_flags |=
>> >(up->port.flags & UPF_IRQ_TRIG_RISING)? IRQF_TRIGGER_RISING :
>> >(up->port.flags & UPF_IRQ_TRIG_FALLING)? IRQF_TRIGGER_FALLING :
>> >(up->port.flags & IRQF_TRIGGER_HIGH)? IRQF_TRIGGER_HIGH :
>> >(up->port.flags & UPF_IRQ_TRIG_LOW)? IRQF_TRIGGER_LOW : 0;
>> >
>> > Makes sense?
>>
>> switch (up->port.flags & UPF_IRQ_FLAGS_MASK) {
>> case UPF_IRQ_TRIG_RISING:
>> irq_flags |= IRQF_TRIGGER_RISING;
>> break;
>> case UPF_IRQ_TRIG_FALLING:
>> irq_flags |= IRQF_TRIGGER_FALLING;
>> break;
>> case UPF_IRQ_TRIG_HIGH:
>> irq_flags |= IRQF_TRIGGER_HIGH;
>> break;
>> case UPF_IRQ_TRIG_LOW:
>> irq_flags |= IRQF_TRIGGER_LOW;
>> break;
>> default:
>> printk(KERN_ERR "Unsupported flag\n");
>> return -EINVAL;
>> }
>>
>Better :) infact, if the port.flags = UPF_IRQ_TRIG_LOW | UPF_IRQ_TRIG_HIGH it
>might be better to
>return -EINVAL, which Felipe's change does :).
Needs more investigation as to how current boards using 8250 driver do not pass
any flag (maybe just SHARED flag) and they still work. Maybe some default taken
by request_irq()
In short NO_ need to return failure as no flag is a valid use case.
>
>Regards,
>Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
> -Original Message-
> From: Felipe Balbi [mailto:felipe.ba...@nokia.com]
> Sent: Friday, June 12, 2009 1:38 AM
> To: Menon, Nishanth
> Cc: Pandita, Vikram; linux-omap@vger.kernel.org
> Subject: Re: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
>
> On Fri, Jun 12, 2009 at 03:46:37AM +0200, ext Menon, Nishanth wrote:
> >
> > > -Original Message-
> > > From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
> > > ow...@vger.kernel.org] On Behalf Of Pandita, Vikram
> > > Sent: Thursday, June 11, 2009 7:50 PM
> > > To: linux-omap@vger.kernel.org
> > > Cc: Pandita, Vikram
> > > Subject: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
> > >
> > >
> > > + /* Get IRQ Trigger Flag */
> > > + if (up->port.flags & UPF_IRQ_TRIG_RISING)
> > > + irq_flags |= IRQF_TRIGGER_RISING;
> > > + else if (up->port.flags & UPF_IRQ_TRIG_FALLING)
> > > + irq_flags |= IRQF_TRIGGER_FALLING;
> > > + else if (up->port.flags & UPF_IRQ_TRIG_HIGH)
> > > + irq_flags |= IRQF_TRIGGER_HIGH;
> > > + else if (up->port.flags & UPF_IRQ_TRIG_LOW)
> > > + irq_flags |= IRQF_TRIGGER_LOW;
> > > +
> > Blame it on my dislike for nested if elseif, but...
> > irq_flags |=
> > (up->port.flags & UPF_IRQ_TRIG_RISING)? IRQF_TRIGGER_RISING :
> > (up->port.flags & UPF_IRQ_TRIG_FALLING)? IRQF_TRIGGER_FALLING :
> > (up->port.flags & IRQF_TRIGGER_HIGH)? IRQF_TRIGGER_HIGH :
> > (up->port.flags & UPF_IRQ_TRIG_LOW)? IRQF_TRIGGER_LOW : 0;
> >
> > Makes sense?
>
> switch (up->port.flags & UPF_IRQ_FLAGS_MASK) {
> case UPF_IRQ_TRIG_RISING:
> irq_flags |= IRQF_TRIGGER_RISING;
> break;
> case UPF_IRQ_TRIG_FALLING:
> irq_flags |= IRQF_TRIGGER_FALLING;
> break;
> case UPF_IRQ_TRIG_HIGH:
> irq_flags |= IRQF_TRIGGER_HIGH;
> break;
> case UPF_IRQ_TRIG_LOW:
> irq_flags |= IRQF_TRIGGER_LOW;
> break;
> default:
> printk(KERN_ERR "Unsupported flag\n");
> return -EINVAL;
> }
>
Better :) infact, if the port.flags = UPF_IRQ_TRIG_LOW | UPF_IRQ_TRIG_HIGH it
might be better to return -EINVAL, which Felipe's change does :).
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
On Fri, Jun 12, 2009 at 03:46:37AM +0200, ext Menon, Nishanth wrote:
>
> > -Original Message-
> > From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
> > ow...@vger.kernel.org] On Behalf Of Pandita, Vikram
> > Sent: Thursday, June 11, 2009 7:50 PM
> > To: linux-omap@vger.kernel.org
> > Cc: Pandita, Vikram
> > Subject: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
> >
> >
> > + /* Get IRQ Trigger Flag */
> > + if (up->port.flags & UPF_IRQ_TRIG_RISING)
> > + irq_flags |= IRQF_TRIGGER_RISING;
> > + else if (up->port.flags & UPF_IRQ_TRIG_FALLING)
> > + irq_flags |= IRQF_TRIGGER_FALLING;
> > + else if (up->port.flags & UPF_IRQ_TRIG_HIGH)
> > + irq_flags |= IRQF_TRIGGER_HIGH;
> > + else if (up->port.flags & UPF_IRQ_TRIG_LOW)
> > + irq_flags |= IRQF_TRIGGER_LOW;
> > +
> Blame it on my dislike for nested if elseif, but...
> irq_flags |=
> (up->port.flags & UPF_IRQ_TRIG_RISING)? IRQF_TRIGGER_RISING :
> (up->port.flags & UPF_IRQ_TRIG_FALLING)? IRQF_TRIGGER_FALLING :
> (up->port.flags & IRQF_TRIGGER_HIGH)? IRQF_TRIGGER_HIGH :
> (up->port.flags & UPF_IRQ_TRIG_LOW)? IRQF_TRIGGER_LOW : 0;
>
> Makes sense?
switch (up->port.flags & UPF_IRQ_FLAGS_MASK) {
case UPF_IRQ_TRIG_RISING:
irq_flags |= IRQF_TRIGGER_RISING;
break;
case UPF_IRQ_TRIG_FALLING:
irq_flags |= IRQF_TRIGGER_FALLING;
break;
case UPF_IRQ_TRIG_HIGH:
irq_flags |= IRQF_TRIGGER_HIGH;
break;
case UPF_IRQ_TRIG_LOW:
irq_flags |= IRQF_TRIGGER_LOW;
break;
default:
printk(KERN_ERR "Unsupported flag\n");
return -EINVAL;
}
???
--
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver
> -Original Message- > From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap- > ow...@vger.kernel.org] On Behalf Of Pandita, Vikram > Sent: Thursday, June 11, 2009 7:50 PM > To: linux-omap@vger.kernel.org > Cc: Pandita, Vikram > Subject: [PATCH 1/2] Serial: Define IRQ flags for 8250 driver > > > + /* Get IRQ Trigger Flag */ > + if (up->port.flags & UPF_IRQ_TRIG_RISING) > + irq_flags |= IRQF_TRIGGER_RISING; > + else if (up->port.flags & UPF_IRQ_TRIG_FALLING) > + irq_flags |= IRQF_TRIGGER_FALLING; > + else if (up->port.flags & UPF_IRQ_TRIG_HIGH) > + irq_flags |= IRQF_TRIGGER_HIGH; > + else if (up->port.flags & UPF_IRQ_TRIG_LOW) > + irq_flags |= IRQF_TRIGGER_LOW; > + Blame it on my dislike for nested if elseif, but... irq_flags |= (up->port.flags & UPF_IRQ_TRIG_RISING)? IRQF_TRIGGER_RISING : (up->port.flags & UPF_IRQ_TRIG_FALLING)? IRQF_TRIGGER_FALLING : (up->port.flags & IRQF_TRIGGER_HIGH)? IRQF_TRIGGER_HIGH : (up->port.flags & UPF_IRQ_TRIG_LOW)? IRQF_TRIGGER_LOW : 0; Makes sense? Regards, Nishanth Menon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] Serial: Define IRQ flags for 8250 driver
Boards with serial irq High/Low/Rising/Falling IRQ requirement
do not work today
8250 serial driver does not have provision to pass on IRQ flags
from platform_device
This is requred for OMAP Zoom2 board for which Serial IRQ trigger
is IRQF_TRIGGER_HIGH
Signed-off-by: Vikram Pandita
---
drivers/serial/8250.c | 10 ++
include/linux/serial_core.h |4
2 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index bab115e..8235ef5 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -1641,6 +1641,16 @@ static int serial_link_irq_chain(struct uart_8250_port
*up)
struct irq_info *i;
int ret, irq_flags = up->port.flags & UPF_SHARE_IRQ ? IRQF_SHARED : 0;
+ /* Get IRQ Trigger Flag */
+ if (up->port.flags & UPF_IRQ_TRIG_RISING)
+ irq_flags |= IRQF_TRIGGER_RISING;
+ else if (up->port.flags & UPF_IRQ_TRIG_FALLING)
+ irq_flags |= IRQF_TRIGGER_FALLING;
+ else if (up->port.flags & UPF_IRQ_TRIG_HIGH)
+ irq_flags |= IRQF_TRIGGER_HIGH;
+ else if (up->port.flags & UPF_IRQ_TRIG_LOW)
+ irq_flags |= IRQF_TRIGGER_LOW;
+
mutex_lock(&hash_mutex);
h = &irq_lists[up->port.irq % NR_IRQ_HASH];
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 57a97e5..07591d5 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -296,7 +296,11 @@ struct uart_port {
#define UPF_SPD_WARP ((__force upf_t) (0x1010))
#define UPF_SKIP_TEST ((__force upf_t) (1 << 6))
#define UPF_AUTO_IRQ ((__force upf_t) (1 << 7))
+#define UPF_IRQ_TRIG_RISING((__force upf_t) (1 << 8))
+#define UPF_IRQ_TRIG_FALLING ((__force upf_t) (1 << 9))
+#define UPF_IRQ_TRIG_HIGH ((__force upf_t) (1 << 10))
#define UPF_HARDPPS_CD ((__force upf_t) (1 << 11))
+#define UPF_IRQ_TRIG_LOW ((__force upf_t) (1 << 12))
#define UPF_LOW_LATENCY((__force upf_t) (1 << 13))
#define UPF_BUGGY_UART ((__force upf_t) (1 << 14))
#define UPF_NO_TXEN_TEST ((__force upf_t) (1 << 15))
--
1.6.0.3.613.g9f8f13
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] Serial: Define IRQ flags for 8250 driver
Boards with serial irq High/Low/Rising/Falling IRQ requirement
do not work today
8250 serial driver does not have provision to pass on IRQ flags
from platform_device
This is requred for OMAP Zoom2 board for which Serial IRQ trigger
is IRQF_TRIGGER_HIGH
Signed-off-by: Vikram Pandita
---
drivers/serial/8250.c | 10 ++
include/linux/serial_core.h |4
2 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/drivers/serial/8250.c b/drivers/serial/8250.c
index bab115e..8235ef5 100644
--- a/drivers/serial/8250.c
+++ b/drivers/serial/8250.c
@@ -1641,6 +1641,16 @@ static int serial_link_irq_chain(struct uart_8250_port
*up)
struct irq_info *i;
int ret, irq_flags = up->port.flags & UPF_SHARE_IRQ ? IRQF_SHARED : 0;
+ /* Get IRQ Trigger Flag */
+ if (up->port.flags & UPF_IRQ_TRIG_RISING)
+ irq_flags |= IRQF_TRIGGER_RISING;
+ else if (up->port.flags & UPF_IRQ_TRIG_FALLING)
+ irq_flags |= IRQF_TRIGGER_FALLING;
+ else if (up->port.flags & UPF_IRQ_TRIG_HIGH)
+ irq_flags |= IRQF_TRIGGER_HIGH;
+ else if (up->port.flags & UPF_IRQ_TRIG_LOW)
+ irq_flags |= IRQF_TRIGGER_LOW;
+
mutex_lock(&hash_mutex);
h = &irq_lists[up->port.irq % NR_IRQ_HASH];
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index 57a97e5..07591d5 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -296,7 +296,11 @@ struct uart_port {
#define UPF_SPD_WARP ((__force upf_t) (0x1010))
#define UPF_SKIP_TEST ((__force upf_t) (1 << 6))
#define UPF_AUTO_IRQ ((__force upf_t) (1 << 7))
+#define UPF_IRQ_TRIG_RISING((__force upf_t) (1 << 8))
+#define UPF_IRQ_TRIG_FALLING ((__force upf_t) (1 << 9))
+#define UPF_IRQ_TRIG_HIGH ((__force upf_t) (1 << 10))
#define UPF_HARDPPS_CD ((__force upf_t) (1 << 11))
+#define UPF_IRQ_TRIG_LOW ((__force upf_t) (1 << 12))
#define UPF_LOW_LATENCY((__force upf_t) (1 << 13))
#define UPF_BUGGY_UART ((__force upf_t) (1 << 14))
#define UPF_NO_TXEN_TEST ((__force upf_t) (1 << 15))
--
1.6.0.3.613.g9f8f13
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
