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.
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,
....
> + 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?
> + 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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
