Hi Thomas,
On Mon, Mar 19, 2018 at 02:30:42PM +0100, Thomas Gleixner wrote:
> > +static struct irq_chip ck_irq_chip = {
> > + .name = "csky_intc_v1",
> > + .irq_mask = ck_irq_mask,
> > + .irq_unmask = ck_irq_unmask,
> > +};
>
> Please use the generic interrupt chip infrastructure for this.
Ok, I will learn it.
> > +static unsigned int ck_get_irqno(void)
> > +{
> > + unsigned int temp;
>
> newline between declaration and code.
Ok
> > + temp = __raw_readl(CK_VA_INTC_ISR);
> > + return temp & 0x3f;
> > +};
> > +
> > +static int __init
> > +__intc_init(struct device_node *np, struct device_node *parent, bool ave)
>
> What is 'ave'?
Ave means the irq-controller works on auto-vector-handler mode, it will
cause 10 exception number and it need read intc-reg to get irq-num.
> No magic numbers. Please use proper defines.
Ok
> > + else
> > + __raw_writel( 0x0, CK_VA_INTC_ICR);
> > + /*
> > + * csky irq ctrl has 64 sources.
> > + */
> > + #define INTC_IRQS 64
>
> No defines in code.
Ok
>
> > + for (i=0; i<INTC_IRQS; i=i+4)
>
> checkpatch.pl would have told you what's wrong with the above
Ok, Thx
> > + __raw_writel((i+3)|((i+2)<<8)|((i+1)<<16)|(i<<24),
>
> Eew. Tons of magic numbers and a unreadable coding style. Please use an
> inline function with proper comments to calculate that value
Ok
> > + CK_VA_INTC_SOURCE + i);
> > +
> > + root_domain = irq_domain_add_legacy(np, INTC_IRQS, 0, 0, &ck_irq_ops, NULL);
> > + if (!root_domain)
> > + panic("root irq domain not available\n");
> > +
> > + irq_set_default_host(root_domain);
> > +
> > + return 0;
> > +}
> > +
> > +static int __init
> > +intc_init(struct device_node *np, struct device_node *parent)
> > +{
> > +
>
> Stray newline
I'll remove, Thx.
> > + return __intc_init(np, parent, false);
> > +}
> > +IRQCHIP_DECLARE(csky_intc_v1, "csky,intc-v1", intc_init);
> > +
> > +/*
> > + * use auto vector exceptions 10 for interrupt.
> > + */
> > +static int __init
> > +intc_init_ave(struct device_node *np, struct device_node *parent)
> > +{
> > + return __intc_init(np, parent, true);
>
> Why is that 'ave' thing not a property of device tree?
I'll change it to a property.
> > +struct irq_chip nc_irq_chip = {
> > + .name = "nationalchip_intc_v1",
> > + .irq_mask = nc_irq_mask,
> > + .irq_unmask = nc_irq_unmask,
> > + .irq_enable = nc_irq_en,
> > + .irq_disable = nc_irq_dis,
> > +};
>
> Again. This all can use the generic interrupt chip.
I'll learn the generic interrupt chip.
> > +inline int ff1_64(unsigned int hi, unsigned int lo)
>
> What on earth means ff1_64?
Find the first high bit '1' of the reg :P
> > + asm volatile(
> > + "ff1 %0\n"
> > + :"=r"(lo)
> > + :"r"(lo)
> > + :
> > + );
>
> So you want to decode the interrupt number from a bitfield. What's wrong
> with ffs()?
There is no wrong with ffs(). Ok, I will use the ffs().
> > + if( lo != 32 )
> > + result = 31-lo;
>
> Why is this subtracted?
ff1 find from high bit, so we need reverse it to get the right num.
> That code makes no sense w/o comments.
Sorry, I will add.
> > + else if( hi != 32 ) result = 31-hi + 32;
> > + else {
> > + printk("nc_get_irqno error hi:%x, lo:%x.\n", hi, lo);
> > + result = NR_IRQS;
> > + }
>
> Pleas use braces consistently.
Ok
> > +unsigned int nc_get_irqno(void)
>
> static?
Yes
> Same comments as for the other variant.
Ok
Best Regards
Guo Ren
