lkml.org 
[lkml]   [2018]   [Feb]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v6 1/2] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs
On Mon, Feb 12 2018 at 13:40 +0000, Thomas Gleixner wrote:
>On Fri, 9 Feb 2018, Lina Iyer wrote:
>> +/*
>> + * GIC does not handle falling edge or active low. To allow falling edge and
>> + * active low interrupts to be handled at GIC, PDC has an inverter that inverts
>> + * falling edge into a rising edge and active low into an active high.
>> + * For the inverter to work, the polarity bit in the IRQ_CONFIG register has to
>> + * set as per the table below.
>> + * (polarity, falling edge, rising edge ) POLARITY
>> + * 3'b0 00 Level sensitive active low LOW
>> + * 3'b0 01 Rising edge sensitive NOT USED
>> + * 3'b0 10 Falling edge sensitive LOW
>> + * 3'b0 11 Dual Edge sensitive NOT USED
>> + * 3'b1 00 Level sensitive active High HIGH
>> + * 3'b1 01 Falling Edge sensitive NOT USED
>> + * 3'b1 10 Rising edge sensitive HIGH
>> + * 3'b1 11 Dual Edge sensitive HIGH
>> + */
>> +enum pdc_irq_config_bits {
>> + PDC_POLARITY_LOW = 0,
>> + PDC_FALLING_EDGE = 2,
>> + PDC_POLARITY_HIGH = 4,
>> + PDC_RISING_EDGE = 6,
>> + PDC_DUAL_EDGE = 7,
>
>My previous comment about using binary constants still stands. Please
>either address review comments or reply at least. Ignoring reviews is not
>an option.
>
I removed them from the enum definitions. Will remove them from the
comments as well. Sorry. It was not my intention to ignore any review
comments.

>Aside of that I really have to ask about the naming of these constants. Are
>these names hardware register nomenclature? If yes, they are disgusting. If
>no, they are still disgusting, but should be changed to sensible ones,
>which just match the IRQ_TYPE naming convention.
>
> PDC_LEVEL_LOW = 000b,
> PDC_EDGE_FALLING = 010b,
> ....
>
>
They are named that way in spec :) Will change.

>> + switch (type) {
>> + case IRQ_TYPE_EDGE_RISING:
>> + pdc_type = PDC_RISING_EDGE;
>> + type = IRQ_TYPE_EDGE_RISING;
>
>Whats the point of assigning the same value again?
>
Failed to notice. Will fix.

Thanks,
Lina

>> + break;
>> + case IRQ_TYPE_EDGE_FALLING:
>> + pdc_type = PDC_FALLING_EDGE;
>> + type = IRQ_TYPE_EDGE_RISING;
>> + break;
>> + case IRQ_TYPE_EDGE_BOTH:
>> + pdc_type = PDC_DUAL_EDGE;
>> + break;
>> + case IRQ_TYPE_LEVEL_HIGH:
>> + pdc_type = PDC_POLARITY_HIGH;
>> + type = IRQ_TYPE_LEVEL_HIGH;
>
>Ditto
>
>Thanks,
>
> tglx

\
 
 \ /
  Last update: 2018-02-12 17:01    [W:0.045 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site