Re: [PATCH v6 1/2] drivers: irqchip: pdc: Add PDC interrupt controller for QCOM SoCs

[Lina Iyer]

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