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

Reply via email to